Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 8, 2024, 08:50 UTC
Message-ID: <ZcSVx4slikt4xB3D@tanuki>
In-Reply-To: <CAOLa=ZQaXxwrXmbmFvGR59EDo3Eqa-Xfc3OG9+6ES-veDU8Bhg@mail.gmail.com>

On Wed, Feb 07, 2024 at 06:02:34PM +0100, Karthik Nayak wrote:

Show 19 quoted lines

> On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Karthik Nayak <karthik.188@gmail.com> writes:
> >
> > > This is a bit of a grey area, what I mean is that we do allow users to create
> > > non "refs/" prefixed refs:
> > >
> > >     $ git update-ref foo @~1
> > >
> > >     $ cat .git/foo
> > >     2b52187cd2930931c6d34436371f470bb26eef4f
> > >
> > > What I mean to say is that, by saying "--include-root-refs" it seems to imply
> > > that any such refs should be included too, but this simply is not the case.
> >
> > But isn't that a quality of implementation issue?  I'd consider it a
> > bug once we have and enforce the definition of what pseudorefs are.
> 
> Yeah, that makes sense. I'll use "--include-root-refs" then.

I'd still argue that this is too specific to the files backend. The fact that we end up only adding root refs to the files backend to me is an implementation-specific limitation and not a feature. What I really want is to ask the backend to give me all refs that it knows of for the current worktree.

The "reftable" backend has this snippet in its iterator:

```
/*
 * The files backend only lists references contained in
 * "refs/". We emulate the same behaviour here and thus skip
 * all references that don't start with this prefix.
 */
if (!starts_with(iter->ref.refname, "refs/"))
    continue;
```

If we add the new `--include-root-refs` flag then we would have to extend it to query whether the ref either starts with "refs/" or whether it might look like a pseudo-ref:

```
if (!starts_with(iter->ref.refname, "refs/") &&
    !(flags & INCLUDE_ROOT_REFS || is_pseudoref(iter->ref.refname)))
    continue;
```

The problem I have is that it still wouldn't end up surfacing all refs which exist in the ref backend while being computationally more expensive. So the original usecase I had in mind when pitching this topic isn't actually addressed.

I know that in theory, the reftable backend shouldn't contain refs other than "refs/" or pseudo-refs anyway. But regardless of that, I think that formulating this in the form of "root refs" is too restrictive and too much focussed on the "files" backend. I wouldn't be surprised if there are ways to have git-update-ref(1) or other tools write refs with unexpected names, and not being able to learn about those is a limitation.

Putting this in the context of "Give me all refs which you know of for the current worktree" is a lot simpler. It would still be equivalent to "--include-root-refs" for the "files" backend, because the files backend doesn't have a way to properly track all refs it has ever created. And for the "reftable" backend it's as simple as this:

```
if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
    !starts_with(iter->ref.refname, "refs/"))
```

I'm not sure about better terminology though. Something like `--include-all-worktree-refs` could easily lead to confusion because it might make the user think that they also want to list refs from other worktrees. The best I can come up with is `--include-non-refs` -- pseudo refs aren't real refs, and neither are refs which don't start with "refs/".

Patrick