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

[Jeff King <peff@peff.net>]

On Tue, Jun 13, 2017 at 05:48:16PM -0700, Jonathan Nieder wrote:

> > The reason for this is that the initial fetch is not a fully fledged
> > 'git fetch' but a bunch of direct calls into the fetch/transport
> > machinery with clone's own refs-to-refspec matching logic, which
> > bypasses parts of 'git fetch' processing configured fetch refspecs.
> 
> Agh, subtle.
> 
> I'm hoping that longer term we can make fetch behave more like a
> library and make the initial fetch into a fully fledged 'git fetch'
> like thing again.  But this smaller change is the logical fix in the
> meantime.

We talked about this earlier in the thread, but I doubt that will ever
happen. Things like --single-branch means that clone has to actually
look at what the remote has before it decides what to fetch.

Of course we _could_ teach all that logic to fetch, too, but I don't
think you'll ever get the nice simple:

  1. configure refspecs
  2. fetch
  3. checkout

So the best thing is probably to push any repeated logic down into the
transport layer, where it can be easily used by both commands.

> > +	/* Not free_refspecs(), as we copied its pointers above */
> > +	free(rs);
> 
> Allocating an array to put the parsed refspec in and then freeing it
> seems wasteful.  Should parse_refspec_internal be changed to take an
> output parameter so it can put the refspec into remote->fetch
> directly?

It's not quite trivial to make that change. parse_refspec_internal()
actually handles an array of refspecs. So its callers would all have to
start allocating the correctly-sized array.

I doubt that one malloc per clone is worth caring about. I'd worry more
about the trickiness that merited the comment above, but it's at least
contained in this one function.

-Peff