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
