Re: git-clone --config order & fetching extra refs during initial clone

From

Jeff King <peff@peff.net>

Date

May 9, 2017, 02:26 UTC

Message-ID

<20170509022621.ku6pozb2zdeltz6o@sigill.intra.peff.net>

In-Reply-To

<20170509021028.fr5mc76kcbpnn4zs@sigill.intra.peff.net>

On Mon, May 08, 2017 at 10:10:28PM -0400, Jeff King wrote:

> On Tue, May 09, 2017 at 10:33:39AM +0900, Junio C Hamano wrote:
> 
> > >> My patch deals with 'remote.<name>.refspec', i.e. 'remote->fetch'.
> > >> Apparently some extra care is necessary for 'remote.<name>.tagOpt' and
> > >> 'remote->fetch_tags', too.  Perhaps there are more, I haven't checked
> > >> again, and maybe we'll add similar config variables in the future.  So
> > >> I don't think that dealing with such config variables one by one in
> > >> 'git clone', too, is the right long-term solution...  but perhaps it's
> > >> sufficient for the time being?
> > >
> > > I think your patch is a strict improvement and we don't need to hold up
> > > waiting for a perfect fix (and because of the --single-branch thing you
> > > mentioned, this may be the best we can do anyway).
> > 
> > OK, so where does this patch stand now?  It already is too late for
> > the upcoming release, but should we merge it to 'next' once the
> > release is made, cook it in 'next' and shoot for the next release
> > as-is, or do we want to allow minor tweaks before it hits 'next'?
> 
> I'd be OK with it as-is, but here are my nitpicks as a diff (keep the
> assignment of refspec_count in one place, and free fetch_patterns as
> soon as it is no longer used).
> 
> I actually think it might be nice to pull the whole thing out into its
> own function, but the parameters it takes would be oddly specific.

But I'm OK with any of Gábor's original, my earlier squash, or this one.

diff --git a/builtin/clone.c b/builtin/clone.c
index 0630202c8..645cfa4fd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -841,6 +841,37 @@ static void dissociate_from_references(void)
 	free(alternates);
 }
 
+static struct refspec *get_clone_refspecs(const char *base_refspec,
+					  unsigned int *count)
+{
+	char *key;
+	const struct string_list *config_fetch_patterns;
+	struct refspec *ret;
+
+	key = xstrfmt("remote.%s.fetch", option_origin);
+	config_fetch_patterns = git_config_get_value_multi(key);
+
+	if (!config_fetch_patterns) {
+		*count = 1;
+		ret = parse_fetch_refspec(1, &base_refspec);
+	} else {
+		const char **fetch_patterns;
+		struct string_list_item *fp;
+		unsigned int i = 1;
+
+		*count = 1 + config_fetch_patterns->nr;
+		ALLOC_ARRAY(fetch_patterns, *count);
+		fetch_patterns[0] = base_refspec;
+		for_each_string_list_item(fp, config_fetch_patterns)
+			fetch_patterns[i++] = fp->string;
+		ret = parse_fetch_refspec(*count, fetch_patterns);
+		free(fetch_patterns);
+	}
+
+	free(key);
+	return ret;
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
@@ -861,9 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int err = 0, complete_refs_before_fetch = 1;
 
 	struct refspec *refspec;
-	unsigned int refspec_count = 1;
-	const char **fetch_patterns;
-	const struct string_list *config_fetch_patterns;
+	unsigned int refspec_count;
 
 	packet_trace_identity("clone");
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
@@ -982,7 +1011,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);
@@ -990,21 +1018,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_reference.nr)
 		setup_reference();
 
-	strbuf_addf(&key, "remote.%s.fetch", option_origin);
-	config_fetch_patterns = git_config_get_value_multi(key.buf);
-	if (config_fetch_patterns)
-		refspec_count = 1 + config_fetch_patterns->nr;
-	fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));
-	fetch_patterns[0] = value.buf;
-	if (config_fetch_patterns) {
-		struct string_list_item *fp;
-		unsigned int i = 1;
-		for_each_string_list_item(fp, config_fetch_patterns)
-			fetch_patterns[i++] = fp->string;
-	}
-	refspec = parse_fetch_refspec(refspec_count, fetch_patterns);
-
-	strbuf_reset(&key);
+	strbuf_addf(&value, "+%s*:%s*", src_ref_prefix, branch_top.buf);
+	refspec = get_clone_refspecs(value.buf, &refspec_count);
 	strbuf_reset(&value);
 
 	remote = remote_get(option_origin);
@@ -1129,7 +1144,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&value);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	free(fetch_patterns);
 	free(refspec);
 	return err;
 }