Re: [RFC PATCH v3 5/8] Support taking over transports

From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Date: Dec 7, 2009, 21:19 UTC
Message-ID: <20091207211907.GA3916@Knoppix>
In-Reply-To: <20091207174947.GF17173@spearce.org>

On Mon, Dec 07, 2009 at 09:49:47AM -0800, Shawn O. Pearce wrote:

Show 10 quoted lines

> 
> > +	if (duped < 0)
> > +		die_errno("Can't dup helper output fd");
> > +	data->out = xfdopen(duped, "r");
> > +	setvbuf(data->out, NULL, _IONBF, 0);
> 
> I wonder if this is really a good idea.  Most helpers actually
> use a lot of text based IO to communicate.  Disabling buffering
> for those helpers to avoid overreading the advertisement from a
> connect is a problem.

I dropped the buffering change.

Show 10 quoted lines

> Maybe we could leave buffering on, but use a handshake protocol
> with the helper during connect:
> 
>   (1) > "connect git-upload-pack\n"
>   (2) < "\n"
>   (3) > "begin\n"
> 
> During 2 we are still buffered, but the only content on the pipe
> should be the single blank line, so we pull that in and the FILE*
> buffer should be empty.

Doesn't work. Stream buffering can only be changed before first
I/O.
 
I figured out a solution. Dup the file descriptor second time and
make another FILE* for it. Then send the connect and read the
response over the new stream. This avoids overreading and allows
other I/O to be buffered. After connect attempt, the new streaam
can be closed.

This way most of the I/O can be buffered, only reading responses to connect commands can't, and that is at most 1 byte if fallbacks aren't required. And the thing fits nicely inside _process_connect(). Oh, and no protocol change needed.

-Ilari