* Bug sorting git branch output by ahead-behind:HEAD @ 2025-01-05 9:48 Ross Goldberg 2025-01-12 10:01 ` [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting René Scharfe 0 siblings, 1 reply; 19+ messages in thread From: Ross Goldberg @ 2025-01-05 9:48 UTC (permalink / raw) To: git Bug sorting git branch output by ahead-behind:HEAD What did you do before the bug happened? (Steps to reproduce your issue) $ git branch --format '%(refname:short) %(ahead-behind:HEAD)' (HEAD detached at 6aab6bfe) 0 0 190-json 1 6 560-swupd 3 6 667-reset-no-process 2 6 678-completions 2 0 693-readme-accordions 3 6 705-branch-in-version 2 6 main 0 6 $ git branch --format '%(refname:short) %(ahead-behind:HEAD)' --sort 'ahead-behind:HEAD' (HEAD detached at 6aab6bfe) 190-json 560-swupd 667-reset-no-process 678-completions 693-readme-accordions 705-branch-in-version main What did you expect to happen? (Expected behavior) I expected the second command's output to: - include the value of `ahead-behind:HEAD` on each line - have been sorted by each line's value of `ahead-behind:HEAD` What happened instead? (Actual behavior) Second command's output missing `ahead-behind:HEAD` & sorted by `refname` What's different between what you expected and what actually happened? Fields missing from second command's output Incorrect sorting of second command's output lines Anything else you want to add: If I sort by the `objectsize` field: - all requested fields are output - sorting works (except `(HEAD detached at 6aab6bfe)` is always listed first) If I do not include `%(ahead-behind:HEAD)` in the format string, sorting by `ahead-behind:HEAD` still doesn't work. Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.47.1 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon libcurl: 7.79.1 zlib: 1.2.11 uname: Darwin 21.6.0 Darwin Kernel Version 21.6.0: Mon Jun 24 00:56:10 PDT 2024; root:xnu-8020.240.18.709.2~1/RELEASE_X86_64 x86_64 compiler info: clang: 14.0.0 (clang-1400.0.29.202) libc info: no libc information available $SHELL (typically, interactive shell): /usr/local/bin/zsh [Enabled Hooks] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-05 9:48 Bug sorting git branch output by ahead-behind:HEAD Ross Goldberg @ 2025-01-12 10:01 ` René Scharfe 2025-01-13 5:17 ` Jeff King ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: René Scharfe @ 2025-01-12 10:01 UTC (permalink / raw) To: Ross Goldberg, git; +Cc: Jeff King, Derrick Stolee, Junio C Hamano verify_ref_format() parses a ref-filter format string and stores recognized items in the static array "used_atom". For "ahead-behind:<committish>" and "is-base:<committish>" it stores the committish part in string_lists that are part of struct ref_format. ref_sorting_options() also parses bare ref-filter format items and also stores recognized ones in "used_atom". The committish parts go to a dummy struct ref_format in parse_sorting_atom(), though, and are leaked and forgotten. If verify_ref_format() is called before ref_sorting_options(), like in git for-each-ref, then all works well if the sort key is included in the format string. If it isn't then sorting cannot work as the committishes are missing. If ref_sorting_options() is called first, like in git branch, then we have the additional issue that if the sort key is included in the format string then filter_ahead_behind() and filter_is_base() can't see their committishes, will not generate any results for them and thus they will for expanded to empty strings. Fix those issues by making the string_lists static, like their sibling "used_atom". This way they can all be shared for handling both ref-filter format strings and sorting options in the same command. And since struct ref_format no longer contains any allocated members, remove the now unnecessary ref_format_init() and ref_format_clear(). Reported-by: Ross Goldberg <ross.goldberg@gmail.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/branch.c | 3 +- builtin/for-each-ref.c | 1 - builtin/tag.c | 1 - builtin/verify-tag.c | 1 - ref-filter.c | 70 ++++++++++++++++++---------------------- ref-filter.h | 13 -------- t/t3203-branch-output.sh | 28 ++++++++++++++++ t/t6600-test-reach.sh | 29 +++++++++++++++++ 8 files changed, 89 insertions(+), 57 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 6e7b0cfddb..9a29de5bf1 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -473,7 +473,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin if (verify_ref_format(format)) die(_("unable to parse format string")); - filter_ahead_behind(the_repository, format, &array); + filter_ahead_behind(the_repository, &array); ref_array_sort(sorting, &array); if (column_active(colopts)) { @@ -884,7 +884,6 @@ int cmd_branch(int argc, string_list_clear(&output, 0); ref_sorting_release(sorting); ref_filter_clear(&filter); - ref_format_clear(&format); ret = 0; goto out; diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 715745a262..8085ebd8fe 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -108,7 +108,6 @@ int cmd_for_each_ref(int argc, filter_and_format_refs(&filter, flags, sorting, &format); ref_filter_clear(&filter); - ref_format_clear(&format); ref_sorting_release(sorting); strvec_clear(&vec); return 0; diff --git a/builtin/tag.c b/builtin/tag.c index c4bd145831..e8a344b926 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -698,7 +698,6 @@ int cmd_tag(int argc, cleanup: ref_sorting_release(sorting); ref_filter_clear(&filter); - ref_format_clear(&format); strbuf_release(&buf); strbuf_release(&ref); strbuf_release(&reflog_msg); diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index a7f20618ff..f6b97048a5 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -69,6 +69,5 @@ int cmd_verify_tag(int argc, if (format.format) pretty_print_ref(name, &oid, &format); } - ref_format_clear(&format); return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 23054694c2..aef142e105 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -242,6 +242,12 @@ static struct used_atom { } *used_atom; static int used_atom_cnt, need_tagged, need_symref; +/* List of bases for ahead-behind counts. */ +static struct string_list bases = STRING_LIST_INIT_DUP; + +/* List of bases for is-base indicators. */ +static struct string_list is_base_tips = STRING_LIST_INIT_DUP; + /* * Expand string, append it to strbuf *sb, then return error code ret. * Allow to save few lines of code. @@ -891,7 +897,7 @@ static int rest_atom_parser(struct ref_format *format UNUSED, return 0; } -static int ahead_behind_atom_parser(struct ref_format *format, +static int ahead_behind_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom UNUSED, const char *arg, struct strbuf *err) { @@ -900,7 +906,7 @@ static int ahead_behind_atom_parser(struct ref_format *format, if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(ahead-behind:<committish>)")); - item = string_list_append(&format->bases, arg); + item = string_list_append(&bases, arg); item->util = lookup_commit_reference_by_name(arg); if (!item->util) die("failed to find '%s'", arg); @@ -908,7 +914,7 @@ static int ahead_behind_atom_parser(struct ref_format *format, return 0; } -static int is_base_atom_parser(struct ref_format *format, +static int is_base_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom UNUSED, const char *arg, struct strbuf *err) { @@ -917,7 +923,7 @@ static int is_base_atom_parser(struct ref_format *format, if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(is-base:<committish>)")); - item = string_list_append(&format->is_base_tips, arg); + item = string_list_append(&is_base_tips, arg); item->util = lookup_commit_reference_by_name(arg); if (!item->util) die("failed to find '%s'", arg); @@ -3024,6 +3030,8 @@ void ref_array_clear(struct ref_array *array) } FREE_AND_NULL(used_atom); used_atom_cnt = 0; + string_list_clear(&bases, 0); + string_list_clear(&is_base_tips, 0); if (ref_to_worktree_map.worktrees) { hashmap_clear_and_free(&(ref_to_worktree_map.map), @@ -3084,22 +3092,21 @@ static void reach_filter(struct ref_array *array, } void filter_ahead_behind(struct repository *r, - struct ref_format *format, struct ref_array *array) { struct commit **commits; - size_t commits_nr = format->bases.nr + array->nr; + size_t commits_nr = bases.nr + array->nr; - if (!format->bases.nr || !array->nr) + if (!bases.nr || !array->nr) return; ALLOC_ARRAY(commits, commits_nr); - for (size_t i = 0; i < format->bases.nr; i++) - commits[i] = format->bases.items[i].util; + for (size_t i = 0; i < bases.nr; i++) + commits[i] = bases.items[i].util; - ALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr)); + ALLOC_ARRAY(array->counts, st_mult(bases.nr, array->nr)); - commits_nr = format->bases.nr; + commits_nr = bases.nr; array->counts_nr = 0; for (size_t i = 0; i < array->nr; i++) { const char *name = array->items[i]->refname; @@ -3108,8 +3115,8 @@ void filter_ahead_behind(struct repository *r, if (!commits[commits_nr]) continue; - CALLOC_ARRAY(array->items[i]->counts, format->bases.nr); - for (size_t j = 0; j < format->bases.nr; j++) { + CALLOC_ARRAY(array->items[i]->counts, bases.nr); + for (size_t j = 0; j < bases.nr; j++) { struct ahead_behind_count *count; count = &array->counts[array->counts_nr++]; count->tip_index = commits_nr; @@ -3125,14 +3132,13 @@ void filter_ahead_behind(struct repository *r, } void filter_is_base(struct repository *r, - struct ref_format *format, struct ref_array *array) { struct commit **bases; size_t bases_nr = 0; struct ref_array_item **back_index; - if (!format->is_base_tips.nr || !array->nr) + if (!is_base_tips.nr || !array->nr) return; CALLOC_ARRAY(back_index, array->nr); @@ -3142,7 +3148,7 @@ void filter_is_base(struct repository *r, const char *name = array->items[i]->refname; struct commit *c = lookup_commit_reference_by_name_gently(name, 1); - CALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr); + CALLOC_ARRAY(array->items[i]->is_base, is_base_tips.nr); if (!c) continue; @@ -3152,15 +3158,15 @@ void filter_is_base(struct repository *r, bases_nr++; } - for (size_t i = 0; i < format->is_base_tips.nr; i++) { - struct commit *tip = format->is_base_tips.items[i].util; + for (size_t i = 0; i < is_base_tips.nr; i++) { + struct commit *tip = is_base_tips.items[i].util; int base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); if (base_index < 0) continue; /* Store the string for use in output later. */ - back_index[base_index]->is_base[i] = xstrdup(format->is_base_tips.items[i].string); + back_index[base_index]->is_base[i] = xstrdup(is_base_tips.items[i].string); } free(back_index); @@ -3252,8 +3258,7 @@ struct ref_sorting { }; static inline int can_do_iterative_format(struct ref_filter *filter, - struct ref_sorting *sorting, - struct ref_format *format) + struct ref_sorting *sorting) { /* * Reference backends sort patterns lexicographically by refname, so if @@ -3279,15 +3284,15 @@ static inline int can_do_iterative_format(struct ref_filter *filter, */ return !(filter->reachable_from || filter->unreachable_from || - format->bases.nr || - format->is_base_tips.nr); + bases.nr || + is_base_tips.nr); } void filter_and_format_refs(struct ref_filter *filter, unsigned int type, struct ref_sorting *sorting, struct ref_format *format) { - if (can_do_iterative_format(filter, sorting, format)) { + if (can_do_iterative_format(filter, sorting)) { int save_commit_buffer_orig; struct ref_filter_and_format_cbdata ref_cbdata = { .filter = filter, @@ -3303,8 +3308,8 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type, } else { struct ref_array array = { 0 }; filter_refs(&array, filter, type); - filter_ahead_behind(the_repository, format, &array); - filter_is_base(the_repository, format, &array); + filter_ahead_behind(the_repository, &array); + filter_is_base(the_repository, &array); ref_array_sort(sorting, &array); print_formatted_ref_array(&array, format); ref_array_clear(&array); @@ -3638,16 +3643,3 @@ void ref_filter_clear(struct ref_filter *filter) free_commit_list(filter->unreachable_from); ref_filter_init(filter); } - -void ref_format_init(struct ref_format *format) -{ - struct ref_format blank = REF_FORMAT_INIT; - memcpy(format, &blank, sizeof(blank)); -} - -void ref_format_clear(struct ref_format *format) -{ - string_list_clear(&format->bases, 0); - string_list_clear(&format->is_base_tips, 0); - ref_format_init(format); -} diff --git a/ref-filter.h b/ref-filter.h index 754038ab07..013d4cfa64 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -99,12 +99,6 @@ struct ref_format { /* Internal state to ref-filter */ int need_color_reset_at_eol; - /* List of bases for ahead-behind counts. */ - struct string_list bases; - - /* List of bases for is-base indicators. */ - struct string_list is_base_tips; - struct { int max_count; int omit_empty; @@ -117,8 +111,6 @@ struct ref_format { } #define REF_FORMAT_INIT { \ .use_color = -1, \ - .bases = STRING_LIST_INIT_DUP, \ - .is_base_tips = STRING_LIST_INIT_DUP, \ } /* Macros for checking --merged and --no-merged options */ @@ -205,7 +197,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array, * If this is not called, then any ahead-behind atoms will be blank. */ void filter_ahead_behind(struct repository *r, - struct ref_format *format, struct ref_array *array); /* @@ -215,13 +206,9 @@ void filter_ahead_behind(struct repository *r, * If this is not called, then any is-base atoms will be blank. */ void filter_is_base(struct repository *r, - struct ref_format *format, struct ref_array *array); void ref_filter_init(struct ref_filter *filter); void ref_filter_clear(struct ref_filter *filter); -void ref_format_init(struct ref_format *format); -void ref_format_clear(struct ref_format *format); - #endif /* REF_FILTER_H */ diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index 500c9d0e72..a6bd88a58d 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -368,6 +368,34 @@ test_expect_success 'git branch --format with ahead-behind' ' test_cmp expect actual ' +test_expect_success 'git branch `--sort=[-]ahead-behind` option' ' + cat >expect <<-\EOF && + (HEAD detached from fromtag) 0 0 + refs/heads/ambiguous 0 0 + refs/heads/branch-two 0 0 + refs/heads/branch-one 1 0 + refs/heads/main 1 0 + refs/heads/ref-to-branch 1 0 + refs/heads/ref-to-remote 1 0 + EOF + git branch --format="%(refname) %(ahead-behind:HEAD)" \ + --sort=refname --sort=ahead-behind:HEAD >actual && + test_cmp expect actual && + + cat >expect <<-\EOF && + (HEAD detached from fromtag) 0 0 + refs/heads/branch-one 1 0 + refs/heads/main 1 0 + refs/heads/ref-to-branch 1 0 + refs/heads/ref-to-remote 1 0 + refs/heads/ambiguous 0 0 + refs/heads/branch-two 0 0 + EOF + git branch --format="%(refname) %(ahead-behind:HEAD)" \ + --sort=refname --sort=-ahead-behind:HEAD >actual && + test_cmp expect actual +' + test_expect_success 'git branch with --format=%(rest) must fail' ' test_must_fail git branch --format="%(rest)" >actual ' diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index 2591f8b8b3..6638d1aa1d 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -733,4 +733,33 @@ test_expect_success 'for-each-ref is-base:multiple' ' --format="%(refname)[%(is-base:commit-2-3)-%(is-base:commit-6-5)]" --stdin ' +test_expect_success 'for-each-ref is-base: --sort' ' + cat >input <<-\EOF && + refs/heads/commit-1-1 + refs/heads/commit-4-2 + refs/heads/commit-4-4 + refs/heads/commit-8-4 + EOF + + cat >expect <<-\EOF && + refs/heads/commit-1-1 + refs/heads/commit-4-4 + refs/heads/commit-8-4 + refs/heads/commit-4-2 + EOF + run_all_modes git for-each-ref \ + --format="%(refname)" --stdin \ + --sort=refname --sort=is-base:commit-2-3 && + + cat >expect <<-\EOF && + refs/heads/commit-4-2 + refs/heads/commit-1-1 + refs/heads/commit-4-4 + refs/heads/commit-8-4 + EOF + run_all_modes git for-each-ref \ + --format="%(refname)" --stdin \ + --sort=refname --sort=-is-base:commit-2-3 +' + test_done -- 2.48.0 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-12 10:01 ` [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting René Scharfe @ 2025-01-13 5:17 ` Jeff King 2025-01-13 5:23 ` Jeff King ` (2 more replies) 2025-01-18 17:11 ` [PATCH v2 1/3] ref-filter: move ahead-behind bases into used_atom René Scharfe ` (2 subsequent siblings) 3 siblings, 3 replies; 19+ messages in thread From: Jeff King @ 2025-01-13 5:17 UTC (permalink / raw) To: René Scharfe; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano On Sun, Jan 12, 2025 at 11:01:52AM +0100, René Scharfe wrote: > verify_ref_format() parses a ref-filter format string and stores > recognized items in the static array "used_atom". For > "ahead-behind:<committish>" and "is-base:<committish>" it stores the > committish part in string_lists that are part of struct ref_format. > > ref_sorting_options() also parses bare ref-filter format items and also > stores recognized ones in "used_atom". The committish parts go to a > dummy struct ref_format in parse_sorting_atom(), though, and are leaked > and forgotten. > > If verify_ref_format() is called before ref_sorting_options(), like in > git for-each-ref, then all works well if the sort key is included in the > format string. If it isn't then sorting cannot work as the committishes > are missing. > > If ref_sorting_options() is called first, like in git branch, then we > have the additional issue that if the sort key is included in the format > string then filter_ahead_behind() and filter_is_base() can't see their > committishes, will not generate any results for them and thus they will > for expanded to empty strings. Good analysis. The sorting and formatting are definitely tied in subtle ways, and not all code takes that into account. The dummy ref_format here is one such problem. Another is that we don't do the equivalent of verify_ref_format() on the sorting fields. Most of what it does is probably superfluous, but for example it's supposed to reject some atoms that have parsers. So: $ git for-each-ref --format='%(rest)' fatal: this command reject atom %(rest) but: $ git for-each-ref --sort=rest [...no error...] That's somewhat orthogonal, but it may influence the direction of our solution. > Fix those issues by making the string_lists static, like their sibling > "used_atom". This way they can all be shared for handling both > ref-filter format strings and sorting options in the same command. > And since struct ref_format no longer contains any allocated members, > remove the now unnecessary ref_format_init() and ref_format_clear(). Hmm. So this certainly fixes the problem. But is it where we want to go in the long run? For now there is no program that uses more than one ref-filter format. But it seems like an obvious interface that would want to be lib-ified eventually. We are not there yet because of the static global used_atoms array. But the obvious path forward is to have a context struct representing one ref-filter iteration. I think the intent was that ref_format would be that context struct, though arguably it is a little funny since it forces the sorting and formatting to be joined (OTOH, that is very much how the code works, since it wants to share results between the two for efficiency). So one solution would be to make the use of that context struct more explicit, and require ref_sorting callers to provide a format struct. Like the patch below, which also passes your tests. I dunno. Your patch is deleting more code, which is nice. But I think in the long run we'd end up replacing it. But maybe making a clean slate now would make that easier? I could go either way. --- builtin/branch.c | 2 +- builtin/for-each-ref.c | 2 +- builtin/ls-remote.c | 4 +++- builtin/tag.c | 2 +- ref-filter.c | 19 ++++++++----------- ref-filter.h | 2 +- 6 files changed, 15 insertions(+), 16 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 6e7b0cfddb..0c3f35cd0a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -875,7 +875,7 @@ int cmd_branch(int argc, * local branches 'refs/heads/...' and finally remote-tracking * branches 'refs/remotes/...'. */ - sorting = ref_sorting_options(&sorting_options); + sorting = ref_sorting_options(&sorting_options, &format); ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); ref_sorting_set_sort_flags_all( sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1); diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 715745a262..4f247efe57 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -80,7 +80,7 @@ int cmd_for_each_ref(int argc, if (verify_ref_format(&format)) usage_with_options(for_each_ref_usage, opts); - sorting = ref_sorting_options(&sorting_options); + sorting = ref_sorting_options(&sorting_options, &format); ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); filter.ignore_case = icase; diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 42f34e1236..ed38b82346 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc, const struct ref *ref; struct ref_array ref_array; struct ref_sorting *sorting; + struct ref_format format = REF_FORMAT_INIT; struct string_list sorting_options = STRING_LIST_INIT_DUP; struct option options[] = { @@ -155,7 +156,7 @@ int cmd_ls_remote(int argc, item->symref = xstrdup_or_null(ref->symref); } - sorting = ref_sorting_options(&sorting_options); + sorting = ref_sorting_options(&sorting_options, &format); ref_array_sort(sorting, &ref_array); for (i = 0; i < ref_array.nr; i++) { @@ -173,6 +174,7 @@ int cmd_ls_remote(int argc, status = 1; transport_ls_refs_options_release(&transport_options); + ref_format_clear(&format); strvec_clear(&pattern); string_list_clear(&server_options, 0); return status; diff --git a/builtin/tag.c b/builtin/tag.c index c4bd145831..a5240f66e2 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -574,7 +574,7 @@ int cmd_tag(int argc, die(_("options '%s' and '%s' cannot be used together"), "--column", "-n"); colopts = 0; } - sorting = ref_sorting_options(&sorting_options); + sorting = ref_sorting_options(&sorting_options, &format); ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); filter.ignore_case = icase; if (cmdmode == 'l') { diff --git a/ref-filter.c b/ref-filter.c index 23054694c2..f5d0c448ed 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3536,23 +3536,19 @@ void pretty_print_ref(const char *name, const struct object_id *oid, free_array_item(ref_item); } -static int parse_sorting_atom(const char *atom) +static int parse_sorting_atom(struct ref_format *format, const char *atom) { - /* - * This parses an atom using a dummy ref_format, since we don't - * actually care about the formatting details. - */ - struct ref_format dummy = REF_FORMAT_INIT; const char *end = atom + strlen(atom); struct strbuf err = STRBUF_INIT; - int res = parse_ref_filter_atom(&dummy, atom, end, &err); + int res = parse_ref_filter_atom(format, atom, end, &err); if (res < 0) die("%s", err.buf); strbuf_release(&err); return res; } -static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) +static void parse_ref_sorting(struct ref_format *format, + struct ref_sorting **sorting_tail, const char *arg) { struct ref_sorting *s; @@ -3567,17 +3563,18 @@ static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg)) s->sort_flags |= REF_SORTING_VERSION; - s->atom = parse_sorting_atom(arg); + s->atom = parse_sorting_atom(format, arg); } -struct ref_sorting *ref_sorting_options(struct string_list *options) +struct ref_sorting *ref_sorting_options(struct string_list *options, + struct ref_format *format) { struct string_list_item *item; struct ref_sorting *sorting = NULL, **tail = &sorting; if (options->nr) { for_each_string_list_item(item, options) - parse_ref_sorting(tail, item->string); + parse_ref_sorting(format, tail, item->string); } /* diff --git a/ref-filter.h b/ref-filter.h index 754038ab07..1531bf1762 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -168,7 +168,7 @@ int format_ref_array_item(struct ref_array_item *info, /* Release a "struct ref_sorting" */ void ref_sorting_release(struct ref_sorting *); /* Convert list of sort options into ref_sorting */ -struct ref_sorting *ref_sorting_options(struct string_list *); +struct ref_sorting *ref_sorting_options(struct string_list *, struct ref_format *); /* Function to parse --merged and --no-merged options */ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); /* Get the current HEAD's description */ ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-13 5:17 ` Jeff King @ 2025-01-13 5:23 ` Jeff King 2025-01-13 17:25 ` Junio C Hamano 2025-01-14 18:55 ` René Scharfe 2 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2025-01-13 5:23 UTC (permalink / raw) To: René Scharfe; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano On Mon, Jan 13, 2025 at 12:17:01AM -0500, Jeff King wrote: > diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c > index 42f34e1236..ed38b82346 100644 > --- a/builtin/ls-remote.c > +++ b/builtin/ls-remote.c > @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc, > const struct ref *ref; > struct ref_array ref_array; > struct ref_sorting *sorting; > + struct ref_format format = REF_FORMAT_INIT; > struct string_list sorting_options = STRING_LIST_INIT_DUP; This caller in ls-remote is the only one that doesn't otherwise have a ref_format struct. I don't think it's a big deal to add one like this, and it might even be nice to support --format. But I suspect there are other weird errors lurking (however we structure this code) because we may not actually have access to the objects! The ahead-behind atom seems to gloss over that, but others will barf. Interestingly we detect the out-of-repo case: $ git ls-remote --sort=authordate git.git fatal: not a git repository, but the field 'authordate' requires access to object data but not if we have a repo: $ git init $ git ls-remote --sort=authordate git.git fatal: missing object 978601ccf7b27399aa349c535b29965e664046c4 for refs/heads/ci-config I guess it would work _sometimes_ if you've fetched recently from the remote. So maybe it does not make sense to outlaw all object-inspecting atoms for ls-remote, and just say that the error above is the best we can do? It does get weird with ahead-behind, though, as you'll get a different sort order depending on whether you've actually fetched the object. Anyway, all orthogonal to what you're fixing, but kind of gross none the less. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-13 5:17 ` Jeff King 2025-01-13 5:23 ` Jeff King @ 2025-01-13 17:25 ` Junio C Hamano 2025-01-14 18:55 ` René Scharfe 2 siblings, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2025-01-13 17:25 UTC (permalink / raw) To: Jeff King; +Cc: René Scharfe, Ross Goldberg, git, Derrick Stolee Jeff King <peff@peff.net> writes: > For now there is no program that uses more than one ref-filter format. > But it seems like an obvious interface that would want to be lib-ified > eventually. We are not there yet because of the static global used_atoms > array. But the obvious path forward is to have a context struct > representing one ref-filter iteration. > > I think the intent was that ref_format would be that context struct, > though arguably it is a little funny since it forces the sorting and > formatting to be joined (OTOH, that is very much how the code works, > since it wants to share results between the two for efficiency). > > So one solution would be to make the use of that context struct more > explicit, and require ref_sorting callers to provide a format struct. > Like the patch below, which also passes your tests. > > I dunno. Your patch is deleting more code, which is nice. But I think in > the long run we'd end up replacing it. But maybe making a clean slate > now would make that easier? I could go either way. I agree with you that libification effort would want to move more static variables to members in a context structure. I initially suspected that would be more or less orthogonal, because it is not like we are adding a static or two to a code path that already holds everything else in a context structure, and would be a lot more work, but now you have a patch that makes the first step in that direction and it does not look too bad at all, so ... Thanks. > --- > builtin/branch.c | 2 +- > builtin/for-each-ref.c | 2 +- > builtin/ls-remote.c | 4 +++- > builtin/tag.c | 2 +- > ref-filter.c | 19 ++++++++----------- > ref-filter.h | 2 +- > 6 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 6e7b0cfddb..0c3f35cd0a 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -875,7 +875,7 @@ int cmd_branch(int argc, > * local branches 'refs/heads/...' and finally remote-tracking > * branches 'refs/remotes/...'. > */ > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); > ref_sorting_set_sort_flags_all( > sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1); > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c > index 715745a262..4f247efe57 100644 > --- a/builtin/for-each-ref.c > +++ b/builtin/for-each-ref.c > @@ -80,7 +80,7 @@ int cmd_for_each_ref(int argc, > if (verify_ref_format(&format)) > usage_with_options(for_each_ref_usage, opts); > > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); > filter.ignore_case = icase; > > diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c > index 42f34e1236..ed38b82346 100644 > --- a/builtin/ls-remote.c > +++ b/builtin/ls-remote.c > @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc, > const struct ref *ref; > struct ref_array ref_array; > struct ref_sorting *sorting; > + struct ref_format format = REF_FORMAT_INIT; > struct string_list sorting_options = STRING_LIST_INIT_DUP; > > struct option options[] = { > @@ -155,7 +156,7 @@ int cmd_ls_remote(int argc, > item->symref = xstrdup_or_null(ref->symref); > } > > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_array_sort(sorting, &ref_array); > > for (i = 0; i < ref_array.nr; i++) { > @@ -173,6 +174,7 @@ int cmd_ls_remote(int argc, > status = 1; > transport_ls_refs_options_release(&transport_options); > > + ref_format_clear(&format); > strvec_clear(&pattern); > string_list_clear(&server_options, 0); > return status; > diff --git a/builtin/tag.c b/builtin/tag.c > index c4bd145831..a5240f66e2 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -574,7 +574,7 @@ int cmd_tag(int argc, > die(_("options '%s' and '%s' cannot be used together"), "--column", "-n"); > colopts = 0; > } > - sorting = ref_sorting_options(&sorting_options); > + sorting = ref_sorting_options(&sorting_options, &format); > ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase); > filter.ignore_case = icase; > if (cmdmode == 'l') { > diff --git a/ref-filter.c b/ref-filter.c > index 23054694c2..f5d0c448ed 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -3536,23 +3536,19 @@ void pretty_print_ref(const char *name, const struct object_id *oid, > free_array_item(ref_item); > } > > -static int parse_sorting_atom(const char *atom) > +static int parse_sorting_atom(struct ref_format *format, const char *atom) > { > - /* > - * This parses an atom using a dummy ref_format, since we don't > - * actually care about the formatting details. > - */ > - struct ref_format dummy = REF_FORMAT_INIT; > const char *end = atom + strlen(atom); > struct strbuf err = STRBUF_INIT; > - int res = parse_ref_filter_atom(&dummy, atom, end, &err); > + int res = parse_ref_filter_atom(format, atom, end, &err); > if (res < 0) > die("%s", err.buf); > strbuf_release(&err); > return res; > } > > -static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) > +static void parse_ref_sorting(struct ref_format *format, > + struct ref_sorting **sorting_tail, const char *arg) > { > struct ref_sorting *s; > > @@ -3567,17 +3563,18 @@ static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg > if (skip_prefix(arg, "version:", &arg) || > skip_prefix(arg, "v:", &arg)) > s->sort_flags |= REF_SORTING_VERSION; > - s->atom = parse_sorting_atom(arg); > + s->atom = parse_sorting_atom(format, arg); > } > > -struct ref_sorting *ref_sorting_options(struct string_list *options) > +struct ref_sorting *ref_sorting_options(struct string_list *options, > + struct ref_format *format) > { > struct string_list_item *item; > struct ref_sorting *sorting = NULL, **tail = &sorting; > > if (options->nr) { > for_each_string_list_item(item, options) > - parse_ref_sorting(tail, item->string); > + parse_ref_sorting(format, tail, item->string); > } > > /* > diff --git a/ref-filter.h b/ref-filter.h > index 754038ab07..1531bf1762 100644 > --- a/ref-filter.h > +++ b/ref-filter.h > @@ -168,7 +168,7 @@ int format_ref_array_item(struct ref_array_item *info, > /* Release a "struct ref_sorting" */ > void ref_sorting_release(struct ref_sorting *); > /* Convert list of sort options into ref_sorting */ > -struct ref_sorting *ref_sorting_options(struct string_list *); > +struct ref_sorting *ref_sorting_options(struct string_list *, struct ref_format *); > /* Function to parse --merged and --no-merged options */ > int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset); > /* Get the current HEAD's description */ ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-13 5:17 ` Jeff King 2025-01-13 5:23 ` Jeff King 2025-01-13 17:25 ` Junio C Hamano @ 2025-01-14 18:55 ` René Scharfe 2025-01-16 9:51 ` Jeff King 2 siblings, 1 reply; 19+ messages in thread From: René Scharfe @ 2025-01-14 18:55 UTC (permalink / raw) To: Jeff King; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano Am 13.01.25 um 06:17 schrieb Jeff King: > On Sun, Jan 12, 2025 at 11:01:52AM +0100, René Scharfe wrote: > >> verify_ref_format() parses a ref-filter format string and stores >> recognized items in the static array "used_atom". For >> "ahead-behind:<committish>" and "is-base:<committish>" it stores the >> committish part in string_lists that are part of struct ref_format. >> >> ref_sorting_options() also parses bare ref-filter format items and also >> stores recognized ones in "used_atom". The committish parts go to a >> dummy struct ref_format in parse_sorting_atom(), though, and are leaked >> and forgotten. >> >> If verify_ref_format() is called before ref_sorting_options(), like in >> git for-each-ref, then all works well if the sort key is included in the >> format string. If it isn't then sorting cannot work as the committishes >> are missing. >> >> If ref_sorting_options() is called first, like in git branch, then we >> have the additional issue that if the sort key is included in the format >> string then filter_ahead_behind() and filter_is_base() can't see their >> committishes, will not generate any results for them and thus they will >> for expanded to empty strings. > > Good analysis. The sorting and formatting are definitely tied in subtle > ways, and not all code takes that into account. > > The dummy ref_format here is one such problem. Another is that we don't > do the equivalent of verify_ref_format() on the sorting fields. Most of > what it does is probably superfluous, but for example it's supposed to > reject some atoms that have parsers. So: > > $ git for-each-ref --format='%(rest)' > fatal: this command reject atom %(rest) > > but: > > $ git for-each-ref --sort=rest > [...no error...] > > That's somewhat orthogonal, but it may influence the direction of our > solution. Not nice. Erroring out would be better than leaving users wondering why that sort arg does nothing. A totally different thing that bugs me: Calling ahead-behind an atom is weird; it's more of a molecule. It should be possible to add separate ahead and behind atoms, with scalar values, that we then could sort separately, preferably numerically instead of lexically. >> Fix those issues by making the string_lists static, like their sibling >> "used_atom". This way they can all be shared for handling both >> ref-filter format strings and sorting options in the same command. >> And since struct ref_format no longer contains any allocated members, >> remove the now unnecessary ref_format_init() and ref_format_clear(). > > Hmm. So this certainly fixes the problem. But is it where we want to go > in the long run? > > For now there is no program that uses more than one ref-filter format. > But it seems like an obvious interface that would want to be lib-ified > eventually. We are not there yet because of the static global used_atoms > array. But the obvious path forward is to have a context struct > representing one ref-filter iteration. > > I think the intent was that ref_format would be that context struct, > though arguably it is a little funny since it forces the sorting and > formatting to be joined (OTOH, that is very much how the code works, > since it wants to share results between the two for efficiency). > > So one solution would be to make the use of that context struct more > explicit, and require ref_sorting callers to provide a format struct. > Like the patch below, which also passes your tests. Did that in the first version of the patch. It works, but keeps the cause of the issue unaddressed: The separation of used_atom and the string_lists, which together represent the parsed items. I'm not convinced that ref_format is the right place for them, but haven't thought this through, admittedly. struct ref_filter and a new dedicated struct would be alternatives. Moving used_atom will be painful in any case. > I dunno. Your patch is deleting more code, which is nice. But I think in > the long run we'd end up replacing it. But maybe making a clean slate > now would make that easier? I could go either way. I think joining the three structs is better than leaving them apart, but if someone is moving them to their right place soon anyway, be it ref_format or somewhere else, then I could send the the first version of the patch (basically what you sent). René ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-14 18:55 ` René Scharfe @ 2025-01-16 9:51 ` Jeff King 2025-01-16 10:06 ` Jeff King 2025-01-18 17:11 ` René Scharfe 0 siblings, 2 replies; 19+ messages in thread From: Jeff King @ 2025-01-16 9:51 UTC (permalink / raw) To: René Scharfe; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano On Tue, Jan 14, 2025 at 07:55:03PM +0100, René Scharfe wrote: > A totally different thing that bugs me: Calling ahead-behind an atom is > weird; it's more of a molecule. It should be possible to add separate > ahead and behind atoms, with scalar values, that we then could sort > separately, preferably numerically instead of lexically. Yeah, I agree that would be the most flexible way to do it. I'm not sure if people would find it useful or not (I've never even used the ahead-behind atom myself). > > So one solution would be to make the use of that context struct more > > explicit, and require ref_sorting callers to provide a format struct. > > Like the patch below, which also passes your tests. > > Did that in the first version of the patch. It works, but keeps the > cause of the issue unaddressed: The separation of used_atom and the > string_lists, which together represent the parsed items. Right. I think the end goal is: all pieces of data together in a non-global struct. Anything short of that has to choose the least-bad option. ;) In a sense, having _anything_ global means that there is little point in having a struct at all (since it is still not safe to be called twice). But it feels like that's a necessary step on the way, and getting rid of the struct is a step backwards. I dunno. I still have dreams about rewriting ref-filter completely for better clarity and efficiency. > I'm not convinced that ref_format is the right place for them, but > haven't thought this through, admittedly. struct ref_filter and a > new dedicated struct would be alternatives. Moving used_atom will be > painful in any case. Yet another option in the near term might be storing these ahead-behind bits in the individual atoms. Since the point is to do a single traversal, we'd have to marshal them into a unified data structure at some point. But we already do that! In filter_ahead_behind() we convert the string list into an array (and ironically do not even look at the strings, only their "util" fields). So something like this (only lightly tested) seems to work: diff --git a/ref-filter.c b/ref-filter.c index 23054694c2..4c10b6fe75 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -235,6 +235,9 @@ static struct used_atom { enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; + struct { + struct commit *commit; + } base; struct strvec describe_args; struct refname_atom refname; char *head; @@ -892,17 +895,14 @@ static int rest_atom_parser(struct ref_format *format UNUSED, } static int ahead_behind_atom_parser(struct ref_format *format, - struct used_atom *atom UNUSED, + struct used_atom *atom, const char *arg, struct strbuf *err) { - struct string_list_item *item; - if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(ahead-behind:<committish>)")); - item = string_list_append(&format->bases, arg); - item->util = lookup_commit_reference_by_name(arg); - if (!item->util) + atom->u.base.commit = lookup_commit_reference_by_name(arg); + if (!atom->u.base.commit) die("failed to find '%s'", arg); return 0; @@ -3088,18 +3088,31 @@ void filter_ahead_behind(struct repository *r, struct ref_array *array) { struct commit **commits; - size_t commits_nr = format->bases.nr + array->nr; + size_t bases_nr, commits_nr; - if (!format->bases.nr || !array->nr) + if (!array->nr) return; + bases_nr = 0; + for (size_t i = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + bases_nr++; + } + + if (!bases_nr) + return; + + commits_nr = bases_nr + array->nr; + ALLOC_ARRAY(commits, commits_nr); - for (size_t i = 0; i < format->bases.nr; i++) - commits[i] = format->bases.items[i].util; + for (size_t i = 0, j = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + commits[j++] = used_atom[i].u.base.commit; + } - ALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr)); + ALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr)); - commits_nr = format->bases.nr; + commits_nr = bases_nr; array->counts_nr = 0; for (size_t i = 0; i < array->nr; i++) { const char *name = array->items[i]->refname; @@ -3108,8 +3121,8 @@ void filter_ahead_behind(struct repository *r, if (!commits[commits_nr]) continue; - CALLOC_ARRAY(array->items[i]->counts, format->bases.nr); - for (size_t j = 0; j < format->bases.nr; j++) { + CALLOC_ARRAY(array->items[i]->counts, bases_nr); + for (size_t j = 0; j < bases_nr; j++) { struct ahead_behind_count *count; count = &array->counts[array->counts_nr++]; count->tip_index = commits_nr; @@ -3277,9 +3290,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter, * - filtering on reachability * - including ahead-behind information in the formatted output */ + for (size_t i = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + return 0; + } return !(filter->reachable_from || filter->unreachable_from || - format->bases.nr || format->is_base_tips.nr); } @@ -3647,7 +3663,6 @@ void ref_format_init(struct ref_format *format) void ref_format_clear(struct ref_format *format) { - string_list_clear(&format->bases, 0); string_list_clear(&format->is_base_tips, 0); ref_format_init(format); } diff --git a/ref-filter.h b/ref-filter.h index 754038ab07..d048317802 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -99,9 +99,6 @@ struct ref_format { /* Internal state to ref-filter */ int need_color_reset_at_eol; - /* List of bases for ahead-behind counts. */ - struct string_list bases; - /* List of bases for is-base indicators. */ struct string_list is_base_tips; @@ -117,7 +114,6 @@ struct ref_format { } #define REF_FORMAT_INIT { \ .use_color = -1, \ - .bases = STRING_LIST_INIT_DUP, \ .is_base_tips = STRING_LIST_INIT_DUP, \ } We should be able to do the same thing with is_base_tips, but I've left it as an exercise for the reader. ;) -Peff ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-16 9:51 ` Jeff King @ 2025-01-16 10:06 ` Jeff King 2025-01-16 10:21 ` Jeff King 2025-01-18 17:11 ` René Scharfe 1 sibling, 1 reply; 19+ messages in thread From: Jeff King @ 2025-01-16 10:06 UTC (permalink / raw) To: René Scharfe; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano On Thu, Jan 16, 2025 at 04:51:28AM -0500, Jeff King wrote: > Yet another option in the near term might be storing these ahead-behind > bits in the individual atoms. Since the point is to do a single > traversal, we'd have to marshal them into a unified data structure at > some point. But we already do that! In filter_ahead_behind() we convert > the string list into an array (and ironically do not even look at the > strings, only their "util" fields). > > So something like this (only lightly tested) seems to work: I compiled it without DEVELOPER=1, so I missed a few unused parameters. We'd want this on top: diff --git a/builtin/branch.c b/builtin/branch.c index 6e7b0cfddb..fbb9536282 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -473,7 +473,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin if (verify_ref_format(format)) die(_("unable to parse format string")); - filter_ahead_behind(the_repository, format, &array); + filter_ahead_behind(the_repository, &array); ref_array_sort(sorting, &array); if (column_active(colopts)) { diff --git a/ref-filter.c b/ref-filter.c index 4c10b6fe75..0de51f13e6 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -894,7 +894,7 @@ static int rest_atom_parser(struct ref_format *format UNUSED, return 0; } -static int ahead_behind_atom_parser(struct ref_format *format, +static int ahead_behind_atom_parser(struct ref_format *format UNUSED, struct used_atom *atom, const char *arg, struct strbuf *err) { @@ -3084,7 +3084,6 @@ static void reach_filter(struct ref_array *array, } void filter_ahead_behind(struct repository *r, - struct ref_format *format, struct ref_array *array) { struct commit **commits; @@ -3319,7 +3318,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type, } else { struct ref_array array = { 0 }; filter_refs(&array, filter, type); - filter_ahead_behind(the_repository, format, &array); + filter_ahead_behind(the_repository, &array); filter_is_base(the_repository, format, &array); ref_array_sort(sorting, &array); print_formatted_ref_array(&array, format); diff --git a/ref-filter.h b/ref-filter.h index d048317802..5f3dd6c931 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -201,7 +201,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array, * If this is not called, then any ahead-behind atoms will be blank. */ void filter_ahead_behind(struct repository *r, - struct ref_format *format, struct ref_array *array); /* Getting rid of the ref_format argument to filter_ahead_behind makes it even more plain that we are operating on magic global data behind the scenes. ;) So in that sense this is all the same step back that I was complaining about from your patch; eventually we'd need to pass the used_atom array to filter_ahead_behind() via some struct. But somehow it seems less bad to me because it is all being folded into the existing global structure. -Peff ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-16 10:06 ` Jeff King @ 2025-01-16 10:21 ` Jeff King 2025-01-16 17:31 ` Junio C Hamano 2025-01-18 17:11 ` René Scharfe 0 siblings, 2 replies; 19+ messages in thread From: Jeff King @ 2025-01-16 10:21 UTC (permalink / raw) To: René Scharfe; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano On Thu, Jan 16, 2025 at 05:06:37AM -0500, Jeff King wrote: > On Thu, Jan 16, 2025 at 04:51:28AM -0500, Jeff King wrote: > > > Yet another option in the near term might be storing these ahead-behind > > bits in the individual atoms. Since the point is to do a single > > traversal, we'd have to marshal them into a unified data structure at > > some point. But we already do that! In filter_ahead_behind() we convert > > the string list into an array (and ironically do not even look at the > > strings, only their "util" fields). > > > > So something like this (only lightly tested) seems to work: > > I compiled it without DEVELOPER=1, so I missed a few unused parameters. > We'd want this on top: And one final thought on this approach: if we do want to do it, perhaps it would make sense to build on top of the patch you sent. I think yours fixes the bug in a more direct and obvious way, and then my approach would merely be internal reorganization on top. At any rate, here is the is_base conversion for posterity. diff --git a/ref-filter.c b/ref-filter.c index 0de51f13e6..28d48ec585 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -236,6 +236,7 @@ static struct used_atom { S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; struct { + char *name; struct commit *commit; } base; struct strvec describe_args; @@ -908,18 +909,16 @@ static int ahead_behind_atom_parser(struct ref_format *format UNUSED, return 0; } -static int is_base_atom_parser(struct ref_format *format, - struct used_atom *atom UNUSED, +static int is_base_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, const char *arg, struct strbuf *err) { - struct string_list_item *item; - if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(is-base:<committish>)")); - item = string_list_append(&format->is_base_tips, arg); - item->util = lookup_commit_reference_by_name(arg); - if (!item->util) + atom->u.base.name = xstrdup(arg); + atom->u.base.commit = lookup_commit_reference_by_name(arg); + if (!atom->u.base.commit) die("failed to find '%s'", arg); return 0; @@ -3137,14 +3136,20 @@ void filter_ahead_behind(struct repository *r, } void filter_is_base(struct repository *r, - struct ref_format *format, struct ref_array *array) { struct commit **bases; size_t bases_nr = 0; struct ref_array_item **back_index; + size_t atoms_nr; - if (!format->is_base_tips.nr || !array->nr) + atoms_nr = 0; + for (size_t i = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_ISBASE) + atoms_nr++; + } + + if (!atoms_nr || !array->nr) return; CALLOC_ARRAY(back_index, array->nr); @@ -3154,7 +3159,7 @@ void filter_is_base(struct repository *r, const char *name = array->items[i]->refname; struct commit *c = lookup_commit_reference_by_name_gently(name, 1); - CALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr); + CALLOC_ARRAY(array->items[i]->is_base, atoms_nr); if (!c) continue; @@ -3164,15 +3169,21 @@ void filter_is_base(struct repository *r, bases_nr++; } - for (size_t i = 0; i < format->is_base_tips.nr; i++) { - struct commit *tip = format->is_base_tips.items[i].util; - int base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); + for (size_t i = 0, j = 0; i < used_atom_cnt; i++) { + struct commit *tip; + int base_index; + + if (used_atom[i].atom_type != ATOM_ISBASE) + continue; + + tip = used_atom[i].u.base.commit; + base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); if (base_index < 0) continue; /* Store the string for use in output later. */ - back_index[base_index]->is_base[i] = xstrdup(format->is_base_tips.items[i].string); + back_index[base_index]->is_base[j++] = xstrdup(used_atom[i].u.base.name); } free(back_index); @@ -3264,8 +3275,7 @@ struct ref_sorting { }; static inline int can_do_iterative_format(struct ref_filter *filter, - struct ref_sorting *sorting, - struct ref_format *format) + struct ref_sorting *sorting) { /* * Reference backends sort patterns lexicographically by refname, so if @@ -3292,17 +3302,18 @@ static inline int can_do_iterative_format(struct ref_filter *filter, for (size_t i = 0; i < used_atom_cnt; i++) { if (used_atom[i].atom_type == ATOM_AHEADBEHIND) return 0; + if (used_atom[i].atom_type == ATOM_ISBASE) + return 0; } return !(filter->reachable_from || - filter->unreachable_from || - format->is_base_tips.nr); + filter->unreachable_from); } void filter_and_format_refs(struct ref_filter *filter, unsigned int type, struct ref_sorting *sorting, struct ref_format *format) { - if (can_do_iterative_format(filter, sorting, format)) { + if (can_do_iterative_format(filter, sorting)) { int save_commit_buffer_orig; struct ref_filter_and_format_cbdata ref_cbdata = { .filter = filter, @@ -3319,7 +3330,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type, struct ref_array array = { 0 }; filter_refs(&array, filter, type); filter_ahead_behind(the_repository, &array); - filter_is_base(the_repository, format, &array); + filter_is_base(the_repository, &array); ref_array_sort(sorting, &array); print_formatted_ref_array(&array, format); ref_array_clear(&array); @@ -3662,6 +3673,5 @@ void ref_format_init(struct ref_format *format) void ref_format_clear(struct ref_format *format) { - string_list_clear(&format->is_base_tips, 0); ref_format_init(format); } diff --git a/ref-filter.h b/ref-filter.h index 5f3dd6c931..0ba94df651 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -99,9 +99,6 @@ struct ref_format { /* Internal state to ref-filter */ int need_color_reset_at_eol; - /* List of bases for is-base indicators. */ - struct string_list is_base_tips; - struct { int max_count; int omit_empty; @@ -114,7 +111,6 @@ struct ref_format { } #define REF_FORMAT_INIT { \ .use_color = -1, \ - .is_base_tips = STRING_LIST_INIT_DUP, \ } /* Macros for checking --merged and --no-merged options */ @@ -210,7 +206,6 @@ void filter_ahead_behind(struct repository *r, * If this is not called, then any is-base atoms will be blank. */ void filter_is_base(struct repository *r, - struct ref_format *format, struct ref_array *array); void ref_filter_init(struct ref_filter *filter); ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-16 10:21 ` Jeff King @ 2025-01-16 17:31 ` Junio C Hamano 2025-01-18 17:11 ` René Scharfe 1 sibling, 0 replies; 19+ messages in thread From: Junio C Hamano @ 2025-01-16 17:31 UTC (permalink / raw) To: Jeff King; +Cc: René Scharfe, Ross Goldberg, git, Derrick Stolee Jeff King <peff@peff.net> writes: > On Thu, Jan 16, 2025 at 05:06:37AM -0500, Jeff King wrote: > >> On Thu, Jan 16, 2025 at 04:51:28AM -0500, Jeff King wrote: >> >> > Yet another option in the near term might be storing these ahead-behind >> > bits in the individual atoms. Since the point is to do a single >> > traversal, we'd have to marshal them into a unified data structure at >> > some point. But we already do that! In filter_ahead_behind() we convert >> > the string list into an array (and ironically do not even look at the >> > strings, only their "util" fields). >> > >> > So something like this (only lightly tested) seems to work: >> >> I compiled it without DEVELOPER=1, so I missed a few unused parameters. >> We'd want this on top: > > And one final thought on this approach: if we do want to do it, perhaps > it would make sense to build on top of the patch you sent. I think yours > fixes the bug in a more direct and obvious way, and then my approach > would merely be internal reorganization on top. > > At any rate, here is the is_base conversion for posterity. Looking good. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-16 10:21 ` Jeff King 2025-01-16 17:31 ` Junio C Hamano @ 2025-01-18 17:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe ` (2 more replies) 1 sibling, 3 replies; 19+ messages in thread From: René Scharfe @ 2025-01-18 17:11 UTC (permalink / raw) To: Jeff King; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano Am 16.01.25 um 11:21 schrieb Jeff King: > And one final thought on this approach: if we do want to do it, perhaps > it would make sense to build on top of the patch you sent. I think yours > fixes the bug in a more direct and obvious way, and then my approach > would merely be internal reorganization on top. I think moving to the right place in one step requires less churn. Let's compare; v2 coming up. René ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-18 17:11 ` René Scharfe @ 2025-01-19 9:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe 2025-01-19 9:26 ` René Scharfe 2 siblings, 0 replies; 19+ messages in thread From: René Scharfe @ 2025-01-19 9:11 UTC (permalink / raw) To: Jeff King; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano Am 18.01.25 um 18:11 schrieb René Scharfe: > I think moving to the right place in one step requires less churn. Let's > compare; v2 coming up. The version building on top of v1 would look like this: --- ref-filter.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 89f44936f9..caf28466ab 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -235,6 +235,9 @@ static struct used_atom { enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; + struct { + struct commit *commit; + } base; struct strvec describe_args; struct refname_atom refname; char *head; @@ -242,9 +245,6 @@ static struct used_atom { } *used_atom; static int used_atom_cnt, need_tagged, need_symref; -/* List of bases for ahead-behind counts. */ -static struct string_list bases = STRING_LIST_INIT_DUP; - /* List of bases for is-base indicators. */ static struct string_list is_base_tips = STRING_LIST_INIT_DUP; @@ -898,17 +898,14 @@ static int rest_atom_parser(struct ref_format *format UNUSED, } static int ahead_behind_atom_parser(struct ref_format *format UNUSED, - struct used_atom *atom UNUSED, + struct used_atom *atom, const char *arg, struct strbuf *err) { - struct string_list_item *item; - if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(ahead-behind:<committish>)")); - item = string_list_append(&bases, arg); - item->util = lookup_commit_reference_by_name(arg); - if (!item->util) + atom->u.base.commit = lookup_commit_reference_by_name(arg); + if (!atom->u.base.commit) die("failed to find '%s'", arg); return 0; @@ -3030,7 +3027,6 @@ void ref_array_clear(struct ref_array *array) } FREE_AND_NULL(used_atom); used_atom_cnt = 0; - string_list_clear(&bases, 0); string_list_clear(&is_base_tips, 0); if (ref_to_worktree_map.worktrees) { @@ -3095,18 +3091,27 @@ void filter_ahead_behind(struct repository *r, struct ref_array *array) { struct commit **commits; - size_t commits_nr = bases.nr + array->nr; + size_t bases_nr, commits_nr; + + if (!array->nr) + return; - if (!bases.nr || !array->nr) + for (size_t i = bases_nr = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + bases_nr++; + } + if (!bases_nr) return; - ALLOC_ARRAY(commits, commits_nr); - for (size_t i = 0; i < bases.nr; i++) - commits[i] = bases.items[i].util; + ALLOC_ARRAY(commits, st_add(bases_nr, array->nr)); + for (size_t i = 0, j = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + commits[j++] = used_atom[i].u.base.commit; + } - ALLOC_ARRAY(array->counts, st_mult(bases.nr, array->nr)); + ALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr)); - commits_nr = bases.nr; + commits_nr = bases_nr; array->counts_nr = 0; for (size_t i = 0; i < array->nr; i++) { const char *name = array->items[i]->refname; @@ -3115,8 +3120,8 @@ void filter_ahead_behind(struct repository *r, if (!commits[commits_nr]) continue; - CALLOC_ARRAY(array->items[i]->counts, bases.nr); - for (size_t j = 0; j < bases.nr; j++) { + CALLOC_ARRAY(array->items[i]->counts, bases_nr); + for (size_t j = 0; j < bases_nr; j++) { struct ahead_behind_count *count; count = &array->counts[array->counts_nr++]; count->tip_index = commits_nr; @@ -3282,9 +3287,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter, * - filtering on reachability * - including ahead-behind information in the formatted output */ + for (size_t i = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + return 0; + } return !(filter->reachable_from || filter->unreachable_from || - bases.nr || is_base_tips.nr); } -- 2.48.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-18 17:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe @ 2025-01-19 9:11 ` René Scharfe 2025-01-19 9:26 ` René Scharfe 2 siblings, 0 replies; 19+ messages in thread From: René Scharfe @ 2025-01-19 9:11 UTC (permalink / raw) To: Jeff King; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano Am 18.01.25 um 18:11 schrieb René Scharfe: > I think moving to the right place in one step requires less churn. Let's > compare; v2 coming up. ... and this: --- ref-filter.c | 49 +++++++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index caf28466ab..6da8d4c03b 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -236,6 +236,7 @@ static struct used_atom { S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; struct { + char *name; struct commit *commit; } base; struct strvec describe_args; @@ -245,9 +246,6 @@ static struct used_atom { } *used_atom; static int used_atom_cnt, need_tagged, need_symref; -/* List of bases for is-base indicators. */ -static struct string_list is_base_tips = STRING_LIST_INIT_DUP; - /* * Expand string, append it to strbuf *sb, then return error code ret. * Allow to save few lines of code. @@ -912,17 +910,15 @@ static int ahead_behind_atom_parser(struct ref_format *format UNUSED, } static int is_base_atom_parser(struct ref_format *format UNUSED, - struct used_atom *atom UNUSED, + struct used_atom *atom, const char *arg, struct strbuf *err) { - struct string_list_item *item; - if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(is-base:<committish>)")); - item = string_list_append(&is_base_tips, arg); - item->util = lookup_commit_reference_by_name(arg); - if (!item->util) + atom->u.base.name = xstrdup(arg); + atom->u.base.commit = lookup_commit_reference_by_name(arg); + if (!atom->u.base.commit) die("failed to find '%s'", arg); return 0; @@ -3012,6 +3008,8 @@ void ref_array_clear(struct ref_array *array) free(atom->u.head); else if (atom->atom_type == ATOM_DESCRIBE) strvec_clear(&atom->u.describe_args); + else if (atom->atom_type == ATOM_ISBASE) + free(atom->u.base.name); else if (atom->atom_type == ATOM_TRAILERS || (atom->atom_type == ATOM_CONTENTS && atom->u.contents.option == C_TRAILERS)) { @@ -3027,7 +3025,6 @@ void ref_array_clear(struct ref_array *array) } FREE_AND_NULL(used_atom); used_atom_cnt = 0; - string_list_clear(&is_base_tips, 0); if (ref_to_worktree_map.worktrees) { hashmap_clear_and_free(&(ref_to_worktree_map.map), @@ -3140,10 +3137,17 @@ void filter_is_base(struct repository *r, struct ref_array *array) { struct commit **bases; - size_t bases_nr = 0; + size_t bases_nr = 0, is_base_nr; struct ref_array_item **back_index; - if (!is_base_tips.nr || !array->nr) + if (!array->nr) + return; + + for (size_t i = is_base_nr = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_ISBASE) + is_base_nr++; + } + if (!is_base_nr) return; CALLOC_ARRAY(back_index, array->nr); @@ -3153,7 +3157,7 @@ void filter_is_base(struct repository *r, const char *name = array->items[i]->refname; struct commit *c = lookup_commit_reference_by_name_gently(name, 1); - CALLOC_ARRAY(array->items[i]->is_base, is_base_tips.nr); + CALLOC_ARRAY(array->items[i]->is_base, is_base_nr); if (!c) continue; @@ -3163,15 +3167,20 @@ void filter_is_base(struct repository *r, bases_nr++; } - for (size_t i = 0; i < is_base_tips.nr; i++) { - struct commit *tip = is_base_tips.items[i].util; - int base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); + for (size_t i = 0, j = 0; i < used_atom_cnt; i++) { + struct commit *tip; + int base_index; + if (used_atom[i].atom_type != ATOM_ISBASE) + continue; + + tip = used_atom[i].u.base.commit; + base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); if (base_index < 0) continue; /* Store the string for use in output later. */ - back_index[base_index]->is_base[i] = xstrdup(is_base_tips.items[i].string); + back_index[base_index]->is_base[j++] = xstrdup(used_atom[i].u.base.name); } free(back_index); @@ -3290,10 +3299,10 @@ static inline int can_do_iterative_format(struct ref_filter *filter, for (size_t i = 0; i < used_atom_cnt; i++) { if (used_atom[i].atom_type == ATOM_AHEADBEHIND) return 0; + if (used_atom[i].atom_type == ATOM_ISBASE) + return 0; } - return !(filter->reachable_from || - filter->unreachable_from || - is_base_tips.nr); + return !(filter->reachable_from || filter->unreachable_from); } void filter_and_format_refs(struct ref_filter *filter, unsigned int type, -- 2.48.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-18 17:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe @ 2025-01-19 9:26 ` René Scharfe 2025-01-19 12:40 ` Jeff King 2 siblings, 1 reply; 19+ messages in thread From: René Scharfe @ 2025-01-19 9:26 UTC (permalink / raw) To: Jeff King; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano Am 18.01.25 um 18:11 schrieb René Scharfe: > I think moving to the right place in one step requires less churn. On the other hand attribution would be more correct if we stacked your patches on my v1. Just don't forget to free u.base.name. ;) René ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-19 9:26 ` René Scharfe @ 2025-01-19 12:40 ` Jeff King 0 siblings, 0 replies; 19+ messages in thread From: Jeff King @ 2025-01-19 12:40 UTC (permalink / raw) To: René Scharfe; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano On Sun, Jan 19, 2025 at 10:26:26AM +0100, René Scharfe wrote: > Am 18.01.25 um 18:11 schrieb René Scharfe: > > I think moving to the right place in one step requires less churn. > > On the other hand attribution would be more correct if we stacked your > patches on my v1. Just don't forget to free u.base.name. ;) :) Yes, I actually had prepared them on top of yours yesterday before I saw your email. And I also double checked that you found the leak in my original. (I don't usually leak check explicitly, but wait for CI to do it, and I only run that when I do my daily integration). I hadn't gotten around to writing commit messages, though. So I think we should go with what the v2 you posted, as it is already done. For attribution, I am happy enough with a Helped-by trailer. :) I also like how you pulled the final cleanup into patch 3. It's probably reasonable to do, as the code is not doing anything useful. The only thing we lose is that the original had found all of the spots that needed to call _clear(), so if we later need it again we'd have to do so again. But maybe that hypothetical author would find the commit in the history. Actually, ls-remote does not appear to have called it. I didn't check but presumably "ls-remote --format=%(is-base:foo)" leaked. -Peff ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting 2025-01-16 9:51 ` Jeff King 2025-01-16 10:06 ` Jeff King @ 2025-01-18 17:11 ` René Scharfe 1 sibling, 0 replies; 19+ messages in thread From: René Scharfe @ 2025-01-18 17:11 UTC (permalink / raw) To: Jeff King; +Cc: Ross Goldberg, git, Derrick Stolee, Junio C Hamano Am 16.01.25 um 10:51 schrieb Jeff King: > Yet another option in the near term might be storing these ahead-behind > bits in the individual atoms. That makes a lot of sense. René ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/3] ref-filter: move ahead-behind bases into used_atom 2025-01-12 10:01 ` [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting René Scharfe 2025-01-13 5:17 ` Jeff King @ 2025-01-18 17:11 ` René Scharfe 2025-01-18 17:11 ` [PATCH v2 2/3] ref-filter: move is-base tip to used_atom René Scharfe 2025-01-18 17:11 ` [PATCH v2 3/3] ref-filter: remove ref_format_clear() René Scharfe 3 siblings, 0 replies; 19+ messages in thread From: René Scharfe @ 2025-01-18 17:11 UTC (permalink / raw) To: git; +Cc: Jeff King, Derrick Stolee, Junio C Hamano, Ross Goldberg verify_ref_format() parses a ref-filter format string and stores recognized items in the static array "used_atom". For "ahead-behind:<committish>" it stores the committish part in a string_list member "bases" of struct ref_format. ref_sorting_options() also parses bare ref-filter format items and stores stores recognized ones in "used_atom" as well. The committish parts go to a dummy struct ref_format in parse_sorting_atom(), though, and are leaked and forgotten. If verify_ref_format() is called before ref_sorting_options(), like in git for-each-ref, then all works well if the sort key is included in the format string. If it isn't then sorting cannot work as the committishes are missing. If ref_sorting_options() is called first, like in git branch, then we have the additional issue that if the sort key is included in the format string then filter_ahead_behind() can't see its committish, will not generate any results for it and thus it will be expanded to an empty string. Fix those issues by replacing the string_list with a field in used_atom for storing the committish. This way it can be shared for handling both ref-filter format strings and sorting options in the same command. Reported-by: Ross Goldberg <ross.goldberg@gmail.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/branch.c | 2 +- ref-filter.c | 50 ++++++++++++++++++++++++---------------- ref-filter.h | 5 ---- t/t3203-branch-output.sh | 28 ++++++++++++++++++++++ 4 files changed, 59 insertions(+), 26 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 6e7b0cfddb..fbb9536282 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -473,7 +473,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin if (verify_ref_format(format)) die(_("unable to parse format string")); - filter_ahead_behind(the_repository, format, &array); + filter_ahead_behind(the_repository, &array); ref_array_sort(sorting, &array); if (column_active(colopts)) { diff --git a/ref-filter.c b/ref-filter.c index bf5534605e..c3957473e5 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -235,6 +235,9 @@ static struct used_atom { enum { S_BARE, S_GRADE, S_SIGNER, S_KEY, S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; + struct { + struct commit *commit; + } base; struct strvec describe_args; struct refname_atom refname; char *head; @@ -891,18 +894,15 @@ static int rest_atom_parser(struct ref_format *format UNUSED, return 0; } -static int ahead_behind_atom_parser(struct ref_format *format, - struct used_atom *atom UNUSED, +static int ahead_behind_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, const char *arg, struct strbuf *err) { - struct string_list_item *item; - if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(ahead-behind:<committish>)")); - item = string_list_append(&format->bases, arg); - item->util = lookup_commit_reference_by_name(arg); - if (!item->util) + atom->u.base.commit = lookup_commit_reference_by_name(arg); + if (!atom->u.base.commit) die("failed to find '%s'", arg); return 0; @@ -3084,22 +3084,30 @@ static void reach_filter(struct ref_array *array, } void filter_ahead_behind(struct repository *r, - struct ref_format *format, struct ref_array *array) { struct commit **commits; - size_t commits_nr = format->bases.nr + array->nr; + size_t bases_nr, commits_nr; - if (!format->bases.nr || !array->nr) + if (!array->nr) return; - ALLOC_ARRAY(commits, commits_nr); - for (size_t i = 0; i < format->bases.nr; i++) - commits[i] = format->bases.items[i].util; + for (size_t i = bases_nr = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + bases_nr++; + } + if (!bases_nr) + return; - ALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr)); + ALLOC_ARRAY(commits, st_add(bases_nr, array->nr)); + for (size_t i = 0, j = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + commits[j++] = used_atom[i].u.base.commit; + } - commits_nr = format->bases.nr; + ALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr)); + + commits_nr = bases_nr; array->counts_nr = 0; for (size_t i = 0; i < array->nr; i++) { const char *name = array->items[i]->refname; @@ -3108,8 +3116,8 @@ void filter_ahead_behind(struct repository *r, if (!commits[commits_nr]) continue; - CALLOC_ARRAY(array->items[i]->counts, format->bases.nr); - for (size_t j = 0; j < format->bases.nr; j++) { + CALLOC_ARRAY(array->items[i]->counts, bases_nr); + for (size_t j = 0; j < bases_nr; j++) { struct ahead_behind_count *count; count = &array->counts[array->counts_nr++]; count->tip_index = commits_nr; @@ -3277,9 +3285,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter, * - filtering on reachability * - including ahead-behind information in the formatted output */ + for (size_t i = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_AHEADBEHIND) + return 0; + } return !(filter->reachable_from || filter->unreachable_from || - format->bases.nr || format->is_base_tips.nr); } @@ -3303,7 +3314,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type, } else { struct ref_array array = { 0 }; filter_refs(&array, filter, type); - filter_ahead_behind(the_repository, format, &array); + filter_ahead_behind(the_repository, &array); filter_is_base(the_repository, format, &array); ref_array_sort(sorting, &array); print_formatted_ref_array(&array, format); @@ -3647,7 +3658,6 @@ void ref_format_init(struct ref_format *format) void ref_format_clear(struct ref_format *format) { - string_list_clear(&format->bases, 0); string_list_clear(&format->is_base_tips, 0); ref_format_init(format); } diff --git a/ref-filter.h b/ref-filter.h index 754038ab07..5f3dd6c931 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -99,9 +99,6 @@ struct ref_format { /* Internal state to ref-filter */ int need_color_reset_at_eol; - /* List of bases for ahead-behind counts. */ - struct string_list bases; - /* List of bases for is-base indicators. */ struct string_list is_base_tips; @@ -117,7 +114,6 @@ struct ref_format { } #define REF_FORMAT_INIT { \ .use_color = -1, \ - .bases = STRING_LIST_INIT_DUP, \ .is_base_tips = STRING_LIST_INIT_DUP, \ } @@ -205,7 +201,6 @@ struct ref_array_item *ref_array_push(struct ref_array *array, * If this is not called, then any ahead-behind atoms will be blank. */ void filter_ahead_behind(struct repository *r, - struct ref_format *format, struct ref_array *array); /* diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index 500c9d0e72..a6bd88a58d 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -368,6 +368,34 @@ test_expect_success 'git branch --format with ahead-behind' ' test_cmp expect actual ' +test_expect_success 'git branch `--sort=[-]ahead-behind` option' ' + cat >expect <<-\EOF && + (HEAD detached from fromtag) 0 0 + refs/heads/ambiguous 0 0 + refs/heads/branch-two 0 0 + refs/heads/branch-one 1 0 + refs/heads/main 1 0 + refs/heads/ref-to-branch 1 0 + refs/heads/ref-to-remote 1 0 + EOF + git branch --format="%(refname) %(ahead-behind:HEAD)" \ + --sort=refname --sort=ahead-behind:HEAD >actual && + test_cmp expect actual && + + cat >expect <<-\EOF && + (HEAD detached from fromtag) 0 0 + refs/heads/branch-one 1 0 + refs/heads/main 1 0 + refs/heads/ref-to-branch 1 0 + refs/heads/ref-to-remote 1 0 + refs/heads/ambiguous 0 0 + refs/heads/branch-two 0 0 + EOF + git branch --format="%(refname) %(ahead-behind:HEAD)" \ + --sort=refname --sort=-ahead-behind:HEAD >actual && + test_cmp expect actual +' + test_expect_success 'git branch with --format=%(rest) must fail' ' test_must_fail git branch --format="%(rest)" >actual ' -- 2.48.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/3] ref-filter: move is-base tip to used_atom 2025-01-12 10:01 ` [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting René Scharfe 2025-01-13 5:17 ` Jeff King 2025-01-18 17:11 ` [PATCH v2 1/3] ref-filter: move ahead-behind bases into used_atom René Scharfe @ 2025-01-18 17:11 ` René Scharfe 2025-01-18 17:11 ` [PATCH v2 3/3] ref-filter: remove ref_format_clear() René Scharfe 3 siblings, 0 replies; 19+ messages in thread From: René Scharfe @ 2025-01-18 17:11 UTC (permalink / raw) To: git; +Cc: Jeff King, Derrick Stolee, Junio C Hamano, Ross Goldberg The string_list "is_base_tips" in struct ref_format stores the committish part of "is-base:<committish>". It has the same problems that its sibling string_list "bases" had. Fix them the same way as the previous commit did for the latter, by replacing the string_list with fields in "used_atom". Helped-by: Jeff King <peff@peff.net> Signed-off-by: René Scharfe <l.s.r@web.de> --- ref-filter.c | 56 +++++++++++++++++++++++++------------------ ref-filter.h | 5 ---- t/t6600-test-reach.sh | 29 ++++++++++++++++++++++ 3 files changed, 62 insertions(+), 28 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index c3957473e5..9a4b2d4cea 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -236,6 +236,7 @@ static struct used_atom { S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option; } signature; struct { + char *name; struct commit *commit; } base; struct strvec describe_args; @@ -908,18 +909,16 @@ static int ahead_behind_atom_parser(struct ref_format *format UNUSED, return 0; } -static int is_base_atom_parser(struct ref_format *format, - struct used_atom *atom UNUSED, +static int is_base_atom_parser(struct ref_format *format UNUSED, + struct used_atom *atom, const char *arg, struct strbuf *err) { - struct string_list_item *item; - if (!arg) return strbuf_addf_ret(err, -1, _("expected format: %%(is-base:<committish>)")); - item = string_list_append(&format->is_base_tips, arg); - item->util = lookup_commit_reference_by_name(arg); - if (!item->util) + atom->u.base.name = xstrdup(arg); + atom->u.base.commit = lookup_commit_reference_by_name(arg); + if (!atom->u.base.commit) die("failed to find '%s'", arg); return 0; @@ -3009,6 +3008,8 @@ void ref_array_clear(struct ref_array *array) free(atom->u.head); else if (atom->atom_type == ATOM_DESCRIBE) strvec_clear(&atom->u.describe_args); + else if (atom->atom_type == ATOM_ISBASE) + free(atom->u.base.name); else if (atom->atom_type == ATOM_TRAILERS || (atom->atom_type == ATOM_CONTENTS && atom->u.contents.option == C_TRAILERS)) { @@ -3133,14 +3134,20 @@ void filter_ahead_behind(struct repository *r, } void filter_is_base(struct repository *r, - struct ref_format *format, struct ref_array *array) { struct commit **bases; - size_t bases_nr = 0; + size_t bases_nr = 0, is_base_nr; struct ref_array_item **back_index; - if (!format->is_base_tips.nr || !array->nr) + if (!array->nr) + return; + + for (size_t i = is_base_nr = 0; i < used_atom_cnt; i++) { + if (used_atom[i].atom_type == ATOM_ISBASE) + is_base_nr++; + } + if (!is_base_nr) return; CALLOC_ARRAY(back_index, array->nr); @@ -3150,7 +3157,7 @@ void filter_is_base(struct repository *r, const char *name = array->items[i]->refname; struct commit *c = lookup_commit_reference_by_name_gently(name, 1); - CALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr); + CALLOC_ARRAY(array->items[i]->is_base, is_base_nr); if (!c) continue; @@ -3160,15 +3167,20 @@ void filter_is_base(struct repository *r, bases_nr++; } - for (size_t i = 0; i < format->is_base_tips.nr; i++) { - struct commit *tip = format->is_base_tips.items[i].util; - int base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); + for (size_t i = 0, j = 0; i < used_atom_cnt; i++) { + struct commit *tip; + int base_index; + + if (used_atom[i].atom_type != ATOM_ISBASE) + continue; + tip = used_atom[i].u.base.commit; + base_index = get_branch_base_for_tip(r, tip, bases, bases_nr); if (base_index < 0) continue; /* Store the string for use in output later. */ - back_index[base_index]->is_base[i] = xstrdup(format->is_base_tips.items[i].string); + back_index[base_index]->is_base[j++] = xstrdup(used_atom[i].u.base.name); } free(back_index); @@ -3260,8 +3272,7 @@ struct ref_sorting { }; static inline int can_do_iterative_format(struct ref_filter *filter, - struct ref_sorting *sorting, - struct ref_format *format) + struct ref_sorting *sorting) { /* * Reference backends sort patterns lexicographically by refname, so if @@ -3288,17 +3299,17 @@ static inline int can_do_iterative_format(struct ref_filter *filter, for (size_t i = 0; i < used_atom_cnt; i++) { if (used_atom[i].atom_type == ATOM_AHEADBEHIND) return 0; + if (used_atom[i].atom_type == ATOM_ISBASE) + return 0; } - return !(filter->reachable_from || - filter->unreachable_from || - format->is_base_tips.nr); + return !(filter->reachable_from || filter->unreachable_from); } void filter_and_format_refs(struct ref_filter *filter, unsigned int type, struct ref_sorting *sorting, struct ref_format *format) { - if (can_do_iterative_format(filter, sorting, format)) { + if (can_do_iterative_format(filter, sorting)) { int save_commit_buffer_orig; struct ref_filter_and_format_cbdata ref_cbdata = { .filter = filter, @@ -3315,7 +3326,7 @@ void filter_and_format_refs(struct ref_filter *filter, unsigned int type, struct ref_array array = { 0 }; filter_refs(&array, filter, type); filter_ahead_behind(the_repository, &array); - filter_is_base(the_repository, format, &array); + filter_is_base(the_repository, &array); ref_array_sort(sorting, &array); print_formatted_ref_array(&array, format); ref_array_clear(&array); @@ -3658,6 +3669,5 @@ void ref_format_init(struct ref_format *format) void ref_format_clear(struct ref_format *format) { - string_list_clear(&format->is_base_tips, 0); ref_format_init(format); } diff --git a/ref-filter.h b/ref-filter.h index 5f3dd6c931..0ba94df651 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -99,9 +99,6 @@ struct ref_format { /* Internal state to ref-filter */ int need_color_reset_at_eol; - /* List of bases for is-base indicators. */ - struct string_list is_base_tips; - struct { int max_count; int omit_empty; @@ -114,7 +111,6 @@ struct ref_format { } #define REF_FORMAT_INIT { \ .use_color = -1, \ - .is_base_tips = STRING_LIST_INIT_DUP, \ } /* Macros for checking --merged and --no-merged options */ @@ -210,7 +206,6 @@ void filter_ahead_behind(struct repository *r, * If this is not called, then any is-base atoms will be blank. */ void filter_is_base(struct repository *r, - struct ref_format *format, struct ref_array *array); void ref_filter_init(struct ref_filter *filter); diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index 2591f8b8b3..6638d1aa1d 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -733,4 +733,33 @@ test_expect_success 'for-each-ref is-base:multiple' ' --format="%(refname)[%(is-base:commit-2-3)-%(is-base:commit-6-5)]" --stdin ' +test_expect_success 'for-each-ref is-base: --sort' ' + cat >input <<-\EOF && + refs/heads/commit-1-1 + refs/heads/commit-4-2 + refs/heads/commit-4-4 + refs/heads/commit-8-4 + EOF + + cat >expect <<-\EOF && + refs/heads/commit-1-1 + refs/heads/commit-4-4 + refs/heads/commit-8-4 + refs/heads/commit-4-2 + EOF + run_all_modes git for-each-ref \ + --format="%(refname)" --stdin \ + --sort=refname --sort=is-base:commit-2-3 && + + cat >expect <<-\EOF && + refs/heads/commit-4-2 + refs/heads/commit-1-1 + refs/heads/commit-4-4 + refs/heads/commit-8-4 + EOF + run_all_modes git for-each-ref \ + --format="%(refname)" --stdin \ + --sort=refname --sort=-is-base:commit-2-3 +' + test_done -- 2.48.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/3] ref-filter: remove ref_format_clear() 2025-01-12 10:01 ` [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting René Scharfe ` (2 preceding siblings ...) 2025-01-18 17:11 ` [PATCH v2 2/3] ref-filter: move is-base tip to used_atom René Scharfe @ 2025-01-18 17:11 ` René Scharfe 3 siblings, 0 replies; 19+ messages in thread From: René Scharfe @ 2025-01-18 17:11 UTC (permalink / raw) To: git; +Cc: Jeff King, Derrick Stolee, Junio C Hamano, Ross Goldberg Now that ref_format_clear() no longer releases any memory we don't need it anymore. Remove it and its counterpart, ref_format_init(). Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/branch.c | 1 - builtin/for-each-ref.c | 1 - builtin/tag.c | 1 - builtin/verify-tag.c | 1 - ref-filter.c | 11 ----------- ref-filter.h | 3 --- 6 files changed, 18 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index fbb9536282..9a29de5bf1 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -884,7 +884,6 @@ int cmd_branch(int argc, string_list_clear(&output, 0); ref_sorting_release(sorting); ref_filter_clear(&filter); - ref_format_clear(&format); ret = 0; goto out; diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 715745a262..8085ebd8fe 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -108,7 +108,6 @@ int cmd_for_each_ref(int argc, filter_and_format_refs(&filter, flags, sorting, &format); ref_filter_clear(&filter); - ref_format_clear(&format); ref_sorting_release(sorting); strvec_clear(&vec); return 0; diff --git a/builtin/tag.c b/builtin/tag.c index c4bd145831..e8a344b926 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -698,7 +698,6 @@ int cmd_tag(int argc, cleanup: ref_sorting_release(sorting); ref_filter_clear(&filter); - ref_format_clear(&format); strbuf_release(&buf); strbuf_release(&ref); strbuf_release(&reflog_msg); diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index a7f20618ff..f6b97048a5 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -69,6 +69,5 @@ int cmd_verify_tag(int argc, if (format.format) pretty_print_ref(name, &oid, &format); } - ref_format_clear(&format); return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 9a4b2d4cea..6da8d4c03b 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3660,14 +3660,3 @@ void ref_filter_clear(struct ref_filter *filter) free_commit_list(filter->unreachable_from); ref_filter_init(filter); } - -void ref_format_init(struct ref_format *format) -{ - struct ref_format blank = REF_FORMAT_INIT; - memcpy(format, &blank, sizeof(blank)); -} - -void ref_format_clear(struct ref_format *format) -{ - ref_format_init(format); -} diff --git a/ref-filter.h b/ref-filter.h index 0ba94df651..013d4cfa64 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -211,7 +211,4 @@ void filter_is_base(struct repository *r, void ref_filter_init(struct ref_filter *filter); void ref_filter_clear(struct ref_filter *filter); -void ref_format_init(struct ref_format *format); -void ref_format_clear(struct ref_format *format); - #endif /* REF_FILTER_H */ -- 2.48.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
end of thread, other threads:[~2025-01-19 12:46 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-05 9:48 Bug sorting git branch output by ahead-behind:HEAD Ross Goldberg 2025-01-12 10:01 ` [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting René Scharfe 2025-01-13 5:17 ` Jeff King 2025-01-13 5:23 ` Jeff King 2025-01-13 17:25 ` Junio C Hamano 2025-01-14 18:55 ` René Scharfe 2025-01-16 9:51 ` Jeff King 2025-01-16 10:06 ` Jeff King 2025-01-16 10:21 ` Jeff King 2025-01-16 17:31 ` Junio C Hamano 2025-01-18 17:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe 2025-01-19 9:11 ` René Scharfe 2025-01-19 9:26 ` René Scharfe 2025-01-19 12:40 ` Jeff King 2025-01-18 17:11 ` René Scharfe 2025-01-18 17:11 ` [PATCH v2 1/3] ref-filter: move ahead-behind bases into used_atom René Scharfe 2025-01-18 17:11 ` [PATCH v2 2/3] ref-filter: move is-base tip to used_atom René Scharfe 2025-01-18 17:11 ` [PATCH v2 3/3] ref-filter: remove ref_format_clear() René Scharfe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).