Re: [PATCH v3 1/2] rebase: skip branch symref aliases
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 24, 2026, 09:55 UTC
- 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:
Show 81 quoted lines
> On 22/07/2026 09:15, Son Luong Ngoc via GitGitGadget wrote:
>> 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.
>
> 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 <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;
>> + }
>
> 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().
Show 24 quoted lines
>> + 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
Show 86 quoted lines
> >> + 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. >