From: Phillip Wood Date: Fri, 24 Jul 2026 09:55:18 GMT Subject: Re: [PATCH v3 1/2] rebase: skip branch symref aliases Message-ID: <00e529b6-7ae7-463f-a4b3-0991e9411aba@gmail.com> In-Reply-To: <5bece313-6ffb-450b-add1-29652b64de10@gmail.com> On 23/07/2026 19:58, Phillip Wood wrote: > On 22/07/2026 09:15, Son Luong Ngoc via GitGitGadget wrote: >> From: Son Luong Ngoc >> >> 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. > >> Signed-off-by: Son Luong Ngoc >> --- >>   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(). >> +        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. > >> +        /* >> +         * 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 > >> +        path = branch_checked_out(decoration->name); >> + > > This belongs in the next patch I think. > >> 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=% \ >>            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? > >> 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 && > > 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? > >>       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 > >>       q_to_tab >expect <<-\EOF && >>       Successfully rebased and updated refs/heads/update-refs. >