Re: [PATCH v2] rebase: skip branch symref aliases

On 04/06/2026 16:46, Phillip Wood wrote:
> 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
<[email protected]> wrote:
> On 03/06/2026 11:27, Son Luong Ngoc via GitGitGadget wrote:
> > From: Son Luong Ngoc <[email protected]>
> >
> > 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