Re: [PATCH v2 09/10] ref-filter: properly distinuish pseudo and root refs

> On Tue, Apr 30, 2024 at 06:11:05AM -0700, Karthik Nayak wrote:
>> In the subject: s/distinuish/distinguish
>>
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > The ref-filter interfaces currently define root refs as either a
>> > detached HEAD or a pseudo ref. Pseudo refs aren't root refs though, so
>> > let's properly distinguish those ref types.
>> >
>> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
>> > ---
>> >  builtin/for-each-ref.c |  2 +-
>> >  ref-filter.c           | 16 +++++++++-------
>> >  ref-filter.h           |  4 ++--
>> >  refs.c                 | 18 +-----------------
>> >  refs.h                 | 18 ++++++++++++++++++
>> >  5 files changed, 31 insertions(+), 27 deletions(-)
>> >
>> > diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
>> > index 919282e12a..5517a4a1c0 100644
>> > --- a/builtin/for-each-ref.c
>> > +++ b/builtin/for-each-ref.c
>> > @@ -98,7 +98,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>> >  	}
>> >
>> >  	if (include_root_refs)
>> > -		flags |= FILTER_REFS_ROOT_REFS;
>> > +		flags |= FILTER_REFS_ROOT_REFS | FILTER_REFS_DETACHED_HEAD;
>>
>> The only issue I see with this patch is that it makes me think that HEAD
>> is not a root ref anymore. I get that this is the best way to define the
>> directives because otherwise you'd need a new flag something like
>> `FILTER_REFS_ROOT_REFS_WITHOUT_HEAD` and `FILTER_REFS_ROOT_REFS` would
>> be the summation of that and the HEAD flag.
>>
>> Apart from this, the patch looks good.
>
> Well, it is a root ref, but we treat it differently in the ref-filter
> interface because it's rendered differently than any other root ref.
> Furthermore, the ref-filter interfaces allow you to _only_ list the HEAD
> ref, which is another reason why it's singled out.
>
> Renaming this to FILTER_REFS_ROOT_REFS_WITHOUT_HEAD ould be quite
> misleading, too, because we have the following snippet:
>
> @@ -2794,11 +2796,11 @@ static struct ref_array_item *apply_ref_filter(const char *refname, const struct
>         /*
>          * Generally HEAD refs are printed with special description denoting a rebase,
>          * detached state and so forth. This is useful when only printing the HEAD ref
>          * But when it is being printed along with other root refs, it makes sense to
>          * keep the formatting consistent. So we mask the type to act like a root ref.
>          */
>         if (filter->kind & FILTER_REFS_ROOT_REFS && kind == FILTER_REFS_DETACHED_HEAD)
>                 kind = FILTER_REFS_ROOT_REFS;
>         else if (!(kind & filter->kind))
>                 return NULL;
>

> If we named this FILTER_REFS_ROOT_REFS_WITHOUT_HEAD then the above code
> would be even more surprising.
>
> So yeah, it's a bit weird, but I think it's more sensible to retain the
> code as proposed.