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

[Jeff King <peff@peff.net>]

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