Re: git-clone --config order & fetching extra refs during initial clone

On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote:

quoted

quoted

 static struct ref *wanted_peer_refs(const struct ref *refs,
-             struct refspec *refspec)
+             struct refspec *refspec, unsigned int refspec_count)

Most of the changes here and elsewhere are just about passing along
multiple refspecs instead of a single, which makes sense.

The new parameter should perhaps be renamed to 'refspec_nr', though,
as '_nr' suffix seems to be more common in the codebase than '_count'.

Yeah, agreed.

quoted

Though if I'm bikeshedding, I'd probably have written the whole thing
with an argv_array and avoided counting at all.

Yeah, I did notice that you love argv_array :)  I had to raise an
eyebrow recently while looking at send-pack and how it uses argv_array
to read refspecs from stdin into an array.  I think argv_array feels a
bit out of place in both cases.  Yes, it does exactly what's needed.
However, it's called *argv*_array, indicating that its contents is
destined to become the options of some command.  But that's not the
case in these two cases, we are not dealing with arguments to a
command, these are just arrays of strings.

In my mind, "argv" is synonymous with "NULL-terminated array of
strings". If the name is the only thing keeping it from wider use, I'd
much prefer us to give it a more generic name. All I really care about
is simplifying memory management. :)

However, leveraging get_remote() makes it moot anyway.

Even better.

quoted

I do also notice that right _after_ this parsing, we use remote_get(),
which is supposed to give us this config anyway. Which makes me wonder
if we could just reorder this to put remote_get() first, and then read
the resulting refspecs from remote->fetch.

Though get_remote() does give us this config, at this point the
default fetch refspec has not been configured yet, therefore it's not
included in the resulting remote->fetch array.  The default refspec is
set in write_refspec_config(), but that's called only about two
screenfulls later.  So there is a bit of extra work to do in order to
leverage get_remote()'s parsing.

I think the simplest way is to keep parsing the default fetch refspec
manually, and then append it to the remote->fetch array.  Definitely
shorter and simpler than that parsing in the current patch.
Alternatively, we could set the default fetch refspec in the
configuration temporarily, only for the duration of the get_remote()
call, but it feels a bit too hackish...

Yeah, I think manually combining the two here is fine. Though I won't
complain if you want to look into setting the config earlier. If the
refactoring isn't too bad, it would probably provide the nicest outcome.

However, the tests should also check that refs matching the default
fetch refspec are transferred, too, i.e. that the clone has something
under refs/remotes/origin/ as well.  Case in point is using the result
of get_remote(): at first I naively set out to use remote->fetch
as-is, which didn't include the default fetch refspec, hence didn't
fetch refs/heads/master, but the test succeeded nonetheless.

Good point.

quoted

If we wanted to be thorough, we could also check that the feature works
correctly with "--origin" (I think it does).

I think it works, but I'm not quite sure what you mean with "works
correctly with --origin".

So just to be clear: the behaviour depends on whether the remote name
given in '-c remote.<name>.fetch=<refspec>' matches the name given to
the '--origin=<name>'.  If they match, then refs matching the
additional refspec are transferred, too.  That's good.  However, if
the two remote names don't match, then refs matching the additional
refspec are NOT transferred.  I think this is good, too, because only
the origin remote should be cloned, whatever it is called, and in this
case that additional refspec belongs to a different remote.

Yes, exactly. Mostly I was suggesting that if you do "--origin=foo",
then we do not fetch items named "remote.origin.fetch" (i.e., that the
code correctly uses the origin variable and not the hard-coded name
"origin").

-Peff