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

[Patrick Steinhardt <ps@pks.im>]

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

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.

Patrick

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