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

[Jeff King <peff@peff.net>]

On Wed, May 31, 2017 at 11:34:23AM +0200, SZEDER Gábor wrote:

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

Yeah, I agree it is safe now. I'm just worried about some function in
remote.c later doing:

   read_config();
   add_and_parse_fetch_refspec(remotes[0], whatever);

which leaves the struct in an inconsistent state (we realloc NULL which
allocates from scratch, and all of the other entries in remote->fetch
end up uninitialized).  Can we at least add an assertion like:

  if (!remote->fetch)
	BUG("cannot add refspec to an unparsed remote");

?

-Peff