{"thread":{"id":"45960","subject":"[PATCH 0/3] cleanups on top of sg/clone-refspec-from-command-line-config","startedAt":"2017-05-15T07:46:26Z","lastAt":"2017-05-15T07:50:23Z","messageCount":4,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"319760","messageId":"20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net","threadId":"45960","inReplyTo":null,"subject":"[PATCH 0/3] cleanups on top of sg/clone-refspec-from-command-line-config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T07:46:17Z","receivedAt":"2017-05-15T07:46:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These three patches replace the DONTMERGE at the tip of\nsg/clone-refspec-from-command-line-config. The first one is the\nrefactoring I posted earlier (and which was in the DONTMERGE), with a\ncommit message and signoff.\n\nThe third one fixes the minor leak I mentioned during the discussion.\nAnd the second fixes some puzzling code I found while trying to write\nthe third one. :)\n\n  [1/3]: clone: factor out refspec parsing\n  [2/3]: remote: drop free_refspecs() function\n  [3/3]: clone: use free_refspec() to free refspec list\n\n builtin/clone.c | 56 +++++++++++++++++++++++++++++++++++---------------------\n remote.c        | 28 ++++++----------------------\n 2 files changed, 41 insertions(+), 43 deletions(-)\n\n-Peff\n"},{"id":"319761","messageId":"20170515074805.lev667i37fgnycve@sigill.intra.peff.net","threadId":"45960","inReplyTo":"20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net","subject":"[PATCH 1/3] clone: factor out refspec parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T07:48:05Z","receivedAt":"2017-05-15T07:48:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The cmd_clone function is rather long and uses many\nvariables that are only valid or useful for a small part of\nthe function. Let's pull the refspec parsing into a helper,\nwhich lets us remove some variables from the main function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 54 ++++++++++++++++++++++++++++++++++--------------------\n 1 file changed, 34 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0630202c8..645cfa4fd 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -841,6 +841,37 @@ static void dissociate_from_references(void)\n \tfree(alternates);\n }\n \n+static struct refspec *get_clone_refspecs(const char *base_refspec,\n+\t\t\t\t\t  unsigned int *count)\n+{\n+\tchar *key;\n+\tconst struct string_list *config_fetch_patterns;\n+\tstruct refspec *ret;\n+\n+\tkey = xstrfmt(\"remote.%s.fetch\", option_origin);\n+\tconfig_fetch_patterns = git_config_get_value_multi(key);\n+\n+\tif (!config_fetch_patterns) {\n+\t\t*count = 1;\n+\t\tret = parse_fetch_refspec(1, &base_refspec);\n+\t} else {\n+\t\tconst char **fetch_patterns;\n+\t\tstruct string_list_item *fp;\n+\t\tunsigned int i = 1;\n+\n+\t\t*count = 1 + config_fetch_patterns->nr;\n+\t\tALLOC_ARRAY(fetch_patterns, *count);\n+\t\tfetch_patterns[0] = base_refspec;\n+\t\tfor_each_string_list_item(fp, config_fetch_patterns)\n+\t\t\tfetch_patterns[i++] = fp->string;\n+\t\tret = parse_fetch_refspec(*count, fetch_patterns);\n+\t\tfree(fetch_patterns);\n+\t}\n+\n+\tfree(key);\n+\treturn ret;\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n@@ -861,9 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint err = 0, complete_refs_before_fetch = 1;\n \n \tstruct refspec *refspec;\n-\tunsigned int refspec_count = 1;\n-\tconst char **fetch_patterns;\n-\tconst struct string_list *config_fetch_patterns;\n+\tunsigned int refspec_count;\n \n \tpacket_trace_identity(\"clone\");\n \targc = parse_options(argc, argv, prefix, builtin_clone_options,\n@@ -982,7 +1011,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_addf(&branch_top, \"refs/remotes/%s/\", option_origin);\n \t}\n \n-\tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n \tstrbuf_addf(&key, \"remote.%s.url\", option_origin);\n \tgit_config_set(key.buf, repo);\n \tstrbuf_reset(&key);\n@@ -990,21 +1018,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (option_reference.nr)\n \t\tsetup_reference();\n \n-\tstrbuf_addf(&key, \"remote.%s.fetch\", option_origin);\n-\tconfig_fetch_patterns = git_config_get_value_multi(key.buf);\n-\tif (config_fetch_patterns)\n-\t\trefspec_count = 1 + config_fetch_patterns->nr;\n-\tfetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));\n-\tfetch_patterns[0] = value.buf;\n-\tif (config_fetch_patterns) {\n-\t\tstruct string_list_item *fp;\n-\t\tunsigned int i = 1;\n-\t\tfor_each_string_list_item(fp, config_fetch_patterns)\n-\t\t\tfetch_patterns[i++] = fp->string;\n-\t}\n-\trefspec = parse_fetch_refspec(refspec_count, fetch_patterns);\n-\n-\tstrbuf_reset(&key);\n+\tstrbuf_addf(&value, \"+%s*:%s*\", src_ref_prefix, branch_top.buf);\n+\trefspec = get_clone_refspecs(value.buf, &refspec_count);\n \tstrbuf_reset(&value);\n \n \tremote = remote_get(option_origin);\n@@ -1129,7 +1144,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tfree(fetch_patterns);\n \tfree(refspec);\n \treturn err;\n }\n-- \n2.13.0.458.g025404205\n\n"},{"id":"319762","messageId":"20170515074926.q3rubolg6qtjz7dz@sigill.intra.peff.net","threadId":"45960","inReplyTo":"20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net","subject":"[PATCH 2/3] remote: drop free_refspecs() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T07:49:26Z","receivedAt":"2017-05-15T07:49:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We already have free_refspec(), a public function which does\nthe same thing as the static free_refspecs(). Let's just\nkeep one.  There are two minor differences between the\nfunctions:\n\n  1. free_refspecs() is a noop when the refspec argument is\n     NULL. This probably doesn't matter in practice.  The\n     nr_refspec parameter would presumably be 0 in that\n     case, skipping the loop. And free(NULL) is explicitly\n     OK. But it doesn't hurt for us to port this extra\n     safety to free_refspec(), as one of the callers passes\n     a funny \"i+1\" count.\n\n  2. The order of arguments is reversed between the two\n     functions. This patch uses the already-public order of\n     free_refspec(), as it matches the argument order on the\n     parsing side.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 28 ++++++----------------------\n 1 file changed, 6 insertions(+), 22 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424e..7918e0dac 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -473,26 +473,6 @@ static void read_config(void)\n \talias_all_urls();\n }\n \n-/*\n- * This function frees a refspec array.\n- * Warning: code paths should be checked to ensure that the src\n- *          and dst pointers are always freeable pointers as well\n- *          as the refspec pointer itself.\n- */\n-static void free_refspecs(struct refspec *refspec, int nr_refspec)\n-{\n-\tint i;\n-\n-\tif (!refspec)\n-\t\treturn;\n-\n-\tfor (i = 0; i < nr_refspec; i++) {\n-\t\tfree(refspec[i].src);\n-\t\tfree(refspec[i].dst);\n-\t}\n-\tfree(refspec);\n-}\n-\n static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)\n {\n \tint i;\n@@ -606,7 +586,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t * since it is only possible to reach this point from within\n \t\t * the for loop above.\n \t\t */\n-\t\tfree_refspecs(rs, i+1);\n+\t\tfree_refspec(i+1, rs);\n \t\treturn NULL;\n \t}\n \tdie(\"Invalid refspec '%s'\", refspec[i]);\n@@ -617,7 +597,7 @@ int valid_fetch_refspec(const char *fetch_refspec_str)\n \tstruct refspec *refspec;\n \n \trefspec = parse_refspec_internal(1, &fetch_refspec_str, 1, 1);\n-\tfree_refspecs(refspec, 1);\n+\tfree_refspec(1, refspec);\n \treturn !!refspec;\n }\n \n@@ -634,6 +614,10 @@ static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n void free_refspec(int nr_refspec, struct refspec *refspec)\n {\n \tint i;\n+\n+\tif (!refspec)\n+\t\treturn;\n+\n \tfor (i = 0; i < nr_refspec; i++) {\n \t\tfree(refspec[i].src);\n \t\tfree(refspec[i].dst);\n-- \n2.13.0.458.g025404205\n\n"},{"id":"319763","messageId":"20170515075011.5pm6j3k3fwentm2o@sigill.intra.peff.net","threadId":"45960","inReplyTo":"20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net","subject":"[PATCH 3/3] clone: use free_refspec() to free refspec list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-15T07:50:11Z","receivedAt":"2017-05-15T07:50:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using free() on a refspec was always leaky, as its string\nfields also need freed. But it became more so when ad00f128d\n(clone: respect configured fetch respecs during initial\nfetch, 2016-03-30) taught clone to create a list of\nrefspecs, each of which need to be freed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt's doubtful anyone cares, as this is right before program exit anyway.\nBut if we're going to bother to free(), we may as well do it right.\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 645cfa4fd..672c5b0a6 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1144,6 +1144,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstrbuf_release(&value);\n \tjunk_mode = JUNK_LEAVE_ALL;\n \n-\tfree(refspec);\n+\tfree_refspec(refspec_count, refspec);\n \treturn err;\n }\n-- \n2.13.0.458.g025404205\n"}]}