On Mon, Jun 5, 2017 at 10:18 AM, Jeff King <peff@xxxxxxxx> 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 :)