[PATCH v2] 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 fail after the normal rebase path has
updated the current branch when another local branch is a symref to it.
This can happen during a default-branch rename where refs/heads/main
points at refs/heads/master while users migrate.

The sequencer queues update-ref commands from local branch decorations.
Commit 106b6885c7 (rebase: ignore non-branch update-refs) filters out
decorations that are not local branches, such as HEAD and tags. A branch
symref is different: it is still a local branch decoration, but if it
resolves to another branch then that target branch is itself present in
the decoration list and will be updated as a concrete branch.

Skip branch decorations whose symrefs resolve to refs/heads/*, because
those targets are already represented by concrete branch decorations.
This prevents aliases from scheduling a second update for the same
branch. Keep symrefs to non-branch targets on the existing path.

Preserve the existing checked-out branch handling before applying these
skips. Such refs still need a todo-list comment instead of an update-ref
command, even when the checked-out ref is the branch being rebased or a
branch symref alias. Use a copy of the resolved HEAD ref so later ref
resolution does not overwrite it.

Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
---
    rebase: handle --update-refs branch symrefs
    
    Changes since v1:
    
     * Squashed the regression test and fix into a single patch.
     * Reworked the implementation per Phillip's review: skip local branch
       symrefs that are not checked out when their targets resolve to
       refs/heads/*, while preserving the existing behavior for symrefs to
       non-branch targets.
     * Preserved checked-out branch comments before applying the new symref
       skip, so worktrees still get the existing todo-list warning instead
       of an update-ref command.
     * Folded the symref coverage into the existing "--update-refs updates
       refs correctly" test, covering both an alias of HEAD and an alias of
       another branch.
     * Kept the relationship to 106b6885c7 in the commit message: non-local
       decorations are already filtered, and this patch handles the
       remaining local branch decorations that are symref aliases.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2126%2Fsluongng%2Fsl%2Frebase-update-refs-symrefs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2126/sluongng/sl/rebase-update-refs-symrefs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2126

Range-diff vs v1:

 1:  a550923440 < -:  ---------- t3404: add failing branch symref test
 2:  0ab0a71744 ! 1:  68f698225c rebase: skip branch symref aliases
     @@ Metadata
       ## Commit message ##
          rebase: skip branch symref aliases
      
     -    rebase --update-refs records local branch decorations before replaying
     -    commits. If a decoration is a symbolic branch such as refs/heads/main
     -    pointing at refs/heads/master, updating it later dereferences back to
     -    master and can fail because the normal rebase path already moved that
     -    branch.
     +    git rebase --update-refs can fail after the normal rebase path has
     +    updated the current branch when another local branch is a symref to it.
     +    This can happen during a default-branch rename where refs/heads/main
     +    points at refs/heads/master while users migrate.
      
     -    Resolve local branch symref decorations to their referents before
     -    queuing update-ref commands, and skip duplicates. This keeps branch
     -    aliases from scheduling a second update for the same underlying branch
     -    while still using the existing old-OID check for the single queued
     -    update.
     +    The sequencer queues update-ref commands from local branch decorations.
     +    Commit 106b6885c7 (rebase: ignore non-branch update-refs) filters out
     +    decorations that are not local branches, such as HEAD and tags. A branch
     +    symref is different: it is still a local branch decoration, but if it
     +    resolves to another branch then that target branch is itself present in
     +    the decoration list and will be updated as a concrete branch.
     +
     +    Skip branch decorations whose symrefs resolve to refs/heads/*, because
     +    those targets are already represented by concrete branch decorations.
     +    This prevents aliases from scheduling a second update for the same
     +    branch. Keep symrefs to non-branch targets on the existing path.
     +
     +    Preserve the existing checked-out branch handling before applying these
     +    skips. Such refs still need a todo-list comment instead of an update-ref
     +    command, even when the checked-out ref is the branch being rebased or a
     +    branch symref alias. Use a copy of the resolved HEAD ref so later ref
     +    resolution does not overwrite it.
      
          Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
      
     @@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
       	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",
     -+	struct ref_store *refs = get_main_ref_store(the_repository);
     -+	const char *head_ref = refs_resolve_ref_unsafe(refs, "HEAD",
     - 						       RESOLVE_REF_READING,
     +-						       RESOLVE_REF_READING,
      -						       NULL,
      -						       NULL);
     -+						       NULL, NULL);
     -+	char *resolved_head_ref = refs_resolve_refdup(refs, "HEAD",
     -+						       RESOLVE_REF_READING,
     -+						       NULL, NULL);
     -+	struct strbuf update_ref = STRBUF_INIT;
     ++	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;
     -+		const char *ref = decoration->name;
      +		const char *resolved_ref;
     -+		int is_symref = 0;
      +		int flags = 0;
       		size_t base_offset = ctx->buf->len;
       
       		/*
     -@@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
     - 		 * updated by the default rebase behavior.
     - 		 * Exclude it from the list of refs to update,
     - 		 * as well as any non-branch decorations.
     -+		 *
     -+		 * Resolve branch symrefs after checking for the current HEAD so
     -+		 * that aliases do not schedule duplicate updates for their
     -+		 * referents.
     -+		 *
     +-		 * 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.
     @@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
      +			continue;
      +		}
      +
     -+		if (head_ref && !strcmp(head_ref, ref)) {
     ++		path = branch_checked_out(decoration->name);
     ++
     ++		/*
     ++		 * 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, unless it is checked out and
     ++		 * needs a comment in the todo list.
     ++		 */
     ++		if (!path && head_ref && !strcmp(head_ref, decoration->name)) {
      +			decoration = decoration->next;
      +			continue;
      +		}
      +
     -+		strbuf_reset(&update_ref);
     -+		resolved_ref = refs_resolve_ref_unsafe(refs, ref,
     -+						       RESOLVE_REF_READING |
     -+						       RESOLVE_REF_NO_RECURSE,
     ++		resolved_ref = refs_resolve_ref_unsafe(refs, decoration->name,
     ++						       RESOLVE_REF_READING,
      +						       NULL, &flags);
     -+		if ((flags & REF_ISSYMREF) && resolved_ref) {
     -+			if (!starts_with(resolved_ref, "refs/heads/")) {
     -+				decoration = decoration->next;
     -+				continue;
     -+			}
     -+
     -+			strbuf_addstr(&update_ref, resolved_ref);
     -+			ref = update_ref.buf;
     -+			is_symref = 1;
     -+		}
     -+
     -+		if ((is_symref && resolved_head_ref &&
     -+		     !strcmp(resolved_head_ref, ref)) ||
     -+		    string_list_has_string(&ctx->refs_to_oids, ref)) {
     ++		if (!path && resolved_ref && (flags & REF_ISSYMREF) &&
     ++		    starts_with(resolved_ref, "refs/heads/")) {
       			decoration = decoration->next;
       			continue;
       		}
     @@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
       
       		/* If the branch is checked out, then leave a comment instead. */
      -		if ((path = branch_checked_out(decoration->name))) {
     -+		if ((path = branch_checked_out(ref))) {
     ++		if (path) {
       			item->command = TODO_COMMENT;
       			strbuf_commented_addf(ctx->buf, comment_line_str,
       					      "Ref %s checked out at '%s'\n",
     --					      decoration->name, path);
     -+					      ref, path);
     - 		} else {
     - 			struct string_list_item *sti;
     - 			item->command = TODO_UPDATE_REF;
     --			strbuf_addf(ctx->buf, "%s\n", decoration->name);
     -+			strbuf_addf(ctx->buf, "%s\n", ref);
     - 
     - 			sti = string_list_insert(&ctx->refs_to_oids,
     --						 decoration->name);
     --			sti->util = init_update_ref_record(decoration->name);
     -+						 ref);
     -+			sti->util = init_update_ref_record(ref);
     - 		}
     - 
     - 		item->offset_in_buf = base_offset;
      @@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
       		decoration = decoration->next;
       	}
       
     -+	strbuf_release(&update_ref);
     -+	free(resolved_head_ref);
     ++	free(head_ref);
       	return 0;
       }
       
      
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs ignores non-branch decorations' '
     - 	test_cmp expect actual
       '
       
     --test_expect_failure '--update-refs skips branch symrefs to current branch' '
     -+test_expect_success '--update-refs skips branch symrefs to current branch' '
     - 	test_when_finished "
     - 		test_might_fail git rebase --abort &&
     - 		git checkout primary &&
     + 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 &&
     + 
     +@@ t/t3404-rebase-interactive.sh: 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.


 sequencer.c                   | 43 +++++++++++++++++++++++++----------
 t/t3404-rebase-interactive.sh | 15 ++++++++++++
 2 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 1ee4b2875b..6ab8b47108 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6445,28 +6445,46 @@ 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;
+		const 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;
+		}
+
+		path = branch_checked_out(decoration->name);
+
+		/*
+		 * 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, unless it is checked out and
+		 * needs a comment in the todo list.
+		 */
+		if (!path && head_ref && !strcmp(head_ref, decoration->name)) {
+			decoration = decoration->next;
+			continue;
+		}
+
+		resolved_ref = refs_resolve_ref_unsafe(refs, decoration->name,
+						       RESOLVE_REF_READING,
+						       NULL, &flags);
+		if (!path && resolved_ref && (flags & REF_ISSYMREF) &&
+		    starts_with(resolved_ref, "refs/heads/")) {
 			decoration = decoration->next;
 			continue;
 		}
@@ -6478,7 +6496,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",
@@ -6501,6 +6519,7 @@ static int add_decorations_to_list(const struct commit *commit,
 		decoration = decoration->next;
 	}
 
+	free(head_ref);
 	return 0;
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 58b3bb0c27..bc0b6a31f7 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1979,11 +1979,18 @@ test_expect_success '--update-refs ignores non-branch decorations' '
 '
 
 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 +1998,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.

base-commit: c69baaf57ba26cf117c2b6793802877f19738b0d
-- 
gitgitgadget