From 8f61321ccbdea20945a73446f24a6c380cb2c53b Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 14 Dec 2023 14:36:58 +0100 Subject: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb We read both the HEAD and ORIG_HEAD references directly from the filesystem in order to figure out whether we're currently splitting a commit. If both of the following are true: - HEAD points to the same object as "rebase-merge/amend". - ORIG_HEAD points to the same object as "rebase-merge/orig-head". Then we are currently splitting commits. The current code only works by chance because we only have a single reference backend implementation. Refactor it to instead read both refs via the refdb layer so that we'll also be compatible with alternate reference backends. There are some subtleties involved here: - We pass `RESOLVE_REF_READING` so that a missing ref will cause `read_ref_full()` to return an error. - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve symrefs. The old code didn't resolve symrefs either, and we only ever write object IDs into the refs in "rebase-merge/". - In the same spirit we verify that successfully-read refs are not symbolic refs. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- wt-status.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/wt-status.c b/wt-status.c index 9f45bf69490e6f..da19923981966d 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1295,26 +1295,32 @@ static char *read_line_from_git_path(const char *filename) static int split_commit_in_progress(struct wt_status *s) { int split_in_progress = 0; - char *head, *orig_head, *rebase_amend, *rebase_orig_head; + struct object_id head_oid, orig_head_oid; + char *rebase_amend, *rebase_orig_head; + int head_flags, orig_head_flags; if ((!s->amend && !s->nowarn && !s->workdir_dirty) || !s->branch || strcmp(s->branch, "HEAD")) return 0; - head = read_line_from_git_path("HEAD"); - orig_head = read_line_from_git_path("ORIG_HEAD"); + if (read_ref_full("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + &head_oid, &head_flags) || + read_ref_full("ORIG_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + &orig_head_oid, &orig_head_flags)) + return 0; + if (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF) + return 0; + rebase_amend = read_line_from_git_path("rebase-merge/amend"); rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head"); - if (!head || !orig_head || !rebase_amend || !rebase_orig_head) + if (!rebase_amend || !rebase_orig_head) ; /* fall through, no split in progress */ else if (!strcmp(rebase_amend, rebase_orig_head)) - split_in_progress = !!strcmp(head, rebase_amend); - else if (strcmp(orig_head, rebase_orig_head)) + split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend); + else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head)) split_in_progress = 1; - free(head); - free(orig_head); free(rebase_amend); free(rebase_orig_head); From 668cdc043fe6f6d1fa2bf2b3f3c2375a20819e77 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 14 Dec 2023 14:37:02 +0100 Subject: [PATCH 2/4] refs: propagate errno when reading special refs fails Some refs in Git are more special than others due to reasons explained in the next commit. These refs are read via `refs_read_special_head()`, but this function doesn't behave the same as when we try to read a normal ref. Most importantly, we do not propagate `failure_errno` in the case where the reference does not exist, which is behaviour that we rely on in many parts of Git. Fix this bug by propagating errno when `strbuf_read_file()` fails. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 4 +++- t/t1403-show-ref.sh | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index fcae5dddc60506..00e72a2abfe6c7 100644 --- a/refs.c +++ b/refs.c @@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store, int result = -1; strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname); - if (strbuf_read_file(&content, full_path.buf, 0) < 0) + if (strbuf_read_file(&content, full_path.buf, 0) < 0) { + *failure_errno = errno; goto done; + } result = parse_loose_ref_contents(content.buf, oid, referent, type, failure_errno); diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index b50ae6fcf115ab..66e6e77fa9e711 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' ' test_cmp expect err ' +test_expect_success '--exists with non-existent special ref' ' + test_expect_code 2 git show-ref --exists FETCH_HEAD +' + +test_expect_success '--exists with existing special ref' ' + test_when_finished "rm .git/FETCH_HEAD" && + git rev-parse HEAD >.git/FETCH_HEAD && + git show-ref --exists FETCH_HEAD +' + test_done From 70c70de616c306cd4bb7c70426e394d08f929dff Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 14 Dec 2023 14:37:06 +0100 Subject: [PATCH 3/4] refs: complete list of special refs We have some references that are more special than others. The reason for them being special is that they either do not follow the usual format of references, or that they are written to the filesystem directly by the respective owning subsystem and thus circumvent the reference backend. This works perfectly fine right now because the reffiles backend will know how to read those refs just fine. But with the prospect of gaining a new reference backend implementation we need to be a lot more careful here: - We need to make sure that we are consistent about how those refs are written. They must either always be written via the filesystem, or they must always be written via the reference backend. Any mixture will lead to inconsistent state. - We need to make sure that such special refs are always handled specially when reading them. We're already mostly good with regard to the first item, except for `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit. But the current list of special refs is missing some refs that really should be treated specially. Right now, we only treat `FETCH_HEAD` and `MERGE_HEAD` specially here. Introduce a new function `is_special_ref()` that contains all current instances of special refs to fix the reading path. Note that this is only a temporary measure where we record and rectify the current state. Ideally, the list of special refs should in the end only contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may reference multiple objects and can contain annotations, so they indeed are special. Based-on-patch-by: Han-Wen Nienhuys Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 00e72a2abfe6c7..8fe34d51e4a6d4 100644 --- a/refs.c +++ b/refs.c @@ -1820,15 +1820,65 @@ static int refs_read_special_head(struct ref_store *ref_store, return result; } +static int is_special_ref(const char *refname) +{ + /* + * Special references get written and read directly via the filesystem + * by the subsystems that create them. Thus, they must not go through + * the reference backend but must instead be read directly. It is + * arguable whether this behaviour is sensible, or whether it's simply + * a leaky abstraction enabled by us only having a single reference + * backend implementation. But at least for a subset of references it + * indeed does make sense to treat them specially: + * + * - FETCH_HEAD may contain multiple object IDs, and each one of them + * carries additional metadata like where it came from. + * + * - MERGE_HEAD may contain multiple object IDs when merging multiple + * heads. + * + * There are some exceptions that you might expect to see on this list + * but which are handled exclusively via the reference backend: + * + * - CHERRY_PICK_HEAD + * + * - HEAD + * + * - ORIG_HEAD + * + * - "rebase-apply/" and "rebase-merge/" contain all of the state for + * rebases, including some reference-like files. These are + * exclusively read and written via the filesystem and never go + * through the refdb. + * + * Writing or deleting references must consistently go either through + * the filesystem (special refs) or through the reference backend + * (normal ones). + */ + static const char * const special_refs[] = { + "AUTO_MERGE", + "BISECT_EXPECTED_REV", + "FETCH_HEAD", + "MERGE_AUTOSTASH", + "MERGE_HEAD", + }; + size_t i; + + for (i = 0; i < ARRAY_SIZE(special_refs); i++) + if (!strcmp(refname, special_refs[i])) + return 1; + + return 0; +} + int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, struct object_id *oid, struct strbuf *referent, unsigned int *type, int *failure_errno) { assert(failure_errno); - if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) { + if (is_special_ref(refname)) return refs_read_special_head(ref_store, refname, oid, referent, type, failure_errno); - } return ref_store->be->read_raw_ref(ref_store, refname, oid, referent, type, failure_errno); From 0a06892ddde5bc3a82a4fe2963e3ea294252ffdd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 14 Dec 2023 14:37:13 +0100 Subject: [PATCH 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem and via the refdb, which violates the newly established rules for how special refs must be treated. This works alright in practice with the reffiles reference backend, but will cause bugs once we gain additional backends. Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb so that it is no longer a special ref. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 25 ++++--------------------- builtin/bisect.c | 8 ++------ refs.c | 3 ++- t/t6030-bisect-porcelain.sh | 2 +- 4 files changed, 9 insertions(+), 29 deletions(-) diff --git a/bisect.c b/bisect.c index 1be8e0a2711df9..985b96ed132516 100644 --- a/bisect.c +++ b/bisect.c @@ -471,7 +471,6 @@ static int read_bisect_refs(void) } static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES") -static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") @@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried, static int is_expected_rev(const struct object_id *oid) { - const char *filename = git_path_bisect_expected_rev(); - struct stat st; - struct strbuf str = STRBUF_INIT; - FILE *fp; - int res = 0; - - if (stat(filename, &st) || !S_ISREG(st.st_mode)) + struct object_id expected_oid; + if (read_ref("BISECT_EXPECTED_REV", &expected_oid)) return 0; - - fp = fopen_or_warn(filename, "r"); - if (!fp) - return 0; - - if (strbuf_getline_lf(&str, fp) != EOF) - res = !strcmp(str.buf, oid_to_hex(oid)); - - strbuf_release(&str); - fclose(fp); - - return res; + return oideq(oid, &expected_oid); } enum bisect_error bisect_checkout(const struct object_id *bisect_rev, @@ -1185,10 +1168,10 @@ int bisect_clean_state(void) struct string_list refs_for_removal = STRING_LIST_INIT_NODUP; for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal); string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD")); + string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV")); result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF); refs_for_removal.strdup_strings = 1; string_list_clear(&refs_for_removal, 0); - unlink_or_warn(git_path_bisect_expected_rev()); unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_log()); unlink_or_warn(git_path_bisect_names()); diff --git a/builtin/bisect.c b/builtin/bisect.c index 35938b05fd1c0b..4e2c43daf57cd7 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -17,7 +17,6 @@ #include "revision.h" static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") -static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK") static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START") static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG") @@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc, const char *state; int i, verify_expected = 1; struct object_id oid, expected; - struct strbuf buf = STRBUF_INIT; struct oid_array revs = OID_ARRAY_INIT; if (!argc) @@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc, oid_array_append(&revs, &commit->object.oid); } - if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz || - get_oid_hex(buf.buf, &expected) < 0) + if (read_ref("BISECT_EXPECTED_REV", &expected)) verify_expected = 0; /* Ignore invalid file contents */ - strbuf_release(&buf); for (i = 0; i < revs.nr; i++) { if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) { @@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc, } if (verify_expected && !oideq(&revs.oid[i], &expected)) { unlink_or_warn(git_path_bisect_ancestors_ok()); - unlink_or_warn(git_path_bisect_expected_rev()); + delete_ref(NULL, "BISECT_EXPECTED_REV", NULL, REF_NO_DEREF); verify_expected = 0; } } diff --git a/refs.c b/refs.c index 8fe34d51e4a6d4..c76ce86bef7b12 100644 --- a/refs.c +++ b/refs.c @@ -1840,6 +1840,8 @@ static int is_special_ref(const char *refname) * There are some exceptions that you might expect to see on this list * but which are handled exclusively via the reference backend: * + * - BISECT_EXPECTED_REV + * * - CHERRY_PICK_HEAD * * - HEAD @@ -1857,7 +1859,6 @@ static int is_special_ref(const char *refname) */ static const char * const special_refs[] = { "AUTO_MERGE", - "BISECT_EXPECTED_REV", "FETCH_HEAD", "MERGE_AUTOSTASH", "MERGE_HEAD", diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index 2a5b7d8379c2a9..792c1504bcc7bd 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' ' git bisect bad $HASH4 && git bisect reset && test -z "$(git for-each-ref "refs/bisect/*")" && - test_path_is_missing ".git/BISECT_EXPECTED_REV" && + test_ref_missing BISECT_EXPECTED_REV && test_path_is_missing ".git/BISECT_ANCESTORS_OK" && test_path_is_missing ".git/BISECT_LOG" && test_path_is_missing ".git/BISECT_RUN" &&