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:
>> From: Son Luong Ngoc <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>>   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.
>