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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Oct 20, 2007, 02:53 UTC
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.

...

Show 15 quoted lines

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