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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 8, 2024, 17:24 UTC
Message-ID: <ZcUOP_rWUwymhe5c@ncase>
In-Reply-To: <xmqq7cjeq43t.fsf@gitster.g>

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

Show 21 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > ```
> > 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.
> 
> The reftable format, as a database format, may be capable of having
> "refs/heads/master" and "refs/heads/master/1" at the same time, but
> to be used as a ref backend for Git, it must refrain from surfacing
> both at the same time.  I think it is the same deal that it should
> only allow "refs/*", "HEAD", and so called pseudorefs to be stored.
> So INCLUDE_ROOT_REFS should be sufficient as long as the "ref
> creation and update" side is not letting random cruft (e.g.,
> "config") in.  Isn't that sufficient?

That's a different problem from the one I have right now. Let's take the following sequence of commands:

    $ git init repo
    Initialized empty Git repository in /tmp/repo/.git/
    $ git -C repo commit --allow-empty --message message
    [main (root-commit) aa5eec4] message
    $ git -C repo update-ref ref/head/foo HEAD
    $ ls repo/.git/ref/head/foo
    repo/.git/ref/head/foo

Now the fact that you can create "ref/head/foo" is a bug that needs to be fixed, no arguing there. The problem is that rectifying this problem with the "files" backend is easy -- you look into the repo, notice that there's a weird directory, and then "rm -rf" it.

But how do you learn about this ref existing with the "reftable" backend in the first place? You can't without looking at the binary format -- there doesn't exist a single command that would allow you to list all refs unfiltered. But that is very much required in order to learn about misbehaviour and fix it.

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.

Spelled out like that it brings me a different idea: maybe I'm just trying to address this in the wrong tool. I plan to introduce ref backend specific fsck checks, so that could be a better place to warn about such refs with bad names. Like this we don't erode the tree-shaped nature by somehow accepting them in some tools, and we make clear that this is indeed something that shouldn't happen.

Show 16 quoted lines

> > 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.
> 
> It is not "focused on".  The ref namespace of Git is tree-shaped,
> period.  The shape may have originated from its first ref backend
> implementation's limitation, but as we gain other backends, we are
> not planning to lift such limitations, are we?  So we may still say
> "when there is a master branch, you cannot have master/1 branch (due
> to D/F conflict)", even if there is no notion of directory or file
> in a backend implementation backed by a databasy file format.  "HEAD"
> and "CHERRY_PICK_HEAD", unlike "refs/tags/v1.0", are at the "root
> level", not only when they are stored in a files backend, but always
> when they are presented to end-users, who can tell that they are not
> inside "refs/".

I agree, and I do not intend to change this.

Patrick