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

From: René Scharfe <[email-masked]>
Date: Tue, 14 Jan 2025 19:55:03 +0100

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é