Ilari Liusvaara <ilari.liusvaara@xxxxxxxxxxx> writes:
> 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.
> @@ -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"?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html