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

[Patrick Steinhardt <ps@pks.im>]

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

On Thu, Feb 08, 2024 at 09:53:24AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > As I said -- this is a bug, and I agree that it shouldn't happen. But
> > bugs happen, and especially with the new reftable format I expect them
> > to happen. What I look for in this context is to create the tools to fix
> > problems like this, but `--include-root-refs` doesn't. A flag that
> > unconditionally returns all refs, regardless of whether they have a bad
> > name or not, does address the issue. Think of it of more of a debugging
> > tool.
> 
> OK, "--include-all-refs" would be fine.  And without bugs there
> should not be a difference.
> 
> Where does "all refs in this worktree" you mentioned fit in this
> picture?  Should a bogus "ref/foo/bar" be considered to be worktree
> specific, or is it an incorrect attempt to create a ref that is
> specific to 'foo' worktree that is not the current one and be
> filtered out?

Good question indeed. The reason I said "all refs in this worktree" is
mostly that I don't think we should also be returning refs from other
worktrees. I consider their refdbs to be separate refdbs, even though it
is possible to access them via the special "worktrees/$wt/refs" syntax.
So I would say that such an option should return all refs of the current
worktree's refdb, plus the common worktree refs.

Another important question here is how the "files" backend should behave
with such a flag. Should it try to read all loose files as refs and
return those which just happen to look like one? That feels really wrong
to me, as we would now have to special case those namespaces where we
know that it's not a proper ref, like "worktrees/", "rebase-apply/" and
others.

The alternative would be to make it behave like `--include-root-refs`,
where we do a best effort and live with the fact that the "files"
backend cannot enumerate all refs it has created. This would mean that
behaviour between the "files" and "reftable" backend is diverging though
because the latter can trivially figure out all refs it contains. The
question is whether we are fine with that or not.

Depending on the answer, I think we can go one of two ways:

  - Accept the diverging behaviour and add `--include-all-refs`. The
    "files" backend does a best effort and only includes root refs, the
    "reftable" backend lists all refs.

  - Double down on the fact that refs must either be pseudo refs or
    start with "refs/" and treat any ref that doesn't fit this bill as
    corrupted. The consequence here would be that we instead go with
    `--include-root-refs` that can be implemented the same for both
    backends. In addition, we add checks to git-fsck(1) to surface and
    flag refs with bogus names for the "reftable" backend.

While I seem to have convinced you that `--include-all-refs` might not
be a bad idea after all, you have convinced me by now that the second
option would be preferable. I'd be okay with either of these options as
both of them address the issue at hand.

Patrick

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