Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Tue, May 16, 2017 at 1:07 AM, Jeff King <peff@peff.net> wrote:
>> @@ -989,6 +994,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>       strbuf_reset(&value);
>>
>>       remote = remote_get(option_origin);
>> +     REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);
>> +     memcpy(remote->fetch+remote->fetch_refspec_nr, refspec,
>> +            sizeof(*refspec));
>
> Here we append to remote->fetch. We are assuming then that
> remote->fetch_refspec has already been parsed into remote->fetch. Which
> I think it always is by remote_get(),

Right.

> but given that it lazy-parses in
> some cases, it feels a little dangerous.

I'm not sure about lazy parsing.
remote_get() returns a fully-parsed, cached struct remote instance
without re-reading the configuration, so all fields directly
corresponding to configuration variables stay the same.  However, it
does parse fetch and push refspecs on every invocation.  So if it were
to be called to return the origin remote more than once during
cloning, then the default refspec would get lost on subsequent
invocations.  Is this what you meant with dangerous?

(Sidenote: and it would leak some memory, too, because it re-parses
the refspecs without free()ing the results of the previous
invocation.)

Your proposed function to add a refspec as a string would eliminate
this danger.

> I think in the earlier discussion you mentioned there are some
> ordering
> problems with writing out the new on-disk config. But could we add
> it to
> the temporary environment, like:
>
>   strbuf_addf(&key, "remote.%s.fetch=%s", option_origin,
>   refspec_pattern);
>   git_config_push_parameter(key.buf);
>
> ?

> If all that's correct, then I think the push_parameter() thing would
> work. It does feel like a round-a-bout way to solve the problem, but
> it's at least manipulating solid, public APIs.

It certainly looks better, see the patch below the scissors for
reference, and I thought it works because until last night I only run
the corresponding test script (t5611-clone-config), though I know very
well that "Thou shalt always run the full test suite!" :)

Unfortunately, putting the default refspec into this temporary
configuration environment breaks a few submodule tests
(t5614-clone-submodules or t5614-clone-submodules-shallow (it's got
renamed between this topic and master), t7407-submodule-foreach,
t7410-submodule-checkout-to), because it "leaks" to the submodule
environment.


 -- >8 --

Subject: [PATCH] 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.  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 configured fetch refspecs into account to
retrieve all matching refs during the initial fetch.  Note that the
configuration at that point only includes the fetch refspecs specified
by the user, but it doesn't include the default fetch refspec.
To keep the code simple and parsing and memory management of the
refspecs in one place, add the default fetch refspec to the temporary
configuration environment, so remote_get() can parse it along with all
other refspecs that might have been 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         | 32 ++++++++++++++++----------------
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a35d62293..40f4a327b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -520,7 +520,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 *refspec)
+		struct refspec *refspec, unsigned int refspec_nr)
 {
 	struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
 	struct ref *local_refs = head;
@@ -541,13 +541,18 @@ 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);
+			unsigned int i;
+			for (i = 0; i < refspec_nr; i++)
+				get_fetch_map(remote_head, &refspec[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 {
+		unsigned int i;
+		for (i = 0; i < refspec_nr; i++)
+			get_fetch_map(refs, &refspec[i], &tail, 0);
+	}
 
 	if (!option_mirror && !option_single_branch)
 		get_fetch_map(refs, tag_refspec, &tail, 0);
@@ -848,16 +853,13 @@ 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, default_refspec_config = STRBUF_INIT;
 	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
 	struct transport *transport = NULL;
 	const char *src_ref_prefix = "refs/heads/";
 	struct remote *remote;
 	int err = 0, complete_refs_before_fetch = 1;
 
-	struct refspec *refspec;
-	const char *fetch_pattern;
-
 	packet_trace_identity("clone");
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
@@ -975,7 +977,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);
@@ -983,10 +984,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_reference.nr)
 		setup_reference();
 
-	fetch_pattern = value.buf;
-	refspec = parse_fetch_refspec(1, &fetch_pattern);
-
-	strbuf_reset(&value);
+	strbuf_addf(&default_refspec_config, "remote.%s.fetch=+%s*:%s*",
+		    option_origin, src_ref_prefix, branch_top.buf);
+	git_config_push_parameter(default_refspec_config.buf);
 
 	remote = remote_get(option_origin);
 	transport = transport_get(remote, remote->url[0]);
@@ -1029,7 +1029,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	refs = transport_get_remote_refs(transport);
 
 	if (refs) {
-		mapped_refs = wanted_peer_refs(refs, refspec);
+		mapped_refs = wanted_peer_refs(refs, remote->fetch,
+					       remote->fetch_refspec_nr);
 		/*
 		 * transport_get_remote_refs() may return refs with null sha-1
 		 * in mapped_refs (see struct transport->get_refs_list
@@ -1107,9 +1108,8 @@ 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_config);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	free(refspec);
 	return err;
 }
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index e4850b778..114b53920 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -37,6 +37,50 @@ 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.13.0.35.g14b6294b1