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

Karthik Nayak
2024-05-02T10:03:38Z

[-- Attachment #1: Type: text/plain, Size: 2910 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

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

Right..

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

Agreed. Let's keep it as is!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]