Re: [RFC/PATCH] clone: make 'git clone -c remote.origin.fetch=<refspec>' work

SZEDER Gábor [off-list ref] writes:

Check whether there are any relevant configured fetch refspecs and
take those into account during the initial fetch, unless running 'git
clone --single-branch'.

Signed-off-by: SZEDER Gábor <redacted>
---

Even though I think the original description did not mean to include
the fetch refspecs when it talked about configuration taking effect, 
I think what this change wants to do probably makes sense.

I'm unsure what to do with the '-c <fetch-refspec> --single-branch'
combination: it doesn't really make sense to me and can't imagaine a
use case where it would be useful...  but perhaps I just lack
imagination on this Sunday night.  Hence the RFC.

My knee-jerk reaction is to change the last paragraph of your log
message to read more like

	Always read the fetch refspecs from the newly created config
	file, and use that for the initial fetching.

and do so even when running with "--single-branch".

Quoting Junio C Hamano [off-list ref]:

SZEDER Gábor [off-list ref] writes:

quoted

Check whether there are any relevant configured fetch refspecs and
take those into account during the initial fetch, unless running 'git
clone --single-branch'.

Signed-off-by: SZEDER Gábor <redacted>
---

Even though I think the original description did not mean to include
the fetch refspecs when it talked about configuration taking effect,
I think what this change wants to do probably makes sense.

Well, currently one would have to clone, set additional fetch refspecs,
fetch again and repack.  Using 'git clone -c <refspecs>' would do it in
a single step, requiring fewer commands, less time, less data transfer
and less disk space, which fits the justification of v1.7.7-rc0~90^2
perfectly.

Or init, add remote, set additional refspecs and fetch, which still
requires more commands, but at least doesn't transfer more data.

quoted

I'm unsure what to do with the '-c <fetch-refspec> --single-branch'
combination: it doesn't really make sense to me and can't imagaine a
use case where it would be useful...  but perhaps I just lack
imagination on this Sunday night.  Hence the RFC.

My knee-jerk reaction is to change the last paragraph of your log
message to read more like

	Always read the fetch refspecs from the newly created config
	file, and use that for the initial fetching.

and do so even when running with "--single-branch".

Ok, will change the '--single-branch' codepath as well.

But before doing so, to avoid a possible misunderstanding on my part:
I'm not sure how literally you meant that "from the newly created
config file" part, because it ignores refspecs specified via any
other means, e.g. 'git -c <fetch-refspec> clone'.  I think the
initial fetch should be no different from "regular" fetches, and
should respect all configured fetch refspecs regardless where they
come from.

On Mon, Mar 07, 2016 at 04:19:31PM +0100, SZEDER Gábor wrote:

quoted

Even though I think the original description did not mean to include
the fetch refspecs when it talked about configuration taking effect,
I think what this change wants to do probably makes sense.

Well, currently one would have to clone, set additional fetch refspecs,
fetch again and repack.  Using 'git clone -c <refspecs>' would do it in
a single step, requiring fewer commands, less time, less data transfer
and less disk space, which fits the justification of v1.7.7-rc0~90^2
perfectly.

Yeah, I think your change very much fits the spirit of what the original
commit was trying for.

quoted

My knee-jerk reaction is to change the last paragraph of your log
message to read more like

Always read the fetch refspecs from the newly created config
file, and use that for the initial fetching.

and do so even when running with "--single-branch".

Ok, will change the '--single-branch' codepath as well.

But before doing so, to avoid a possible misunderstanding on my part:
I'm not sure how literally you meant that "from the newly created
config file" part, because it ignores refspecs specified via any
other means, e.g. 'git -c <fetch-refspec> clone'.  I think the
initial fetch should be no different from "regular" fetches, and
should respect all configured fetch refspecs regardless where they
come from.

IMHO, we should stick to the conceptual model that "git clone" is:

  git init
  git config ... ;# set up remote, etc
  git fetch
  git checkout ;# obviously not for --bare

The implementation has to diverge from that to do certain optimizations,
but absent any good reason not to, I think we should aim for behaving
"as if" those commands were run.

It certainly may produce surprising behavior at times, but at least it
is a conceptually simple mental model.  I do admit, though I haven't
thought hard enough to know whether there are any terrible gotchas
there.

-Peff

Conceptually 'git clone' should behave as if the following commands
were run:

  git init
  git config ... # set default configuration and origin remote
  git fetch
  git checkout   # unless '--bare' is given

However, that initial 'git fetch' behaves differently from any
subsequent fetches, because it takes only the default fetch refspec
into account and ignores all other fetch refspecs that might have
been explicitly specified on the command line (e.g. 'git -c
remote.origin.fetch=<refspec> clone' or 'git clone -c ...').

Check whether there are any fetch refspecs configured for the origin
remote and take all of them into account during the initial fetch as
well.

Signed-off-by: SZEDER Gábor <redacted>
---
Changes since previous (RFC) version:
 - new commit message
 - additional configured fetch refspecs are taken into account with
   '--single-branch' as well

 builtin/clone.c         | 36 ++++++++++++++++++++++++++++--------
 t/t5708-clone-config.sh | 24 ++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 661639255c56..5e2d2c21e456 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c

@@ -515,7 +515,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_count)
 {
 	struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
 	struct ref *local_refs = head;

@@ -536,13 +536,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_count; 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_count; i++)
+			get_fetch_map(refs, &refspec[i], &tail, 0);
+	}
 
 	if (!option_mirror && !option_single_branch)
 		get_fetch_map(refs, tag_refspec, &tail, 0);

@@ -840,7 +845,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 
 	struct refspec *refspec;
-	const char *fetch_pattern;
+	unsigned int refspec_count = 1;
+	const char **fetch_patterns;
+	const struct string_list *config_fetch_patterns;
 
 	packet_trace_identity("clone");
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,

@@ -967,9 +974,21 @@ 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_addf(&key, "remote.%s.fetch", option_origin);
+	config_fetch_patterns = git_config_get_value_multi(key.buf);
+	if (config_fetch_patterns)
+		refspec_count = 1 + config_fetch_patterns->nr;
+	fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));
+	fetch_patterns[0] = value.buf;
+	if (config_fetch_patterns) {
+		struct string_list_item *fp;
+		unsigned int i = 1;
+		for_each_string_list_item(fp, config_fetch_patterns)
+			fetch_patterns[i++] = fp->string;
+	}
+	refspec = parse_fetch_refspec(refspec_count, fetch_patterns);
 
+	strbuf_reset(&key);
 	strbuf_reset(&value);
 
 	remote = remote_get(option_origin);

@@ -1013,7 +1032,7 @@ 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, refspec, refspec_count);
 		/*
 		 * transport_get_remote_refs() may return refs with null sha-1
 		 * in mapped_refs (see struct transport->get_refs_list

@@ -1094,6 +1113,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&value);
 	junk_mode = JUNK_LEAVE_ALL;
 
+	free(fetch_patterns);
 	free(refspec);
 	return err;
 }

diff --git a/t/t5708-clone-config.sh b/t/t5708-clone-config.sh
index 27d730c0a720..136a8611c7f3 100755
--- a/t/t5708-clone-config.sh
+++ b/t/t5708-clone-config.sh

@@ -37,4 +37,28 @@ 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/keep/out refs/heads/master &&
+	git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child &&
+	(
+		cd child &&
+		git for-each-ref --format="%(refname)" refs/grab/ >../actual
+	) &&
+	echo refs/grab/it >expect &&
+	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 &&
+	(
+		cd child &&
+		git for-each-ref --format="%(refname)" refs/grab/ >../actual
+	) &&
+	echo refs/grab/it >expect &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.8.0.46.gb821760