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

[Ilari Liusvaara <ilari.liusvaara@elisanet.fi>]

On Tue, Dec 08, 2009 at 03:37:06PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> Will we ever have another 'xxxoptions' field in this structure that is not
> so git-ish?  'gitoptions' somehow doesn't feel right, unless you plan to
> later add scm specific options like 'hgoptions', 'bzroptions' in this
> field you need to distinguish "git" one from them.
> 
> I know you needed to name the new field to store the transport option
> under a different name from the existing 'option' field, but for that
> purpose, 'transport_options' might be a more appropriate name.

Changed.
 
> > @@ -109,9 +111,19 @@ static struct child_process *get_helper(struct transport *transport)
> >  		die("Unable to run helper: git %s", helper->argv[0]);
> >  	data->helper = helper;
> >  
> > +	/*
> > +	 * Open the output as FILE* so strbuf_getline() can be used.
> > +	 * Do this with duped fd because fclose() will close the fd,
> > +	 * and stuff like taking over will require the fd to remain.
> > +	 *
> > +	 */
> > +	duped = dup(helper->out);
> > +	if (duped < 0)
> > +		die_errno("Can't dup helper output fd");
> > +	data->out = xfdopen(duped, "r");
> > +
> 
> Neat hack (the kind I often love), but it makes something deep inside me
> cringe.  This looks fragile and possibly wrong.
> 
> Who guarantees that reading from the (FILE *)data->out by strbuf_getline()
> that eventually calls into fgetc() does not cause more data be read in the
> buffer assiciated with the (FILE *) than we will consume?  The other FILE*
> you will make out of helper->out won't be able to read what data->out has
> already slurped in to its stdio buffer.

Causality impiles this can happen only if buffered version gets its buffer
filled after sending connect command. And looking at stdio operations that
can occur after sending the command:

- fprintfs on stderr (debug mode only).
- fgetc()s on unbuffered version.

-Ilari