On Wed, May 31, 2017 at 6:23 AM, Jeff King <peff@xxxxxxxx> 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