Re: [PATCH v2] rebase: skip branch symref aliases
- From
Son Luong Ngoc <sluongng@gmail.com>
- Date
- Jul 22, 2026, 08:16 UTC
- Message-ID
- <CAL3xRKdoGpNOdbY7_dt3oSfn6728gf8uA4qa79Kcp1UuS=jpiQ@mail.gmail.com>
- In-Reply-To
- <f982c386-e329-4ab0-b695-e540bcb9de3d@gmail.com>
On 04/06/2026 16:46, Phillip Wood wrote:
Show 11 quoted lines
> A symref that points to another branch should always be skipped. When we > look up which branches are checked out (see worktree.c:add_head_info()) we > use > > refs_resolve_ref_unsafe(get_worktree_ref_store(wt), > "HEAD", > 0, > &wt->head_oid, &flags); > > so it will never report a symref as being checked out - it always resolves > any symrefs first.
Yes, this is the right distinction. Patch 1 now resolves each local branch decoration before deciding whether to queue it. If the target is under refs/heads/, the alias is skipped unconditionally and the concrete branch decoration remains the only update that is queued.
> If we have a symref pointing somewhere outside of "refs/heads" then we > need to check whether the target is checked out, not the symref itself. > I'm not sure how likely that is to happen in practice.
Patch 2 handles that case separately. It checks both the literal alias and its resolved target ref against the checked-out reservations. I also added a test with a non-branch target checked out in another worktree.
> If a decoration matches the current branch why don't we just skip it like > we used to? (As an aside the existing code in wrong because if the user > runs "git rebase --update-refs <upstream> <branch>" HEAD does not point to > "<branch>" but lets not worry about that now)
Agreed. Patch 1 now skips the current branch before checked-out handling, as the old code did.
The contrary expectation in t3400 came from head_ref pointing into a buffer that was reused while resolving another decoration. That could make the current-branch comparison fail. head_ref is now an owned copy. The test expects the current branch to be omitted from the todo list.
While adding the non-branch coverage, I found that two aliases to the same target ref could queue the same update twice and make the second compare-and-swap fail. Patch 2 deduplicates those updates by target ref. It also records resolved target refs from other worktrees' in-progress update-refs state so that a different alias honors the same reservation.
I split the reroll into these two patches so that the branch-alias fix and the non-branch safeguards can be reviewed independently.
Thanks for the review, and sorry for the slow response.
Thanks, Son
On Thu, 4 Jun 2026 16:37:39 +0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 101 quoted lines
> On 03/06/2026 11:27, Son Luong Ngoc via GitGitGadget wrote:
> > 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.
>
> Makes sense
>
> > 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.
>
> I don't quite understand this. A symref that points to another branch
> should always be skipped. When we look up which branches are checked out
> (see worktree.c:add_head_info()) we use
>
> refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
> "HEAD",
> 0,
> &wt->head_oid, &flags);
>
> so it will never report a symref as being checked out - it always
> resolves any symrefs first.
>
> If we have a symref pointing somewhere outside of "refs/heads" then we
> need to check whether the target is checked out, not the symref itself.
> I'm not sure how likely that is to happen in practice.
>
> > 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);
>
> This part and the test look good now
> > 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;
> > + }
>
> If a decoration matches the current branch why don't we just skip it
> like we used to? (As an aside the existing code in wrong because if the
> user runs "git rebase --update-refs <upstream> <branch>" HEAD does not
> point to "<branch>" but lets not worry about that now)
>
> > + path = branch_checked_out(decoration->name);
>
> As I said above if the symref target is anther branch we should skip it
> and if the target is not a branch then we need to check if the target is
> checked out so we need to resolve the ref before calling
> branch_checked_out().
>
> Thanks
>
> Phillip