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

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

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