{"thread":{"id":"62740","subject":"Bug sorting git branch output by ahead-behind:HEAD","startedAt":"2025-01-05T09:48:22Z","lastAt":"2025-01-19T12:46:53Z","messageCount":19,"participants":["Ross Goldberg","René Scharfe","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"509923","messageId":"CAAEcvMovcZucuq4H70ouQzKS=_wHnBi1DqJ-4MMFVXX9XLEciw@mail.gmail.com","threadId":"62740","inReplyTo":null,"subject":"Bug sorting git branch output by ahead-behind:HEAD","fromName":"Ross Goldberg","fromEmail":"ross.goldberg@gmail.com","sentAt":"2025-01-05T09:48:07Z","receivedAt":"2025-01-05T09:48:22Z","isPatch":false,"sender":{"key":"ross.goldberg@gmail.com","avatar":null},"body":"Bug sorting git branch output by ahead-behind:HEAD\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n$ git branch --format '%(refname:short) %(ahead-behind:HEAD)'\n(HEAD detached at 6aab6bfe) 0 0\n190-json 1 6\n560-swupd 3 6\n667-reset-no-process 2 6\n678-completions 2 0\n693-readme-accordions 3 6\n705-branch-in-version 2 6\nmain 0 6\n\n$ git branch --format '%(refname:short) %(ahead-behind:HEAD)' --sort\n'ahead-behind:HEAD'\n(HEAD detached at 6aab6bfe)\n190-json\n560-swupd\n667-reset-no-process\n678-completions\n693-readme-accordions\n705-branch-in-version\nmain\n\nWhat did you expect to happen? (Expected behavior)\n\nI expected the second command's output to:\n\n- include the value of `ahead-behind:HEAD` on each line\n- have been sorted by each line's value of `ahead-behind:HEAD`\n\nWhat happened instead? (Actual behavior)\n\nSecond command's output missing `ahead-behind:HEAD` & sorted by `refname`\n\nWhat's different between what you expected and what actually happened?\n\nFields missing from second command's output\n\nIncorrect sorting of second command's output lines\n\nAnything else you want to add:\n\nIf I sort by the `objectsize` field:\n\n- all requested fields are output\n- sorting works (except `(HEAD detached at 6aab6bfe)` is always listed first)\n\nIf I do not include `%(ahead-behind:HEAD)` in the format string,\nsorting by `ahead-behind:HEAD` still doesn't work.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.47.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nfeature: fsmonitor--daemon\nlibcurl: 7.79.1\nzlib: 1.2.11\nuname: Darwin 21.6.0 Darwin Kernel Version 21.6.0: Mon Jun 24 00:56:10\nPDT 2024; root:xnu-8020.240.18.709.2~1/RELEASE_X86_64 x86_64\ncompiler info: clang: 14.0.0 (clang-1400.0.29.202)\nlibc info: no libc information available\n$SHELL (typically, interactive shell): /usr/local/bin/zsh\n\n\n[Enabled Hooks]\n"},{"id":"510379","messageId":"6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de","threadId":"62740","inReplyTo":"CAAEcvMovcZucuq4H70ouQzKS=_wHnBi1DqJ-4MMFVXX9XLEciw@mail.gmail.com","subject":"[PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-12T10:01:52Z","receivedAt":"2025-01-12T10:02:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"verify_ref_format() parses a ref-filter format string and stores\nrecognized items in the static array \"used_atom\".  For\n\"ahead-behind:<committish>\" and \"is-base:<committish>\" it stores the\ncommittish part in string_lists that are part of struct ref_format.\n\nref_sorting_options() also parses bare ref-filter format items and also\nstores recognized ones in \"used_atom\".  The committish parts go to a\ndummy struct ref_format in parse_sorting_atom(), though, and are leaked\nand forgotten.\n\nIf verify_ref_format() is called before ref_sorting_options(), like in\ngit for-each-ref, then all works well if the sort key is included in the\nformat string.  If it isn't then sorting cannot work as the committishes\nare missing.\n\nIf ref_sorting_options() is called first, like in git branch, then we\nhave the additional issue that if the sort key is included in the format\nstring then filter_ahead_behind() and filter_is_base() can't see their\ncommittishes, will not generate any results for them and thus they will\nfor expanded to empty strings.\n\nFix those issues by making the string_lists static, like their sibling\n\"used_atom\".  This way they can all be shared for handling both\nref-filter format strings and sorting options in the same command.\nAnd since struct ref_format no longer contains any allocated members,\nremove the now unnecessary ref_format_init() and ref_format_clear().\n\nReported-by: Ross Goldberg <ross.goldberg@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/branch.c         |  3 +-\n builtin/for-each-ref.c   |  1 -\n builtin/tag.c            |  1 -\n builtin/verify-tag.c     |  1 -\n ref-filter.c             | 70 ++++++++++++++++++----------------------\n ref-filter.h             | 13 --------\n t/t3203-branch-output.sh | 28 ++++++++++++++++\n t/t6600-test-reach.sh    | 29 +++++++++++++++++\n 8 files changed, 89 insertions(+), 57 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..9a29de5bf1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -473,7 +473,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tif (verify_ref_format(format))\n \t\tdie(_(\"unable to parse format string\"));\n\n-\tfilter_ahead_behind(the_repository, format, &array);\n+\tfilter_ahead_behind(the_repository, &array);\n \tref_array_sort(sorting, &array);\n\n \tif (column_active(colopts)) {\n@@ -884,7 +884,6 @@ int cmd_branch(int argc,\n \t\tstring_list_clear(&output, 0);\n \t\tref_sorting_release(sorting);\n \t\tref_filter_clear(&filter);\n-\t\tref_format_clear(&format);\n\n \t\tret = 0;\n \t\tgoto out;\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 715745a262..8085ebd8fe 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -108,7 +108,6 @@ int cmd_for_each_ref(int argc,\n \tfilter_and_format_refs(&filter, flags, sorting, &format);\n\n \tref_filter_clear(&filter);\n-\tref_format_clear(&format);\n \tref_sorting_release(sorting);\n \tstrvec_clear(&vec);\n \treturn 0;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex c4bd145831..e8a344b926 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -698,7 +698,6 @@ int cmd_tag(int argc,\n cleanup:\n \tref_sorting_release(sorting);\n \tref_filter_clear(&filter);\n-\tref_format_clear(&format);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&ref);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex a7f20618ff..f6b97048a5 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -69,6 +69,5 @@ int cmd_verify_tag(int argc,\n \t\tif (format.format)\n \t\t\tpretty_print_ref(name, &oid, &format);\n \t}\n-\tref_format_clear(&format);\n \treturn had_error;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 23054694c2..aef142e105 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -242,6 +242,12 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n\n+/* List of bases for ahead-behind counts. */\n+static struct string_list bases = STRING_LIST_INIT_DUP;\n+\n+/* List of bases for is-base indicators. */\n+static struct string_list is_base_tips = STRING_LIST_INIT_DUP;\n+\n /*\n  * Expand string, append it to strbuf *sb, then return error code ret.\n  * Allow to save few lines of code.\n@@ -891,7 +897,7 @@ static int rest_atom_parser(struct ref_format *format UNUSED,\n \treturn 0;\n }\n\n-static int ahead_behind_atom_parser(struct ref_format *format,\n+static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n \t\t\t\t    struct used_atom *atom UNUSED,\n \t\t\t\t    const char *arg, struct strbuf *err)\n {\n@@ -900,7 +906,7 @@ static int ahead_behind_atom_parser(struct ref_format *format,\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(ahead-behind:<committish>)\"));\n\n-\titem = string_list_append(&format->bases, arg);\n+\titem = string_list_append(&bases, arg);\n \titem->util = lookup_commit_reference_by_name(arg);\n \tif (!item->util)\n \t\tdie(\"failed to find '%s'\", arg);\n@@ -908,7 +914,7 @@ static int ahead_behind_atom_parser(struct ref_format *format,\n \treturn 0;\n }\n\n-static int is_base_atom_parser(struct ref_format *format,\n+static int is_base_atom_parser(struct ref_format *format UNUSED,\n \t\t\t       struct used_atom *atom UNUSED,\n \t\t\t       const char *arg, struct strbuf *err)\n {\n@@ -917,7 +923,7 @@ static int is_base_atom_parser(struct ref_format *format,\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(is-base:<committish>)\"));\n\n-\titem = string_list_append(&format->is_base_tips, arg);\n+\titem = string_list_append(&is_base_tips, arg);\n \titem->util = lookup_commit_reference_by_name(arg);\n \tif (!item->util)\n \t\tdie(\"failed to find '%s'\", arg);\n@@ -3024,6 +3030,8 @@ void ref_array_clear(struct ref_array *array)\n \t}\n \tFREE_AND_NULL(used_atom);\n \tused_atom_cnt = 0;\n+\tstring_list_clear(&bases, 0);\n+\tstring_list_clear(&is_base_tips, 0);\n\n \tif (ref_to_worktree_map.worktrees) {\n \t\thashmap_clear_and_free(&(ref_to_worktree_map.map),\n@@ -3084,22 +3092,21 @@ static void reach_filter(struct ref_array *array,\n }\n\n void filter_ahead_behind(struct repository *r,\n-\t\t\t struct ref_format *format,\n \t\t\t struct ref_array *array)\n {\n \tstruct commit **commits;\n-\tsize_t commits_nr = format->bases.nr + array->nr;\n+\tsize_t commits_nr = bases.nr + array->nr;\n\n-\tif (!format->bases.nr || !array->nr)\n+\tif (!bases.nr || !array->nr)\n \t\treturn;\n\n \tALLOC_ARRAY(commits, commits_nr);\n-\tfor (size_t i = 0; i < format->bases.nr; i++)\n-\t\tcommits[i] = format->bases.items[i].util;\n+\tfor (size_t i = 0; i < bases.nr; i++)\n+\t\tcommits[i] = bases.items[i].util;\n\n-\tALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr));\n+\tALLOC_ARRAY(array->counts, st_mult(bases.nr, array->nr));\n\n-\tcommits_nr = format->bases.nr;\n+\tcommits_nr = bases.nr;\n \tarray->counts_nr = 0;\n \tfor (size_t i = 0; i < array->nr; i++) {\n \t\tconst char *name = array->items[i]->refname;\n@@ -3108,8 +3115,8 @@ void filter_ahead_behind(struct repository *r,\n \t\tif (!commits[commits_nr])\n \t\t\tcontinue;\n\n-\t\tCALLOC_ARRAY(array->items[i]->counts, format->bases.nr);\n-\t\tfor (size_t j = 0; j < format->bases.nr; j++) {\n+\t\tCALLOC_ARRAY(array->items[i]->counts, bases.nr);\n+\t\tfor (size_t j = 0; j < bases.nr; j++) {\n \t\t\tstruct ahead_behind_count *count;\n \t\t\tcount = &array->counts[array->counts_nr++];\n \t\t\tcount->tip_index = commits_nr;\n@@ -3125,14 +3132,13 @@ void filter_ahead_behind(struct repository *r,\n }\n\n void filter_is_base(struct repository *r,\n-\t\t    struct ref_format *format,\n \t\t    struct ref_array *array)\n {\n \tstruct commit **bases;\n \tsize_t bases_nr = 0;\n \tstruct ref_array_item **back_index;\n\n-\tif (!format->is_base_tips.nr || !array->nr)\n+\tif (!is_base_tips.nr || !array->nr)\n \t\treturn;\n\n \tCALLOC_ARRAY(back_index, array->nr);\n@@ -3142,7 +3148,7 @@ void filter_is_base(struct repository *r,\n \t\tconst char *name = array->items[i]->refname;\n \t\tstruct commit *c = lookup_commit_reference_by_name_gently(name, 1);\n\n-\t\tCALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr);\n+\t\tCALLOC_ARRAY(array->items[i]->is_base, is_base_tips.nr);\n\n \t\tif (!c)\n \t\t\tcontinue;\n@@ -3152,15 +3158,15 @@ void filter_is_base(struct repository *r,\n \t\tbases_nr++;\n \t}\n\n-\tfor (size_t i = 0; i < format->is_base_tips.nr; i++) {\n-\t\tstruct commit *tip = format->is_base_tips.items[i].util;\n+\tfor (size_t i = 0; i < is_base_tips.nr; i++) {\n+\t\tstruct commit *tip = is_base_tips.items[i].util;\n \t\tint base_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n\n \t\tif (base_index < 0)\n \t\t\tcontinue;\n\n \t\t/* Store the string for use in output later. */\n-\t\tback_index[base_index]->is_base[i] = xstrdup(format->is_base_tips.items[i].string);\n+\t\tback_index[base_index]->is_base[i] = xstrdup(is_base_tips.items[i].string);\n \t}\n\n \tfree(back_index);\n@@ -3252,8 +3258,7 @@ struct ref_sorting {\n };\n\n static inline int can_do_iterative_format(struct ref_filter *filter,\n-\t\t\t\t\t  struct ref_sorting *sorting,\n-\t\t\t\t\t  struct ref_format *format)\n+\t\t\t\t\t  struct ref_sorting *sorting)\n {\n \t/*\n \t * Reference backends sort patterns lexicographically by refname, so if\n@@ -3279,15 +3284,15 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \t */\n \treturn !(filter->reachable_from ||\n \t\t filter->unreachable_from ||\n-\t\t format->bases.nr ||\n-\t\t format->is_base_tips.nr);\n+\t\t bases.nr ||\n+\t\t is_base_tips.nr);\n }\n\n void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t\t\t    struct ref_sorting *sorting,\n \t\t\t    struct ref_format *format)\n {\n-\tif (can_do_iterative_format(filter, sorting, format)) {\n+\tif (can_do_iterative_format(filter, sorting)) {\n \t\tint save_commit_buffer_orig;\n \t\tstruct ref_filter_and_format_cbdata ref_cbdata = {\n \t\t\t.filter = filter,\n@@ -3303,8 +3308,8 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t} else {\n \t\tstruct ref_array array = { 0 };\n \t\tfilter_refs(&array, filter, type);\n-\t\tfilter_ahead_behind(the_repository, format, &array);\n-\t\tfilter_is_base(the_repository, format, &array);\n+\t\tfilter_ahead_behind(the_repository, &array);\n+\t\tfilter_is_base(the_repository, &array);\n \t\tref_array_sort(sorting, &array);\n \t\tprint_formatted_ref_array(&array, format);\n \t\tref_array_clear(&array);\n@@ -3638,16 +3643,3 @@ void ref_filter_clear(struct ref_filter *filter)\n \tfree_commit_list(filter->unreachable_from);\n \tref_filter_init(filter);\n }\n-\n-void ref_format_init(struct ref_format *format)\n-{\n-\tstruct ref_format blank = REF_FORMAT_INIT;\n-\tmemcpy(format, &blank, sizeof(blank));\n-}\n-\n-void ref_format_clear(struct ref_format *format)\n-{\n-\tstring_list_clear(&format->bases, 0);\n-\tstring_list_clear(&format->is_base_tips, 0);\n-\tref_format_init(format);\n-}\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 754038ab07..013d4cfa64 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -99,12 +99,6 @@ struct ref_format {\n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n\n-\t/* List of bases for ahead-behind counts. */\n-\tstruct string_list bases;\n-\n-\t/* List of bases for is-base indicators. */\n-\tstruct string_list is_base_tips;\n-\n \tstruct {\n \t\tint max_count;\n \t\tint omit_empty;\n@@ -117,8 +111,6 @@ struct ref_format {\n }\n #define REF_FORMAT_INIT {             \\\n \t.use_color = -1,              \\\n-\t.bases = STRING_LIST_INIT_DUP, \\\n-\t.is_base_tips = STRING_LIST_INIT_DUP, \\\n }\n\n /*  Macros for checking --merged and --no-merged options */\n@@ -205,7 +197,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array,\n  * If this is not called, then any ahead-behind atoms will be blank.\n  */\n void filter_ahead_behind(struct repository *r,\n-\t\t\t struct ref_format *format,\n \t\t\t struct ref_array *array);\n\n /*\n@@ -215,13 +206,9 @@ void filter_ahead_behind(struct repository *r,\n  * If this is not called, then any is-base atoms will be blank.\n  */\n void filter_is_base(struct repository *r,\n-\t\t    struct ref_format *format,\n \t\t    struct ref_array *array);\n\n void ref_filter_init(struct ref_filter *filter);\n void ref_filter_clear(struct ref_filter *filter);\n\n-void ref_format_init(struct ref_format *format);\n-void ref_format_clear(struct ref_format *format);\n-\n #endif /*  REF_FILTER_H  */\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 500c9d0e72..a6bd88a58d 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -368,6 +368,34 @@ test_expect_success 'git branch --format with ahead-behind' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'git branch `--sort=[-]ahead-behind` option' '\n+\tcat >expect <<-\\EOF &&\n+\t(HEAD detached from fromtag) 0 0\n+\trefs/heads/ambiguous 0 0\n+\trefs/heads/branch-two 0 0\n+\trefs/heads/branch-one 1 0\n+\trefs/heads/main 1 0\n+\trefs/heads/ref-to-branch 1 0\n+\trefs/heads/ref-to-remote 1 0\n+\tEOF\n+\tgit branch --format=\"%(refname) %(ahead-behind:HEAD)\" \\\n+\t\t--sort=refname --sort=ahead-behind:HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t(HEAD detached from fromtag) 0 0\n+\trefs/heads/branch-one 1 0\n+\trefs/heads/main 1 0\n+\trefs/heads/ref-to-branch 1 0\n+\trefs/heads/ref-to-remote 1 0\n+\trefs/heads/ambiguous 0 0\n+\trefs/heads/branch-two 0 0\n+\tEOF\n+\tgit branch --format=\"%(refname) %(ahead-behind:HEAD)\" \\\n+\t\t--sort=refname --sort=-ahead-behind:HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git branch with --format=%(rest) must fail' '\n \ttest_must_fail git branch --format=\"%(rest)\" >actual\n '\ndiff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\nindex 2591f8b8b3..6638d1aa1d 100755\n--- a/t/t6600-test-reach.sh\n+++ b/t/t6600-test-reach.sh\n@@ -733,4 +733,33 @@ test_expect_success 'for-each-ref is-base:multiple' '\n \t\t--format=\"%(refname)[%(is-base:commit-2-3)-%(is-base:commit-6-5)]\" --stdin\n '\n\n+test_expect_success 'for-each-ref is-base: --sort' '\n+\tcat >input <<-\\EOF &&\n+\trefs/heads/commit-1-1\n+\trefs/heads/commit-4-2\n+\trefs/heads/commit-4-4\n+\trefs/heads/commit-8-4\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/commit-1-1\n+\trefs/heads/commit-4-4\n+\trefs/heads/commit-8-4\n+\trefs/heads/commit-4-2\n+\tEOF\n+\trun_all_modes git for-each-ref \\\n+\t\t--format=\"%(refname)\" --stdin \\\n+\t\t--sort=refname --sort=is-base:commit-2-3 &&\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/commit-4-2\n+\trefs/heads/commit-1-1\n+\trefs/heads/commit-4-4\n+\trefs/heads/commit-8-4\n+\tEOF\n+\trun_all_modes git for-each-ref \\\n+\t\t--format=\"%(refname)\" --stdin \\\n+\t\t--sort=refname --sort=-is-base:commit-2-3\n+'\n+\n test_done\n--\n2.48.0\n"},{"id":"510384","messageId":"20250113051700.GA767856@coredump.intra.peff.net","threadId":"62740","inReplyTo":"6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-13T05:17:00Z","receivedAt":"2025-01-13T05:17:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 12, 2025 at 11:01:52AM +0100, René Scharfe wrote:\n\n> verify_ref_format() parses a ref-filter format string and stores\n> recognized items in the static array \"used_atom\".  For\n> \"ahead-behind:<committish>\" and \"is-base:<committish>\" it stores the\n> committish part in string_lists that are part of struct ref_format.\n> \n> ref_sorting_options() also parses bare ref-filter format items and also\n> stores recognized ones in \"used_atom\".  The committish parts go to a\n> dummy struct ref_format in parse_sorting_atom(), though, and are leaked\n> and forgotten.\n>\n> If verify_ref_format() is called before ref_sorting_options(), like in\n> git for-each-ref, then all works well if the sort key is included in the\n> format string.  If it isn't then sorting cannot work as the committishes\n> are missing.\n>\n> If ref_sorting_options() is called first, like in git branch, then we\n> have the additional issue that if the sort key is included in the format\n> string then filter_ahead_behind() and filter_is_base() can't see their\n> committishes, will not generate any results for them and thus they will\n> for expanded to empty strings.\n\nGood analysis. The sorting and formatting are definitely tied in subtle\nways, and not all code takes that into account.\n\nThe dummy ref_format here is one such problem. Another is that we don't\ndo the equivalent of verify_ref_format() on the sorting fields. Most of\nwhat it does is probably superfluous, but for example it's supposed to\nreject some atoms that have parsers. So:\n\n  $ git for-each-ref --format='%(rest)'\n  fatal: this command reject atom %(rest)\n\nbut:\n\n  $ git for-each-ref --sort=rest\n  [...no error...]\n\nThat's somewhat orthogonal, but it may influence the direction of our\nsolution.\n\n> Fix those issues by making the string_lists static, like their sibling\n> \"used_atom\".  This way they can all be shared for handling both\n> ref-filter format strings and sorting options in the same command.\n> And since struct ref_format no longer contains any allocated members,\n> remove the now unnecessary ref_format_init() and ref_format_clear().\n\nHmm. So this certainly fixes the problem. But is it where we want to go\nin the long run?\n\nFor now there is no program that uses more than one ref-filter format.\nBut it seems like an obvious interface that would want to be lib-ified\neventually. We are not there yet because of the static global used_atoms\narray. But the obvious path forward is to have a context struct\nrepresenting one ref-filter iteration.\n\nI think the intent was that ref_format would be that context struct,\nthough arguably it is a little funny since it forces the sorting and\nformatting to be joined (OTOH, that is very much how the code works,\nsince it wants to share results between the two for efficiency).\n\nSo one solution would be to make the use of that context struct more\nexplicit, and require ref_sorting callers to provide a format struct.\nLike the patch below, which also passes your tests.\n\nI dunno. Your patch is deleting more code, which is nice. But I think in\nthe long run we'd end up replacing it. But maybe making a clean slate\nnow would make that easier? I could go either way.\n\n---\n builtin/branch.c       |  2 +-\n builtin/for-each-ref.c |  2 +-\n builtin/ls-remote.c    |  4 +++-\n builtin/tag.c          |  2 +-\n ref-filter.c           | 19 ++++++++-----------\n ref-filter.h           |  2 +-\n 6 files changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..0c3f35cd0a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -875,7 +875,7 @@ int cmd_branch(int argc,\n \t\t * local branches 'refs/heads/...' and finally remote-tracking\n \t\t * branches 'refs/remotes/...'.\n \t\t */\n-\t\tsorting = ref_sorting_options(&sorting_options);\n+\t\tsorting = ref_sorting_options(&sorting_options, &format);\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 715745a262..4f247efe57 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -80,7 +80,7 @@ int cmd_for_each_ref(int argc,\n \tif (verify_ref_format(&format))\n \t\tusage_with_options(for_each_ref_usage, opts);\n \n-\tsorting = ref_sorting_options(&sorting_options);\n+\tsorting = ref_sorting_options(&sorting_options, &format);\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 42f34e1236..ed38b82346 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -61,6 +61,7 @@ int cmd_ls_remote(int argc,\n \tconst struct ref *ref;\n \tstruct ref_array ref_array;\n \tstruct ref_sorting *sorting;\n+\tstruct ref_format format = REF_FORMAT_INIT;\n \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n \n \tstruct option options[] = {\n@@ -155,7 +156,7 @@ int cmd_ls_remote(int argc,\n \t\titem->symref = xstrdup_or_null(ref->symref);\n \t}\n \n-\tsorting = ref_sorting_options(&sorting_options);\n+\tsorting = ref_sorting_options(&sorting_options, &format);\n \tref_array_sort(sorting, &ref_array);\n \n \tfor (i = 0; i < ref_array.nr; i++) {\n@@ -173,6 +174,7 @@ int cmd_ls_remote(int argc,\n \t\tstatus = 1;\n \ttransport_ls_refs_options_release(&transport_options);\n \n+\tref_format_clear(&format);\n \tstrvec_clear(&pattern);\n \tstring_list_clear(&server_options, 0);\n \treturn status;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex c4bd145831..a5240f66e2 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -574,7 +574,7 @@ int cmd_tag(int argc,\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--column\", \"-n\");\n \t\tcolopts = 0;\n \t}\n-\tsorting = ref_sorting_options(&sorting_options);\n+\tsorting = ref_sorting_options(&sorting_options, &format);\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 23054694c2..f5d0c448ed 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -3536,23 +3536,19 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n \tfree_array_item(ref_item);\n }\n \n-static int parse_sorting_atom(const char *atom)\n+static int parse_sorting_atom(struct ref_format *format, const char *atom)\n {\n-\t/*\n-\t * This parses an atom using a dummy ref_format, since we don't\n-\t * actually care about the formatting details.\n-\t */\n-\tstruct ref_format dummy = REF_FORMAT_INIT;\n \tconst char *end = atom + strlen(atom);\n \tstruct strbuf err = STRBUF_INIT;\n-\tint res = parse_ref_filter_atom(&dummy, atom, end, &err);\n+\tint res = parse_ref_filter_atom(format, atom, end, &err);\n \tif (res < 0)\n \t\tdie(\"%s\", err.buf);\n \tstrbuf_release(&err);\n \treturn res;\n }\n \n-static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n+static void parse_ref_sorting(struct ref_format *format,\n+\t\t\t      struct ref_sorting **sorting_tail, const char *arg)\n {\n \tstruct ref_sorting *s;\n \n@@ -3567,17 +3563,18 @@ static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg\n \tif (skip_prefix(arg, \"version:\", &arg) ||\n \t    skip_prefix(arg, \"v:\", &arg))\n \t\ts->sort_flags |= REF_SORTING_VERSION;\n-\ts->atom = parse_sorting_atom(arg);\n+\ts->atom = parse_sorting_atom(format, arg);\n }\n \n-struct ref_sorting *ref_sorting_options(struct string_list *options)\n+struct ref_sorting *ref_sorting_options(struct string_list *options,\n+\t\t\t\t\tstruct ref_format *format)\n {\n \tstruct string_list_item *item;\n \tstruct ref_sorting *sorting = NULL, **tail = &sorting;\n \n \tif (options->nr) {\n \t\tfor_each_string_list_item(item, options)\n-\t\t\tparse_ref_sorting(tail, item->string);\n+\t\t\tparse_ref_sorting(format, tail, item->string);\n \t}\n \n \t/*\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 754038ab07..1531bf1762 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -168,7 +168,7 @@ int format_ref_array_item(struct ref_array_item *info,\n /* Release a \"struct ref_sorting\" */\n void ref_sorting_release(struct ref_sorting *);\n /*  Convert list of sort options into ref_sorting */\n-struct ref_sorting *ref_sorting_options(struct string_list *);\n+struct ref_sorting *ref_sorting_options(struct string_list *, struct ref_format *);\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"},{"id":"510385","messageId":"20250113052313.GA768703@coredump.intra.peff.net","threadId":"62740","inReplyTo":"20250113051700.GA767856@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-13T05:23:13Z","receivedAt":"2025-01-13T05:23:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 13, 2025 at 12:17:01AM -0500, Jeff King wrote:\n\n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index 42f34e1236..ed38b82346 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc,\n>  \tconst struct ref *ref;\n>  \tstruct ref_array ref_array;\n>  \tstruct ref_sorting *sorting;\n> +\tstruct ref_format format = REF_FORMAT_INIT;\n>  \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n\nThis caller in ls-remote is the only one that doesn't otherwise have a\nref_format struct. I don't think it's a big deal to add one like this,\nand it might even be nice to support --format.\n\nBut I suspect there are other weird errors lurking (however we structure\nthis code) because we may not actually have access to the objects! The\nahead-behind atom seems to gloss over that, but others will barf.\nInterestingly we detect the out-of-repo case:\n\n  $ git ls-remote --sort=authordate git.git\n  fatal: not a git repository, but the field 'authordate' requires access to object data\n\nbut not if we have a repo:\n\n  $ git init\n  $ git ls-remote --sort=authordate git.git\n  fatal: missing object 978601ccf7b27399aa349c535b29965e664046c4 for refs/heads/ci-config\n\nI guess it would work _sometimes_ if you've fetched recently from the\nremote. So maybe it does not make sense to outlaw all object-inspecting\natoms for ls-remote, and just say that the error above is the best we\ncan do? It does get weird with ahead-behind, though, as you'll get a\ndifferent sort order depending on whether you've actually fetched the\nobject.\n\nAnyway, all orthogonal to what you're fixing, but kind of gross none the\nless.\n\n-Peff\n"},{"id":"510439","messageId":"xmqqfrlmaaoa.fsf@gitster.g","threadId":"62740","inReplyTo":"20250113051700.GA767856@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-13T17:25:09Z","receivedAt":"2025-01-13T17:25:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> For now there is no program that uses more than one ref-filter format.\n> But it seems like an obvious interface that would want to be lib-ified\n> eventually. We are not there yet because of the static global used_atoms\n> array. But the obvious path forward is to have a context struct\n> representing one ref-filter iteration.\n>\n> I think the intent was that ref_format would be that context struct,\n> though arguably it is a little funny since it forces the sorting and\n> formatting to be joined (OTOH, that is very much how the code works,\n> since it wants to share results between the two for efficiency).\n>\n> So one solution would be to make the use of that context struct more\n> explicit, and require ref_sorting callers to provide a format struct.\n> Like the patch below, which also passes your tests.\n>\n> I dunno. Your patch is deleting more code, which is nice. But I think in\n> the long run we'd end up replacing it. But maybe making a clean slate\n> now would make that easier? I could go either way.\n\nI agree with you that libification effort would want to move more\nstatic variables to members in a context structure.  I initially\nsuspected that would be more or less orthogonal, because it is not\nlike we are adding a static or two to a code path that already holds\neverything else in a context structure, and would be a lot more\nwork, but now you have a patch that makes the first step in that\ndirection and it does not look too bad at all, so ...\n\nThanks.\n\n> ---\n>  builtin/branch.c       |  2 +-\n>  builtin/for-each-ref.c |  2 +-\n>  builtin/ls-remote.c    |  4 +++-\n>  builtin/tag.c          |  2 +-\n>  ref-filter.c           | 19 ++++++++-----------\n>  ref-filter.h           |  2 +-\n>  6 files changed, 15 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 6e7b0cfddb..0c3f35cd0a 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -875,7 +875,7 @@ int cmd_branch(int argc,\n>  \t\t * local branches 'refs/heads/...' and finally remote-tracking\n>  \t\t * branches 'refs/remotes/...'.\n>  \t\t */\n> -\t\tsorting = ref_sorting_options(&sorting_options);\n> +\t\tsorting = ref_sorting_options(&sorting_options, &format);\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> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 715745a262..4f247efe57 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -80,7 +80,7 @@ int cmd_for_each_ref(int argc,\n>  \tif (verify_ref_format(&format))\n>  \t\tusage_with_options(for_each_ref_usage, opts);\n>  \n> -\tsorting = ref_sorting_options(&sorting_options);\n> +\tsorting = ref_sorting_options(&sorting_options, &format);\n>  \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n>  \tfilter.ignore_case = icase;\n>  \n> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\n> index 42f34e1236..ed38b82346 100644\n> --- a/builtin/ls-remote.c\n> +++ b/builtin/ls-remote.c\n> @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc,\n>  \tconst struct ref *ref;\n>  \tstruct ref_array ref_array;\n>  \tstruct ref_sorting *sorting;\n> +\tstruct ref_format format = REF_FORMAT_INIT;\n>  \tstruct string_list sorting_options = STRING_LIST_INIT_DUP;\n>  \n>  \tstruct option options[] = {\n> @@ -155,7 +156,7 @@ int cmd_ls_remote(int argc,\n>  \t\titem->symref = xstrdup_or_null(ref->symref);\n>  \t}\n>  \n> -\tsorting = ref_sorting_options(&sorting_options);\n> +\tsorting = ref_sorting_options(&sorting_options, &format);\n>  \tref_array_sort(sorting, &ref_array);\n>  \n>  \tfor (i = 0; i < ref_array.nr; i++) {\n> @@ -173,6 +174,7 @@ int cmd_ls_remote(int argc,\n>  \t\tstatus = 1;\n>  \ttransport_ls_refs_options_release(&transport_options);\n>  \n> +\tref_format_clear(&format);\n>  \tstrvec_clear(&pattern);\n>  \tstring_list_clear(&server_options, 0);\n>  \treturn status;\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index c4bd145831..a5240f66e2 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -574,7 +574,7 @@ int cmd_tag(int argc,\n>  \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--column\", \"-n\");\n>  \t\tcolopts = 0;\n>  \t}\n> -\tsorting = ref_sorting_options(&sorting_options);\n> +\tsorting = ref_sorting_options(&sorting_options, &format);\n>  \tref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);\n>  \tfilter.ignore_case = icase;\n>  \tif (cmdmode == 'l') {\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 23054694c2..f5d0c448ed 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -3536,23 +3536,19 @@ void pretty_print_ref(const char *name, const struct object_id *oid,\n>  \tfree_array_item(ref_item);\n>  }\n>  \n> -static int parse_sorting_atom(const char *atom)\n> +static int parse_sorting_atom(struct ref_format *format, const char *atom)\n>  {\n> -\t/*\n> -\t * This parses an atom using a dummy ref_format, since we don't\n> -\t * actually care about the formatting details.\n> -\t */\n> -\tstruct ref_format dummy = REF_FORMAT_INIT;\n>  \tconst char *end = atom + strlen(atom);\n>  \tstruct strbuf err = STRBUF_INIT;\n> -\tint res = parse_ref_filter_atom(&dummy, atom, end, &err);\n> +\tint res = parse_ref_filter_atom(format, atom, end, &err);\n>  \tif (res < 0)\n>  \t\tdie(\"%s\", err.buf);\n>  \tstrbuf_release(&err);\n>  \treturn res;\n>  }\n>  \n> -static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)\n> +static void parse_ref_sorting(struct ref_format *format,\n> +\t\t\t      struct ref_sorting **sorting_tail, const char *arg)\n>  {\n>  \tstruct ref_sorting *s;\n>  \n> @@ -3567,17 +3563,18 @@ static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg\n>  \tif (skip_prefix(arg, \"version:\", &arg) ||\n>  \t    skip_prefix(arg, \"v:\", &arg))\n>  \t\ts->sort_flags |= REF_SORTING_VERSION;\n> -\ts->atom = parse_sorting_atom(arg);\n> +\ts->atom = parse_sorting_atom(format, arg);\n>  }\n>  \n> -struct ref_sorting *ref_sorting_options(struct string_list *options)\n> +struct ref_sorting *ref_sorting_options(struct string_list *options,\n> +\t\t\t\t\tstruct ref_format *format)\n>  {\n>  \tstruct string_list_item *item;\n>  \tstruct ref_sorting *sorting = NULL, **tail = &sorting;\n>  \n>  \tif (options->nr) {\n>  \t\tfor_each_string_list_item(item, options)\n> -\t\t\tparse_ref_sorting(tail, item->string);\n> +\t\t\tparse_ref_sorting(format, tail, item->string);\n>  \t}\n>  \n>  \t/*\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 754038ab07..1531bf1762 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -168,7 +168,7 @@ int format_ref_array_item(struct ref_array_item *info,\n>  /* Release a \"struct ref_sorting\" */\n>  void ref_sorting_release(struct ref_sorting *);\n>  /*  Convert list of sort options into ref_sorting */\n> -struct ref_sorting *ref_sorting_options(struct string_list *);\n> +struct ref_sorting *ref_sorting_options(struct string_list *, struct ref_format *);\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"},{"id":"510537","messageId":"57066d48-ce68-42ec-8e94-4ef6c1eba920@web.de","threadId":"62740","inReplyTo":"20250113051700.GA767856@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-14T18:55:03Z","receivedAt":"2025-01-14T18:55:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.01.25 um 06:17 schrieb Jeff King:\n> On Sun, Jan 12, 2025 at 11:01:52AM +0100, René Scharfe wrote:\n>\n>> verify_ref_format() parses a ref-filter format string and stores\n>> recognized items in the static array \"used_atom\".  For\n>> \"ahead-behind:<committish>\" and \"is-base:<committish>\" it stores the\n>> committish part in string_lists that are part of struct ref_format.\n>>\n>> ref_sorting_options() also parses bare ref-filter format items and also\n>> stores recognized ones in \"used_atom\".  The committish parts go to a\n>> dummy struct ref_format in parse_sorting_atom(), though, and are leaked\n>> and forgotten.\n>>\n>> If verify_ref_format() is called before ref_sorting_options(), like in\n>> git for-each-ref, then all works well if the sort key is included in the\n>> format string.  If it isn't then sorting cannot work as the committishes\n>> are missing.\n>>\n>> If ref_sorting_options() is called first, like in git branch, then we\n>> have the additional issue that if the sort key is included in the format\n>> string then filter_ahead_behind() and filter_is_base() can't see their\n>> committishes, will not generate any results for them and thus they will\n>> for expanded to empty strings.\n>\n> Good analysis. The sorting and formatting are definitely tied in subtle\n> ways, and not all code takes that into account.\n>\n> The dummy ref_format here is one such problem. Another is that we don't\n> do the equivalent of verify_ref_format() on the sorting fields. Most of\n> what it does is probably superfluous, but for example it's supposed to\n> reject some atoms that have parsers. So:\n>\n>   $ git for-each-ref --format='%(rest)'\n>   fatal: this command reject atom %(rest)\n>\n> but:\n>\n>   $ git for-each-ref --sort=rest\n>   [...no error...]\n>\n> That's somewhat orthogonal, but it may influence the direction of our\n> solution.\n\nNot nice.  Erroring out would be better than leaving users wondering why\nthat sort arg does nothing.\n\nA totally different thing that bugs me: Calling ahead-behind an atom is\nweird; it's more of a molecule.  It should be possible to add separate\nahead and behind atoms, with scalar values, that we then could sort\nseparately, preferably numerically instead of lexically.\n\n>> Fix those issues by making the string_lists static, like their sibling\n>> \"used_atom\".  This way they can all be shared for handling both\n>> ref-filter format strings and sorting options in the same command.\n>> And since struct ref_format no longer contains any allocated members,\n>> remove the now unnecessary ref_format_init() and ref_format_clear().\n>\n> Hmm. So this certainly fixes the problem. But is it where we want to go\n> in the long run?\n>\n> For now there is no program that uses more than one ref-filter format.\n> But it seems like an obvious interface that would want to be lib-ified\n> eventually. We are not there yet because of the static global used_atoms\n> array. But the obvious path forward is to have a context struct\n> representing one ref-filter iteration.\n>\n> I think the intent was that ref_format would be that context struct,\n> though arguably it is a little funny since it forces the sorting and\n> formatting to be joined (OTOH, that is very much how the code works,\n> since it wants to share results between the two for efficiency).\n>\n> So one solution would be to make the use of that context struct more\n> explicit, and require ref_sorting callers to provide a format struct.\n> Like the patch below, which also passes your tests.\n\nDid that in the first version of the patch.  It works, but keeps the\ncause of the issue unaddressed: The separation of used_atom and the\nstring_lists, which together represent the parsed items.\n\nI'm not convinced that ref_format is the right place for them, but\nhaven't thought this through, admittedly.  struct ref_filter and a\nnew dedicated struct would be alternatives.  Moving used_atom will be\npainful in any case.\n\n> I dunno. Your patch is deleting more code, which is nice. But I think in\n> the long run we'd end up replacing it. But maybe making a clean slate\n> now would make that easier? I could go either way.\n\nI think joining the three structs is better than leaving them apart,\nbut if someone is moving them to their right place soon anyway, be it\nref_format or somewhere else, then I could send the the first version\nof the patch (basically what you sent).\n\nRené\n\n"},{"id":"510653","messageId":"20250116095128.GA144555@coredump.intra.peff.net","threadId":"62740","inReplyTo":"57066d48-ce68-42ec-8e94-4ef6c1eba920@web.de","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T09:51:28Z","receivedAt":"2025-01-16T09:51:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 14, 2025 at 07:55:03PM +0100, René Scharfe wrote:\n\n> A totally different thing that bugs me: Calling ahead-behind an atom is\n> weird; it's more of a molecule.  It should be possible to add separate\n> ahead and behind atoms, with scalar values, that we then could sort\n> separately, preferably numerically instead of lexically.\n\nYeah, I agree that would be the most flexible way to do it. I'm not sure\nif people would find it useful or not (I've never even used the\nahead-behind atom myself).\n\n> > So one solution would be to make the use of that context struct more\n> > explicit, and require ref_sorting callers to provide a format struct.\n> > Like the patch below, which also passes your tests.\n> \n> Did that in the first version of the patch.  It works, but keeps the\n> cause of the issue unaddressed: The separation of used_atom and the\n> string_lists, which together represent the parsed items.\n\nRight. I think the end goal is: all pieces of data together in a\nnon-global struct. Anything short of that has to choose the least-bad\noption. ;)\n\nIn a sense, having _anything_ global means that there is little point in\nhaving a struct at all (since it is still not safe to be called twice).\nBut it feels like that's a necessary step on the way, and getting rid of\nthe struct is a step backwards.\n\nI dunno. I still have dreams about rewriting ref-filter completely for\nbetter clarity and efficiency.\n\n> I'm not convinced that ref_format is the right place for them, but\n> haven't thought this through, admittedly.  struct ref_filter and a\n> new dedicated struct would be alternatives.  Moving used_atom will be\n> painful in any case.\n\nYet another option in the near term might be storing these ahead-behind\nbits in the individual atoms. Since the point is to do a single\ntraversal, we'd have to marshal them into a unified data structure at\nsome point. But we already do that! In filter_ahead_behind() we convert\nthe string list into an array (and ironically do not even look at the\nstrings, only their \"util\" fields).\n\nSo something like this (only lightly tested) seems to work:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 23054694c2..4c10b6fe75 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -235,6 +235,9 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n+\t\tstruct {\n+\t\t\tstruct commit *commit;\n+\t\t} base;\n \t\tstruct strvec describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n@@ -892,17 +895,14 @@ static int rest_atom_parser(struct ref_format *format UNUSED,\n }\n \n static int ahead_behind_atom_parser(struct ref_format *format,\n-\t\t\t\t    struct used_atom *atom UNUSED,\n+\t\t\t\t    struct used_atom *atom,\n \t\t\t\t    const char *arg, struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n-\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(ahead-behind:<committish>)\"));\n \n-\titem = string_list_append(&format->bases, arg);\n-\titem->util = lookup_commit_reference_by_name(arg);\n-\tif (!item->util)\n+\tatom->u.base.commit = lookup_commit_reference_by_name(arg);\n+\tif (!atom->u.base.commit)\n \t\tdie(\"failed to find '%s'\", arg);\n \n \treturn 0;\n@@ -3088,18 +3088,31 @@ void filter_ahead_behind(struct repository *r,\n \t\t\t struct ref_array *array)\n {\n \tstruct commit **commits;\n-\tsize_t commits_nr = format->bases.nr + array->nr;\n+\tsize_t bases_nr, commits_nr;\n \n-\tif (!format->bases.nr || !array->nr)\n+\tif (!array->nr)\n \t\treturn;\n \n+\tbases_nr = 0;\n+\tfor (size_t i = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\tbases_nr++;\n+\t}\n+\n+\tif (!bases_nr)\n+\t\treturn;\n+\n+\tcommits_nr = bases_nr + array->nr;\n+\n \tALLOC_ARRAY(commits, commits_nr);\n-\tfor (size_t i = 0; i < format->bases.nr; i++)\n-\t\tcommits[i] = format->bases.items[i].util;\n+\tfor (size_t i = 0, j = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\tcommits[j++] = used_atom[i].u.base.commit;\n+\t}\n \n-\tALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr));\n+\tALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr));\n \n-\tcommits_nr = format->bases.nr;\n+\tcommits_nr = bases_nr;\n \tarray->counts_nr = 0;\n \tfor (size_t i = 0; i < array->nr; i++) {\n \t\tconst char *name = array->items[i]->refname;\n@@ -3108,8 +3121,8 @@ void filter_ahead_behind(struct repository *r,\n \t\tif (!commits[commits_nr])\n \t\t\tcontinue;\n \n-\t\tCALLOC_ARRAY(array->items[i]->counts, format->bases.nr);\n-\t\tfor (size_t j = 0; j < format->bases.nr; j++) {\n+\t\tCALLOC_ARRAY(array->items[i]->counts, bases_nr);\n+\t\tfor (size_t j = 0; j < bases_nr; j++) {\n \t\t\tstruct ahead_behind_count *count;\n \t\t\tcount = &array->counts[array->counts_nr++];\n \t\t\tcount->tip_index = commits_nr;\n@@ -3277,9 +3290,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \t * - filtering on reachability\n \t * - including ahead-behind information in the formatted output\n \t */\n+\tfor (size_t i = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\treturn 0;\n+\t}\n \treturn !(filter->reachable_from ||\n \t\t filter->unreachable_from ||\n-\t\t format->bases.nr ||\n \t\t format->is_base_tips.nr);\n }\n \n@@ -3647,7 +3663,6 @@ void ref_format_init(struct ref_format *format)\n \n void ref_format_clear(struct ref_format *format)\n {\n-\tstring_list_clear(&format->bases, 0);\n \tstring_list_clear(&format->is_base_tips, 0);\n \tref_format_init(format);\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 754038ab07..d048317802 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -99,9 +99,6 @@ struct ref_format {\n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n \n-\t/* List of bases for ahead-behind counts. */\n-\tstruct string_list bases;\n-\n \t/* List of bases for is-base indicators. */\n \tstruct string_list is_base_tips;\n \n@@ -117,7 +114,6 @@ struct ref_format {\n }\n #define REF_FORMAT_INIT {             \\\n \t.use_color = -1,              \\\n-\t.bases = STRING_LIST_INIT_DUP, \\\n \t.is_base_tips = STRING_LIST_INIT_DUP, \\\n }\n \n\nWe should be able to do the same thing with is_base_tips, but I've left\nit as an exercise for the reader. ;)\n\n-Peff\n"},{"id":"510658","messageId":"20250116100637.GB144555@coredump.intra.peff.net","threadId":"62740","inReplyTo":"20250116095128.GA144555@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T10:06:37Z","receivedAt":"2025-01-16T10:06:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2025 at 04:51:28AM -0500, Jeff King wrote:\n\n> Yet another option in the near term might be storing these ahead-behind\n> bits in the individual atoms. Since the point is to do a single\n> traversal, we'd have to marshal them into a unified data structure at\n> some point. But we already do that! In filter_ahead_behind() we convert\n> the string list into an array (and ironically do not even look at the\n> strings, only their \"util\" fields).\n> \n> So something like this (only lightly tested) seems to work:\n\nI compiled it without DEVELOPER=1, so I missed a few unused parameters.\nWe'd want this on top:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..fbb9536282 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -473,7 +473,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tif (verify_ref_format(format))\n \t\tdie(_(\"unable to parse format string\"));\n \n-\tfilter_ahead_behind(the_repository, format, &array);\n+\tfilter_ahead_behind(the_repository, &array);\n \tref_array_sort(sorting, &array);\n \n \tif (column_active(colopts)) {\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4c10b6fe75..0de51f13e6 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -894,7 +894,7 @@ static int rest_atom_parser(struct ref_format *format UNUSED,\n \treturn 0;\n }\n \n-static int ahead_behind_atom_parser(struct ref_format *format,\n+static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n \t\t\t\t    struct used_atom *atom,\n \t\t\t\t    const char *arg, struct strbuf *err)\n {\n@@ -3084,7 +3084,6 @@ static void reach_filter(struct ref_array *array,\n }\n \n void filter_ahead_behind(struct repository *r,\n-\t\t\t struct ref_format *format,\n \t\t\t struct ref_array *array)\n {\n \tstruct commit **commits;\n@@ -3319,7 +3318,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t} else {\n \t\tstruct ref_array array = { 0 };\n \t\tfilter_refs(&array, filter, type);\n-\t\tfilter_ahead_behind(the_repository, format, &array);\n+\t\tfilter_ahead_behind(the_repository, &array);\n \t\tfilter_is_base(the_repository, format, &array);\n \t\tref_array_sort(sorting, &array);\n \t\tprint_formatted_ref_array(&array, format);\ndiff --git a/ref-filter.h b/ref-filter.h\nindex d048317802..5f3dd6c931 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -201,7 +201,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array,\n  * If this is not called, then any ahead-behind atoms will be blank.\n  */\n void filter_ahead_behind(struct repository *r,\n-\t\t\t struct ref_format *format,\n \t\t\t struct ref_array *array);\n \n /*\n\nGetting rid of the ref_format argument to filter_ahead_behind makes it\neven more plain that we are operating on magic global data behind the\nscenes. ;) So in that sense this is all the same step back that I was\ncomplaining about from your patch; eventually we'd need to pass the\nused_atom array to filter_ahead_behind() via some struct. But somehow it\nseems less bad to me because it is all being folded into the existing\nglobal structure.\n\n-Peff\n"},{"id":"510672","messageId":"20250116102154.GA724126@coredump.intra.peff.net","threadId":"62740","inReplyTo":"20250116100637.GB144555@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T10:21:54Z","receivedAt":"2025-01-16T10:21:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2025 at 05:06:37AM -0500, Jeff King wrote:\n\n> On Thu, Jan 16, 2025 at 04:51:28AM -0500, Jeff King wrote:\n> \n> > Yet another option in the near term might be storing these ahead-behind\n> > bits in the individual atoms. Since the point is to do a single\n> > traversal, we'd have to marshal them into a unified data structure at\n> > some point. But we already do that! In filter_ahead_behind() we convert\n> > the string list into an array (and ironically do not even look at the\n> > strings, only their \"util\" fields).\n> > \n> > So something like this (only lightly tested) seems to work:\n> \n> I compiled it without DEVELOPER=1, so I missed a few unused parameters.\n> We'd want this on top:\n\nAnd one final thought on this approach: if we do want to do it, perhaps\nit would make sense to build on top of the patch you sent. I think yours\nfixes the bug in a more direct and obvious way, and then my approach\nwould merely be internal reorganization on top.\n\nAt any rate, here is the is_base conversion for posterity.\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0de51f13e6..28d48ec585 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -236,6 +236,7 @@ static struct used_atom {\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n \t\tstruct {\n+\t\t\tchar *name;\n \t\t\tstruct commit *commit;\n \t\t} base;\n \t\tstruct strvec describe_args;\n@@ -908,18 +909,16 @@ static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n \treturn 0;\n }\n \n-static int is_base_atom_parser(struct ref_format *format,\n-\t\t\t       struct used_atom *atom UNUSED,\n+static int is_base_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t       struct used_atom *atom,\n \t\t\t       const char *arg, struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n-\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(is-base:<committish>)\"));\n \n-\titem = string_list_append(&format->is_base_tips, arg);\n-\titem->util = lookup_commit_reference_by_name(arg);\n-\tif (!item->util)\n+\tatom->u.base.name = xstrdup(arg);\n+\tatom->u.base.commit = lookup_commit_reference_by_name(arg);\n+\tif (!atom->u.base.commit)\n \t\tdie(\"failed to find '%s'\", arg);\n \n \treturn 0;\n@@ -3137,14 +3136,20 @@ void filter_ahead_behind(struct repository *r,\n }\n \n void filter_is_base(struct repository *r,\n-\t\t    struct ref_format *format,\n \t\t    struct ref_array *array)\n {\n \tstruct commit **bases;\n \tsize_t bases_nr = 0;\n \tstruct ref_array_item **back_index;\n+\tsize_t atoms_nr;\n \n-\tif (!format->is_base_tips.nr || !array->nr)\n+\tatoms_nr = 0;\n+\tfor (size_t i = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_ISBASE)\n+\t\t\tatoms_nr++;\n+\t}\n+\n+\tif (!atoms_nr || !array->nr)\n \t\treturn;\n \n \tCALLOC_ARRAY(back_index, array->nr);\n@@ -3154,7 +3159,7 @@ void filter_is_base(struct repository *r,\n \t\tconst char *name = array->items[i]->refname;\n \t\tstruct commit *c = lookup_commit_reference_by_name_gently(name, 1);\n \n-\t\tCALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr);\n+\t\tCALLOC_ARRAY(array->items[i]->is_base, atoms_nr);\n \n \t\tif (!c)\n \t\t\tcontinue;\n@@ -3164,15 +3169,21 @@ void filter_is_base(struct repository *r,\n \t\tbases_nr++;\n \t}\n \n-\tfor (size_t i = 0; i < format->is_base_tips.nr; i++) {\n-\t\tstruct commit *tip = format->is_base_tips.items[i].util;\n-\t\tint base_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n+\tfor (size_t i = 0, j = 0; i < used_atom_cnt; i++) {\n+\t\tstruct commit *tip;\n+\t\tint base_index;\n+\n+\t\tif (used_atom[i].atom_type != ATOM_ISBASE)\n+\t\t\tcontinue;\n+\n+\t\ttip = used_atom[i].u.base.commit;\n+\t\tbase_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n \n \t\tif (base_index < 0)\n \t\t\tcontinue;\n \n \t\t/* Store the string for use in output later. */\n-\t\tback_index[base_index]->is_base[i] = xstrdup(format->is_base_tips.items[i].string);\n+\t\tback_index[base_index]->is_base[j++] = xstrdup(used_atom[i].u.base.name);\n \t}\n \n \tfree(back_index);\n@@ -3264,8 +3275,7 @@ struct ref_sorting {\n };\n \n static inline int can_do_iterative_format(struct ref_filter *filter,\n-\t\t\t\t\t  struct ref_sorting *sorting,\n-\t\t\t\t\t  struct ref_format *format)\n+\t\t\t\t\t  struct ref_sorting *sorting)\n {\n \t/*\n \t * Reference backends sort patterns lexicographically by refname, so if\n@@ -3292,17 +3302,18 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \tfor (size_t i = 0; i < used_atom_cnt; i++) {\n \t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n \t\t\treturn 0;\n+\t\tif (used_atom[i].atom_type == ATOM_ISBASE)\n+\t\t\treturn 0;\n \t}\n \treturn !(filter->reachable_from ||\n-\t\t filter->unreachable_from ||\n-\t\t format->is_base_tips.nr);\n+\t\t filter->unreachable_from);\n }\n \n void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t\t\t    struct ref_sorting *sorting,\n \t\t\t    struct ref_format *format)\n {\n-\tif (can_do_iterative_format(filter, sorting, format)) {\n+\tif (can_do_iterative_format(filter, sorting)) {\n \t\tint save_commit_buffer_orig;\n \t\tstruct ref_filter_and_format_cbdata ref_cbdata = {\n \t\t\t.filter = filter,\n@@ -3319,7 +3330,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t\tstruct ref_array array = { 0 };\n \t\tfilter_refs(&array, filter, type);\n \t\tfilter_ahead_behind(the_repository, &array);\n-\t\tfilter_is_base(the_repository, format, &array);\n+\t\tfilter_is_base(the_repository, &array);\n \t\tref_array_sort(sorting, &array);\n \t\tprint_formatted_ref_array(&array, format);\n \t\tref_array_clear(&array);\n@@ -3662,6 +3673,5 @@ void ref_format_init(struct ref_format *format)\n \n void ref_format_clear(struct ref_format *format)\n {\n-\tstring_list_clear(&format->is_base_tips, 0);\n \tref_format_init(format);\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 5f3dd6c931..0ba94df651 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -99,9 +99,6 @@ struct ref_format {\n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n \n-\t/* List of bases for is-base indicators. */\n-\tstruct string_list is_base_tips;\n-\n \tstruct {\n \t\tint max_count;\n \t\tint omit_empty;\n@@ -114,7 +111,6 @@ struct ref_format {\n }\n #define REF_FORMAT_INIT {             \\\n \t.use_color = -1,              \\\n-\t.is_base_tips = STRING_LIST_INIT_DUP, \\\n }\n \n /*  Macros for checking --merged and --no-merged options */\n@@ -210,7 +206,6 @@ void filter_ahead_behind(struct repository *r,\n  * If this is not called, then any is-base atoms will be blank.\n  */\n void filter_is_base(struct repository *r,\n-\t\t    struct ref_format *format,\n \t\t    struct ref_array *array);\n \n void ref_filter_init(struct ref_filter *filter);\n"},{"id":"510721","messageId":"xmqqr052sm16.fsf@gitster.g","threadId":"62740","inReplyTo":"20250116102154.GA724126@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T17:31:33Z","receivedAt":"2025-01-16T17:31:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 16, 2025 at 05:06:37AM -0500, Jeff King wrote:\n>\n>> On Thu, Jan 16, 2025 at 04:51:28AM -0500, Jeff King wrote:\n>> \n>> > Yet another option in the near term might be storing these ahead-behind\n>> > bits in the individual atoms. Since the point is to do a single\n>> > traversal, we'd have to marshal them into a unified data structure at\n>> > some point. But we already do that! In filter_ahead_behind() we convert\n>> > the string list into an array (and ironically do not even look at the\n>> > strings, only their \"util\" fields).\n>> > \n>> > So something like this (only lightly tested) seems to work:\n>> \n>> I compiled it without DEVELOPER=1, so I missed a few unused parameters.\n>> We'd want this on top:\n>\n> And one final thought on this approach: if we do want to do it, perhaps\n> it would make sense to build on top of the patch you sent. I think yours\n> fixes the bug in a more direct and obvious way, and then my approach\n> would merely be internal reorganization on top.\n>\n> At any rate, here is the is_base conversion for posterity.\n\nLooking good.  Thanks.\n"},{"id":"510875","messageId":"400bbcae-5dc8-4263-b5c4-86884c54c37c@web.de","threadId":"62740","inReplyTo":"20250116095128.GA144555@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-18T17:11:21Z","receivedAt":"2025-01-18T17:11:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.01.25 um 10:51 schrieb Jeff King:\n> Yet another option in the near term might be storing these ahead-behind\n> bits in the individual atoms.\n\nThat makes a lot of sense.\n\nRené\n\n"},{"id":"510876","messageId":"08fbb64d-8d01-4c24-8072-f6065b4f4ab9@web.de","threadId":"62740","inReplyTo":"6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de","subject":"[PATCH v2 1/3] ref-filter: move ahead-behind bases into used_atom","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-18T17:11:34Z","receivedAt":"2025-01-18T17:11:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"verify_ref_format() parses a ref-filter format string and stores\nrecognized items in the static array \"used_atom\".  For\n\"ahead-behind:<committish>\" it stores the committish part in a\nstring_list member \"bases\" of struct ref_format.\n\nref_sorting_options() also parses bare ref-filter format items and\nstores stores recognized ones in \"used_atom\" as well.  The committish\nparts go to a dummy struct ref_format in parse_sorting_atom(), though,\nand are leaked and forgotten.\n\nIf verify_ref_format() is called before ref_sorting_options(), like in\ngit for-each-ref, then all works well if the sort key is included in the\nformat string.  If it isn't then sorting cannot work as the committishes\nare missing.\n\nIf ref_sorting_options() is called first, like in git branch, then we\nhave the additional issue that if the sort key is included in the format\nstring then filter_ahead_behind() can't see its committish, will not\ngenerate any results for it and thus it will be expanded to an empty\nstring.\n\nFix those issues by replacing the string_list with a field in used_atom\nfor storing the committish.  This way it can be shared for handling both\nref-filter format strings and sorting options in the same command.\n\nReported-by: Ross Goldberg <ross.goldberg@gmail.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/branch.c         |  2 +-\n ref-filter.c             | 50 ++++++++++++++++++++++++----------------\n ref-filter.h             |  5 ----\n t/t3203-branch-output.sh | 28 ++++++++++++++++++++++\n 4 files changed, 59 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..fbb9536282 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -473,7 +473,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tif (verify_ref_format(format))\n \t\tdie(_(\"unable to parse format string\"));\n\n-\tfilter_ahead_behind(the_repository, format, &array);\n+\tfilter_ahead_behind(the_repository, &array);\n \tref_array_sort(sorting, &array);\n\n \tif (column_active(colopts)) {\ndiff --git a/ref-filter.c b/ref-filter.c\nindex bf5534605e..c3957473e5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -235,6 +235,9 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n+\t\tstruct {\n+\t\t\tstruct commit *commit;\n+\t\t} base;\n \t\tstruct strvec describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n@@ -891,18 +894,15 @@ static int rest_atom_parser(struct ref_format *format UNUSED,\n \treturn 0;\n }\n\n-static int ahead_behind_atom_parser(struct ref_format *format,\n-\t\t\t\t    struct used_atom *atom UNUSED,\n+static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t\t    struct used_atom *atom,\n \t\t\t\t    const char *arg, struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n-\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(ahead-behind:<committish>)\"));\n\n-\titem = string_list_append(&format->bases, arg);\n-\titem->util = lookup_commit_reference_by_name(arg);\n-\tif (!item->util)\n+\tatom->u.base.commit = lookup_commit_reference_by_name(arg);\n+\tif (!atom->u.base.commit)\n \t\tdie(\"failed to find '%s'\", arg);\n\n \treturn 0;\n@@ -3084,22 +3084,30 @@ static void reach_filter(struct ref_array *array,\n }\n\n void filter_ahead_behind(struct repository *r,\n-\t\t\t struct ref_format *format,\n \t\t\t struct ref_array *array)\n {\n \tstruct commit **commits;\n-\tsize_t commits_nr = format->bases.nr + array->nr;\n+\tsize_t bases_nr, commits_nr;\n\n-\tif (!format->bases.nr || !array->nr)\n+\tif (!array->nr)\n \t\treturn;\n\n-\tALLOC_ARRAY(commits, commits_nr);\n-\tfor (size_t i = 0; i < format->bases.nr; i++)\n-\t\tcommits[i] = format->bases.items[i].util;\n+\tfor (size_t i = bases_nr = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\tbases_nr++;\n+\t}\n+\tif (!bases_nr)\n+\t\treturn;\n\n-\tALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr));\n+\tALLOC_ARRAY(commits, st_add(bases_nr, array->nr));\n+\tfor (size_t i = 0, j = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\tcommits[j++] = used_atom[i].u.base.commit;\n+\t}\n\n-\tcommits_nr = format->bases.nr;\n+\tALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr));\n+\n+\tcommits_nr = bases_nr;\n \tarray->counts_nr = 0;\n \tfor (size_t i = 0; i < array->nr; i++) {\n \t\tconst char *name = array->items[i]->refname;\n@@ -3108,8 +3116,8 @@ void filter_ahead_behind(struct repository *r,\n \t\tif (!commits[commits_nr])\n \t\t\tcontinue;\n\n-\t\tCALLOC_ARRAY(array->items[i]->counts, format->bases.nr);\n-\t\tfor (size_t j = 0; j < format->bases.nr; j++) {\n+\t\tCALLOC_ARRAY(array->items[i]->counts, bases_nr);\n+\t\tfor (size_t j = 0; j < bases_nr; j++) {\n \t\t\tstruct ahead_behind_count *count;\n \t\t\tcount = &array->counts[array->counts_nr++];\n \t\t\tcount->tip_index = commits_nr;\n@@ -3277,9 +3285,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \t * - filtering on reachability\n \t * - including ahead-behind information in the formatted output\n \t */\n+\tfor (size_t i = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\treturn 0;\n+\t}\n \treturn !(filter->reachable_from ||\n \t\t filter->unreachable_from ||\n-\t\t format->bases.nr ||\n \t\t format->is_base_tips.nr);\n }\n\n@@ -3303,7 +3314,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t} else {\n \t\tstruct ref_array array = { 0 };\n \t\tfilter_refs(&array, filter, type);\n-\t\tfilter_ahead_behind(the_repository, format, &array);\n+\t\tfilter_ahead_behind(the_repository, &array);\n \t\tfilter_is_base(the_repository, format, &array);\n \t\tref_array_sort(sorting, &array);\n \t\tprint_formatted_ref_array(&array, format);\n@@ -3647,7 +3658,6 @@ void ref_format_init(struct ref_format *format)\n\n void ref_format_clear(struct ref_format *format)\n {\n-\tstring_list_clear(&format->bases, 0);\n \tstring_list_clear(&format->is_base_tips, 0);\n \tref_format_init(format);\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 754038ab07..5f3dd6c931 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -99,9 +99,6 @@ struct ref_format {\n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n\n-\t/* List of bases for ahead-behind counts. */\n-\tstruct string_list bases;\n-\n \t/* List of bases for is-base indicators. */\n \tstruct string_list is_base_tips;\n\n@@ -117,7 +114,6 @@ struct ref_format {\n }\n #define REF_FORMAT_INIT {             \\\n \t.use_color = -1,              \\\n-\t.bases = STRING_LIST_INIT_DUP, \\\n \t.is_base_tips = STRING_LIST_INIT_DUP, \\\n }\n\n@@ -205,7 +201,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array,\n  * If this is not called, then any ahead-behind atoms will be blank.\n  */\n void filter_ahead_behind(struct repository *r,\n-\t\t\t struct ref_format *format,\n \t\t\t struct ref_array *array);\n\n /*\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 500c9d0e72..a6bd88a58d 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -368,6 +368,34 @@ test_expect_success 'git branch --format with ahead-behind' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success 'git branch `--sort=[-]ahead-behind` option' '\n+\tcat >expect <<-\\EOF &&\n+\t(HEAD detached from fromtag) 0 0\n+\trefs/heads/ambiguous 0 0\n+\trefs/heads/branch-two 0 0\n+\trefs/heads/branch-one 1 0\n+\trefs/heads/main 1 0\n+\trefs/heads/ref-to-branch 1 0\n+\trefs/heads/ref-to-remote 1 0\n+\tEOF\n+\tgit branch --format=\"%(refname) %(ahead-behind:HEAD)\" \\\n+\t\t--sort=refname --sort=ahead-behind:HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\t(HEAD detached from fromtag) 0 0\n+\trefs/heads/branch-one 1 0\n+\trefs/heads/main 1 0\n+\trefs/heads/ref-to-branch 1 0\n+\trefs/heads/ref-to-remote 1 0\n+\trefs/heads/ambiguous 0 0\n+\trefs/heads/branch-two 0 0\n+\tEOF\n+\tgit branch --format=\"%(refname) %(ahead-behind:HEAD)\" \\\n+\t\t--sort=refname --sort=-ahead-behind:HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git branch with --format=%(rest) must fail' '\n \ttest_must_fail git branch --format=\"%(rest)\" >actual\n '\n--\n2.48.1\n"},{"id":"510877","messageId":"bd53e91c-f33d-43cd-b650-2ed93e0b963b@web.de","threadId":"62740","inReplyTo":"6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de","subject":"[PATCH v2 3/3] ref-filter: remove ref_format_clear()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-18T17:11:59Z","receivedAt":"2025-01-18T17:12:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Now that ref_format_clear() no longer releases any memory we don't need\nit anymore.  Remove it and its counterpart, ref_format_init().\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/branch.c       |  1 -\n builtin/for-each-ref.c |  1 -\n builtin/tag.c          |  1 -\n builtin/verify-tag.c   |  1 -\n ref-filter.c           | 11 -----------\n ref-filter.h           |  3 ---\n 6 files changed, 18 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex fbb9536282..9a29de5bf1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -884,7 +884,6 @@ int cmd_branch(int argc,\n \t\tstring_list_clear(&output, 0);\n \t\tref_sorting_release(sorting);\n \t\tref_filter_clear(&filter);\n-\t\tref_format_clear(&format);\n\n \t\tret = 0;\n \t\tgoto out;\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 715745a262..8085ebd8fe 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -108,7 +108,6 @@ int cmd_for_each_ref(int argc,\n \tfilter_and_format_refs(&filter, flags, sorting, &format);\n\n \tref_filter_clear(&filter);\n-\tref_format_clear(&format);\n \tref_sorting_release(sorting);\n \tstrvec_clear(&vec);\n \treturn 0;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex c4bd145831..e8a344b926 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -698,7 +698,6 @@ int cmd_tag(int argc,\n cleanup:\n \tref_sorting_release(sorting);\n \tref_filter_clear(&filter);\n-\tref_format_clear(&format);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&ref);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex a7f20618ff..f6b97048a5 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -69,6 +69,5 @@ int cmd_verify_tag(int argc,\n \t\tif (format.format)\n \t\t\tpretty_print_ref(name, &oid, &format);\n \t}\n-\tref_format_clear(&format);\n \treturn had_error;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 9a4b2d4cea..6da8d4c03b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -3660,14 +3660,3 @@ void ref_filter_clear(struct ref_filter *filter)\n \tfree_commit_list(filter->unreachable_from);\n \tref_filter_init(filter);\n }\n-\n-void ref_format_init(struct ref_format *format)\n-{\n-\tstruct ref_format blank = REF_FORMAT_INIT;\n-\tmemcpy(format, &blank, sizeof(blank));\n-}\n-\n-void ref_format_clear(struct ref_format *format)\n-{\n-\tref_format_init(format);\n-}\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 0ba94df651..013d4cfa64 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -211,7 +211,4 @@ void filter_is_base(struct repository *r,\n void ref_filter_init(struct ref_filter *filter);\n void ref_filter_clear(struct ref_filter *filter);\n\n-void ref_format_init(struct ref_format *format);\n-void ref_format_clear(struct ref_format *format);\n-\n #endif /*  REF_FILTER_H  */\n--\n2.48.1\n"},{"id":"510878","messageId":"2fbe47c2-22c0-49ff-a211-057bd96ddbc9@web.de","threadId":"62740","inReplyTo":"20250116102154.GA724126@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-18T17:11:25Z","receivedAt":"2025-01-18T17:16:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.01.25 um 11:21 schrieb Jeff King:\n> And one final thought on this approach: if we do want to do it, perhaps\n> it would make sense to build on top of the patch you sent. I think yours\n> fixes the bug in a more direct and obvious way, and then my approach\n> would merely be internal reorganization on top.\n\nI think moving to the right place in one step requires less churn.  Let's\ncompare; v2 coming up.\n\nRené\n\n"},{"id":"510879","messageId":"37f98b6b-b49b-4c17-b157-e15e14dbc91d@web.de","threadId":"62740","inReplyTo":"6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de","subject":"[PATCH v2 2/3] ref-filter: move is-base tip to used_atom","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-18T17:11:51Z","receivedAt":"2025-01-18T17:17:13Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The string_list \"is_base_tips\" in struct ref_format stores the\ncommittish part of \"is-base:<committish>\".  It has the same problems\nthat its sibling string_list \"bases\" had.  Fix them the same way as the\nprevious commit did for the latter, by replacing the string_list with\nfields in \"used_atom\".\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n ref-filter.c          | 56 +++++++++++++++++++++++++------------------\n ref-filter.h          |  5 ----\n t/t6600-test-reach.sh | 29 ++++++++++++++++++++++\n 3 files changed, 62 insertions(+), 28 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex c3957473e5..9a4b2d4cea 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -236,6 +236,7 @@ static struct used_atom {\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n \t\tstruct {\n+\t\t\tchar *name;\n \t\t\tstruct commit *commit;\n \t\t} base;\n \t\tstruct strvec describe_args;\n@@ -908,18 +909,16 @@ static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n \treturn 0;\n }\n\n-static int is_base_atom_parser(struct ref_format *format,\n-\t\t\t       struct used_atom *atom UNUSED,\n+static int is_base_atom_parser(struct ref_format *format UNUSED,\n+\t\t\t       struct used_atom *atom,\n \t\t\t       const char *arg, struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n-\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(is-base:<committish>)\"));\n\n-\titem = string_list_append(&format->is_base_tips, arg);\n-\titem->util = lookup_commit_reference_by_name(arg);\n-\tif (!item->util)\n+\tatom->u.base.name = xstrdup(arg);\n+\tatom->u.base.commit = lookup_commit_reference_by_name(arg);\n+\tif (!atom->u.base.commit)\n \t\tdie(\"failed to find '%s'\", arg);\n\n \treturn 0;\n@@ -3009,6 +3008,8 @@ void ref_array_clear(struct ref_array *array)\n \t\t\tfree(atom->u.head);\n \t\telse if (atom->atom_type == ATOM_DESCRIBE)\n \t\t\tstrvec_clear(&atom->u.describe_args);\n+\t\telse if (atom->atom_type == ATOM_ISBASE)\n+\t\t\tfree(atom->u.base.name);\n \t\telse if (atom->atom_type == ATOM_TRAILERS ||\n \t\t\t (atom->atom_type == ATOM_CONTENTS &&\n \t\t\t  atom->u.contents.option == C_TRAILERS)) {\n@@ -3133,14 +3134,20 @@ void filter_ahead_behind(struct repository *r,\n }\n\n void filter_is_base(struct repository *r,\n-\t\t    struct ref_format *format,\n \t\t    struct ref_array *array)\n {\n \tstruct commit **bases;\n-\tsize_t bases_nr = 0;\n+\tsize_t bases_nr = 0, is_base_nr;\n \tstruct ref_array_item **back_index;\n\n-\tif (!format->is_base_tips.nr || !array->nr)\n+\tif (!array->nr)\n+\t\treturn;\n+\n+\tfor (size_t i = is_base_nr = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_ISBASE)\n+\t\t\tis_base_nr++;\n+\t}\n+\tif (!is_base_nr)\n \t\treturn;\n\n \tCALLOC_ARRAY(back_index, array->nr);\n@@ -3150,7 +3157,7 @@ void filter_is_base(struct repository *r,\n \t\tconst char *name = array->items[i]->refname;\n \t\tstruct commit *c = lookup_commit_reference_by_name_gently(name, 1);\n\n-\t\tCALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr);\n+\t\tCALLOC_ARRAY(array->items[i]->is_base, is_base_nr);\n\n \t\tif (!c)\n \t\t\tcontinue;\n@@ -3160,15 +3167,20 @@ void filter_is_base(struct repository *r,\n \t\tbases_nr++;\n \t}\n\n-\tfor (size_t i = 0; i < format->is_base_tips.nr; i++) {\n-\t\tstruct commit *tip = format->is_base_tips.items[i].util;\n-\t\tint base_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n+\tfor (size_t i = 0, j = 0; i < used_atom_cnt; i++) {\n+\t\tstruct commit *tip;\n+\t\tint base_index;\n+\n+\t\tif (used_atom[i].atom_type != ATOM_ISBASE)\n+\t\t\tcontinue;\n\n+\t\ttip = used_atom[i].u.base.commit;\n+\t\tbase_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n \t\tif (base_index < 0)\n \t\t\tcontinue;\n\n \t\t/* Store the string for use in output later. */\n-\t\tback_index[base_index]->is_base[i] = xstrdup(format->is_base_tips.items[i].string);\n+\t\tback_index[base_index]->is_base[j++] = xstrdup(used_atom[i].u.base.name);\n \t}\n\n \tfree(back_index);\n@@ -3260,8 +3272,7 @@ struct ref_sorting {\n };\n\n static inline int can_do_iterative_format(struct ref_filter *filter,\n-\t\t\t\t\t  struct ref_sorting *sorting,\n-\t\t\t\t\t  struct ref_format *format)\n+\t\t\t\t\t  struct ref_sorting *sorting)\n {\n \t/*\n \t * Reference backends sort patterns lexicographically by refname, so if\n@@ -3288,17 +3299,17 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \tfor (size_t i = 0; i < used_atom_cnt; i++) {\n \t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n \t\t\treturn 0;\n+\t\tif (used_atom[i].atom_type == ATOM_ISBASE)\n+\t\t\treturn 0;\n \t}\n-\treturn !(filter->reachable_from ||\n-\t\t filter->unreachable_from ||\n-\t\t format->is_base_tips.nr);\n+\treturn !(filter->reachable_from || filter->unreachable_from);\n }\n\n void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t\t\t    struct ref_sorting *sorting,\n \t\t\t    struct ref_format *format)\n {\n-\tif (can_do_iterative_format(filter, sorting, format)) {\n+\tif (can_do_iterative_format(filter, sorting)) {\n \t\tint save_commit_buffer_orig;\n \t\tstruct ref_filter_and_format_cbdata ref_cbdata = {\n \t\t\t.filter = filter,\n@@ -3315,7 +3326,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n \t\tstruct ref_array array = { 0 };\n \t\tfilter_refs(&array, filter, type);\n \t\tfilter_ahead_behind(the_repository, &array);\n-\t\tfilter_is_base(the_repository, format, &array);\n+\t\tfilter_is_base(the_repository, &array);\n \t\tref_array_sort(sorting, &array);\n \t\tprint_formatted_ref_array(&array, format);\n \t\tref_array_clear(&array);\n@@ -3658,6 +3669,5 @@ void ref_format_init(struct ref_format *format)\n\n void ref_format_clear(struct ref_format *format)\n {\n-\tstring_list_clear(&format->is_base_tips, 0);\n \tref_format_init(format);\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 5f3dd6c931..0ba94df651 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -99,9 +99,6 @@ struct ref_format {\n \t/* Internal state to ref-filter */\n \tint need_color_reset_at_eol;\n\n-\t/* List of bases for is-base indicators. */\n-\tstruct string_list is_base_tips;\n-\n \tstruct {\n \t\tint max_count;\n \t\tint omit_empty;\n@@ -114,7 +111,6 @@ struct ref_format {\n }\n #define REF_FORMAT_INIT {             \\\n \t.use_color = -1,              \\\n-\t.is_base_tips = STRING_LIST_INIT_DUP, \\\n }\n\n /*  Macros for checking --merged and --no-merged options */\n@@ -210,7 +206,6 @@ void filter_ahead_behind(struct repository *r,\n  * If this is not called, then any is-base atoms will be blank.\n  */\n void filter_is_base(struct repository *r,\n-\t\t    struct ref_format *format,\n \t\t    struct ref_array *array);\n\n void ref_filter_init(struct ref_filter *filter);\ndiff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh\nindex 2591f8b8b3..6638d1aa1d 100755\n--- a/t/t6600-test-reach.sh\n+++ b/t/t6600-test-reach.sh\n@@ -733,4 +733,33 @@ test_expect_success 'for-each-ref is-base:multiple' '\n \t\t--format=\"%(refname)[%(is-base:commit-2-3)-%(is-base:commit-6-5)]\" --stdin\n '\n\n+test_expect_success 'for-each-ref is-base: --sort' '\n+\tcat >input <<-\\EOF &&\n+\trefs/heads/commit-1-1\n+\trefs/heads/commit-4-2\n+\trefs/heads/commit-4-4\n+\trefs/heads/commit-8-4\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/commit-1-1\n+\trefs/heads/commit-4-4\n+\trefs/heads/commit-8-4\n+\trefs/heads/commit-4-2\n+\tEOF\n+\trun_all_modes git for-each-ref \\\n+\t\t--format=\"%(refname)\" --stdin \\\n+\t\t--sort=refname --sort=is-base:commit-2-3 &&\n+\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/commit-4-2\n+\trefs/heads/commit-1-1\n+\trefs/heads/commit-4-4\n+\trefs/heads/commit-8-4\n+\tEOF\n+\trun_all_modes git for-each-ref \\\n+\t\t--format=\"%(refname)\" --stdin \\\n+\t\t--sort=refname --sort=-is-base:commit-2-3\n+'\n+\n test_done\n--\n2.48.1\n"},{"id":"510888","messageId":"246b8d96-19a5-4834-bd17-d0dbb79da182@web.de","threadId":"62740","inReplyTo":"2fbe47c2-22c0-49ff-a211-057bd96ddbc9@web.de","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-19T09:11:07Z","receivedAt":"2025-01-19T09:11:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.01.25 um 18:11 schrieb René Scharfe:\n> I think moving to the right place in one step requires less churn.  Let's\n> compare; v2 coming up.\n\n... and this:\n\n---\n ref-filter.c | 49 +++++++++++++++++++++++++++++--------------------\n 1 file changed, 29 insertions(+), 20 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex caf28466ab..6da8d4c03b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -236,6 +236,7 @@ static struct used_atom {\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n \t\tstruct {\n+\t\t\tchar *name;\n \t\t\tstruct commit *commit;\n \t\t} base;\n \t\tstruct strvec describe_args;\n@@ -245,9 +246,6 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n\n-/* List of bases for is-base indicators. */\n-static struct string_list is_base_tips = STRING_LIST_INIT_DUP;\n-\n /*\n  * Expand string, append it to strbuf *sb, then return error code ret.\n  * Allow to save few lines of code.\n@@ -912,17 +910,15 @@ static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n }\n\n static int is_base_atom_parser(struct ref_format *format UNUSED,\n-\t\t\t       struct used_atom *atom UNUSED,\n+\t\t\t       struct used_atom *atom,\n \t\t\t       const char *arg, struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n-\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(is-base:<committish>)\"));\n\n-\titem = string_list_append(&is_base_tips, arg);\n-\titem->util = lookup_commit_reference_by_name(arg);\n-\tif (!item->util)\n+\tatom->u.base.name = xstrdup(arg);\n+\tatom->u.base.commit = lookup_commit_reference_by_name(arg);\n+\tif (!atom->u.base.commit)\n \t\tdie(\"failed to find '%s'\", arg);\n\n \treturn 0;\n@@ -3012,6 +3008,8 @@ void ref_array_clear(struct ref_array *array)\n \t\t\tfree(atom->u.head);\n \t\telse if (atom->atom_type == ATOM_DESCRIBE)\n \t\t\tstrvec_clear(&atom->u.describe_args);\n+\t\telse if (atom->atom_type == ATOM_ISBASE)\n+\t\t\tfree(atom->u.base.name);\n \t\telse if (atom->atom_type == ATOM_TRAILERS ||\n \t\t\t (atom->atom_type == ATOM_CONTENTS &&\n \t\t\t  atom->u.contents.option == C_TRAILERS)) {\n@@ -3027,7 +3025,6 @@ void ref_array_clear(struct ref_array *array)\n \t}\n \tFREE_AND_NULL(used_atom);\n \tused_atom_cnt = 0;\n-\tstring_list_clear(&is_base_tips, 0);\n\n \tif (ref_to_worktree_map.worktrees) {\n \t\thashmap_clear_and_free(&(ref_to_worktree_map.map),\n@@ -3140,10 +3137,17 @@ void filter_is_base(struct repository *r,\n \t\t    struct ref_array *array)\n {\n \tstruct commit **bases;\n-\tsize_t bases_nr = 0;\n+\tsize_t bases_nr = 0, is_base_nr;\n \tstruct ref_array_item **back_index;\n\n-\tif (!is_base_tips.nr || !array->nr)\n+\tif (!array->nr)\n+\t\treturn;\n+\n+\tfor (size_t i = is_base_nr = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_ISBASE)\n+\t\t\tis_base_nr++;\n+\t}\n+\tif (!is_base_nr)\n \t\treturn;\n\n \tCALLOC_ARRAY(back_index, array->nr);\n@@ -3153,7 +3157,7 @@ void filter_is_base(struct repository *r,\n \t\tconst char *name = array->items[i]->refname;\n \t\tstruct commit *c = lookup_commit_reference_by_name_gently(name, 1);\n\n-\t\tCALLOC_ARRAY(array->items[i]->is_base, is_base_tips.nr);\n+\t\tCALLOC_ARRAY(array->items[i]->is_base, is_base_nr);\n\n \t\tif (!c)\n \t\t\tcontinue;\n@@ -3163,15 +3167,20 @@ void filter_is_base(struct repository *r,\n \t\tbases_nr++;\n \t}\n\n-\tfor (size_t i = 0; i < is_base_tips.nr; i++) {\n-\t\tstruct commit *tip = is_base_tips.items[i].util;\n-\t\tint base_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n+\tfor (size_t i = 0, j = 0; i < used_atom_cnt; i++) {\n+\t\tstruct commit *tip;\n+\t\tint base_index;\n\n+\t\tif (used_atom[i].atom_type != ATOM_ISBASE)\n+\t\t\tcontinue;\n+\n+\t\ttip = used_atom[i].u.base.commit;\n+\t\tbase_index = get_branch_base_for_tip(r, tip, bases, bases_nr);\n \t\tif (base_index < 0)\n \t\t\tcontinue;\n\n \t\t/* Store the string for use in output later. */\n-\t\tback_index[base_index]->is_base[i] = xstrdup(is_base_tips.items[i].string);\n+\t\tback_index[base_index]->is_base[j++] = xstrdup(used_atom[i].u.base.name);\n \t}\n\n \tfree(back_index);\n@@ -3290,10 +3299,10 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \tfor (size_t i = 0; i < used_atom_cnt; i++) {\n \t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n \t\t\treturn 0;\n+\t\tif (used_atom[i].atom_type == ATOM_ISBASE)\n+\t\t\treturn 0;\n \t}\n-\treturn !(filter->reachable_from ||\n-\t\t filter->unreachable_from ||\n-\t\t is_base_tips.nr);\n+\treturn !(filter->reachable_from || filter->unreachable_from);\n }\n\n void filter_and_format_refs(struct ref_filter *filter, unsigned int type,\n--\n2.48.1\n\n"},{"id":"510889","messageId":"d5029c1b-4a21-442c-bb72-00b4e855c3c2@web.de","threadId":"62740","inReplyTo":"2fbe47c2-22c0-49ff-a211-057bd96ddbc9@web.de","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-19T09:11:04Z","receivedAt":"2025-01-19T09:16:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.01.25 um 18:11 schrieb René Scharfe:\n> I think moving to the right place in one step requires less churn.  Let's\n> compare; v2 coming up.\n\nThe version building on top of v1 would look like this:\n\n---\n ref-filter.c | 48 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 28 insertions(+), 20 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 89f44936f9..caf28466ab 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -235,6 +235,9 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n+\t\tstruct {\n+\t\t\tstruct commit *commit;\n+\t\t} base;\n \t\tstruct strvec describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n@@ -242,9 +245,6 @@ static struct used_atom {\n } *used_atom;\n static int used_atom_cnt, need_tagged, need_symref;\n\n-/* List of bases for ahead-behind counts. */\n-static struct string_list bases = STRING_LIST_INIT_DUP;\n-\n /* List of bases for is-base indicators. */\n static struct string_list is_base_tips = STRING_LIST_INIT_DUP;\n\n@@ -898,17 +898,14 @@ static int rest_atom_parser(struct ref_format *format UNUSED,\n }\n\n static int ahead_behind_atom_parser(struct ref_format *format UNUSED,\n-\t\t\t\t    struct used_atom *atom UNUSED,\n+\t\t\t\t    struct used_atom *atom,\n \t\t\t\t    const char *arg, struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n-\n \tif (!arg)\n \t\treturn strbuf_addf_ret(err, -1, _(\"expected format: %%(ahead-behind:<committish>)\"));\n\n-\titem = string_list_append(&bases, arg);\n-\titem->util = lookup_commit_reference_by_name(arg);\n-\tif (!item->util)\n+\tatom->u.base.commit = lookup_commit_reference_by_name(arg);\n+\tif (!atom->u.base.commit)\n \t\tdie(\"failed to find '%s'\", arg);\n\n \treturn 0;\n@@ -3030,7 +3027,6 @@ void ref_array_clear(struct ref_array *array)\n \t}\n \tFREE_AND_NULL(used_atom);\n \tused_atom_cnt = 0;\n-\tstring_list_clear(&bases, 0);\n \tstring_list_clear(&is_base_tips, 0);\n\n \tif (ref_to_worktree_map.worktrees) {\n@@ -3095,18 +3091,27 @@ void filter_ahead_behind(struct repository *r,\n \t\t\t struct ref_array *array)\n {\n \tstruct commit **commits;\n-\tsize_t commits_nr = bases.nr + array->nr;\n+\tsize_t bases_nr, commits_nr;\n+\n+\tif (!array->nr)\n+\t\treturn;\n\n-\tif (!bases.nr || !array->nr)\n+\tfor (size_t i = bases_nr = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\tbases_nr++;\n+\t}\n+\tif (!bases_nr)\n \t\treturn;\n\n-\tALLOC_ARRAY(commits, commits_nr);\n-\tfor (size_t i = 0; i < bases.nr; i++)\n-\t\tcommits[i] = bases.items[i].util;\n+\tALLOC_ARRAY(commits, st_add(bases_nr, array->nr));\n+\tfor (size_t i = 0, j = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\tcommits[j++] = used_atom[i].u.base.commit;\n+\t}\n\n-\tALLOC_ARRAY(array->counts, st_mult(bases.nr, array->nr));\n+\tALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr));\n\n-\tcommits_nr = bases.nr;\n+\tcommits_nr = bases_nr;\n \tarray->counts_nr = 0;\n \tfor (size_t i = 0; i < array->nr; i++) {\n \t\tconst char *name = array->items[i]->refname;\n@@ -3115,8 +3120,8 @@ void filter_ahead_behind(struct repository *r,\n \t\tif (!commits[commits_nr])\n \t\t\tcontinue;\n\n-\t\tCALLOC_ARRAY(array->items[i]->counts, bases.nr);\n-\t\tfor (size_t j = 0; j < bases.nr; j++) {\n+\t\tCALLOC_ARRAY(array->items[i]->counts, bases_nr);\n+\t\tfor (size_t j = 0; j < bases_nr; j++) {\n \t\t\tstruct ahead_behind_count *count;\n \t\t\tcount = &array->counts[array->counts_nr++];\n \t\t\tcount->tip_index = commits_nr;\n@@ -3282,9 +3287,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter,\n \t * - filtering on reachability\n \t * - including ahead-behind information in the formatted output\n \t */\n+\tfor (size_t i = 0; i < used_atom_cnt; i++) {\n+\t\tif (used_atom[i].atom_type == ATOM_AHEADBEHIND)\n+\t\t\treturn 0;\n+\t}\n \treturn !(filter->reachable_from ||\n \t\t filter->unreachable_from ||\n-\t\t bases.nr ||\n \t\t is_base_tips.nr);\n }\n\n--\n2.48.1\n\n"},{"id":"510890","messageId":"cb3d6e8c-6c39-423c-935c-db0e8619c504@web.de","threadId":"62740","inReplyTo":"2fbe47c2-22c0-49ff-a211-057bd96ddbc9@web.de","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-01-19T09:26:26Z","receivedAt":"2025-01-19T09:31:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.01.25 um 18:11 schrieb René Scharfe:\n> I think moving to the right place in one step requires less churn.\n\nOn the other hand attribution would be more correct if we stacked your\npatches on my v1.  Just don't forget to free u.base.name. ;)\n\nRené\n\n"},{"id":"510901","messageId":"20250119124010.GA1538605@coredump.intra.peff.net","threadId":"62740","inReplyTo":"cb3d6e8c-6c39-423c-935c-db0e8619c504@web.de","subject":"Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-19T12:40:10Z","receivedAt":"2025-01-19T12:46:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 19, 2025 at 10:26:26AM +0100, René Scharfe wrote:\n\n> Am 18.01.25 um 18:11 schrieb René Scharfe:\n> > I think moving to the right place in one step requires less churn.\n> \n> On the other hand attribution would be more correct if we stacked your\n> patches on my v1.  Just don't forget to free u.base.name. ;)\n\n:) Yes, I actually had prepared them on top of yours yesterday before I\nsaw your email. And I also double checked that you found the leak in my\noriginal. (I don't usually leak check explicitly, but wait for CI to do\nit, and I only run that when I do my daily integration).\n\nI hadn't gotten around to writing commit messages, though. So I think we\nshould go with what the v2 you posted, as it is already done. For\nattribution, I am happy enough with a Helped-by trailer. :)\n\nI also like how you pulled the final cleanup into patch 3. It's probably\nreasonable to do, as the code is not doing anything useful. The only\nthing we lose is that the original had found all of the spots that\nneeded to call _clear(), so if we later need it again we'd have to\ndo so again. But maybe that hypothetical author would find the commit in\nthe history.\n\n  Actually, ls-remote does not appear to have called it. I didn't check\n  but presumably \"ls-remote --format=%(is-base:foo)\" leaked.\n\n-Peff\n"}]}