{"thread":{"id":"60568","subject":"[PATCH 0/4] refs: improve handling of special refs","startedAt":"2023-11-29T08:14:13Z","lastAt":"2023-12-21T10:08:34Z","messageCount":41,"participants":["Patrick Steinhardt","Taylor Blau","Phillip Wood","Junio C Hamano","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"485237","messageId":"cover.1701243201.git.ps@pks.im","threadId":"60568","inReplyTo":null,"subject":"[PATCH 0/4] refs: improve handling of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-29T08:14:07Z","receivedAt":"2023-11-29T08:14:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthere are a bunch of \"special\" refs in Git that sometimes behave like a\nnormal reference and sometimes they don't. These references are written\nto directly via the filesystem without going through the reference\nbackend, but the expectation is that those references can then be read\nby things like git-rev-parse(1).\n\nWe do not currently have a single source of truth for what those special\nrefs are, and we also don't have clear rules for how they should be\nwritten to. This works in the context of the files backend, because it\nis able to read back such manually-written loose references just fine.\nBut once the reftable backend lands this will stop working.\n\nThis patch series tries to improve this state by doing two things:\n\n    1. We explicitly mark these references as special by introducing a\n       new `is_special_ref()` function. This serves as documentation,\n       but will also cause us to explicitly read all of these special\n       refs via loose files regardless of the actual backend.\n\n    2. We document a new rule around writing refs. Namely, normal\n       references are _always_ written via the reference backend,\n       whereas special references are _always_ written directly via the\n       filesystem. This rule is not enforced anywhere, but at least it's\n       now made more explicit.\n\nThe last patch fixes one of the instances where we treat a reference\ninconsistently by converting it to a normal reference. We can eventually\nmigrate more of the special refs to become normal refs as we deem fit,\nbut I consider this to be out of scope for this patch series.\n\nThese patches improve compatibility with the new reftable backend.\n\nPatrick\n\nPatrick Steinhardt (4):\n  wt-status: read HEAD and ORIG_HEAD via the refdb\n  refs: propagate errno when reading special refs fails\n  refs: complete list of special refs\n  bisect: consistently write BISECT_EXPECTED_REV via the refdb\n\n bisect.c                    | 25 +++------------\n builtin/bisect.c            |  8 ++---\n refs.c                      | 64 +++++++++++++++++++++++++++++++++++--\n t/t1403-show-ref.sh         |  9 ++++++\n t/t6030-bisect-porcelain.sh |  2 +-\n wt-status.c                 | 17 +++++-----\n 6 files changed, 86 insertions(+), 39 deletions(-)\n\n-- \n2.43.0\n\n"},{"id":"485238","messageId":"35b74eb972eed7e08190e826fabcf6b7a241f285.1701243201.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"[PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-29T08:14:12Z","receivedAt":"2023-11-29T08:14:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We read both the HEAD and ORIG_HEAD references directly from the\nfilesystem in order to figure out whether we're currently splitting a\ncommit. If both of the following are true:\n\n  - HEAD points to the same object as \"rebase-merge/amend\".\n\n  - ORIG_HEAD points to the same object as \"rebase-merge/orig-head\".\n\nThen we are currently splitting commits.\n\nThe current code only works by chance because we only have a single\nreference backend implementation. Refactor it to instead read both refs\nvia the refdb layer so that we'll also be compatible with alternate\nreference backends.\n\nNote that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is\nbecause we didn't resolve symrefs before either, and in practice none of\nthe refs in \"rebase-merge/\" would be symbolic. We thus don't want to\nresolve symrefs with the new code either to retain the old behaviour.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n wt-status.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 9f45bf6949..fe9e590b80 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)\n static int split_commit_in_progress(struct wt_status *s)\n {\n \tint split_in_progress = 0;\n-\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n+\tstruct object_id head_oid, orig_head_oid;\n+\tchar *rebase_amend, *rebase_orig_head;\n \n \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n \t\treturn 0;\n \n-\thead = read_line_from_git_path(\"HEAD\");\n-\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n+\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n+\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n+\t\treturn 0;\n+\n \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n \trebase_orig_head = read_line_from_git_path(\"rebase-merge/orig-head\");\n \n-\tif (!head || !orig_head || !rebase_amend || !rebase_orig_head)\n+\tif (!rebase_amend || !rebase_orig_head)\n \t\t; /* fall through, no split in progress */\n \telse if (!strcmp(rebase_amend, rebase_orig_head))\n-\t\tsplit_in_progress = !!strcmp(head, rebase_amend);\n-\telse if (strcmp(orig_head, rebase_orig_head))\n+\t\tsplit_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);\n+\telse if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))\n \t\tsplit_in_progress = 1;\n \n-\tfree(head);\n-\tfree(orig_head);\n \tfree(rebase_amend);\n \tfree(rebase_orig_head);\n \n-- \n2.43.0\n\n"},{"id":"485239","messageId":"691552a17ec587b0c03e758437c33d58767803aa.1701243201.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"[PATCH 2/4] refs: propagate errno when reading special refs fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-29T08:14:16Z","receivedAt":"2023-11-29T08:14:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some refs in Git are more special than others due to reasons explained\nin the next commit. These refs are read via `refs_read_special_head()`,\nbut this function doesn't behave the same as when we try to read a\nnormal ref. Most importantly, we do not propagate `failure_errno` in the\ncase where the reference does not exist, which is behaviour that we rely\non in many parts of Git.\n\nFix this bug by propagating errno when `strbuf_read_file()` fails.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c              | 6 +++++-\n t/t1403-show-ref.sh | 9 +++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex fcae5dddc6..7d4a057f36 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \tint result = -1;\n \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n \n-\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n+\terrno = 0;\n+\n+\tif (strbuf_read_file(&content, full_path.buf, 0) < 0) {\n+\t\t*failure_errno = errno;\n \t\tgoto done;\n+\t}\n \n \tresult = parse_loose_ref_contents(content.buf, oid, referent, type,\n \t\t\t\t\t  failure_errno);\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex b50ae6fcf1..37af154d27 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -266,4 +266,13 @@ test_expect_success '--exists with directory fails with generic error' '\n \ttest_cmp expect err\n '\n \n+test_expect_success '--exists with non-existent special ref' '\n+\ttest_expect_code 2 git show-ref --exists FETCH_HEAD\n+'\n+\n+test_expect_success '--exists with existing special ref' '\n+\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n+\tgit show-ref --exists FETCH_HEAD\n+'\n+\n test_done\n-- \n2.43.0\n\n"},{"id":"485240","messageId":"0e38103114a206bedbbbd7ea97cb77fa05fd3c29.1701243201.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"[PATCH 3/4] refs: complete list of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-29T08:14:20Z","receivedAt":"2023-11-29T08:14:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have some references that are more special than others. The reason\nfor them being special is that they either do not follow the usual\nformat of references, or that they are written to the filesystem\ndirectly by the respective owning subsystem and thus circumvent the\nreference backend.\n\nThis works perfectly fine right now because the reffiles backend will\nknow how to read those refs just fine. But with the prospect of gaining\na new reference backend implementation we need to be a lot more careful\nhere:\n\n  - We need to make sure that we are consistent about how those refs are\n    written. They must either always be written via the filesystem, or\n    they must always be written via the reference backend. Any mixture\n    will lead to inconsistent state.\n\n  - We need to make sure that such special refs are always handled\n    specially when reading them.\n\nWe're already mostly good with regard to the first item, except for\n`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.\nBut the current list of special refs is missing a lot of refs that\nreally should be treated specially. Right now, we only treat\n`FETCH_HEAD` and `MERGE_HEAD` specially here.\n\nIntroduce a new function `is_special_ref()` that contains all current\ninstances of special refs to fix the reading path.\n\nBased-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 56 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7d4a057f36..2d39d3fe80 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \treturn result;\n }\n \n+static int is_special_ref(const char *refname)\n+{\n+\t/*\n+\t * Special references get written and read directly via the filesystem\n+\t * by the subsystems that create them. Thus, they must not go through\n+\t * the reference backend but must instead be read directly. It is\n+\t * arguable whether this behaviour is sensible, or whether it's simply\n+\t * a leaky abstraction enabled by us only having a single reference\n+\t * backend implementation. But at least for a subset of references it\n+\t * indeed does make sense to treat them specially:\n+\t *\n+\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n+\t *   carries additional metadata like where it came from.\n+\t *\n+\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n+\t *   heads.\n+\t *\n+\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n+\t *   rebases, where keeping it closely together feels sensible.\n+\t *\n+\t * There are some exceptions that you might expect to see on this list\n+\t * but which are handled exclusively via the reference backend:\n+\t *\n+\t * - CHERRY_PICK_HEAD\n+\t * - HEAD\n+\t * - ORIG_HEAD\n+\t *\n+\t * Writing or deleting references must consistently go either through\n+\t * the filesystem (special refs) or through the reference backend\n+\t * (normal ones).\n+\t */\n+\tconst char * const special_refs[] = {\n+\t\t\"AUTO_MERGE\",\n+\t\t\"BISECT_EXPECTED_REV\",\n+\t\t\"FETCH_HEAD\",\n+\t\t\"MERGE_AUTOSTASH\",\n+\t\t\"MERGE_HEAD\",\n+\t};\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(special_refs); i++)\n+\t\tif (!strcmp(refname, special_refs[i]))\n+\t\t\treturn 1;\n+\n+\t/*\n+\t * git-rebase(1) stores its state in `rebase-apply/` or\n+\t * `rebase-merge/`, including various reference-like bits.\n+\t */\n+\tif (starts_with(refname, \"rebase-apply/\") ||\n+\t    starts_with(refname, \"rebase-merge/\"))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t      struct object_id *oid, struct strbuf *referent,\n \t\t      unsigned int *type, int *failure_errno)\n {\n \tassert(failure_errno);\n-\tif (!strcmp(refname, \"FETCH_HEAD\") || !strcmp(refname, \"MERGE_HEAD\")) {\n+\tif (is_special_ref(refname))\n \t\treturn refs_read_special_head(ref_store, refname, oid, referent,\n \t\t\t\t\t      type, failure_errno);\n-\t}\n \n \treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\n \t\t\t\t\t   type, failure_errno);\n-- \n2.43.0\n\n"},{"id":"485241","messageId":"c7ab26fb7e9e24b12b83dc26fbc17ff4d96e206c.1701243201.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"[PATCH 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-29T08:14:24Z","receivedAt":"2023-11-29T08:14:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem\nand via the refdb, which violates the newly established rules for how\nspecial refs must be treated. This works alright in practice with the\nreffiles reference backend, but will cause bugs once we gain additional\nbackends.\n\nFix this issue and consistently write BISECT_EXPECTED_REV via the refdb\nso that it is no longer a special ref.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bisect.c                    | 25 ++++---------------------\n builtin/bisect.c            |  8 ++------\n refs.c                      |  2 +-\n t/t6030-bisect-porcelain.sh |  2 +-\n 4 files changed, 8 insertions(+), 29 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 1be8e0a271..985b96ed13 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -471,7 +471,6 @@ static int read_bisect_refs(void)\n }\n \n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n-static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,\n \n static int is_expected_rev(const struct object_id *oid)\n {\n-\tconst char *filename = git_path_bisect_expected_rev();\n-\tstruct stat st;\n-\tstruct strbuf str = STRBUF_INIT;\n-\tFILE *fp;\n-\tint res = 0;\n-\n-\tif (stat(filename, &st) || !S_ISREG(st.st_mode))\n+\tstruct object_id expected_oid;\n+\tif (read_ref(\"BISECT_EXPECTED_REV\", &expected_oid))\n \t\treturn 0;\n-\n-\tfp = fopen_or_warn(filename, \"r\");\n-\tif (!fp)\n-\t\treturn 0;\n-\n-\tif (strbuf_getline_lf(&str, fp) != EOF)\n-\t\tres = !strcmp(str.buf, oid_to_hex(oid));\n-\n-\tstrbuf_release(&str);\n-\tfclose(fp);\n-\n-\treturn res;\n+\treturn oideq(oid, &expected_oid);\n }\n \n enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)\n \tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n \tfor_each_ref_in(\"refs/bisect\", mark_for_removal, (void *) &refs_for_removal);\n \tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n+\tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_EXPECTED_REV\"));\n \tresult = delete_refs(\"bisect: remove\", &refs_for_removal, REF_NO_DEREF);\n \trefs_for_removal.strdup_strings = 1;\n \tstring_list_clear(&refs_for_removal, 0);\n-\tunlink_or_warn(git_path_bisect_expected_rev());\n \tunlink_or_warn(git_path_bisect_ancestors_ok());\n \tunlink_or_warn(git_path_bisect_log());\n \tunlink_or_warn(git_path_bisect_names());\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 35938b05fd..4e2c43daf5 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -17,7 +17,6 @@\n #include \"revision.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n-static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \tconst char *state;\n \tint i, verify_expected = 1;\n \tstruct object_id oid, expected;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct oid_array revs = OID_ARRAY_INIT;\n \n \tif (!argc)\n@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \t\toid_array_append(&revs, &commit->object.oid);\n \t}\n \n-\tif (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n-\t    get_oid_hex(buf.buf, &expected) < 0)\n+\tif (read_ref(\"BISECT_EXPECTED_REV\", &expected))\n \t\tverify_expected = 0; /* Ignore invalid file contents */\n-\tstrbuf_release(&buf);\n \n \tfor (i = 0; i < revs.nr; i++) {\n \t\tif (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {\n@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \t\t}\n \t\tif (verify_expected && !oideq(&revs.oid[i], &expected)) {\n \t\t\tunlink_or_warn(git_path_bisect_ancestors_ok());\n-\t\t\tunlink_or_warn(git_path_bisect_expected_rev());\n+\t\t\tdelete_ref(NULL, \"BISECT_EXPECTED_REV\", NULL, REF_NO_DEREF);\n \t\t\tverify_expected = 0;\n \t\t}\n \t}\ndiff --git a/refs.c b/refs.c\nindex 2d39d3fe80..0290bb0c67 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1845,6 +1845,7 @@ static int is_special_ref(const char *refname)\n \t * There are some exceptions that you might expect to see on this list\n \t * but which are handled exclusively via the reference backend:\n \t *\n+\t * - BISECT_EXPECTED_REV\n \t * - CHERRY_PICK_HEAD\n \t * - HEAD\n \t * - ORIG_HEAD\n@@ -1855,7 +1856,6 @@ static int is_special_ref(const char *refname)\n \t */\n \tconst char * const special_refs[] = {\n \t\t\"AUTO_MERGE\",\n-\t\t\"BISECT_EXPECTED_REV\",\n \t\t\"FETCH_HEAD\",\n \t\t\"MERGE_AUTOSTASH\",\n \t\t\"MERGE_HEAD\",\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 2a5b7d8379..792c1504bc 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n \tgit bisect bad $HASH4 &&\n \tgit bisect reset &&\n \ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n-\ttest_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n+\ttest_ref_missing BISECT_EXPECTED_REV &&\n \ttest_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n \ttest_path_is_missing \".git/BISECT_LOG\" &&\n \ttest_path_is_missing \".git/BISECT_RUN\" &&\n-- \n2.43.0\n\n"},{"id":"485245","messageId":"ZWew3CP4QJ4XDnHj@nand.local","threadId":"60568","inReplyTo":"35b74eb972eed7e08190e826fabcf6b7a241f285.1701243201.git.ps@pks.im","subject":"Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-29T21:45:00Z","receivedAt":"2023-11-29T21:45:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:\n> We read both the HEAD and ORIG_HEAD references directly from the\n> filesystem in order to figure out whether we're currently splitting a\n> commit. If both of the following are true:\n>\n>   - HEAD points to the same object as \"rebase-merge/amend\".\n>\n>   - ORIG_HEAD points to the same object as \"rebase-merge/orig-head\".\n>\n> Then we are currently splitting commits.\n>\n> The current code only works by chance because we only have a single\n> reference backend implementation. Refactor it to instead read both refs\n> via the refdb layer so that we'll also be compatible with alternate\n> reference backends.\n>\n> Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is\n> because we didn't resolve symrefs before either, and in practice none of\n> the refs in \"rebase-merge/\" would be symbolic. We thus don't want to\n> resolve symrefs with the new code either to retain the old behaviour.\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  wt-status.c | 17 +++++++++--------\n>  1 file changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index 9f45bf6949..fe9e590b80 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)\n>  static int split_commit_in_progress(struct wt_status *s)\n>  {\n>  \tint split_in_progress = 0;\n> -\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n> +\tstruct object_id head_oid, orig_head_oid;\n> +\tchar *rebase_amend, *rebase_orig_head;\n>\n>  \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n>  \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n>  \t\treturn 0;\n>\n> -\thead = read_line_from_git_path(\"HEAD\");\n> -\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n> +\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n> +\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n\nSwitching to read_ref_full() here is going to have some slightly\ndifferent behavior than just reading out the contents of\n\"$GIT_DIR/HEAD\", but I think that it should be OK.\n\nBefore we would not have complained, if, for example, the contents of\n\"$GIT_DIR/HEAD\" were malformed, but now we will. I think that's OK,\nespecially given that if that file is bogus, we'll have other problems\nbefore we get here ;-).\n\nAre there any other gotchas that we should be thinking about?\n\n> +\t\treturn 0;\n> +\n>  \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n>  \trebase_orig_head = read_line_from_git_path(\"rebase-merge/orig-head\");\n>\n> -\tif (!head || !orig_head || !rebase_amend || !rebase_orig_head)\n> +\tif (!rebase_amend || !rebase_orig_head)\n>  \t\t; /* fall through, no split in progress */\n>  \telse if (!strcmp(rebase_amend, rebase_orig_head))\n> -\t\tsplit_in_progress = !!strcmp(head, rebase_amend);\n> -\telse if (strcmp(orig_head, rebase_orig_head))\n> +\t\tsplit_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);\n> +\telse if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))\n\nI did a double take at these strcmp(oid_to_hex(...)) calls, but I think\nthat they are the best that we can do given that we're still reading the\ncontents of \"rebase-merge/amend\" and \"rebase-merge/orig-head\" directly.\n\nI suppose we could go the other way and turn their contents into\nobject_ids and then use oidcmp(), but it doesn't seem worth it IMHO.\n\nThanks,\nTaylor\n"},{"id":"485246","messageId":"ZWeyUU/NqmGUvyOL@nand.local","threadId":"60568","inReplyTo":"691552a17ec587b0c03e758437c33d58767803aa.1701243201.git.ps@pks.im","subject":"Re: [PATCH 2/4] refs: propagate errno when reading special refs fails","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-29T21:51:13Z","receivedAt":"2023-11-29T21:51:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:\n> diff --git a/refs.c b/refs.c\n> index fcae5dddc6..7d4a057f36 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,\n>  \tint result = -1;\n>  \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n>\n> -\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n> +\terrno = 0;\n\nDo we need to set errno to 0 here? Looking at the implementation of\nstrbuf_read_file(), it looks like we return early in two cases. Either\nopen() fails, in which errno is set for us, or strbuf_read() fails, in\nwhich case we set errno to whatever it was right after the failed read\n(preventing the subsequent close() call from tainting the value of errno).\n\nSo I think in either case, we have the right value in errno, and don't\nneed to worry about setting it to \"0\" ahead of time.\n\n> +test_expect_success '--exists with existing special ref' '\n> +\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n> +\tgit show-ref --exists FETCH_HEAD\n> +'\n\nI don't think that it matters here, but do we need to worry about\ncleaning up .git/FETCH_HEAD for future tests?\n\nThanks,\nTaylor\n"},{"id":"485247","messageId":"ZWe0RzOoHI9QZMox@nand.local","threadId":"60568","inReplyTo":"0e38103114a206bedbbbd7ea97cb77fa05fd3c29.1701243201.git.ps@pks.im","subject":"Re: [PATCH 3/4] refs: complete list of special refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-29T21:59:35Z","receivedAt":"2023-11-29T21:59:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 29, 2023 at 09:14:20AM +0100, Patrick Steinhardt wrote:\n> We have some references that are more special than others. The reason\n> for them being special is that they either do not follow the usual\n> format of references, or that they are written to the filesystem\n> directly by the respective owning subsystem and thus circumvent the\n> reference backend.\n>\n> This works perfectly fine right now because the reffiles backend will\n> know how to read those refs just fine. But with the prospect of gaining\n> a new reference backend implementation we need to be a lot more careful\n> here:\n>\n>   - We need to make sure that we are consistent about how those refs are\n>     written. They must either always be written via the filesystem, or\n>     they must always be written via the reference backend. Any mixture\n>     will lead to inconsistent state.\n>\n>   - We need to make sure that such special refs are always handled\n>     specially when reading them.\n>\n> We're already mostly good with regard to the first item, except for\n> `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.\n> But the current list of special refs is missing a lot of refs that\n> really should be treated specially. Right now, we only treat\n> `FETCH_HEAD` and `MERGE_HEAD` specially here.\n>\n> Introduce a new function `is_special_ref()` that contains all current\n> instances of special refs to fix the reading path.\n>\n> Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n>  1 file changed, 56 insertions(+), 2 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 7d4a057f36..2d39d3fe80 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,\n>  \treturn result;\n>  }\n>\n> +static int is_special_ref(const char *refname)\n> +{\n> +\t/*\n> +\t * Special references get written and read directly via the filesystem\n> +\t * by the subsystems that create them. Thus, they must not go through\n> +\t * the reference backend but must instead be read directly. It is\n> +\t * arguable whether this behaviour is sensible, or whether it's simply\n> +\t * a leaky abstraction enabled by us only having a single reference\n> +\t * backend implementation. But at least for a subset of references it\n> +\t * indeed does make sense to treat them specially:\n> +\t *\n> +\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n> +\t *   carries additional metadata like where it came from.\n> +\t *\n> +\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n> +\t *   heads.\n> +\t *\n> +\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n> +\t *   rebases, where keeping it closely together feels sensible.\n> +\t *\n> +\t * There are some exceptions that you might expect to see on this list\n> +\t * but which are handled exclusively via the reference backend:\n> +\t *\n> +\t * - CHERRY_PICK_HEAD\n> +\t * - HEAD\n> +\t * - ORIG_HEAD\n> +\t *\n> +\t * Writing or deleting references must consistently go either through\n> +\t * the filesystem (special refs) or through the reference backend\n> +\t * (normal ones).\n> +\t */\n> +\tconst char * const special_refs[] = {\n> +\t\t\"AUTO_MERGE\",\n> +\t\t\"BISECT_EXPECTED_REV\",\n> +\t\t\"FETCH_HEAD\",\n> +\t\t\"MERGE_AUTOSTASH\",\n> +\t\t\"MERGE_HEAD\",\n> +\t};\n\nIs there a reason that we don't want to declare this statically? If we\ndid, I think we could drop one const, since the strings would instead\nreside in the .rodata section.\n\n> +\tint i;\n\nNot that it matters for this case, but it may be worth declaring i to be\nan unsigned type, since it's used as an index into an array. size_t\nseems like an appropriate choice there.\n\n> +\tfor (i = 0; i < ARRAY_SIZE(special_refs); i++)\n> +\t\tif (!strcmp(refname, special_refs[i]))\n> +\t\t\treturn 1;\n> +\n> +\t/*\n> +\t * git-rebase(1) stores its state in `rebase-apply/` or\n> +\t * `rebase-merge/`, including various reference-like bits.\n> +\t */\n> +\tif (starts_with(refname, \"rebase-apply/\") ||\n> +\t    starts_with(refname, \"rebase-merge/\"))\n\nDo we care about case sensitivity here? Definitely not on case-sensitive\nfilesystems, but I'm not sure about case-insensitive ones. For instance,\non macOS, I can do:\n\n    $ git rev-parse hEAd\n\nand get the same value as \"git rev-parse HEAD\" (on my Linux workstation,\nthis fails as expected).\n\nI doubt that there are many users in the wild asking to resolve\nreBASe-APPLY/xyz, but I think that after this patch that would no longer\nwork as-is, so we may want to replace this with istarts_with() instead.\n\nThanks,\nTaylor\n"},{"id":"485248","messageId":"ZWe3hCpTu/OYinWi@nand.local","threadId":"60568","inReplyTo":"c7ab26fb7e9e24b12b83dc26fbc17ff4d96e206c.1701243201.git.ps@pks.im","subject":"Re: [PATCH 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-29T22:13:24Z","receivedAt":"2023-11-29T22:13:35Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 29, 2023 at 09:14:24AM +0100, Patrick Steinhardt wrote:\n> ---\n>  bisect.c                    | 25 ++++---------------------\n>  builtin/bisect.c            |  8 ++------\n>  refs.c                      |  2 +-\n>  t/t6030-bisect-porcelain.sh |  2 +-\n>  4 files changed, 8 insertions(+), 29 deletions(-)\n\nVery nice :-).\n\nThanks,\nTaylor\n"},{"id":"485249","messageId":"ZWe3z59WGE0+8gXN@nand.local","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"Re: [PATCH 0/4] refs: improve handling of special refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-29T22:14:39Z","receivedAt":"2023-11-29T22:14:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 29, 2023 at 09:14:07AM +0100, Patrick Steinhardt wrote:\n> Patrick Steinhardt (4):\n>   wt-status: read HEAD and ORIG_HEAD via the refdb\n>   refs: propagate errno when reading special refs fails\n>   refs: complete list of special refs\n>   bisect: consistently write BISECT_EXPECTED_REV via the refdb\n>\n>  bisect.c                    | 25 +++------------\n>  builtin/bisect.c            |  8 ++---\n>  refs.c                      | 64 +++++++++++++++++++++++++++++++++++--\n>  t/t1403-show-ref.sh         |  9 ++++++\n>  t/t6030-bisect-porcelain.sh |  2 +-\n>  wt-status.c                 | 17 +++++-----\n>  6 files changed, 86 insertions(+), 39 deletions(-)\n\nThese all look pretty good to me. I had a few minor questions on the\nfirst three patches, but I don't think they necessarily require a\nreroll.\n\nThanks,\nTaylor\n"},{"id":"485256","messageId":"ZWg8-HW6yCa6-tnS@tanuki","threadId":"60568","inReplyTo":"ZWew3CP4QJ4XDnHj@nand.local","subject":"Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-30T07:42:48Z","receivedAt":"2023-11-30T07:42:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 29, 2023 at 04:45:00PM -0500, Taylor Blau wrote:\n> On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:\n> > We read both the HEAD and ORIG_HEAD references directly from the\n> > filesystem in order to figure out whether we're currently splitting a\n> > commit. If both of the following are true:\n> >\n> >   - HEAD points to the same object as \"rebase-merge/amend\".\n> >\n> >   - ORIG_HEAD points to the same object as \"rebase-merge/orig-head\".\n> >\n> > Then we are currently splitting commits.\n> >\n> > The current code only works by chance because we only have a single\n> > reference backend implementation. Refactor it to instead read both refs\n> > via the refdb layer so that we'll also be compatible with alternate\n> > reference backends.\n> >\n> > Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is\n> > because we didn't resolve symrefs before either, and in practice none of\n> > the refs in \"rebase-merge/\" would be symbolic. We thus don't want to\n> > resolve symrefs with the new code either to retain the old behaviour.\n> >\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  wt-status.c | 17 +++++++++--------\n> >  1 file changed, 9 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/wt-status.c b/wt-status.c\n> > index 9f45bf6949..fe9e590b80 100644\n> > --- a/wt-status.c\n> > +++ b/wt-status.c\n> > @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)\n> >  static int split_commit_in_progress(struct wt_status *s)\n> >  {\n> >  \tint split_in_progress = 0;\n> > -\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n> > +\tstruct object_id head_oid, orig_head_oid;\n> > +\tchar *rebase_amend, *rebase_orig_head;\n> >\n> >  \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n> >  \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n> >  \t\treturn 0;\n> >\n> > -\thead = read_line_from_git_path(\"HEAD\");\n> > -\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n> > +\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n> > +\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n> \n> Switching to read_ref_full() here is going to have some slightly\n> different behavior than just reading out the contents of\n> \"$GIT_DIR/HEAD\", but I think that it should be OK.\n> \n> Before we would not have complained, if, for example, the contents of\n> \"$GIT_DIR/HEAD\" were malformed, but now we will. I think that's OK,\n> especially given that if that file is bogus, we'll have other problems\n> before we get here ;-).\n> \n> Are there any other gotchas that we should be thinking about?\n\nNot that I can think of. As you say, a repository with malformed HEAD\nwill run into other problems anyway. And `read_ref_full()` would return\nerrors if these refs were malformed, which would cause us to exit early\nfrom anyway. So unless \"rebase-merge/amend\" and \"rebase-merge/orig-head\"\ncontained the same kind of garbage we'd retain the same behaviour as\nbefore, and that shouldn't really be happening.\n\nOne interesting bit is that we don't set `RESOLVE_REF_READING`, so\n`read_ref_full()` may return successfully even if the ref doesn't exist.\nBut in practice this is fine given that the resulting oid would be\ncleared in that case.\n\nPatrick\n\n> > +\t\treturn 0;\n> > +\n> >  \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n> >  \trebase_orig_head = read_line_from_git_path(\"rebase-merge/orig-head\");\n> >\n> > -\tif (!head || !orig_head || !rebase_amend || !rebase_orig_head)\n> > +\tif (!rebase_amend || !rebase_orig_head)\n> >  \t\t; /* fall through, no split in progress */\n> >  \telse if (!strcmp(rebase_amend, rebase_orig_head))\n> > -\t\tsplit_in_progress = !!strcmp(head, rebase_amend);\n> > -\telse if (strcmp(orig_head, rebase_orig_head))\n> > +\t\tsplit_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);\n> > +\telse if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))\n> \n> I did a double take at these strcmp(oid_to_hex(...)) calls, but I think\n> that they are the best that we can do given that we're still reading the\n> contents of \"rebase-merge/amend\" and \"rebase-merge/orig-head\" directly.\n> \n> I suppose we could go the other way and turn their contents into\n> object_ids and then use oidcmp(), but it doesn't seem worth it IMHO.\n> \n> Thanks,\n> Taylor\n"},{"id":"485257","messageId":"ZWg9KfpeyLRdX1b8@tanuki","threadId":"60568","inReplyTo":"ZWeyUU/NqmGUvyOL@nand.local","subject":"Re: [PATCH 2/4] refs: propagate errno when reading special refs fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-30T07:43:37Z","receivedAt":"2023-11-30T07:43:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 29, 2023 at 04:51:13PM -0500, Taylor Blau wrote:\n> On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:\n> > diff --git a/refs.c b/refs.c\n> > index fcae5dddc6..7d4a057f36 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,\n> >  \tint result = -1;\n> >  \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n> >\n> > -\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n> > +\terrno = 0;\n> \n> Do we need to set errno to 0 here? Looking at the implementation of\n> strbuf_read_file(), it looks like we return early in two cases. Either\n> open() fails, in which errno is set for us, or strbuf_read() fails, in\n> which case we set errno to whatever it was right after the failed read\n> (preventing the subsequent close() call from tainting the value of errno).\n> \n> So I think in either case, we have the right value in errno, and don't\n> need to worry about setting it to \"0\" ahead of time.\n\nTrue. I'll drop this when rerolling.\n\n> > +test_expect_success '--exists with existing special ref' '\n> > +\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n> > +\tgit show-ref --exists FETCH_HEAD\n> > +'\n> \n> I don't think that it matters here, but do we need to worry about\n> cleaning up .git/FETCH_HEAD for future tests?\n\nThere's so many tests where I wish that we did a better job of cleanup,\nso I agree that it is sensible to clean up after ourselves. Will drop\nwhen rerolling.\n\nPatrick\n"},{"id":"485258","messageId":"ZWg9RW4L8nBhYmaB@tanuki","threadId":"60568","inReplyTo":"ZWe0RzOoHI9QZMox@nand.local","subject":"Re: [PATCH 3/4] refs: complete list of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-30T07:44:05Z","receivedAt":"2023-11-30T07:44:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 29, 2023 at 04:59:35PM -0500, Taylor Blau wrote:\n> On Wed, Nov 29, 2023 at 09:14:20AM +0100, Patrick Steinhardt wrote:\n> > We have some references that are more special than others. The reason\n> > for them being special is that they either do not follow the usual\n> > format of references, or that they are written to the filesystem\n> > directly by the respective owning subsystem and thus circumvent the\n> > reference backend.\n> >\n> > This works perfectly fine right now because the reffiles backend will\n> > know how to read those refs just fine. But with the prospect of gaining\n> > a new reference backend implementation we need to be a lot more careful\n> > here:\n> >\n> >   - We need to make sure that we are consistent about how those refs are\n> >     written. They must either always be written via the filesystem, or\n> >     they must always be written via the reference backend. Any mixture\n> >     will lead to inconsistent state.\n> >\n> >   - We need to make sure that such special refs are always handled\n> >     specially when reading them.\n> >\n> > We're already mostly good with regard to the first item, except for\n> > `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.\n> > But the current list of special refs is missing a lot of refs that\n> > really should be treated specially. Right now, we only treat\n> > `FETCH_HEAD` and `MERGE_HEAD` specially here.\n> >\n> > Introduce a new function `is_special_ref()` that contains all current\n> > instances of special refs to fix the reading path.\n> >\n> > Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n> >  1 file changed, 56 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/refs.c b/refs.c\n> > index 7d4a057f36..2d39d3fe80 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,\n> >  \treturn result;\n> >  }\n> >\n> > +static int is_special_ref(const char *refname)\n> > +{\n> > +\t/*\n> > +\t * Special references get written and read directly via the filesystem\n> > +\t * by the subsystems that create them. Thus, they must not go through\n> > +\t * the reference backend but must instead be read directly. It is\n> > +\t * arguable whether this behaviour is sensible, or whether it's simply\n> > +\t * a leaky abstraction enabled by us only having a single reference\n> > +\t * backend implementation. But at least for a subset of references it\n> > +\t * indeed does make sense to treat them specially:\n> > +\t *\n> > +\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n> > +\t *   carries additional metadata like where it came from.\n> > +\t *\n> > +\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n> > +\t *   heads.\n> > +\t *\n> > +\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n> > +\t *   rebases, where keeping it closely together feels sensible.\n> > +\t *\n> > +\t * There are some exceptions that you might expect to see on this list\n> > +\t * but which are handled exclusively via the reference backend:\n> > +\t *\n> > +\t * - CHERRY_PICK_HEAD\n> > +\t * - HEAD\n> > +\t * - ORIG_HEAD\n> > +\t *\n> > +\t * Writing or deleting references must consistently go either through\n> > +\t * the filesystem (special refs) or through the reference backend\n> > +\t * (normal ones).\n> > +\t */\n> > +\tconst char * const special_refs[] = {\n> > +\t\t\"AUTO_MERGE\",\n> > +\t\t\"BISECT_EXPECTED_REV\",\n> > +\t\t\"FETCH_HEAD\",\n> > +\t\t\"MERGE_AUTOSTASH\",\n> > +\t\t\"MERGE_HEAD\",\n> > +\t};\n> \n> Is there a reason that we don't want to declare this statically? If we\n> did, I think we could drop one const, since the strings would instead\n> reside in the .rodata section.\n\nNot really, no.\n\n> > +\tint i;\n> \n> Not that it matters for this case, but it may be worth declaring i to be\n> an unsigned type, since it's used as an index into an array. size_t\n> seems like an appropriate choice there.\n\nHm. We do use `int` almost everywhere when iterating through an array\nvia `ARRAY_SIZE`, but ultimately I don't mind whether it's `int`,\n`unsigned` or `size_t`.\n\n> > +\tfor (i = 0; i < ARRAY_SIZE(special_refs); i++)\n> > +\t\tif (!strcmp(refname, special_refs[i]))\n> > +\t\t\treturn 1;\n> > +\n> > +\t/*\n> > +\t * git-rebase(1) stores its state in `rebase-apply/` or\n> > +\t * `rebase-merge/`, including various reference-like bits.\n> > +\t */\n> > +\tif (starts_with(refname, \"rebase-apply/\") ||\n> > +\t    starts_with(refname, \"rebase-merge/\"))\n> \n> Do we care about case sensitivity here? Definitely not on case-sensitive\n> filesystems, but I'm not sure about case-insensitive ones. For instance,\n> on macOS, I can do:\n> \n>     $ git rev-parse hEAd\n> \n> and get the same value as \"git rev-parse HEAD\" (on my Linux workstation,\n> this fails as expected).\n> \n> I doubt that there are many users in the wild asking to resolve\n> reBASe-APPLY/xyz, but I think that after this patch that would no longer\n> work as-is, so we may want to replace this with istarts_with() instead.\n\nIn practice I'd argue that nobody is ever going to ask for something in\n`rebase-apply/` outside of Git internals or scripts, and I'd expect\nthese to always use proper casing. So I rather lean towards a \"no, we\ndon't care about case sensitivity\".\n\nPatrick\n"},{"id":"485259","messageId":"ZWg97gfn80jeB3-_@tanuki","threadId":"60568","inReplyTo":"ZWe3z59WGE0+8gXN@nand.local","subject":"Re: [PATCH 0/4] refs: improve handling of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-11-30T07:46:54Z","receivedAt":"2023-11-30T07:46:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Nov 29, 2023 at 05:14:39PM -0500, Taylor Blau wrote:\n> On Wed, Nov 29, 2023 at 09:14:07AM +0100, Patrick Steinhardt wrote:\n> > Patrick Steinhardt (4):\n> >   wt-status: read HEAD and ORIG_HEAD via the refdb\n> >   refs: propagate errno when reading special refs fails\n> >   refs: complete list of special refs\n> >   bisect: consistently write BISECT_EXPECTED_REV via the refdb\n> >\n> >  bisect.c                    | 25 +++------------\n> >  builtin/bisect.c            |  8 ++---\n> >  refs.c                      | 64 +++++++++++++++++++++++++++++++++++--\n> >  t/t1403-show-ref.sh         |  9 ++++++\n> >  t/t6030-bisect-porcelain.sh |  2 +-\n> >  wt-status.c                 | 17 +++++-----\n> >  6 files changed, 86 insertions(+), 39 deletions(-)\n> \n> These all look pretty good to me. I had a few minor questions on the\n> first three patches, but I don't think they necessarily require a\n> reroll.\n\nI agree that none of the comments really require a reroll, but I'll\naddress them if there will be another iteration of this patch series.\n\nThanks for your review! \n\nPatrick\n"},{"id":"485270","messageId":"15f67e21-c05f-4a72-9557-2a09a1311f25@gmail.com","threadId":"60568","inReplyTo":"0e38103114a206bedbbbd7ea97cb77fa05fd3c29.1701243201.git.ps@pks.im","subject":"Re: [PATCH 3/4] refs: complete list of special refs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-11-30T15:42:06Z","receivedAt":"2023-11-30T15:42:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nThanks for working on this. I've left a couple of thought below.\n\nOn 29/11/2023 08:14, Patrick Steinhardt wrote:\n> +static int is_special_ref(const char *refname)\n> +{\n> +\t/*\n> +\t * Special references get written and read directly via the filesystem\n> +\t * by the subsystems that create them. Thus, they must not go through\n> +\t * the reference backend but must instead be read directly. It is\n> +\t * arguable whether this behaviour is sensible, or whether it's simply\n> +\t * a leaky abstraction enabled by us only having a single reference\n> +\t * backend implementation. But at least for a subset of references it\n> +\t * indeed does make sense to treat them specially:\n> +\t *\n> +\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n> +\t *   carries additional metadata like where it came from.\n> +\t *\n> +\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n> +\t *   heads.\n> +\t *\n> +\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n> +\t *   rebases, where keeping it closely together feels sensible.\n\nI'd really like to get away from treating these files as refs. I think \ntheir use as refs is purely historic and predates the reflog and \npossibly ORIG_HEAD. These days I'm not sure there is a good reason to be \nrunning\n\n     git rev-parse rebase-merge/orig-head\n\nOne reason for not wanting to treat them as refs is that we do not \nhandle multi-level refs that do not begin with \"refs/\" consistently.\n\n     git update-ref foo/bar HEAD\n\nsucceeds and creates .git/foo/bar but\n\n     git update-ref -d foo/bar\n\nfails with\n\n     error: refusing to update ref with bad name 'foo/bar'\n\nTo me it would make sense to refuse to create 'foo/bar' but allow an \nexisting ref named 'foo/bar' to be deleted but the current behavior is \nthe opposite of that.\n\nI'd be quite happy to see us refuse to treat anything that fails\n\n     if (starts_with(refname, \"refs/\") || refname_is_safe(refname))\n\nas a ref but I don't know how much pain that would cause.\n\n> +\tconst char * const special_refs[] = {\n> +\t\t\"AUTO_MERGE\",\n\nIs there any reason to treat this specially in the long term? It points \nto a tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it \nis effectively a \"normal\" ref.\n\n> +\t\t\"BISECT_EXPECTED_REV\",\n> +\t\t\"FETCH_HEAD\",\n> +\t\t\"MERGE_AUTOSTASH\",\n\nShould we be treating this as a ref? I thought it was written as an \nimplementation detail of the autostash implementation rather than to \nprovide a ref for users and scripts.\n\nBest Wishes\n\nPhillip\n"},{"id":"485273","messageId":"ZWjH5kOiHKR8RGkX@nand.local","threadId":"60568","inReplyTo":"ZWg97gfn80jeB3-_@tanuki","subject":"Re: [PATCH 0/4] refs: improve handling of special refs","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-30T17:35:34Z","receivedAt":"2023-11-30T17:35:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 30, 2023 at 08:46:54AM +0100, Patrick Steinhardt wrote:\n> On Wed, Nov 29, 2023 at 05:14:39PM -0500, Taylor Blau wrote:\n> > These all look pretty good to me. I had a few minor questions on the\n> > first three patches, but I don't think they necessarily require a\n> > reroll.\n>\n> I agree that none of the comments really require a reroll, but I'll\n> address them if there will be another iteration of this patch series.\n>\n> Thanks for your review!\n\nNo problem on either. I doubt that there will be another iteration of\nthis series since it is already good, so no need to worry too much about\nthese changes.\n\nThanks,\nTaylor\n"},{"id":"485274","messageId":"ZWjIH9sj5cI8OBLq@nand.local","threadId":"60568","inReplyTo":"ZWg8-HW6yCa6-tnS@tanuki","subject":"Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-11-30T17:36:31Z","receivedAt":"2023-11-30T17:36:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 30, 2023 at 08:42:48AM +0100, Patrick Steinhardt wrote:\n> > Are there any other gotchas that we should be thinking about?\n>\n> Not that I can think of. As you say, a repository with malformed HEAD\n> will run into other problems anyway. And `read_ref_full()` would return\n> errors if these refs were malformed, which would cause us to exit early\n> from anyway. So unless \"rebase-merge/amend\" and \"rebase-merge/orig-head\"\n> contained the same kind of garbage we'd retain the same behaviour as\n> before, and that shouldn't really be happening.\n>\n> One interesting bit is that we don't set `RESOLVE_REF_READING`, so\n> `read_ref_full()` may return successfully even if the ref doesn't exist.\n> But in practice this is fine given that the resulting oid would be\n> cleared in that case.\n\nThanks for thinking through these. I agree with your reasoning and think\nthat this is fine as-is.\n\n(Off-topic, but can you please trim your replies to only include the\nquoted parts/context that you're replying to?)\n\nThanks,\nTaylor\n"},{"id":"485298","messageId":"ZWmAn20UYWBo9i8C@tanuki","threadId":"60568","inReplyTo":"15f67e21-c05f-4a72-9557-2a09a1311f25@gmail.com","subject":"Re: [PATCH 3/4] refs: complete list of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-01T06:43:43Z","receivedAt":"2023-12-01T06:43:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:\n> Hi Patrick\n> \n> Thanks for working on this. I've left a couple of thought below.\n> \n> On 29/11/2023 08:14, Patrick Steinhardt wrote:\n> > +static int is_special_ref(const char *refname)\n> > +{\n> > +\t/*\n> > +\t * Special references get written and read directly via the filesystem\n> > +\t * by the subsystems that create them. Thus, they must not go through\n> > +\t * the reference backend but must instead be read directly. It is\n> > +\t * arguable whether this behaviour is sensible, or whether it's simply\n> > +\t * a leaky abstraction enabled by us only having a single reference\n> > +\t * backend implementation. But at least for a subset of references it\n> > +\t * indeed does make sense to treat them specially:\n> > +\t *\n> > +\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n> > +\t *   carries additional metadata like where it came from.\n> > +\t *\n> > +\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n> > +\t *   heads.\n> > +\t *\n> > +\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n> > +\t *   rebases, where keeping it closely together feels sensible.\n> \n> I'd really like to get away from treating these files as refs. I think their\n> use as refs is purely historic and predates the reflog and possibly\n> ORIG_HEAD. These days I'm not sure there is a good reason to be running\n> \n>     git rev-parse rebase-merge/orig-head\n> \n> One reason for not wanting to treat them as refs is that we do not handle\n> multi-level refs that do not begin with \"refs/\" consistently.\n> \n>     git update-ref foo/bar HEAD\n> \n> succeeds and creates .git/foo/bar but\n> \n>     git update-ref -d foo/bar\n> \n> fails with\n> \n>     error: refusing to update ref with bad name 'foo/bar'\n> \n> To me it would make sense to refuse to create 'foo/bar' but allow an\n> existing ref named 'foo/bar' to be deleted but the current behavior is the\n> opposite of that.\n> \n> I'd be quite happy to see us refuse to treat anything that fails\n> \n>     if (starts_with(refname, \"refs/\") || refname_is_safe(refname))\n> \n> as a ref but I don't know how much pain that would cause.\n\nWell, we already do use these internally as references, but I don't\ndisagree with you. I think the current state is extremely confusing,\nwhich is why my first approach was to simply document what falls into\nthe category of these \"special\" references.\n\nIn my mind, this patch series here is a first step towards addressing\nthe problem more generally. For now it is more or less only documenting\n_what_ is a special ref and why they are special, while also ensuring\nthat these refs are compatible with the reftable backend. But once this\nlands, I'd certainly want to see us continue to iterate on this.\n\nMost importantly, I'd love to see us address two issues:\n\n  - Start to refuse writing these special refs via the refdb so that\n    the rules I've now layed out are also enforced. This would also\n    address your point about things being inconsistent.\n\n  - Gradually reduce the list of special refs so that they are reduced\n    to a bare minimum and so that most refs are simply that, a normal\n    ref.\n\n> > +\tconst char * const special_refs[] = {\n> > +\t\t\"AUTO_MERGE\",\n> \n> Is there any reason to treat this specially in the long term? It points to a\n> tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it is\n> effectively a \"normal\" ref.\n\nNo, I'd love to see this and others converted to become a normal ref\neventually. The goal of this patch series was mostly to document what we\nalready have, and address those cases which are inconsistent with the\nnew rules. But I'd be happy to convert more of these special refs to\nbecome normal refs after it lands.\n\n> > +\t\t\"BISECT_EXPECTED_REV\",\n> > +\t\t\"FETCH_HEAD\",\n> > +\t\t\"MERGE_AUTOSTASH\",\n> \n> Should we be treating this as a ref? I thought it was written as an\n> implementation detail of the autostash implementation rather than to provide\n> a ref for users and scripts.\n\nYes, we have to in the context of the reftable backend. There's a bunch\nof tests that exercise our ability to parse this as a ref, and they\nwould otherwise fail with the reftable backend.\n\nPatrick\n"},{"id":"485350","messageId":"808ca23e-76dc-4435-b1f9-4bd2cdca3acc@gmail.com","threadId":"60568","inReplyTo":"ZWmAn20UYWBo9i8C@tanuki","subject":"Re: [PATCH 3/4] refs: complete list of special refs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-04T14:18:40Z","receivedAt":"2023-12-04T14:18:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 01/12/2023 06:43, Patrick Steinhardt wrote:\n> On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:\n>> Hi Patrick\n>>\n>> Thanks for working on this. I've left a couple of thought below.\n>>\n>> On 29/11/2023 08:14, Patrick Steinhardt wrote:\n>>> +static int is_special_ref(const char *refname)\n>>> +{\n>>> +\t/*\n>>> +\t * Special references get written and read directly via the filesystem\n>>> +\t * by the subsystems that create them. Thus, they must not go through\n>>> +\t * the reference backend but must instead be read directly. It is\n>>> +\t * arguable whether this behaviour is sensible, or whether it's simply\n>>> +\t * a leaky abstraction enabled by us only having a single reference\n>>> +\t * backend implementation. But at least for a subset of references it\n>>> +\t * indeed does make sense to treat them specially:\n>>> +\t *\n>>> +\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n>>> +\t *   carries additional metadata like where it came from.\n>>> +\t *\n>>> +\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n>>> +\t *   heads.\n>>> +\t *\n>>> +\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n>>> +\t *   rebases, where keeping it closely together feels sensible.\n>>\n>> I'd really like to get away from treating these files as refs. I think their\n>> use as refs is purely historic and predates the reflog and possibly\n>> ORIG_HEAD. These days I'm not sure there is a good reason to be running\n>>\n>>      git rev-parse rebase-merge/orig-head\n>>\n>> One reason for not wanting to treat them as refs is that we do not handle\n>> multi-level refs that do not begin with \"refs/\" consistently.\n>>\n>>      git update-ref foo/bar HEAD\n>>\n>> succeeds and creates .git/foo/bar but\n>>\n>>      git update-ref -d foo/bar\n>>\n>> fails with\n>>\n>>      error: refusing to update ref with bad name 'foo/bar'\n>>\n>> To me it would make sense to refuse to create 'foo/bar' but allow an\n>> existing ref named 'foo/bar' to be deleted but the current behavior is the\n>> opposite of that.\n>>\n>> I'd be quite happy to see us refuse to treat anything that fails\n>>\n>>      if (starts_with(refname, \"refs/\") || refname_is_safe(refname))\n>>\n>> as a ref but I don't know how much pain that would cause.\n> \n> Well, we already do use these internally as references, but I don't\n> disagree with you.\n\nI should have been clearer that I was talking about the refs starting \n\"rebase-*\" rather than FETCH_HEAD and MERGE_HEAD. As a user find it \nconvenient to be able to run \"git fetch ... && git log -p FETCH_HEAD\" \neven if the implementation is a bit ugly. As far as I can see we do not \nuse \"rebase-(apply|merge)/(orig-head|amend|autostash)\" as a ref in our \ncode or tests.\n\n> I think the current state is extremely confusing,\n> which is why my first approach was to simply document what falls into\n> the category of these \"special\" references.\n\nThat's certainly a good place to start\n\n> In my mind, this patch series here is a first step towards addressing\n> the problem more generally. For now it is more or less only documenting\n> _what_ is a special ref and why they are special, while also ensuring\n> that these refs are compatible with the reftable backend. But once this\n> lands, I'd certainly want to see us continue to iterate on this.\n> \n> Most importantly, I'd love to see us address two issues:\n> \n>    - Start to refuse writing these special refs via the refdb so that\n>      the rules I've now layed out are also enforced. This would also\n>      address your point about things being inconsistent.\n> \n>    - Gradually reduce the list of special refs so that they are reduced\n>      to a bare minimum and so that most refs are simply that, a normal\n>      ref.\n\nThat sounds like a good plan\n\n>>> +\tconst char * const special_refs[] = {\n>>> +\t\t\"AUTO_MERGE\",\n>>\n>> Is there any reason to treat this specially in the long term? It points to a\n>> tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it is\n>> effectively a \"normal\" ref.\n> \n> No, I'd love to see this and others converted to become a normal ref\n> eventually. The goal of this patch series was mostly to document what we\n> already have, and address those cases which are inconsistent with the\n> new rules. But I'd be happy to convert more of these special refs to\n> become normal refs after it lands.\n\nThat's great\n\n>>> +\t\t\"BISECT_EXPECTED_REV\",\n>>> +\t\t\"FETCH_HEAD\",\n>>> +\t\t\"MERGE_AUTOSTASH\",\n>>\n>> Should we be treating this as a ref? I thought it was written as an\n>> implementation detail of the autostash implementation rather than to provide\n>> a ref for users and scripts.\n> \n> Yes, we have to in the context of the reftable backend. There's a bunch\n> of tests that exercise our ability to parse this as a ref, and they\n> would otherwise fail with the reftable backend.\n\nAh, looking at the the man page for \"git merge\" it seems we do actually \ndocument the existence of MERGE_AUTOSTASH so it is not just an \nimplementation detail after all.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"485569","messageId":"cover.1702365291.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"[PATCH v2 0/4] refs: improve handling of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:18:44Z","receivedAt":"2023-12-12T07:18:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that aims to improve our\nhandling of special refs. This patch series is in preparation for the\nupcoming reftable backend.\n\nChanges compared to v1:\n\n  - Patch 1: Stop needlessly resetting `errno` before calling\n    `strbuf_read_file()`. Also, clean up state we create in our test\n    repos properly.\n\n  - Patch 3: The discussion with Phillip made me reevaluate my claim\n    that \"rebase-apply/\" and \"rebase-merge/\" contain special refs.\n    Indeed they don't, all files in there seem to be exclusively\n    read and written via the filesystem without ever going via the\n    refdb. I've thus dropped them from the set of special refs.\n\n  - Patch 4: The array of static refs is now static.\n\nThanks for your reviews!\n\nPatrick\n\nPatrick Steinhardt (4):\n  wt-status: read HEAD and ORIG_HEAD via the refdb\n  refs: propagate errno when reading special refs fails\n  refs: complete list of special refs\n  bisect: consistently write BISECT_EXPECTED_REV via the refdb\n\n bisect.c                    | 25 +++-------------\n builtin/bisect.c            |  8 ++---\n refs.c                      | 59 +++++++++++++++++++++++++++++++++++--\n t/t1403-show-ref.sh         | 10 +++++++\n t/t6030-bisect-porcelain.sh |  2 +-\n wt-status.c                 | 17 ++++++-----\n 6 files changed, 82 insertions(+), 39 deletions(-)\n\nRange-diff against v1:\n1:  35b74eb972 = 1:  1db3eb3945 wt-status: read HEAD and ORIG_HEAD via the refdb\n2:  691552a17e ! 2:  24032a62e5 refs: propagate errno when reading special refs fails\n    @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,\n      \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n      \n     -\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n    -+\terrno = 0;\n    -+\n     +\tif (strbuf_read_file(&content, full_path.buf, 0) < 0) {\n     +\t\t*failure_errno = errno;\n      \t\tgoto done;\n    @@ t/t1403-show-ref.sh: test_expect_success '--exists with directory fails with gen\n     +'\n     +\n     +test_expect_success '--exists with existing special ref' '\n    ++\ttest_when_finished \"rm .git/FETCH_HEAD\" &&\n     +\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n     +\tgit show-ref --exists FETCH_HEAD\n     +'\n3:  0e38103114 ! 3:  3dd9089fd5 refs: complete list of special refs\n    @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,\n     +\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n     +\t *   heads.\n     +\t *\n    -+\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n    -+\t *   rebases, where keeping it closely together feels sensible.\n    -+\t *\n     +\t * There are some exceptions that you might expect to see on this list\n     +\t * but which are handled exclusively via the reference backend:\n     +\t *\n     +\t * - CHERRY_PICK_HEAD\n    ++\t *\n     +\t * - HEAD\n    ++\t *\n     +\t * - ORIG_HEAD\n     +\t *\n    ++\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n    ++\t *   rebases, including some reference-like files. These are\n    ++\t *   exclusively read and written via the filesystem and never go\n    ++\t *   through the refdb.\n    ++\t *\n     +\t * Writing or deleting references must consistently go either through\n     +\t * the filesystem (special refs) or through the reference backend\n     +\t * (normal ones).\n     +\t */\n    -+\tconst char * const special_refs[] = {\n    ++\tstatic const char * const special_refs[] = {\n     +\t\t\"AUTO_MERGE\",\n     +\t\t\"BISECT_EXPECTED_REV\",\n     +\t\t\"FETCH_HEAD\",\n     +\t\t\"MERGE_AUTOSTASH\",\n     +\t\t\"MERGE_HEAD\",\n     +\t};\n    -+\tint i;\n    ++\tsize_t i;\n     +\n     +\tfor (i = 0; i < ARRAY_SIZE(special_refs); i++)\n     +\t\tif (!strcmp(refname, special_refs[i]))\n     +\t\t\treturn 1;\n     +\n    -+\t/*\n    -+\t * git-rebase(1) stores its state in `rebase-apply/` or\n    -+\t * `rebase-merge/`, including various reference-like bits.\n    -+\t */\n    -+\tif (starts_with(refname, \"rebase-apply/\") ||\n    -+\t    starts_with(refname, \"rebase-merge/\"))\n    -+\t\treturn 1;\n    -+\n     +\treturn 0;\n     +}\n     +\n4:  c7ab26fb7e ! 4:  4a4447a3f5 bisect: consistently write BISECT_EXPECTED_REV via the refdb\n    @@ refs.c: static int is_special_ref(const char *refname)\n      \t * but which are handled exclusively via the reference backend:\n      \t *\n     +\t * - BISECT_EXPECTED_REV\n    ++\t *\n      \t * - CHERRY_PICK_HEAD\n    + \t *\n      \t * - HEAD\n    - \t * - ORIG_HEAD\n     @@ refs.c: static int is_special_ref(const char *refname)\n      \t */\n    - \tconst char * const special_refs[] = {\n    + \tstatic const char * const special_refs[] = {\n      \t\t\"AUTO_MERGE\",\n     -\t\t\"BISECT_EXPECTED_REV\",\n      \t\t\"FETCH_HEAD\",\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \n2.43.GIT\n\n"},{"id":"485570","messageId":"1db3eb3945432964aabe1c559db4c3ac251e83fd.1702365291.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702365291.git.ps@pks.im","subject":"[PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:18:48Z","receivedAt":"2023-12-12T07:18:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We read both the HEAD and ORIG_HEAD references directly from the\nfilesystem in order to figure out whether we're currently splitting a\ncommit. If both of the following are true:\n\n  - HEAD points to the same object as \"rebase-merge/amend\".\n\n  - ORIG_HEAD points to the same object as \"rebase-merge/orig-head\".\n\nThen we are currently splitting commits.\n\nThe current code only works by chance because we only have a single\nreference backend implementation. Refactor it to instead read both refs\nvia the refdb layer so that we'll also be compatible with alternate\nreference backends.\n\nNote that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is\nbecause we didn't resolve symrefs before either, and in practice none of\nthe refs in \"rebase-merge/\" would be symbolic. We thus don't want to\nresolve symrefs with the new code either to retain the old behaviour.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n wt-status.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 9f45bf6949..fe9e590b80 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)\n static int split_commit_in_progress(struct wt_status *s)\n {\n \tint split_in_progress = 0;\n-\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n+\tstruct object_id head_oid, orig_head_oid;\n+\tchar *rebase_amend, *rebase_orig_head;\n \n \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n \t\treturn 0;\n \n-\thead = read_line_from_git_path(\"HEAD\");\n-\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n+\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n+\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n+\t\treturn 0;\n+\n \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n \trebase_orig_head = read_line_from_git_path(\"rebase-merge/orig-head\");\n \n-\tif (!head || !orig_head || !rebase_amend || !rebase_orig_head)\n+\tif (!rebase_amend || !rebase_orig_head)\n \t\t; /* fall through, no split in progress */\n \telse if (!strcmp(rebase_amend, rebase_orig_head))\n-\t\tsplit_in_progress = !!strcmp(head, rebase_amend);\n-\telse if (strcmp(orig_head, rebase_orig_head))\n+\t\tsplit_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);\n+\telse if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))\n \t\tsplit_in_progress = 1;\n \n-\tfree(head);\n-\tfree(orig_head);\n \tfree(rebase_amend);\n \tfree(rebase_orig_head);\n \n-- \n2.43.GIT\n\n"},{"id":"485571","messageId":"24032a62e54fb37dad46c3ede7151efc8a7a8818.1702365291.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702365291.git.ps@pks.im","subject":"[PATCH v2 2/4] refs: propagate errno when reading special refs fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:18:52Z","receivedAt":"2023-12-12T07:18:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some refs in Git are more special than others due to reasons explained\nin the next commit. These refs are read via `refs_read_special_head()`,\nbut this function doesn't behave the same as when we try to read a\nnormal ref. Most importantly, we do not propagate `failure_errno` in the\ncase where the reference does not exist, which is behaviour that we rely\non in many parts of Git.\n\nFix this bug by propagating errno when `strbuf_read_file()` fails.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c              |  4 +++-\n t/t1403-show-ref.sh | 10 ++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex fcae5dddc6..00e72a2abf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \tint result = -1;\n \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n \n-\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n+\tif (strbuf_read_file(&content, full_path.buf, 0) < 0) {\n+\t\t*failure_errno = errno;\n \t\tgoto done;\n+\t}\n \n \tresult = parse_loose_ref_contents(content.buf, oid, referent, type,\n \t\t\t\t\t  failure_errno);\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex b50ae6fcf1..66e6e77fa9 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '\n \ttest_cmp expect err\n '\n \n+test_expect_success '--exists with non-existent special ref' '\n+\ttest_expect_code 2 git show-ref --exists FETCH_HEAD\n+'\n+\n+test_expect_success '--exists with existing special ref' '\n+\ttest_when_finished \"rm .git/FETCH_HEAD\" &&\n+\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n+\tgit show-ref --exists FETCH_HEAD\n+'\n+\n test_done\n-- \n2.43.GIT\n\n"},{"id":"485572","messageId":"3dd9089fd59ca27759dc445664811d52273c8e94.1702365291.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702365291.git.ps@pks.im","subject":"[PATCH v2 3/4] refs: complete list of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:18:56Z","receivedAt":"2023-12-12T07:19:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have some references that are more special than others. The reason\nfor them being special is that they either do not follow the usual\nformat of references, or that they are written to the filesystem\ndirectly by the respective owning subsystem and thus circumvent the\nreference backend.\n\nThis works perfectly fine right now because the reffiles backend will\nknow how to read those refs just fine. But with the prospect of gaining\na new reference backend implementation we need to be a lot more careful\nhere:\n\n  - We need to make sure that we are consistent about how those refs are\n    written. They must either always be written via the filesystem, or\n    they must always be written via the reference backend. Any mixture\n    will lead to inconsistent state.\n\n  - We need to make sure that such special refs are always handled\n    specially when reading them.\n\nWe're already mostly good with regard to the first item, except for\n`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.\nBut the current list of special refs is missing a lot of refs that\nreally should be treated specially. Right now, we only treat\n`FETCH_HEAD` and `MERGE_HEAD` specially here.\n\nIntroduce a new function `is_special_ref()` that contains all current\ninstances of special refs to fix the reading path.\n\nBased-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 52 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 00e72a2abf..8fe34d51e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1820,15 +1820,65 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \treturn result;\n }\n \n+static int is_special_ref(const char *refname)\n+{\n+\t/*\n+\t * Special references get written and read directly via the filesystem\n+\t * by the subsystems that create them. Thus, they must not go through\n+\t * the reference backend but must instead be read directly. It is\n+\t * arguable whether this behaviour is sensible, or whether it's simply\n+\t * a leaky abstraction enabled by us only having a single reference\n+\t * backend implementation. But at least for a subset of references it\n+\t * indeed does make sense to treat them specially:\n+\t *\n+\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n+\t *   carries additional metadata like where it came from.\n+\t *\n+\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n+\t *   heads.\n+\t *\n+\t * There are some exceptions that you might expect to see on this list\n+\t * but which are handled exclusively via the reference backend:\n+\t *\n+\t * - CHERRY_PICK_HEAD\n+\t *\n+\t * - HEAD\n+\t *\n+\t * - ORIG_HEAD\n+\t *\n+\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n+\t *   rebases, including some reference-like files. These are\n+\t *   exclusively read and written via the filesystem and never go\n+\t *   through the refdb.\n+\t *\n+\t * Writing or deleting references must consistently go either through\n+\t * the filesystem (special refs) or through the reference backend\n+\t * (normal ones).\n+\t */\n+\tstatic const char * const special_refs[] = {\n+\t\t\"AUTO_MERGE\",\n+\t\t\"BISECT_EXPECTED_REV\",\n+\t\t\"FETCH_HEAD\",\n+\t\t\"MERGE_AUTOSTASH\",\n+\t\t\"MERGE_HEAD\",\n+\t};\n+\tsize_t i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(special_refs); i++)\n+\t\tif (!strcmp(refname, special_refs[i]))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t      struct object_id *oid, struct strbuf *referent,\n \t\t      unsigned int *type, int *failure_errno)\n {\n \tassert(failure_errno);\n-\tif (!strcmp(refname, \"FETCH_HEAD\") || !strcmp(refname, \"MERGE_HEAD\")) {\n+\tif (is_special_ref(refname))\n \t\treturn refs_read_special_head(ref_store, refname, oid, referent,\n \t\t\t\t\t      type, failure_errno);\n-\t}\n \n \treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\n \t\t\t\t\t   type, failure_errno);\n-- \n2.43.GIT\n\n"},{"id":"485573","messageId":"4a4447a3f5ba9ddef1c81703ef9231ee597c2946.1702365291.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702365291.git.ps@pks.im","subject":"[PATCH v2 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-12T07:19:01Z","receivedAt":"2023-12-12T07:19:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem\nand via the refdb, which violates the newly established rules for how\nspecial refs must be treated. This works alright in practice with the\nreffiles reference backend, but will cause bugs once we gain additional\nbackends.\n\nFix this issue and consistently write BISECT_EXPECTED_REV via the refdb\nso that it is no longer a special ref.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bisect.c                    | 25 ++++---------------------\n builtin/bisect.c            |  8 ++------\n refs.c                      |  3 ++-\n t/t6030-bisect-porcelain.sh |  2 +-\n 4 files changed, 9 insertions(+), 29 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 1be8e0a271..985b96ed13 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -471,7 +471,6 @@ static int read_bisect_refs(void)\n }\n \n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n-static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,\n \n static int is_expected_rev(const struct object_id *oid)\n {\n-\tconst char *filename = git_path_bisect_expected_rev();\n-\tstruct stat st;\n-\tstruct strbuf str = STRBUF_INIT;\n-\tFILE *fp;\n-\tint res = 0;\n-\n-\tif (stat(filename, &st) || !S_ISREG(st.st_mode))\n+\tstruct object_id expected_oid;\n+\tif (read_ref(\"BISECT_EXPECTED_REV\", &expected_oid))\n \t\treturn 0;\n-\n-\tfp = fopen_or_warn(filename, \"r\");\n-\tif (!fp)\n-\t\treturn 0;\n-\n-\tif (strbuf_getline_lf(&str, fp) != EOF)\n-\t\tres = !strcmp(str.buf, oid_to_hex(oid));\n-\n-\tstrbuf_release(&str);\n-\tfclose(fp);\n-\n-\treturn res;\n+\treturn oideq(oid, &expected_oid);\n }\n \n enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)\n \tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n \tfor_each_ref_in(\"refs/bisect\", mark_for_removal, (void *) &refs_for_removal);\n \tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n+\tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_EXPECTED_REV\"));\n \tresult = delete_refs(\"bisect: remove\", &refs_for_removal, REF_NO_DEREF);\n \trefs_for_removal.strdup_strings = 1;\n \tstring_list_clear(&refs_for_removal, 0);\n-\tunlink_or_warn(git_path_bisect_expected_rev());\n \tunlink_or_warn(git_path_bisect_ancestors_ok());\n \tunlink_or_warn(git_path_bisect_log());\n \tunlink_or_warn(git_path_bisect_names());\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 35938b05fd..4e2c43daf5 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -17,7 +17,6 @@\n #include \"revision.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n-static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \tconst char *state;\n \tint i, verify_expected = 1;\n \tstruct object_id oid, expected;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct oid_array revs = OID_ARRAY_INIT;\n \n \tif (!argc)\n@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \t\toid_array_append(&revs, &commit->object.oid);\n \t}\n \n-\tif (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n-\t    get_oid_hex(buf.buf, &expected) < 0)\n+\tif (read_ref(\"BISECT_EXPECTED_REV\", &expected))\n \t\tverify_expected = 0; /* Ignore invalid file contents */\n-\tstrbuf_release(&buf);\n \n \tfor (i = 0; i < revs.nr; i++) {\n \t\tif (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {\n@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \t\t}\n \t\tif (verify_expected && !oideq(&revs.oid[i], &expected)) {\n \t\t\tunlink_or_warn(git_path_bisect_ancestors_ok());\n-\t\t\tunlink_or_warn(git_path_bisect_expected_rev());\n+\t\t\tdelete_ref(NULL, \"BISECT_EXPECTED_REV\", NULL, REF_NO_DEREF);\n \t\t\tverify_expected = 0;\n \t\t}\n \t}\ndiff --git a/refs.c b/refs.c\nindex 8fe34d51e4..c76ce86bef 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1840,6 +1840,8 @@ static int is_special_ref(const char *refname)\n \t * There are some exceptions that you might expect to see on this list\n \t * but which are handled exclusively via the reference backend:\n \t *\n+\t * - BISECT_EXPECTED_REV\n+\t *\n \t * - CHERRY_PICK_HEAD\n \t *\n \t * - HEAD\n@@ -1857,7 +1859,6 @@ static int is_special_ref(const char *refname)\n \t */\n \tstatic const char * const special_refs[] = {\n \t\t\"AUTO_MERGE\",\n-\t\t\"BISECT_EXPECTED_REV\",\n \t\t\"FETCH_HEAD\",\n \t\t\"MERGE_AUTOSTASH\",\n \t\t\"MERGE_HEAD\",\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 2a5b7d8379..792c1504bc 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n \tgit bisect bad $HASH4 &&\n \tgit bisect reset &&\n \ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n-\ttest_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n+\ttest_ref_missing BISECT_EXPECTED_REV &&\n \ttest_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n \ttest_path_is_missing \".git/BISECT_LOG\" &&\n \ttest_path_is_missing \".git/BISECT_RUN\" &&\n-- \n2.43.GIT\n\n"},{"id":"485591","messageId":"xmqqle9zqidj.fsf@gitster.g","threadId":"60568","inReplyTo":"1db3eb3945432964aabe1c559db4c3ac251e83fd.1702365291.git.ps@pks.im","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T20:24:24Z","receivedAt":"2023-12-12T20:24:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The current code only works by chance because we only have a single\n> reference backend implementation. Refactor it to instead read both refs\n> via the refdb layer so that we'll also be compatible with alternate\n> reference backends.\n\n\"via the refdb\" -> \"via the refs API\" or something here and on the\ntitle, and possibly elsewhere in the proposed log messages and\nin-code comments in patches in this series, as I've never seen a\nword \"refdb\" used in the context of this project.\n\nI agree it is bad manners to be intimate with the implementation\ndetails of the how files-backend stores HEAD and ORIG_HEAD.\n\n> Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is\n> because we didn't resolve symrefs before either, and in practice none of\n> the refs in \"rebase-merge/\" would be symbolic. We thus don't want to\n> resolve symrefs with the new code either to retain the old behaviour.\n\nGood to see a rewrite being careful like this.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  wt-status.c | 17 +++++++++--------\n>  1 file changed, 9 insertions(+), 8 deletions(-)\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index 9f45bf6949..fe9e590b80 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)\n>  static int split_commit_in_progress(struct wt_status *s)\n>  {\n>  \tint split_in_progress = 0;\n> -\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n> +\tstruct object_id head_oid, orig_head_oid;\n> +\tchar *rebase_amend, *rebase_orig_head;\n>  \n>  \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n>  \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n>  \t\treturn 0;\n>  \n> -\thead = read_line_from_git_path(\"HEAD\");\n> -\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n> +\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n> +\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n> +\t\treturn 0;\n> +\n\nThis made me wonder if we have changed behaviour when on an unborn\nbranch.  In such a case, the original most likely would have read\n\"ref: blah\" in \"head\" and compared it with \"rebase_amend\", which\nwould be a good way to ensure they would not match.  I would not\nknow offhand what the updated code would do, but head_oid would be\nuninitialized in such a case, so ...?\n\n>  \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n>  \trebase_orig_head = read_line_from_git_path(\"rebase-merge/orig-head\");\n>  \n> -\tif (!head || !orig_head || !rebase_amend || !rebase_orig_head)\n> +\tif (!rebase_amend || !rebase_orig_head)\n>  \t\t; /* fall through, no split in progress */\n>  \telse if (!strcmp(rebase_amend, rebase_orig_head))\n> -\t\tsplit_in_progress = !!strcmp(head, rebase_amend);\n> -\telse if (strcmp(orig_head, rebase_orig_head))\n> +\t\tsplit_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);\n> +\telse if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))\n>  \t\tsplit_in_progress = 1;\n>  \n> -\tfree(head);\n> -\tfree(orig_head);\n>  \tfree(rebase_amend);\n>  \tfree(rebase_orig_head);\n"},{"id":"485592","messageId":"xmqqfs07qi66.fsf@gitster.g","threadId":"60568","inReplyTo":"24032a62e54fb37dad46c3ede7151efc8a7a8818.1702365291.git.ps@pks.im","subject":"Re: [PATCH v2 2/4] refs: propagate errno when reading special refs fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-12T20:28:49Z","receivedAt":"2023-12-12T20:28:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Some refs in Git are more special than others due to reasons explained\n> in the next commit. These refs are read via `refs_read_special_head()`,\n> but this function doesn't behave the same as when we try to read a\n> normal ref. Most importantly, we do not propagate `failure_errno` in the\n> case where the reference does not exist, which is behaviour that we rely\n> on in many parts of Git.\n>\n> Fix this bug by propagating errno when `strbuf_read_file()` fails.\n\nHmph, I thought, judging from what [1/4] did, that your plan was to\nuse the refs API, instead of peeking directly into the filesystem,\nto access these pseudo refs, and am a bit puzzled if it makes all\nthat much difference to fix errno handling while still reading\ndirectly from the filesystem.  Perhaps such a conversion happens in\nlater steps of this series (or a follow on series after this series\nlands)?\n\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  refs.c              |  4 +++-\n>  t/t1403-show-ref.sh | 10 ++++++++++\n>  2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs.c b/refs.c\n> index fcae5dddc6..00e72a2abf 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,\n>  \tint result = -1;\n>  \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n>  \n> -\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n> +\tif (strbuf_read_file(&content, full_path.buf, 0) < 0) {\n> +\t\t*failure_errno = errno;\n>  \t\tgoto done;\n> +\t}\n>  \n>  \tresult = parse_loose_ref_contents(content.buf, oid, referent, type,\n>  \t\t\t\t\t  failure_errno);\n> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\n> index b50ae6fcf1..66e6e77fa9 100755\n> --- a/t/t1403-show-ref.sh\n> +++ b/t/t1403-show-ref.sh\n> @@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '\n>  \ttest_cmp expect err\n>  '\n>  \n> +test_expect_success '--exists with non-existent special ref' '\n> +\ttest_expect_code 2 git show-ref --exists FETCH_HEAD\n> +'\n> +\n> +test_expect_success '--exists with existing special ref' '\n> +\ttest_when_finished \"rm .git/FETCH_HEAD\" &&\n> +\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n> +\tgit show-ref --exists FETCH_HEAD\n> +'\n> +\n>  test_done\n"},{"id":"485601","messageId":"ac84b1b9-2381-406a-b459-6728bf9f8704@ramsayjones.plus.com","threadId":"60568","inReplyTo":"xmqqle9zqidj.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2023-12-12T23:32:37Z","receivedAt":"2023-12-12T23:35:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 12/12/2023 20:24, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n>> The current code only works by chance because we only have a single\n>> reference backend implementation. Refactor it to instead read both refs\n>> via the refdb layer so that we'll also be compatible with alternate\n>> reference backends.\n> \n> \"via the refdb\" -> \"via the refs API\" or something here and on the\n> title, and possibly elsewhere in the proposed log messages and\n> in-code comments in patches in this series, as I've never seen a\n> word \"refdb\" used in the context of this project.\n> \n> I agree it is bad manners to be intimate with the implementation\n> details of the how files-backend stores HEAD and ORIG_HEAD.\n\nHmm, I have never thought of the 'pseudo-refs' as being a part of\nthe 'reference database' at all. ;)\n\nWe seem to have pseudo-refs, special pseudo-refs and (recently)\nex-pseudo-refs!\n\nThis patch (well series) changes the 'status' of some, *but not all*,\npseudo-refs; some graduate to full-blown refs stored as part of *a*\nreference database (ie reftable).\n\nAs far as I recall, this has not been discussed on the ML. Why are\nonly some chosen to become 'full' refs and others not? This is not\ndiscussed in any of the commit messages.\n\nThe '.invalid' HEAD hack featured in a recent completion patch as well.\n[If this is because the JAVA implementation does it this way, I think\nit needs some thought before including it in the Git project].\n\nAnyway, I haven't had the time to study the details here, so please\nignore my uninformed ramblings!\n\nATB,\nRamsay Jones\n\n"},{"id":"485602","messageId":"xmqq34w7os53.fsf@gitster.g","threadId":"60568","inReplyTo":"ac84b1b9-2381-406a-b459-6728bf9f8704@ramsayjones.plus.com","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-13T00:36:24Z","receivedAt":"2023-12-13T00:36:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n>> \"via the refdb\" -> \"via the refs API\" or something here and on the\n>> title, and possibly elsewhere in the proposed log messages and\n>> in-code comments in patches in this series, as I've never seen a\n>> word \"refdb\" used in the context of this project.\n>> \n>> I agree it is bad manners to be intimate with the implementation\n>> details of the how files-backend stores HEAD and ORIG_HEAD.\n>\n> Hmm, I have never thought of the 'pseudo-refs' as being a part of\n> the 'reference database' at all. ;)\n\nMe neither, but once you start thinking about getting rid of the\nneed to use one-file-per-ref filesystem, being able to maintain all\nrefs, including the pseudo refs, in one r/w store backend, becomes a\nvery tempting goal.  From that point of view, I do not have problem\nwith the idea to move _all_ pseudorefs to reftable.\n\nBut I do have reservations on what Patrick, and the code he\ninherited from Han-Wen, calls \"special refs\" (which is not defined\nin the glossary at all), namely, refs.c:is_special_ref() and its\ncallers.  Neither am I very much sympathetic to the hardcoded list\nof \"known\" pseudorefs, refs.c:pseudorefs[].  I cannot quite see why\nwe need anything more than\n\n    any string that passes refs.c:is_pseudoref_syntax() is a\n    pseudoref, is per worktree, and ref backends can store them like\n    any other refs.  Many of them have specific meaning and uses\n    (e.g. HEAD is \"the current branch\").\n\nEnumerating existing pseudorefs in files backend may need to\nopendir(\".git\") + readdir() filtered with is_pseudoref_syntax(),\nand a corresponding implementation for reftable backend may be much\nsimpler (because there won't be \"other cruft\" stored there, unlike\nfiles backend that needs to worry about files that are not refs,\nlike \".git/config\" file.\n\n> We seem to have pseudo-refs, special pseudo-refs and (recently)\n> ex-pseudo-refs!\n>\n> This patch (well series) changes the 'status' of some, *but not all*,\n> pseudo-refs; some graduate to full-blown refs stored as part of *a*\n> reference database (ie reftable).\n\nYeah, that leaves bad taste in my mouth, too.\n"},{"id":"485606","messageId":"ZXldLTofO3tt5JWo@tanuki","threadId":"60568","inReplyTo":"xmqqfs07qi66.fsf@gitster.g","subject":"Re: [PATCH v2 2/4] refs: propagate errno when reading special refs fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-13T07:28:45Z","receivedAt":"2023-12-13T07:28:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 12, 2023 at 12:28:49PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Some refs in Git are more special than others due to reasons explained\n> > in the next commit. These refs are read via `refs_read_special_head()`,\n> > but this function doesn't behave the same as when we try to read a\n> > normal ref. Most importantly, we do not propagate `failure_errno` in the\n> > case where the reference does not exist, which is behaviour that we rely\n> > on in many parts of Git.\n> >\n> > Fix this bug by propagating errno when `strbuf_read_file()` fails.\n> \n> Hmph, I thought, judging from what [1/4] did, that your plan was to\n> use the refs API, instead of peeking directly into the filesystem,\n> to access these pseudo refs, and am a bit puzzled if it makes all\n> that much difference to fix errno handling while still reading\n> directly from the filesystem.  Perhaps such a conversion happens in\n> later steps of this series (or a follow on series after this series\n> lands)?\n\nYes, that's ultimately the goal. The patch series here is only setting\nthe stage by recording what we have, and addressing cases where we are\ninconsistently accessing refs via the filesystem _and_ via the ref API.\nOnce this lands I do want create a follow up patch series that converts\nall refs to be non-special to the extent possible.\n\nI say \"to the extent possible\" though because in the end there will be\ntwo refs that we must continue to treat specially: FETCH_HEAD and\nMERGE_HEAD, which we were treating special before this patch series\nalready. Both of these are not normal refs because they may contain\nmultiple values with annotations, so they will need to stay special.\n\nPatrick\n"},{"id":"485607","messageId":"ZXlfeWtDgr1GQFCL@tanuki","threadId":"60568","inReplyTo":"xmqq34w7os53.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-13T07:38:33Z","receivedAt":"2023-12-13T07:38:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 12, 2023 at 04:36:24PM -0800, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n> >> \"via the refdb\" -> \"via the refs API\" or something here and on the\n> >> title, and possibly elsewhere in the proposed log messages and\n> >> in-code comments in patches in this series, as I've never seen a\n> >> word \"refdb\" used in the context of this project.\n> >> \n> >> I agree it is bad manners to be intimate with the implementation\n> >> details of the how files-backend stores HEAD and ORIG_HEAD.\n> >\n> > Hmm, I have never thought of the 'pseudo-refs' as being a part of\n> > the 'reference database' at all. ;)\n> \n> Me neither, but once you start thinking about getting rid of the\n> need to use one-file-per-ref filesystem, being able to maintain all\n> refs, including the pseudo refs, in one r/w store backend, becomes a\n> very tempting goal.  From that point of view, I do not have problem\n> with the idea to move _all_ pseudorefs to reftable.\n\nYes, we're in agreement.\n\n> But I do have reservations on what Patrick, and the code he\n> inherited from Han-Wen, calls \"special refs\" (which is not defined\n> in the glossary at all), namely, refs.c:is_special_ref() and its\n> callers.\n\nI do not want to add \"special refs\" to the glossary because ultimately\nthey should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD.\nOnce we're there we can of course discuss whether we want to explicitly\npoint them out in the glossary and give them a special name.\n\n> Neither am I very much sympathetic to the hardcoded list\n> of \"known\" pseudorefs, refs.c:pseudorefs[].  I cannot quite see why\n> we need anything more than\n\n\n>     any string that passes refs.c:is_pseudoref_syntax() is a\n>     pseudoref, is per worktree, and ref backends can store them like\n>     any other refs.  Many of them have specific meaning and uses\n>     (e.g. HEAD is \"the current branch\").\n> \n> Enumerating existing pseudorefs in files backend may need to\n> opendir(\".git\") + readdir() filtered with is_pseudoref_syntax(),\n> and a corresponding implementation for reftable backend may be much\n> simpler (because there won't be \"other cruft\" stored there, unlike\n> files backend that needs to worry about files that are not refs,\n> like \".git/config\" file.\n> \n> > We seem to have pseudo-refs, special pseudo-refs and (recently)\n> > ex-pseudo-refs!\n> >\n> > This patch (well series) changes the 'status' of some, *but not all*,\n> > pseudo-refs; some graduate to full-blown refs stored as part of *a*\n> > reference database (ie reftable).\n> \n> Yeah, that leaves bad taste in my mouth, too.\n\nI'm taking an iterative approach to things, which means that we're at\ntimes going to be in an in-between state. I want to avoid changing too\nmany things at once and overwhelming potential reviewers. But I realize\nthat I should've done a better job of explaining that this patch series\nis not the end goal, but rather a step towards that goal.\n\nThe patch series at hand merely records the status quo and rectifies any\ninconsistencies we have with accessing such \"special\" refs. The natural\nnext step here would be to reduce the list of special refs (like e.g. we\ndo in patch 4) so that in the end it will only contain those refs which\nreally are special (FETCH_HEAD, MERGE_HEAD).\n\nPlease let me know in case you strongly disagree with my iterative\napproach, or whether the issue is rather that I didn't make myself\nsufficiently clear.\n\nPatrick\n"},{"id":"485617","messageId":"xmqqmsuennfu.fsf@gitster.g","threadId":"60568","inReplyTo":"ZXlfeWtDgr1GQFCL@tanuki","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-13T15:15:33Z","receivedAt":"2023-12-13T15:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Me neither, but once you start thinking about getting rid of the\n>> need to use one-file-per-ref filesystem, being able to maintain all\n>> refs, including the pseudo refs, in one r/w store backend, becomes a\n>> very tempting goal.  From that point of view, I do not have problem\n>> with the idea to move _all_ pseudorefs to reftable.\n>\n> Yes, we're in agreement.\n>\n>> But I do have reservations on what Patrick, and the code he\n>> inherited from Han-Wen, calls \"special refs\" (which is not defined\n>> in the glossary at all), namely, refs.c:is_special_ref() and its\n>> callers.\n>\n> I do not want to add \"special refs\" to the glossary because ultimately\n> they should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD.\n> Once we're there we can of course discuss whether we want to explicitly\n> point them out in the glossary and give them a special name.\n\nOK, I somehow got a (wrong) impression that you are very close to\nthe finish line.  If the intention is to leave many others still in\nthe \"special\" category (for only the reasons of inertia), with a\nvision that some selected few must remain \"special\" with their own\ngood reasons, then I am perfectly fine.\n\nThanks.\n"},{"id":"485638","messageId":"ZXrFOtKNLxyT8Csj@tanuki","threadId":"60568","inReplyTo":"xmqqmsuennfu.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T09:04:58Z","receivedAt":"2023-12-14T09:05:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 13, 2023 at 07:15:33AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> Me neither, but once you start thinking about getting rid of the\n> >> need to use one-file-per-ref filesystem, being able to maintain all\n> >> refs, including the pseudo refs, in one r/w store backend, becomes a\n> >> very tempting goal.  From that point of view, I do not have problem\n> >> with the idea to move _all_ pseudorefs to reftable.\n> >\n> > Yes, we're in agreement.\n> >\n> >> But I do have reservations on what Patrick, and the code he\n> >> inherited from Han-Wen, calls \"special refs\" (which is not defined\n> >> in the glossary at all), namely, refs.c:is_special_ref() and its\n> >> callers.\n> >\n> > I do not want to add \"special refs\" to the glossary because ultimately\n> > they should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD.\n> > Once we're there we can of course discuss whether we want to explicitly\n> > point them out in the glossary and give them a special name.\n> \n> OK, I somehow got a (wrong) impression that you are very close to\n> the finish line.\n\nYou mean with the reftable backend? I indeed am quite close, I've just\nfinished the last prerequisite (\"extensions.refFormat\" and related\ntooling) today. I will send that patch series upstream for review once\nmy patches that fix repo initialization with git-clone(1) land in the\n\"next\" branch. The current state at [1] passes CI, even though there\nwill of course still be bugs which aren't covered by the test suite.\n\nSo once all prerequisites that are currently in flight plus the pending\n\"extensions.refFormat\" series have landed I will send the reftable\nbackend implementation in for review. If things continue to go smoothly\nI expect that this may happen at the end of January/start of February.\n\nAnyway. This patch series here is in fact already sufficient to make\nreftables work with those special refs. The only thing that we require\nin this context is that refs are either exclusively routed through the\nfilesystem, or exclusively routed through the ref API. If that property\nholds then things work just fine.\n\nBut still, I do want to clean up the remaining special refs regardless\nof that, even though it is not a mandatory prerequisite. I find that the\ncurrent state is just plain confusing with all these special cases, and\nI'd really love for it to be simplified. Also, I think there is benefit\nin having those refs in reftables because it does allow for proper\natomic updates.\n\n> If the intention is to leave many others still in\n> the \"special\" category (for only the reasons of inertia), with a\n> vision that some selected few must remain \"special\" with their own\n> good reasons, then I am perfectly fine.\n\nOkay.\n\nPatrick\n\n[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/58\n"},{"id":"485644","messageId":"ZXsBX-PW5J_5nTPT@tanuki","threadId":"60568","inReplyTo":"xmqqle9zqidj.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T13:21:35Z","receivedAt":"2023-12-14T13:21:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Dec 12, 2023 at 12:24:24PM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > diff --git a/wt-status.c b/wt-status.c\n> > index 9f45bf6949..fe9e590b80 100644\n> > --- a/wt-status.c\n> > +++ b/wt-status.c\n> > @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)\n> >  static int split_commit_in_progress(struct wt_status *s)\n> >  {\n> >  \tint split_in_progress = 0;\n> > -\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n> > +\tstruct object_id head_oid, orig_head_oid;\n> > +\tchar *rebase_amend, *rebase_orig_head;\n> >  \n> >  \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n> >  \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n> >  \t\treturn 0;\n> >  \n> > -\thead = read_line_from_git_path(\"HEAD\");\n> > -\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n> > +\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n> > +\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n> > +\t\treturn 0;\n> > +\n> \n> This made me wonder if we have changed behaviour when on an unborn\n> branch.  In such a case, the original most likely would have read\n> \"ref: blah\" in \"head\" and compared it with \"rebase_amend\", which\n> would be a good way to ensure they would not match.  I would not\n> know offhand what the updated code would do, but head_oid would be\n> uninitialized in such a case, so ...?\n\nThe code flow goes as following:\n\n  1. We call `read_ref_full()`, which itself calls\n     `refs_resolve_ref_unsafe()`.\n\n  2. It calls `refs_read_raw_ref()`, which succeeds and finds the\n     symref.\n\n  3. We notice that this is a symref and that `RESOLVE_REF_NO_RECURSE`\n     is set. We thus clear the object ID and return the name of the\n     symref target.\n\n  4. Back in `read_ref_full()` we see that `refs_resolve_ref_unsafe()`\n     returns the symref target, which we interpret as successful lookup.\n     We thus return `0`.\n\n  5. Now we look up \"rebase-merge/{amend,orig-head}\" and end up\n     comparing whatever they contain with the cleared OID resolved from\n     HEAD/ORIG_HEAD.\n\nSo the OID would not be uninitialized but the zero OID. Now:\n\n  - \"rebase-merge/amend\" always contains the result of `repo_get_oid()`\n    and never contains the zero OID.\n\n  - \"rebase-merge/orig-head\" may contain the zero OID when there was no\n    ORIG_HEAD at the time of starting a rebase or in case it did not\n    resolve\n\nSo... if ORIG_HEAD was rewritten to be a symref pointing into nirvana\nbetween starting the rebase and calling into \"wt-status.c\", and when\nORIG_HEAD didn't exist at the time of starting the rebase, then we might\nnow wrongly report that splitting was in progress.\n\nIn other cases the old code was actually doing the wrong thing. Suppose\nthat ORIG_HEAD was a dangling symref, then we'd have written the zero\nOID into \"rebase-merge/orig-head\". But when calling into \"wt-status.c\"\nnow we read the still-dangling symref value and notice that the zero OID\nis different than \"ref: refs/heads/dangling\".\n\nI dunno. It feels like this is one of many cases where as you start to\nthink deeply about how things behave you realize that it's been broken\nall along. On the other hand, I doubt there was even a single user who\never experienced this issue. It often just needs to be correct enough.\n\nI think the best way to go about this is to check for `REF_ISSSYMREF`\nand exit early in that case. We only want to compare direct refs, so\nthis is closer to the old behaviour and should even fix edge cases like\nthe above.\n\nWill update.\n\nPatrick\n"},{"id":"485645","messageId":"cover.1702560829.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1701243201.git.ps@pks.im","subject":"[PATCH v3 0/4] refs: improve handling of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T13:36:53Z","receivedAt":"2023-12-14T13:36:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series to improve handling of\nspecial refs.\n\nChanges combpared to v2:\n\n  - Patch 1: We're now more careful around missing and symbolic refs.\n\n  - Patch 3: Document in the commit message that the extended list of\n    special refs is not intended to stay like this forever.\n\nThanks for your reviews and the discussion!\n\nPatrick\n\nPatrick Steinhardt (4):\n  wt-status: read HEAD and ORIG_HEAD via the refdb\n  refs: propagate errno when reading special refs fails\n  refs: complete list of special refs\n  bisect: consistently write BISECT_EXPECTED_REV via the refdb\n\n bisect.c                    | 25 +++-------------\n builtin/bisect.c            |  8 ++---\n refs.c                      | 59 +++++++++++++++++++++++++++++++++++--\n t/t1403-show-ref.sh         | 10 +++++++\n t/t6030-bisect-porcelain.sh |  2 +-\n wt-status.c                 | 22 +++++++++-----\n 6 files changed, 87 insertions(+), 39 deletions(-)\n\nRange-diff against v2:\n1:  1db3eb3945 ! 1:  aea9410bd9 wt-status: read HEAD and ORIG_HEAD via the refdb\n    @@ Commit message\n         via the refdb layer so that we'll also be compatible with alternate\n         reference backends.\n     \n    -    Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is\n    -    because we didn't resolve symrefs before either, and in practice none of\n    -    the refs in \"rebase-merge/\" would be symbolic. We thus don't want to\n    -    resolve symrefs with the new code either to retain the old behaviour.\n    +    There are some subtleties involved here:\n    +\n    +      - We pass `RESOLVE_REF_READING` so that a missing ref will cause\n    +        `read_ref_full()` to return an error.\n    +\n    +      - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve\n    +        symrefs. The old code didn't resolve symrefs either, and we only\n    +        ever write object IDs into the refs in \"rebase-merge/\".\n    +\n    +      - In the same spirit we verify that successfully-read refs are not\n    +        symbolic refs.\n     \n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n    @@ wt-status.c: static char *read_line_from_git_path(const char *filename)\n     -\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n     +\tstruct object_id head_oid, orig_head_oid;\n     +\tchar *rebase_amend, *rebase_orig_head;\n    ++\tint head_flags, orig_head_flags;\n      \n      \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n      \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n    @@ wt-status.c: static char *read_line_from_git_path(const char *filename)\n      \n     -\thead = read_line_from_git_path(\"HEAD\");\n     -\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n    -+\tif (read_ref_full(\"HEAD\", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||\n    -+\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))\n    ++\tif (read_ref_full(\"HEAD\", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n    ++\t\t\t  &head_oid, &head_flags) ||\n    ++\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n    ++\t\t\t  &orig_head_oid, &orig_head_flags))\n    ++\t\treturn 0;\n    ++\tif (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF)\n     +\t\treturn 0;\n     +\n      \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n2:  24032a62e5 = 2:  455ab69177 refs: propagate errno when reading special refs fails\n3:  3dd9089fd5 ! 3:  81ac092281 refs: complete list of special refs\n    @@ Commit message\n     \n         We're already mostly good with regard to the first item, except for\n         `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.\n    -    But the current list of special refs is missing a lot of refs that\n    -    really should be treated specially. Right now, we only treat\n    -    `FETCH_HEAD` and `MERGE_HEAD` specially here.\n    +    But the current list of special refs is missing some refs that really\n    +    should be treated specially. Right now, we only treat `FETCH_HEAD` and\n    +    `MERGE_HEAD` specially here.\n     \n         Introduce a new function `is_special_ref()` that contains all current\n         instances of special refs to fix the reading path.\n     \n    +    Note that this is only a temporary measure where we record and rectify\n    +    the current state. Ideally, the list of special refs should in the end\n    +    only contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may\n    +    reference multiple objects and can contain annotations, so they indeed\n    +    are special.\n    +\n         Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n4:  4a4447a3f5 = 4:  3244678161 bisect: consistently write BISECT_EXPECTED_REV via the refdb\n\nbase-commit: 1a87c842ece327d03d08096395969aca5e0a6996\n-- \n2.43.GIT\n\n"},{"id":"485646","messageId":"aea9410bd9e1023e02d4a48ac7bbd2df52a819b8.1702560829.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702560829.git.ps@pks.im","subject":"[PATCH v3 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T13:36:58Z","receivedAt":"2023-12-14T13:37:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We read both the HEAD and ORIG_HEAD references directly from the\nfilesystem in order to figure out whether we're currently splitting a\ncommit. If both of the following are true:\n\n  - HEAD points to the same object as \"rebase-merge/amend\".\n\n  - ORIG_HEAD points to the same object as \"rebase-merge/orig-head\".\n\nThen we are currently splitting commits.\n\nThe current code only works by chance because we only have a single\nreference backend implementation. Refactor it to instead read both refs\nvia the refdb layer so that we'll also be compatible with alternate\nreference backends.\n\nThere are some subtleties involved here:\n\n  - We pass `RESOLVE_REF_READING` so that a missing ref will cause\n    `read_ref_full()` to return an error.\n\n  - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve\n    symrefs. The old code didn't resolve symrefs either, and we only\n    ever write object IDs into the refs in \"rebase-merge/\".\n\n  - In the same spirit we verify that successfully-read refs are not\n    symbolic refs.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n wt-status.c | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 9f45bf6949..da19923981 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1295,26 +1295,32 @@ static char *read_line_from_git_path(const char *filename)\n static int split_commit_in_progress(struct wt_status *s)\n {\n \tint split_in_progress = 0;\n-\tchar *head, *orig_head, *rebase_amend, *rebase_orig_head;\n+\tstruct object_id head_oid, orig_head_oid;\n+\tchar *rebase_amend, *rebase_orig_head;\n+\tint head_flags, orig_head_flags;\n \n \tif ((!s->amend && !s->nowarn && !s->workdir_dirty) ||\n \t    !s->branch || strcmp(s->branch, \"HEAD\"))\n \t\treturn 0;\n \n-\thead = read_line_from_git_path(\"HEAD\");\n-\torig_head = read_line_from_git_path(\"ORIG_HEAD\");\n+\tif (read_ref_full(\"HEAD\", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t  &head_oid, &head_flags) ||\n+\t    read_ref_full(\"ORIG_HEAD\", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t  &orig_head_oid, &orig_head_flags))\n+\t\treturn 0;\n+\tif (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF)\n+\t\treturn 0;\n+\n \trebase_amend = read_line_from_git_path(\"rebase-merge/amend\");\n \trebase_orig_head = read_line_from_git_path(\"rebase-merge/orig-head\");\n \n-\tif (!head || !orig_head || !rebase_amend || !rebase_orig_head)\n+\tif (!rebase_amend || !rebase_orig_head)\n \t\t; /* fall through, no split in progress */\n \telse if (!strcmp(rebase_amend, rebase_orig_head))\n-\t\tsplit_in_progress = !!strcmp(head, rebase_amend);\n-\telse if (strcmp(orig_head, rebase_orig_head))\n+\t\tsplit_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);\n+\telse if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))\n \t\tsplit_in_progress = 1;\n \n-\tfree(head);\n-\tfree(orig_head);\n \tfree(rebase_amend);\n \tfree(rebase_orig_head);\n \n-- \n2.43.GIT\n\n"},{"id":"485647","messageId":"455ab69177586370b809b4a790ae99ce5e97dafa.1702560829.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702560829.git.ps@pks.im","subject":"[PATCH v3 2/4] refs: propagate errno when reading special refs fails","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T13:37:02Z","receivedAt":"2023-12-14T13:37:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Some refs in Git are more special than others due to reasons explained\nin the next commit. These refs are read via `refs_read_special_head()`,\nbut this function doesn't behave the same as when we try to read a\nnormal ref. Most importantly, we do not propagate `failure_errno` in the\ncase where the reference does not exist, which is behaviour that we rely\non in many parts of Git.\n\nFix this bug by propagating errno when `strbuf_read_file()` fails.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c              |  4 +++-\n t/t1403-show-ref.sh | 10 ++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex fcae5dddc6..00e72a2abf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \tint result = -1;\n \tstrbuf_addf(&full_path, \"%s/%s\", ref_store->gitdir, refname);\n \n-\tif (strbuf_read_file(&content, full_path.buf, 0) < 0)\n+\tif (strbuf_read_file(&content, full_path.buf, 0) < 0) {\n+\t\t*failure_errno = errno;\n \t\tgoto done;\n+\t}\n \n \tresult = parse_loose_ref_contents(content.buf, oid, referent, type,\n \t\t\t\t\t  failure_errno);\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex b50ae6fcf1..66e6e77fa9 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '\n \ttest_cmp expect err\n '\n \n+test_expect_success '--exists with non-existent special ref' '\n+\ttest_expect_code 2 git show-ref --exists FETCH_HEAD\n+'\n+\n+test_expect_success '--exists with existing special ref' '\n+\ttest_when_finished \"rm .git/FETCH_HEAD\" &&\n+\tgit rev-parse HEAD >.git/FETCH_HEAD &&\n+\tgit show-ref --exists FETCH_HEAD\n+'\n+\n test_done\n-- \n2.43.GIT\n\n"},{"id":"485648","messageId":"81ac092281d9ebfc0f231faef6adcdce05cdb1ec.1702560829.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702560829.git.ps@pks.im","subject":"[PATCH v3 3/4] refs: complete list of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T13:37:06Z","receivedAt":"2023-12-14T13:37:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have some references that are more special than others. The reason\nfor them being special is that they either do not follow the usual\nformat of references, or that they are written to the filesystem\ndirectly by the respective owning subsystem and thus circumvent the\nreference backend.\n\nThis works perfectly fine right now because the reffiles backend will\nknow how to read those refs just fine. But with the prospect of gaining\na new reference backend implementation we need to be a lot more careful\nhere:\n\n  - We need to make sure that we are consistent about how those refs are\n    written. They must either always be written via the filesystem, or\n    they must always be written via the reference backend. Any mixture\n    will lead to inconsistent state.\n\n  - We need to make sure that such special refs are always handled\n    specially when reading them.\n\nWe're already mostly good with regard to the first item, except for\n`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.\nBut the current list of special refs is missing some refs that really\nshould be treated specially. Right now, we only treat `FETCH_HEAD` and\n`MERGE_HEAD` specially here.\n\nIntroduce a new function `is_special_ref()` that contains all current\ninstances of special refs to fix the reading path.\n\nNote that this is only a temporary measure where we record and rectify\nthe current state. Ideally, the list of special refs should in the end\nonly contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may\nreference multiple objects and can contain annotations, so they indeed\nare special.\n\nBased-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 52 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 00e72a2abf..8fe34d51e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1820,15 +1820,65 @@ static int refs_read_special_head(struct ref_store *ref_store,\n \treturn result;\n }\n \n+static int is_special_ref(const char *refname)\n+{\n+\t/*\n+\t * Special references get written and read directly via the filesystem\n+\t * by the subsystems that create them. Thus, they must not go through\n+\t * the reference backend but must instead be read directly. It is\n+\t * arguable whether this behaviour is sensible, or whether it's simply\n+\t * a leaky abstraction enabled by us only having a single reference\n+\t * backend implementation. But at least for a subset of references it\n+\t * indeed does make sense to treat them specially:\n+\t *\n+\t * - FETCH_HEAD may contain multiple object IDs, and each one of them\n+\t *   carries additional metadata like where it came from.\n+\t *\n+\t * - MERGE_HEAD may contain multiple object IDs when merging multiple\n+\t *   heads.\n+\t *\n+\t * There are some exceptions that you might expect to see on this list\n+\t * but which are handled exclusively via the reference backend:\n+\t *\n+\t * - CHERRY_PICK_HEAD\n+\t *\n+\t * - HEAD\n+\t *\n+\t * - ORIG_HEAD\n+\t *\n+\t * - \"rebase-apply/\" and \"rebase-merge/\" contain all of the state for\n+\t *   rebases, including some reference-like files. These are\n+\t *   exclusively read and written via the filesystem and never go\n+\t *   through the refdb.\n+\t *\n+\t * Writing or deleting references must consistently go either through\n+\t * the filesystem (special refs) or through the reference backend\n+\t * (normal ones).\n+\t */\n+\tstatic const char * const special_refs[] = {\n+\t\t\"AUTO_MERGE\",\n+\t\t\"BISECT_EXPECTED_REV\",\n+\t\t\"FETCH_HEAD\",\n+\t\t\"MERGE_AUTOSTASH\",\n+\t\t\"MERGE_HEAD\",\n+\t};\n+\tsize_t i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(special_refs); i++)\n+\t\tif (!strcmp(refname, special_refs[i]))\n+\t\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t      struct object_id *oid, struct strbuf *referent,\n \t\t      unsigned int *type, int *failure_errno)\n {\n \tassert(failure_errno);\n-\tif (!strcmp(refname, \"FETCH_HEAD\") || !strcmp(refname, \"MERGE_HEAD\")) {\n+\tif (is_special_ref(refname))\n \t\treturn refs_read_special_head(ref_store, refname, oid, referent,\n \t\t\t\t\t      type, failure_errno);\n-\t}\n \n \treturn ref_store->be->read_raw_ref(ref_store, refname, oid, referent,\n \t\t\t\t\t   type, failure_errno);\n-- \n2.43.GIT\n\n"},{"id":"485649","messageId":"324467816187a8d29407f03006c936b5938f811c.1702560829.git.ps@pks.im","threadId":"60568","inReplyTo":"cover.1702560829.git.ps@pks.im","subject":"[PATCH v3 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-14T13:37:13Z","receivedAt":"2023-12-14T13:37:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem\nand via the refdb, which violates the newly established rules for how\nspecial refs must be treated. This works alright in practice with the\nreffiles reference backend, but will cause bugs once we gain additional\nbackends.\n\nFix this issue and consistently write BISECT_EXPECTED_REV via the refdb\nso that it is no longer a special ref.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bisect.c                    | 25 ++++---------------------\n builtin/bisect.c            |  8 ++------\n refs.c                      |  3 ++-\n t/t6030-bisect-porcelain.sh |  2 +-\n 4 files changed, 9 insertions(+), 29 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 1be8e0a271..985b96ed13 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -471,7 +471,6 @@ static int read_bisect_refs(void)\n }\n \n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n-static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_run, \"BISECT_RUN\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,\n \n static int is_expected_rev(const struct object_id *oid)\n {\n-\tconst char *filename = git_path_bisect_expected_rev();\n-\tstruct stat st;\n-\tstruct strbuf str = STRBUF_INIT;\n-\tFILE *fp;\n-\tint res = 0;\n-\n-\tif (stat(filename, &st) || !S_ISREG(st.st_mode))\n+\tstruct object_id expected_oid;\n+\tif (read_ref(\"BISECT_EXPECTED_REV\", &expected_oid))\n \t\treturn 0;\n-\n-\tfp = fopen_or_warn(filename, \"r\");\n-\tif (!fp)\n-\t\treturn 0;\n-\n-\tif (strbuf_getline_lf(&str, fp) != EOF)\n-\t\tres = !strcmp(str.buf, oid_to_hex(oid));\n-\n-\tstrbuf_release(&str);\n-\tfclose(fp);\n-\n-\treturn res;\n+\treturn oideq(oid, &expected_oid);\n }\n \n enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)\n \tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n \tfor_each_ref_in(\"refs/bisect\", mark_for_removal, (void *) &refs_for_removal);\n \tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n+\tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_EXPECTED_REV\"));\n \tresult = delete_refs(\"bisect: remove\", &refs_for_removal, REF_NO_DEREF);\n \trefs_for_removal.strdup_strings = 1;\n \tstring_list_clear(&refs_for_removal, 0);\n-\tunlink_or_warn(git_path_bisect_expected_rev());\n \tunlink_or_warn(git_path_bisect_ancestors_ok());\n \tunlink_or_warn(git_path_bisect_log());\n \tunlink_or_warn(git_path_bisect_names());\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 35938b05fd..4e2c43daf5 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -17,7 +17,6 @@\n #include \"revision.h\"\n \n static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n-static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, \"BISECT_ANCESTORS_OK\")\n static GIT_PATH_FUNC(git_path_bisect_start, \"BISECT_START\")\n static GIT_PATH_FUNC(git_path_bisect_log, \"BISECT_LOG\")\n@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \tconst char *state;\n \tint i, verify_expected = 1;\n \tstruct object_id oid, expected;\n-\tstruct strbuf buf = STRBUF_INIT;\n \tstruct oid_array revs = OID_ARRAY_INIT;\n \n \tif (!argc)\n@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \t\toid_array_append(&revs, &commit->object.oid);\n \t}\n \n-\tif (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||\n-\t    get_oid_hex(buf.buf, &expected) < 0)\n+\tif (read_ref(\"BISECT_EXPECTED_REV\", &expected))\n \t\tverify_expected = 0; /* Ignore invalid file contents */\n-\tstrbuf_release(&buf);\n \n \tfor (i = 0; i < revs.nr; i++) {\n \t\tif (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {\n@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,\n \t\t}\n \t\tif (verify_expected && !oideq(&revs.oid[i], &expected)) {\n \t\t\tunlink_or_warn(git_path_bisect_ancestors_ok());\n-\t\t\tunlink_or_warn(git_path_bisect_expected_rev());\n+\t\t\tdelete_ref(NULL, \"BISECT_EXPECTED_REV\", NULL, REF_NO_DEREF);\n \t\t\tverify_expected = 0;\n \t\t}\n \t}\ndiff --git a/refs.c b/refs.c\nindex 8fe34d51e4..c76ce86bef 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1840,6 +1840,8 @@ static int is_special_ref(const char *refname)\n \t * There are some exceptions that you might expect to see on this list\n \t * but which are handled exclusively via the reference backend:\n \t *\n+\t * - BISECT_EXPECTED_REV\n+\t *\n \t * - CHERRY_PICK_HEAD\n \t *\n \t * - HEAD\n@@ -1857,7 +1859,6 @@ static int is_special_ref(const char *refname)\n \t */\n \tstatic const char * const special_refs[] = {\n \t\t\"AUTO_MERGE\",\n-\t\t\"BISECT_EXPECTED_REV\",\n \t\t\"FETCH_HEAD\",\n \t\t\"MERGE_AUTOSTASH\",\n \t\t\"MERGE_HEAD\",\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex 2a5b7d8379..792c1504bc 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '\n \tgit bisect bad $HASH4 &&\n \tgit bisect reset &&\n \ttest -z \"$(git for-each-ref \"refs/bisect/*\")\" &&\n-\ttest_path_is_missing \".git/BISECT_EXPECTED_REV\" &&\n+\ttest_ref_missing BISECT_EXPECTED_REV &&\n \ttest_path_is_missing \".git/BISECT_ANCESTORS_OK\" &&\n \ttest_path_is_missing \".git/BISECT_LOG\" &&\n \ttest_path_is_missing \".git/BISECT_RUN\" &&\n-- \n2.43.GIT\n\n"},{"id":"485657","messageId":"xmqqy1dwohxx.fsf@gitster.g","threadId":"60568","inReplyTo":"ZXrFOtKNLxyT8Csj@tanuki","subject":"Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-14T16:41:14Z","receivedAt":"2023-12-14T16:41:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> OK, I somehow got a (wrong) impression that you are very close to\n>> the finish line.\n>\n> You mean with the reftable backend?\n\nIt would be a major achievement if we just stop bypassing refs API\nto read/write ORIG_HEAD and friends, even if we are still using only\nthe files backend.  And I meant that I got an impression that you\nare very close to that, regardless of the readiness of the reftable\nsupport.\n"},{"id":"485884","messageId":"xmqqil4ssmfg.fsf@gitster.g","threadId":"60568","inReplyTo":"cover.1702560829.git.ps@pks.im","subject":"Re: [PATCH v3 0/4] refs: improve handling of special refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-20T19:28:51Z","receivedAt":"2023-12-20T19:28:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Patrick Steinhardt (4):\n>   wt-status: read HEAD and ORIG_HEAD via the refdb\n>   refs: propagate errno when reading special refs fails\n>   refs: complete list of special refs\n>   bisect: consistently write BISECT_EXPECTED_REV via the refdb\n\nWith the clear understanding that we plan to make those other than\nFETCH_HEAD and MERGE_HEAD in the is_special_ref().special_refs[]\neventually not special at all, this round looked quite sensible to\nme.\n\nLet's merge it down to 'next'.\n\nThanks.\n"},{"id":"485915","messageId":"ZYQOnAeRWsiGTr6n@framework","threadId":"60568","inReplyTo":"xmqqil4ssmfg.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] refs: improve handling of special refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-12-21T10:08:28Z","receivedAt":"2023-12-21T10:08:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 20, 2023 at 11:28:51AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Patrick Steinhardt (4):\n> >   wt-status: read HEAD and ORIG_HEAD via the refdb\n> >   refs: propagate errno when reading special refs fails\n> >   refs: complete list of special refs\n> >   bisect: consistently write BISECT_EXPECTED_REV via the refdb\n> \n> With the clear understanding that we plan to make those other than\n> FETCH_HEAD and MERGE_HEAD in the is_special_ref().special_refs[]\n> eventually not special at all, this round looked quite sensible to\n> me.\n> \n> Let's merge it down to 'next'.\n\nThanks. We (myself or a fellow team member at GitLab) will work on\nconverting the remaining not-so-special refs once this topic lands on\nthe `master` branch. Unless of course somebody else picks it up before\nwe do.\n\nPatrick\n"}]}