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

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

On Mon, Jun 5, 2017 at 10:18 AM, Jeff King <peff@peff.net> wrote:
> 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");
>
> ?

But as mentioned before, remote->fetch being NULL is not a bug in
itself, it's a perfectly valid value even in a fully parsed remote
when the remote has no fetch refspecs.
Therefore, I think, the condition should instead be:

  remote->fetch_refspec_nr && !remote->fetch

We could even try to be extra helpful by checking this condition and
calling parse_fetch_refspec() to initialize remote->fetch instead of
BUG()ing out.  However, that would mask the real issue, namely not
using remote_get() to get the remote, so I don't actually think that's
a good thing to do.

OTOH, having remote->fetch so closely related to, yet separate from
remote->fetch_refspec{,_nr,_alloc} will always inherently be error
prone.  This assertion would catch one case where a less than careful
dev could cause trouble, sure, but there will be still others left,
e.g. he could still do:

  add_fetch_refspec(remote, ...);    // this doesn't update remote->fetch
  for (i = 0; i < remote->fetch_refspec_nr; i++)
        func(remote->fetch[i]);

and watch the array indexing blow up in the last iteration.

Or a non-hypothetical one: when I first tried to use remote_get() for
an earlier version of this patch, I ALLOC_GROW()-ed remote->fetch to
create room for the default refspec, because in struct remote not
**fetch_refspec but *fetch is listed right above
fetch_refspec_{nr,alloc}, being way past my bedtime may be my only
excuse...  It didn't work :) [1]

To put your worries to rest we should eliminate remote->fetch_refspec
altogether and parse refspecs into remote->fetch right away, I'd
think.  After all, that's what's used in most places anyway, and it
can be easily turned back to a single string where needed (I think in
only 3 places in builtin/remote.c).


[1] - Though in the end this could be considered beneficial, because
      commits 53c5de29 (pickaxe: fix segfault with '-S<...>
      --pickaxe-regex', 2017-03-18), 59210dd56 (tests: make the
      'test_pause' helper work in non-verbose mode, 2017-03-18), and
      4ecae3c8c (tests: create an interactive gdb session with the
      'debug' helper, 2017-03-18) were all fallouts from the ensuing
      debugging session :)