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

[Jeff King <peff@peff.net>]

On Wed, May 15, 2024 at 06:16:18AM +0200, Patrick Steinhardt wrote:

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

Thanks for doing the digging on the callers. That matches my intuition /
light analysis, which is good. ;)

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

I don't mind deferring. I thought it might make the simplifications
you're doing in this series easier to reason about. But TBH I haven't
had the chance to look through your series very carefully yet (and I'm
still a bit back-logged), so I'm happy to go with your judgement on how
to split it up.

-Peff