From f6936e62a50e8bc9e268f3396618d33e88f4df7e Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:50:42 +0200 Subject: refs: rename `is_pseudoref()` to `is_root_ref()` Rename `is_pseudoref()` to `is_root_ref()` to adapt to the newly defined terminology in our gitglossary(7). Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'refs/files-backend.c') diff --git a/refs/files-backend.c b/refs/files-backend.c index a098d14ea0..0fcb601444 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -351,7 +351,7 @@ static void add_pseudoref_and_head_entries(struct ref_store *ref_store, strbuf_addstr(&refname, de->d_name); dtype = get_dtype(de, &path, 1); - if (dtype == DT_REG && (is_pseudoref(ref_store, de->d_name) || + if (dtype == DT_REG && (is_root_ref(ref_store, de->d_name) || is_headref(ref_store, de->d_name))) loose_fill_ref_dir_regular_file(refs, refname.buf, dir); -- cgit v1.2.3 From afcd067dad27fa7eddde01ebde90daa6cc541cc5 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:50:51 +0200 Subject: 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 Signed-off-by: Junio C Hamano --- ref-filter.c | 2 +- refs.c | 19 +++++-------------- refs.h | 5 +++-- refs/files-backend.c | 4 ++-- refs/reftable-backend.c | 2 +- t/t6302-for-each-ref-filter.sh | 17 +++++++++++++++++ 6 files changed, 29 insertions(+), 20 deletions(-) (limited to 'refs/files-backend.c') diff --git a/ref-filter.c b/ref-filter.c index 361beb6619..23e81e3e04 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2756,7 +2756,7 @@ static int ref_kind_from_refname(const char *refname) return ref_kind[i].kind; } - if (is_root_ref(get_main_ref_store(the_repository), refname)) + if (is_root_ref(refname)) return FILTER_REFS_PSEUDOREFS; return FILTER_REFS_OTHERS; diff --git a/refs.c b/refs.c index c1c406fc5f..4fec29e660 100644 --- a/refs.c +++ b/refs.c @@ -856,7 +856,7 @@ static int is_root_ref_syntax(const char *refname) return 1; } -int is_root_ref(struct ref_store *refs, const char *refname) +int is_root_ref(const char *refname) { static const char *const irregular_root_refs[] = { "AUTO_MERGE", @@ -865,26 +865,17 @@ int is_root_ref(struct ref_store *refs, const char *refname) "NOTES_MERGE_REF", "MERGE_AUTOSTASH", }; - struct object_id oid; size_t i; if (!is_root_ref_syntax(refname)) return 0; - if (ends_with(refname, "_HEAD")) { - refs_resolve_ref_unsafe(refs, refname, - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL); - return !is_null_oid(&oid); - } + if (ends_with(refname, "_HEAD")) + return 1; for (i = 0; i < ARRAY_SIZE(irregular_root_refs); i++) - if (!strcmp(refname, irregular_root_refs[i])) { - refs_resolve_ref_unsafe(refs, refname, - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL); - return !is_null_oid(&oid); - } + if (!strcmp(refname, irregular_root_refs[i])) + return 1; return 0; } diff --git a/refs.h b/refs.h index d0374c3275..8a574a22c7 100644 --- a/refs.h +++ b/refs.h @@ -1052,7 +1052,8 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT]; void update_ref_namespace(enum ref_namespace namespace, char *ref); /* - * Check whether the reference is an existing root reference. + * Check whether the provided name names a root reference. This function only + * performs a syntactic check. * * A root ref is a reference that lives in the root of the reference hierarchy. * These references must conform to special syntax: @@ -1076,7 +1077,7 @@ void update_ref_namespace(enum ref_namespace namespace, char *ref); * * - MERGE_AUTOSTASH */ -int is_root_ref(struct ref_store *refs, const char *refname); +int is_root_ref(const char *refname); int is_headref(struct ref_store *refs, const char *refname); diff --git a/refs/files-backend.c b/refs/files-backend.c index 0fcb601444..06240ce327 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -351,8 +351,8 @@ static void add_pseudoref_and_head_entries(struct ref_store *ref_store, strbuf_addstr(&refname, de->d_name); dtype = get_dtype(de, &path, 1); - if (dtype == DT_REG && (is_root_ref(ref_store, de->d_name) || - is_headref(ref_store, de->d_name))) + if (dtype == DT_REG && (is_root_ref(de->d_name) || + is_headref(ref_store, de->d_name))) loose_fill_ref_dir_regular_file(refs, refname.buf, dir); strbuf_setlen(&refname, dirnamelen); diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 5a5e64fe69..c11ba95736 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -356,7 +356,7 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) */ if (!starts_with(iter->ref.refname, "refs/") && !(iter->flags & DO_FOR_EACH_INCLUDE_ROOT_REFS && - (is_root_ref(&iter->refs->base, iter->ref.refname) || + (is_root_ref(iter->ref.refname) || is_headref(&iter->refs->base, iter->ref.refname)))) { continue; } diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 948f1bb5f4..92ed8957c8 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -62,6 +62,23 @@ test_expect_success '--include-root-refs with other patterns' ' test_cmp expect actual ' +test_expect_success '--include-root-refs omits dangling symrefs' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit initial && + git symbolic-ref DANGLING_HEAD refs/heads/missing && + cat >expect <<-EOF && + HEAD + $(git symbolic-ref HEAD) + refs/tags/initial + EOF + git for-each-ref --format="%(refname)" --include-root-refs >actual && + test_cmp expect actual + ) +' + test_expect_success 'filtering with --points-at' ' cat >expect <<-\EOF && refs/heads/main -- cgit v1.2.3 From 31951c22489dc4238ef881478a896a460531b722 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Wed, 15 May 2024 08:50:56 +0200 Subject: refs: classify HEAD as a root ref Root refs are those refs that live in the root of the ref hierarchy. Our old and venerable "HEAD" reference falls into this category, but we don't yet classify it as such in `is_root_ref()`. Adapt the function to also treat "HEAD" as a root ref. This change is safe to do for all current callers: - `ref_kind_from_refname()` already handles "HEAD" explicitly before calling `is_root_ref()`. - The "files" and "reftable" backends explicitly call both `is_root_ref()` and `is_headref()` together. This also aligns behaviour or `is_root_ref()` and `is_headref()` such that we stop checking for ref existence. This changes semantics for our backends: - In the reftable backend we already know that the ref must exist because `is_headref()` is called as part of the ref iterator. The existence check is thus redundant, and the change is safe to do. - In the files backend we use it when populating root refs, where we would skip adding the "HEAD" file if it was not possible to resolve it. The new behaviour is to instead mark "HEAD" as broken, which will cause us to emit warnings in various places. As there are no callers of `is_headref()` left afer the refactoring, we can absorb it completely into `is_root_ref()`. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 9 +-------- refs.h | 5 ++--- refs/files-backend.c | 3 +-- refs/reftable-backend.c | 3 +-- 4 files changed, 5 insertions(+), 15 deletions(-) (limited to 'refs/files-backend.c') diff --git a/refs.c b/refs.c index 4fec29e660..9fb1061d52 100644 --- a/refs.c +++ b/refs.c @@ -859,6 +859,7 @@ static int is_root_ref_syntax(const char *refname) int is_root_ref(const char *refname) { static const char *const irregular_root_refs[] = { + "HEAD", "AUTO_MERGE", "BISECT_EXPECTED_REV", "NOTES_MERGE_PARTIAL", @@ -880,14 +881,6 @@ int is_root_ref(const char *refname) return 0; } -int is_headref(struct ref_store *refs, const char *refname) -{ - if (!strcmp(refname, "HEAD")) - return refs_ref_exists(refs, refname); - - return 0; -} - static int is_current_worktree_ref(const char *ref) { return is_root_ref_syntax(ref) || is_per_worktree_ref(ref); } diff --git a/refs.h b/refs.h index 8a574a22c7..8489b45265 100644 --- a/refs.h +++ b/refs.h @@ -1060,7 +1060,8 @@ void update_ref_namespace(enum ref_namespace namespace, char *ref); * * - Their name must be all-uppercase or underscores ("_"). * - * - Their name must end with "_HEAD". + * - Their name must end with "_HEAD". As a special rule, "HEAD" is a root + * ref, as well. * * - Their name may not contain a slash. * @@ -1079,6 +1080,4 @@ void update_ref_namespace(enum ref_namespace namespace, char *ref); */ int is_root_ref(const char *refname); -int is_headref(struct ref_store *refs, const char *refname); - #endif /* REFS_H */ diff --git a/refs/files-backend.c b/refs/files-backend.c index 06240ce327..6f9a631592 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -351,8 +351,7 @@ static void add_pseudoref_and_head_entries(struct ref_store *ref_store, strbuf_addstr(&refname, de->d_name); dtype = get_dtype(de, &path, 1); - if (dtype == DT_REG && (is_root_ref(de->d_name) || - is_headref(ref_store, de->d_name))) + if (dtype == DT_REG && is_root_ref(de->d_name)) loose_fill_ref_dir_regular_file(refs, refname.buf, dir); strbuf_setlen(&refname, dirnamelen); diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index c11ba95736..b6668e7c7e 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -356,8 +356,7 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator) */ if (!starts_with(iter->ref.refname, "refs/") && !(iter->flags & DO_FOR_EACH_INCLUDE_ROOT_REFS && - (is_root_ref(iter->ref.refname) || - is_headref(&iter->refs->base, iter->ref.refname)))) { + is_root_ref(iter->ref.refname))) { continue; } -- cgit v1.2.3