Re: [PATCH v5 06/10] refs: do not check ref existence in `is_root_ref()`

Justin Tobler
2024-05-15T20:40:16Z

On 24/05/15 08:50AM, 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.
>
> 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`,
> but then read it via `is_null_oid()`.
>
> We have now changed terminology to clarify that pseudorefs are really
> only "MERGE_HEAD" and "FETCH_HEAD", whereas all the other refs that live
> in the root of the ref hierarchy are just plain refs. Thus, we do not
> need to check whether the ref is symbolic or not. In fact, we can now
> avoid looking up the ref completely as the name is sufficient for us to
> figure out whether something would be a root ref or not.
>
> This change of course changes semantics for our callers. As there are
> only three of them we can assess each of them individually:
>
> - "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/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 we know it does exist.
>
> - "refs/files_backend.c:add_pseudoref_and_head_entries()" uses it to
> determine whether it should add a ref to the root directory of its
> iterator. This had the effect that we skipped over any files that
> are either a symbolic ref, or which are not a ref at all.
>
> The new behaviour is to include symbolic refs know, which aligns us

s/know/now/

> with the adapted terminology. Furthermore, files which look like
> root refs but aren't are now mark those as "broken". As broken refs
> are not surfaced by our tooling, this should not lead to a change in
> user-visible behaviour, but may cause us to emit warnings. This
> feels like the right thing to do as we would otherwise just silently
> ignore corrupted root refs completely.

Is there an expected source of broken root refs? Or would it just be due
to bugs?

> So in all cases the existence check was either superfluous, not in line
> with the adapted terminology or masked potential issues. This commit
> thus changes the behaviour as proposed and drops the existence check
> altogether.

Dropping the existence check makes sense to me. It also has the added
benefit of simplifying `is_root_ref()` which is nice.

>
> Add a test that verifies that this does not change user-visible
> behaviour. Namely, we still don't want to show broken refs to the user
> by default in git-for-each-ref(1). What this does allow though is for
> internal callers to surface dangling root refs when they pass in the
> `DO_FOR_EACH_INCLUDE_BROKEN` flag.