Subject: Re: [PATCH 09/14] Use the asyncronous function infrastructure in builtin-fetch-pack.c.

From: "Shawn O. Pearce" <[email-masked]>
Date: Fri, 19 Oct 2007 22:53:52 -0400

I've gone through this entire series and am quite happy with it.
Except one bug in this particular patch.

Johannes Sixt <[email-masked]> wrote:
> We run the sideband demultiplexer in an asynchronous function.
...
> diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
> index 871b704..51d8a32 100644
> --- a/builtin-fetch-pack.c
> +++ b/builtin-fetch-pack.c
> @@ -457,42 +457,37 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
>  	return retval;
>  }
>  
> -static pid_t setup_sideband(int fd[2], int xd[2])
> +static int sideband_demux(int fd, void *data)
>  {
> -	pid_t side_pid;
> +	int *xd = data;
>  
> +	close(xd[1]);

If this is a threaded start_async() system this close is going
to impact the caller.

>  	close(xd[0]);
> -	close(fd[1]);
> +	fd[0] = demux->out;
>  	fd[1] = xd[1];

Which is relying on xd[1] right here in the caller.  Therefore you
cannot actually use this code with a start_async() implementation
that isn't fork() based.  Isn't that going to cause you trouble in
the msysGit tree?

-- 
Shawn.