Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:09:08

SZEDER Gábor [off-list ref] writes:
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 ...').
Is it really 'git fetch' behaves differently?

What is missing in the above description is your expected behaviour
of "git -c var=val clone", and without it we cannot tell if your
expectation is sane to begin with.

Is the expectation like this?

    git init
    git config ... # set default configuration and origin remote
    git config var val # update with what "-c var=val" told us
    git fetch
    git checkout   # unless '--bare' is given

or is it something else?

Is "-c var=val" adding to the config variables set by default, or is
it replacing them?  Does the choice depend on the nature of
individual variables, and if so what is the criteria?

Are all "-c var=val" update the configuration of the resulting
repository?  Or are certain "var"s treated as special and placed in
the config but not other "var"s?  If the latter, what makes these
certain "var"s special?

These design decisions need to be explained so that they will serve
to guide people to decide what other variables to propagate and how
when they have to add new configuration variables in the future.
Otherwise we'd end up with an inconsistent mess.
quoted hunk
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

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

From: SZEDER Gábor <hidden>
Date: 2016-06-15 23:09:09

Quoting Junio C Hamano [off-list ref]:
SZEDER Gábor [off-list ref] writes:
quoted
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 ...').
Is it really 'git fetch' behaves differently?
Certainly.
What is missing in the above description is your expected behaviour
of "git -c var=val clone", and without it we cannot tell if your
expectation is sane to begin with.
These 'git -c var=val cmd' one-shot configuration parameters exist
during the lifespan of the command.  Therefore, in case of 'git -c
var=val clone' they should exist while all the commands in our
mental model are executed.  IOW, those commands should behave as if
these configuration parameters were specified for them, see below.
Is the expectation like this?

    git init
    git config ... # set default configuration and origin remote
    git config var val # update with what "-c var=val" told us
    git fetch
    git checkout   # unless '--bare' is given

or is it something else?
For 'git -c var=val clone':

   git -c var=val init
   git -c var=val config ... # though this probably doesn't really
                             # matter, as it is about writing the
                             # configuration, and it gets the
                             # to-be-written variables and values
                             # in the "..." part anyway
   git -c var=val fetch
   git -c var=val checkout

Being one-shot configuration parameters, they shouldn't be written
to the new repository's config file.

'git clone -c var=val' is designed to be different:

   - it does write var=val into the new repository's config file

   - it specifies that var.val "takes effect immediately after the
     repository is initialized, but before the remote history is
     fetched or any files checked out".

Additionally, there may be relevant config variables defined in the
global and system-wide config files, which of course should be
respected by all these commands.

And it all works just fine as described above, even the initial fetch
respects most of the config variables, wherever specified, except for
fetch refspecs which are completely ignored.
Is "-c var=val" adding to the config variables set by default, or is
it replacing them?  Does the choice depend on the nature of
individual variables, and if so what is the criteria?
It's up to the individual command how it treats a particular config
variable: single-valued variables like 'fetch.fsckObjects' should
override (they already do), multi-valued variables like fetch refspecs
should be added.
Running as part of 'git clone' shouldn't matter at all.

This patch only affects how fetch refspecs are handled, the effects of
other config variables are unchanged.
Are all "-c var=val" update the configuration of the resulting
repository?  Or are certain "var"s treated as special and placed in
the config but not other "var"s?  If the latter, what makes these
certain "var"s special?
In this regard it doesn't matter what 'val=var' is.  What matters is
how the configuration parameter is specified (i.e. 'git -c var=val
clone' vs. 'git clone -c var=val').

This patch doesn't change anything in this regard.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help