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.
