{"thread":{"id":"56733","subject":"[PATCH] for-each-ref: delay parsing of --sort= options","startedAt":"2021-10-18T18:32:44Z","lastAt":"2021-10-21T14:54:38Z","messageCount":20,"participants":["Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"438996","messageId":"xmqqv91uw5dl.fsf@gitster.g","threadId":"56733","inReplyTo":null,"subject":"[PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-18T18:32:38Z","receivedAt":"2021-10-18T18:32:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The for-each-ref family of commands invoke parsers immediately when\nit sees each --sort= option, and die before even seeing the\nother options on the command line when the is unrecognised.\n\nInstead, accumulate them in a string list, and have them parsed into\na ref_sorting structure after the command line parsing is done. As\na consequence, \"git branch --sort=bogus -h\" used to fail to give the\nbrief help, which arguably may have been a feature, now does so,\nwhich is more consistent with how other options work.\n\nThe patch is smaller than the actual extent of the \"damage\" to the\ncodebase, thanks to the fact that the original code consistently\nused OPT_REF_SORT() macro to handle command line options. We only\nneeded to replace the variable used for the list, and implementation\nof the callback function used in the macro.\n\nThe old rule was for the users of the API to:\n\n - Declare ref_sorting and ref_sorting_tail variables;\n\n - OPT_REF_SORT() macro will instantiate ref_sorting instance (which\n may barf and die) and append it to the tail;\n\n - Append to the tail each ref_sorting read from the configuration\n by parsing in the config callback (which may barf and die);\n\n - See if ref_sorting is null and use ref_sorting_default() instead.\n\nNow the rule is not all that different but is simpler:\n\n - Declare ref_sorting_options string list.\n\n - OPT_REF_SORT() macro will append it to the string list;\n\n - Append to the string list the sort key read from the\n configuration;\n\n - call ref_sorting_options() to turn the string list to ref_sorting\n structure (which also deals with the default value).\n\nAs side effects, this change also cleans up a few issues:\n\n - 95be717c (parse_opt_ref_sorting: always use with NONEG flag,\n 2019-03-20) muses that \"git for-each-ref --no-sort\" should simply\n clear the sort keys accumulated so far; it now does.\n\n - The implementation detail of \"struct ref_sorting\" and the helper\n function parse_ref_sorting() can now be private to the ref-filter\n API implementation.\n\n - If you set branch.sort to a bogus value, the any \"git branch\"\n invocation, not only the listing mode, would abort with the\n original code; now it doesn't\n\nSigned-off-by: Junio C Hamano \n---\n builtin/branch.c | 13 ++++++-------\n builtin/for-each-ref.c | 8 ++++----\n builtin/ls-remote.c | 13 ++++++++-----\n builtin/tag.c | 13 ++++++-------\n ref-filter.c | 34 ++++++++++++++++++++++++----------\n ref-filter.h | 28 ++++++++++------------------\n t/t3200-branch.sh | 12 +++++++++++-\n t/t6300-for-each-ref.sh | 5 +++++\n 8 files changed, 74 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0b7ed82654..69211f8dd1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -77,12 +77,11 @@ define_list_config_array(color_branch_slots);\n static int git_branch_config(const char *var, const char *value, void *cb)\n {\n \tconst char *slot_name;\n-\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"branch.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_ref_sorting(sorting_tail, value);\n+\t\tstring_list_append(cb, value);\n \t\treturn 0;\n \t}\n \n@@ -624,7 +623,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tenum branch_track track;\n \tstruct ref_filter filter;\n \tint icase = 0;\n-\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstatic struct ref_sorting *sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \n \tstruct option options[] = {\n@@ -665,7 +665,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_MERGED(&filter, N_(\"print only branches that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only branches that are not merged\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only branches of the object\"), parse_opt_object_name),\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n@@ -682,7 +682,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_branch_usage, options);\n \n-\tgit_config(git_branch_config, sorting_tail);\n+\tgit_config(git_branch_config, &sorting_options);\n \n \ttrack = git_branch_track;\n \n@@ -748,8 +748,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t * local branches 'refs/heads/...' and finally remote-tracking\n \t\t * branches 'refs/remotes/...'.\n \t\t */\n-\t\tif (!sorting)\n-\t\t\tsorting = ref_default_sorting();\n+\t\tsorting = ref_sorting_options(&sorting_options);\n \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \t\tref_sorting_set_sort_flags_all(\n \t\t\tsorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 642b4b888f..75d277ed60 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -17,7 +17,8 @@ static char const * const for_each_ref_usage[] = {\n int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstruct ref_sorting *sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tint maxcount = 0, icase = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n@@ -39,7 +40,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n \t\tOPT_STRING( 0 , \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT__COLOR(&format.use_color, N_(\"respect format colors\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at,\n \t\t\t N_(\"object\"), N_(\"print only refs which points at the given object\"),\n \t\t\t parse_opt_object_name),\n@@ -70,8 +71,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (verify_ref_format(&format))\n \t\tusage_with_options(for_each_ref_usage, opts);\n \n-\tif (!sorting)\n-\t\tsorting = ref_default_sorting();\n+\tsorting = ref_sorting_options(&sorting_options);\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 318949c3d7..1e6017cdaa 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -54,7 +54,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \tstruct transport *transport;\n \tconst struct ref *ref;\n \tstruct ref_array ref_array;\n-\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"do not print remote URL\")),\n@@ -68,7 +68,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"refs\", &flags, N_(\"do not show peeled tags\"), REF_NORMAL),\n \t\tOPT_BOOL(0, \"get-url\", &get_url,\n \t\t\t N_(\"take url..insteadOf into account\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\tOPT_SET_INT_F(0, \"exit-code\", &status,\n \t\t\t N_(\"exit with exit code 2 if no matching refs are found\"),\n \t\t\t 2, PARSE_OPT_NOCOMPLETE),\n@@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"ls-remote\");\n \n-\tUNLEAK(sorting);\n-\n \tif (argc > 1) {\n \t\tint i;\n \t\tCALLOC_ARRAY(pattern, argc);\n@@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\titem->symref = xstrdup_or_null(ref->symref);\n \t}\n \n-\tif (sorting)\n+\tif (sorting_options.nr) {\n+\t\tstruct ref_sorting *sorting;\n+\t\tUNLEAK(sorting);\n+\n+\t\tsorting = ref_sorting_options(&sorting_options);\n \t\tref_array_sort(sorting, &ref_array);\n+\t}\n \n \tfor (i = 0; i < ref_array.nr; i++) {\n \t\tconst struct ref_array_item *ref = ref_array.items[i];\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 6535ed27ee..7b03cddd29 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -178,7 +178,6 @@ static const char tag_template_nocleanup[] =\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status;\n-\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"tag.gpgsign\")) {\n \t\tconfig_sign_tag = git_config_bool(var, value);\n@@ -188,7 +187,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_ref_sorting(sorting_tail, value);\n+\t\tstring_list_append(cb, value);\n \t\treturn 0;\n \t}\n \n@@ -436,7 +435,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n-\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstruct ref_sorting *sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \tint icase = 0;\n \tint edit_flag = 0;\n@@ -470,7 +470,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_WITHOUT(&filter.no_commit, N_(\"print only tags that don't contain the commit\")),\n \t\tOPT_MERGED(&filter, N_(\"print only tags that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only tags that are not merged\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), PARSE_OPT_LASTARG_DEFAULT,\n@@ -485,7 +485,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \tsetup_ref_filter_porcelain_msg();\n \n-\tgit_config(git_tag_config, sorting_tail);\n+\tgit_config(git_tag_config, &sorting_options);\n \n \tmemset(&opt, 0, sizeof(opt));\n \tmemset(&filter, 0, sizeof(filter));\n@@ -524,8 +524,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--column and -n are incompatible\"));\n \t\tcolopts = 0;\n \t}\n-\tif (!sorting)\n-\t\tsorting = ref_default_sorting();\n+\tsorting = ref_sorting_options(&sorting_options);\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \tif (cmdmode == 'l') {\ndiff --git a/ref-filter.c b/ref-filter.c\nindex add429be79..fc80abd432 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2470,6 +2470,12 @@ static int memcasecmp(const void *vs1, const void *vs2, size_t n)\n \treturn 0;\n }\n \n+struct ref_sorting {\n+\tstruct ref_sorting *next;\n+\tint atom; /* index into used_atom array (internal) */\n+\tenum ref_sorting_order sort_flags;\n+};\n+\n static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)\n {\n \tstruct atom_value *va, *vb;\n@@ -2663,7 +2669,7 @@ static int parse_sorting_atom(const char *atom)\n }\n \n /* If no sorting option is given, use refname to sort as default */\n-struct ref_sorting *ref_default_sorting(void)\n+static struct ref_sorting *ref_default_sorting(void)\n {\n \tstatic const char cstr_name[] = \"refname\";\n \n@@ -2674,7 +2680,7 @@ struct ref_sorting *ref_default_sorting(void)\n \treturn sorting;\n }\n \n-void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n+static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n {\n \tstruct ref_sorting *s;\n \n@@ -2692,17 +2698,25 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n \ts->atom = parse_sorting_atom(arg);\n }\n \n-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n+struct ref_sorting *ref_sorting_options(struct string_list *options)\n {\n+\tstruct string_list_item *item;\n+\tstruct ref_sorting *sorting = NULL, **tail = &sorting;\n+\n+\tif (!options->nr) {\n+\t\tsorting = ref_default_sorting();\n+\t} else {\n+\t\tfor_each_string_list_item(item, options)\n+\t\t\tparse_ref_sorting(tail, item->string);\n+\t}\n+\n \t/*\n-\t * NEEDSWORK: We should probably clear the list in this case, but we've\n-\t * already munged the global used_atoms list, which would need to be\n-\t * undone.\n+\t * From here on, the ref_sorting list should be used to talk\n+\t * about the sort order used for the output. The caller\n+\t * should not touch the string form anymore.\n \t */\n-\tBUG_ON_OPT_NEG(unset);\n-\n-\tparse_ref_sorting(opt->value, arg);\n-\treturn 0;\n+\tstring_list_clear(options, 0);\n+\treturn sorting;\n }\n \n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex b636f4389d..706c90a18f 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,16 +23,13 @@\n #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n struct atom_value;\n+struct ref_sorting;\n \n-struct ref_sorting {\n-\tstruct ref_sorting *next;\n-\tint atom; /* index into used_atom array (internal) */\n-\tenum {\n-\t\tREF_SORTING_REVERSE = 1<<0,\n-\t\tREF_SORTING_ICASE = 1<<1,\n-\t\tREF_SORTING_VERSION = 1<<2,\n-\t\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n-\t} sort_flags;\n+enum ref_sorting_order {\n+\tREF_SORTING_REVERSE = 1<<0,\n+\tREF_SORTING_ICASE = 1<<1,\n+\tREF_SORTING_VERSION = 1<<2,\n+\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n };\n \n struct ref_array_item {\n@@ -97,9 +94,8 @@ struct ref_format {\n #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED(\"no-merged\", f, h)\n \n #define OPT_REF_SORT(var) \\\n-\tOPT_CALLBACK_F(0, \"sort\", (var), \\\n-\t\t N_(\"key\"), N_(\"field name to sort on\"), \\\n-\t\t PARSE_OPT_NONEG, parse_opt_ref_sorting)\n+\tOPT_STRING_LIST(0, \"sort\", (var), \\\n+\t\t\tN_(\"key\"), N_(\"field name to sort on\"))\n \n /*\n * API for filtering a set of refs. Based on the type of refs the user\n@@ -121,12 +117,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\t struct ref_format *format,\n \t\t\t struct strbuf *final_buf,\n \t\t\t struct strbuf *error_buf);\n-/* Parse a single sort specifier and add it to the list */\n-void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n-/* Callback function for parsing the sort option */\n-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n-/* Default sort option based on refname */\n-struct ref_sorting *ref_default_sorting(void);\n+/* Convert list of sort options into ref_sorting */\n+struct ref_sorting *ref_sorting_options(struct string_list *);\n /* Function to parse --merged and --no-merged options */\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);\n /* Get the current HEAD's description */\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ff..0e23d18807 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1418,7 +1418,17 @@ test_expect_success 'invalid sort parameter in configuration' '\n \t(\n \t\tcd sort &&\n \t\tgit config branch.sort \"v:notvalid\" &&\n-\t\ttest_must_fail git branch\n+\n+\t\t# this works in the \"listing\" mode, so bad sort key\n+\t\t# is a dying offence.\n+\t\ttest_must_fail git branch &&\n+\n+\t\t# these do not need to use sorting, and should all\n+\t\t# succeed\n+\t\tgit branch newone main &&\n+\t\tgit branch -c newone newerone &&\n+\t\tgit branch -m newone newestone &&\n+\t\tgit branch -d newerone newestone\n \t)\n '\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 80679d5e12..83921502a4 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -419,6 +419,11 @@ test_expect_success 'Verify descending sort' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'Give help even with invalid sort atoms' '\n+\ttest_expect_code 129 git for-each-ref --sort=bogus -h >actual 2>&1 &&\n+\tgrep \"^usage: git for-each-ref\" actual\n+'\n+\n cat >expected <<\\EOF\n refs/tags/testtag\n refs/tags/testtag-2\n-- \n2.33.1-878-gcde26e7e77\n\n"},{"id":"439047","messageId":"YW9EP5UNX0f+eOke@coredump.intra.peff.net","threadId":"56733","inReplyTo":"xmqqv91uw5dl.fsf@gitster.g","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-19T22:18:39Z","receivedAt":"2021-10-19T22:18:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 18, 2021 at 11:32:38AM -0700, Junio C Hamano wrote:\n\n> The for-each-ref family of commands invoke parsers immediately when\n> it sees each --sort= option, and die before even seeing the\n> other options on the command line when the is unrecognised.\n> \n> Instead, accumulate them in a string list, and have them parsed into\n> a ref_sorting structure after the command line parsing is done. As\n> a consequence, \"git branch --sort=bogus -h\" used to fail to give the\n> brief help, which arguably may have been a feature, now does so,\n> which is more consistent with how other options work.\n> [...]\n\nVery nicely explained. As you often say, I wish all patches came with a\ncommit message like this. :)\n\n> As side effects, this change also cleans up a few issues:\n> \n> - 95be717c (parse_opt_ref_sorting: always use with NONEG flag,\n> 2019-03-20) muses that \"git for-each-ref --no-sort\" should simply\n> clear the sort keys accumulated so far; it now does.\n\nNeat. Is it worth adding a test here?\n\n> - The implementation detail of \"struct ref_sorting\" and the helper\n> function parse_ref_sorting() can now be private to the ref-filter\n> API implementation.\n\nYes, I think this makes the API overall cleaner. Including the fact that\nref_sorting_default() is now handled internally.\n\n> - If you set branch.sort to a bogus value, the any \"git branch\"\n> invocation, not only the listing mode, would abort with the\n> original code; now it doesn't\n\nAnd the new tests nicely cover these cases.\n\n> builtin/branch.c | 13 ++++++-------\n> builtin/for-each-ref.c | 8 ++++----\n> builtin/ls-remote.c | 13 ++++++++-----\n> builtin/tag.c | 13 ++++++-------\n> ref-filter.c | 34 ++++++++++++++++++++++++----------\n> ref-filter.h | 28 ++++++++++------------------\n> t/t3200-branch.sh | 12 +++++++++++-\n> t/t6300-for-each-ref.sh | 5 +++++\n> 8 files changed, 74 insertions(+), 52 deletions(-)\n\nThe patch overall looks good to me. Just a few observations:\n\n> @@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \n> \tpacket_trace_identity(\"ls-remote\");\n> \n> -\tUNLEAK(sorting);\n> -\n> \tif (argc > 1) {\n> \t\tint i;\n> \t\tCALLOC_ARRAY(pattern, argc);\n> @@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \t\titem->symref = xstrdup_or_null(ref->symref);\n> \t}\n> \n> -\tif (sorting)\n> +\tif (sorting_options.nr) {\n> +\t\tstruct ref_sorting *sorting;\n> +\t\tUNLEAK(sorting);\n> +\n> +\t\tsorting = ref_sorting_options(&sorting_options);\n> \t\tref_array_sort(sorting, &ref_array);\n> +\t}\n\nI wondered at first about pulling this UNLEAK() down, but it's because\nyou move the \"sorting\" variable itself into the smaller scope. So this\nmakes sense (and calling UNLEAK() before the pointer is set is perfectly\nfine, since it takes the address of the auto variable). It is a shame\nyou can't just ref_sorting_free() afterwards, but we don't have that\nfunction yet. And adding it is way out of scope here. :)\n\nI do find it interesting that this case checks sorting_options.nr\nitself, rather than relying on ref_sorting_options() to give us the\ndefault. But that's because the existing code avoids sorting at all in\nthat case, so this is staying faithful to the original.\n\n> -int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n> +struct ref_sorting *ref_sorting_options(struct string_list *options)\n> {\n> +\tstruct string_list_item *item;\n> +\tstruct ref_sorting *sorting = NULL, **tail = &sorting;\n> +\n> +\tif (!options->nr) {\n> +\t\tsorting = ref_default_sorting();\n> +\t} else {\n> +\t\tfor_each_string_list_item(item, options)\n> +\t\t\tparse_ref_sorting(tail, item->string);\n> +\t}\n> +\n> \t/*\n> -\t * NEEDSWORK: We should probably clear the list in this case, but we've\n> -\t * already munged the global used_atoms list, which would need to be\n> -\t * undone.\n> +\t * From here on, the ref_sorting list should be used to talk\n> +\t * about the sort order used for the output. The caller\n> +\t * should not touch the string form anymore.\n> \t */\n> -\tBUG_ON_OPT_NEG(unset);\n> -\n> -\tparse_ref_sorting(opt->value, arg);\n> -\treturn 0;\n> +\tstring_list_clear(options, 0);\n> +\treturn sorting;\n> }\n\nThis interface seems pretty sensible overall. A few idle thoughts:\n\n - I'd probably have kept the word \"parse\" somewhere in the name, since\n it really is turning the user-provided text into our internal form\n\n - using a string_list is probably reasonable. We don't care about the\n util field here. Taking a \"const char **\" with NULL-termination or a\n number would be more general (and the callers could use a strvec to\n collect it). But I doubt any of the callers would ever be care about\n the distinction.\n\n - clearing the list at the end feels a little funny to me, just\n because this is conceptually a read-only operation (parse the user's\n text into our internal format). Your comment tells me what you're\n trying to protect against, but I find it unlikely anybody would\n mis-use the string_list afterwards (it doesn't do anything itself\n unless you parse it into the ref_sorting struct).\n\n All of the current callers are happy with this (and it even saves\n them clearing it themselves), but it just feels like an unusual\n interface.\n\n> -struct ref_sorting {\n> -\tstruct ref_sorting *next;\n> -\tint atom; /* index into used_atom array (internal) */\n> -\tenum {\n> -\t\tREF_SORTING_REVERSE = 1<<0,\n> -\t\tREF_SORTING_ICASE = 1<<1,\n> -\t\tREF_SORTING_VERSION = 1<<2,\n> -\t\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n> -\t} sort_flags;\n> +enum ref_sorting_order {\n> +\tREF_SORTING_REVERSE = 1<<0,\n> +\tREF_SORTING_ICASE = 1<<1,\n> +\tREF_SORTING_VERSION = 1<<2,\n> +\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n> };\n\nOK, so we have to leave this order enum public because some of the\ncallers use it with ref_sorting_set_sort_flags_all(). I was confused\nthat it never had a name before, but it looks like we just passed it as\nan \"unsigned int\".\n\nWe could change that now that it has a name, but it's not a big deal\neither way (and certainly a separate patch if we do).\n\n> @@ -97,9 +94,8 @@ struct ref_format {\n> #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED(\"no-merged\", f, h)\n> \n> #define OPT_REF_SORT(var) \\\n> -\tOPT_CALLBACK_F(0, \"sort\", (var), \\\n> -\t\t N_(\"key\"), N_(\"field name to sort on\"), \\\n> -\t\t PARSE_OPT_NONEG, parse_opt_ref_sorting)\n> +\tOPT_STRING_LIST(0, \"sort\", (var), \\\n> +\t\t\tN_(\"key\"), N_(\"field name to sort on\"))\n\nOh, this part makes using a string_list more appealing. ;)\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index e575ffb4ff..0e23d18807 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -1418,7 +1418,17 @@ test_expect_success 'invalid sort parameter in configuration' '\n> \t(\n> \t\tcd sort &&\n> \t\tgit config branch.sort \"v:notvalid\" &&\n> -\t\ttest_must_fail git branch\n> +\n> +\t\t# this works in the \"listing\" mode, so bad sort key\n> +\t\t# is a dying offence.\n> +\t\ttest_must_fail git branch &&\n> +\n> +\t\t# these do not need to use sorting, and should all\n> +\t\t# succeed\n> +\t\tgit branch newone main &&\n> +\t\tgit branch -c newone newerone &&\n> +\t\tgit branch -m newone newestone &&\n> +\t\tgit branch -d newerone newestone\n> \t)\n> '\n\nAnd this test looks very nice.\n\n-Peff\n"},{"id":"439082","messageId":"YW98+Lj9xVsR9u9Q@coredump.intra.peff.net","threadId":"56733","inReplyTo":"YW9EP5UNX0f+eOke@coredump.intra.peff.net","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-20T02:20:40Z","receivedAt":"2021-10-20T02:20:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 19, 2021 at 06:18:40PM -0400, Jeff King wrote:\n\n> > @@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> > \n> > \tpacket_trace_identity(\"ls-remote\");\n> > \n> > -\tUNLEAK(sorting);\n> > -\n> > \tif (argc > 1) {\n> > \t\tint i;\n> > \t\tCALLOC_ARRAY(pattern, argc);\n> > @@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> > \t\titem->symref = xstrdup_or_null(ref->symref);\n> > \t}\n> > \n> > -\tif (sorting)\n> > +\tif (sorting_options.nr) {\n> > +\t\tstruct ref_sorting *sorting;\n> > +\t\tUNLEAK(sorting);\n> > +\n> > +\t\tsorting = ref_sorting_options(&sorting_options);\n> > \t\tref_array_sort(sorting, &ref_array);\n> > +\t}\n> \n> I wondered at first about pulling this UNLEAK() down, but it's because\n> you move the \"sorting\" variable itself into the smaller scope. So this\n> makes sense (and calling UNLEAK() before the pointer is set is perfectly\n> fine, since it takes the address of the auto variable). It is a shame\n> you can't just ref_sorting_free() afterwards, but we don't have that\n> function yet. And adding it is way out of scope here. :)\n\nActually, I think I was wrong here. UNLEAK() will look at &sorting, but\nit will snapshot its data at the time of the call. So it won't do\nanything when the variable doesn't yet have a value.\n\nYou can demonstrate with:\n\n $ make SANITIZE=leak\n $ ./git ls-remote --sort=refname .\n\nwhich will complain. Bumping it down like this:\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 1e6017cdaa..a94a220256 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -139,10 +139,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \n \tif (sorting_options.nr) {\n \t\tstruct ref_sorting *sorting;\n-\t\tUNLEAK(sorting);\n \n \t\tsorting = ref_sorting_options(&sorting_options);\n \t\tref_array_sort(sorting, &ref_array);\n+\t\tUNLEAK(sorting);\n \t}\n \n \tfor (i = 0; i < ref_array.nr; i++) {\n\nclears it up. Note that there are other similar \"leaks\" (e.g., if you\ngive a pattern in argv[1]) which should be punted to another topic, but\nI think you'd want to deal with this one since you're moving the\nUNLEAK() around.\n\n-Peff\n"},{"id":"439096","messageId":"211020.864k9boo0f.gmgdl@evledraar.gmail.com","threadId":"56733","inReplyTo":"YW98+Lj9xVsR9u9Q@coredump.intra.peff.net","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T12:30:42Z","receivedAt":"2021-10-20T12:54:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 19 2021, Jeff King wrote:\n\n> On Tue, Oct 19, 2021 at 06:18:40PM -0400, Jeff King wrote:\n>\n>> > @@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n>> > \n>> > \tpacket_trace_identity(\"ls-remote\");\n>> > \n>> > -\tUNLEAK(sorting);\n>> > -\n>> > \tif (argc > 1) {\n>> > \t\tint i;\n>> > \t\tCALLOC_ARRAY(pattern, argc);\n>> > @@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n>> > \t\titem->symref = xstrdup_or_null(ref->symref);\n>> > \t}\n>> > \n>> > -\tif (sorting)\n>> > +\tif (sorting_options.nr) {\n>> > +\t\tstruct ref_sorting *sorting;\n>> > +\t\tUNLEAK(sorting);\n>> > +\n>> > +\t\tsorting = ref_sorting_options(&sorting_options);\n>> > \t\tref_array_sort(sorting, &ref_array);\n>> > +\t}\n>> \n>> I wondered at first about pulling this UNLEAK() down, but it's because\n>> you move the \"sorting\" variable itself into the smaller scope. So this\n>> makes sense (and calling UNLEAK() before the pointer is set is perfectly\n>> fine, since it takes the address of the auto variable). It is a shame\n>> you can't just ref_sorting_free() afterwards, but we don't have that\n>> function yet. And adding it is way out of scope here. :)\n>\n> Actually, I think I was wrong here. UNLEAK() will look at &sorting, but\n> it will snapshot its data at the time of the call. So it won't do\n> anything when the variable doesn't yet have a value.\n>\n> You can demonstrate with:\n>\n> $ make SANITIZE=leak\n> $ ./git ls-remote --sort=refname .\n>\n> which will complain. Bumping it down like this:\n>\n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index 1e6017cdaa..a94a220256 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -139,10 +139,10 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n> \n> \tif (sorting_options.nr) {\n> \t\tstruct ref_sorting *sorting;\n> -\t\tUNLEAK(sorting);\n> \n> \t\tsorting = ref_sorting_options(&sorting_options);\n> \t\tref_array_sort(sorting, &ref_array);\n> +\t\tUNLEAK(sorting);\n> \t}\n> \n> \tfor (i = 0; i < ref_array.nr; i++) {\n>\n> clears it up. Note that there are other similar \"leaks\" (e.g., if you\n> give a pattern in argv[1]) which should be punted to another topic, but\n> I think you'd want to deal with this one since you're moving the\n> UNLEAK() around.\n>\n> -Peff\n\nWith or without your change Junio's patch still makes t0016-oidmap.sh\nfail when applied on top of master under SANITIZE=leak, it passed\nbefore:\n \n =================================================================\n ==3448774==ERROR: LeakSanitizer: detected memory leaks\n \n Direct leak of 16 byte(s) in 1 object(s) allocated from:\n #0 0x7ff5c1c8ab45 in __interceptor_calloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:76\n #1 0x5614d26ac4e5 in xcalloc /home/avar/g/git/wrapper.c:140\n #2 0x5614d26186b0 in ref_default_sorting /home/avar/g/git/ref-filter.c:2676\n #3 0x5614d26187c9 in ref_sorting_options /home/avar/g/git/ref-filter.c:2707\n #4 0x5614d24e21c3 in cmd_tag builtin/tag.c:527\n #5 0x5614d2407a89 in run_builtin /home/avar/g/git/git.c:461\n #6 0x5614d2407e98 in handle_builtin /home/avar/g/git/git.c:713\n #7 0x5614d2408105 in run_argv /home/avar/g/git/git.c:780\n #8 0x5614d24085ae in cmd_main /home/avar/g/git/git.c:911\n #9 0x5614d24ef3d8 in main /home/avar/g/git/common-main.c:52\n #10 0x7ff5c1a06d09 in __libc_start_main ../csu/libc-start.c:308\n\nI.e. the leak from ref_sorting_options() via ref_default_sorting() is\nnew in this commit for those codepaths.\n \n"},{"id":"439097","messageId":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","threadId":"56733","inReplyTo":"211020.864k9boo0f.gmgdl@evledraar.gmail.com","subject":"[RFC PATCH v2 0/4] for-each-ref: delay parsing of --sort= options + linux-leaks fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T13:24:32Z","receivedAt":"2021-10-20T13:24:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in <211020.864k9boo0f.gmgdl@evledraar.gmail.com> the upthread\npatch breaks the linux-leaks job, since simple invocations of \"git\ntag\" start leaking memory: https://github.com/git/git/runs/3934925278\n\nI tried to structure this as somethnig that could be merged or queued\nup separately, but the semantic conflict in \"branch.c\" was difficult,\nas Junio's patch changes how those variables are managed.\n\nJunio C Hamano (1):\n for-each-ref: delay parsing of --sort= options\n\nÆvar Arnfjörð Bjarmason (3):\n tag: use a \"goto cleanup\" pattern, leak less memory\n ref-filter API user: add and use a ref_sorting_release()\n branch: use ref_sorting_release()\n\n builtin/branch.c | 24 ++++++++++++-----------\n builtin/for-each-ref.c | 10 +++++-----\n builtin/ls-remote.c | 13 ++++++++-----\n builtin/tag.c | 42 ++++++++++++++++++++++-------------------\n ref-filter.c | 40 ++++++++++++++++++++++++++++++---------\n ref-filter.h | 30 ++++++++++++-----------------\n t/t3200-branch.sh | 12 +++++++++++-\n t/t6300-for-each-ref.sh | 5 +++++\n 8 files changed, 108 insertions(+), 68 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: fc776c3f1cd tag: use a \"goto cleanup\" pattern, leak less memory\n-: ----------- > 2: 0ae71c19ab7 ref-filter API user: add and use a ref_sorting_release()\n1: 21a1f4d3b08 ! 3: 7abbbe4468c for-each-ref: delay parsing of --sort= options\n @@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *\n -\tif (sorting)\n +\tif (sorting_options.nr) {\n +\t\tstruct ref_sorting *sorting;\n -+\t\tUNLEAK(sorting);\n +\n +\t\tsorting = ref_sorting_options(&sorting_options);\n \t\tref_array_sort(sorting, &ref_array);\n ++\t\tref_sorting_release(sorting);\n +\t}\n \n \tfor (i = 0; i < ref_array.nr; i++) {\n @@ ref-filter.c: void parse_ref_sorting(struct ref_sorting **sorting_tail, const ch\n +\treturn sorting;\n }\n \n - int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n + void ref_sorting_release(struct ref_sorting *sorting)\n \n ## ref-filter.h ##\n @@\n @@ ref-filter.h: int format_ref_array_item(struct ref_array_item *info,\n -struct ref_sorting *ref_default_sorting(void);\n +/* Convert list of sort options into ref_sorting */\n +struct ref_sorting *ref_sorting_options(struct string_list *);\n + /* Release a \"struct ref_sorting\" */\n + void ref_sorting_release(struct ref_sorting *);\n /* Function to parse --merged and --no-merged options */\n - int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);\n - /* Get the current HEAD's description */\n \n ## t/t3200-branch.sh ##\n @@ t/t3200-branch.sh: test_expect_success 'invalid sort parameter in configuration' '\n-: ----------- > 4: f7d87aea384 branch: use ref_sorting_release()\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439098","messageId":"RFC-patch-v2-1.4-fc776c3f1cd-20211020T131627Z-avarab@gmail.com","threadId":"56733","inReplyTo":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","subject":"[RFC PATCH v2 1/4] tag: use a \"goto cleanup\" pattern, leak less memory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T13:24:33Z","receivedAt":"2021-10-20T13:24:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change cmd_tag() to free its \"struct string_list\"'s instead of using\nan UNLEAK() pattern. This changes code added in 886e1084d78 (builtin/:\nadd UNLEAKs, 2017-10-01).\n\nAs shown in the context of the deceleration of the \"struct\nmsg_arg\" (which I'm changing to use a designated initializer while at\nit, and to show the context in this change), that struct is just a\nthin wrapper around an int and \"struct strbuf\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/tag.c | 28 ++++++++++++++++------------\n 1 file changed, 16 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 6535ed27ee9..ad6c9855914 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -432,7 +432,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tint annotate = 0, force = 0;\n \tint cmdmode = 0, create_tag_object = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\n-\tstruct msg_arg msg = { 0, STRBUF_INIT };\n+\tstruct msg_arg msg = { .buf = STRBUF_INIT };\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n@@ -482,6 +482,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n \t\tOPT_END()\n \t};\n+\tint ret = 0;\n \n \tsetup_ref_filter_porcelain_msg();\n \n@@ -529,7 +530,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \tif (cmdmode == 'l') {\n-\t\tint ret;\n \t\tif (column_active(colopts)) {\n \t\t\tstruct column_options copts;\n \t\t\tmemset(&copts, 0, sizeof(copts));\n@@ -540,7 +540,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tret = list_tags(&filter, sorting, &format);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \t}\n \tif (filter.lines != -1)\n \t\tdie(_(\"-n option is only allowed in list mode\"));\n@@ -552,12 +552,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n \tif (filter.reachable_from || filter.unreachable_from)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n-\tif (cmdmode == 'd')\n-\t\treturn delete_tags(argv);\n+\tif (cmdmode == 'd') {\n+\t\tret = delete_tags(argv);\n+\t\tgoto cleanup;\n+\t}\n \tif (cmdmode == 'v') {\n \t\tif (format.format && verify_ref_format(&format))\n \t\t\tusage_with_options(git_tag_usage, options);\n-\t\treturn for_each_tag_name(argv, verify_tag, &format);\n+\t\tret = for_each_tag_name(argv, verify_tag, &format);\n+\t\tgoto cleanup;\n \t}\n \n \tif (msg.given || msgfile) {\n@@ -626,10 +629,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\n \t\t find_unique_abbrev(&prev, DEFAULT_ABBREV));\n \n-\tUNLEAK(buf);\n-\tUNLEAK(ref);\n-\tUNLEAK(reflog_msg);\n-\tUNLEAK(msg);\n-\tUNLEAK(err);\n-\treturn 0;\n+cleanup:\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&ref);\n+\tstrbuf_release(&reflog_msg);\n+\tstrbuf_release(&msg.buf);\n+\tstrbuf_release(&err);\n+\treturn ret;\n }\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439099","messageId":"RFC-patch-v2-2.4-0ae71c19ab7-20211020T131627Z-avarab@gmail.com","threadId":"56733","inReplyTo":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","subject":"[RFC PATCH v2 2/4] ref-filter API user: add and use a ref_sorting_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T13:24:34Z","receivedAt":"2021-10-20T13:24:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a ref_sorting_release() and use it for some of the current API\nusers, the ref_sorting_default() function and its siblings will do a\nmalloc() which wasn't being free'd previously.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 1 +\n ref-filter.c | 8 ++++++++\n ref-filter.h | 2 ++\n 4 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 642b4b888fb..16a2c7d57ca 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -96,6 +96,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tref_array_clear(&array);\n \tfree_commit_list(filter.with_commit);\n \tfree_commit_list(filter.no_commit);\n-\tUNLEAK(sorting);\n+\tref_sorting_release(sorting);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ad6c9855914..6fe646710d6 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -630,6 +630,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t find_unique_abbrev(&prev, DEFAULT_ABBREV));\n \n cleanup:\n+\tref_sorting_release(sorting);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&ref);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex add429be797..bdf8b0725df 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2705,6 +2705,14 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+void ref_sorting_release(struct ref_sorting *sorting)\n+{\n+\tstruct ref_sorting *next = sorting->next;\n+\tif (next)\n+\t ref_sorting_release(next);\n+\tfree(sorting);\n+}\n+\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n {\n \tstruct ref_filter *rf = opt->value;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex b636f4389d0..6228458d306 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -127,6 +127,8 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n /* Default sort option based on refname */\n struct ref_sorting *ref_default_sorting(void);\n+/* Release a \"struct ref_sorting\" */\n+void ref_sorting_release(struct ref_sorting *);\n /* Function to parse --merged and --no-merged options */\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);\n /* Get the current HEAD's description */\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439100","messageId":"RFC-patch-v2-3.4-7abbbe4468c-20211020T131627Z-avarab@gmail.com","threadId":"56733","inReplyTo":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","subject":"[RFC PATCH v2 3/4] for-each-ref: delay parsing of --sort= options","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T13:24:35Z","receivedAt":"2021-10-20T13:24:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"From: Junio C Hamano \n\nThe for-each-ref family of commands invoke parsers immediately when\nit sees each --sort= option, and die before even seeing the\nother options on the command line when the is unrecognised.\n\nInstead, accumulate them in a string list, and have them parsed into\na ref_sorting structure after the command line parsing is done. As\na consequence, \"git branch --sort=bogus -h\" used to fail to give the\nbrief help, which arguably may have been a feature, now does so,\nwhich is more consistent with how other options work.\n\nThe patch is smaller than the actual extent of the \"damage\" to the\ncodebase, thanks to the fact that the original code consistently\nused OPT_REF_SORT() macro to handle command line options. We only\nneeded to replace the variable used for the list, and implementation\nof the callback function used in the macro.\n\nThe old rule was for the users of the API to:\n\n - Declare ref_sorting and ref_sorting_tail variables;\n\n - OPT_REF_SORT() macro will instantiate ref_sorting instance (which\n may barf and die) and append it to the tail;\n\n - Append to the tail each ref_sorting read from the configuration\n by parsing in the config callback (which may barf and die);\n\n - See if ref_sorting is null and use ref_sorting_default() instead.\n\nNow the rule is not all that different but is simpler:\n\n - Declare ref_sorting_options string list.\n\n - OPT_REF_SORT() macro will append it to the string list;\n\n - Append to the string list the sort key read from the\n configuration;\n\n - call ref_sorting_options() to turn the string list to ref_sorting\n structure (which also deals with the default value).\n\nAs side effects, this change also cleans up a few issues:\n\n - 95be717c (parse_opt_ref_sorting: always use with NONEG flag,\n 2019-03-20) muses that \"git for-each-ref --no-sort\" should simply\n clear the sort keys accumulated so far; it now does.\n\n - The implementation detail of \"struct ref_sorting\" and the helper\n function parse_ref_sorting() can now be private to the ref-filter\n API implementation.\n\n - If you set branch.sort to a bogus value, the any \"git branch\"\n invocation, not only the listing mode, would abort with the\n original code; now it doesn't\n\nSigned-off-by: Junio C Hamano \n---\n builtin/branch.c | 13 ++++++-------\n builtin/for-each-ref.c | 8 ++++----\n builtin/ls-remote.c | 13 ++++++++-----\n builtin/tag.c | 13 ++++++-------\n ref-filter.c | 34 ++++++++++++++++++++++++----------\n ref-filter.h | 28 ++++++++++------------------\n t/t3200-branch.sh | 12 +++++++++++-\n t/t6300-for-each-ref.sh | 5 +++++\n 8 files changed, 74 insertions(+), 52 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0b7ed82654a..69211f8dd1c 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -77,12 +77,11 @@ define_list_config_array(color_branch_slots);\n static int git_branch_config(const char *var, const char *value, void *cb)\n {\n \tconst char *slot_name;\n-\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"branch.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_ref_sorting(sorting_tail, value);\n+\t\tstring_list_append(cb, value);\n \t\treturn 0;\n \t}\n \n@@ -624,7 +623,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tenum branch_track track;\n \tstruct ref_filter filter;\n \tint icase = 0;\n-\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstatic struct ref_sorting *sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \n \tstruct option options[] = {\n@@ -665,7 +665,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_MERGED(&filter, N_(\"print only branches that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only branches that are not merged\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only branches of the object\"), parse_opt_object_name),\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n@@ -682,7 +682,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_branch_usage, options);\n \n-\tgit_config(git_branch_config, sorting_tail);\n+\tgit_config(git_branch_config, &sorting_options);\n \n \ttrack = git_branch_track;\n \n@@ -748,8 +748,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t * local branches 'refs/heads/...' and finally remote-tracking\n \t\t * branches 'refs/remotes/...'.\n \t\t */\n-\t\tif (!sorting)\n-\t\t\tsorting = ref_default_sorting();\n+\t\tsorting = ref_sorting_options(&sorting_options);\n \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \t\tref_sorting_set_sort_flags_all(\n \t\t\tsorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 16a2c7d57ca..6f62f40d126 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -17,7 +17,8 @@ static char const * const for_each_ref_usage[] = {\n int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tstruct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstruct ref_sorting *sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tint maxcount = 0, icase = 0;\n \tstruct ref_array array;\n \tstruct ref_filter filter;\n@@ -39,7 +40,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \t\tOPT_INTEGER( 0 , \"count\", &maxcount, N_(\"show only matched refs\")),\n \t\tOPT_STRING( 0 , \"format\", &format.format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT__COLOR(&format.use_color, N_(\"respect format colors\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\tOPT_CALLBACK(0, \"points-at\", &filter.points_at,\n \t\t\t N_(\"object\"), N_(\"print only refs which points at the given object\"),\n \t\t\t parse_opt_object_name),\n@@ -70,8 +71,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tif (verify_ref_format(&format))\n \t\tusage_with_options(for_each_ref_usage, opts);\n \n-\tif (!sorting)\n-\t\tsorting = ref_default_sorting();\n+\tsorting = ref_sorting_options(&sorting_options);\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 318949c3d75..44448fa61d1 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -54,7 +54,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \tstruct transport *transport;\n \tconst struct ref *ref;\n \tstruct ref_array ref_array;\n-\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"do not print remote URL\")),\n@@ -68,7 +68,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"refs\", &flags, N_(\"do not show peeled tags\"), REF_NORMAL),\n \t\tOPT_BOOL(0, \"get-url\", &get_url,\n \t\t\t N_(\"take url..insteadOf into account\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\tOPT_SET_INT_F(0, \"exit-code\", &status,\n \t\t\t N_(\"exit with exit code 2 if no matching refs are found\"),\n \t\t\t 2, PARSE_OPT_NOCOMPLETE),\n@@ -86,8 +86,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \n \tpacket_trace_identity(\"ls-remote\");\n \n-\tUNLEAK(sorting);\n-\n \tif (argc > 1) {\n \t\tint i;\n \t\tCALLOC_ARRAY(pattern, argc);\n@@ -139,8 +137,13 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)\n \t\titem->symref = xstrdup_or_null(ref->symref);\n \t}\n \n-\tif (sorting)\n+\tif (sorting_options.nr) {\n+\t\tstruct ref_sorting *sorting;\n+\n+\t\tsorting = ref_sorting_options(&sorting_options);\n \t\tref_array_sort(sorting, &ref_array);\n+\t\tref_sorting_release(sorting);\n+\t}\n \n \tfor (i = 0; i < ref_array.nr; i++) {\n \t\tconst struct ref_array_item *ref = ref_array.items[i];\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 6fe646710d6..41863c5ab77 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -178,7 +178,6 @@ static const char tag_template_nocleanup[] =\n static int git_tag_config(const char *var, const char *value, void *cb)\n {\n \tint status;\n-\tstruct ref_sorting **sorting_tail = (struct ref_sorting **)cb;\n \n \tif (!strcmp(var, \"tag.gpgsign\")) {\n \t\tconfig_sign_tag = git_config_bool(var, value);\n@@ -188,7 +187,7 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"tag.sort\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\tparse_ref_sorting(sorting_tail, value);\n+\t\tstring_list_append(cb, value);\n \t\treturn 0;\n \t}\n \n@@ -436,7 +435,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n-\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tstruct ref_sorting *sorting;\n+\tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \tint icase = 0;\n \tint edit_flag = 0;\n@@ -470,7 +470,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_WITHOUT(&filter.no_commit, N_(\"print only tags that don't contain the commit\")),\n \t\tOPT_MERGED(&filter, N_(\"print only tags that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only tags that are not merged\")),\n-\t\tOPT_REF_SORT(sorting_tail),\n+\t\tOPT_REF_SORT(&sorting_options),\n \t\t{\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only tags of the object\"), PARSE_OPT_LASTARG_DEFAULT,\n@@ -486,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \tsetup_ref_filter_porcelain_msg();\n \n-\tgit_config(git_tag_config, sorting_tail);\n+\tgit_config(git_tag_config, &sorting_options);\n \n \tmemset(&opt, 0, sizeof(opt));\n \tmemset(&filter, 0, sizeof(filter));\n@@ -525,8 +525,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--column and -n are incompatible\"));\n \t\tcolopts = 0;\n \t}\n-\tif (!sorting)\n-\t\tsorting = ref_default_sorting();\n+\tsorting = ref_sorting_options(&sorting_options);\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \tif (cmdmode == 'l') {\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bdf8b0725df..ec7f30c5d31 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2470,6 +2470,12 @@ static int memcasecmp(const void *vs1, const void *vs2, size_t n)\n \treturn 0;\n }\n \n+struct ref_sorting {\n+\tstruct ref_sorting *next;\n+\tint atom; /* index into used_atom array (internal) */\n+\tenum ref_sorting_order sort_flags;\n+};\n+\n static int cmp_ref_sorting(struct ref_sorting *s, struct ref_array_item *a, struct ref_array_item *b)\n {\n \tstruct atom_value *va, *vb;\n@@ -2663,7 +2669,7 @@ static int parse_sorting_atom(const char *atom)\n }\n \n /* If no sorting option is given, use refname to sort as default */\n-struct ref_sorting *ref_default_sorting(void)\n+static struct ref_sorting *ref_default_sorting(void)\n {\n \tstatic const char cstr_name[] = \"refname\";\n \n@@ -2674,7 +2680,7 @@ struct ref_sorting *ref_default_sorting(void)\n \treturn sorting;\n }\n \n-void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n+static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n {\n \tstruct ref_sorting *s;\n \n@@ -2692,17 +2698,25 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n \ts->atom = parse_sorting_atom(arg);\n }\n \n-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n+struct ref_sorting *ref_sorting_options(struct string_list *options)\n {\n+\tstruct string_list_item *item;\n+\tstruct ref_sorting *sorting = NULL, **tail = &sorting;\n+\n+\tif (!options->nr) {\n+\t\tsorting = ref_default_sorting();\n+\t} else {\n+\t\tfor_each_string_list_item(item, options)\n+\t\t\tparse_ref_sorting(tail, item->string);\n+\t}\n+\n \t/*\n-\t * NEEDSWORK: We should probably clear the list in this case, but we've\n-\t * already munged the global used_atoms list, which would need to be\n-\t * undone.\n+\t * From here on, the ref_sorting list should be used to talk\n+\t * about the sort order used for the output. The caller\n+\t * should not touch the string form anymore.\n \t */\n-\tBUG_ON_OPT_NEG(unset);\n-\n-\tparse_ref_sorting(opt->value, arg);\n-\treturn 0;\n+\tstring_list_clear(options, 0);\n+\treturn sorting;\n }\n \n void ref_sorting_release(struct ref_sorting *sorting)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 6228458d306..b6a317e92c5 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -23,16 +23,13 @@\n #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD)\n \n struct atom_value;\n+struct ref_sorting;\n \n-struct ref_sorting {\n-\tstruct ref_sorting *next;\n-\tint atom; /* index into used_atom array (internal) */\n-\tenum {\n-\t\tREF_SORTING_REVERSE = 1<<0,\n-\t\tREF_SORTING_ICASE = 1<<1,\n-\t\tREF_SORTING_VERSION = 1<<2,\n-\t\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n-\t} sort_flags;\n+enum ref_sorting_order {\n+\tREF_SORTING_REVERSE = 1<<0,\n+\tREF_SORTING_ICASE = 1<<1,\n+\tREF_SORTING_VERSION = 1<<2,\n+\tREF_SORTING_DETACHED_HEAD_FIRST = 1<<3,\n };\n \n struct ref_array_item {\n@@ -97,9 +94,8 @@ struct ref_format {\n #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED(\"no-merged\", f, h)\n \n #define OPT_REF_SORT(var) \\\n-\tOPT_CALLBACK_F(0, \"sort\", (var), \\\n-\t\t N_(\"key\"), N_(\"field name to sort on\"), \\\n-\t\t PARSE_OPT_NONEG, parse_opt_ref_sorting)\n+\tOPT_STRING_LIST(0, \"sort\", (var), \\\n+\t\t\tN_(\"key\"), N_(\"field name to sort on\"))\n \n /*\n * API for filtering a set of refs. Based on the type of refs the user\n@@ -121,12 +117,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t\t struct ref_format *format,\n \t\t\t struct strbuf *final_buf,\n \t\t\t struct strbuf *error_buf);\n-/* Parse a single sort specifier and add it to the list */\n-void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n-/* Callback function for parsing the sort option */\n-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n-/* Default sort option based on refname */\n-struct ref_sorting *ref_default_sorting(void);\n+/* Convert list of sort options into ref_sorting */\n+struct ref_sorting *ref_sorting_options(struct string_list *);\n /* Release a \"struct ref_sorting\" */\n void ref_sorting_release(struct ref_sorting *);\n /* Function to parse --merged and --no-merged options */\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e575ffb4ffb..0e23d18807b 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1418,7 +1418,17 @@ test_expect_success 'invalid sort parameter in configuration' '\n \t(\n \t\tcd sort &&\n \t\tgit config branch.sort \"v:notvalid\" &&\n-\t\ttest_must_fail git branch\n+\n+\t\t# this works in the \"listing\" mode, so bad sort key\n+\t\t# is a dying offence.\n+\t\ttest_must_fail git branch &&\n+\n+\t\t# these do not need to use sorting, and should all\n+\t\t# succeed\n+\t\tgit branch newone main &&\n+\t\tgit branch -c newone newerone &&\n+\t\tgit branch -m newone newestone &&\n+\t\tgit branch -d newerone newestone\n \t)\n '\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 80679d5e12d..83921502a40 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -419,6 +419,11 @@ test_expect_success 'Verify descending sort' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'Give help even with invalid sort atoms' '\n+\ttest_expect_code 129 git for-each-ref --sort=bogus -h >actual 2>&1 &&\n+\tgrep \"^usage: git for-each-ref\" actual\n+'\n+\n cat >expected <<\\EOF\n refs/tags/testtag\n refs/tags/testtag-2\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439101","messageId":"RFC-patch-v2-4.4-f7d87aea384-20211020T131627Z-avarab@gmail.com","threadId":"56733","inReplyTo":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","subject":"[RFC PATCH v2 4/4] branch: use ref_sorting_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T13:24:36Z","receivedAt":"2021-10-20T13:24:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use a ref_sorting_release() in branch.c to free the memory from the\nref_sorting_options(). This plugs the final in-tree memory leak of\nthat API.\n\nIn the preceding commit the \"sorting\" variable was left in the\ncmd_branch() scope, even though that wasn't needed anymore. Move it to\nthe \"else if (list)\" scope instead. We can also move the \"struct\nstring_list\" only used for that branch to be declared in that block\n\nThat \"struct ref_sorting\" does not need to be \"static\" (and isn't\nre-used). The \"ref_sorting_options()\" will return a valid one, we\ndon't need to make it \"static\" to have it zero'd out. That it was\nstatic was another artifact of the pre-image of the preceding commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 69211f8dd1c..3963c592f56 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -69,7 +69,6 @@ static const char *color_branch_slots[] = {\n \t[BRANCH_COLOR_WORKTREE] = \"worktree\",\n };\n \n-static struct string_list output = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n \n define_list_config_array(color_branch_slots);\n@@ -406,7 +405,8 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r\n \treturn strbuf_detach(&fmt, NULL);\n }\n \n-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format)\n+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting,\n+\t\t\t struct ref_format *format, struct string_list *output)\n {\n \tint i;\n \tstruct ref_array array;\n@@ -448,7 +448,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\tif (column_active(colopts)) {\n \t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n \t\t\t /* format to a string_list to let print_columns() do its job */\n-\t\t\tstring_list_append(&output, out.buf);\n+\t\t\tstring_list_append(output, out.buf);\n \t\t} else {\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n@@ -623,7 +623,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tenum branch_track track;\n \tstruct ref_filter filter;\n \tint icase = 0;\n-\tstatic struct ref_sorting *sorting;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \tstruct ref_format format = REF_FORMAT_INIT;\n \n@@ -737,6 +736,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tprint_current_branch_name();\n \t\treturn 0;\n \t} else if (list) {\n+\t\tstruct ref_sorting *sorting;\n+\t\tstatic struct string_list output = STRING_LIST_INIT_DUP;\n+\n \t\t/* git branch --list also shows HEAD when it is detached */\n \t\tif ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)\n \t\t\tfilter.kind |= FILTER_REFS_DETACHED_HEAD;\n@@ -752,9 +754,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \t\tref_sorting_set_sort_flags_all(\n \t\t\tsorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);\n-\t\tprint_ref_list(&filter, sorting, &format);\n+\t\tprint_ref_list(&filter, sorting, &format, &output);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n+\t\tref_sorting_release(sorting);\n \t\treturn 0;\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439103","messageId":"xmqq4k9bssr7.fsf@gitster.g","threadId":"56733","inReplyTo":"YW9EP5UNX0f+eOke@coredump.intra.peff.net","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T13:58:04Z","receivedAt":"2021-10-20T13:58:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> As side effects, this change also cleans up a few issues:\n>> \n>> - 95be717c (parse_opt_ref_sorting: always use with NONEG flag,\n>> 2019-03-20) muses that \"git for-each-ref --no-sort\" should simply\n>> clear the sort keys accumulated so far; it now does.\n>\n> Neat. Is it worth adding a test here?\n\nIt probably is. The feature lets you defeat the configured personal\ndefault, if I understand the code right, which is probably a good\nthing.\n\nI think that the command line option is cumulative on top of\nconfigured values, with or without this change, and I think that\nqualifies as a bug to be fixed. E.g. with a command line option\n\n $ git -c branch.sort=-committerdate branch --sort=subject\n\nany configured sort keys should be cleared and the branches ought to\nbe sorted solely on their subject string, but I think the code with\nor without the patch still uses the \"-committerdate\" as a secondary\nkey to tiebreak sorting by \"subject\".\n\nUntil that bug is fixed, using --no-sort as the first command line\noption before the true --sort= option(s) you want to use would\nbe a workaround.\n\nHowever, it is tricky to arrange, as the command already takes\nmultiple --sort keys, and the laster ones are taken as more\nsignificant sort order, so it is tricky to come up with two keys A\nand B such that --sort=A --no-sort --sort=B will produce one order,\nwhile --sort=A --sort=B will produce another different order.\n\n>> +\tif (sorting_options.nr) {\n>> +\t\tstruct ref_sorting *sorting;\n>> +\t\tUNLEAK(sorting);\n>> +\n>> +\t\tsorting = ref_sorting_options(&sorting_options);\n>> \t\tref_array_sort(sorting, &ref_array);\n>> +\t}\n>\n> I wondered at first about pulling this UNLEAK() down, but it's because\n> you move the \"sorting\" variable itself into the smaller scope. So this\n> makes sense (and calling UNLEAK() before the pointer is set is perfectly\n> fine, since it takes the address of the auto variable). It is a shame\n> you can't just ref_sorting_free() afterwards, but we don't have that\n> function yet. And adding it is way out of scope here. :)\n>\n> I do find it interesting that this case checks sorting_options.nr\n> itself, rather than relying on ref_sorting_options() to give us the\n> default. But that's because the existing code avoids sorting at all in\n> that case, so this is staying faithful to the original.\n\nOne thing that is somewhat scary was that with all the other\nchanges, but without the changes to builtin/ls-remote.c file, the\nresulting tree still _compiles_ without any warning and only\nsegfaults at runtime. Since this does not use the \"if nothing is\nspecified, use the default\", I didn't even find it as a candidate\nfor conversion before seeing the tests to fail. This is an oddball\ncase.\n\n> - I'd probably have kept the word \"parse\" somewhere in the name, since\n> it really is turning the user-provided text into our internal form\n\nPerhaps.\n\n> - clearing the list at the end feels a little funny to me, just\n> because this is conceptually a read-only operation (parse the user's\n> text into our internal format). Your comment tells me what you're\n> trying to protect against, but I find it unlikely anybody would\n> mis-use the string_list afterwards (it doesn't do anything itself\n> unless you parse it into the ref_sorting struct).\n>\n> All of the current callers are happy with this (and it even saves\n> them clearing it themselves), but it just feels like an unusual\n> interface.\n\nYes. The story the comment gives is an officially sounding lame\nexcuse; the true motivation was that I was too lazy to repeat\nwriting resource deallocation for each caller and made the callee to\ndo the freeing ;-)\n\n>> @@ -97,9 +94,8 @@ struct ref_format {\n>> #define OPT_NO_MERGED(f, h) _OPT_MERGED_NO_MERGED(\"no-merged\", f, h)\n>> \n>> #define OPT_REF_SORT(var) \\\n>> -\tOPT_CALLBACK_F(0, \"sort\", (var), \\\n>> -\t\t N_(\"key\"), N_(\"field name to sort on\"), \\\n>> -\t\t PARSE_OPT_NONEG, parse_opt_ref_sorting)\n>> +\tOPT_STRING_LIST(0, \"sort\", (var), \\\n>> +\t\t\tN_(\"key\"), N_(\"field name to sort on\"))\n>\n> Oh, this part makes using a string_list more appealing. ;)\n\nYes.\n"},{"id":"439109","messageId":"xmqqwnm7rcds.fsf@gitster.g","threadId":"56733","inReplyTo":"RFC-patch-v2-1.4-fc776c3f1cd-20211020T131627Z-avarab@gmail.com","subject":"Re: [RFC PATCH v2 1/4] tag: use a \"goto cleanup\" pattern, leak less memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T14:37:03Z","receivedAt":"2021-10-20T14:37:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change cmd_tag() to free its \"struct string_list\"'s instead of using\n\nstring_list -> strbuf\n\n> an UNLEAK() pattern. This changes code added in 886e1084d78 (builtin/:\n> add UNLEAKs, 2017-10-01).\n>\n> As shown in the context of the deceleration of the \"struct\n\ndeceleration?\n\n> msg_arg\" (which I'm changing to use a designated initializer while at\n> it, and to show the context in this change), that struct is just a\n> thin wrapper around an int and \"struct strbuf\".\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/tag.c | 28 ++++++++++++++++------------\n> 1 file changed, 16 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 6535ed27ee9..ad6c9855914 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -432,7 +432,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \tint annotate = 0, force = 0;\n> \tint cmdmode = 0, create_tag_object = 0;\n> \tconst char *msgfile = NULL, *keyid = NULL;\n> -\tstruct msg_arg msg = { 0, STRBUF_INIT };\n> +\tstruct msg_arg msg = { .buf = STRBUF_INIT };\n> \tstruct ref_transaction *transaction;\n> \tstruct strbuf err = STRBUF_INIT;\n> \tstruct ref_filter filter;\n> @@ -482,6 +482,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n> \t\tOPT_END()\n> \t};\n> +\tint ret = 0;\n> \n> \tsetup_ref_filter_porcelain_msg();\n> \n> @@ -529,7 +530,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n> \tfilter.ignore_case = icase;\n> \tif (cmdmode == 'l') {\n> -\t\tint ret;\n> \t\tif (column_active(colopts)) {\n> \t\t\tstruct column_options copts;\n> \t\t\tmemset(&copts, 0, sizeof(copts));\n> @@ -540,7 +540,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t\tret = list_tags(&filter, sorting, &format);\n> \t\tif (column_active(colopts))\n> \t\t\tstop_column_filter();\n> -\t\treturn ret;\n> +\t\tgoto cleanup;\n> \t}\n> \tif (filter.lines != -1)\n> \t\tdie(_(\"-n option is only allowed in list mode\"));\n> @@ -552,12 +552,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n> \tif (filter.reachable_from || filter.unreachable_from)\n> \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n> -\tif (cmdmode == 'd')\n> -\t\treturn delete_tags(argv);\n> +\tif (cmdmode == 'd') {\n> +\t\tret = delete_tags(argv);\n> +\t\tgoto cleanup;\n> +\t}\n> \tif (cmdmode == 'v') {\n> \t\tif (format.format && verify_ref_format(&format))\n> \t\t\tusage_with_options(git_tag_usage, options);\n> -\t\treturn for_each_tag_name(argv, verify_tag, &format);\n> +\t\tret = for_each_tag_name(argv, verify_tag, &format);\n> +\t\tgoto cleanup;\n> \t}\n> \n> \tif (msg.given || msgfile) {\n> @@ -626,10 +629,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\n> \t\t find_unique_abbrev(&prev, DEFAULT_ABBREV));\n> \n> -\tUNLEAK(buf);\n> -\tUNLEAK(ref);\n> -\tUNLEAK(reflog_msg);\n> -\tUNLEAK(msg);\n> -\tUNLEAK(err);\n> -\treturn 0;\n> +cleanup:\n> +\tstrbuf_release(&buf);\n> +\tstrbuf_release(&ref);\n> +\tstrbuf_release(&reflog_msg);\n> +\tstrbuf_release(&msg.buf);\n> +\tstrbuf_release(&err);\n> +\treturn ret;\n> }\n"},{"id":"439110","messageId":"xmqqpmrzrcah.fsf@gitster.g","threadId":"56733","inReplyTo":"RFC-patch-v2-2.4-0ae71c19ab7-20211020T131627Z-avarab@gmail.com","subject":"Re: [RFC PATCH v2 2/4] ref-filter API user: add and use a ref_sorting_release()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T14:39:02Z","receivedAt":"2021-10-20T14:39:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +void ref_sorting_release(struct ref_sorting *sorting)\n> +{\n> +\tstruct ref_sorting *next = sorting->next;\n> +\tif (next)\n> +\t ref_sorting_release(next);\n> +\tfree(sorting);\n> +}\n\nLooks like a deep recursion that can be turned into an iteration\nfairly easily? I guess it does not matter as long as we won't deal\nwith thousands of sort keys...\n\n"},{"id":"439112","messageId":"xmqqlf2nrc3o.fsf@gitster.g","threadId":"56733","inReplyTo":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","subject":"Re: [RFC PATCH v2 0/4] for-each-ref: delay parsing of --sort= options + linux-leaks fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T14:43:07Z","receivedAt":"2021-10-20T14:43:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I tried to structure this as somethnig that could be merged or queued\n> up separately, but the semantic conflict in \"branch.c\" was difficult,\n> as Junio's patch changes how those variables are managed.\n\nI tend to agree that it would have been simpler if you did 1, 2 and\n4 first and then rebased 3 on top, possibly as an \"after the dust\nsettles\" separate topic.\n\nThe releasing of these allocated pieces of memory does make sense.\n\nThanks.\n\n"},{"id":"439119","messageId":"cover-0.3-00000000000-20211020T182516Z-avarab@gmail.com","threadId":"56733","inReplyTo":"RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com","subject":"[PATCH 0/3] ref-filter: add a ref_sorting_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T18:27:18Z","receivedAt":"2021-10-20T18:27:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A follow-up to\nhttps://lore.kernel.org/git/RFC-cover-v2-0.4-00000000000-20211020T131627Z-avarab@gmail.com/;\n\nJunio: Hopefully this addresses the things you noted, thank[s| you]!\n:)\n\nI ejected your patch from this iteration, if you'd like to build on\ntop of it it should address the build failures in \"seen\" when combined\nwith your patch.\n\nÆvar Arnfjörð Bjarmason (3):\n tag: use a \"goto cleanup\" pattern, leak less memory\n ref-filter API user: add and use a ref_sorting_release()\n branch: use ref_sorting_release()\n\n builtin/branch.c | 8 +++++---\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 29 +++++++++++++++++------------\n ref-filter.c | 9 +++++++++\n ref-filter.h | 2 ++\n 5 files changed, 34 insertions(+), 16 deletions(-)\n\nRange-diff:\n1: fc776c3f1cd ! 1: 8ccd07abf86 tag: use a \"goto cleanup\" pattern, leak less memory\n @@ Metadata\n ## Commit message ##\n tag: use a \"goto cleanup\" pattern, leak less memory\n \n - Change cmd_tag() to free its \"struct string_list\"'s instead of using\n - an UNLEAK() pattern. This changes code added in 886e1084d78 (builtin/:\n + Change cmd_tag() to free its \"struct strbuf\"'s instead of using an\n + UNLEAK() pattern. This changes code added in 886e1084d78 (builtin/:\n add UNLEAKs, 2017-10-01).\n \n - As shown in the context of the deceleration of the \"struct\n + As shown in the context of the declaration of the \"struct\n msg_arg\" (which I'm changing to use a designated initializer while at\n it, and to show the context in this change), that struct is just a\n thin wrapper around an int and \"struct strbuf\".\n2: 0ae71c19ab7 ! 2: 07062ca276a ref-filter API user: add and use a ref_sorting_release()\n @@ ref-filter.c: int parse_opt_ref_sorting(const struct option *opt, const char *ar\n \n +void ref_sorting_release(struct ref_sorting *sorting)\n +{\n -+\tstruct ref_sorting *next = sorting->next;\n -+\tif (next)\n -+\t ref_sorting_release(next);\n -+\tfree(sorting);\n ++\twhile (sorting) {\n ++\t\tstruct ref_sorting *next = sorting->next;\n ++\t\tfree(sorting);\n ++\t\tsorting = next;\n ++\t}\n +}\n +\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n3: 7abbbe4468c < -: ----------- for-each-ref: delay parsing of --sort= options\n4: f7d87aea384 ! 3: c8e8bc46ac4 branch: use ref_sorting_release()\n @@ Commit message\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/branch.c ##\n -@@ builtin/branch.c: static const char *color_branch_slots[] = {\n - \t[BRANCH_COLOR_WORKTREE] = \"worktree\",\n - };\n - \n --static struct string_list output = STRING_LIST_INIT_DUP;\n - static unsigned int colopts;\n - \n - define_list_config_array(color_branch_slots);\n @@ builtin/branch.c: static char *build_format(struct ref_filter *filter, int maxwidth, const char *r\n \treturn strbuf_detach(&fmt, NULL);\n }\n @@ builtin/branch.c: static void print_ref_list(struct ref_filter *filter, struct r\n \t\t} else {\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n -@@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n - \tenum branch_track track;\n - \tstruct ref_filter filter;\n - \tint icase = 0;\n --\tstatic struct ref_sorting *sorting;\n - \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n - \tstruct ref_format format = REF_FORMAT_INIT;\n - \n -@@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n - \t\tprint_current_branch_name();\n - \t\treturn 0;\n - \t} else if (list) {\n -+\t\tstruct ref_sorting *sorting;\n -+\t\tstatic struct string_list output = STRING_LIST_INIT_DUP;\n -+\n - \t\t/* git branch --list also shows HEAD when it is detached */\n - \t\tif ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)\n - \t\t\tfilter.kind |= FILTER_REFS_DETACHED_HEAD;\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \t\tref_sorting_set_sort_flags_all(\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439120","messageId":"patch-1.3-8ccd07abf86-20211020T182516Z-avarab@gmail.com","threadId":"56733","inReplyTo":"cover-0.3-00000000000-20211020T182516Z-avarab@gmail.com","subject":"[PATCH 1/3] tag: use a \"goto cleanup\" pattern, leak less memory","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T18:27:19Z","receivedAt":"2021-10-20T18:27:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change cmd_tag() to free its \"struct strbuf\"'s instead of using an\nUNLEAK() pattern. This changes code added in 886e1084d78 (builtin/:\nadd UNLEAKs, 2017-10-01).\n\nAs shown in the context of the declaration of the \"struct\nmsg_arg\" (which I'm changing to use a designated initializer while at\nit, and to show the context in this change), that struct is just a\nthin wrapper around an int and \"struct strbuf\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/tag.c | 28 ++++++++++++++++------------\n 1 file changed, 16 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 6535ed27ee9..ad6c9855914 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -432,7 +432,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tint annotate = 0, force = 0;\n \tint cmdmode = 0, create_tag_object = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\n-\tstruct msg_arg msg = { 0, STRBUF_INIT };\n+\tstruct msg_arg msg = { .buf = STRBUF_INIT };\n \tstruct ref_transaction *transaction;\n \tstruct strbuf err = STRBUF_INIT;\n \tstruct ref_filter filter;\n@@ -482,6 +482,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('i', \"ignore-case\", &icase, N_(\"sorting and filtering are case insensitive\")),\n \t\tOPT_END()\n \t};\n+\tint ret = 0;\n \n \tsetup_ref_filter_porcelain_msg();\n \n@@ -529,7 +530,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \tfilter.ignore_case = icase;\n \tif (cmdmode == 'l') {\n-\t\tint ret;\n \t\tif (column_active(colopts)) {\n \t\t\tstruct column_options copts;\n \t\t\tmemset(&copts, 0, sizeof(copts));\n@@ -540,7 +540,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tret = list_tags(&filter, sorting, &format);\n \t\tif (column_active(colopts))\n \t\t\tstop_column_filter();\n-\t\treturn ret;\n+\t\tgoto cleanup;\n \t}\n \tif (filter.lines != -1)\n \t\tdie(_(\"-n option is only allowed in list mode\"));\n@@ -552,12 +552,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--points-at option is only allowed in list mode\"));\n \tif (filter.reachable_from || filter.unreachable_from)\n \t\tdie(_(\"--merged and --no-merged options are only allowed in list mode\"));\n-\tif (cmdmode == 'd')\n-\t\treturn delete_tags(argv);\n+\tif (cmdmode == 'd') {\n+\t\tret = delete_tags(argv);\n+\t\tgoto cleanup;\n+\t}\n \tif (cmdmode == 'v') {\n \t\tif (format.format && verify_ref_format(&format))\n \t\t\tusage_with_options(git_tag_usage, options);\n-\t\treturn for_each_tag_name(argv, verify_tag, &format);\n+\t\tret = for_each_tag_name(argv, verify_tag, &format);\n+\t\tgoto cleanup;\n \t}\n \n \tif (msg.given || msgfile) {\n@@ -626,10 +629,11 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag,\n \t\t find_unique_abbrev(&prev, DEFAULT_ABBREV));\n \n-\tUNLEAK(buf);\n-\tUNLEAK(ref);\n-\tUNLEAK(reflog_msg);\n-\tUNLEAK(msg);\n-\tUNLEAK(err);\n-\treturn 0;\n+cleanup:\n+\tstrbuf_release(&buf);\n+\tstrbuf_release(&ref);\n+\tstrbuf_release(&reflog_msg);\n+\tstrbuf_release(&msg.buf);\n+\tstrbuf_release(&err);\n+\treturn ret;\n }\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439121","messageId":"patch-2.3-07062ca276a-20211020T182516Z-avarab@gmail.com","threadId":"56733","inReplyTo":"cover-0.3-00000000000-20211020T182516Z-avarab@gmail.com","subject":"[PATCH 2/3] ref-filter API user: add and use a ref_sorting_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T18:27:20Z","receivedAt":"2021-10-20T18:27:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a ref_sorting_release() and use it for some of the current API\nusers, the ref_sorting_default() function and its siblings will do a\nmalloc() which wasn't being free'd previously.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/for-each-ref.c | 2 +-\n builtin/tag.c | 1 +\n ref-filter.c | 9 +++++++++\n ref-filter.h | 2 ++\n 4 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 642b4b888fb..16a2c7d57ca 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -96,6 +96,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tref_array_clear(&array);\n \tfree_commit_list(filter.with_commit);\n \tfree_commit_list(filter.no_commit);\n-\tUNLEAK(sorting);\n+\tref_sorting_release(sorting);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex ad6c9855914..6fe646710d6 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -630,6 +630,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t find_unique_abbrev(&prev, DEFAULT_ABBREV));\n \n cleanup:\n+\tref_sorting_release(sorting);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&ref);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex add429be797..282cdad1036 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2705,6 +2705,15 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+void ref_sorting_release(struct ref_sorting *sorting)\n+{\n+\twhile (sorting) {\n+\t\tstruct ref_sorting *next = sorting->next;\n+\t\tfree(sorting);\n+\t\tsorting = next;\n+\t}\n+}\n+\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)\n {\n \tstruct ref_filter *rf = opt->value;\ndiff --git a/ref-filter.h b/ref-filter.h\nindex b636f4389d0..6228458d306 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -127,6 +127,8 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom);\n int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset);\n /* Default sort option based on refname */\n struct ref_sorting *ref_default_sorting(void);\n+/* Release a \"struct ref_sorting\" */\n+void ref_sorting_release(struct ref_sorting *);\n /* Function to parse --merged and --no-merged options */\n int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);\n /* Get the current HEAD's description */\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439122","messageId":"patch-3.3-c8e8bc46ac4-20211020T182516Z-avarab@gmail.com","threadId":"56733","inReplyTo":"cover-0.3-00000000000-20211020T182516Z-avarab@gmail.com","subject":"[PATCH 3/3] branch: use ref_sorting_release()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-20T18:27:21Z","receivedAt":"2021-10-20T18:27:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use a ref_sorting_release() in branch.c to free the memory from the\nref_sorting_options(). This plugs the final in-tree memory leak of\nthat API.\n\nIn the preceding commit the \"sorting\" variable was left in the\ncmd_branch() scope, even though that wasn't needed anymore. Move it to\nthe \"else if (list)\" scope instead. We can also move the \"struct\nstring_list\" only used for that branch to be declared in that block\n\nThat \"struct ref_sorting\" does not need to be \"static\" (and isn't\nre-used). The \"ref_sorting_options()\" will return a valid one, we\ndon't need to make it \"static\" to have it zero'd out. That it was\nstatic was another artifact of the pre-image of the preceding commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/branch.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0b7ed82654a..7a1d1eeb070 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -407,7 +407,8 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r\n \treturn strbuf_detach(&fmt, NULL);\n }\n \n-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format)\n+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting,\n+\t\t\t struct ref_format *format, struct string_list *output)\n {\n \tint i;\n \tstruct ref_array array;\n@@ -449,7 +450,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\tif (column_active(colopts)) {\n \t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n \t\t\t /* format to a string_list to let print_columns() do its job */\n-\t\t\tstring_list_append(&output, out.buf);\n+\t\t\tstring_list_append(output, out.buf);\n \t\t} else {\n \t\t\tfwrite(out.buf, 1, out.len, stdout);\n \t\t\tputchar('\\n');\n@@ -753,9 +754,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n \t\tref_sorting_set_sort_flags_all(\n \t\t\tsorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);\n-\t\tprint_ref_list(&filter, sorting, &format);\n+\t\tprint_ref_list(&filter, sorting, &format, &output);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n+\t\tref_sorting_release(sorting);\n \t\treturn 0;\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n-- \n2.33.1.1338.g20da966911a\n\n"},{"id":"439140","messageId":"YXCAuAwZenM6EPug@coredump.intra.peff.net","threadId":"56733","inReplyTo":"xmqq4k9bssr7.fsf@gitster.g","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-20T20:48:56Z","receivedAt":"2021-10-20T20:49:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 20, 2021 at 06:58:04AM -0700, Junio C Hamano wrote:\n\n> However, it is tricky to arrange, as the command already takes\n> multiple --sort keys, and the laster ones are taken as more\n> significant sort order, so it is tricky to come up with two keys A\n> and B such that --sort=A --no-sort --sort=B will produce one order,\n> while --sort=A --sort=B will produce another different order.\n\nYeah, I faced something similar with 7c5045fc18 (ref-filter: apply\nfallback refname sort only after all user sorts, 2020-05-03). I suspect\nyou could use the same keys as those tests, though I am OK if we simply\nleave it as a quietly-fixed bug.\n\n-Peff\n"},{"id":"439143","messageId":"xmqqpmrzo00q.fsf@gitster.g","threadId":"56733","inReplyTo":"YXCAuAwZenM6EPug@coredump.intra.peff.net","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-20T21:32:21Z","receivedAt":"2021-10-20T21:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I faced something similar with 7c5045fc18 (ref-filter: apply\n> fallback refname sort only after all user sorts, 2020-05-03). I suspect\n> you could use the same keys as those tests, though I am OK if we simply\n> leave it as a quietly-fixed bug.\n\nAh, I guess I can cheat and add a new test after these.\n\nIf --no-sort weren't taking effect, the expected outcome would be\nthe asme as the previous step this copied from, but with --no-sort\nclearing the sort keys, we sort by taggerdate and then tiebreak with\nthe refname, and taggeremail does not get into the picture (other\nthan being repeated at the end of the refname).\n\n t/t6300-for-each-ref.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git c/t/t6300-for-each-ref.sh w/t/t6300-for-each-ref.sh\nindex 83921502a4..9f2c706c12 100755\n--- c/t/t6300-for-each-ref.sh\n+++ w/t/t6300-for-each-ref.sh\n@@ -1024,6 +1024,27 @@ test_expect_success 'equivalent sorts fall back on refname' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '--no-sort cancels the previous sort keys' '\n+\tcat >expected <<-\\EOF &&\n+\t100000 refs/tags/multi-ref1-100000-user1\n+\t100000 refs/tags/multi-ref1-100000-user2\n+\t100000 refs/tags/multi-ref2-100000-user1\n+\t100000 refs/tags/multi-ref2-100000-user2\n+\t200000 refs/tags/multi-ref1-200000-user1\n+\t200000 refs/tags/multi-ref1-200000-user2\n+\t200000 refs/tags/multi-ref2-200000-user1\n+\t200000 refs/tags/multi-ref2-200000-user2\n+\tEOF\n+\tgit for-each-ref \\\n+\t\t--format=\"%(taggerdate:unix) %(taggeremail) %(refname)\" \\\n+\t\t--sort=-refname \\\n+\t\t--sort=taggeremail \\\n+\t\t--no-sort \\\n+\t\t--sort=taggerdate \\\n+\t\t\"refs/tags/multi-*\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '\n \ttest_when_finished \"git checkout main\" &&\n \tgit for-each-ref --format=\"%(HEAD) %(refname:short)\" refs/heads/ >actual &&\n"},{"id":"439238","messageId":"YXF/KTD7NgTAPw6G@coredump.intra.peff.net","threadId":"56733","inReplyTo":"xmqqpmrzo00q.fsf@gitster.g","subject":"Re: [PATCH] for-each-ref: delay parsing of --sort= options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-10-21T14:54:33Z","receivedAt":"2021-10-21T14:54:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 20, 2021 at 02:32:21PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, I faced something similar with 7c5045fc18 (ref-filter: apply\n> > fallback refname sort only after all user sorts, 2020-05-03). I suspect\n> > you could use the same keys as those tests, though I am OK if we simply\n> > leave it as a quietly-fixed bug.\n> \n> Ah, I guess I can cheat and add a new test after these.\n> \n> If --no-sort weren't taking effect, the expected outcome would be\n> the asme as the previous step this copied from, but with --no-sort\n> clearing the sort keys, we sort by taggerdate and then tiebreak with\n> the refname, and taggeremail does not get into the picture (other\n> than being repeated at the end of the refname).\n\nYeah, I think what you have here makes sense. It's too bad we can't run\nit on the \"before\" state to double-check that we are triggering the old\nbreakage, but there is simply no \"--no-sort\" at all before your patch\n(which is good, because it would have been broken ;) ).\n\n-Peff\n"}]}