From: Shawn O. Pearce <spearce@spearce.org>
Date: Sat, 20 Oct 2007 02:53:52 GMT
Subject: Re: [PATCH 09/14] Use the asyncronous function infrastructure in builtin-fetch-pack.c.
Message-ID: <20071020025352.GA6569@spearce.org>
In-Reply-To: <1192823286-9654-10-git-send-email-johannes.sixt@telecom.at>

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

Johannes Sixt <johannes.sixt@telecom.at> 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.