threads / patch / 45960

patch, 3 partscleanups on top of sg/clone-refspec-from-command-line-config

Subject: [PATCH 0/3] cleanups on top of sg/clone-refspec-from-command-line-config

## tl;dr

4 messages between May 15, 2017 and May 15, 2017. Diffs are folded; open one to read it.

replies: 3people: 1as markdown or json

Jeff King· May 15, 2017, 07:46 UTC · lore

These three patches replace the DONTMERGE at the tip of sg/clone-refspec-from-command-line-config. The first one is the refactoring I posted earlier (and which was in the DONTMERGE), with a commit message and signoff.

The third one fixes the minor leak I mentioned during the discussion. And the second fixes some puzzling code I found while trying to write the third one. :)

  [1/3]: clone: factor out refspec parsing
  [2/3]: remote: drop free_refspecs() function
  [3/3]: clone: use free_refspec() to free refspec list

 builtin/clone.c | 56 +++++++++++++++++++++++++++++++++++---------------------
 remote.c        | 28 ++++++----------------------
 2 files changed, 41 insertions(+), 43 deletions(-)

-Peff

Jeff King· May 15, 2017, 07:48 UTC · re: Jeff King · lore

[PATCH 1/3] clone: factor out refspec parsing

The cmd_clone function is rather long and uses many variables that are only valid or useful for a small part of the function. Let's pull the refspec parsing into a helper, which lets us remove some variables from the main function.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 54 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

Show changes to builtin/clone.c +34 −20

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

-- 
2.13.0.458.g025404205

Jeff King· May 15, 2017, 07:49 UTC · re: Jeff King · lore

[PATCH 2/3] 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(-)

Show changes to remote.c +6 −22

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

Jeff King· May 15, 2017, 07:50 UTC · re: Jeff King · lore

[PATCH 3/3] clone: use free_refspec() to free refspec list

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.

Signed-off-by: Jeff King <peff@peff.net>
---
It's doubtful anyone cares, as this is right before program exit anyway.
But if we're going to bother to free(), we may as well do it right.

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

Show changes to builtin/clone.c +1 −1

diff --git a/builtin/clone.c b/builtin/clone.c
index 645cfa4fd..672c5b0a6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1144,6 +1144,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	strbuf_release(&value);
 	junk_mode = JUNK_LEAVE_ALL;
 
-	free(refspec);
+	free_refspec(refspec_count, refspec);
 	return err;
 }

-- 
2.13.0.458.g025404205