Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Wed, May 31, 2017 at 6:23 AM, Jeff King <peff@peff.net> wrote:
> On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:
>
>> diff --git a/remote.c b/remote.c
>> index ad6c5424e..b8fd09dc9 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>>       return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>>  }
>>
>> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
>> +{
>> +     struct refspec *rs;
>> +
>> +     add_fetch_refspec(remote, refspec);
>> +     rs = parse_fetch_refspec(1, &refspec);
>> +     REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
>> +     remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
>> +
>> +     /* Not free_refspecs(), as we copied its pointers above */
>> +     free(rs);
>> +}
>
> What happens here if remote->fetch isn't already initialized? I think
> we'd end up with a bunch of garbage values. That's what I was trying to
> protect against in my original suggestion.
>
> I'm not sure if that's possible or not. We seem to initialize it in both
> remote_get() and for_each_remote(), and I don't think there are any
> other ways to get a remote.

The only place creating remotes is remote.c:make_remote(), which
calloc()s the required memory, making all of struct remote's fields
zero-initialized.  In case of clone the common case is that the user
doesn't specify any additional fetch refspecs, so remote->fetch will
still be NULL after full initialization and when
add_and_parse_fetch_refspec() is called with the default fetch
refspec, meaning we can't 'if (remote->fetch) { parse ... }'.  OTOH,
all functions involved can cope with the fetch-refspec-related fields
being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for
array indexing it's not 0 anymore.

> (In fact, I kind of wondered why we do this
> parsing lazily at all, but I think it's so that misconfigured remotes
> don't cause us to die() if we aren't accessing them at all).
>
> If we assume that the contract that remote.c provides is that the
> fetch fields are always filled in before a "struct remote" is returned
> to a caller, and that only such callers would use this function, it's
> OK. It just seems more dangerous than it needs to be.
>
> -Peff