diff mbox series

ref-filter: share bases and is_base_tips between formatting and sorting

Message ID 6b824f05-6f16-4cd9-85b7-3b8b236158b4@web.de (mailing list archive)
State New
Headers show
Series ref-filter: share bases and is_base_tips between formatting and sorting | expand

Commit Message

René Scharfe Jan. 12, 2025, 10:01 a.m. UTC
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(-)

--
2.48.0

Comments

Jeff King Jan. 13, 2025, 5:17 a.m. UTC | #1
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 */
Jeff King Jan. 13, 2025, 5:23 a.m. UTC | #2
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
Junio C Hamano Jan. 13, 2025, 5:25 p.m. UTC | #3
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 */
René Scharfe Jan. 14, 2025, 6:55 p.m. UTC | #4
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é
diff mbox series

Patch

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