Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting
- From
Jeff King <peff@peff.net>
- Date
- Jan 13, 2025, 05:17 UTC
- Message-ID
- <20250113051700.GA767856@coredump.intra.peff.net>
- In-Reply-To
- <6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de>
On Sun, Jan 12, 2025 at 11:01:52AM +0100, René Scharfe wrote:
Show 20 quoted lines
> 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.
Show 5 quoted lines
> 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 */