# [PATCH 0/3] cleanups on top of sg/clone-refspec-from-command-line-config 4 messages from 2017-05-15 to 2017-05-15. Participants: Jeff King. Thread: https://gitlist.dev/t/45960 ## Jeff King, 2017-05-15 07:46 Subject: [PATCH 0/3] cleanups on top of sg/clone-refspec-from-command-line-config Message-ID: <20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170515074617.wsdzogshc4ilnlsb%40sigill.intra.peff.net ``` 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, 2017-05-15 07:48 Subject: [PATCH 1/3] clone: factor out refspec parsing Message-ID: <20170515074805.lev667i37fgnycve@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170515074805.lev667i37fgnycve%40sigill.intra.peff.net In-Reply-To: <20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net> ``` 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 --- builtin/clone.c | 54 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 20 deletions(-) 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, 2017-05-15 07:49 Subject: [PATCH 2/3] remote: drop free_refspecs() function Message-ID: <20170515074926.q3rubolg6qtjz7dz@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170515074926.q3rubolg6qtjz7dz%40sigill.intra.peff.net In-Reply-To: <20170515074617.wsdzogshc4ilnlsb@sigill.intra.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 --- 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.458.g025404205 ``` ## Jeff King, 2017-05-15 07:50 Subject: [PATCH 3/3] clone: use free_refspec() to free refspec list Message-ID: <20170515075011.5pm6j3k3fwentm2o@sigill.intra.peff.net> URL: https://gitlist.dev/e/20170515075011.5pm6j3k3fwentm2o%40sigill.intra.peff.net In-Reply-To: <20170515074617.wsdzogshc4ilnlsb@sigill.intra.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. Signed-off-by: Jeff King --- 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(-) 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 ```