[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch

[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch

[SZEDER Gábor]

This is a reroll of sg/clone-refspec-from-command-line-config.
Sorry for the delay, family visit.

The first patch is the updated version of what is now the first commit
of that topic.  The changes are those mentioned in [1]:

 - updated commit message,
 - renamed 'refspec_count' to 'refspec_nr',
 - use the parsed fetch refspecs returned by remote.c:remote_get()
   instead of parsing them ourselves (look at the third hunk of the
   diff of builtin/clone.c, how much shorter it looks),
 - modified tests to check that refs matching the default refspecs are
   transferred as well, and
 - added a test for the combination of '-c
   remote.<remote>.fetch=<refspec> --origin=<name>'.

The second patch is a doc update to warn users that not all
configuration variables are supported via 'git clone -c ...' at the
moment.

Patches 3 and 4 are the last two patches from Peff from this morning
[2].  I picked those up, because his last patch required a bit of
variable name adjustments.  I didn't pick up his first patch, because
using remote_get() already factors out refspec parsing.

[1] - http://public-inbox.org/git/xmqq4lwu7r0s.fsf@gitster.mtv.corp.google.com/T/#u
[2] - http://public-inbox.org/git/20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net/T/#m021eadff5d1e4351d99a27096090be39f53df961

Jeff King (2):
  remote: drop free_refspecs() function
  clone: use free_refspec() to free refspec list

SZEDER Gábor (2):
  clone: respect additional configured fetch refspecs during initial
    fetch
  Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  4 ++++
 builtin/clone.c             | 22 ++++++++++++++++------
 remote.c                    | 28 ++++++----------------------
 t/t5611-clone-config.sh     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 28 deletions(-)

-- 
2.13.0.35.g14b6294b1

[PATCHv3 1/4] 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.  This contradicts to 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.  This implies that any
fetch refspecs specified this way should already be taken into account
during the initial fetch.

The reason is that the initial fetch is not a fully fledged 'git
fetch' but a bunch of direct calls into the fetch/transport machinery,
bypassing parts of 'git fetch' that processes configured fetch
refspecs.  The configured refspecs are, however, 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, so we
have to append it manually at the end of the parsed refspecs array.

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 the remote name via '--origin=<name>'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 20 +++++++++++++++-----
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a35d62293..4144190da 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);
@@ -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));
+
 	transport = transport_get(remote, remote->url[0]);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
 	transport->family = family;
@@ -1029,7 +1038,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 + 1);
 		/*
 		 * transport_get_remote_refs() may return refs with null sha-1
 		 * in mapped_refs (see struct transport->get_refs_list
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

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

[Jeff King]

On Mon, May 15, 2017 at 01:05:54PM +0200, 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.  This contradicts to the documentation stating that

Minor gramm-o: s/to the/the/

> @@ -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(), but given that it lazy-parses in
some cases, it feels a little dangerous.

I also notice that you don't touch remote->fetch_refspec_nr, nor
fetch_refspec_alloc. So the remote struct doesn't actually know about
this entry.  It would probably be wrong if you _did_ update them,
because remote->fetch_refspec (the list of refspec strings) would not
have a matching entry, and would potentially access uninitialized
memory.

I think the whole thing would be a lot less messy if "struct remote" let
you add a new refspec (as a string) after the initial parse, and it
would handle the details. Just making the existing add_fetch_refspec()
public isn't quite enough, because you'd need to invalidate and re-parse
the matching "fetch" array, too. Something like:

diff --git a/remote.c b/remote.c
index 9c8912ab1..0881ed32c 100644
--- a/remote.c
+++ b/remote.c
@@ -2319,3 +2319,17 @@ void apply_push_cas(struct push_cas_option *cas,
 	for (ref = remote_refs; ref; ref = ref->next)
 		apply_cas(cas, remote, ref);
 }
+
+void remote_add_fetch_refspec(struct remote *remote, const char *refspec)
+{
+	add_fetch_refspec(remote, refspec);
+	if (remote->fetch) {
+		struct refspec *parsed;
+
+		parsed = parse_refspec_internal(1, &refspec, 1, 0);
+		REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
+		remote->fetch[remote->fetch_refspec_nr - 1] = *parsed;
+		/* Not free_refspec, as we copied its pointers above */
+		free(parsed);
+	}
+}

That feels a bit dirty, too, but at least it's not reaching across
module boundaries. I think the cleanest thing would be to actually add
it to the config before calling remote_get().

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

?

Come to think of it, though, I thought the reason we weren't using
remote_get() in the first place is that some code paths (like
single-branch) needed to actually get the remote ref list before we knew
the refspec? So how does this approach work at all? :)

I guess that doesn't apply here. We always feed the transport code with
a broad refspec, and then narrow it down later. It's only that we can't
write the final config to disk until we've computed the correct branch
based on the remote refs. gotten the branch.

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.

-Peff

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

[SZEDER Gábor]

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

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

[Jeff King]

On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:

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

More or less. I actually didn't look far enough to see under what
circumstances we might re-parse (or might not have parsed when we add
our extra refspec), but that's definitely the sort of thing I was
worried about.

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

Yes, I'd argue that the current code is buggy, since:

  x = remote_get("foo");
  y = remote_get("foo");

is a guaranteed leak. It seems like remote_get_1() should protect the
call to parse_fetch_refspec() by checking whether ret->fetch is NULL
(and ditto for ret->push).

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

Yeah, I think it's not that hard to support. I'd just rather have the
logic inside remote.c, rather than infecting the caller with the
complexity.

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

Doh, of course. I didn't think of that. That's probably a bad direction,
then, as there's no "just for this process" global config.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:
>
>> 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.
>
> Doh, of course. I didn't think of that. That's probably a bad direction,
> then, as there's no "just for this process" global config.

Yuck, right.  So... do we have a further plan for this topic, or are
the patches more or less good as they are?

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

[Jeff King]

On Tue, May 30, 2017 at 12:53:47PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:
> >
> >> 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.
> >
> > Doh, of course. I didn't think of that. That's probably a bad direction,
> > then, as there's no "just for this process" global config.
> 
> Yuck, right.  So... do we have a further plan for this topic, or are
> the patches more or less good as they are?

I was hoping we'd see one more iteration with the abstracted function
for adding to the "struct remote". Gábor, let me know if you're too sick
of the topic, and I can try to re-spin it.

-Peff

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

[SZEDER Gábor]

On Tue, May 30, 2017 at 5:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> So... do we have a further plan for this topic, or are
> the patches more or less good as they are?

Yup, here it comes.

The main change since v3 is that clone's default refspec is now added
to the 'struct remote' using a function based on Peff's
recommendation, with slight changes:

 - I call that function add_and_parse_fetch_refspec(), because, well,
   that's what it does, and
 - I dropped the if (remote->fetch) condition, because the function
   should work when there are no refspecs in the configuration, too.

I also dropped Peff's two patches that were included in v3, because:

 - his last patch doesn't apply anymore, because the variable it frees
   properly doesn't exist anymore, and
 - with that patch gone his second patch is not tangled up with this
   topic, so I think it should go on its own branch.

Interdiff against the first two patch of v3 below.


SZEDER Gábor (2):
  clone: respect additional configured fetch refspecs during initial
    fetch
  Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  4 ++++
 builtin/clone.c             | 34 +++++++++++++++++-----------------
 remote.c                    | 13 +++++++++++++
 remote.h                    |  1 +
 t/t5611-clone-config.sh     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+), 17 deletions(-)

--
2.13.0.35.g14b6294b1


diff --git a/builtin/clone.c b/builtin/clone.c
index 4144190da..4157922d8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -853,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 = 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);
@@ -980,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);
@@ -988,15 +984,10 @@ 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);
-
        remote = remote_get(option_origin);
-       REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr + 1);
-       memcpy(remote->fetch+remote->fetch_refspec_nr, refspec,
-              sizeof(*refspec));
+       strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
+                   branch_top.buf);
+       add_and_parse_fetch_refspec(remote, default_refspec.buf);

        transport = transport_get(remote, remote->url[0]);
        transport_set_verbosity(transport, option_verbosity, option_progress);
@@ -1039,7 +1030,7 @@ int cmd_clone(int argc, const char **argv, const
char *prefix)

        if (refs) {
                mapped_refs = wanted_peer_refs(refs, remote->fetch,
-                                              remote->fetch_refspec_nr + 1);
+                                              remote->fetch_refspec_nr);
                /*
                 * transport_get_remote_refs() may return refs with null sha-1
                 * in mapped_refs (see struct transport->get_refs_list
@@ -1117,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);
        junk_mode = JUNK_LEAVE_ALL;

-       free(refspec);
        return err;
 }
diff --git a/remote.c b/remote.c
index ad6c5424e..b8fd09dc9 100644
--- a/remote.c
+++ b/remote.c
@@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int
nr_refspec, const char **refspec)
        return parse_refspec_internal(nr_refspec, refspec, 1, 0);
 }

+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
+{
+       struct refspec *rs;
+
+       add_fetch_refspec(remote, refspec);
+       rs = parse_fetch_refspec(1, &refspec);
+       REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
+       remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
+
+       /* Not free_refspecs(), as we copied its pointers above */
+       free(rs);
+}
+
 static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 {
        return parse_refspec_internal(nr_refspec, refspec, 0, 0);
diff --git a/remote.h b/remote.h
index 924881169..9ad8c1085 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);

 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);

 void free_refspec(int nr_refspec, struct refspec *refspec);

[PATCHv4 1/2] 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.  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.  Add a
function to the remote API which encapsulates adding a fetch refspec
to a 'struct remote' and parsing it, including all the necessary
memory management housekeeping as well.  Use this new function to add
clone's default fetch refspec to the other refspecs the user might
have specified.

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

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 34 +++++++++++++++++-----------------
 remote.c                | 13 +++++++++++++
 remote.h                |  1 +
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 17 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a35d62293..4157922d8 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 = 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,12 +984,11 @@ 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);
-
 	remote = remote_get(option_origin);
+	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
+		    branch_top.buf);
+	add_and_parse_fetch_refspec(remote, default_refspec.buf);
+
 	transport = transport_get(remote, remote->url[0]);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
 	transport->family = family;
@@ -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);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	free(refspec);
 	return err;
 }
diff --git a/remote.c b/remote.c
index ad6c5424e..b8fd09dc9 100644
--- a/remote.c
+++ b/remote.c
@@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
 	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
 }
 
+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
+{
+	struct refspec *rs;
+
+	add_fetch_refspec(remote, refspec);
+	rs = parse_fetch_refspec(1, &refspec);
+	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
+	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
+
+	/* Not free_refspecs(), as we copied its pointers above */
+	free(rs);
+}
+
 static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 {
 	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
diff --git a/remote.h b/remote.h
index 924881169..9ad8c1085 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
 
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);
 
 void free_refspec(int nr_refspec, struct refspec *refspec);
 
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

[PATCHv4 2/2] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

Due to limitations/bugs 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'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d69..4f1e7d4ba 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -186,6 +186,10 @@ 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.
+	Note that due to limitations of the current implementation some
+	configuration variables don't take effect during the initial
+	fetch and checkout.  Configuration variables known to not take
+	effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.
 
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the
-- 
2.13.0.35.g14b6294b1

Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables

[Ævar Arnfjörð Bjarmason]

On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Due to limitations/bugs 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'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-clone.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..4f1e7d4ba 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,10 @@ 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.
> +       Note that due to limitations of the current implementation some
> +       configuration variables don't take effect during the initial
> +       fetch and checkout.  Configuration variables known to not take
> +       effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.

We should add to that: "Instead supply the --mirror and --no-tags
options, respectively".

>  --depth <depth>::
>         Create a 'shallow' clone with a history truncated to the
> --
> 2.13.0.35.g14b6294b1
>

Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

On Tue, May 30, 2017 at 11:01 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>> index ec41d3d69..4f1e7d4ba 100644
>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -186,6 +186,10 @@ 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.
>> +       Note that due to limitations of the current implementation some
>> +       configuration variables don't take effect during the initial
>> +       fetch and checkout.  Configuration variables known to not take
>> +       effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.
>
> We should add to that: "Instead supply the --mirror and --no-tags
> options, respectively".

I don't think we can do that in this patch right now.  This topic
branches off from c3808ca69 (preparing for 2.10.3, 2016-12-05), I
assume because, as a bugfix, it will be included in maintenance
releases for older releases, and those won't have the '--no-tags'
option.  Perhaps as a follow-up, only on master.

Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables

[Ævar Arnfjörð Bjarmason]

On Wed, May 31, 2017 at 10:50 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, May 30, 2017 at 11:01 AM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
>>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>>> index ec41d3d69..4f1e7d4ba 100644
>>> --- a/Documentation/git-clone.txt
>>> +++ b/Documentation/git-clone.txt
>>> @@ -186,6 +186,10 @@ 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.
>>> +       Note that due to limitations of the current implementation some
>>> +       configuration variables don't take effect during the initial
>>> +       fetch and checkout.  Configuration variables known to not take
>>> +       effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.
>>
>> We should add to that: "Instead supply the --mirror and --no-tags
>> options, respectively".
>
> I don't think we can do that in this patch right now.  This topic
> branches off from c3808ca69 (preparing for 2.10.3, 2016-12-05), I
> assume because, as a bugfix, it will be included in maintenance
> releases for older releases, and those won't have the '--no-tags'
> option.  Perhaps as a follow-up, only on master.

Sorry, my misunderstanding. I didn't think we'd be updating docs like
this in maint releases, thought this was for to-be-2.14.

Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables

[Jonathan Nieder]

Hi,

SZEDER Gábor wrote:

> Due to limitations/bugs 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'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-clone.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Makes sense.

> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..4f1e7d4ba 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,10 @@ 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.
> +	Note that due to limitations of the current implementation some
> +	configuration variables don't take effect during the initial
> +	fetch and checkout.  Configuration variables known to not take
> +	effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.
>  

Tiny nit: the paragraph of --config description is already a bit
overwhelming, and I think this additional note takes it over the edge
where I give up and stop reading.  Could it go in a separate
paragraph?

		the config file. This makes it safe, for example, to add
		additional fetch refspecs to the origin remote.
	+
	Due to limitations in the current implementation, some
	configuration variables do not take effect until after the
	initial fetch and checkout. Configuration variables known
	not to take effect are `remote.<name>.mirror` and
	`remote.<name>.tagOpt`.

Thanks,
Jonathan

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:

> diff --git a/remote.c b/remote.c
> index ad6c5424e..b8fd09dc9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>  	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>  }
>  
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> +	struct refspec *rs;
> +
> +	add_fetch_refspec(remote, refspec);
> +	rs = parse_fetch_refspec(1, &refspec);
> +	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> +	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);
> +}

What happens here if remote->fetch isn't already initialized? I think
we'd end up with a bunch of garbage values. That's what I was trying to
protect against in my original suggestion.

I'm not sure if that's possible or not. We seem to initialize it in both
remote_get() and for_each_remote(), and I don't think there are any
other ways to get a remote. (In fact, I kind of wondered why we do this
parsing lazily at all, but I think it's so that misconfigured remotes
don't cause us to die() if we aren't accessing them at all).

If we assume that the contract that remote.c provides is that the
fetch fields are always filled in before a "struct remote" is returned
to a caller, and that only such callers would use this function, it's
OK. It just seems more dangerous than it needs to be.

-Peff

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[SZEDER Gábor]

On Wed, May 31, 2017 at 6:23 AM, Jeff King <peff@peff.net> wrote:
> On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:
>
>> diff --git a/remote.c b/remote.c
>> index ad6c5424e..b8fd09dc9 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>>       return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>>  }
>>
>> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
>> +{
>> +     struct refspec *rs;
>> +
>> +     add_fetch_refspec(remote, refspec);
>> +     rs = parse_fetch_refspec(1, &refspec);
>> +     REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
>> +     remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
>> +
>> +     /* Not free_refspecs(), as we copied its pointers above */
>> +     free(rs);
>> +}
>
> What happens here if remote->fetch isn't already initialized? I think
> we'd end up with a bunch of garbage values. That's what I was trying to
> protect against in my original suggestion.
>
> I'm not sure if that's possible or not. We seem to initialize it in both
> remote_get() and for_each_remote(), and I don't think there are any
> other ways to get a remote.

The only place creating remotes is remote.c:make_remote(), which
calloc()s the required memory, making all of struct remote's fields
zero-initialized.  In case of clone the common case is that the user
doesn't specify any additional fetch refspecs, so remote->fetch will
still be NULL after full initialization and when
add_and_parse_fetch_refspec() is called with the default fetch
refspec, meaning we can't 'if (remote->fetch) { parse ... }'.  OTOH,
all functions involved can cope with the fetch-refspec-related fields
being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for
array indexing it's not 0 anymore.

> (In fact, I kind of wondered why we do this
> parsing lazily at all, but I think it's so that misconfigured remotes
> don't cause us to die() if we aren't accessing them at all).
>
> If we assume that the contract that remote.c provides is that the
> fetch fields are always filled in before a "struct remote" is returned
> to a caller, and that only such callers would use this function, it's
> OK. It just seems more dangerous than it needs to be.
>
> -Peff

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Wed, May 31, 2017 at 11:34:23AM +0200, SZEDER Gábor wrote:

> >> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> >> +{
> >> +     struct refspec *rs;
> >> +
> >> +     add_fetch_refspec(remote, refspec);
> >> +     rs = parse_fetch_refspec(1, &refspec);
> >> +     REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> >> +     remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> >> +
> >> +     /* Not free_refspecs(), as we copied its pointers above */
> >> +     free(rs);
> >> +}
> >
> > What happens here if remote->fetch isn't already initialized? I think
> > we'd end up with a bunch of garbage values. That's what I was trying to
> > protect against in my original suggestion.
> >
> > I'm not sure if that's possible or not. We seem to initialize it in both
> > remote_get() and for_each_remote(), and I don't think there are any
> > other ways to get a remote.
> 
> The only place creating remotes is remote.c:make_remote(), which
> calloc()s the required memory, making all of struct remote's fields
> zero-initialized.  In case of clone the common case is that the user
> doesn't specify any additional fetch refspecs, so remote->fetch will
> still be NULL after full initialization and when
> add_and_parse_fetch_refspec() is called with the default fetch
> refspec, meaning we can't 'if (remote->fetch) { parse ... }'.  OTOH,
> all functions involved can cope with the fetch-refspec-related fields
> being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for
> array indexing it's not 0 anymore.

Yeah, I agree it is safe now. I'm just worried about some function in
remote.c later doing:

   read_config();
   add_and_parse_fetch_refspec(remotes[0], whatever);

which leaves the struct in an inconsistent state (we realloc NULL which
allocates from scratch, and all of the other entries in remote->fetch
end up uninitialized).  Can we at least add an assertion like:

  if (!remote->fetch)
	BUG("cannot add refspec to an unparsed remote");

?

-Peff

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[SZEDER Gábor]

On Mon, Jun 5, 2017 at 10:18 AM, Jeff King <peff@peff.net> wrote:
> Yeah, I agree it is safe now. I'm just worried about some function in
> remote.c later doing:
>
>    read_config();
>    add_and_parse_fetch_refspec(remotes[0], whatever);
>
> which leaves the struct in an inconsistent state (we realloc NULL which
> allocates from scratch, and all of the other entries in remote->fetch
> end up uninitialized).  Can we at least add an assertion like:
>
>   if (!remote->fetch)
>         BUG("cannot add refspec to an unparsed remote");
>
> ?

But as mentioned before, remote->fetch being NULL is not a bug in
itself, it's a perfectly valid value even in a fully parsed remote
when the remote has no fetch refspecs.
Therefore, I think, the condition should instead be:

  remote->fetch_refspec_nr && !remote->fetch

We could even try to be extra helpful by checking this condition and
calling parse_fetch_refspec() to initialize remote->fetch instead of
BUG()ing out.  However, that would mask the real issue, namely not
using remote_get() to get the remote, so I don't actually think that's
a good thing to do.

OTOH, having remote->fetch so closely related to, yet separate from
remote->fetch_refspec{,_nr,_alloc} will always inherently be error
prone.  This assertion would catch one case where a less than careful
dev could cause trouble, sure, but there will be still others left,
e.g. he could still do:

  add_fetch_refspec(remote, ...);    // this doesn't update remote->fetch
  for (i = 0; i < remote->fetch_refspec_nr; i++)
        func(remote->fetch[i]);

and watch the array indexing blow up in the last iteration.

Or a non-hypothetical one: when I first tried to use remote_get() for
an earlier version of this patch, I ALLOC_GROW()-ed remote->fetch to
create room for the default refspec, because in struct remote not
**fetch_refspec but *fetch is listed right above
fetch_refspec_{nr,alloc}, being way past my bedtime may be my only
excuse...  It didn't work :) [1]

To put your worries to rest we should eliminate remote->fetch_refspec
altogether and parse refspecs into remote->fetch right away, I'd
think.  After all, that's what's used in most places anyway, and it
can be easily turned back to a single string where needed (I think in
only 3 places in builtin/remote.c).


[1] - Though in the end this could be considered beneficial, because
      commits 53c5de29 (pickaxe: fix segfault with '-S<...>
      --pickaxe-regex', 2017-03-18), 59210dd56 (tests: make the
      'test_pause' helper work in non-verbose mode, 2017-03-18), and
      4ecae3c8c (tests: create an interactive gdb session with the
      'debug' helper, 2017-03-18) were all fallouts from the ensuing
      debugging session :)

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Tue, Jun 06, 2017 at 08:19:09PM +0200, SZEDER Gábor wrote:

> >   if (!remote->fetch)
> >         BUG("cannot add refspec to an unparsed remote");
> >
> > ?
> 
> But as mentioned before, remote->fetch being NULL is not a bug in
> itself, it's a perfectly valid value even in a fully parsed remote
> when the remote has no fetch refspecs.
> Therefore, I think, the condition should instead be:
> 
>   remote->fetch_refspec_nr && !remote->fetch

Right, that would be a better check.

> We could even try to be extra helpful by checking this condition and
> calling parse_fetch_refspec() to initialize remote->fetch instead of
> BUG()ing out.  However, that would mask the real issue, namely not
> using remote_get() to get the remote, so I don't actually think that's
> a good thing to do.

OK.

> To put your worries to rest we should eliminate remote->fetch_refspec
> altogether and parse refspecs into remote->fetch right away, I'd
> think.  After all, that's what's used in most places anyway, and it
> can be easily turned back to a single string where needed (I think in
> only 3 places in builtin/remote.c).

I don't think we can parse right away without regressing the error
handling. If I have two remotes, one with a bogus refspec, like:

  [remote "one"]
  url = ...
  fetch = refs/heads/*:refs/remotes/one/*
  [remote "two"]
  url = ...
  fetch = ***bogus***

and I do:

  git fetch one

then read_config() will grab the data for _both_ of them, but only call
remote_get() on the first one. If we parsed the refspecs during
read_config(), we'd parse the bogus remote.two.fetch and die().

I guess that's a minor case, but as far as I can tell that's the
motivation for the lazy parsing.

-Peff

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[SZEDER Gábor]

On Tue, Jun 6, 2017 at 8:37 PM, Jeff King <peff@peff.net> wrote:

>> To put your worries to rest we should eliminate remote->fetch_refspec
>> altogether and parse refspecs into remote->fetch right away, I'd
>> think.  After all, that's what's used in most places anyway, and it
>> can be easily turned back to a single string where needed (I think in
>> only 3 places in builtin/remote.c).
>
> I don't think we can parse right away without regressing the error
> handling. If I have two remotes, one with a bogus refspec, like:
>
>   [remote "one"]
>   url = ...
>   fetch = refs/heads/*:refs/remotes/one/*
>   [remote "two"]
>   url = ...
>   fetch = ***bogus***
>
> and I do:
>
>   git fetch one
>
> then read_config() will grab the data for _both_ of them, but only call
> remote_get() on the first one. If we parsed the refspecs during
> read_config(), we'd parse the bogus remote.two.fetch and die().
>
> I guess that's a minor case, but as far as I can tell that's the
> motivation for the lazy parsing.

Yeah, I know, we'd need a parse_refspec_gently() or something.
parse_refspec_internal() already has a 'verify' parameter which
prevents it from die()ing while parsing a bogus refspec, but in its
current form it doesn't tell us which refspec was bogus, so we'd need
a bit more than that to let the user know what's wrong.

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jonathan Nieder]

Hi,

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

Agh, subtle.

I'm hoping that longer term we can make fetch behave more like a
library and make the initial fetch into a fully fledged 'git fetch'
like thing again.  But this smaller change is the logical fix in the
meantime.

[...]
> diff --git a/remote.c b/remote.c
> index ad6c5424e..b8fd09dc9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>  	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>  }
>  
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> +	struct refspec *rs;
> +
> +	add_fetch_refspec(remote, refspec);
> +	rs = parse_fetch_refspec(1, &refspec);
> +	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> +	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);
> +}
> +
>  static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
>  {
>  	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
> diff --git a/remote.h b/remote.h
> index 924881169..9ad8c1085 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
>  
>  int valid_fetch_refspec(const char *refspec);
>  struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);
>  
>  void free_refspec(int nr_refspec, struct refspec *refspec);

I realize its neighbors don't have this, but can this function have a
brief comment explaining how it is meant to be used and what
guarantees it makes?

For example:

	/** Adds a refspec to remote->fetch_refspec and remote->fetch. */
	void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);

I'm tempted to say that this one should be named add_fetch_refspec (or
something like remote_add_refspec) --- this is the only way to add a
fetch refspec in the public remote API, and the fact that it parses is
an implementation detail.  The private add_fetch_refpsec that builds
the fetch_refspec as preparation for parsing them in a batch is not
part of the exported API.

Also, now that the API is appending to remote->fetch instead of
allocating it in one go, should it use the ALLOC_GROW heuristic /
fetch_refspec_alloc size?

The caller adds one refspec right after calling remote_get.  I'm
starting to wonder if this could be done more simply by having a
variant of remote_get that allows naming an additional refspec, so
that remote->fetch could be immutable after construction like it was
before.  What do you think?

[...]
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);

Allocating an array to put the parsed refspec in and then freeing it
seems wasteful.  Should parse_refspec_internal be changed to take an
output parameter so it can put the refspec into remote->fetch
directly?

[...]
> +++ b/builtin/clone.c
[...]
> @@ -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 = STRBUF_INIT;

nit: since it's not part of a key, value pair like value,
default_refspec should probably go on its own line.

[...]
> --- 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
> +'

Can use <<-\EOF to save the reviewer from having to look for variable
interpolations.

optional nit: might be easier to read with a blank line before the
"cat >expect" line or the for-each-ref line.  That way, it's easier to
separate the validation of output from the commands being run at a
glance and see what the test is about.

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

Likewise.

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

Likewise.  Nice.

> +	test_cmp expect actual
> +'
> +
>  # Tests for the hidden file attribute on windows
>  is_hidden () {
>  	# Use the output of `attrib`, ignore the absolute path

The rest looks good.  Thanks for a pleasant read.

Sincerely,
Jonathan

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Tue, Jun 13, 2017 at 05:48:16PM -0700, Jonathan Nieder wrote:

> > 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.
> 
> Agh, subtle.
> 
> I'm hoping that longer term we can make fetch behave more like a
> library and make the initial fetch into a fully fledged 'git fetch'
> like thing again.  But this smaller change is the logical fix in the
> meantime.

We talked about this earlier in the thread, but I doubt that will ever
happen. Things like --single-branch means that clone has to actually
look at what the remote has before it decides what to fetch.

Of course we _could_ teach all that logic to fetch, too, but I don't
think you'll ever get the nice simple:

  1. configure refspecs
  2. fetch
  3. checkout

So the best thing is probably to push any repeated logic down into the
transport layer, where it can be easily used by both commands.

> > +	/* Not free_refspecs(), as we copied its pointers above */
> > +	free(rs);
> 
> Allocating an array to put the parsed refspec in and then freeing it
> seems wasteful.  Should parse_refspec_internal be changed to take an
> output parameter so it can put the refspec into remote->fetch
> directly?

It's not quite trivial to make that change. parse_refspec_internal()
actually handles an array of refspecs. So its callers would all have to
start allocating the correctly-sized array.

I doubt that one malloc per clone is worth caring about. I'd worry more
about the trickiness that merited the comment above, but it's at least
contained in this one function.

-Peff

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[SZEDER Gábor]

On Wed, Jun 14, 2017 at 2:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> diff --git a/remote.h b/remote.h
>> index 924881169..9ad8c1085 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
>>
>>  int valid_fetch_refspec(const char *refspec);
>>  struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
>> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);

> I'm tempted to say that this one should be named add_fetch_refspec (or
> something like remote_add_refspec) --- this is the only way to add a
> fetch refspec in the public remote API, and the fact that it parses is
> an implementation detail.  The private add_fetch_refpsec that builds
> the fetch_refspec as preparation for parsing them in a batch is not
> part of the exported API.

I kind of agree, but ...

First, there is an add_push_refspec() function as well, which, just
like its fetch counterpart, doesn't parse the given refspec, only
appends it to remote->push_refspec.  Changing add_fetch_refspec() to
parse, too, would break this symmetry.

Furthermore, at the moment we have both remote->fetch_refspec (for
strings) and remote->fetch (for parsed refspecs), and parsing a
refspec die()s if it's bogus, therefore I think that parsing is not an
implementation detail that should be hidden.

> The caller adds one refspec right after calling remote_get.  I'm
> starting to wonder if this could be done more simply by having a
> variant of remote_get that allows naming an additional refspec, so
> that remote->fetch could be immutable after construction like it was
> before.  What do you think?

That's such a very specific and narrow use case that I don't think it
justifies a dedicated function.
I don't think remote->fetch should be immutable; I think
remote->{fetch,push}_refspec and the lazy parsing of refspecs should
go away.  Cleaning up this corner of the remote API is beyond the
scope of this patch series.

> [...]
>> +     /* Not free_refspecs(), as we copied its pointers above */
>> +     free(rs);
>
> Allocating an array to put the parsed refspec in and then freeing it
> seems wasteful.  Should parse_refspec_internal be changed to take an
> output parameter so it can put the refspec into remote->fetch
> directly?

No, I found that extracting the huge body of its loop into a helper
function that fills an output parameter is much more useful.


> [...]
>> +++ b/builtin/clone.c
> [...]
>> @@ -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 = STRBUF_INIT;
>
> nit: since it's not part of a key, value pair like value,
> default_refspec should probably go on its own line.

Fun fact: they were never part of a key-value pair.  While 'key' is
indeed the name of a configuration variable, 'value' is not the value
of that configuration variable, or any other configuration variable
for that matter.


Best,
Gábor

[PATCHv5 0/2] clone: respect additional configured fetch refspecs during initial fetch

[SZEDER Gábor]

Only small, straightforward adjustments, mostly based on Jonathan's
comments.

The one exception is passing the default refspec using
strbuf_detach(), because add_fetch_refspec() doesn't make a copy of it
internally.  It doesn't make any difference in practice, because
strbuf default_refspec remains unaltered til the very end, but this is
the right thing to do.

SZEDER Gábor (2):
  clone: respect additional configured fetch refspecs during initial
    fetch
  Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  5 +++++
 builtin/clone.c             | 36 ++++++++++++++++++----------------
 remote.c                    | 13 +++++++++++++
 remote.h                    |  1 +
 t/t5611-clone-config.sh     | 47 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 85 insertions(+), 17 deletions(-)

-- 
2.13.1.505.g7cc9fcafb



diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 4f1e7d4ba..5ceccb258 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -186,10 +186,11 @@ 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.
-	Note that due to limitations of the current implementation some
-	configuration variables don't take effect during the initial
-	fetch and checkout.  Configuration variables known to not take
-	effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.
++
+Due to limitations if 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`.
 
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the
diff --git a/builtin/clone.c b/builtin/clone.c
index 4157922d8..8a7edd4e5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -853,7 +853,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, default_refspec = 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/";
@@ -987,7 +988,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	remote = remote_get(option_origin);
 	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
 		    branch_top.buf);
-	add_and_parse_fetch_refspec(remote, default_refspec.buf);
+	add_and_parse_fetch_refspec(remote,
+				    strbuf_detach(&default_refspec, NULL));
 
 	transport = transport_get(remote, remote->url[0]);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 114b53920..f240b22cc 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -43,7 +43,8 @@ test_expect_success 'clone -c remote.origin.fetch=<refspec> works' '
 	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 &&
+
+	cat >expect <<-\EOF &&
 	refs/grab/it
 	refs/heads/master
 	refs/remotes/origin/HEAD
@@ -56,7 +57,8 @@ 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 &&
+
+	cat >expect <<-\EOF &&
 	refs/grab/it
 	refs/heads/master
 	refs/remotes/origin/HEAD
@@ -72,7 +74,8 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
 		-c "remote.origin.fetch=+refs/leave/*:refs/leave/*" \
 		. child &&
 	git -C child for-each-ref --format="%(refname)" >actual &&
-	cat >expect <<-EOF &&
+
+	cat >expect <<-\EOF &&
 	refs/grab/it
 	refs/heads/master
 	refs/remotes/upstream/HEAD

[PATCHv5 1/2] 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.  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.  Add a
function to the remote API which encapsulates adding a fetch refspec
to a 'struct remote' and parsing it, including all the necessary
memory management housekeeping as well.  Use this new function to add
clone's default fetch refspec to the other refspecs the user might
have specified.

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

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 36 +++++++++++++++++++-----------------
 remote.c                | 13 +++++++++++++
 remote.h                |  1 +
 t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 17 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a35d62293..8a7edd4e5 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,14 @@ 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/";
 	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 +978,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,12 +985,12 @@ 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);
-
 	remote = remote_get(option_origin);
+	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
+		    branch_top.buf);
+	add_and_parse_fetch_refspec(remote,
+				    strbuf_detach(&default_refspec, NULL));
+
 	transport = transport_get(remote, remote->url[0]);
 	transport_set_verbosity(transport, option_verbosity, option_progress);
 	transport->family = family;
@@ -1029,7 +1031,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 +1110,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);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	free(refspec);
 	return err;
 }
diff --git a/remote.c b/remote.c
index ad6c5424e..b8fd09dc9 100644
--- a/remote.c
+++ b/remote.c
@@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
 	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
 }
 
+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
+{
+	struct refspec *rs;
+
+	add_fetch_refspec(remote, refspec);
+	rs = parse_fetch_refspec(1, &refspec);
+	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
+	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
+
+	/* Not free_refspecs(), as we copied its pointers above */
+	free(rs);
+}
+
 static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 {
 	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
diff --git a/remote.h b/remote.h
index 924881169..9ad8c1085 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
 
 int valid_fetch_refspec(const char *refspec);
 struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
+void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);
 
 void free_refspec(int nr_refspec, struct refspec *refspec);
 
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index e4850b778..f240b22cc 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -37,6 +37,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.13.1.505.g7cc9fcafb

Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jonathan Nieder]

SZEDER Gábor wrote:

> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  builtin/clone.c         | 36 +++++++++++++++++++-----------------
>  remote.c                | 13 +++++++++++++
>  remote.h                |  1 +
>  t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 80 insertions(+), 17 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Fri, Jun 16, 2017 at 07:38:48PM +0200, SZEDER Gábor wrote:

> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> +	struct refspec *rs;
> +
> +	add_fetch_refspec(remote, refspec);
> +	rs = parse_fetch_refspec(1, &refspec);
> +	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> +	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);
> +}

I'd still prefer this to have:

  if (!remote->fetch && remote->fetch_refspec_nr)
	BUG("attempt to add refspec to uninitialized list");

at the top, as otherwise this case writes garbage into remote->fetch[0].

I see you have another series dealing with the lazy parsing, but I
haven't looked at it yet (hopefully this danger would just go away after
that).

Other than that, the patch looks fine to me.

-Peff

Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'd still prefer this to have:
>
>   if (!remote->fetch && remote->fetch_refspec_nr)
> 	BUG("attempt to add refspec to uninitialized list");
>
> at the top, as otherwise this case writes garbage into remote->fetch[0].
>
> I see you have another series dealing with the lazy parsing, but I
> haven't looked at it yet (hopefully this danger would just go away after
> that).
>
> Other than that, the patch looks fine to me.

SZEDER?  As long as the end result together with two series are
safe, I do not have a strong preference, but given that the other
one is a lot more invasive change [*1*], I think it is nicer to have
this two-patch series already safe without the other one.

What's your take on Peff's point?


[Footnote]

*1* Especially the other branch does not merge cleanly into 'pu' and
    I haven't managed to include it in my tree yet.

Re: [PATCHv5 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> I'd still prefer this to have:
>>
>>   if (!remote->fetch && remote->fetch_refspec_nr)
>> 	BUG("attempt to add refspec to uninitialized list");
>>
>> at the top, as otherwise this case writes garbage into remote->fetch[0].
>>
>> I see you have another series dealing with the lazy parsing, but I
>> haven't looked at it yet (hopefully this danger would just go away after
>> that).
>>
>> Other than that, the patch looks fine to me.
>
> SZEDER?  As long as the end result together with two series are
> safe, I do not have a strong preference, but given that the other
> one is a lot more invasive change [*1*], I think it is nicer to have
> this two-patch series already safe without the other one.
>
> What's your take on Peff's point?
>
> [Footnote]
>
> *1* Especially the other branch does not merge cleanly into 'pu' and
>     I haven't managed to include it in my tree yet.

After infinite amount of time has passed, I no longer even remember
what "the other" topic was X-<.  I'll tentatively eject these two
patches from my tree in preparation for starting the cycle after Git
2.14 early next week, but hopefully you can find time and energy to
resurrect it.

Or somebody else may be interested to take the topic over--which is
also fine by me, too.

Thanks.

[PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

Due to limitations/bugs 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'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d69..5ceccb258 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -186,6 +186,11 @@ 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 if 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`.
 
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the
-- 
2.13.1.505.g7cc9fcafb

Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 16 2017, SZEDER Gábor jotted:

> Due to limitations/bugs 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'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-clone.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..5ceccb258 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,11 @@ 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 if 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`.
>
>  --depth <depth>::
>  	Create a 'shallow' clone with a history truncated to the

Just so we're all on the same page, in
CACBZZX740rcQKnfkRXgn0+fmeUDaWL-Kz5WzKeyUvBhXWPwPhg@mail.gmail.com I had
the feedback that this patch didn't make sense on top of 2.13.0 since we
have --no-tags now which should be documented here, but you replied in
CAM0VKjmz7MpVt3oPBuwHiXNoLkZmdmrZ66ggk+aY5-4oVkE35A@mail.gmail.com in an
answer I understood to mean that this was a patch not meant for master,
but for the main track.

But this is now cooking in pu, Junio: is it clear that this patchu
as-cooking ideally shouldn't land in next/master without the fix on top
which I mentioned in my mail above? I can just submit that as a patch on
top, but I'm confused about the current state with this cooking in pu,
so I thought I'd ask first how this should be handled.

Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Fri, Jun 16 2017, SZEDER Gábor jotted:

>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -186,6 +186,11 @@ 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 if 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`.
>>
>>  --depth <depth>::
>>  	Create a 'shallow' clone with a history truncated to the
[...]
> But this is now cooking in pu, Junio: is it clear that this patchu
> as-cooking ideally shouldn't land in next/master without the fix on top
> which I mentioned in my mail above? I can just submit that as a patch on
> top, but I'm confused about the current state with this cooking in pu,
> so I thought I'd ask first how this should be handled.

I think it's simplest to write a patch on top that discusses --no-tags.
That way, Junio (and anyone else applying the patch) has the
flexibility to apply or cherry-pick this change to old branches
without the --no-tags discussion and newer branches with it.

Would you like to write it (or suggest wording), or would you prefer
if someone else does?

Thanks,
Jonathan

Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 16 2017, Jonathan Nieder jotted:

> Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Jun 16 2017, SZEDER Gábor jotted:
>
>>> --- a/Documentation/git-clone.txt
>>> +++ b/Documentation/git-clone.txt
>>> @@ -186,6 +186,11 @@ 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 if 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`.
>>>
>>>  --depth <depth>::
>>>  	Create a 'shallow' clone with a history truncated to the
> [...]
>> But this is now cooking in pu, Junio: is it clear that this patchu
>> as-cooking ideally shouldn't land in next/master without the fix on top
>> which I mentioned in my mail above? I can just submit that as a patch on
>> top, but I'm confused about the current state with this cooking in pu,
>> so I thought I'd ask first how this should be handled.
>
> I think it's simplest to write a patch on top that discusses --no-tags.
> That way, Junio (and anyone else applying the patch) has the
> flexibility to apply or cherry-pick this change to old branches
> without the --no-tags discussion and newer branches with it.
>
> Would you like to write it (or suggest wording), or would you prefer
> if someone else does?

I can do that no problem.

I just first wanted to clarify what the status of this was, from
SZEDER's comments in the referenced E-Mails I had the impression that
this was only meant for an old maintenance release:

    SZEDER: "I assume because, as a bugfix, it will be included in
    maintenance releases for older releases, and those won't have the
    '--no-tags' option."

But thinking about it I don't see why we'd be doing such minor doc
changes in an old maintenance release. I haven't read the whole backlog
of this topic though, so maybe I missed something.

I initially suggested just adding "Instead supply the --mirror and
--no-tags options, respectively" to the patch quoted above.

But actually, thinking about this again now, and being recently familiar
with this code after having implemented  on --no-tags, I think this
whole wording is just misleading. It makes it sound as though git clone
is init + fetch, and that the initial fetch just ignores these two
specific options because of some quirk of the implementation.

In reality clone doesn't use fetch at all, they just share some of the
underlying fetch machinery.

I think something like this as a replacement is better, assuming this
really needs to be applied to pre-2.13.0:

    diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
    index 35cc34b2fb..2169e5c97f 100644
    --- a/Documentation/git-clone.txt
    +++ b/Documentation/git-clone.txt
    @@ -189,6 +189,14 @@ 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.
    ++
    +The underlying implementation of `git clone` isn't equivalent to `git
    +init` followed by a `git fetch`, but the two share some of the
    +underlying fetch machinery. Because of this, setting configuration
    +variables which would impact `git fetch` doesn't have any effect on
    +`git clone` at all. For example, setting `remote.<name>.mirror` and
    +`remote.<name>.tagOpt` will do to change how the initial fetch is
    +carried out.

     --depth <depth>::
            Create a 'shallow' clone with a history truncated to the

Or, in case this just needs to be applied on top of master we can
mention --no-tags:

    diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
    index 83c8e9b394..52a371176e 100644
    --- a/Documentation/git-clone.txt
    +++ b/Documentation/git-clone.txt
    @@ -189,6 +189,16 @@ 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.
    ++
    +The underlying implementation of `git clone` isn't equivalent to `git
    +init` followed by a `git fetch`, but the two share some of the
    +underlying fetch machinery. Because of this, setting configuration
    +variables which would impact `git fetch` doesn't have any effect on
    +`git clone` at all.
    ++
    +For example, setting `remote.<name>.mirror` and `remote.<name>.tagOpt`
    +will do to change how the initial fetch is carried out. Instead supply
    +the --mirror and --no-tags options, respectively.

     --depth <depth>::
            Create a 'shallow' clone with a history truncated to the

Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

On Fri, Jun 16, 2017 at 11:10 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:

>     diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>     index 35cc34b2fb..2169e5c97f 100644
>     --- a/Documentation/git-clone.txt
>     +++ b/Documentation/git-clone.txt
>     @@ -189,6 +189,14 @@ 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.
>     ++
>     +The underlying implementation of `git clone` isn't equivalent to `git
>     +init` followed by a `git fetch`, but the two share some of the
>     +underlying fetch machinery.

I think this is unnecessarily technical and detailed to be included
under the docs of the '--config' option.

> Because of this, setting configuration
>     +variables which would impact `git fetch` doesn't have any effect on
>     +`git clone` at all.

This is not true, several such configuration variables work even
during cloning, see:

  http://public-inbox.org/git/CAM0VKjkSMnemwPbFihXiQui3wm_wYmQeQmgYrFs5bfsH1jMg1A@mail.gmail.com/

Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[Jonathan Nieder]

SZEDER Gábor wrote:

> Due to limitations/bugs 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'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-clone.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCHv5 2/2] Documentation/clone: document ignored configuration variables

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> Due to limitations/bugs 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'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-clone.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..5ceccb258 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,11 @@ 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 if the current implementation, some configuration

s/if/of/ (will squash in while queuing).

> +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`.
>  
>  --depth <depth>::
>  	Create a 'shallow' clone with a history truncated to the

[PATCH] remote: drop free_refspecs() function

[Jeff King]

On Tue, May 30, 2017 at 09:11:28AM +0200, SZEDER Gábor wrote:

> I also dropped Peff's two patches that were included in v3, because:
> 
>  - his last patch doesn't apply anymore, because the variable it frees
>    properly doesn't exist anymore, and
>  - with that patch gone his second patch is not tangled up with this
>    topic, so I think it should go on its own branch.

Makes sense on both counts. Here's that stand-alone patch for reference,
which can go onto its own topic.

-- >8 --
Subject: [PATCH] remote: drop free_refspecs() function

We already have free_refspec(), a public function which does
the same thing as the static free_refspecs(). Let's just
keep one.  There are two minor differences between the
functions:

  1. free_refspecs() is a noop when the refspec argument is
     NULL. This probably doesn't matter in practice.  The
     nr_refspec parameter would presumably be 0 in that
     case, skipping the loop. And free(NULL) is explicitly
     OK. But it doesn't hurt for us to port this extra
     safety to free_refspec(), as one of the callers passes
     a funny "i+1" count.

  2. The order of arguments is reversed between the two
     functions. This patch uses the already-public order of
     free_refspec(), as it matches the argument order on the
     parsing side.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/remote.c b/remote.c
index fdc52d802..2e39bf930 100644
--- a/remote.c
+++ b/remote.c
@@ -477,26 +477,6 @@ static void read_config(void)
 	alias_all_urls();
 }
 
-/*
- * This function frees a refspec array.
- * Warning: code paths should be checked to ensure that the src
- *          and dst pointers are always freeable pointers as well
- *          as the refspec pointer itself.
- */
-static void free_refspecs(struct refspec *refspec, int nr_refspec)
-{
-	int i;
-
-	if (!refspec)
-		return;
-
-	for (i = 0; i < nr_refspec; i++) {
-		free(refspec[i].src);
-		free(refspec[i].dst);
-	}
-	free(refspec);
-}
-
 static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)
 {
 	int i;
@@ -610,7 +590,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		 * since it is only possible to reach this point from within
 		 * the for loop above.
 		 */
-		free_refspecs(rs, i+1);
+		free_refspec(i+1, rs);
 		return NULL;
 	}
 	die("Invalid refspec '%s'", refspec[i]);
@@ -621,7 +601,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	struct refspec *refspec;
 
 	refspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);
-	free_refspecs(refspec, 1);
+	free_refspec(1, refspec);
 	return !!refspec;
 }
 
@@ -638,6 +618,10 @@ struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 void free_refspec(int nr_refspec, struct refspec *refspec)
 {
 	int i;
+
+	if (!refspec)
+		return;
+
 	for (i = 0; i < nr_refspec; i++) {
 		free(refspec[i].src);
 		free(refspec[i].dst);
-- 
2.13.0.676.ge5a8f17ed

Re: [PATCH] remote: drop free_refspecs() function

[Jonathan Nieder]

Jeff King wrote:

> Subject: [PATCH] remote: drop free_refspecs() function
>
> We already have free_refspec(), a public function which does
> the same thing as the static free_refspecs(). Let's just
> keep one.  There are two minor differences between the
> functions:
>
>   1. free_refspecs() is a noop when the refspec argument is
>      NULL. This probably doesn't matter in practice.  The
>      nr_refspec parameter would presumably be 0 in that
>      case, skipping the loop. And free(NULL) is explicitly
>      OK. But it doesn't hurt for us to port this extra
>      safety to free_refspec(), as one of the callers passes
>      a funny "i+1" count.
>
>   2. The order of arguments is reversed between the two
>      functions. This patch uses the already-public order of
>      free_refspec(), as it matches the argument order on the
>      parsing side.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  remote.c | 28 ++++++----------------------
>  1 file changed, 6 insertions(+), 22 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

[PATCHv3 2/4] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

Due to limitations/bugs 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'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d69..4f1e7d4ba 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -186,6 +186,10 @@ 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.
+	Note that due to limitations of the current implementation some
+	configuration variables don't take effect during the initial
+	fetch and checkout.  Configuration variables known to not take
+	effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.
 
 --depth <depth>::
 	Create a 'shallow' clone with a history truncated to the
-- 
2.13.0.35.g14b6294b1

Re: [PATCHv3 2/4] Documentation/clone: document ignored configuration variables

[SZEDER Gábor]

On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Due to limitations/bugs 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'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  Documentation/git-clone.txt | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..4f1e7d4ba 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,10 @@ 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.
> +       Note that due to limitations of the current implementation some
> +       configuration variables don't take effect during the initial
> +       fetch and checkout.  Configuration variables known to not take
> +       effect are: `remote.<name>.mirror` and `remote.<name>.tagOpt`.

A few notes to this patch, because I didn't like that "known to not
take effect" expression, and looked into how some configuration
variables are handled during the initial fetch and checkout.  Far from
comprehensive, but here they are anyway:

Concerning the initial fetch:

 - Configuration variables influencing the refspecs to be fetched are
   currently ignored.  These are the fetch refspecs, of course, and
   remote.<name>.{mirror,tagOpt}.
   This series makes fetch refspecs work.  The other two are mentioned
   in this patch, and are less urgent, because their functionality is
   or will soon be available through command line options (--mirror
   and --no-tags).
 - remote.<name>.url is strange.  It's not just ignored, it's ignored
   so much that it isn't even written to the config file when
   specified as 'git clone -c ...'.  Nonetheless, specifying it for
   'git clone' doesn't make much sense in the first place, does it?
   So I think it's actually good that it's ignored and it isn't worth
   mentioning it in the docs.
 - Some fetch-related config variables, e.g.
   remote.<name>.{prune,skipDefaultUpdate,skipFetchAll} or
   fetch.{prune,output}, don't matter during the initial fetch.
 - Other fetch-related config variables, e.g.
   fetch.{fsckObjects,unpackLimit}, are handled deep down in
   fetch-pack and work properly.
 - Transport-specific config variables, e.g. url.<base>.insteadOf,
   remote.<name>.{proxy,uploadpack,vcs}, or http.*, if applicable, are
   handled in the transport layer or remote helper.
 - I'm not sure about submodule-related config variables, but there
   are command line options for that.

I'm not sure about the initial checkout, in particular I'm not sure
how many configuration variables there are that could/should influence
the initial checkout (or any checkout, for that matter).

 - core.autocrlf works properly, we even have a test for that.
 - filter.<name>.smudge is read during initial checkout, but I'm not
   sure whether that should do anything, since no attributes files
   exist at that point.
 - I glanced through builtin/checkout.c to see whether it looks at any
   configuration variables that clone doesn't, and while it does so,
   it seems to only look at variables that are either irrelevant
   during the initial checkout, e.g. 'diff.ignoresubmodules' and
   'merge.conflictstyle', or are submodule-specific, and about those I
   have no idea.


Like I said, far from comprehensive, but I think at least the fetch
part is well covered.

[PATCHv3 3/4] remote: drop free_refspecs() function

[SZEDER Gábor]

From: Jeff King <peff@peff.net>

We already have free_refspec(), a public function which does
the same thing as the static free_refspecs(). Let's just
keep one.  There are two minor differences between the
functions:

  1. free_refspecs() is a noop when the refspec argument is
     NULL. This probably doesn't matter in practice.  The
     nr_refspec parameter would presumably be 0 in that
     case, skipping the loop. And free(NULL) is explicitly
     OK. But it doesn't hurt for us to port this extra
     safety to free_refspec(), as one of the callers passes
     a funny "i+1" count.

  2. The order of arguments is reversed between the two
     functions. This patch uses the already-public order of
     free_refspec(), as it matches the argument order on the
     parsing side.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/remote.c b/remote.c
index ad6c5424e..7918e0dac 100644
--- a/remote.c
+++ b/remote.c
@@ -473,26 +473,6 @@ static void read_config(void)
 	alias_all_urls();
 }
 
-/*
- * This function frees a refspec array.
- * Warning: code paths should be checked to ensure that the src
- *          and dst pointers are always freeable pointers as well
- *          as the refspec pointer itself.
- */
-static void free_refspecs(struct refspec *refspec, int nr_refspec)
-{
-	int i;
-
-	if (!refspec)
-		return;
-
-	for (i = 0; i < nr_refspec; i++) {
-		free(refspec[i].src);
-		free(refspec[i].dst);
-	}
-	free(refspec);
-}
-
 static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)
 {
 	int i;
@@ -606,7 +586,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp
 		 * since it is only possible to reach this point from within
 		 * the for loop above.
 		 */
-		free_refspecs(rs, i+1);
+		free_refspec(i+1, rs);
 		return NULL;
 	}
 	die("Invalid refspec '%s'", refspec[i]);
@@ -617,7 +597,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	struct refspec *refspec;
 
 	refspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);
-	free_refspecs(refspec, 1);
+	free_refspec(1, refspec);
 	return !!refspec;
 }
 
@@ -634,6 +614,10 @@ static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
 void free_refspec(int nr_refspec, struct refspec *refspec)
 {
 	int i;
+
+	if (!refspec)
+		return;
+
 	for (i = 0; i < nr_refspec; i++) {
 		free(refspec[i].src);
 		free(refspec[i].dst);
-- 
2.13.0.35.g14b6294b1

[PATCHv3 4/4] clone: use free_refspec() to free refspec list

[SZEDER Gábor]

From: Jeff King <peff@peff.net>

Using free() on a refspec was always leaky, as its string
fields also need freed. But it became more so when ad00f128d
(clone: respect configured fetch respecs during initial
fetch, 2016-03-30) taught clone to create a list of
refspecs, each of which need to be freed.

[sg: adjusted the function parameters.]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 4144190da..4bf28d7f5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&value);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	free(refspec);
+	free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);
 	return err;
 }
-- 
2.13.0.35.g14b6294b1

Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list

[SZEDER Gábor]

On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> From: Jeff King <peff@peff.net>
>
> Using free() on a refspec was always leaky, as its string
> fields also need freed. But it became more so when ad00f128d
> (clone: respect configured fetch respecs during initial
> fetch, 2016-03-30) taught clone to create a list of
> refspecs, each of which need to be freed.
>
> [sg: adjusted the function parameters.]
>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  builtin/clone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 4144190da..4bf28d7f5 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>         strbuf_release(&value);
>         junk_mode = JUNK_LEAVE_ALL;
>
> -       free(refspec);
> +       free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);
>         return err;
>  }

Erm...  I should have given a bit more thought to this last patch,
shouldn't I?

First, the unchanged commit message is now (i.e. by using the parsed
refspecs returned by remote_get()) completely outdated.
Second, while it properly frees those refspecs, i.e. the array and all
its string fields, it will now leak the memory pointed by the
'refspec' variable.  However, why free just that one field of the
'struct *remote'?  Alas, we don't seem to have a free_remote()
function...

Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list

[Jeff King]

On Mon, May 15, 2017 at 01:29:07PM +0200, SZEDER Gábor wrote:

> On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > From: Jeff King <peff@peff.net>
> >
> > Using free() on a refspec was always leaky, as its string
> > fields also need freed. But it became more so when ad00f128d
> > (clone: respect configured fetch respecs during initial
> > fetch, 2016-03-30) taught clone to create a list of
> > refspecs, each of which need to be freed.
> >
> > [sg: adjusted the function parameters.]
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> > ---
> >  builtin/clone.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 4144190da..4bf28d7f5 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >         strbuf_release(&value);
> >         junk_mode = JUNK_LEAVE_ALL;
> >
> > -       free(refspec);
> > +       free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);
> >         return err;
> >  }
> 
> Erm...  I should have given a bit more thought to this last patch,
> shouldn't I?
> 
> First, the unchanged commit message is now (i.e. by using the parsed
> refspecs returned by remote_get()) completely outdated.
> Second, while it properly frees those refspecs, i.e. the array and all
> its string fields, it will now leak the memory pointed by the
> 'refspec' variable.  However, why free just that one field of the
> 'struct *remote'?  Alas, we don't seem to have a free_remote()
> function...

Right, the "struct remote" fields are meant to last the program
lifetime, and just go away when the program does. So they are never
freed. We could just lump this in with them, and remove the free()
entirely.

It's a little funny with your patch as-is, because "struct remote"
doesn't actually know about the refspec we stuck on the end. But since
it doesn't free its refspecs anyway, it's not any more leaky than all
the other entries.

-Peff

Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> From: Jeff King <peff@peff.net>
>>
>> Using free() on a refspec was always leaky, as its string
>> fields also need freed. But it became more so when ad00f128d
>> (clone: respect configured fetch respecs during initial
>> fetch, 2016-03-30) taught clone to create a list of
>> refspecs, each of which need to be freed.
>>
>> [sg: adjusted the function parameters.]
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  builtin/clone.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 4144190da..4bf28d7f5 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -1120,6 +1120,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>         strbuf_release(&value);
>>         junk_mode = JUNK_LEAVE_ALL;
>>
>> -       free(refspec);
>> +       free_refspec(remote->fetch_refspec_nr + 1, remote->fetch);
>>         return err;
>>  }
>
> Erm...  I should have given a bit more thought to this last patch,
> shouldn't I?
>
> First, the unchanged commit message is now (i.e. by using the parsed
> refspecs returned by remote_get()) completely outdated.
> Second, while it properly frees those refspecs, i.e. the array and all
> its string fields, it will now leak the memory pointed by the
> 'refspec' variable.  However, why free just that one field of the
> 'struct *remote'?  Alas, we don't seem to have a free_remote()
> function...

I was sifting entries in the draft "What's cooking" report to find
topics to merge to 'next'.  I read the series over and as Peff said
in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I
think the series overall is good.

Do you want to update the proposed log message for this one before
the entire thing is merged to 'next'?

Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list

[Jeff King]

On Tue, May 23, 2017 at 04:38:07PM +0900, Junio C Hamano wrote:

> > First, the unchanged commit message is now (i.e. by using the parsed
> > refspecs returned by remote_get()) completely outdated.
> > Second, while it properly frees those refspecs, i.e. the array and all
> > its string fields, it will now leak the memory pointed by the
> > 'refspec' variable.  However, why free just that one field of the
> > 'struct *remote'?  Alas, we don't seem to have a free_remote()
> > function...
> 
> I was sifting entries in the draft "What's cooking" report to find
> topics to merge to 'next'.  I read the series over and as Peff said
> in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I
> think the series overall is good.
> 
> Do you want to update the proposed log message for this one before
> the entire thing is merged to 'next'?

I didn't quite say that it was good. I think the overall direction is
good, but I had some comments on patch 1. Maybe we want to accept the
ugliness there, but I had some suggestions that might make it less bad.

-Peff

Re: [PATCHv3 4/4] clone: use free_refspec() to free refspec list

[SZEDER Gábor]

On Tue, May 23, 2017 at 9:38 AM, Junio C Hamano <gitster@pobox.com> wrote:

> I was sifting entries in the draft "What's cooking" report to find
> topics to merge to 'next'.  I read the series over and as Peff said
> in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I
> think the series overall is good.
>
> Do you want to update the proposed log message for this one before
> the entire thing is merged to 'next'?

I followed Peff's advice on adding the default fetch refspec to this
temporary configuration environment (what's the proper term for
that?), which resulted in the simplest and cleanest code yet, and, as
a by product made this last patch moot, so I will drop it in the next
version.  The code is ready, but the change requires updates to the
first patch's commit message, maybe I can get around to it in the
evening.

Re: [PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch

[Jeff King]

On Mon, May 15, 2017 at 01:05:53PM +0200, SZEDER Gábor wrote:

> This is a reroll of sg/clone-refspec-from-command-line-config.
> Sorry for the delay, family visit.

No problem. Thanks for letting us know before it went to 'next'. ;)

> The first patch is the updated version of what is now the first commit
> of that topic.  The changes are those mentioned in [1]:
> 
>  - updated commit message,
>  - renamed 'refspec_count' to 'refspec_nr',

Good.

>  - use the parsed fetch refspecs returned by remote.c:remote_get()
>    instead of parsing them ourselves (look at the third hunk of the
>    diff of builtin/clone.c, how much shorter it looks),

Yeah, that is much nicer. It does feel a little dirty modifying
remote->fetch, though. I'll comment on the specific patch.

>  - modified tests to check that refs matching the default refspecs are
>    transferred as well, and
>  - added a test for the combination of '-c
>    remote.<remote>.fetch=<refspec> --origin=<name>'.

Sounds good.

> The second patch is a doc update to warn users that not all
> configuration variables are supported via 'git clone -c ...' at the
> moment.

Good idea.

> Patches 3 and 4 are the last two patches from Peff from this morning
> [2].  I picked those up, because his last patch required a bit of
> variable name adjustments.  I didn't pick up his first patch, because
> using remote_get() already factors out refspec parsing.

Makes sense. Thanks for including them.

-Peff