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

[Patrick Steinhardt <ps@pks.im>]

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

On Wed, Feb 07, 2024 at 06:02:34PM +0100, Karthik Nayak wrote:
> 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

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