threads / patch / 60766

patch, 7 partsrefs: convert special refs to become normal pseudo-refs

Subject: [PATCH 0/7] refs: convert special refs to become normal pseudo-refs

## tl;dr

23 messages between Jan 19, 2024 and Jan 24, 2024. Diffs are folded; open one to read it.

replies: 22people: 5as markdown or json

Patrick Steinhardt· Jan 19, 2024, 10:39 UTC · lore
Hi,

this patch series is a follow-up to the one that introduced `is_special_ref()` [1]. Back then, we agreed that the list of special refs ought to shrink over time [2] so that only the actually-special refs FETCH_HEAD and MERGE_HEAD remain, which is exactly what I'm doing now.

Patrick

[1]: https://public-inbox.org/git/cover.1701243201.git.ps@pks.im/ [2]: https://public-inbox.org/git/ZXlfeWtDgr1GQFCL@tanuki/

Patrick Steinhardt (7):
  sequencer: clean up pseudo refs with REF_NO_DEREF
  sequencer: delete REBASE_HEAD in correct repo when picking commits
  refs: convert AUTO_MERGE to become a normal pseudo-ref
  sequencer: introduce functions to handle autostashes via refs
  refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref
  refs: redefine special refs
  Documentation: add "special refs" to the glossary
 Documentation/glossary-content.txt | 14 +++++
 branch.c                           |  5 +-
 builtin/commit.c                   |  2 +-
 builtin/merge.c                    | 27 ++++-----
 builtin/rebase.c                   |  2 +-
 merge-ort.c                        | 19 +++---
 path.c                             |  2 -
 path.h                             |  2 -
 refs.c                             | 35 +++--------
 repository.c                       |  2 -
 repository.h                       |  2 -
 sequencer.c                        | 95 ++++++++++++++++++++++++------
 sequencer.h                        |  3 +
 13 files changed, 132 insertions(+), 78 deletions(-)
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
-- 
2.43.GIT
Patrick Steinhardt· Jan 19, 2024, 10:39 UTC · re: Patrick Steinhardt · lore

[PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF

When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those refs are a symref we would thus end up deleting the symref targets, and not the symrefs themselves.

Harden the code to use REF_NO_DEREF to fix this.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sequencer.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
Show changes to sequencer.c +7 −7
diff --git a/sequencer.c b/sequencer.c
index 3cc88d8a80..b9cbc290ea 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -474,7 +474,7 @@ static void print_advice(struct repository *r, int show_hint,
 		 * of the commit itself so remove CHERRY_PICK_HEAD
 		 */
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
-				NULL, 0);
+				NULL, REF_NO_DEREF);
 		return;
 	}
 
@@ -1667,7 +1667,7 @@ static int do_commit(struct repository *r,
 		strbuf_release(&sb);
 		if (!res) {
 			refs_delete_ref(get_main_ref_store(r), "",
-					"CHERRY_PICK_HEAD", NULL, 0);
+					"CHERRY_PICK_HEAD", NULL, REF_NO_DEREF);
 			unlink(git_path_merge_msg(r));
 			if (!is_rebase_i(opts))
 				print_commit_summary(r, NULL, &oid,
@@ -2406,7 +2406,7 @@ static int do_pick_commit(struct repository *r,
 	} else if (allow == 2) {
 		drop_commit = 1;
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
-				NULL, 0);
+				NULL, REF_NO_DEREF);
 		unlink(git_path_merge_msg(r));
 		unlink(git_path_auto_merge(r));
 		fprintf(stderr,
@@ -2802,7 +2802,7 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 
 	if (refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD")) {
 		if (!refs_delete_ref(get_main_ref_store(r), "",
-				     "CHERRY_PICK_HEAD", NULL, 0) &&
+				     "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF) &&
 		    verbose)
 			warning(_("cancelling a cherry picking in progress"));
 		opts.action = REPLAY_PICK;
@@ -2811,7 +2811,7 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 
 	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD")) {
 		if (!refs_delete_ref(get_main_ref_store(r), "", "REVERT_HEAD",
-				     NULL, 0) &&
+				     NULL, REF_NO_DEREF) &&
 		    verbose)
 			warning(_("cancelling a revert in progress"));
 		opts.action = REPLAY_REVERT;
@@ -4116,7 +4116,7 @@ static int do_merge(struct repository *r,
 
 		strbuf_release(&ref_name);
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
-				NULL, 0);
+				NULL, REF_NO_DEREF);
 		rollback_lock_file(&lock);
 
 		ret = run_command(&cmd);
@@ -5108,7 +5108,7 @@ static int commit_staged_changes(struct repository *r,
 		if (refs_ref_exists(get_main_ref_store(r),
 				    "CHERRY_PICK_HEAD") &&
 		    refs_delete_ref(get_main_ref_store(r), "",
-				    "CHERRY_PICK_HEAD", NULL, 0))
+				    "CHERRY_PICK_HEAD", NULL, REF_NO_DEREF))
 			return error(_("could not remove CHERRY_PICK_HEAD"));
 		if (unlink(git_path_merge_msg(r)) && errno != ENOENT)
 			return error_errno(_("could not remove '%s'"),
-- 
2.43.GIT
Junio C Hamano· Jan 19, 2024, 19:14 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF

Patrick Steinhardt <ps@pks.im> writes:
Show 6 quoted lines
> When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> refs are a symref we would thus end up deleting the symref targets, and
> not the symrefs themselves.
>
> Harden the code to use REF_NO_DEREF to fix this.

This level of attention to detail is very much appreciated. I suspect that this was inspired by the other topic we discussed the other day to tighten accesses to {MERGE,CHERRY_PICK,REVERT}_HEAD that is done via repo_get_oid() to the one based on read_ref_full()?

Patrick Steinhardt· Jan 22, 2024, 10:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF

On Fri, Jan 19, 2024 at 11:14:57AM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> > REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> > refs are a symref we would thus end up deleting the symref targets, and
> > not the symrefs themselves.
> >
> > Harden the code to use REF_NO_DEREF to fix this.
> 
> This level of attention to detail is very much appreciated.  I
> suspect that this was inspired by the other topic we discussed the
> other day to tighten accesses to {MERGE,CHERRY_PICK,REVERT}_HEAD
> that is done via repo_get_oid() to the one based on read_ref_full()?

At least not directly, no. I was adding new calls to `delete_ref()` and was reflexively adding REF_NO_DEREF, which made me wonder why the other cases in the vicinity didn't use it yet. So it's more inspired by the first patch series that introduced the "special refs" classification, where I already added ref deletions with `REF_NO_DEREF`.

Patrick
Karthik Nayak· Jan 22, 2024, 11:49 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF

Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
> When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> refs are a symref we would thus end up deleting the symref targets, and
> not the symrefs themselves.
>
> Harden the code to use REF_NO_DEREF to fix this.
>

Just a question for my understanding. Is that that currently CHERRY_PICK_HEAD or REVERT_HEAD are never symrefs, so we don't really worry about this, but adding this change makes it safer?

Patrick Steinhardt· Jan 22, 2024, 12:28 UTC · re: Karthik Nayak · lore

Re: [PATCH 1/7] sequencer: clean up pseudo refs with REF_NO_DEREF

On Mon, Jan 22, 2024 at 03:49:37AM -0800, Karthik Nayak wrote:
Show 13 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
> > REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
> > refs are a symref we would thus end up deleting the symref targets, and
> > not the symrefs themselves.
> >
> > Harden the code to use REF_NO_DEREF to fix this.
> >
> 
> Just a question for my understanding. Is that that currently
> CHERRY_PICK_HEAD or REVERT_HEAD are never symrefs, so we don't really
> worry about this, but adding this change makes it safer?

They shouldn't ever be, at least Git would never end up writing symrefs there. So for those refs to be symrefs the user would need to manually create them as such, and in that case it would certainly be unexpected if we deleted the symref targets when cleaning up state after a revert or cherry-pick.

So yes, it's about hardening the code for edge cases that shouldn't really happen in the first place.

Patrick
Patrick Steinhardt· Jan 19, 2024, 10:40 UTC · re: Patrick Steinhardt · lore

[PATCH 2/7] sequencer: delete REBASE_HEAD in correct repo when picking commits

When picking commits, we delete some state before executing the next sequencer action on interactive rebases. But while we use the correct repository to calculate paths to state files that need deletion, we use the repo-less `delete_ref()` function to delete REBASE_HEAD. Thus, if the sequencer ran in a different repository than `the_repository`, we would end up deleting the ref in the wrong repository.

Fix this by using `refs_delete_ref()` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sequencer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to sequencer.c +2 −1
diff --git a/sequencer.c b/sequencer.c
index b9cbc290ea..6f620f5717 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4767,7 +4767,8 @@ static int pick_commits(struct repository *r,
 			unlink(rebase_path_author_script());
 			unlink(git_path_merge_head(r));
 			unlink(git_path_auto_merge(r));
-			delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
+			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
+					NULL, REF_NO_DEREF);
 
 			if (item->command == TODO_BREAK) {
 				if (!opts->verbose)
-- 
2.43.GIT
Patrick Steinhardt· Jan 19, 2024, 10:40 UTC · re: Patrick Steinhardt · lore

[PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref

In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have inrtoduced a new `is_special_ref()` function that classifies some refs as being special. The rule is that special refs are exclusively read and written via the filesystem directly, whereas normal refs exclucsively go via the refs API.

The intent of that commit was to record the status quo so that we know to route reads of such special refs consistently. Eventually, the list should be reduced to its bare minimum of refs which really are special, namely FETCH_HEAD and MERGE_HEAD.

Follow up on this promise and convert the AUTO_MERGE ref to become a normal pseudo-ref by using the refs API to both read and write it instead of accessing the filesystem directly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 branch.c         |  3 ++-
 builtin/rebase.c |  2 +-
 merge-ort.c      | 19 ++++++++++++-------
 path.c           |  1 -
 path.h           |  1 -
 refs.c           |  1 -
 repository.c     |  1 -
 repository.h     |  1 -
 sequencer.c      | 12 ++++++++----
 9 files changed, 23 insertions(+), 18 deletions(-)
Show changes to 9 files +23 −18

branch.c, builtin/rebase.c, merge-ort.c, path.c, path.h, refs.c, repository.c, repository.h, sequencer.c

diff --git a/branch.c b/branch.c
index 534594f7f8..c8bd9519e6 100644
--- a/branch.c
+++ b/branch.c
@@ -817,7 +817,8 @@ void remove_merge_branch_state(struct repository *r)
 	unlink(git_path_merge_rr(r));
 	unlink(git_path_merge_msg(r));
 	unlink(git_path_merge_mode(r));
-	unlink(git_path_auto_merge(r));
+	refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+			NULL, REF_NO_DEREF);
 	save_autostash(git_path_merge_autostash(r));
 }
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 995818c28d..5b086f651a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -515,7 +515,7 @@ static int finish_rebase(struct rebase_options *opts)
 	int ret = 0;
 
 	delete_ref(NULL, "REBASE_HEAD", NULL, REF_NO_DEREF);
-	unlink(git_path_auto_merge(the_repository));
+	delete_ref(NULL, "AUTO_MERGE", NULL, REF_NO_DEREF);
 	apply_autostash(state_dir_path("autostash", opts));
 	/*
 	 * We ignore errors in 'git maintenance run --auto', since the
diff --git a/merge-ort.c b/merge-ort.c
index 77ba7f3020..d72fd04f58 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -38,6 +38,7 @@
 #include "path.h"
 #include "promisor-remote.h"
 #include "read-cache-ll.h"
+#include "refs.h"
 #include "revision.h"
 #include "sparse-index.h"
 #include "strmap.h"
@@ -4659,9 +4660,6 @@ void merge_switch_to_result(struct merge_options *opt,
 {
 	assert(opt->priv == NULL);
 	if (result->clean >= 0 && update_worktree_and_index) {
-		const char *filename;
-		FILE *fp;
-
 		trace2_region_enter("merge", "checkout", opt->repo);
 		if (checkout(opt, head, result->tree)) {
 			/* failure to function */
@@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
 		trace2_region_leave("merge", "record_conflicted", opt->repo);
 
 		trace2_region_enter("merge", "write_auto_merge", opt->repo);
-		filename = git_path_auto_merge(opt->repo);
-		fp = xfopen(filename, "w");
-		fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
-		fclose(fp);
+		if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
+				    &result->tree->object.oid, NULL, REF_NO_DEREF,
+				    UPDATE_REFS_MSG_ON_ERR)) {
+			/* failure to function */
+			opt->priv = NULL;
+			result->clean = -1;
+			merge_finalize(opt, result);
+			trace2_region_leave("merge", "write_auto_merge",
+					    opt->repo);
+			return;
+		}
 		trace2_region_leave("merge", "write_auto_merge", opt->repo);
 	}
 	if (display_update_msgs)
diff --git a/path.c b/path.c
index 67e2690efe..f881c03171 100644
--- a/path.c
+++ b/path.c
@@ -1589,6 +1589,5 @@ REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
 REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE")
 REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD")
 REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
-REPO_GIT_PATH_FUNC(auto_merge, "AUTO_MERGE")
 REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD")
 REPO_GIT_PATH_FUNC(shallow, "shallow")
diff --git a/path.h b/path.h
index 639372edd9..f387410f8c 100644
--- a/path.h
+++ b/path.h
@@ -176,7 +176,6 @@ const char *git_path_merge_rr(struct repository *r);
 const char *git_path_merge_mode(struct repository *r);
 const char *git_path_merge_head(struct repository *r);
 const char *git_path_merge_autostash(struct repository *r);
-const char *git_path_auto_merge(struct repository *r);
 const char *git_path_fetch_head(struct repository *r);
 const char *git_path_shallow(struct repository *r);
 
diff --git a/refs.c b/refs.c
index 20e8f1ff1f..906c7e5f27 100644
--- a/refs.c
+++ b/refs.c
@@ -1874,7 +1874,6 @@ static int is_special_ref(const char *refname)
 	 * (normal ones).
 	 */
 	static const char * const special_refs[] = {
-		"AUTO_MERGE",
 		"FETCH_HEAD",
 		"MERGE_AUTOSTASH",
 		"MERGE_HEAD",
diff --git a/repository.c b/repository.c
index d7d24d416a..a931e3b1b3 100644
--- a/repository.c
+++ b/repository.c
@@ -263,7 +263,6 @@ static void repo_clear_path_cache(struct repo_path_cache *cache)
 	FREE_AND_NULL(cache->merge_mode);
 	FREE_AND_NULL(cache->merge_head);
 	FREE_AND_NULL(cache->merge_autostash);
-	FREE_AND_NULL(cache->auto_merge);
 	FREE_AND_NULL(cache->fetch_head);
 	FREE_AND_NULL(cache->shallow);
 }
diff --git a/repository.h b/repository.h
index f5269b3730..47e7d20b59 100644
--- a/repository.h
+++ b/repository.h
@@ -68,7 +68,6 @@ struct repo_path_cache {
 	char *merge_mode;
 	char *merge_head;
 	char *merge_autostash;
-	char *auto_merge;
 	char *fetch_head;
 	char *shallow;
 };
diff --git a/sequencer.c b/sequencer.c
index 6f620f5717..47c8d17cbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2408,7 +2408,8 @@ static int do_pick_commit(struct repository *r,
 		refs_delete_ref(get_main_ref_store(r), "", "CHERRY_PICK_HEAD",
 				NULL, REF_NO_DEREF);
 		unlink(git_path_merge_msg(r));
-		unlink(git_path_auto_merge(r));
+		refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+				NULL, REF_NO_DEREF);
 		fprintf(stderr,
 			_("dropping %s %s -- patch contents already upstream\n"),
 			oid_to_hex(&commit->object.oid), msg.subject);
@@ -2818,7 +2819,8 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose)
 		need_cleanup = 1;
 	}
 
-	unlink(git_path_auto_merge(r));
+	refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+			NULL, REF_NO_DEREF);
 
 	if (!need_cleanup)
 		return;
@@ -4766,7 +4768,8 @@ static int pick_commits(struct repository *r,
 			}
 			unlink(rebase_path_author_script());
 			unlink(git_path_merge_head(r));
-			unlink(git_path_auto_merge(r));
+			refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+					NULL, REF_NO_DEREF);
 			refs_delete_ref(get_main_ref_store(r), "", "REBASE_HEAD",
 					NULL, REF_NO_DEREF);
 
@@ -5123,7 +5126,8 @@ static int commit_staged_changes(struct repository *r,
 		return error(_("could not commit staged changes."));
 	unlink(rebase_path_amend());
 	unlink(git_path_merge_head(r));
-	unlink(git_path_auto_merge(r));
+	refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
+			NULL, REF_NO_DEREF);
 	if (final_fixup) {
 		unlink(rebase_path_fixup_msg());
 		unlink(rebase_path_squash_msg());
-- 
2.43.GIT
Junio C Hamano· Jan 19, 2024, 19:28 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref

Patrick Steinhardt <ps@pks.im> writes:
> In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> inrtoduced a new `is_special_ref()` function that classifies some refs
"introduced"
Show 22 quoted lines
> @@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
>  		trace2_region_leave("merge", "record_conflicted", opt->repo);
>  
>  		trace2_region_enter("merge", "write_auto_merge", opt->repo);
> -		filename = git_path_auto_merge(opt->repo);
> -		fp = xfopen(filename, "w");
> -		fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> -		fclose(fp);
> +		if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
> +				    &result->tree->object.oid, NULL, REF_NO_DEREF,
> +				    UPDATE_REFS_MSG_ON_ERR)) {
> +			/* failure to function */
> +			opt->priv = NULL;
> +			result->clean = -1;
> +			merge_finalize(opt, result);
> +			trace2_region_leave("merge", "write_auto_merge",
> +					    opt->repo);
> +			return;
> +		}
>  		trace2_region_leave("merge", "write_auto_merge", opt->repo);
>  	}
>  	if (display_update_msgs)

We used to ignore errors while updating AUTO_MERGE, implying that it is an optional nicety that does not have to block the merge. Now we explicitly mark the resulting index unclean. While my gut feeling says that it should not matter all that much (as such a failure would be rare enough that the user may want to inspect and double check the situation before going forward), I am not 100% sure if the change is behaviour is acceptable by everybody (cc'ed Elijah for second opinion).

Everything else in this patch looked good.
Thanks.
Patrick Steinhardt· Jan 22, 2024, 10:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref

On Fri, Jan 19, 2024 at 11:28:10AM -0800, Junio C Hamano wrote:
Show 38 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> > inrtoduced a new `is_special_ref()` function that classifies some refs
> 
> "introduced"
> 
> > @@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
> >  		trace2_region_leave("merge", "record_conflicted", opt->repo);
> >  
> >  		trace2_region_enter("merge", "write_auto_merge", opt->repo);
> > -		filename = git_path_auto_merge(opt->repo);
> > -		fp = xfopen(filename, "w");
> > -		fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> > -		fclose(fp);
> > +		if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
> > +				    &result->tree->object.oid, NULL, REF_NO_DEREF,
> > +				    UPDATE_REFS_MSG_ON_ERR)) {
> > +			/* failure to function */
> > +			opt->priv = NULL;
> > +			result->clean = -1;
> > +			merge_finalize(opt, result);
> > +			trace2_region_leave("merge", "write_auto_merge",
> > +					    opt->repo);
> > +			return;
> > +		}
> >  		trace2_region_leave("merge", "write_auto_merge", opt->repo);
> >  	}
> >  	if (display_update_msgs)
> 
> We used to ignore errors while updating AUTO_MERGE, implying that it
> is an optional nicety that does not have to block the merge.  Now we
> explicitly mark the resulting index unclean.  While my gut feeling
> says that it should not matter all that much (as such a failure
> would be rare enough that the user may want to inspect and double
> check the situation before going forward), I am not 100% sure if the
> change is behaviour is acceptable by everybody (cc'ed Elijah for
> second opinion).

We only ignored _some_ errors when updating AUTO_MERGE. Most notably we die when we fail to create the file, but we succeed in case its contents aren't written. This does not make much sense to me -- my expectation would be that we should verify either the complete operation or nothing of it and ignore all failures. Gracefully leaving an empty file behind is a weird in-between state, so I'd claim it's more or less an oversight that we did not perform proper error checking here.

Patrick
Elijah Newren· Jan 24, 2024, 03:19 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref

On Mon, Jan 22, 2024 at 2:45 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 48 quoted lines
>
> On Fri, Jan 19, 2024 at 11:28:10AM -0800, Junio C Hamano wrote:
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> > > inrtoduced a new `is_special_ref()` function that classifies some refs
> >
> > "introduced"
> >
> > > @@ -4687,10 +4685,17 @@ void merge_switch_to_result(struct merge_options *opt,
> > >             trace2_region_leave("merge", "record_conflicted", opt->repo);
> > >
> > >             trace2_region_enter("merge", "write_auto_merge", opt->repo);
> > > -           filename = git_path_auto_merge(opt->repo);
> > > -           fp = xfopen(filename, "w");
> > > -           fprintf(fp, "%s\n", oid_to_hex(&result->tree->object.oid));
> > > -           fclose(fp);
> > > +           if (refs_update_ref(get_main_ref_store(opt->repo), "", "AUTO_MERGE",
> > > +                               &result->tree->object.oid, NULL, REF_NO_DEREF,
> > > +                               UPDATE_REFS_MSG_ON_ERR)) {
> > > +                   /* failure to function */
> > > +                   opt->priv = NULL;
> > > +                   result->clean = -1;
> > > +                   merge_finalize(opt, result);
> > > +                   trace2_region_leave("merge", "write_auto_merge",
> > > +                                       opt->repo);
> > > +                   return;
> > > +           }
> > >             trace2_region_leave("merge", "write_auto_merge", opt->repo);
> > >     }
> > >     if (display_update_msgs)
> >
> > We used to ignore errors while updating AUTO_MERGE, implying that it
> > is an optional nicety that does not have to block the merge.  Now we
> > explicitly mark the resulting index unclean.  While my gut feeling
> > says that it should not matter all that much (as such a failure
> > would be rare enough that the user may want to inspect and double
> > check the situation before going forward), I am not 100% sure if the
> > change is behaviour is acceptable by everybody (cc'ed Elijah for
> > second opinion).
>
> We only ignored _some_ errors when updating AUTO_MERGE. Most notably we
> die when we fail to create the file, but we succeed in case its contents
> aren't written. This does not make much sense to me -- my expectation
> would be that we should verify either the complete operation or nothing
> of it and ignore all failures. Gracefully leaving an empty file behind
> is a weird in-between state, so I'd claim it's more or less an oversight
> that we did not perform proper error checking here.

I can confirm it was indeed just an oversight. I like your change to make this code more careful.

Karthik Nayak· Jan 22, 2024, 12:02 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 3/7] refs: convert AUTO_MERGE to become a normal pseudo-ref

Patrick Steinhardt <ps@pks.im> writes:
Show 6 quoted lines
> In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
> inrtoduced a new `is_special_ref()` function that classifies some refs
> as being special. The rule is that special refs are exclusively read and
> written via the filesystem directly, whereas normal refs exclucsively go
> via the refs API.
>
s/exclucsively/exclusively
Patrick Steinhardt· Jan 19, 2024, 10:40 UTC · re: Patrick Steinhardt · lore

[PATCH 4/7] sequencer: introduce functions to handle autostashes via refs

We're about to convert the MERGE_AUTOSTASH ref to become non-special, using the refs API instead of direct filesystem access to both read and write the ref. The current interfaces to write autostashes is entirely path-based though, so we need to extend them to also support writes via the refs API instead.

Ideally, we would be able to fully replace the old set of path-based interfaces. But the sequencer will continue to write state into "rebase-merge/autostash". This path is not considered to be a ref at all and will thus stay is-is for now, which requires us to keep both path- and refs-based interfaces to handle autostashes.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 sequencer.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++----
 sequencer.h |  3 +++
 2 files changed, 64 insertions(+), 5 deletions(-)
Show changes to 2 files +64 −5

sequencer.c, sequencer.h

diff --git a/sequencer.c b/sequencer.c
index 47c8d17cbb..91de546b32 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4463,12 +4463,17 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset)
 	return -1;
 }
 
-void create_autostash(struct repository *r, const char *path)
+static void create_autostash_internal(struct repository *r,
+				      const char *path,
+				      const char *refname)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct lock_file lock_file = LOCK_INIT;
 	int fd;
 
+	if (path && refname)
+		BUG("can only pass path or refname");
+
 	fd = repo_hold_locked_index(r, &lock_file, 0);
 	refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
 	if (0 <= fd)
@@ -4495,10 +4500,16 @@ void create_autostash(struct repository *r, const char *path)
 		strbuf_reset(&buf);
 		strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV);
 
-		if (safe_create_leading_directories_const(path))
-			die(_("Could not create directory for '%s'"),
-			    path);
-		write_file(path, "%s", oid_to_hex(&oid));
+		if (path) {
+			if (safe_create_leading_directories_const(path))
+				die(_("Could not create directory for '%s'"),
+				    path);
+			write_file(path, "%s", oid_to_hex(&oid));
+		} else {
+			refs_update_ref(get_main_ref_store(r), "", refname,
+					&oid, null_oid(), 0, UPDATE_REFS_DIE_ON_ERR);
+		}
+
 		printf(_("Created autostash: %s\n"), buf.buf);
 		if (reset_head(r, &ropts) < 0)
 			die(_("could not reset --hard"));
@@ -4509,6 +4520,16 @@ void create_autostash(struct repository *r, const char *path)
 	strbuf_release(&buf);
 }
 
+void create_autostash(struct repository *r, const char *path)
+{
+	create_autostash_internal(r, path, NULL);
+}
+
+void create_autostash_ref(struct repository *r, const char *refname)
+{
+	create_autostash_internal(r, NULL, refname);
+}
+
 static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply)
 {
 	struct child_process child = CHILD_PROCESS_INIT;
@@ -4586,6 +4607,41 @@ int apply_autostash_oid(const char *stash_oid)
 	return apply_save_autostash_oid(stash_oid, 1);
 }
 
+static int apply_save_autostash_ref(struct repository *r, const char *refname,
+				    int attempt_apply)
+{
+	struct object_id stash_oid;
+	char stash_oid_hex[GIT_MAX_HEXSZ + 1];
+	int flag, ret;
+
+	if (!refs_ref_exists(get_main_ref_store(r), refname))
+		return 0;
+
+	if (!refs_resolve_ref_unsafe(get_main_ref_store(r), refname,
+				     RESOLVE_REF_READING, &stash_oid, &flag))
+		return -1;
+	if (flag & REF_ISSYMREF)
+		return error(_("autostash reference is a symref"));
+
+	oid_to_hex_r(stash_oid_hex, &stash_oid);
+	ret = apply_save_autostash_oid(stash_oid_hex, attempt_apply);
+
+	refs_delete_ref(get_main_ref_store(r), "", refname,
+			&stash_oid, REF_NO_DEREF);
+
+	return ret;
+}
+
+int save_autostash_ref(struct repository *r, const char *refname)
+{
+	return apply_save_autostash_ref(r, refname, 0);
+}
+
+int apply_autostash_ref(struct repository *r, const char *refname)
+{
+	return apply_save_autostash_ref(r, refname, 1);
+}
+
 static int checkout_onto(struct repository *r, struct replay_opts *opts,
 			 const char *onto_name, const struct object_id *onto,
 			 const struct object_id *orig_head)
diff --git a/sequencer.h b/sequencer.h
index 913a0f652d..dcef7bb99c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -225,9 +225,12 @@ void commit_post_rewrite(struct repository *r,
 			 const struct object_id *new_head);
 
 void create_autostash(struct repository *r, const char *path);
+void create_autostash_ref(struct repository *r, const char *refname);
 int save_autostash(const char *path);
+int save_autostash_ref(struct repository *r, const char *refname);
 int apply_autostash(const char *path);
 int apply_autostash_oid(const char *stash_oid);
+int apply_autostash_ref(struct repository *r, const char *refname);
 
 #define SUMMARY_INITIAL_COMMIT   (1 << 0)
 #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
-- 
2.43.GIT
Junio C Hamano· Jan 19, 2024, 20:09 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs

Patrick Steinhardt <ps@pks.im> writes:
Show 10 quoted lines
> We're about to convert the MERGE_AUTOSTASH ref to become non-special,
> using the refs API instead of direct filesystem access to both read and
> write the ref. The current interfaces to write autostashes is entirely
> path-based though, so we need to extend them to also support writes via
> the refs API instead.
>
> Ideally, we would be able to fully replace the old set of path-based
> interfaces. But the sequencer will continue to write state into
> "rebase-merge/autostash". This path is not considered to be a ref at all
> and will thus stay is-is for now, which requires us to keep both path-
"is-is"???
Show 7 quoted lines
> and refs-based interfaces to handle autostashes.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  sequencer.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  sequencer.h |  3 +++
>  2 files changed, 64 insertions(+), 5 deletions(-)

The conversion (rather, the introduction to allow refs API to be used to access them) look correct, but offhand I do not know what the implication of leaving the file based interface would be.

Thanks.
Patrick Steinhardt· Jan 22, 2024, 10:51 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs

On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We're about to convert the MERGE_AUTOSTASH ref to become non-special,
> > using the refs API instead of direct filesystem access to both read and
> > write the ref. The current interfaces to write autostashes is entirely
> > path-based though, so we need to extend them to also support writes via
> > the refs API instead.
> >
> > Ideally, we would be able to fully replace the old set of path-based
> > interfaces. But the sequencer will continue to write state into
> > "rebase-merge/autostash". This path is not considered to be a ref at all
> > and will thus stay is-is for now, which requires us to keep both path-
> 
> "is-is"???
Oops, "as-is" :)
Show 11 quoted lines
> > and refs-based interfaces to handle autostashes.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  sequencer.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++----
> >  sequencer.h |  3 +++
> >  2 files changed, 64 insertions(+), 5 deletions(-)
> 
> The conversion (rather, the introduction to allow refs API to be
> used to access them) look correct, but offhand I do not know what
> the implication of leaving the file based interface would be.

We have said in past discussions that the sequencer state should remain self contained, and that requires us to keep the files-based interface. Refactoring it would likely be a larger undertaking, as we have also said that refs must either have pseudo-ref syntax or start with "refs/".

The sequencer with its "rebase-merge/autostash" files doesn't conform to either of those requirements, so we would also have to rename those reflike files. I doubt that this is really worth it, so I would keep those around as internal implementation details of how the sequencer works. These refs are not supposed to be accessed by the user in the first place, and we do not document their existence to the best of my knowledge. Also, `git rev-parse rebase-merge/autostash` would fail.

So overall I think it's fine to leave this internal sequencer state as self-contained as it is.

Patrick
Phillip Wood· Jan 22, 2024, 19:54 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs

On 22/01/2024 10:51, Patrick Steinhardt wrote:
Show 18 quoted lines
> On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> The conversion (rather, the introduction to allow refs API to be
>> used to access them) look correct, but offhand I do not know what
>> the implication of leaving the file based interface would be.
> 
> We have said in past discussions that the sequencer state should remain
> self contained, and that requires us to keep the files-based interface.
> Refactoring it would likely be a larger undertaking, as we have also
> said that refs must either have pseudo-ref syntax or start with "refs/".
> 
> The sequencer with its "rebase-merge/autostash" files doesn't conform to
> either of those requirements, so we would also have to rename those
> reflike files. I doubt that this is really worth it, so I would keep
> those around as internal implementation details of how the sequencer
> works. These refs are not supposed to be accessed by the user in the
> first place, and we do not document their existence to the best of my
> knowledge. Also, `git rev-parse rebase-merge/autostash` would fail.

Exactly "rebase-merge/autostash" is a detail of the rebase implementation, it is not a ref that users should be using.

> So overall I think it's fine to leave this internal sequencer state as
> self-contained as it is.
That's my feeling too
Best Wishes
Phillip
Junio C Hamano· Jan 22, 2024, 20:16 UTC · re: Phillip Wood · lore

Re: [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 11 quoted lines
>> ... These refs are not supposed to be accessed by the user in the
>> first place, and we do not document their existence to the best of my
>> knowledge. Also, `git rev-parse rebase-merge/autostash` would fail.
>
> Exactly "rebase-merge/autostash" is a detail of the rebase
> implementation, it is not a ref that users should be using.
>
>> So overall I think it's fine to leave this internal sequencer state as
>> self-contained as it is.
>
> That's my feeling too

Good. As long as "git rev-parse rebase-merge/autostash" fails, and regardless of the ref backend in use, we always do "read one line from the on-disk file and run get_oid_hex() on it", I would be happy with that direction.

Thanks.
[Footnote]
 * We seem to overuse strbuf_read_file() even when we expect that
   the file is a single-liner.  We have read_line_from_git_path() in
   wt-status.c that only reads one line and we may want to split it
   into two: one that takes a filename and calls the other with
   git_path("%s", filename), and the other that takes a path for any
   on-disk file, reads a single line, and returns the string that
   was read, or something like that.  We might later want to update
   the "other" function so that it errors out if there are extra
   trailing lines (i.e. we expect it is a single-liner, but that
   expectation is violated---now what?).
Patrick Steinhardt· Jan 19, 2024, 10:40 UTC · re: Patrick Steinhardt · lore

[PATCH 5/7] refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref

Similar to the preceding conversion of the AUTO_MERGE pseudo-ref, let's convert the MERGE_AUTOSTASH ref to become a normal pseudo-ref as well.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 branch.c         |  2 +-
 builtin/commit.c |  2 +-
 builtin/merge.c  | 27 +++++++++++++--------------
 path.c           |  1 -
 path.h           |  1 -
 refs.c           |  1 -
 repository.c     |  1 -
 repository.h     |  1 -
 8 files changed, 15 insertions(+), 21 deletions(-)
Show changes to 8 files +15 −21

branch.c, builtin/commit.c, builtin/merge.c, path.c, path.h, refs.c, repository.c, repository.h

diff --git a/branch.c b/branch.c
index c8bd9519e6..6719a181bd 100644
--- a/branch.c
+++ b/branch.c
@@ -819,7 +819,7 @@ void remove_merge_branch_state(struct repository *r)
 	unlink(git_path_merge_mode(r));
 	refs_delete_ref(get_main_ref_store(r), "", "AUTO_MERGE",
 			NULL, REF_NO_DEREF);
-	save_autostash(git_path_merge_autostash(r));
+	save_autostash_ref(r, "MERGE_AUTOSTASH");
 }
 
 void remove_branch_state(struct repository *r, int verbose)
diff --git a/builtin/commit.c b/builtin/commit.c
index 65196a2827..6d1fa71676 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1877,7 +1877,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 				     &oid, flags);
 	}
 
-	apply_autostash(git_path_merge_autostash(the_repository));
+	apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 
 cleanup:
 	strbuf_release(&author_ident);
diff --git a/builtin/merge.c b/builtin/merge.c
index ebbe05033e..8f819781cc 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -476,7 +476,7 @@ static void finish(struct commit *head_commit,
 	run_hooks_l("post-merge", squash ? "1" : "0", NULL);
 
 	if (new_head)
-		apply_autostash(git_path_merge_autostash(the_repository));
+		apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 	strbuf_release(&reflog_message);
 }
 
@@ -1315,7 +1315,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (abort_current_merge) {
 		int nargc = 2;
 		const char *nargv[] = {"reset", "--merge", NULL};
-		struct strbuf stash_oid = STRBUF_INIT;
+		char stash_oid_hex[GIT_MAX_HEXSZ + 1];
+		struct object_id stash_oid = {0};
 
 		if (orig_argc != 2)
 			usage_msg_opt(_("--abort expects no arguments"),
@@ -1324,17 +1325,17 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		if (!file_exists(git_path_merge_head(the_repository)))
 			die(_("There is no merge to abort (MERGE_HEAD missing)."));
 
-		if (read_oneliner(&stash_oid, git_path_merge_autostash(the_repository),
-		    READ_ONELINER_SKIP_IF_EMPTY))
-			unlink(git_path_merge_autostash(the_repository));
+		if (!read_ref("MERGE_AUTOSTASH", &stash_oid))
+			delete_ref("", "MERGE_AUTOSTASH", &stash_oid, REF_NO_DEREF);
 
 		/* Invoke 'git reset --merge' */
 		ret = cmd_reset(nargc, nargv, prefix);
 
-		if (stash_oid.len)
-			apply_autostash_oid(stash_oid.buf);
+		if (!is_null_oid(&stash_oid)) {
+			oid_to_hex_r(stash_oid_hex, &stash_oid);
+			apply_autostash_oid(stash_oid_hex);
+		}
 
-		strbuf_release(&stash_oid);
 		goto done;
 	}
 
@@ -1563,13 +1564,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 
 		if (autostash)
-			create_autostash(the_repository,
-					 git_path_merge_autostash(the_repository));
+			create_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 		if (checkout_fast_forward(the_repository,
 					  &head_commit->object.oid,
 					  &commit->object.oid,
 					  overwrite_ignore)) {
-			apply_autostash(git_path_merge_autostash(the_repository));
+			apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 			ret = 1;
 			goto done;
 		}
@@ -1655,8 +1655,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		die_ff_impossible();
 
 	if (autostash)
-		create_autostash(the_repository,
-				 git_path_merge_autostash(the_repository));
+		create_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 
 	/* We are going to make a new commit. */
 	git_committer_info(IDENT_STRICT);
@@ -1741,7 +1740,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			fprintf(stderr, _("Merge with strategy %s failed.\n"),
 				use_strategies[0]->name);
-		apply_autostash(git_path_merge_autostash(the_repository));
+		apply_autostash_ref(the_repository, "MERGE_AUTOSTASH");
 		ret = 2;
 		goto done;
 	} else if (best_strategy == wt_strategy)
diff --git a/path.c b/path.c
index f881c03171..0fb527918b 100644
--- a/path.c
+++ b/path.c
@@ -1588,6 +1588,5 @@ REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG")
 REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR")
 REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE")
 REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD")
-REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH")
 REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD")
 REPO_GIT_PATH_FUNC(shallow, "shallow")
diff --git a/path.h b/path.h
index f387410f8c..b3233c51fa 100644
--- a/path.h
+++ b/path.h
@@ -175,7 +175,6 @@ const char *git_path_merge_msg(struct repository *r);
 const char *git_path_merge_rr(struct repository *r);
 const char *git_path_merge_mode(struct repository *r);
 const char *git_path_merge_head(struct repository *r);
-const char *git_path_merge_autostash(struct repository *r);
 const char *git_path_fetch_head(struct repository *r);
 const char *git_path_shallow(struct repository *r);
 
diff --git a/refs.c b/refs.c
index 906c7e5f27..047c81b1c1 100644
--- a/refs.c
+++ b/refs.c
@@ -1875,7 +1875,6 @@ static int is_special_ref(const char *refname)
 	 */
 	static const char * const special_refs[] = {
 		"FETCH_HEAD",
-		"MERGE_AUTOSTASH",
 		"MERGE_HEAD",
 	};
 	size_t i;
diff --git a/repository.c b/repository.c
index a931e3b1b3..7aacb51b65 100644
--- a/repository.c
+++ b/repository.c
@@ -262,7 +262,6 @@ static void repo_clear_path_cache(struct repo_path_cache *cache)
 	FREE_AND_NULL(cache->merge_rr);
 	FREE_AND_NULL(cache->merge_mode);
 	FREE_AND_NULL(cache->merge_head);
-	FREE_AND_NULL(cache->merge_autostash);
 	FREE_AND_NULL(cache->fetch_head);
 	FREE_AND_NULL(cache->shallow);
 }
diff --git a/repository.h b/repository.h
index 47e7d20b59..7a250a6605 100644
--- a/repository.h
+++ b/repository.h
@@ -67,7 +67,6 @@ struct repo_path_cache {
 	char *merge_rr;
 	char *merge_mode;
 	char *merge_head;
-	char *merge_autostash;
 	char *fetch_head;
 	char *shallow;
 };
-- 
2.43.GIT
Patrick Steinhardt· Jan 19, 2024, 10:40 UTC · re: Patrick Steinhardt · lore

[PATCH 6/7] refs: redefine special refs

Now that our list of special refs really only contains refs which have actually-special semantics, let's redefine what makes a special ref.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 refs.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)
Show changes to refs.c +7 −26
diff --git a/refs.c b/refs.c
index 047c81b1c1..08a900a047 100644
--- a/refs.c
+++ b/refs.c
@@ -1839,13 +1839,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
 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:
+	 * Special references are refs that have different semantics compared
+	 * to "normal" refs. These refs can thus not be stored in the ref
+	 * backend, but must always be accessed via the filesystem. The
+	 * following refs are special:
 	 *
 	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
 	 *   carries additional metadata like where it came from.
@@ -1853,25 +1850,9 @@ static int is_special_ref(const char *refname)
 	 * - 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:
-	 *
-	 * - BISECT_EXPECTED_REV
-	 *
-	 * - 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).
+	 * Reading, 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[] = {
 		"FETCH_HEAD",
-- 
2.43.GIT
Junio C Hamano· Jan 19, 2024, 20:28 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 6/7] refs: redefine special refs

Patrick Steinhardt <ps@pks.im> writes:
> Now that our list of special refs really only contains refs which have
> actually-special semantics, let's redefine what makes a special ref.
Yay.
Show 57 quoted lines
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs.c | 33 +++++++--------------------------
>  1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 047c81b1c1..08a900a047 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1839,13 +1839,10 @@ static int refs_read_special_head(struct ref_store *ref_store,
>  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:
> +	 * Special references are refs that have different semantics compared
> +	 * to "normal" refs. These refs can thus not be stored in the ref
> +	 * backend, but must always be accessed via the filesystem. The
> +	 * following refs are special:
>  	 *
>  	 * - FETCH_HEAD may contain multiple object IDs, and each one of them
>  	 *   carries additional metadata like where it came from.
> @@ -1853,25 +1850,9 @@ static int is_special_ref(const char *refname)
>  	 * - 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:
> -	 *
> -	 * - BISECT_EXPECTED_REV
> -	 *
> -	 * - 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).
> +	 * Reading, 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[] = {
>  		"FETCH_HEAD",
Patrick Steinhardt· Jan 19, 2024, 10:40 UTC · re: Patrick Steinhardt · lore

[PATCH 7/7] Documentation: add "special refs" to the glossary

Add the "special refs" term to our glossary.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/glossary-content.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
Show changes to Documentation/glossary-content.txt +14 −0
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index f7d98c11e3..d71b199955 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -638,6 +638,20 @@ The most notable example is `HEAD`.
 	An <<def_object,object>> used to temporarily store the contents of a
 	<<def_dirty,dirty>> working directory and the index for future reuse.
 
+[[def_special_ref]]special ref::
+	A ref that has different semantics than normal refs. These refs can be
+	accessed via normal Git commands but may not behave the same as a
+	normal ref in some cases.
++
+The following special refs are known to Git:
+
+ - "`FETCH_HEAD`" is written by linkgit:git-fetch[1] or linkgit:git-pull[1]. It
+   may refer to multiple object IDs. Each object ID is annotated with metadata
+   indicating where it was fetched from and its fetch status.
+
+ - "`MERGE_HEAD`" is written by linkgit:git-merge[1] when resolving merge
+   conflicts. It contains all commit IDs which are being merged.
+
 [[def_submodule]]submodule::
 	A <<def_repository,repository>> that holds the history of a
 	separate project inside another repository (the latter of
-- 
2.43.GIT
Phillip Wood· Jan 23, 2024, 16:27 UTC · re: Patrick Steinhardt · lore

Re: [PATCH 7/7] Documentation: add "special refs" to the glossary

Hi Patrick
On 19/01/2024 10:40, Patrick Steinhardt wrote:
> Add the "special refs" term to our glossary.
Related to this the glossary entry for pseudorefs says
         Pseudorefs are a class of files under `$GIT_DIR` which behave
         like refs for the purposes of rev-parse, but which are treated
         specially by git.  Pseudorefs both have names that are all-caps,
         and always start with a line consisting of a
         <<def_SHA1,SHA-1>> followed by whitespace.  So, HEAD is not a
         pseudoref, because it is sometimes a symbolic ref.  They might
         optionally contain some additional data.  `MERGE_HEAD` and
         `CHERRY_PICK_HEAD` are examples.  Unlike
         <<def_per_worktree_ref,per-worktree refs>>, these files cannot
         be symbolic refs, and never have reflogs.  They also cannot be
         updated through the normal ref update machinery.  Instead,
         they are updated by directly writing to the files.  However,
         they can be read as if they were refs, so `git rev-parse
         MERGE_HEAD` will work.

which is very file-centric. We should probably update that as we're moving away from filesystem access except for special refs.

Best Wishes
Phillip
Show 30 quoted lines
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>   Documentation/glossary-content.txt | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index f7d98c11e3..d71b199955 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -638,6 +638,20 @@ The most notable example is `HEAD`.
>   	An <<def_object,object>> used to temporarily store the contents of a
>   	<<def_dirty,dirty>> working directory and the index for future reuse.
>   
> +[[def_special_ref]]special ref::
> +	A ref that has different semantics than normal refs. These refs can be
> +	accessed via normal Git commands but may not behave the same as a
> +	normal ref in some cases.
> ++
> +The following special refs are known to Git:
> +
> + - "`FETCH_HEAD`" is written by linkgit:git-fetch[1] or linkgit:git-pull[1]. It
> +   may refer to multiple object IDs. Each object ID is annotated with metadata
> +   indicating where it was fetched from and its fetch status.
> +
> + - "`MERGE_HEAD`" is written by linkgit:git-merge[1] when resolving merge
> +   conflicts. It contains all commit IDs which are being merged.
> +
>   [[def_submodule]]submodule::
>   	A <<def_repository,repository>> that holds the history of a
>   	separate project inside another repository (the latter of
Patrick Steinhardt· Jan 24, 2024, 09:05 UTC · re: Phillip Wood · lore

Re: [PATCH 7/7] Documentation: add "special refs" to the glossary

On Tue, Jan 23, 2024 at 04:27:20PM +0000, Phillip Wood wrote:
Show 24 quoted lines
> Hi Patrick
> 
> On 19/01/2024 10:40, Patrick Steinhardt wrote:
> > Add the "special refs" term to our glossary.
> 
> Related to this the glossary entry for pseudorefs says
> 
>         Pseudorefs are a class of files under `$GIT_DIR` which behave
>         like refs for the purposes of rev-parse, but which are treated
>         specially by git.  Pseudorefs both have names that are all-caps,
>         and always start with a line consisting of a
>         <<def_SHA1,SHA-1>> followed by whitespace.  So, HEAD is not a
>         pseudoref, because it is sometimes a symbolic ref.  They might
>         optionally contain some additional data.  `MERGE_HEAD` and
>         `CHERRY_PICK_HEAD` are examples.  Unlike
>         <<def_per_worktree_ref,per-worktree refs>>, these files cannot
>         be symbolic refs, and never have reflogs.  They also cannot be
>         updated through the normal ref update machinery.  Instead,
>         they are updated by directly writing to the files.  However,
>         they can be read as if they were refs, so `git rev-parse
>         MERGE_HEAD` will work.
> 
> which is very file-centric. We should probably update that as we're moving
> away from filesystem access except for special refs.

Good point indeed, thanks for the hint! Will update in a future patch series.

Patrick

← back to recent threads