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

On 23/07/2026 19:58, Phillip Wood wrote:

On 22/07/2026 09:15, Son Luong Ngoc via GitGitGadget wrote:

quoted

From: Son Luong Ngoc <redacted>

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.

Thanks for re-rolling I'm pretty sure the logic is sound now but I'm a 
bit confused by a couple of things - see my comments below.

quoted

Signed-off-by: Son Luong Ngoc <redacted>
---
  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;
+        }

It would be nice to have a comment here explaining what we're doing. 
Also I don't think we need to copy the refname so it would be more 
efficient to use refs_resolve_ref_unsafe().

Looking at this again we cannot use refs_resolve_ref_unsafe() because 
the result would be overwritten by the call to refs_resolve_refdup() in 
branch_checked_out().

quoted

+        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;
+        }

We skip any symbolic refs that point to another branch which is good.

quoted

+        /*
+         * 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;
          }

Then we check to see if the decoration matches HEAD which we used to do 
above - I'm not clear why we have moved this check.

Should we be using "resolved_ref" instead of "decoration->name"? That 
would explain why this was moved and would makes sense as we resolve 
symrefs when reading HEAD. When HEAD points outside "refs/heads/" we'd 
then skip updating any symrefs under "refs/heads/" that pointed to the 
same ref as HEAD.

Thanks

Phillip

quoted

+        path = branch_checked_out(decoration->name);
+

This belongs in the next patch I think.

quoted

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

Adding an extra context line shows

     git checkout topic2>       GIT_SEQUENCE_EDITOR="cat >actual" git -c 
core.commentChar=% \

quoted

           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

As topic2 is checked out in the worktree where the rebase is running why 
did this line appear before?

quoted

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" 

quoted

expect &&

+    test_grep ! "^# Ref refs/heads/update-refs checked out" todo &&

Lets move this line below test_cmp so we keep that line next to the ones 
that create the files that are being compared. Is this another case 
where we used to add this comment and no longer do so?

quoted

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

This looks good - we check that "rebase --update-refs" succeeds withh 
branches that are symrefs and also that those refs are untouched by the 
rebase.

Thanks

Phillip

quoted

      q_to_tab >expect <<-\EOF &&
      Successfully rebased and updated refs/heads/update-refs.