On Thu, Feb 08, 2024 at 09:04:54AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> 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.
> > 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
Attachment:
signature.asc
Description: PGP signature
