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

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

[Patrick Steinhardt]

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

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

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

[Patrick Steinhardt]

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

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(-)

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


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

[PATCH 6/7] refs: redefine special refs

[Patrick Steinhardt]

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

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(-)

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


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

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

[Patrick Steinhardt]

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

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(+)

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


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

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

[Junio C Hamano]

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()?

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

[Junio C Hamano]

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

Everything else in this patch looked good.

Thanks.

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

[Junio C Hamano]

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

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

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

[Junio C Hamano]

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.

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

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

[Patrick Steinhardt]

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

On Fri, Jan 19, 2024 at 11:14:57AM -0800, Junio C Hamano wrote:
> 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

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

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

[Patrick Steinhardt]

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

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.

Patrick

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

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

[Patrick Steinhardt]

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

On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote:
> 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" :)

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

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

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

[Karthik Nayak]

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

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?

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

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

[Karthik Nayak]

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

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

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

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

[Patrick Steinhardt]

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

On Mon, Jan 22, 2024 at 03:49:37AM -0800, Karthik Nayak wrote:
> 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

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

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

[Phillip Wood]

On 22/01/2024 10:51, Patrick Steinhardt wrote:
> 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

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> ... 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?).

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

[Phillip Wood]

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

> 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

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

[Elijah Newren]

On Mon, Jan 22, 2024 at 2:45 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> 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.

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

[Patrick Steinhardt]

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

On Tue, Jan 23, 2024 at 04:27:20PM +0000, Phillip Wood wrote:
> 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

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