refs: improve handling of special refs

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

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.

---
 wt-status.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index 9f45bf6949..fe9e590b80 100644
@@ -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

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

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

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.

---
 refs.c              | 6 +++++-
 t/t1403-show-ref.sh | 9 +++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

index fcae5dddc6..7d4a057f36 100644
@@ -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);

index b50ae6fcf1..37af154d27 100755
@@ -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

True. I'll drop this when rerolling.

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

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.

---
 refs.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

index 7d4a057f36..2d39d3fe80 100644
@@ -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

Not really, no.

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.

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

Hi Patrick

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

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

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.

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.

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

Hi Patrick

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.

That's certainly a good place to start

That sounds like a good plan

That's great

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

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.

---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  2 +-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 8 insertions(+), 29 deletions(-)

index 1be8e0a271..985b96ed13 100644
@@ -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());

index 35938b05fd..4e2c43daf5 100644
@@ -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;
 		}
 	}

index 2d39d3fe80..0290bb0c67 100644
@@ -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",

index 2a5b7d8379..792c1504bc 100755
@@ -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

Very nice :-).

Thanks,
Taylor

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

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

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

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

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.

---
 wt-status.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index 9f45bf6949..fe9e590b80 100644
@@ -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

"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.

Good to see a rewrite being careful like this.

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

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.

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

Yes, we're in agreement.

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.

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

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.

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.

Okay.

Patrick

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

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.

[snip]

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

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.

---
 refs.c              |  4 +++-
 t/t1403-show-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

index fcae5dddc6..00e72a2abf 100644
@@ -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);

index b50ae6fcf1..66e6e77fa9 100755
@@ -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

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

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.

---
 refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

index 00e72a2abf..8fe34d51e4 100644
@@ -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

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.

---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  3 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 9 insertions(+), 29 deletions(-)

index 1be8e0a271..985b96ed13 100644
@@ -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());

index 35938b05fd..4e2c43daf5 100644
@@ -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;
 		}
 	}

index 8fe34d51e4..c76ce86bef 100644
@@ -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",

index 2a5b7d8379..792c1504bc 100755
@@ -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

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.

@@ 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.
+

4: 4a4447a3f5 = 4: 3244678161 bisect: consistently write BISECT_EXPECTED_REV via the refdb

base-commit: 1a87c842ece327d03d08096395969aca5e0a6996

2.43.GIT

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.

---
 wt-status.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

index 9f45bf6949..da19923981 100644
@@ -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

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.

---
 refs.c              |  4 +++-
 t/t1403-show-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

index fcae5dddc6..00e72a2abf 100644
@@ -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);

index b50ae6fcf1..66e6e77fa9 100755
@@ -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

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.

---
 refs.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

index 00e72a2abf..8fe34d51e4 100644
@@ -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

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.

---
 bisect.c                    | 25 ++++---------------------
 builtin/bisect.c            |  8 ++------
 refs.c                      |  3 ++-
 t/t6030-bisect-porcelain.sh |  2 +-
 4 files changed, 9 insertions(+), 29 deletions(-)

index 1be8e0a271..985b96ed13 100644
@@ -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());

index 35938b05fd..4e2c43daf5 100644
@@ -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;
 		}
 	}

index 8fe34d51e4..c76ce86bef 100644
@@ -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",

index 2a5b7d8379..792c1504bc 100755
@@ -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

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.

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