[PATCH v3 1/2] rebase: skip branch symref aliases

["Son Luong Ngoc via GitGitGadget" <gitgitgadget@gmail.com>]

From: Son Luong Ngoc <sluongng@gmail.com>

git rebase --update-refs can finish rewriting the current branch and
then fail while updating a local branch that is a symbolic ref. This can
happen during a default-branch rename where refs/heads/main points at
refs/heads/master while users migrate.

The problem is a partially applied ref update: the main rebase has
already succeeded when the later ref update fails.

The sequencer queues updates from local branch decorations. Commit
106b6885c7 (rebase: ignore non-branch update-refs) filters out
decorations such as HEAD and tags. A branch symref is still a local
branch decoration, but refs_update_ref() dereferences it, so an alias to
another branch duplicates the concrete branch update.

Resolve local branch decorations before queuing them. Skip symrefs whose
targets are under refs/heads/ so that only the concrete branch update is
queued. Keep an owned copy of the resolved HEAD and skip the current
branch before checked-out handling so later ref resolution cannot change
the comparison.

This prevents a successful rebase from being followed by a failed,
partially applied ref update while preserving each alias as a symref.

Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
---
 sequencer.c                   | 44 +++++++++++++++++++++++++----------
 t/t3400-rebase.sh             |  2 +-
 t/t3404-rebase-interactive.sh | 16 +++++++++++++
 3 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1355a99a09..63aba60a08 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6465,32 +6465,50 @@ static int add_decorations_to_list(const struct commit *commit,
 				   struct todo_add_branch_context *ctx)
 {
 	const struct name_decoration *decoration = get_name_decoration(&commit->object);
-	const char *head_ref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
-						       "HEAD",
-						       RESOLVE_REF_READING,
-						       NULL,
-						       NULL);
+	struct ref_store *refs = get_main_ref_store(the_repository);
+	char *head_ref = refs_resolve_refdup(refs, "HEAD",
+					     RESOLVE_REF_READING,
+					     NULL, NULL);
 
 	while (decoration) {
 		struct todo_item *item;
 		const char *path;
+		char *resolved_ref;
+		int flags = 0;
 		size_t base_offset = ctx->buf->len;
 
 		/*
-		 * If the branch is the current HEAD, then it will be
-		 * updated by the default rebase behavior.
-		 * Exclude it from the list of refs to update,
-		 * as well as any non-branch decorations.
 		 * Non-branch decorations may be present if the pretty format
 		 * includes "%d", which would have loaded all refs
 		 * into the global decoration table.
 		 */
-		if ((head_ref && !strcmp(head_ref, decoration->name)) ||
-		    (decoration->type != DECORATION_REF_LOCAL)) {
+		if (decoration->type != DECORATION_REF_LOCAL) {
+			decoration = decoration->next;
+			continue;
+		}
+
+		resolved_ref = refs_resolve_refdup(refs, decoration->name,
+						      RESOLVE_REF_READING,
+						      NULL, &flags);
+		if (resolved_ref && (flags & REF_ISSYMREF) &&
+		    starts_with(resolved_ref, "refs/heads/")) {
+			free(resolved_ref);
+			decoration = decoration->next;
+			continue;
+		}
+
+		/*
+		 * If the branch is the current HEAD, then it will be
+		 * updated by the default rebase behavior.
+		 */
+		if (head_ref && !strcmp(head_ref, decoration->name)) {
+			free(resolved_ref);
 			decoration = decoration->next;
 			continue;
 		}
 
+		path = branch_checked_out(decoration->name);
+
 		ALLOC_GROW(ctx->items,
 			ctx->items_nr + 1,
 			ctx->items_alloc);
@@ -6498,7 +6516,7 @@ static int add_decorations_to_list(const struct commit *commit,
 		memset(item, 0, sizeof(*item));
 
 		/* If the branch is checked out, then leave a comment instead. */
-		if ((path = branch_checked_out(decoration->name))) {
+		if (path) {
 			item->command = TODO_COMMENT;
 			strbuf_commented_addf(ctx->buf, comment_line_str,
 					      "Ref %s checked out at '%s'\n",
@@ -6518,9 +6536,11 @@ static int add_decorations_to_list(const struct commit *commit,
 		item->arg_len = ctx->buf->len - base_offset;
 		ctx->items_nr++;
 
+		free(resolved_ref);
 		decoration = decoration->next;
 	}
 
+	free(head_ref);
 	return 0;
 }
 
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index e62e07b894..1a02f6546b 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -471,7 +471,7 @@ test_expect_success 'git rebase --update-ref with core.commentChar and branch on
 	GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
 		 rebase -i --update-refs base &&
 	test_grep "% Ref refs/heads/wt-topic checked out at" actual &&
-	test_grep "% Ref refs/heads/topic2 checked out at" actual
+	test_grep ! "% Ref refs/heads/topic2 checked out at" actual
 '
 
 test_done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e64816770a..11afa8be56 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1975,15 +1975,23 @@ test_expect_success '--update-refs ignores non-branch decorations' '
 	) &&
 	grep ^update-ref todo >actual &&
 	test_write_lines "update-ref refs/heads/no-conflict-branch" >expect &&
+	test_grep ! "^# Ref refs/heads/update-refs checked out" todo &&
 	test_cmp expect actual
 '
 
 test_expect_success '--update-refs updates refs correctly' '
+	test_when_finished "
+		test_might_fail git symbolic-ref -d refs/heads/no-conflict-branch-alias &&
+		test_might_fail git symbolic-ref -d refs/heads/second-alias
+	" &&
 	git checkout -B update-refs no-conflict-branch &&
 	git branch -f base HEAD~4 &&
 	git branch -f first HEAD~3 &&
 	git branch -f second HEAD~3 &&
 	git branch -f third HEAD~1 &&
+	git symbolic-ref refs/heads/no-conflict-branch-alias \
+		refs/heads/no-conflict-branch &&
+	git symbolic-ref refs/heads/second-alias refs/heads/second &&
 	test_commit extra2 fileX &&
 	git commit --amend --fixup=L &&
 
@@ -1991,8 +1999,16 @@ test_expect_success '--update-refs updates refs correctly' '
 
 	test_cmp_rev HEAD~3 refs/heads/first &&
 	test_cmp_rev HEAD~3 refs/heads/second &&
+	test_cmp_rev HEAD~3 refs/heads/second-alias &&
 	test_cmp_rev HEAD~1 refs/heads/third &&
 	test_cmp_rev HEAD refs/heads/no-conflict-branch &&
+	test_cmp_rev HEAD refs/heads/no-conflict-branch-alias &&
+	test_write_lines refs/heads/no-conflict-branch >expect &&
+	git symbolic-ref refs/heads/no-conflict-branch-alias >actual &&
+	test_cmp expect actual &&
+	test_write_lines refs/heads/second >expect &&
+	git symbolic-ref refs/heads/second-alias >actual &&
+	test_cmp expect actual &&
 
 	q_to_tab >expect <<-\EOF &&
 	Successfully rebased and updated refs/heads/update-refs.
-- 
gitgitgadget