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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 9, 2024, 08:08 UTC
Message-ID: <ZcXddvQzlt6j7T7L@tanuki>
In-Reply-To: <xmqq34u2onaj.fsf@gitster.g>

On Thu, Feb 08, 2024 at 09:53:24AM -0800, Junio C Hamano wrote:

> Patrick Steinhardt <ps@pks.im> writes:

[snip]

Show 16 quoted lines

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