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

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

[SZEDER Gábor]

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

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

[SZEDER Gábor]

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(-)

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

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

[Jeff King]

On Wed, Nov 14, 2018 at 11:46:18AM +0100, SZEDER Gábor wrote:

> 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

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

[SZEDER Gábor]

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(-)

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

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

[Jeff King]

On Wed, Nov 14, 2018 at 11:46:19AM +0100, SZEDER Gábor wrote:

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

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

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

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

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

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

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

[SZEDER Gábor]

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(+)

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

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

[Jeff King]

On Wed, Nov 14, 2018 at 11:46:20AM +0100, SZEDER Gábor wrote:

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

> 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

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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