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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2009, 23:37 UTC
Message-ID: <7vljhd597h.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1260278177-9029-6-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

Show 10 quoted lines

> diff --git a/transport-helper.c b/transport-helper.c
> index 0e82553..3b7340c 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -22,6 +22,7 @@ struct helper_data
>  	/* These go from remote name (as in "list") to private name */
>  	struct refspec *refspecs;
>  	int refspec_nr;
> +	struct git_transport_options gitoptions;
>  };

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.

Show 15 quoted lines

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

The call sequence, after [6/8] is applied as I understand it is:

    - _process_connect()
      - get_helper()
        - start_command() that gives a pipe to read from the helper in
          helper->out;
        - the above dup dance makes (FILE *)data->out out of a copied fd;
        - read from data->out, potentially reading a lot more than
          the loop consumes into the stdio buffer of data->out;
      - dup dance again to make (FILE *)input;
      - read from input, unbuffered.

But if "capabilities" exchange has read past the capability response from the helper into helper->out inside get_helper(), wouldn't it make the dup dance to make an extra "input" to read the rest unbuffered moot, as "input" won't be even reading from the beginning of "the rest"?