Re: [PATCH v3 07/10] refs: root refs can be symbolic refs

[Patrick Steinhardt <ps@pks.im>]

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

On Fri, May 03, 2024 at 02:13:39PM -0400, Jeff King wrote:
> On Thu, May 02, 2024 at 10:17:42AM +0200, Patrick Steinhardt wrote:
> 
> > Before this patch series, root refs except for "HEAD" and our special
> > refs were classified as pseudorefs. Furthermore, our terminology
> > clarified that pseudorefs must not be symbolic refs. This restriction
> > is enforced in `is_root_ref()`, which explicitly checks that a supposed
> > root ref resolves to an object ID without recursing.
> > 
> > This has been extremely confusing right from the start because (in old
> > terminology) a ref name may sometimes be a pseudoref and sometimes not
> > depending on whether it is a symbolic or regular ref. This behaviour
> > does not seem reasonable at all and I very much doubt that it results in
> > anything sane.
> > 
> > Furthermore, the behaviour is different to `is_headref()`, which only
> > checks for the ref to exist. While that is in line with our glossary,
> > this inconsistency only adds to the confusion.
> > 
> > Last but not least, the current behaviour can actually lead to a
> > segfault when calling `is_root_ref()` with a reference that either does
> > not exist or that is a symbolic ref because we never initialized `oid`.
> > 
> > Let's loosen the restrictions in accordance to the new definition of
> > root refs, which are simply plain refs that may as well be a symbolic
> > ref. Consequently, we can just check for the ref to exist instead of
> > requiring it to be a regular ref.
> 
> It's not clear to me that this existence check is particularly useful.
> Something that fails read_raw_ref() will fail if:
> 
>   - the file does not exist at all. But then how did somebody find out
>     about it at all to ask is_pseudoref()?
> 
>   - it does exist, but does not look like a ref. Is this important? If I
>     do "echo foo >.git/CHERRY_PICK_HEAD", does it become not a root ref
>     anymore? Or is it a root ref that is broken? I'd have thought the
>     latter, and the syntax is what distinguishes it.
> 
> Making the classification purely syntactic based on the name feels
> simpler to me to reason about. You'll never run into confusing cases
> where repo state changes how commands may behave.

I certainly agree and have been complaining about that in the past, too.
I didn't dare to change the semantics this far yet. Let's have a look at
the callers:

  - "ref-filter.c:ref_kind_from_refname()" uses it to classify refs.
    It's clear that the intent is to classify based on the ref name,
    only.

  - "refs/files_backend.c:add_pseudoref_and_head_entries()" uses it to
    determine whether it should add a ref to the root directory. It
    feels fishy that this uses ref existence checks to do that.

  - "refs/reftable_backend.c:reftable_ref_iterator_advance()" uses it to
    filter root refs. Again, using existence checks is pointless here as
    the iterator has just surfaced the ref, so it does exist.

  - "refs.c:is_current_worktree_ref()" uses it. Fishy as well, as the
    call to `is_per_worktree_ref()` also only checks for the refname.

So let's remove these existence checks altogether and make this a check
that purely checks semantics.

> And arguably is_pseudoref_syntax() should be taking into account the
> "_HEAD" restriction and special names anyway. It is a bit weird that
> even if we tighten up the refname checking to use is_pseudoref_syntax(),
> you'd still be able to "git update-ref FOO" but then not see it as a
> root ref!

True, as well. I'm less comfortable with doing that change in this
series though as it does impose a major restriction that did not exist
previously. We probably want some escape hatches so that it would still
be possible to modify those refs when really required, for example to
delete such broken refs.

I would thus like to defer this to a follow up patch series, if you
don't mind.

Patrick

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