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

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

[SZEDER Gábor]

Configuration variables specified via 'git clone -c <key>=<value>'
"take effect immediately after the repository is initialized, but
before the remote history is fetched".  This implies that any fetch
refspecs specified this way should already be taken into account
during the initial fetch and remote refs matching these refspecs
should be retrieved as well.

This never worked, however, not even when the feature was introduced
in v1.7.7-rc0~90^2 (clone: accept config options on the command line,
2011-06-09).  While the given refspecs are written to the config file
alright, no matching refs are fetched, because the initial fetch
ignores them and respects only that single default refspec.

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 <szeder@ira.uka.de>
---

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.

 builtin/clone.c         | 32 +++++++++++++++++++++++++-------
 t/t5708-clone-config.sh | 13 +++++++++++++
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 9ac6c0144279..5b96b373675a 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;
@@ -541,8 +541,11 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
 			/* 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 +843,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 +972,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 +1030,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 +1111,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..377837e3539a 100755
--- a/t/t5708-clone-config.sh
+++ b/t/t5708-clone-config.sh
@@ -37,4 +37,17 @@ 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_done
-- 
2.7.2.410.g92cb358

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> 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 <szeder@ira.uka.de>
> ---

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

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> 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 <szeder@ira.uka.de>
>> ---
>
> 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.

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

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

[Jeff King]

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yup, I 100% agree that we should aim for matching that mental model.

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

[SZEDER Gábor]

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 <szeder@ira.uka.de>
---
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

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> 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.

> 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 <szeder@ira.uka.de>
> ---
> 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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

The above did not start as rhetorical questions, but was merely me
thinking aloud.  However, it showed me a different approach might be
more appropriate.

Taken as rhetorical questions, the sane answers to them would
revolve around "we do not know the semantics of each and every
configuration variable that will be given to this codepath, and by
definition we will never know in advance the ones that will be
introduced later".  IOW, special casing -c remote.origin.fetch=spec
is a bad idea.

So how about teaching "git clone" a new _option_ that is about what
branches are followed?

	git clone $there --branches="master next pu"

would give

	[remote "origin"]
        	fetch = +refs/heads/master:refs/remotes/origin/master
        	fetch = +refs/heads/next:refs/remotes/origin/next
        	fetch = +refs/heads/pu:refs/remotes/origin/pu

instead of the usual

	[remote "origin"]
		fetch = +refs/heads/*:refs/remotes/origin/*

And that can be made to work orthognonal to --single-branch by a
small additional rule: if the branch given by -b <name> (or their
HEAD) is not part of --branches, then we add it to the set of
branches to be followed (i.e. if you give only --single-branch,
without --branches, the set of branches to be followed will become
that single branch).

Hmm?

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> IOW, special casing -c remote.origin.fetch=spec
> is a bad idea.

I completely agree :)
But it's the other way around.

'remote.origin.fetch=spec' during clone is special _now_, because the
initial fetch ignores it, no matter where it is set.

My patch makes it non-special, so that the initial fetch respects it,
the same way it already respects 'fetch.fsckObjects' and
'fsck.unpackLimit', or the way the initial checkout respects e.g.
'core.eol'.

> So how about teaching "git clone" a new _option_ that is about what
> branches are followed?
>
> 	git clone $there --branches="master next pu"
>
> would give
>
> 	[remote "origin"]
>         	fetch = +refs/heads/master:refs/remotes/origin/master
>         	fetch = +refs/heads/next:refs/remotes/origin/next
>         	fetch = +refs/heads/pu:refs/remotes/origin/pu

Without my patch the initial fetch would ignore these refspecs, too.

> instead of the usual
>
> 	[remote "origin"]
> 		fetch = +refs/heads/*:refs/remotes/origin/*

Typing only branch names is much shorter and simpler than typing the
name of a config var and full refspecs, so this would be a nice
simplification of the UI for the case when the user is only
interested in certain branches.  But it wouldn't help if the user
wants to include 'refs/interesting/*' in the initial fetch.


> And that can be made to work orthognonal to --single-branch by a
> small additional rule: if the branch given by -b <name> (or their
> HEAD) is not part of --branches, then we add it to the set of
> branches to be followed (i.e. if you give only --single-branch,
> without --branches, the set of branches to be followed will become
> that single branch).
>
> Hmm?

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

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> IOW, special casing -c remote.origin.fetch=spec
>> is a bad idea.
>
> I completely agree :)
> But it's the other way around.
>
> 'remote.origin.fetch=spec' during clone is special _now_, because the
> initial fetch ignores it, no matter where it is set.
>
> My patch makes it non-special, so that the initial fetch respects it,
> the same way it already respects 'fetch.fsckObjects' and
> 'fsck.unpackLimit', or the way the initial checkout respects e.g.
> 'core.eol'.

... but does "git -c core.eol clone" leave that configuration in the
resulting repository's .git/config?  "git -c user.name=foo" for that
matter.

They may affect the one-shot operation but are not left in the
resulting .git/config, which was what I was driving at.  To make
clone behave as if it is truly a short-hand of

	git init
	git config ;# with default and necessary tweaks to adjust
		   ;# for things like -b, -o, --single-branch
        git fetch
        git checkout

which by the way I think everybody agrees is a worth goal, then
shouldn't the update to the current code be more like "prepare the
default config, tweak with whatever configuration necessary, and
re-read the config before driving the equivalent of 'git fetch'?"

And the conclusion my rhetorical questions led to was that "adjust
for things like..." should not include what comes from "-c var=val"
because there is no sensible way to incorporate them in general.

The most important point is that "-c var=val" is the wrong source of
information to blindly propagete to the resulting .git/config.  And
the point of "--branches" option is not that it would be short and
tidy, but it is more targetted.  With such an approach, nobody would
imagine "git -c random.var=value clone" would be propagated into the
resulting .git/config in a random and unspecified way.

Once you learn what custom set of refs the user wants to fetch, you
would need futzing of the refspecs like you did in your patch. That
part of your patch is salvageable.  The part that special cased the
information that came from "-c remote.origin.fetch" while ignoring
others like user.name that came from exactly the same mechanism via
"-c user.name" is not.

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> Quoting Junio C Hamano <gitster@pobox.com>:
>>
>>> IOW, special casing -c remote.origin.fetch=spec
>>> is a bad idea.
>>
>> I completely agree :)
>> But it's the other way around.
>>
>> 'remote.origin.fetch=spec' during clone is special _now_, because the
>> initial fetch ignores it, no matter where it is set.
>>
>> My patch makes it non-special, so that the initial fetch respects it,
>> the same way it already respects 'fetch.fsckObjects' and
>> 'fsck.unpackLimit', or the way the initial checkout respects e.g.
>> 'core.eol'.
>
> ... but does "git -c core.eol clone" leave that configuration in the
> resulting repository's .git/config?  "git -c user.name=foo" for that
> matter.

No, and it shouldn't.

'git clone -c core.eol', however, should and indeed does.

> They may affect the one-shot operation but are not left in the
> resulting .git/config, which was what I was driving at.  To make
> clone behave as if it is truly a short-hand of
>
> 	git init
> 	git config ;# with default and necessary tweaks to adjust
> 		   ;# for things like -b, -o, --single-branch
>        git fetch
>        git checkout
>
> which by the way I think everybody agrees is a worth goal, then
> shouldn't the update to the current code be more like "prepare the
> default config, tweak with whatever configuration necessary, and
> re-read the config before driving the equivalent of 'git fetch'?"
>
> And the conclusion my rhetorical questions led to was that "adjust
> for things like..." should not include what comes from "-c var=val"
> because there is no sensible way to incorporate them in general.
>
> The most important point is that "-c var=val" is the wrong source of
> information to blindly propagete to the resulting .git/config.

In case of 'git -c var=val clone', I agree, but then again, 'git clone
-c var=val' was specifically designed to write that 'var=val' to the
new repository's config file.

Config variables set in the global or system-wide config files are not
written to the new repository's config file either.

> And
> the point of "--branches" option is not that it would be short and
> tidy, but it is more targetted.  With such an approach, nobody would
> imagine "git -c random.var=value clone" would be propagated into the
> resulting .git/config in a random and unspecified way.
>
> Once you learn what custom set of refs the user wants to fetch, you
> would need futzing of the refspecs like you did in your patch. That
> part of your patch is salvageable.  The part that special cased the
> information that came from "-c remote.origin.fetch" while ignoring
> others like user.name that came from exactly the same mechanism via
> "-c user.name" is not.

My patch did not special case anything and it did not change anything
with respect to what config settings are written under what
circumstances to the new repository's config file.
All that already works properly and almost all those config settings
are already taken into account when they should be by the commands in
our conceptual model of 'git clone'.  It doesn't matter at all where
and how they were specified or whether they are written to the new
repository's config file or not, almost all of them are taken into
account.

Almost all, because there is that one exception: additional fetch
refspecs are ignored during the initial fetch.

And all my patch did was to make the initial fetch aware of any
additional fetch refspecs which are configured when the initial
fetch is executed.  (and again: no matter where those refspecs were
specified and no matter whether they were written to the new config
file)


Eh, I guess I should have went for a refined version of the RFC's
commit message, rewriting it based on that conceptual modell caused
way too much confusion.

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

[SZEDER Gábor]

Quoting Junio C Hamano <gitster@pobox.com>:

> SZEDER Gábor <szeder@ira.uka.de> 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?

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.