clone: respect configured fetch respecs during initial fetch

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/[email protected]/T/#u
[2] - http://public-inbox.org/git/[email protected]/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

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

---
 builtin/clone.c         | 20 +++++++++++++++-----
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 5 deletions(-)

index a35d62293..4144190da 100644
@@ -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

2.13.0.35.g14b6294b1

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

index 9c8912ab1..0881ed32c 100644
@@ -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);
+	}
+}

  strbuf_addf(&key, "remote.%s.fetch=%s", option_origin, refspec_pattern);
  git_config_push_parameter(key.buf);

Right.

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.

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

---
 builtin/clone.c         | 32 ++++++++++++++++----------------
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 16 deletions(-)

index a35d62293..40f4a327b 100644
@@ -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;
 }

2.13.0.35.g14b6294b1

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.

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

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.

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

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

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

index 4144190da..4157922d8 100644
@@ -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;

}

index ad6c5424e..b8fd09dc9 100644
@@ -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);

index 924881169..9ad8c1085 100644
@@ -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);

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

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

index a35d62293..4157922d8 100644
@@ -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;
 }

index ad6c5424e..b8fd09dc9 100644
@@ -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);

index 924881169..9ad8c1085 100644
@@ -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);
 

2.13.0.35.g14b6294b1

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

---
 Documentation/git-clone.txt | 4 ++++
 1 file changed, 4 insertions(+)

index ec41d3d69..4f1e7d4ba 100644
@@ -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

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.

Hi,

Makes sense.

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

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

Right, that would be a better check.

OK.

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

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.

Hi,

[...]

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.

[...]

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

[...]

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.

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

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.

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.

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

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

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

index 4f1e7d4ba..5ceccb258 100644
@@ -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

index 4157922d8..8a7edd4e5 100644
@@ -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);

index 114b53920..f240b22cc 100755
@@ -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

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

---
 builtin/clone.c         | 36 +++++++++++++++++++-----------------
 remote.c                | 13 +++++++++++++
 remote.h                |  1 +
 t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 17 deletions(-)

index a35d62293..8a7edd4e5 100644
@@ -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;
 }

index ad6c5424e..b8fd09dc9 100644
@@ -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);

index 924881169..9ad8c1085 100644
@@ -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);
 

2.13.1.505.g7cc9fcafb

Thanks.

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.

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

---
 Documentation/git-clone.txt | 5 +++++
 1 file changed, 5 insertions(+)

index ec41d3d69..5ceccb258 100644
@@ -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

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

[...]

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

On Fri, Jun 16 2017, Jonathan Nieder jotted:

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

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

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

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

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.

---
 remote.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

index fdc52d802..2e39bf930 100644
@@ -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

Thanks.

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

---
 Documentation/git-clone.txt | 4 ++++
 1 file changed, 4 insertions(+)

index ec41d3d69..4f1e7d4ba 100644
@@ -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

From: Jeff King <[email protected]>

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.

---
 remote.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

index ad6c5424e..7918e0dac 100644
@@ -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

From: Jeff King <[email protected]>

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

---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4144190da..4bf28d7f5 100644
@@ -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

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

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 <[email protected]>, 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

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.

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

Good.

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

Sounds good.

Good idea.

Makes sense. Thanks for including them.

-Peff