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é
