clone: respect configured fetch respecs during initial fetch

The initial fetch during a clone doesn't transfer refs matching additional fetch refspecs given on the command line as configuration variables, e.g. '-c remote.origin.fetch=<refspec>'. This contradicts the documentation stating that configuration variables specified via 'git clone -c <key>=<value> ...' "take effect immediately after the repository is initialized, but before the remote history is fetched" and the given example specifically mentions "adding additional fetch refspecs to the origin remote".

The second patch in this series makes it work. The other two are
while-at-its: the first is a little cleanup, and the last one documents other known ignored configuration variables (but whose functionality is available through command line options).

This patch series should have been marked as v6, but I chose to reset the counter, because:

- v5 was sent out way over a year ago [1], and surely everybody has forgotten about it since then anyway. But more importantly:

- A lot has happened since then, most notably we now have a refspec API, which makes this patch series much simpler (now it only touches 'builtin/clone.c', the previous version had to add stuff to 'remote.{c,h}' as well).

[1] For reference, though I actually doubt it's worth looking up:

    https://public-inbox.org/git/[email protected]/T/#u

SZEDER Gábor (3):
clone: use a more appropriate variable name for the default refspec
clone: respect additional configured fetch refspecs during initial
fetch
Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  6 +++++
 builtin/clone.c             | 33 +++++++++++++++-----------
 t/t5611-clone-config.sh     | 47 +++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 14 deletions(-)

2.19.1.1182.gbfcc7ed3e6

cmd_clone() declares two strbufs 'key' and 'value' on the same line, suggesting that they are used to contruct a config variable's name and value. However, this is not the case: 'key' is used to construct the names of multiple config variables, while 'value' is never used as a value for any of those config variables, or for any other config variable for that matter, but only to contruct the default fetch refspec.

Let's rename 'value' to 'default_refspec' to make the intent clearer.

---
 builtin/clone.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index 15b142d646..ae1bf242c6 100644
@@ -890,7 +890,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	const struct ref *our_head_points_at;
 	struct ref *mapped_refs;
 	const struct ref *ref;
-	struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
+	struct strbuf key = STRBUF_INIT;
+	struct strbuf default_refspec = STRBUF_INIT;
 	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
 	struct transport *transport = NULL;
 	const char *src_ref_prefix = "refs/heads/";

@@ -1067,7 +1068,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
 	}
 
-	strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
 	strbuf_addf(&key, "remote.%s.url", option_origin);
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);

@@ -1081,9 +1081,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_required_reference.nr || option_optional_reference.nr)
 		setup_reference();
 
-	refspec_append(&rs, value.buf);
-
-	strbuf_reset(&value);
+	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
+		    branch_top.buf);
+	refspec_append(&rs, default_refspec.buf);
 
 	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);

@@ -1240,7 +1240,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);
-	strbuf_release(&value);
+	strbuf_release(&default_refspec);
 	junk_mode = JUNK_LEAVE_ALL;
 
 	refspec_clear(&rs);

2.19.1.1182.gbfcc7ed3e6

Yep, this is much nicer.

-Peff

The initial fetch during a clone doesn't transfer refs matching additional fetch refspecs given on the command line as configuration variables, e.g. '-c remote.origin.fetch=<refspec>'. This contradicts the documentation stating that configuration variables specified via 'git clone -c <key>=<value> ...' "take effect immediately after the repository is initialized, but before the remote history is fetched" and the given example specifically mentions "adding additional fetch refspecs to the origin remote". Furthermore, one-shot configuration variables specified via 'git -c <key>=<value> clone ...', though not written to the newly created repository's config file, live during the lifetime of the 'clone' command, including the initial fetch. All this implies that any fetch refspecs specified this way should already be taken into account during the initial fetch.

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. This logic only considers a single default refspec, potentially influenced by options like '--single-branch' and '--mirror'. The configured refspecs are, however, already read and parsed properly when clone calls remote.c:remote_get(), but it never looks at the parsed refspecs in the resulting 'struct remote'.

Modify clone to take the remote's configured fetch refspecs into account to retrieve all matching refs during the initial fetch. Note that we have to explicitly add the default fetch refspec to the remote's refspecs, because at that point the remote only includes the fetch refspecs specified on the command line.

Add tests to check that refspecs given both via 'git clone -c ...' and 'git -c ... clone' retrieve all refs matching either the default or the additional refspecs, and that it works even when the user specifies an alternative remote name via '--origin=<name>'.

---
 builtin/clone.c         | 25 +++++++++++++---------
 t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 10 deletions(-)

index ae1bf242c6..7c7f98c72c 100644
@@ -548,7 +548,7 @@ static struct ref *find_remote_branch(const struct ref *refs, const char *branch
 }
 
 static struct ref *wanted_peer_refs(const struct ref *refs,
-		struct refspec_item *refspec)
+		struct refspec *refspec)
 {
 	struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
 	struct ref *local_refs = head;

@@ -569,13 +569,19 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
 			warning(_("Could not find remote branch %s to clone."),
 				option_branch);
 		else {
-			get_fetch_map(remote_head, refspec, &tail, 0);
+			int i;
+			for (i = 0; i < refspec->nr; i++)
+				get_fetch_map(remote_head, &refspec->items[i],
+					      &tail, 0);
 
 			/* if --branch=tag, pull the requested tag explicitly */
 			get_fetch_map(remote_head, tag_refspec, &tail, 0);
 		}
-	} else
-		get_fetch_map(refs, refspec, &tail, 0);
+	} else {
+		int i;
+		for (i = 0; i < refspec->nr; i++)
+			get_fetch_map(refs, &refspec->items[i], &tail, 0);
+	}
 
 	if (!option_mirror && !option_single_branch && !option_no_tags)
 		get_fetch_map(refs, tag_refspec, &tail, 0);

@@ -899,7 +905,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
 
-	struct refspec rs = REFSPEC_INIT_FETCH;
 	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
 	fetch_if_missing = 0;

@@ -1081,11 +1086,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_required_reference.nr || option_optional_reference.nr)
 		setup_reference();
 
+	remote = remote_get(option_origin);
+
 	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
 		    branch_top.buf);
-	refspec_append(&rs, default_refspec.buf);
+	refspec_append(&remote->fetch, default_refspec.buf);
 
-	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
 	transport->family = family;

@@ -1140,7 +1146,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 
 	argv_array_push(&ref_prefixes, "HEAD");
-	refspec_ref_prefixes(&rs, &ref_prefixes);
+	refspec_ref_prefixes(&remote->fetch, &ref_prefixes);
 	if (option_branch)
 		expand_ref_prefix(&ref_prefixes, option_branch);
 	if (!option_no_tags)

@@ -1149,7 +1155,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refs = transport_get_remote_refs(transport, &ref_prefixes);
 
 	if (refs) {
-		mapped_refs = wanted_peer_refs(refs, &rs.items[0]);
+		mapped_refs = wanted_peer_refs(refs, &remote->fetch);
 		/*
 		 * transport_get_remote_refs() may return refs with null sha-1
 		 * in mapped_refs (see struct transport->get_refs_list

@@ -1243,7 +1249,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&default_refspec);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	refspec_clear(&rs);
 	argv_array_clear(&ref_prefixes);
 	return err;
 }

2.19.1.1182.gbfcc7ed3e6

Nicely explained.

Good. This is all sufficiently subtle that we definitely want to check the related cases.

Most of the rest of the patch is just swapping out "rs" for "remote->fetch", which makes sense. So the implementation looks good to me.

Due to limitations in the current implementation, some configuration variables specified via 'git clone -c var=val' (or 'git -c var=val clone') are ignored during the initial fetch and checkout.

Let the users know which configuration variables are known to be ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the documentation of 'git clone -c', along with hints to use the options '--mirror' and '--no-tags' instead.

---
 Documentation/git-clone.txt | 6 ++++++
 1 file changed, 6 insertions(+)

index a55536f0bf..2fd12524f9 100644
@@ -189,6 +189,12 @@ objects from the source repository into a pack in the cloned repository.
 	values are given for the same key, each value will be written to
 	the config file. This makes it safe, for example, to add
 	additional fetch refspecs to the origin remote.
++
+Due to limitations of the current implementation, some configuration
+variables do not take effect until after the initial fetch and checkout.
+Configuration variables known to not take effect are:
+`remote.<name>.mirror` and `remote.<name>.tagOpt`.  Use the
+corresponding `--mirror` and `--no-tags` options instead.
 
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the

2.19.1.1182.gbfcc7ed3e6

Good idea.

Thanks for sticking with this!

I skimmed over the old discussion, mostly just to make sure there wasn't anything subtle that might have been forgotten. But nope, all of the subtlety went away because of the refspec API you mentioned.

The whole series looks good to me.

-Peff

Thanks, both.