Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting

> 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.