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

From: Jeff King <peff@peff.net>
Date: May 3, 2024, 18:13 UTC
Message-ID: <20240503181339.GH3631237@coredump.intra.peff.net>
In-Reply-To: <92a71222e1067ca4ce9ecaaa555d78d0cce0d9d0.1714637671.git.ps@pks.im>

On Thu, May 02, 2024 at 10:17:42AM +0200, Patrick Steinhardt wrote:

Show 24 quoted lines

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

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!

-Peff