From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Date: Mon, 07 Dec 2009 21:19:08 GMT
Subject: Re: [RFC PATCH v3 5/8] Support taking over transports
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:
> 
> > +	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. 
 
> 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