refs: do not check ref existence in `is_root_ref()`

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 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. 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. 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. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed May 15, 2024 at 08:50 UTC afcd067dad27fa7eddde01ebde90daa6cc541cc5
6 files changed +29 -20
ref-filter.c
+1 -1
@@ -2756,7 +2756,7 @@ static int ref_kind_from_refname(const char *refname)
2756 return ref_kind[i].kind;
2757 }
2758
2759 - if (is_root_ref(get_main_ref_store(the_repository), refname))
2759 + if (is_root_ref(refname))
2760 return FILTER_REFS_PSEUDOREFS;
2761
2762 return FILTER_REFS_OTHERS;
refs.c
+5 -14
@@ -856,7 +856,7 @@ static int is_root_ref_syntax(const char *refname)
856 return 1;
857 }
858
859 -int is_root_ref(struct ref_store *refs, const char *refname)
859 +int is_root_ref(const char *refname)
860 {
861 static const char *const irregular_root_refs[] = {
862 "AUTO_MERGE",
@@ -865,26 +865,17 @@ int is_root_ref(struct ref_store *refs, const char *refname)
865 "NOTES_MERGE_REF",
866 "MERGE_AUTOSTASH",
867 };
868 - struct object_id oid;
868 size_t i;
869
870 if (!is_root_ref_syntax(refname))
871 return 0;
872
874 - if (ends_with(refname, "_HEAD")) {
875 - refs_resolve_ref_unsafe(refs, refname,
876 - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
877 - &oid, NULL);
878 - return !is_null_oid(&oid);
879 - }
873 + if (ends_with(refname, "_HEAD"))
874 + return 1;
875
876 for (i = 0; i < ARRAY_SIZE(irregular_root_refs); i++)
882 - if (!strcmp(refname, irregular_root_refs[i])) {
883 - refs_resolve_ref_unsafe(refs, refname,
884 - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
885 - &oid, NULL);
886 - return !is_null_oid(&oid);
887 - }
877 + if (!strcmp(refname, irregular_root_refs[i]))
878 + return 1;
879
880 return 0;
881 }
refs.h
+3 -2
@@ -1052,7 +1052,8 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
1052 void update_ref_namespace(enum ref_namespace namespace, char *ref);
1053
1054 /*
1055 - * Check whether the reference is an existing root reference.
1055 + * Check whether the provided name names a root reference. This function only
1056 + * performs a syntactic check.
1057 *
1058 * A root ref is a reference that lives in the root of the reference hierarchy.
1059 * These references must conform to special syntax:
@@ -1076,7 +1077,7 @@ void update_ref_namespace(enum ref_namespace namespace, char *ref);
1077 *
1078 * - MERGE_AUTOSTASH
1079 */
1079 -int is_root_ref(struct ref_store *refs, const char *refname);
1080 +int is_root_ref(const char *refname);
1081
1082 int is_headref(struct ref_store *refs, const char *refname);
1083
refs/files-backend.c
+2 -2
@@ -351,8 +351,8 @@ static void add_pseudoref_and_head_entries(struct ref_store *ref_store,
351 strbuf_addstr(&refname, de->d_name);
352
353 dtype = get_dtype(de, &path, 1);
354 - if (dtype == DT_REG && (is_root_ref(ref_store, de->d_name) ||
355 - is_headref(ref_store, de->d_name)))
354 + if (dtype == DT_REG && (is_root_ref(de->d_name) ||
355 + is_headref(ref_store, de->d_name)))
356 loose_fill_ref_dir_regular_file(refs, refname.buf, dir);
357
358 strbuf_setlen(&refname, dirnamelen);
refs/reftable-backend.c
+1 -1
@@ -356,7 +356,7 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
356 */
357 if (!starts_with(iter->ref.refname, "refs/") &&
358 !(iter->flags & DO_FOR_EACH_INCLUDE_ROOT_REFS &&
359 - (is_root_ref(&iter->refs->base, iter->ref.refname) ||
359 + (is_root_ref(iter->ref.refname) ||
360 is_headref(&iter->refs->base, iter->ref.refname)))) {
361 continue;
362 }
t/t6302-for-each-ref-filter.sh
+17
@@ -62,6 +62,23 @@ test_expect_success '--include-root-refs with other patterns' '
62 test_cmp expect actual
63 '
64
65 +test_expect_success '--include-root-refs omits dangling symrefs' '
66 + test_when_finished "rm -rf repo" &&
67 + git init repo &&
68 + (
69 + cd repo &&
70 + test_commit initial &&
71 + git symbolic-ref DANGLING_HEAD refs/heads/missing &&
72 + cat >expect <<-EOF &&
73 + HEAD
74 + $(git symbolic-ref HEAD)
75 + refs/tags/initial
76 + EOF
77 + git for-each-ref --format="%(refname)" --include-root-refs >actual &&
78 + test_cmp expect actual
79 + )
80 +'
81 +
82 test_expect_success 'filtering with --points-at' '
83 cat >expect <<-\EOF &&
84 refs/heads/main