Re: [PATCH 0/14] fork/exec removal series

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 14, 2007, 02:11 UTC
Message-ID: <20071014021149.GO27899@spearce.org>
In-Reply-To: <1192305984-22594-1-git-send-email-johannes.sixt@telecom.at>

Johannes Sixt <johannes.sixt@telecom.at> wrote:

> here is a series of patches that removes a number fork/exec pairs.

...

Show 11 quoted lines

> 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.

>  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.