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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jun 6, 2017, 18:19 UTC
Message-ID: <CAM0VKjngnRv6iAozvhY_c61CyWhQP2khcr0bs1=7G_-MDNu4kg@mail.gmail.com>
In-Reply-To: <20170605081845.tvzidc5nblbnuner@sigill.intra.peff.net>

On Mon, Jun 5, 2017 at 10:18 AM, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

> 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 :)