[PATCH 0/4] refs: improve handling of special refs

[PATCH 0/4] refs: improve handling of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2305 bytes --]

Hi,

there are a bunch of "special" refs in Git that sometimes behave like a
normal reference and sometimes they don't. These references are written
to directly via the filesystem without going through the reference
backend, but the expectation is that those references can then be read
by things like git-rev-parse(1).

We do not currently have a single source of truth for what those special
refs are, and we also don't have clear rules for how they should be
written to. This works in the context of the files backend, because it
is able to read back such manually-written loose references just fine.
But once the reftable backend lands this will stop working.

This patch series tries to improve this state by doing two things:

    1. We explicitly mark these references as special by introducing a
       new `is_special_ref()` function. This serves as documentation,
       but will also cause us to explicitly read all of these special
       refs via loose files regardless of the actual backend.

    2. We document a new rule around writing refs. Namely, normal
       references are _always_ written via the reference backend,
       whereas special references are _always_ written directly via the
       filesystem. This rule is not enforced anywhere, but at least it's
       now made more explicit.

The last patch fixes one of the instances where we treat a reference
inconsistently by converting it to a normal reference. We can eventually
migrate more of the special refs to become normal refs as we deem fit,
but I consider this to be out of scope for this patch series.

These patches improve compatibility with the new reftable backend.

Patrick

Patrick Steinhardt (4):
  wt-status: read HEAD and ORIG_HEAD via the refdb
  refs: propagate errno when reading special refs fails
  refs: complete list of special refs
  bisect: consistently write BISECT_EXPECTED_REV via the refdb

 bisect.c                    | 25 +++------------
 builtin/bisect.c            |  8 ++---
 refs.c                      | 64 +++++++++++++++++++++++++++++++++++--
 t/t1403-show-ref.sh         |  9 ++++++
 t/t6030-bisect-porcelain.sh |  2 +-
 wt-status.c                 | 17 +++++-----
 6 files changed, 86 insertions(+), 39 deletions(-)

-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2583 bytes --]

We read both the HEAD and ORIG_HEAD references directly from the
filesystem in order to figure out whether we're currently splitting a
commit. If both of the following are true:

  - HEAD points to the same object as "rebase-merge/amend".

  - ORIG_HEAD points to the same object as "rebase-merge/orig-head".

Then we are currently splitting commits.

The current code only works by chance because we only have a single
reference backend implementation. Refactor it to instead read both refs
via the refdb layer so that we'll also be compatible with alternate
reference backends.

Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
because we didn't resolve symrefs before either, and in practice none of
the refs in "rebase-merge/" would be symbolic. We thus don't want to
resolve symrefs with the new code either to retain the old behaviour.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 wt-status.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..fe9e590b80 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
 static int split_commit_in_progress(struct wt_status *s)
 {
 	int split_in_progress = 0;
-	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+	struct object_id head_oid, orig_head_oid;
+	char *rebase_amend, *rebase_orig_head;
 
 	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
 	    !s->branch || strcmp(s->branch, "HEAD"))
 		return 0;
 
-	head = read_line_from_git_path("HEAD");
-	orig_head = read_line_from_git_path("ORIG_HEAD");
+	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
+	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
+		return 0;
+
 	rebase_amend = read_line_from_git_path("rebase-merge/amend");
 	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
 
-	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+	if (!rebase_amend || !rebase_orig_head)
 		; /* fall through, no split in progress */
 	else if (!strcmp(rebase_amend, rebase_orig_head))
-		split_in_progress = !!strcmp(head, rebase_amend);
-	else if (strcmp(orig_head, rebase_orig_head))
+		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
+	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
 		split_in_progress = 1;
 
-	free(head);
-	free(orig_head);
 	free(rebase_amend);
 	free(rebase_orig_head);
 
-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/4] refs: propagate errno when reading special refs fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]

Some refs in Git are more special than others due to reasons explained
in the next commit. These refs are read via `refs_read_special_head()`,
but this function doesn't behave the same as when we try to read a
normal ref. Most importantly, we do not propagate `failure_errno` in the
case where the reference does not exist, which is behaviour that we rely
on in many parts of Git.

Fix this bug by propagating errno when `strbuf_read_file()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c              | 6 +++++-
 t/t1403-show-ref.sh | 9 +++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index fcae5dddc6..7d4a057f36 100644
--- a/refs.c
+++ b/refs.c
@@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	int result = -1;
 	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
 
-	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
+	errno = 0;
+
+	if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
+		*failure_errno = errno;
 		goto done;
+	}
 
 	result = parse_loose_ref_contents(content.buf, oid, referent, type,
 					  failure_errno);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..37af154d27 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -266,4 +266,13 @@ test_expect_success '--exists with directory fails with generic error' '
 	test_cmp expect err
 '
 
+test_expect_success '--exists with non-existent special ref' '
+	test_expect_code 2 git show-ref --exists FETCH_HEAD
+'
+
+test_expect_success '--exists with existing special ref' '
+	git rev-parse HEAD >.git/FETCH_HEAD &&
+	git show-ref --exists FETCH_HEAD
+'
+
 test_done
-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 3/4] refs: complete list of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4174 bytes --]

We have some references that are more special than others. The reason
for them being special is that they either do not follow the usual
format of references, or that they are written to the filesystem
directly by the respective owning subsystem and thus circumvent the
reference backend.

This works perfectly fine right now because the reffiles backend will
know how to read those refs just fine. But with the prospect of gaining
a new reference backend implementation we need to be a lot more careful
here:

  - We need to make sure that we are consistent about how those refs are
    written. They must either always be written via the filesystem, or
    they must always be written via the reference backend. Any mixture
    will lead to inconsistent state.

  - We need to make sure that such special refs are always handled
    specially when reading them.

We're already mostly good with regard to the first item, except for
`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
But the current list of special refs is missing a lot of refs that
really should be treated specially. Right now, we only treat
`FETCH_HEAD` and `MERGE_HEAD` specially here.

Introduce a new function `is_special_ref()` that contains all current
instances of special refs to fix the reading path.

Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 7d4a057f36..2d39d3fe80 100644
--- a/refs.c
+++ b/refs.c
@@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	return result;
 }
 
+static int is_special_ref(const char *refname)
+{
+	/*
+	 * Special references get written and read directly via the filesystem
+	 * by the subsystems that create them. Thus, they must not go through
+	 * the reference backend but must instead be read directly. It is
+	 * arguable whether this behaviour is sensible, or whether it's simply
+	 * a leaky abstraction enabled by us only having a single reference
+	 * backend implementation. But at least for a subset of references it
+	 * indeed does make sense to treat them specially:
+	 *
+	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
+	 *   carries additional metadata like where it came from.
+	 *
+	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
+	 *   heads.
+	 *
+	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
+	 *   rebases, where keeping it closely together feels sensible.
+	 *
+	 * There are some exceptions that you might expect to see on this list
+	 * but which are handled exclusively via the reference backend:
+	 *
+	 * - CHERRY_PICK_HEAD
+	 * - HEAD
+	 * - ORIG_HEAD
+	 *
+	 * Writing or deleting references must consistently go either through
+	 * the filesystem (special refs) or through the reference backend
+	 * (normal ones).
+	 */
+	const char * const special_refs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"FETCH_HEAD",
+		"MERGE_AUTOSTASH",
+		"MERGE_HEAD",
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
+		if (!strcmp(refname, special_refs[i]))
+			return 1;
+
+	/*
+	 * git-rebase(1) stores its state in `rebase-apply/` or
+	 * `rebase-merge/`, including various reference-like bits.
+	 */
+	if (starts_with(refname, "rebase-apply/") ||
+	    starts_with(refname, "rebase-merge/"))
+		return 1;
+
+	return 0;
+}
+
 int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		      struct object_id *oid, struct strbuf *referent,
 		      unsigned int *type, int *failure_errno)
 {
 	assert(failure_errno);
-	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
+	if (is_special_ref(refname))
 		return refs_read_special_head(ref_store, refname, oid, referent,
 					      type, failure_errno);
-	}
 
 	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
 					   type, failure_errno);
-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5645 bytes --]

We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem
and via the refdb, which violates the newly established rules for how
special refs must be treated. This works alright in practice with the
reffiles reference backend, but will cause bugs once we gain additional
backends.

Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb
so that it is no longer a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  2 +-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 8 insertions(+), 29 deletions(-)

diff --git a/bisect.c b/bisect.c
index 1be8e0a271..985b96ed13 100644
--- a/bisect.c
+++ b/bisect.c
@@ -471,7 +471,6 @@ static int read_bisect_refs(void)
 }
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,
 
 static int is_expected_rev(const struct object_id *oid)
 {
-	const char *filename = git_path_bisect_expected_rev();
-	struct stat st;
-	struct strbuf str = STRBUF_INIT;
-	FILE *fp;
-	int res = 0;
-
-	if (stat(filename, &st) || !S_ISREG(st.st_mode))
+	struct object_id expected_oid;
+	if (read_ref("BISECT_EXPECTED_REV", &expected_oid))
 		return 0;
-
-	fp = fopen_or_warn(filename, "r");
-	if (!fp)
-		return 0;
-
-	if (strbuf_getline_lf(&str, fp) != EOF)
-		res = !strcmp(str.buf, oid_to_hex(oid));
-
-	strbuf_release(&str);
-	fclose(fp);
-
-	return res;
+	return oideq(oid, &expected_oid);
 }
 
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
 	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF);
 	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
-	unlink_or_warn(git_path_bisect_expected_rev());
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());
 	unlink_or_warn(git_path_bisect_names());
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 35938b05fd..4e2c43daf5 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -17,7 +17,6 @@
 #include "revision.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 	const char *state;
 	int i, verify_expected = 1;
 	struct object_id oid, expected;
-	struct strbuf buf = STRBUF_INIT;
 	struct oid_array revs = OID_ARRAY_INIT;
 
 	if (!argc)
@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 		oid_array_append(&revs, &commit->object.oid);
 	}
 
-	if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
-	    get_oid_hex(buf.buf, &expected) < 0)
+	if (read_ref("BISECT_EXPECTED_REV", &expected))
 		verify_expected = 0; /* Ignore invalid file contents */
-	strbuf_release(&buf);
 
 	for (i = 0; i < revs.nr; i++) {
 		if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {
@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 		}
 		if (verify_expected && !oideq(&revs.oid[i], &expected)) {
 			unlink_or_warn(git_path_bisect_ancestors_ok());
-			unlink_or_warn(git_path_bisect_expected_rev());
+			delete_ref(NULL, "BISECT_EXPECTED_REV", NULL, REF_NO_DEREF);
 			verify_expected = 0;
 		}
 	}
diff --git a/refs.c b/refs.c
index 2d39d3fe80..0290bb0c67 100644
--- a/refs.c
+++ b/refs.c
@@ -1845,6 +1845,7 @@ static int is_special_ref(const char *refname)
 	 * There are some exceptions that you might expect to see on this list
 	 * but which are handled exclusively via the reference backend:
 	 *
+	 * - BISECT_EXPECTED_REV
 	 * - CHERRY_PICK_HEAD
 	 * - HEAD
 	 * - ORIG_HEAD
@@ -1855,7 +1856,6 @@ static int is_special_ref(const char *refname)
 	 */
 	const char * const special_refs[] = {
 		"AUTO_MERGE",
-		"BISECT_EXPECTED_REV",
 		"FETCH_HEAD",
 		"MERGE_AUTOSTASH",
 		"MERGE_HEAD",
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2a5b7d8379..792c1504bc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	git bisect bad $HASH4 &&
 	git bisect reset &&
 	test -z "$(git for-each-ref "refs/bisect/*")" &&
-	test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
+	test_ref_missing BISECT_EXPECTED_REV &&
 	test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
 	test_path_is_missing ".git/BISECT_LOG" &&
 	test_path_is_missing ".git/BISECT_RUN" &&
-- 
2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Taylor Blau]

On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:
> We read both the HEAD and ORIG_HEAD references directly from the
> filesystem in order to figure out whether we're currently splitting a
> commit. If both of the following are true:
>
>   - HEAD points to the same object as "rebase-merge/amend".
>
>   - ORIG_HEAD points to the same object as "rebase-merge/orig-head".
>
> Then we are currently splitting commits.
>
> The current code only works by chance because we only have a single
> reference backend implementation. Refactor it to instead read both refs
> via the refdb layer so that we'll also be compatible with alternate
> reference backends.
>
> Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
> because we didn't resolve symrefs before either, and in practice none of
> the refs in "rebase-merge/" would be symbolic. We thus don't want to
> resolve symrefs with the new code either to retain the old behaviour.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  wt-status.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 9f45bf6949..fe9e590b80 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
>  static int split_commit_in_progress(struct wt_status *s)
>  {
>  	int split_in_progress = 0;
> -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> +	struct object_id head_oid, orig_head_oid;
> +	char *rebase_amend, *rebase_orig_head;
>
>  	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
>  	    !s->branch || strcmp(s->branch, "HEAD"))
>  		return 0;
>
> -	head = read_line_from_git_path("HEAD");
> -	orig_head = read_line_from_git_path("ORIG_HEAD");
> +	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> +	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))

Switching to read_ref_full() here is going to have some slightly
different behavior than just reading out the contents of
"$GIT_DIR/HEAD", but I think that it should be OK.

Before we would not have complained, if, for example, the contents of
"$GIT_DIR/HEAD" were malformed, but now we will. I think that's OK,
especially given that if that file is bogus, we'll have other problems
before we get here ;-).

Are there any other gotchas that we should be thinking about?

> +		return 0;
> +
>  	rebase_amend = read_line_from_git_path("rebase-merge/amend");
>  	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>
> -	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> +	if (!rebase_amend || !rebase_orig_head)
>  		; /* fall through, no split in progress */
>  	else if (!strcmp(rebase_amend, rebase_orig_head))
> -		split_in_progress = !!strcmp(head, rebase_amend);
> -	else if (strcmp(orig_head, rebase_orig_head))
> +		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
> +	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))

I did a double take at these strcmp(oid_to_hex(...)) calls, but I think
that they are the best that we can do given that we're still reading the
contents of "rebase-merge/amend" and "rebase-merge/orig-head" directly.

I suppose we could go the other way and turn their contents into
object_ids and then use oidcmp(), but it doesn't seem worth it IMHO.

Thanks,
Taylor

Re: [PATCH 2/4] refs: propagate errno when reading special refs fails

[Taylor Blau]

On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:
> diff --git a/refs.c b/refs.c
> index fcae5dddc6..7d4a057f36 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  	int result = -1;
>  	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
>
> -	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
> +	errno = 0;

Do we need to set errno to 0 here? Looking at the implementation of
strbuf_read_file(), it looks like we return early in two cases. Either
open() fails, in which errno is set for us, or strbuf_read() fails, in
which case we set errno to whatever it was right after the failed read
(preventing the subsequent close() call from tainting the value of errno).

So I think in either case, we have the right value in errno, and don't
need to worry about setting it to "0" ahead of time.

> +test_expect_success '--exists with existing special ref' '
> +	git rev-parse HEAD >.git/FETCH_HEAD &&
> +	git show-ref --exists FETCH_HEAD
> +'

I don't think that it matters here, but do we need to worry about
cleaning up .git/FETCH_HEAD for future tests?

Thanks,
Taylor

Re: [PATCH 3/4] refs: complete list of special refs

[Taylor Blau]

On Wed, Nov 29, 2023 at 09:14:20AM +0100, Patrick Steinhardt wrote:
> We have some references that are more special than others. The reason
> for them being special is that they either do not follow the usual
> format of references, or that they are written to the filesystem
> directly by the respective owning subsystem and thus circumvent the
> reference backend.
>
> This works perfectly fine right now because the reffiles backend will
> know how to read those refs just fine. But with the prospect of gaining
> a new reference backend implementation we need to be a lot more careful
> here:
>
>   - We need to make sure that we are consistent about how those refs are
>     written. They must either always be written via the filesystem, or
>     they must always be written via the reference backend. Any mixture
>     will lead to inconsistent state.
>
>   - We need to make sure that such special refs are always handled
>     specially when reading them.
>
> We're already mostly good with regard to the first item, except for
> `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
> But the current list of special refs is missing a lot of refs that
> really should be treated specially. Right now, we only treat
> `FETCH_HEAD` and `MERGE_HEAD` specially here.
>
> Introduce a new function `is_special_ref()` that contains all current
> instances of special refs to fix the reading path.
>
> Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 7d4a057f36..2d39d3fe80 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  	return result;
>  }
>
> +static int is_special_ref(const char *refname)
> +{
> +	/*
> +	 * Special references get written and read directly via the filesystem
> +	 * by the subsystems that create them. Thus, they must not go through
> +	 * the reference backend but must instead be read directly. It is
> +	 * arguable whether this behaviour is sensible, or whether it's simply
> +	 * a leaky abstraction enabled by us only having a single reference
> +	 * backend implementation. But at least for a subset of references it
> +	 * indeed does make sense to treat them specially:
> +	 *
> +	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
> +	 *   carries additional metadata like where it came from.
> +	 *
> +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
> +	 *   heads.
> +	 *
> +	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
> +	 *   rebases, where keeping it closely together feels sensible.
> +	 *
> +	 * There are some exceptions that you might expect to see on this list
> +	 * but which are handled exclusively via the reference backend:
> +	 *
> +	 * - CHERRY_PICK_HEAD
> +	 * - HEAD
> +	 * - ORIG_HEAD
> +	 *
> +	 * Writing or deleting references must consistently go either through
> +	 * the filesystem (special refs) or through the reference backend
> +	 * (normal ones).
> +	 */
> +	const char * const special_refs[] = {
> +		"AUTO_MERGE",
> +		"BISECT_EXPECTED_REV",
> +		"FETCH_HEAD",
> +		"MERGE_AUTOSTASH",
> +		"MERGE_HEAD",
> +	};

Is there a reason that we don't want to declare this statically? If we
did, I think we could drop one const, since the strings would instead
reside in the .rodata section.

> +	int i;

Not that it matters for this case, but it may be worth declaring i to be
an unsigned type, since it's used as an index into an array. size_t
seems like an appropriate choice there.

> +	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
> +		if (!strcmp(refname, special_refs[i]))
> +			return 1;
> +
> +	/*
> +	 * git-rebase(1) stores its state in `rebase-apply/` or
> +	 * `rebase-merge/`, including various reference-like bits.
> +	 */
> +	if (starts_with(refname, "rebase-apply/") ||
> +	    starts_with(refname, "rebase-merge/"))

Do we care about case sensitivity here? Definitely not on case-sensitive
filesystems, but I'm not sure about case-insensitive ones. For instance,
on macOS, I can do:

    $ git rev-parse hEAd

and get the same value as "git rev-parse HEAD" (on my Linux workstation,
this fails as expected).

I doubt that there are many users in the wild asking to resolve
reBASe-APPLY/xyz, but I think that after this patch that would no longer
work as-is, so we may want to replace this with istarts_with() instead.

Thanks,
Taylor

Re: [PATCH 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb

[Taylor Blau]

On Wed, Nov 29, 2023 at 09:14:24AM +0100, Patrick Steinhardt wrote:
> ---
>  bisect.c                    | 25 ++++---------------------
>  builtin/bisect.c            |  8 ++------
>  refs.c                      |  2 +-
>  t/t6030-bisect-porcelain.sh |  2 +-
>  4 files changed, 8 insertions(+), 29 deletions(-)

Very nice :-).

Thanks,
Taylor

Re: [PATCH 0/4] refs: improve handling of special refs

[Taylor Blau]

On Wed, Nov 29, 2023 at 09:14:07AM +0100, Patrick Steinhardt wrote:
> Patrick Steinhardt (4):
>   wt-status: read HEAD and ORIG_HEAD via the refdb
>   refs: propagate errno when reading special refs fails
>   refs: complete list of special refs
>   bisect: consistently write BISECT_EXPECTED_REV via the refdb
>
>  bisect.c                    | 25 +++------------
>  builtin/bisect.c            |  8 ++---
>  refs.c                      | 64 +++++++++++++++++++++++++++++++++++--
>  t/t1403-show-ref.sh         |  9 ++++++
>  t/t6030-bisect-porcelain.sh |  2 +-
>  wt-status.c                 | 17 +++++-----
>  6 files changed, 86 insertions(+), 39 deletions(-)

These all look pretty good to me. I had a few minor questions on the
first three patches, but I don't think they necessarily require a
reroll.

Thanks,
Taylor

Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4365 bytes --]

On Wed, Nov 29, 2023 at 04:45:00PM -0500, Taylor Blau wrote:
> On Wed, Nov 29, 2023 at 09:14:12AM +0100, Patrick Steinhardt wrote:
> > We read both the HEAD and ORIG_HEAD references directly from the
> > filesystem in order to figure out whether we're currently splitting a
> > commit. If both of the following are true:
> >
> >   - HEAD points to the same object as "rebase-merge/amend".
> >
> >   - ORIG_HEAD points to the same object as "rebase-merge/orig-head".
> >
> > Then we are currently splitting commits.
> >
> > The current code only works by chance because we only have a single
> > reference backend implementation. Refactor it to instead read both refs
> > via the refdb layer so that we'll also be compatible with alternate
> > reference backends.
> >
> > Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
> > because we didn't resolve symrefs before either, and in practice none of
> > the refs in "rebase-merge/" would be symbolic. We thus don't want to
> > resolve symrefs with the new code either to retain the old behaviour.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  wt-status.c | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/wt-status.c b/wt-status.c
> > index 9f45bf6949..fe9e590b80 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
> >  static int split_commit_in_progress(struct wt_status *s)
> >  {
> >  	int split_in_progress = 0;
> > -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> > +	struct object_id head_oid, orig_head_oid;
> > +	char *rebase_amend, *rebase_orig_head;
> >
> >  	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> >  	    !s->branch || strcmp(s->branch, "HEAD"))
> >  		return 0;
> >
> > -	head = read_line_from_git_path("HEAD");
> > -	orig_head = read_line_from_git_path("ORIG_HEAD");
> > +	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> > +	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
> 
> Switching to read_ref_full() here is going to have some slightly
> different behavior than just reading out the contents of
> "$GIT_DIR/HEAD", but I think that it should be OK.
> 
> Before we would not have complained, if, for example, the contents of
> "$GIT_DIR/HEAD" were malformed, but now we will. I think that's OK,
> especially given that if that file is bogus, we'll have other problems
> before we get here ;-).
> 
> Are there any other gotchas that we should be thinking about?

Not that I can think of. As you say, a repository with malformed HEAD
will run into other problems anyway. And `read_ref_full()` would return
errors if these refs were malformed, which would cause us to exit early
from anyway. So unless "rebase-merge/amend" and "rebase-merge/orig-head"
contained the same kind of garbage we'd retain the same behaviour as
before, and that shouldn't really be happening.

One interesting bit is that we don't set `RESOLVE_REF_READING`, so
`read_ref_full()` may return successfully even if the ref doesn't exist.
But in practice this is fine given that the resulting oid would be
cleared in that case.

Patrick

> > +		return 0;
> > +
> >  	rebase_amend = read_line_from_git_path("rebase-merge/amend");
> >  	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
> >
> > -	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> > +	if (!rebase_amend || !rebase_orig_head)
> >  		; /* fall through, no split in progress */
> >  	else if (!strcmp(rebase_amend, rebase_orig_head))
> > -		split_in_progress = !!strcmp(head, rebase_amend);
> > -	else if (strcmp(orig_head, rebase_orig_head))
> > +		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
> > +	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
> 
> I did a double take at these strcmp(oid_to_hex(...)) calls, but I think
> that they are the best that we can do given that we're still reading the
> contents of "rebase-merge/amend" and "rebase-merge/orig-head" directly.
> 
> I suppose we could go the other way and turn their contents into
> object_ids and then use oidcmp(), but it doesn't seem worth it IMHO.
> 
> Thanks,
> Taylor

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] refs: propagate errno when reading special refs fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On Wed, Nov 29, 2023 at 04:51:13PM -0500, Taylor Blau wrote:
> On Wed, Nov 29, 2023 at 09:14:16AM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs.c b/refs.c
> > index fcae5dddc6..7d4a057f36 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1806,8 +1806,12 @@ static int refs_read_special_head(struct ref_store *ref_store,
> >  	int result = -1;
> >  	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
> >
> > -	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
> > +	errno = 0;
> 
> Do we need to set errno to 0 here? Looking at the implementation of
> strbuf_read_file(), it looks like we return early in two cases. Either
> open() fails, in which errno is set for us, or strbuf_read() fails, in
> which case we set errno to whatever it was right after the failed read
> (preventing the subsequent close() call from tainting the value of errno).
> 
> So I think in either case, we have the right value in errno, and don't
> need to worry about setting it to "0" ahead of time.

True. I'll drop this when rerolling.

> > +test_expect_success '--exists with existing special ref' '
> > +	git rev-parse HEAD >.git/FETCH_HEAD &&
> > +	git show-ref --exists FETCH_HEAD
> > +'
> 
> I don't think that it matters here, but do we need to worry about
> cleaning up .git/FETCH_HEAD for future tests?

There's so many tests where I wish that we did a better job of cleanup,
so I agree that it is sensible to clean up after ourselves. Will drop
when rerolling.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] refs: complete list of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5442 bytes --]

On Wed, Nov 29, 2023 at 04:59:35PM -0500, Taylor Blau wrote:
> On Wed, Nov 29, 2023 at 09:14:20AM +0100, Patrick Steinhardt wrote:
> > We have some references that are more special than others. The reason
> > for them being special is that they either do not follow the usual
> > format of references, or that they are written to the filesystem
> > directly by the respective owning subsystem and thus circumvent the
> > reference backend.
> >
> > This works perfectly fine right now because the reffiles backend will
> > know how to read those refs just fine. But with the prospect of gaining
> > a new reference backend implementation we need to be a lot more careful
> > here:
> >
> >   - We need to make sure that we are consistent about how those refs are
> >     written. They must either always be written via the filesystem, or
> >     they must always be written via the reference backend. Any mixture
> >     will lead to inconsistent state.
> >
> >   - We need to make sure that such special refs are always handled
> >     specially when reading them.
> >
> > We're already mostly good with regard to the first item, except for
> > `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
> > But the current list of special refs is missing a lot of refs that
> > really should be treated specially. Right now, we only treat
> > `FETCH_HEAD` and `MERGE_HEAD` specially here.
> >
> > Introduce a new function `is_special_ref()` that contains all current
> > instances of special refs to fix the reading path.
> >
> > Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> >
> > diff --git a/refs.c b/refs.c
> > index 7d4a057f36..2d39d3fe80 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1822,15 +1822,69 @@ static int refs_read_special_head(struct ref_store *ref_store,
> >  	return result;
> >  }
> >
> > +static int is_special_ref(const char *refname)
> > +{
> > +	/*
> > +	 * Special references get written and read directly via the filesystem
> > +	 * by the subsystems that create them. Thus, they must not go through
> > +	 * the reference backend but must instead be read directly. It is
> > +	 * arguable whether this behaviour is sensible, or whether it's simply
> > +	 * a leaky abstraction enabled by us only having a single reference
> > +	 * backend implementation. But at least for a subset of references it
> > +	 * indeed does make sense to treat them specially:
> > +	 *
> > +	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
> > +	 *   carries additional metadata like where it came from.
> > +	 *
> > +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
> > +	 *   heads.
> > +	 *
> > +	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
> > +	 *   rebases, where keeping it closely together feels sensible.
> > +	 *
> > +	 * There are some exceptions that you might expect to see on this list
> > +	 * but which are handled exclusively via the reference backend:
> > +	 *
> > +	 * - CHERRY_PICK_HEAD
> > +	 * - HEAD
> > +	 * - ORIG_HEAD
> > +	 *
> > +	 * Writing or deleting references must consistently go either through
> > +	 * the filesystem (special refs) or through the reference backend
> > +	 * (normal ones).
> > +	 */
> > +	const char * const special_refs[] = {
> > +		"AUTO_MERGE",
> > +		"BISECT_EXPECTED_REV",
> > +		"FETCH_HEAD",
> > +		"MERGE_AUTOSTASH",
> > +		"MERGE_HEAD",
> > +	};
> 
> Is there a reason that we don't want to declare this statically? If we
> did, I think we could drop one const, since the strings would instead
> reside in the .rodata section.

Not really, no.

> > +	int i;
> 
> Not that it matters for this case, but it may be worth declaring i to be
> an unsigned type, since it's used as an index into an array. size_t
> seems like an appropriate choice there.

Hm. We do use `int` almost everywhere when iterating through an array
via `ARRAY_SIZE`, but ultimately I don't mind whether it's `int`,
`unsigned` or `size_t`.

> > +	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
> > +		if (!strcmp(refname, special_refs[i]))
> > +			return 1;
> > +
> > +	/*
> > +	 * git-rebase(1) stores its state in `rebase-apply/` or
> > +	 * `rebase-merge/`, including various reference-like bits.
> > +	 */
> > +	if (starts_with(refname, "rebase-apply/") ||
> > +	    starts_with(refname, "rebase-merge/"))
> 
> Do we care about case sensitivity here? Definitely not on case-sensitive
> filesystems, but I'm not sure about case-insensitive ones. For instance,
> on macOS, I can do:
> 
>     $ git rev-parse hEAd
> 
> and get the same value as "git rev-parse HEAD" (on my Linux workstation,
> this fails as expected).
> 
> I doubt that there are many users in the wild asking to resolve
> reBASe-APPLY/xyz, but I think that after this patch that would no longer
> work as-is, so we may want to replace this with istarts_with() instead.

In practice I'd argue that nobody is ever going to ask for something in
`rebase-apply/` outside of Git internals or scripts, and I'd expect
these to always use proper casing. So I rather lean towards a "no, we
don't care about case sensitivity".

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] refs: improve handling of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On Wed, Nov 29, 2023 at 05:14:39PM -0500, Taylor Blau wrote:
> On Wed, Nov 29, 2023 at 09:14:07AM +0100, Patrick Steinhardt wrote:
> > Patrick Steinhardt (4):
> >   wt-status: read HEAD and ORIG_HEAD via the refdb
> >   refs: propagate errno when reading special refs fails
> >   refs: complete list of special refs
> >   bisect: consistently write BISECT_EXPECTED_REV via the refdb
> >
> >  bisect.c                    | 25 +++------------
> >  builtin/bisect.c            |  8 ++---
> >  refs.c                      | 64 +++++++++++++++++++++++++++++++++++--
> >  t/t1403-show-ref.sh         |  9 ++++++
> >  t/t6030-bisect-porcelain.sh |  2 +-
> >  wt-status.c                 | 17 +++++-----
> >  6 files changed, 86 insertions(+), 39 deletions(-)
> 
> These all look pretty good to me. I had a few minor questions on the
> first three patches, but I don't think they necessarily require a
> reroll.

I agree that none of the comments really require a reroll, but I'll
address them if there will be another iteration of this patch series.

Thanks for your review! 

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] refs: complete list of special refs

[Phillip Wood]

Hi Patrick

Thanks for working on this. I've left a couple of thought below.

On 29/11/2023 08:14, Patrick Steinhardt wrote:
> +static int is_special_ref(const char *refname)
> +{
> +	/*
> +	 * Special references get written and read directly via the filesystem
> +	 * by the subsystems that create them. Thus, they must not go through
> +	 * the reference backend but must instead be read directly. It is
> +	 * arguable whether this behaviour is sensible, or whether it's simply
> +	 * a leaky abstraction enabled by us only having a single reference
> +	 * backend implementation. But at least for a subset of references it
> +	 * indeed does make sense to treat them specially:
> +	 *
> +	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
> +	 *   carries additional metadata like where it came from.
> +	 *
> +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
> +	 *   heads.
> +	 *
> +	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
> +	 *   rebases, where keeping it closely together feels sensible.

I'd really like to get away from treating these files as refs. I think 
their use as refs is purely historic and predates the reflog and 
possibly ORIG_HEAD. These days I'm not sure there is a good reason to be 
running

     git rev-parse rebase-merge/orig-head

One reason for not wanting to treat them as refs is that we do not 
handle multi-level refs that do not begin with "refs/" consistently.

     git update-ref foo/bar HEAD

succeeds and creates .git/foo/bar but

     git update-ref -d foo/bar

fails with

     error: refusing to update ref with bad name 'foo/bar'

To me it would make sense to refuse to create 'foo/bar' but allow an 
existing ref named 'foo/bar' to be deleted but the current behavior is 
the opposite of that.

I'd be quite happy to see us refuse to treat anything that fails

     if (starts_with(refname, "refs/") || refname_is_safe(refname))

as a ref but I don't know how much pain that would cause.

> +	const char * const special_refs[] = {
> +		"AUTO_MERGE",

Is there any reason to treat this specially in the long term? It points 
to a tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it 
is effectively a "normal" ref.

> +		"BISECT_EXPECTED_REV",
> +		"FETCH_HEAD",
> +		"MERGE_AUTOSTASH",

Should we be treating this as a ref? I thought it was written as an 
implementation detail of the autostash implementation rather than to 
provide a ref for users and scripts.

Best Wishes

Phillip

Re: [PATCH 0/4] refs: improve handling of special refs

[Taylor Blau]

On Thu, Nov 30, 2023 at 08:46:54AM +0100, Patrick Steinhardt wrote:
> On Wed, Nov 29, 2023 at 05:14:39PM -0500, Taylor Blau wrote:
> > These all look pretty good to me. I had a few minor questions on the
> > first three patches, but I don't think they necessarily require a
> > reroll.
>
> I agree that none of the comments really require a reroll, but I'll
> address them if there will be another iteration of this patch series.
>
> Thanks for your review!

No problem on either. I doubt that there will be another iteration of
this series since it is already good, so no need to worry too much about
these changes.

Thanks,
Taylor

Re: [PATCH 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Taylor Blau]

On Thu, Nov 30, 2023 at 08:42:48AM +0100, Patrick Steinhardt wrote:
> > Are there any other gotchas that we should be thinking about?
>
> Not that I can think of. As you say, a repository with malformed HEAD
> will run into other problems anyway. And `read_ref_full()` would return
> errors if these refs were malformed, which would cause us to exit early
> from anyway. So unless "rebase-merge/amend" and "rebase-merge/orig-head"
> contained the same kind of garbage we'd retain the same behaviour as
> before, and that shouldn't really be happening.
>
> One interesting bit is that we don't set `RESOLVE_REF_READING`, so
> `read_ref_full()` may return successfully even if the ref doesn't exist.
> But in practice this is fine given that the resulting oid would be
> cleared in that case.

Thanks for thinking through these. I agree with your reasoning and think
that this is fine as-is.

(Off-topic, but can you please trim your replies to only include the
quoted parts/context that you're replying to?)

Thanks,
Taylor

Re: [PATCH 3/4] refs: complete list of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4298 bytes --]

On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:
> Hi Patrick
> 
> Thanks for working on this. I've left a couple of thought below.
> 
> On 29/11/2023 08:14, Patrick Steinhardt wrote:
> > +static int is_special_ref(const char *refname)
> > +{
> > +	/*
> > +	 * Special references get written and read directly via the filesystem
> > +	 * by the subsystems that create them. Thus, they must not go through
> > +	 * the reference backend but must instead be read directly. It is
> > +	 * arguable whether this behaviour is sensible, or whether it's simply
> > +	 * a leaky abstraction enabled by us only having a single reference
> > +	 * backend implementation. But at least for a subset of references it
> > +	 * indeed does make sense to treat them specially:
> > +	 *
> > +	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
> > +	 *   carries additional metadata like where it came from.
> > +	 *
> > +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
> > +	 *   heads.
> > +	 *
> > +	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
> > +	 *   rebases, where keeping it closely together feels sensible.
> 
> I'd really like to get away from treating these files as refs. I think their
> use as refs is purely historic and predates the reflog and possibly
> ORIG_HEAD. These days I'm not sure there is a good reason to be running
> 
>     git rev-parse rebase-merge/orig-head
> 
> One reason for not wanting to treat them as refs is that we do not handle
> multi-level refs that do not begin with "refs/" consistently.
> 
>     git update-ref foo/bar HEAD
> 
> succeeds and creates .git/foo/bar but
> 
>     git update-ref -d foo/bar
> 
> fails with
> 
>     error: refusing to update ref with bad name 'foo/bar'
> 
> To me it would make sense to refuse to create 'foo/bar' but allow an
> existing ref named 'foo/bar' to be deleted but the current behavior is the
> opposite of that.
> 
> I'd be quite happy to see us refuse to treat anything that fails
> 
>     if (starts_with(refname, "refs/") || refname_is_safe(refname))
> 
> as a ref but I don't know how much pain that would cause.

Well, we already do use these internally as references, but I don't
disagree with you. I think the current state is extremely confusing,
which is why my first approach was to simply document what falls into
the category of these "special" references.

In my mind, this patch series here is a first step towards addressing
the problem more generally. For now it is more or less only documenting
_what_ is a special ref and why they are special, while also ensuring
that these refs are compatible with the reftable backend. But once this
lands, I'd certainly want to see us continue to iterate on this.

Most importantly, I'd love to see us address two issues:

  - Start to refuse writing these special refs via the refdb so that
    the rules I've now layed out are also enforced. This would also
    address your point about things being inconsistent.

  - Gradually reduce the list of special refs so that they are reduced
    to a bare minimum and so that most refs are simply that, a normal
    ref.

> > +	const char * const special_refs[] = {
> > +		"AUTO_MERGE",
> 
> Is there any reason to treat this specially in the long term? It points to a
> tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it is
> effectively a "normal" ref.

No, I'd love to see this and others converted to become a normal ref
eventually. The goal of this patch series was mostly to document what we
already have, and address those cases which are inconsistent with the
new rules. But I'd be happy to convert more of these special refs to
become normal refs after it lands.

> > +		"BISECT_EXPECTED_REV",
> > +		"FETCH_HEAD",
> > +		"MERGE_AUTOSTASH",
> 
> Should we be treating this as a ref? I thought it was written as an
> implementation detail of the autostash implementation rather than to provide
> a ref for users and scripts.

Yes, we have to in the context of the reftable backend. There's a bunch
of tests that exercise our ability to parse this as a ref, and they
would otherwise fail with the reftable backend.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] refs: complete list of special refs

[Phillip Wood]

Hi Patrick

On 01/12/2023 06:43, Patrick Steinhardt wrote:
> On Thu, Nov 30, 2023 at 03:42:06PM +0000, Phillip Wood wrote:
>> Hi Patrick
>>
>> Thanks for working on this. I've left a couple of thought below.
>>
>> On 29/11/2023 08:14, Patrick Steinhardt wrote:
>>> +static int is_special_ref(const char *refname)
>>> +{
>>> +	/*
>>> +	 * Special references get written and read directly via the filesystem
>>> +	 * by the subsystems that create them. Thus, they must not go through
>>> +	 * the reference backend but must instead be read directly. It is
>>> +	 * arguable whether this behaviour is sensible, or whether it's simply
>>> +	 * a leaky abstraction enabled by us only having a single reference
>>> +	 * backend implementation. But at least for a subset of references it
>>> +	 * indeed does make sense to treat them specially:
>>> +	 *
>>> +	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
>>> +	 *   carries additional metadata like where it came from.
>>> +	 *
>>> +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
>>> +	 *   heads.
>>> +	 *
>>> +	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
>>> +	 *   rebases, where keeping it closely together feels sensible.
>>
>> I'd really like to get away from treating these files as refs. I think their
>> use as refs is purely historic and predates the reflog and possibly
>> ORIG_HEAD. These days I'm not sure there is a good reason to be running
>>
>>      git rev-parse rebase-merge/orig-head
>>
>> One reason for not wanting to treat them as refs is that we do not handle
>> multi-level refs that do not begin with "refs/" consistently.
>>
>>      git update-ref foo/bar HEAD
>>
>> succeeds and creates .git/foo/bar but
>>
>>      git update-ref -d foo/bar
>>
>> fails with
>>
>>      error: refusing to update ref with bad name 'foo/bar'
>>
>> To me it would make sense to refuse to create 'foo/bar' but allow an
>> existing ref named 'foo/bar' to be deleted but the current behavior is the
>> opposite of that.
>>
>> I'd be quite happy to see us refuse to treat anything that fails
>>
>>      if (starts_with(refname, "refs/") || refname_is_safe(refname))
>>
>> as a ref but I don't know how much pain that would cause.
> 
> Well, we already do use these internally as references, but I don't
> disagree with you.

I should have been clearer that I was talking about the refs starting 
"rebase-*" rather than FETCH_HEAD and MERGE_HEAD. As a user find it 
convenient to be able to run "git fetch ... && git log -p FETCH_HEAD" 
even if the implementation is a bit ugly. As far as I can see we do not 
use "rebase-(apply|merge)/(orig-head|amend|autostash)" as a ref in our 
code or tests.

> I think the current state is extremely confusing,
> which is why my first approach was to simply document what falls into
> the category of these "special" references.

That's certainly a good place to start

> In my mind, this patch series here is a first step towards addressing
> the problem more generally. For now it is more or less only documenting
> _what_ is a special ref and why they are special, while also ensuring
> that these refs are compatible with the reftable backend. But once this
> lands, I'd certainly want to see us continue to iterate on this.
> 
> Most importantly, I'd love to see us address two issues:
> 
>    - Start to refuse writing these special refs via the refdb so that
>      the rules I've now layed out are also enforced. This would also
>      address your point about things being inconsistent.
> 
>    - Gradually reduce the list of special refs so that they are reduced
>      to a bare minimum and so that most refs are simply that, a normal
>      ref.

That sounds like a good plan

>>> +	const char * const special_refs[] = {
>>> +		"AUTO_MERGE",
>>
>> Is there any reason to treat this specially in the long term? It points to a
>> tree rather than a commit but unlike MERGE_HEAD and FETCH_HEAD it is
>> effectively a "normal" ref.
> 
> No, I'd love to see this and others converted to become a normal ref
> eventually. The goal of this patch series was mostly to document what we
> already have, and address those cases which are inconsistent with the
> new rules. But I'd be happy to convert more of these special refs to
> become normal refs after it lands.

That's great

>>> +		"BISECT_EXPECTED_REV",
>>> +		"FETCH_HEAD",
>>> +		"MERGE_AUTOSTASH",
>>
>> Should we be treating this as a ref? I thought it was written as an
>> implementation detail of the autostash implementation rather than to provide
>> a ref for users and scripts.
> 
> Yes, we have to in the context of the reftable backend. There's a bunch
> of tests that exercise our ability to parse this as a ref, and they
> would otherwise fail with the reftable backend.

Ah, looking at the the man page for "git merge" it seems we do actually 
document the existence of MERGE_AUTOSTASH so it is not just an 
implementation detail after all.

Best Wishes

Phillip

[PATCH v2 0/4] refs: improve handling of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4823 bytes --]

Hi,

this is the second version of my patch series that aims to improve our
handling of special refs. This patch series is in preparation for the
upcoming reftable backend.

Changes compared to v1:

  - Patch 1: Stop needlessly resetting `errno` before calling
    `strbuf_read_file()`. Also, clean up state we create in our test
    repos properly.

  - Patch 3: The discussion with Phillip made me reevaluate my claim
    that "rebase-apply/" and "rebase-merge/" contain special refs.
    Indeed they don't, all files in there seem to be exclusively
    read and written via the filesystem without ever going via the
    refdb. I've thus dropped them from the set of special refs.

  - Patch 4: The array of static refs is now static.

Thanks for your reviews!

Patrick

Patrick Steinhardt (4):
  wt-status: read HEAD and ORIG_HEAD via the refdb
  refs: propagate errno when reading special refs fails
  refs: complete list of special refs
  bisect: consistently write BISECT_EXPECTED_REV via the refdb

 bisect.c                    | 25 +++-------------
 builtin/bisect.c            |  8 ++---
 refs.c                      | 59 +++++++++++++++++++++++++++++++++++--
 t/t1403-show-ref.sh         | 10 +++++++
 t/t6030-bisect-porcelain.sh |  2 +-
 wt-status.c                 | 17 ++++++-----
 6 files changed, 82 insertions(+), 39 deletions(-)

Range-diff against v1:
1:  35b74eb972 = 1:  1db3eb3945 wt-status: read HEAD and ORIG_HEAD via the refdb
2:  691552a17e ! 2:  24032a62e5 refs: propagate errno when reading special refs fails
    @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
      	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
      
     -	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
    -+	errno = 0;
    -+
     +	if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
     +		*failure_errno = errno;
      		goto done;
    @@ t/t1403-show-ref.sh: test_expect_success '--exists with directory fails with gen
     +'
     +
     +test_expect_success '--exists with existing special ref' '
    ++	test_when_finished "rm .git/FETCH_HEAD" &&
     +	git rev-parse HEAD >.git/FETCH_HEAD &&
     +	git show-ref --exists FETCH_HEAD
     +'
3:  0e38103114 ! 3:  3dd9089fd5 refs: complete list of special refs
    @@ refs.c: static int refs_read_special_head(struct ref_store *ref_store,
     +	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
     +	 *   heads.
     +	 *
    -+	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
    -+	 *   rebases, where keeping it closely together feels sensible.
    -+	 *
     +	 * There are some exceptions that you might expect to see on this list
     +	 * but which are handled exclusively via the reference backend:
     +	 *
     +	 * - CHERRY_PICK_HEAD
    ++	 *
     +	 * - HEAD
    ++	 *
     +	 * - ORIG_HEAD
     +	 *
    ++	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
    ++	 *   rebases, including some reference-like files. These are
    ++	 *   exclusively read and written via the filesystem and never go
    ++	 *   through the refdb.
    ++	 *
     +	 * Writing or deleting references must consistently go either through
     +	 * the filesystem (special refs) or through the reference backend
     +	 * (normal ones).
     +	 */
    -+	const char * const special_refs[] = {
    ++	static const char * const special_refs[] = {
     +		"AUTO_MERGE",
     +		"BISECT_EXPECTED_REV",
     +		"FETCH_HEAD",
     +		"MERGE_AUTOSTASH",
     +		"MERGE_HEAD",
     +	};
    -+	int i;
    ++	size_t i;
     +
     +	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
     +		if (!strcmp(refname, special_refs[i]))
     +			return 1;
     +
    -+	/*
    -+	 * git-rebase(1) stores its state in `rebase-apply/` or
    -+	 * `rebase-merge/`, including various reference-like bits.
    -+	 */
    -+	if (starts_with(refname, "rebase-apply/") ||
    -+	    starts_with(refname, "rebase-merge/"))
    -+		return 1;
    -+
     +	return 0;
     +}
     +
4:  c7ab26fb7e ! 4:  4a4447a3f5 bisect: consistently write BISECT_EXPECTED_REV via the refdb
    @@ refs.c: static int is_special_ref(const char *refname)
      	 * but which are handled exclusively via the reference backend:
      	 *
     +	 * - BISECT_EXPECTED_REV
    ++	 *
      	 * - CHERRY_PICK_HEAD
    + 	 *
      	 * - HEAD
    - 	 * - ORIG_HEAD
     @@ refs.c: static int is_special_ref(const char *refname)
      	 */
    - 	const char * const special_refs[] = {
    + 	static const char * const special_refs[] = {
      		"AUTO_MERGE",
     -		"BISECT_EXPECTED_REV",
      		"FETCH_HEAD",

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2585 bytes --]

We read both the HEAD and ORIG_HEAD references directly from the
filesystem in order to figure out whether we're currently splitting a
commit. If both of the following are true:

  - HEAD points to the same object as "rebase-merge/amend".

  - ORIG_HEAD points to the same object as "rebase-merge/orig-head".

Then we are currently splitting commits.

The current code only works by chance because we only have a single
reference backend implementation. Refactor it to instead read both refs
via the refdb layer so that we'll also be compatible with alternate
reference backends.

Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
because we didn't resolve symrefs before either, and in practice none of
the refs in "rebase-merge/" would be symbolic. We thus don't want to
resolve symrefs with the new code either to retain the old behaviour.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 wt-status.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..fe9e590b80 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
 static int split_commit_in_progress(struct wt_status *s)
 {
 	int split_in_progress = 0;
-	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+	struct object_id head_oid, orig_head_oid;
+	char *rebase_amend, *rebase_orig_head;
 
 	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
 	    !s->branch || strcmp(s->branch, "HEAD"))
 		return 0;
 
-	head = read_line_from_git_path("HEAD");
-	orig_head = read_line_from_git_path("ORIG_HEAD");
+	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
+	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
+		return 0;
+
 	rebase_amend = read_line_from_git_path("rebase-merge/amend");
 	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
 
-	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+	if (!rebase_amend || !rebase_orig_head)
 		; /* fall through, no split in progress */
 	else if (!strcmp(rebase_amend, rebase_orig_head))
-		split_in_progress = !!strcmp(head, rebase_amend);
-	else if (strcmp(orig_head, rebase_orig_head))
+		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
+	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
 		split_in_progress = 1;
 
-	free(head);
-	free(orig_head);
 	free(rebase_amend);
 	free(rebase_orig_head);
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/4] refs: propagate errno when reading special refs fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

Some refs in Git are more special than others due to reasons explained
in the next commit. These refs are read via `refs_read_special_head()`,
but this function doesn't behave the same as when we try to read a
normal ref. Most importantly, we do not propagate `failure_errno` in the
case where the reference does not exist, which is behaviour that we rely
on in many parts of Git.

Fix this bug by propagating errno when `strbuf_read_file()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c              |  4 +++-
 t/t1403-show-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index fcae5dddc6..00e72a2abf 100644
--- a/refs.c
+++ b/refs.c
@@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	int result = -1;
 	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
 
-	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
+	if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
+		*failure_errno = errno;
 		goto done;
+	}
 
 	result = parse_loose_ref_contents(content.buf, oid, referent, type,
 					  failure_errno);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..66e6e77fa9 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '
 	test_cmp expect err
 '
 
+test_expect_success '--exists with non-existent special ref' '
+	test_expect_code 2 git show-ref --exists FETCH_HEAD
+'
+
+test_expect_success '--exists with existing special ref' '
+	test_when_finished "rm .git/FETCH_HEAD" &&
+	git rev-parse HEAD >.git/FETCH_HEAD &&
+	git show-ref --exists FETCH_HEAD
+'
+
 test_done
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/4] refs: complete list of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4042 bytes --]

We have some references that are more special than others. The reason
for them being special is that they either do not follow the usual
format of references, or that they are written to the filesystem
directly by the respective owning subsystem and thus circumvent the
reference backend.

This works perfectly fine right now because the reffiles backend will
know how to read those refs just fine. But with the prospect of gaining
a new reference backend implementation we need to be a lot more careful
here:

  - We need to make sure that we are consistent about how those refs are
    written. They must either always be written via the filesystem, or
    they must always be written via the reference backend. Any mixture
    will lead to inconsistent state.

  - We need to make sure that such special refs are always handled
    specially when reading them.

We're already mostly good with regard to the first item, except for
`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
But the current list of special refs is missing a lot of refs that
really should be treated specially. Right now, we only treat
`FETCH_HEAD` and `MERGE_HEAD` specially here.

Introduce a new function `is_special_ref()` that contains all current
instances of special refs to fix the reading path.

Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 00e72a2abf..8fe34d51e4 100644
--- a/refs.c
+++ b/refs.c
@@ -1820,15 +1820,65 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	return result;
 }
 
+static int is_special_ref(const char *refname)
+{
+	/*
+	 * Special references get written and read directly via the filesystem
+	 * by the subsystems that create them. Thus, they must not go through
+	 * the reference backend but must instead be read directly. It is
+	 * arguable whether this behaviour is sensible, or whether it's simply
+	 * a leaky abstraction enabled by us only having a single reference
+	 * backend implementation. But at least for a subset of references it
+	 * indeed does make sense to treat them specially:
+	 *
+	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
+	 *   carries additional metadata like where it came from.
+	 *
+	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
+	 *   heads.
+	 *
+	 * There are some exceptions that you might expect to see on this list
+	 * but which are handled exclusively via the reference backend:
+	 *
+	 * - CHERRY_PICK_HEAD
+	 *
+	 * - HEAD
+	 *
+	 * - ORIG_HEAD
+	 *
+	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
+	 *   rebases, including some reference-like files. These are
+	 *   exclusively read and written via the filesystem and never go
+	 *   through the refdb.
+	 *
+	 * Writing or deleting references must consistently go either through
+	 * the filesystem (special refs) or through the reference backend
+	 * (normal ones).
+	 */
+	static const char * const special_refs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"FETCH_HEAD",
+		"MERGE_AUTOSTASH",
+		"MERGE_HEAD",
+	};
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
+		if (!strcmp(refname, special_refs[i]))
+			return 1;
+
+	return 0;
+}
+
 int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		      struct object_id *oid, struct strbuf *referent,
 		      unsigned int *type, int *failure_errno)
 {
 	assert(failure_errno);
-	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
+	if (is_special_ref(refname))
 		return refs_read_special_head(ref_store, refname, oid, referent,
 					      type, failure_errno);
-	}
 
 	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
 					   type, failure_errno);
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5649 bytes --]

We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem
and via the refdb, which violates the newly established rules for how
special refs must be treated. This works alright in practice with the
reffiles reference backend, but will cause bugs once we gain additional
backends.

Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb
so that it is no longer a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  3 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/bisect.c b/bisect.c
index 1be8e0a271..985b96ed13 100644
--- a/bisect.c
+++ b/bisect.c
@@ -471,7 +471,6 @@ static int read_bisect_refs(void)
 }
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,
 
 static int is_expected_rev(const struct object_id *oid)
 {
-	const char *filename = git_path_bisect_expected_rev();
-	struct stat st;
-	struct strbuf str = STRBUF_INIT;
-	FILE *fp;
-	int res = 0;
-
-	if (stat(filename, &st) || !S_ISREG(st.st_mode))
+	struct object_id expected_oid;
+	if (read_ref("BISECT_EXPECTED_REV", &expected_oid))
 		return 0;
-
-	fp = fopen_or_warn(filename, "r");
-	if (!fp)
-		return 0;
-
-	if (strbuf_getline_lf(&str, fp) != EOF)
-		res = !strcmp(str.buf, oid_to_hex(oid));
-
-	strbuf_release(&str);
-	fclose(fp);
-
-	return res;
+	return oideq(oid, &expected_oid);
 }
 
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
 	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF);
 	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
-	unlink_or_warn(git_path_bisect_expected_rev());
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());
 	unlink_or_warn(git_path_bisect_names());
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 35938b05fd..4e2c43daf5 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -17,7 +17,6 @@
 #include "revision.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 	const char *state;
 	int i, verify_expected = 1;
 	struct object_id oid, expected;
-	struct strbuf buf = STRBUF_INIT;
 	struct oid_array revs = OID_ARRAY_INIT;
 
 	if (!argc)
@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 		oid_array_append(&revs, &commit->object.oid);
 	}
 
-	if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
-	    get_oid_hex(buf.buf, &expected) < 0)
+	if (read_ref("BISECT_EXPECTED_REV", &expected))
 		verify_expected = 0; /* Ignore invalid file contents */
-	strbuf_release(&buf);
 
 	for (i = 0; i < revs.nr; i++) {
 		if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {
@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 		}
 		if (verify_expected && !oideq(&revs.oid[i], &expected)) {
 			unlink_or_warn(git_path_bisect_ancestors_ok());
-			unlink_or_warn(git_path_bisect_expected_rev());
+			delete_ref(NULL, "BISECT_EXPECTED_REV", NULL, REF_NO_DEREF);
 			verify_expected = 0;
 		}
 	}
diff --git a/refs.c b/refs.c
index 8fe34d51e4..c76ce86bef 100644
--- a/refs.c
+++ b/refs.c
@@ -1840,6 +1840,8 @@ static int is_special_ref(const char *refname)
 	 * There are some exceptions that you might expect to see on this list
 	 * but which are handled exclusively via the reference backend:
 	 *
+	 * - BISECT_EXPECTED_REV
+	 *
 	 * - CHERRY_PICK_HEAD
 	 *
 	 * - HEAD
@@ -1857,7 +1859,6 @@ static int is_special_ref(const char *refname)
 	 */
 	static const char * const special_refs[] = {
 		"AUTO_MERGE",
-		"BISECT_EXPECTED_REV",
 		"FETCH_HEAD",
 		"MERGE_AUTOSTASH",
 		"MERGE_HEAD",
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2a5b7d8379..792c1504bc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	git bisect bad $HASH4 &&
 	git bisect reset &&
 	test -z "$(git for-each-ref "refs/bisect/*")" &&
-	test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
+	test_ref_missing BISECT_EXPECTED_REV &&
 	test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
 	test_path_is_missing ".git/BISECT_LOG" &&
 	test_path_is_missing ".git/BISECT_RUN" &&
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The current code only works by chance because we only have a single
> reference backend implementation. Refactor it to instead read both refs
> via the refdb layer so that we'll also be compatible with alternate
> reference backends.

"via the refdb" -> "via the refs API" or something here and on the
title, and possibly elsewhere in the proposed log messages and
in-code comments in patches in this series, as I've never seen a
word "refdb" used in the context of this project.

I agree it is bad manners to be intimate with the implementation
details of the how files-backend stores HEAD and ORIG_HEAD.

> Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
> because we didn't resolve symrefs before either, and in practice none of
> the refs in "rebase-merge/" would be symbolic. We thus don't want to
> resolve symrefs with the new code either to retain the old behaviour.

Good to see a rewrite being careful like this.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  wt-status.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 9f45bf6949..fe9e590b80 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
>  static int split_commit_in_progress(struct wt_status *s)
>  {
>  	int split_in_progress = 0;
> -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> +	struct object_id head_oid, orig_head_oid;
> +	char *rebase_amend, *rebase_orig_head;
>  
>  	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
>  	    !s->branch || strcmp(s->branch, "HEAD"))
>  		return 0;
>  
> -	head = read_line_from_git_path("HEAD");
> -	orig_head = read_line_from_git_path("ORIG_HEAD");
> +	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> +	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
> +		return 0;
> +

This made me wonder if we have changed behaviour when on an unborn
branch.  In such a case, the original most likely would have read
"ref: blah" in "head" and compared it with "rebase_amend", which
would be a good way to ensure they would not match.  I would not
know offhand what the updated code would do, but head_oid would be
uninitialized in such a case, so ...?

>  	rebase_amend = read_line_from_git_path("rebase-merge/amend");
>  	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
>  
> -	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
> +	if (!rebase_amend || !rebase_orig_head)
>  		; /* fall through, no split in progress */
>  	else if (!strcmp(rebase_amend, rebase_orig_head))
> -		split_in_progress = !!strcmp(head, rebase_amend);
> -	else if (strcmp(orig_head, rebase_orig_head))
> +		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
> +	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
>  		split_in_progress = 1;
>  
> -	free(head);
> -	free(orig_head);
>  	free(rebase_amend);
>  	free(rebase_orig_head);

Re: [PATCH v2 2/4] refs: propagate errno when reading special refs fails

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Some refs in Git are more special than others due to reasons explained
> in the next commit. These refs are read via `refs_read_special_head()`,
> but this function doesn't behave the same as when we try to read a
> normal ref. Most importantly, we do not propagate `failure_errno` in the
> case where the reference does not exist, which is behaviour that we rely
> on in many parts of Git.
>
> Fix this bug by propagating errno when `strbuf_read_file()` fails.

Hmph, I thought, judging from what [1/4] did, that your plan was to
use the refs API, instead of peeking directly into the filesystem,
to access these pseudo refs, and am a bit puzzled if it makes all
that much difference to fix errno handling while still reading
directly from the filesystem.  Perhaps such a conversion happens in
later steps of this series (or a follow on series after this series
lands)?

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c              |  4 +++-
>  t/t1403-show-ref.sh | 10 ++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index fcae5dddc6..00e72a2abf 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  	int result = -1;
>  	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
>  
> -	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
> +	if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
> +		*failure_errno = errno;
>  		goto done;
> +	}
>  
>  	result = parse_loose_ref_contents(content.buf, oid, referent, type,
>  					  failure_errno);
> diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
> index b50ae6fcf1..66e6e77fa9 100755
> --- a/t/t1403-show-ref.sh
> +++ b/t/t1403-show-ref.sh
> @@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '
>  	test_cmp expect err
>  '
>  
> +test_expect_success '--exists with non-existent special ref' '
> +	test_expect_code 2 git show-ref --exists FETCH_HEAD
> +'
> +
> +test_expect_success '--exists with existing special ref' '
> +	test_when_finished "rm .git/FETCH_HEAD" &&
> +	git rev-parse HEAD >.git/FETCH_HEAD &&
> +	git show-ref --exists FETCH_HEAD
> +'
> +
>  test_done

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Ramsay Jones]

On 12/12/2023 20:24, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
>> The current code only works by chance because we only have a single
>> reference backend implementation. Refactor it to instead read both refs
>> via the refdb layer so that we'll also be compatible with alternate
>> reference backends.
> 
> "via the refdb" -> "via the refs API" or something here and on the
> title, and possibly elsewhere in the proposed log messages and
> in-code comments in patches in this series, as I've never seen a
> word "refdb" used in the context of this project.
> 
> I agree it is bad manners to be intimate with the implementation
> details of the how files-backend stores HEAD and ORIG_HEAD.

Hmm, I have never thought of the 'pseudo-refs' as being a part of
the 'reference database' at all. ;)

We seem to have pseudo-refs, special pseudo-refs and (recently)
ex-pseudo-refs!

This patch (well series) changes the 'status' of some, *but not all*,
pseudo-refs; some graduate to full-blown refs stored as part of *a*
reference database (ie reftable).

As far as I recall, this has not been discussed on the ML. Why are
only some chosen to become 'full' refs and others not? This is not
discussed in any of the commit messages.

The '.invalid' HEAD hack featured in a recent completion patch as well.
[If this is because the JAVA implementation does it this way, I think
it needs some thought before including it in the Git project].

Anyway, I haven't had the time to study the details here, so please
ignore my uninformed ramblings!

ATB,
Ramsay Jones

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> "via the refdb" -> "via the refs API" or something here and on the
>> title, and possibly elsewhere in the proposed log messages and
>> in-code comments in patches in this series, as I've never seen a
>> word "refdb" used in the context of this project.
>> 
>> I agree it is bad manners to be intimate with the implementation
>> details of the how files-backend stores HEAD and ORIG_HEAD.
>
> Hmm, I have never thought of the 'pseudo-refs' as being a part of
> the 'reference database' at all. ;)

Me neither, but once you start thinking about getting rid of the
need to use one-file-per-ref filesystem, being able to maintain all
refs, including the pseudo refs, in one r/w store backend, becomes a
very tempting goal.  From that point of view, I do not have problem
with the idea to move _all_ pseudorefs to reftable.

But I do have reservations on what Patrick, and the code he
inherited from Han-Wen, calls "special refs" (which is not defined
in the glossary at all), namely, refs.c:is_special_ref() and its
callers.  Neither am I very much sympathetic to the hardcoded list
of "known" pseudorefs, refs.c:pseudorefs[].  I cannot quite see why
we need anything more than

    any string that passes refs.c:is_pseudoref_syntax() is a
    pseudoref, is per worktree, and ref backends can store them like
    any other refs.  Many of them have specific meaning and uses
    (e.g. HEAD is "the current branch").

Enumerating existing pseudorefs in files backend may need to
opendir(".git") + readdir() filtered with is_pseudoref_syntax(),
and a corresponding implementation for reftable backend may be much
simpler (because there won't be "other cruft" stored there, unlike
files backend that needs to worry about files that are not refs,
like ".git/config" file.

> We seem to have pseudo-refs, special pseudo-refs and (recently)
> ex-pseudo-refs!
>
> This patch (well series) changes the 'status' of some, *but not all*,
> pseudo-refs; some graduate to full-blown refs stored as part of *a*
> reference database (ie reftable).

Yeah, that leaves bad taste in my mouth, too.

Re: [PATCH v2 2/4] refs: propagate errno when reading special refs fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On Tue, Dec 12, 2023 at 12:28:49PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Some refs in Git are more special than others due to reasons explained
> > in the next commit. These refs are read via `refs_read_special_head()`,
> > but this function doesn't behave the same as when we try to read a
> > normal ref. Most importantly, we do not propagate `failure_errno` in the
> > case where the reference does not exist, which is behaviour that we rely
> > on in many parts of Git.
> >
> > Fix this bug by propagating errno when `strbuf_read_file()` fails.
> 
> Hmph, I thought, judging from what [1/4] did, that your plan was to
> use the refs API, instead of peeking directly into the filesystem,
> to access these pseudo refs, and am a bit puzzled if it makes all
> that much difference to fix errno handling while still reading
> directly from the filesystem.  Perhaps such a conversion happens in
> later steps of this series (or a follow on series after this series
> lands)?

Yes, that's ultimately the goal. The patch series here is only setting
the stage by recording what we have, and addressing cases where we are
inconsistently accessing refs via the filesystem _and_ via the ref API.
Once this lands I do want create a follow up patch series that converts
all refs to be non-special to the extent possible.

I say "to the extent possible" though because in the end there will be
two refs that we must continue to treat specially: FETCH_HEAD and
MERGE_HEAD, which we were treating special before this patch series
already. Both of these are not normal refs because they may contain
multiple values with annotations, so they will need to stay special.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]

On Tue, Dec 12, 2023 at 04:36:24PM -0800, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
> >> "via the refdb" -> "via the refs API" or something here and on the
> >> title, and possibly elsewhere in the proposed log messages and
> >> in-code comments in patches in this series, as I've never seen a
> >> word "refdb" used in the context of this project.
> >> 
> >> I agree it is bad manners to be intimate with the implementation
> >> details of the how files-backend stores HEAD and ORIG_HEAD.
> >
> > Hmm, I have never thought of the 'pseudo-refs' as being a part of
> > the 'reference database' at all. ;)
> 
> Me neither, but once you start thinking about getting rid of the
> need to use one-file-per-ref filesystem, being able to maintain all
> refs, including the pseudo refs, in one r/w store backend, becomes a
> very tempting goal.  From that point of view, I do not have problem
> with the idea to move _all_ pseudorefs to reftable.

Yes, we're in agreement.

> But I do have reservations on what Patrick, and the code he
> inherited from Han-Wen, calls "special refs" (which is not defined
> in the glossary at all), namely, refs.c:is_special_ref() and its
> callers.

I do not want to add "special refs" to the glossary because ultimately
they should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD.
Once we're there we can of course discuss whether we want to explicitly
point them out in the glossary and give them a special name.

> Neither am I very much sympathetic to the hardcoded list
> of "known" pseudorefs, refs.c:pseudorefs[].  I cannot quite see why
> we need anything more than


>     any string that passes refs.c:is_pseudoref_syntax() is a
>     pseudoref, is per worktree, and ref backends can store them like
>     any other refs.  Many of them have specific meaning and uses
>     (e.g. HEAD is "the current branch").
> 
> Enumerating existing pseudorefs in files backend may need to
> opendir(".git") + readdir() filtered with is_pseudoref_syntax(),
> and a corresponding implementation for reftable backend may be much
> simpler (because there won't be "other cruft" stored there, unlike
> files backend that needs to worry about files that are not refs,
> like ".git/config" file.
> 
> > We seem to have pseudo-refs, special pseudo-refs and (recently)
> > ex-pseudo-refs!
> >
> > This patch (well series) changes the 'status' of some, *but not all*,
> > pseudo-refs; some graduate to full-blown refs stored as part of *a*
> > reference database (ie reftable).
> 
> Yeah, that leaves bad taste in my mouth, too.

I'm taking an iterative approach to things, which means that we're at
times going to be in an in-between state. I want to avoid changing too
many things at once and overwhelming potential reviewers. But I realize
that I should've done a better job of explaining that this patch series
is not the end goal, but rather a step towards that goal.

The patch series at hand merely records the status quo and rectifies any
inconsistencies we have with accessing such "special" refs. The natural
next step here would be to reduce the list of special refs (like e.g. we
do in patch 4) so that in the end it will only contain those refs which
really are special (FETCH_HEAD, MERGE_HEAD).

Please let me know in case you strongly disagree with my iterative
approach, or whether the issue is rather that I didn't make myself
sufficiently clear.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Me neither, but once you start thinking about getting rid of the
>> need to use one-file-per-ref filesystem, being able to maintain all
>> refs, including the pseudo refs, in one r/w store backend, becomes a
>> very tempting goal.  From that point of view, I do not have problem
>> with the idea to move _all_ pseudorefs to reftable.
>
> Yes, we're in agreement.
>
>> But I do have reservations on what Patrick, and the code he
>> inherited from Han-Wen, calls "special refs" (which is not defined
>> in the glossary at all), namely, refs.c:is_special_ref() and its
>> callers.
>
> I do not want to add "special refs" to the glossary because ultimately
> they should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD.
> Once we're there we can of course discuss whether we want to explicitly
> point them out in the glossary and give them a special name.

OK, I somehow got a (wrong) impression that you are very close to
the finish line.  If the intention is to leave many others still in
the "special" category (for only the reasons of inertia), with a
vision that some selected few must remain "special" with their own
good reasons, then I am perfectly fine.

Thanks.

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]

On Wed, Dec 13, 2023 at 07:15:33AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Me neither, but once you start thinking about getting rid of the
> >> need to use one-file-per-ref filesystem, being able to maintain all
> >> refs, including the pseudo refs, in one r/w store backend, becomes a
> >> very tempting goal.  From that point of view, I do not have problem
> >> with the idea to move _all_ pseudorefs to reftable.
> >
> > Yes, we're in agreement.
> >
> >> But I do have reservations on what Patrick, and the code he
> >> inherited from Han-Wen, calls "special refs" (which is not defined
> >> in the glossary at all), namely, refs.c:is_special_ref() and its
> >> callers.
> >
> > I do not want to add "special refs" to the glossary because ultimately
> > they should go away, with two exceptions: FETCH_HEAD and MERGE_HEAD.
> > Once we're there we can of course discuss whether we want to explicitly
> > point them out in the glossary and give them a special name.
> 
> OK, I somehow got a (wrong) impression that you are very close to
> the finish line.

You mean with the reftable backend? I indeed am quite close, I've just
finished the last prerequisite ("extensions.refFormat" and related
tooling) today. I will send that patch series upstream for review once
my patches that fix repo initialization with git-clone(1) land in the
"next" branch. The current state at [1] passes CI, even though there
will of course still be bugs which aren't covered by the test suite.

So once all prerequisites that are currently in flight plus the pending
"extensions.refFormat" series have landed I will send the reftable
backend implementation in for review. If things continue to go smoothly
I expect that this may happen at the end of January/start of February.

Anyway. This patch series here is in fact already sufficient to make
reftables work with those special refs. The only thing that we require
in this context is that refs are either exclusively routed through the
filesystem, or exclusively routed through the ref API. If that property
holds then things work just fine.

But still, I do want to clean up the remaining special refs regardless
of that, even though it is not a mandatory prerequisite. I find that the
current state is just plain confusing with all these special cases, and
I'd really love for it to be simplified. Also, I think there is benefit
in having those refs in reftables because it does allow for proper
atomic updates.

> If the intention is to leave many others still in
> the "special" category (for only the reasons of inertia), with a
> vision that some selected few must remain "special" with their own
> good reasons, then I am perfectly fine.

Okay.

Patrick

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/58

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3621 bytes --]

On Tue, Dec 12, 2023 at 12:24:24PM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > diff --git a/wt-status.c b/wt-status.c
> > index 9f45bf6949..fe9e590b80 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -1295,26 +1295,27 @@ static char *read_line_from_git_path(const char *filename)
> >  static int split_commit_in_progress(struct wt_status *s)
> >  {
> >  	int split_in_progress = 0;
> > -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> > +	struct object_id head_oid, orig_head_oid;
> > +	char *rebase_amend, *rebase_orig_head;
> >  
> >  	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
> >  	    !s->branch || strcmp(s->branch, "HEAD"))
> >  		return 0;
> >  
> > -	head = read_line_from_git_path("HEAD");
> > -	orig_head = read_line_from_git_path("ORIG_HEAD");
> > +	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
> > +	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
> > +		return 0;
> > +
> 
> This made me wonder if we have changed behaviour when on an unborn
> branch.  In such a case, the original most likely would have read
> "ref: blah" in "head" and compared it with "rebase_amend", which
> would be a good way to ensure they would not match.  I would not
> know offhand what the updated code would do, but head_oid would be
> uninitialized in such a case, so ...?

The code flow goes as following:

  1. We call `read_ref_full()`, which itself calls
     `refs_resolve_ref_unsafe()`.

  2. It calls `refs_read_raw_ref()`, which succeeds and finds the
     symref.

  3. We notice that this is a symref and that `RESOLVE_REF_NO_RECURSE`
     is set. We thus clear the object ID and return the name of the
     symref target.

  4. Back in `read_ref_full()` we see that `refs_resolve_ref_unsafe()`
     returns the symref target, which we interpret as successful lookup.
     We thus return `0`.

  5. Now we look up "rebase-merge/{amend,orig-head}" and end up
     comparing whatever they contain with the cleared OID resolved from
     HEAD/ORIG_HEAD.

So the OID would not be uninitialized but the zero OID. Now:

  - "rebase-merge/amend" always contains the result of `repo_get_oid()`
    and never contains the zero OID.

  - "rebase-merge/orig-head" may contain the zero OID when there was no
    ORIG_HEAD at the time of starting a rebase or in case it did not
    resolve

So... if ORIG_HEAD was rewritten to be a symref pointing into nirvana
between starting the rebase and calling into "wt-status.c", and when
ORIG_HEAD didn't exist at the time of starting the rebase, then we might
now wrongly report that splitting was in progress.

In other cases the old code was actually doing the wrong thing. Suppose
that ORIG_HEAD was a dangling symref, then we'd have written the zero
OID into "rebase-merge/orig-head". But when calling into "wt-status.c"
now we read the still-dangling symref value and notice that the zero OID
is different than "ref: refs/heads/dangling".

I dunno. It feels like this is one of many cases where as you start to
think deeply about how things behave you realize that it's been broken
all along. On the other hand, I doubt there was even a single user who
ever experienced this issue. It often just needs to be correct enough.

I think the best way to go about this is to check for `REF_ISSSYMREF`
and exit early in that case. We only want to compare direct refs, so
this is closer to the old behaviour and should even fix edge cases like
the above.

Will update.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 0/4] refs: improve handling of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4913 bytes --]

Hi,

this is the third version of my patch series to improve handling of
special refs.

Changes combpared to v2:

  - Patch 1: We're now more careful around missing and symbolic refs.

  - Patch 3: Document in the commit message that the extended list of
    special refs is not intended to stay like this forever.

Thanks for your reviews and the discussion!

Patrick

Patrick Steinhardt (4):
  wt-status: read HEAD and ORIG_HEAD via the refdb
  refs: propagate errno when reading special refs fails
  refs: complete list of special refs
  bisect: consistently write BISECT_EXPECTED_REV via the refdb

 bisect.c                    | 25 +++-------------
 builtin/bisect.c            |  8 ++---
 refs.c                      | 59 +++++++++++++++++++++++++++++++++++--
 t/t1403-show-ref.sh         | 10 +++++++
 t/t6030-bisect-porcelain.sh |  2 +-
 wt-status.c                 | 22 +++++++++-----
 6 files changed, 87 insertions(+), 39 deletions(-)

Range-diff against v2:
1:  1db3eb3945 ! 1:  aea9410bd9 wt-status: read HEAD and ORIG_HEAD via the refdb
    @@ Commit message
         via the refdb layer so that we'll also be compatible with alternate
         reference backends.
     
    -    Note that we pass `RESOLVE_REF_NO_RECURSE` to `read_ref_full()`. This is
    -    because we didn't resolve symrefs before either, and in practice none of
    -    the refs in "rebase-merge/" would be symbolic. We thus don't want to
    -    resolve symrefs with the new code either to retain the old behaviour.
    +    There are some subtleties involved here:
    +
    +      - We pass `RESOLVE_REF_READING` so that a missing ref will cause
    +        `read_ref_full()` to return an error.
    +
    +      - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve
    +        symrefs. The old code didn't resolve symrefs either, and we only
    +        ever write object IDs into the refs in "rebase-merge/".
    +
    +      - In the same spirit we verify that successfully-read refs are not
    +        symbolic refs.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ wt-status.c: static char *read_line_from_git_path(const char *filename)
     -	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
     +	struct object_id head_oid, orig_head_oid;
     +	char *rebase_amend, *rebase_orig_head;
    ++	int head_flags, orig_head_flags;
      
      	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
      	    !s->branch || strcmp(s->branch, "HEAD"))
    @@ wt-status.c: static char *read_line_from_git_path(const char *filename)
      
     -	head = read_line_from_git_path("HEAD");
     -	orig_head = read_line_from_git_path("ORIG_HEAD");
    -+	if (read_ref_full("HEAD", RESOLVE_REF_NO_RECURSE, &head_oid, NULL) ||
    -+	    read_ref_full("ORIG_HEAD", RESOLVE_REF_NO_RECURSE, &orig_head_oid, NULL))
    ++	if (read_ref_full("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    ++			  &head_oid, &head_flags) ||
    ++	    read_ref_full("ORIG_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
    ++			  &orig_head_oid, &orig_head_flags))
    ++		return 0;
    ++	if (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF)
     +		return 0;
     +
      	rebase_amend = read_line_from_git_path("rebase-merge/amend");
2:  24032a62e5 = 2:  455ab69177 refs: propagate errno when reading special refs fails
3:  3dd9089fd5 ! 3:  81ac092281 refs: complete list of special refs
    @@ Commit message
     
         We're already mostly good with regard to the first item, except for
         `BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
    -    But the current list of special refs is missing a lot of refs that
    -    really should be treated specially. Right now, we only treat
    -    `FETCH_HEAD` and `MERGE_HEAD` specially here.
    +    But the current list of special refs is missing some refs that really
    +    should be treated specially. Right now, we only treat `FETCH_HEAD` and
    +    `MERGE_HEAD` specially here.
     
         Introduce a new function `is_special_ref()` that contains all current
         instances of special refs to fix the reading path.
     
    +    Note that this is only a temporary measure where we record and rectify
    +    the current state. Ideally, the list of special refs should in the end
    +    only contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may
    +    reference multiple objects and can contain annotations, so they indeed
    +    are special.
    +
         Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
4:  4a4447a3f5 = 4:  3244678161 bisect: consistently write BISECT_EXPECTED_REV via the refdb

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]

We read both the HEAD and ORIG_HEAD references directly from the
filesystem in order to figure out whether we're currently splitting a
commit. If both of the following are true:

  - HEAD points to the same object as "rebase-merge/amend".

  - ORIG_HEAD points to the same object as "rebase-merge/orig-head".

Then we are currently splitting commits.

The current code only works by chance because we only have a single
reference backend implementation. Refactor it to instead read both refs
via the refdb layer so that we'll also be compatible with alternate
reference backends.

There are some subtleties involved here:

  - We pass `RESOLVE_REF_READING` so that a missing ref will cause
    `read_ref_full()` to return an error.

  - We pass `RESOLVE_REF_NO_RECURSE` so that we do not try to resolve
    symrefs. The old code didn't resolve symrefs either, and we only
    ever write object IDs into the refs in "rebase-merge/".

  - In the same spirit we verify that successfully-read refs are not
    symbolic refs.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 wt-status.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 9f45bf6949..da19923981 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1295,26 +1295,32 @@ static char *read_line_from_git_path(const char *filename)
 static int split_commit_in_progress(struct wt_status *s)
 {
 	int split_in_progress = 0;
-	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
+	struct object_id head_oid, orig_head_oid;
+	char *rebase_amend, *rebase_orig_head;
+	int head_flags, orig_head_flags;
 
 	if ((!s->amend && !s->nowarn && !s->workdir_dirty) ||
 	    !s->branch || strcmp(s->branch, "HEAD"))
 		return 0;
 
-	head = read_line_from_git_path("HEAD");
-	orig_head = read_line_from_git_path("ORIG_HEAD");
+	if (read_ref_full("HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+			  &head_oid, &head_flags) ||
+	    read_ref_full("ORIG_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
+			  &orig_head_oid, &orig_head_flags))
+		return 0;
+	if (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF)
+		return 0;
+
 	rebase_amend = read_line_from_git_path("rebase-merge/amend");
 	rebase_orig_head = read_line_from_git_path("rebase-merge/orig-head");
 
-	if (!head || !orig_head || !rebase_amend || !rebase_orig_head)
+	if (!rebase_amend || !rebase_orig_head)
 		; /* fall through, no split in progress */
 	else if (!strcmp(rebase_amend, rebase_orig_head))
-		split_in_progress = !!strcmp(head, rebase_amend);
-	else if (strcmp(orig_head, rebase_orig_head))
+		split_in_progress = !!strcmp(oid_to_hex(&head_oid), rebase_amend);
+	else if (strcmp(oid_to_hex(&orig_head_oid), rebase_orig_head))
 		split_in_progress = 1;
 
-	free(head);
-	free(orig_head);
 	free(rebase_amend);
 	free(rebase_orig_head);
 
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/4] refs: propagate errno when reading special refs fails

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

Some refs in Git are more special than others due to reasons explained
in the next commit. These refs are read via `refs_read_special_head()`,
but this function doesn't behave the same as when we try to read a
normal ref. Most importantly, we do not propagate `failure_errno` in the
case where the reference does not exist, which is behaviour that we rely
on in many parts of Git.

Fix this bug by propagating errno when `strbuf_read_file()` fails.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c              |  4 +++-
 t/t1403-show-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index fcae5dddc6..00e72a2abf 100644
--- a/refs.c
+++ b/refs.c
@@ -1806,8 +1806,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	int result = -1;
 	strbuf_addf(&full_path, "%s/%s", ref_store->gitdir, refname);
 
-	if (strbuf_read_file(&content, full_path.buf, 0) < 0)
+	if (strbuf_read_file(&content, full_path.buf, 0) < 0) {
+		*failure_errno = errno;
 		goto done;
+	}
 
 	result = parse_loose_ref_contents(content.buf, oid, referent, type,
 					  failure_errno);
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index b50ae6fcf1..66e6e77fa9 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -266,4 +266,14 @@ test_expect_success '--exists with directory fails with generic error' '
 	test_cmp expect err
 '
 
+test_expect_success '--exists with non-existent special ref' '
+	test_expect_code 2 git show-ref --exists FETCH_HEAD
+'
+
+test_expect_success '--exists with existing special ref' '
+	test_when_finished "rm .git/FETCH_HEAD" &&
+	git rev-parse HEAD >.git/FETCH_HEAD &&
+	git show-ref --exists FETCH_HEAD
+'
+
 test_done
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/4] refs: complete list of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4342 bytes --]

We have some references that are more special than others. The reason
for them being special is that they either do not follow the usual
format of references, or that they are written to the filesystem
directly by the respective owning subsystem and thus circumvent the
reference backend.

This works perfectly fine right now because the reffiles backend will
know how to read those refs just fine. But with the prospect of gaining
a new reference backend implementation we need to be a lot more careful
here:

  - We need to make sure that we are consistent about how those refs are
    written. They must either always be written via the filesystem, or
    they must always be written via the reference backend. Any mixture
    will lead to inconsistent state.

  - We need to make sure that such special refs are always handled
    specially when reading them.

We're already mostly good with regard to the first item, except for
`BISECT_EXPECTED_REV` which will be addressed in a subsequent commit.
But the current list of special refs is missing some refs that really
should be treated specially. Right now, we only treat `FETCH_HEAD` and
`MERGE_HEAD` specially here.

Introduce a new function `is_special_ref()` that contains all current
instances of special refs to fix the reading path.

Note that this is only a temporary measure where we record and rectify
the current state. Ideally, the list of special refs should in the end
only contain `FETCH_HEAD` and `MERGE_HEAD` again because they both may
reference multiple objects and can contain annotations, so they indeed
are special.

Based-on-patch-by: Han-Wen Nienhuys <hanwenn@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 00e72a2abf..8fe34d51e4 100644
--- a/refs.c
+++ b/refs.c
@@ -1820,15 +1820,65 @@ static int refs_read_special_head(struct ref_store *ref_store,
 	return result;
 }
 
+static int is_special_ref(const char *refname)
+{
+	/*
+	 * Special references get written and read directly via the filesystem
+	 * by the subsystems that create them. Thus, they must not go through
+	 * the reference backend but must instead be read directly. It is
+	 * arguable whether this behaviour is sensible, or whether it's simply
+	 * a leaky abstraction enabled by us only having a single reference
+	 * backend implementation. But at least for a subset of references it
+	 * indeed does make sense to treat them specially:
+	 *
+	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
+	 *   carries additional metadata like where it came from.
+	 *
+	 * - MERGE_HEAD may contain multiple object IDs when merging multiple
+	 *   heads.
+	 *
+	 * There are some exceptions that you might expect to see on this list
+	 * but which are handled exclusively via the reference backend:
+	 *
+	 * - CHERRY_PICK_HEAD
+	 *
+	 * - HEAD
+	 *
+	 * - ORIG_HEAD
+	 *
+	 * - "rebase-apply/" and "rebase-merge/" contain all of the state for
+	 *   rebases, including some reference-like files. These are
+	 *   exclusively read and written via the filesystem and never go
+	 *   through the refdb.
+	 *
+	 * Writing or deleting references must consistently go either through
+	 * the filesystem (special refs) or through the reference backend
+	 * (normal ones).
+	 */
+	static const char * const special_refs[] = {
+		"AUTO_MERGE",
+		"BISECT_EXPECTED_REV",
+		"FETCH_HEAD",
+		"MERGE_AUTOSTASH",
+		"MERGE_HEAD",
+	};
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(special_refs); i++)
+		if (!strcmp(refname, special_refs[i]))
+			return 1;
+
+	return 0;
+}
+
 int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
 		      struct object_id *oid, struct strbuf *referent,
 		      unsigned int *type, int *failure_errno)
 {
 	assert(failure_errno);
-	if (!strcmp(refname, "FETCH_HEAD") || !strcmp(refname, "MERGE_HEAD")) {
+	if (is_special_ref(refname))
 		return refs_read_special_head(ref_store, refname, oid, referent,
 					      type, failure_errno);
-	}
 
 	return ref_store->be->read_raw_ref(ref_store, refname, oid, referent,
 					   type, failure_errno);
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 4/4] bisect: consistently write BISECT_EXPECTED_REV via the refdb

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5649 bytes --]

We're inconsistently writing BISECT_EXPECTED_REV both via the filesystem
and via the refdb, which violates the newly established rules for how
special refs must be treated. This works alright in practice with the
reffiles reference backend, but will cause bugs once we gain additional
backends.

Fix this issue and consistently write BISECT_EXPECTED_REV via the refdb
so that it is no longer a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  3 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 9 insertions(+), 29 deletions(-)

diff --git a/bisect.c b/bisect.c
index 1be8e0a271..985b96ed13 100644
--- a/bisect.c
+++ b/bisect.c
@@ -471,7 +471,6 @@ static int read_bisect_refs(void)
 }
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
@@ -707,26 +706,10 @@ static enum bisect_error error_if_skipped_commits(struct commit_list *tried,
 
 static int is_expected_rev(const struct object_id *oid)
 {
-	const char *filename = git_path_bisect_expected_rev();
-	struct stat st;
-	struct strbuf str = STRBUF_INIT;
-	FILE *fp;
-	int res = 0;
-
-	if (stat(filename, &st) || !S_ISREG(st.st_mode))
+	struct object_id expected_oid;
+	if (read_ref("BISECT_EXPECTED_REV", &expected_oid))
 		return 0;
-
-	fp = fopen_or_warn(filename, "r");
-	if (!fp)
-		return 0;
-
-	if (strbuf_getline_lf(&str, fp) != EOF)
-		res = !strcmp(str.buf, oid_to_hex(oid));
-
-	strbuf_release(&str);
-	fclose(fp);
-
-	return res;
+	return oideq(oid, &expected_oid);
 }
 
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
@@ -1185,10 +1168,10 @@ int bisect_clean_state(void)
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
 	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
 	result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF);
 	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
-	unlink_or_warn(git_path_bisect_expected_rev());
 	unlink_or_warn(git_path_bisect_ancestors_ok());
 	unlink_or_warn(git_path_bisect_log());
 	unlink_or_warn(git_path_bisect_names());
diff --git a/builtin/bisect.c b/builtin/bisect.c
index 35938b05fd..4e2c43daf5 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -17,7 +17,6 @@
 #include "revision.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
@@ -921,7 +920,6 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 	const char *state;
 	int i, verify_expected = 1;
 	struct object_id oid, expected;
-	struct strbuf buf = STRBUF_INIT;
 	struct oid_array revs = OID_ARRAY_INIT;
 
 	if (!argc)
@@ -976,10 +974,8 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 		oid_array_append(&revs, &commit->object.oid);
 	}
 
-	if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
-	    get_oid_hex(buf.buf, &expected) < 0)
+	if (read_ref("BISECT_EXPECTED_REV", &expected))
 		verify_expected = 0; /* Ignore invalid file contents */
-	strbuf_release(&buf);
 
 	for (i = 0; i < revs.nr; i++) {
 		if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0)) {
@@ -988,7 +984,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, int argc,
 		}
 		if (verify_expected && !oideq(&revs.oid[i], &expected)) {
 			unlink_or_warn(git_path_bisect_ancestors_ok());
-			unlink_or_warn(git_path_bisect_expected_rev());
+			delete_ref(NULL, "BISECT_EXPECTED_REV", NULL, REF_NO_DEREF);
 			verify_expected = 0;
 		}
 	}
diff --git a/refs.c b/refs.c
index 8fe34d51e4..c76ce86bef 100644
--- a/refs.c
+++ b/refs.c
@@ -1840,6 +1840,8 @@ static int is_special_ref(const char *refname)
 	 * There are some exceptions that you might expect to see on this list
 	 * but which are handled exclusively via the reference backend:
 	 *
+	 * - BISECT_EXPECTED_REV
+	 *
 	 * - CHERRY_PICK_HEAD
 	 *
 	 * - HEAD
@@ -1857,7 +1859,6 @@ static int is_special_ref(const char *refname)
 	 */
 	static const char * const special_refs[] = {
 		"AUTO_MERGE",
-		"BISECT_EXPECTED_REV",
 		"FETCH_HEAD",
 		"MERGE_AUTOSTASH",
 		"MERGE_HEAD",
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2a5b7d8379..792c1504bc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'git bisect reset cleans bisection state properly' '
 	git bisect bad $HASH4 &&
 	git bisect reset &&
 	test -z "$(git for-each-ref "refs/bisect/*")" &&
-	test_path_is_missing ".git/BISECT_EXPECTED_REV" &&
+	test_ref_missing BISECT_EXPECTED_REV &&
 	test_path_is_missing ".git/BISECT_ANCESTORS_OK" &&
 	test_path_is_missing ".git/BISECT_LOG" &&
 	test_path_is_missing ".git/BISECT_RUN" &&
-- 
2.43.GIT


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] wt-status: read HEAD and ORIG_HEAD via the refdb

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> OK, I somehow got a (wrong) impression that you are very close to
>> the finish line.
>
> You mean with the reftable backend?

It would be a major achievement if we just stop bypassing refs API
to read/write ORIG_HEAD and friends, even if we are still using only
the files backend.  And I meant that I got an impression that you
are very close to that, regardless of the readiness of the reftable
support.

Re: [PATCH v3 0/4] refs: improve handling of special refs

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Patrick Steinhardt (4):
>   wt-status: read HEAD and ORIG_HEAD via the refdb
>   refs: propagate errno when reading special refs fails
>   refs: complete list of special refs
>   bisect: consistently write BISECT_EXPECTED_REV via the refdb

With the clear understanding that we plan to make those other than
FETCH_HEAD and MERGE_HEAD in the is_special_ref().special_refs[]
eventually not special at all, this round looked quite sensible to
me.

Let's merge it down to 'next'.

Thanks.

Re: [PATCH v3 0/4] refs: improve handling of special refs

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Wed, Dec 20, 2023 at 11:28:51AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Patrick Steinhardt (4):
> >   wt-status: read HEAD and ORIG_HEAD via the refdb
> >   refs: propagate errno when reading special refs fails
> >   refs: complete list of special refs
> >   bisect: consistently write BISECT_EXPECTED_REV via the refdb
> 
> With the clear understanding that we plan to make those other than
> FETCH_HEAD and MERGE_HEAD in the is_special_ref().special_refs[]
> eventually not special at all, this round looked quite sensible to
> me.
> 
> Let's merge it down to 'next'.

Thanks. We (myself or a fellow team member at GitLab) will work on
converting the remaining not-so-special refs once this topic lands on
the `master` branch. Unless of course somebody else picks it up before
we do.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]