On Tue, Jun 06, 2017 at 08:19:09PM +0200, SZEDER Gábor wrote:
> > 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
Right, that would be a better check.
> 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.
OK.
> 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).
I don't think we can parse right away without regressing the error
handling. If I have two remotes, one with a bogus refspec, like:
[remote "one"]
url = ...
fetch = refs/heads/*:refs/remotes/one/*
[remote "two"]
url = ...
fetch = ***bogus***
and I do:
git fetch one
then read_config() will grab the data for _both_ of them, but only call
remote_get() on the first one. If we parsed the refspecs during
read_config(), we'd parse the bogus remote.two.fetch and die().
I guess that's a minor case, but as far as I can tell that's the
motivation for the lazy parsing.
-Peff