Johannes Sixt <johannes.sixt@telecom.at> wrote:
quoted text
> here is a series of patches that removes a number fork/exec pairs.
...
quoted text
> The series consists of 2 parts:
> 
> - The first half replaces a number of fork/exec pairs by start_command/
>   finish_command or run_command.
> 
> - The second half introduces a new framework that runs a function
>   asynchronously. New functions start_async and finish_async are implemented
>   similarly to start_command and run_command. They are used to replace
>   occurrences of fork() that does not exec() in the child. Such code
>   could in principle be run in a thread, and on MinGW port we will go this
>   route, but on Posix we stay with fork().

This series looks pretty good to me.  I like seeing huge blocks
go away only to be replaced with the simple API offered by
run-command.h.  Makes the result much easier to follow.

The async interface is also quite simple.  Unfortunately there
is some risk with the canonical fork() implementation in that the
async routine might attempt to alter global data that the parent
is also using, and folks on a good UNIX that is using the fork()
implementation will not even notice as they are in totally separated
address spaces.  But you'll see it in MSYS Git.

Since builtin-pack-objects now accepts (limited) pthread support,
perhaps this should be implemented in terms of pthread support
when pthreads are available?  Most Linux/BSD/Mac OS X systems do
have pthreads these days and that's the majority of git users and
developers.  This would make it more likely that bugs in this sort
of code would be detected early.  Just a thought.
 
quoted text
>  13 files changed, 334 insertions(+), 369 deletions(-)

Hard to argue with that final state.  You killed 35 lines and
also made Git easier to port to "that OS unfortunately named after
transparent glass thingies".

-- 
Shawn.
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html