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

From: Jeff King <peff@peff.net>
Date: Jun 14, 2017, 09:50 UTC
Message-ID: <20170614095023.nzu535pv6cluzdz7@sigill.intra.peff.net>
In-Reply-To: <20170614004816.GR133952@aiede.mtv.corp.google.com>

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

Show 11 quoted lines

> > 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.

Show 7 quoted lines

> > +	/* 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