threads / patch / 49834

patch, 3 partsclone: respect configured fetch respecs during initial fetch

Subject: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch

## tl;dr

9 messages between Nov 14, 2018 and Nov 16, 2018. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

SZEDER Gábor· Nov 14, 2018, 10:46 UTC · lore

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/20170616173849.8071-1-szeder.dev@gmail.com/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
SZEDER Gábor· Nov 14, 2018, 10:46 UTC · re: SZEDER Gábor · lore

[PATCH 1/3] clone: use a more appropriate variable name for the default refspec

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.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to builtin/clone.c +6 −6
diff --git a/builtin/clone.c b/builtin/clone.c
index 15b142d646..ae1bf242c6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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
Jeff King· Nov 15, 2018, 10:54 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/3] clone: use a more appropriate variable name for the default refspec

On Wed, Nov 14, 2018 at 11:46:18AM +0100, SZEDER Gábor wrote:
Show 9 quoted lines
> 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.
Yep, this is much nicer.
-Peff
SZEDER Gábor· Nov 14, 2018, 10:46 UTC · re: SZEDER Gábor · lore

[PATCH 2/3] clone: respect additional configured fetch refspecs 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". 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>'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 25 +++++++++++++---------
 t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 10 deletions(-)
Show changes to 2 files +62 −10

builtin/clone.c, t/t5611-clone-config.sh

diff --git a/builtin/clone.c b/builtin/clone.c
index ae1bf242c6..7c7f98c72c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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;
 }
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 39329eb7a8..60c1ba951b 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -45,6 +45,53 @@ test_expect_success 'clone -c config is available during clone' '
 	test_cmp expect child/file
 '
 
+test_expect_success 'clone -c remote.origin.fetch=<refspec> works' '
+	rm -rf child &&
+	git update-ref refs/grab/it refs/heads/master &&
+	git update-ref refs/leave/out refs/heads/master &&
+	git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child &&
+	git -C child for-each-ref --format="%(refname)" >actual &&
+
+	cat >expect <<-\EOF &&
+	refs/grab/it
+	refs/heads/master
+	refs/remotes/origin/HEAD
+	refs/remotes/origin/master
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'git -c remote.origin.fetch=<refspec> clone works' '
+	rm -rf child &&
+	git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child &&
+	git -C child for-each-ref --format="%(refname)" >actual &&
+
+	cat >expect <<-\EOF &&
+	refs/grab/it
+	refs/heads/master
+	refs/remotes/origin/HEAD
+	refs/remotes/origin/master
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
+	rm -rf child &&
+	git clone --origin=upstream \
+		  -c "remote.upstream.fetch=+refs/grab/*:refs/grab/*" \
+		  -c "remote.origin.fetch=+refs/leave/*:refs/leave/*" \
+		  . child &&
+	git -C child for-each-ref --format="%(refname)" >actual &&
+
+	cat >expect <<-\EOF &&
+	refs/grab/it
+	refs/heads/master
+	refs/remotes/upstream/HEAD
+	refs/remotes/upstream/master
+	EOF
+	test_cmp expect actual
+'
+
 # Tests for the hidden file attribute on windows
 is_hidden () {
 	# Use the output of `attrib`, ignore the absolute path
-- 
2.19.1.1182.gbfcc7ed3e6
Jeff King· Nov 15, 2018, 11:04 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/3] clone: respect additional configured fetch refspecs during initial fetch

On Wed, Nov 14, 2018 at 11:46:19AM +0100, SZEDER Gábor wrote:
Show 29 quoted lines
> 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.
Nicely explained.
> 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>'.

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

Show 10 quoted lines
> @@ -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);
Wow, this is so much nicer than the older versions. :)

I wondered briefly whether this ought to only kick in when the user didn't specify any refspecs. I.e., there is no way with this to say "don't fetch refs/heads/*, but this other thing instead". But the way the existing documentation is written, it's pretty clear that it's about adding to the list of refspecs. We can always something like "--no-default-refspec" later if somebody wants it.

> [...]

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.

Show 12 quoted lines
> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index 39329eb7a8..60c1ba951b 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -45,6 +45,53 @@ test_expect_success 'clone -c config is available during clone' '
>  	test_cmp expect child/file
>  '
>  
> +test_expect_success 'clone -c remote.origin.fetch=<refspec> works' '
> +	rm -rf child &&
> +	git update-ref refs/grab/it refs/heads/master &&
> +	git update-ref refs/leave/out refs/heads/master &&
Checking one in and one out; nice.
Show 11 quoted lines
> +	git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child &&
> +	git -C child for-each-ref --format="%(refname)" >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	refs/grab/it
> +	refs/heads/master
> +	refs/remotes/origin/HEAD
> +	refs/remotes/origin/master
> +	EOF
> +	test_cmp expect actual
> +'
Makes sense.
Show 13 quoted lines
> +test_expect_success 'git -c remote.origin.fetch=<refspec> clone works' '
> +	rm -rf child &&
> +	git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child &&
> +	git -C child for-each-ref --format="%(refname)" >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	refs/grab/it
> +	refs/heads/master
> +	refs/remotes/origin/HEAD
> +	refs/remotes/origin/master
> +	EOF
> +	test_cmp expect actual
> +'

This relies on establishing grab/it in the previous test. A minor nit, but we could break that more clear by breaking it out into its own "set up refs for extra refspec tests" block.

Show 6 quoted lines
> +test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
> +	rm -rf child &&
> +	git clone --origin=upstream \
> +		  -c "remote.upstream.fetch=+refs/grab/*:refs/grab/*" \
> +		  -c "remote.origin.fetch=+refs/leave/*:refs/leave/*" \
> +		  . child &&
Nice.
The whole thing looks very cleanly done.
-Peff
SZEDER Gábor· Nov 14, 2018, 10:46 UTC · re: SZEDER Gábor · lore

[PATCH 3/3] Documentation/clone: document ignored configuration variables

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.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to Documentation/git-clone.txt +6 −0
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index a55536f0bf..2fd12524f9 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -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
Jeff King· Nov 15, 2018, 11:06 UTC · re: SZEDER Gábor · lore

Re: [PATCH 3/3] Documentation/clone: document ignored configuration variables

On Wed, Nov 14, 2018 at 11:46:20AM +0100, SZEDER Gábor wrote:
Show 8 quoted lines
> 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.
Good idea.
Show 14 quoted lines
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index a55536f0bf..2fd12524f9 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -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.

This looks good. I considered at first that this might want to go in a BUGS section of the manpage, but it makes the most sense being right next to the definition of "-c".

-Peff
Jeff King· Nov 15, 2018, 11:08 UTC · re: SZEDER Gábor · lore

Re: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch

On Wed, Nov 14, 2018 at 11:46:17AM +0100, SZEDER Gábor wrote:
Show 10 quoted lines
> 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).
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
Junio C Hamano· Nov 16, 2018, 04:14 UTC · re: Jeff King · lore

Re: [PATCH 0/3] clone: respect configured fetch respecs during initial fetch

Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> On Wed, Nov 14, 2018 at 11:46:17AM +0100, SZEDER Gábor wrote:
>
>> 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).
>
> 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.
Thanks, both.

← back to recent threads