rebase: handle --update-refs branch symrefs #2126
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6459,34 +6459,71 @@ struct todo_add_branch_context { | |
| size_t items_alloc; | ||
| struct strbuf *buf; | ||
| struct string_list refs_to_oids; | ||
| struct string_list symref_update_targets; | ||
| }; | ||
|
|
||
| static int add_decorations_to_list(const struct commit *commit, | ||
| struct todo_add_branch_context *ctx) | ||
|
There was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 28/05/2026 06:42, Son Luong Ngoc via GitGitGadget wrote:
> From: Son Luong Ngoc <sluongng@gmail.com>
> > rebase --update-refs records local branch decorations before replaying
> commits. If a decoration is a symbolic branch such as refs/heads/main
> pointing at refs/heads/master, updating it later dereferences back to
> master and can fail because the normal rebase path already moved that
> branch.
Good explanation, thanks for working on this.
> Resolve local branch symref decorations to their referents before
s/referents/targets/ ?
> queuing update-ref commands, and skip duplicates. This keeps branch
> aliases from scheduling a second update for the same underlying branch
> while still using the existing old-OID check for the single queued
> update.
That's not quite what the patch does though - it only checks that the target of the symref differs from the target of HEAD. If a symref points to another branch we still try to update it when we should skip it.
> Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
> ---
> sequencer.c | 63 +++++++++++++++++++++++++++++------
> t/t3404-rebase-interactive.sh | 2 +-
> 2 files changed, 53 insertions(+), 12 deletions(-)
> > diff --git a/sequencer.c b/sequencer.c
> index 1ee4b2875b..4a83d1337c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6445,15 +6445,22 @@ 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",
> + struct ref_store *refs = get_main_ref_store(the_repository);
> + const char *head_ref = refs_resolve_ref_unsafe(refs, "HEAD",
> RESOLVE_REF_READING,
> - NULL,
> - NULL);
> + NULL, NULL);
> + char *resolved_head_ref = refs_resolve_refdup(refs, "HEAD",
> + RESOLVE_REF_READING,
> + NULL, NULL);
We need to use refs_resolve_refdup() instead of refs_resolve_ref_unsafe() so that the return value is not overwritten by the later calls to refs_resolve_ref_unsafe() that are added below. But that is the only change that is needed - we do not need to add a new variable, we just replace refs_resolve_ref_unsafe() with refs_resole_refdup() and free "head_ref" before we return.
> + struct strbuf update_ref = STRBUF_INIT;
> > while (decoration) {
> struct todo_item *item;
> const char *path;
> + const char *ref = decoration->name;
> + const char *resolved_ref;
> + int is_symref = 0;
> + int flags = 0;
> size_t base_offset = ctx->buf->len;
> > /*
> @@ -6461,12 +6468,44 @@ static int add_decorations_to_list(const struct commit *commit,
> * updated by the default rebase behavior.
> * Exclude it from the list of refs to update,
> * as well as any non-branch decorations.
> + *
> + * Resolve branch symrefs after checking for the current HEAD so
> + * that aliases do not schedule duplicate updates for their
> + * referents.
> + *
> * 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 (head_ref && !strcmp(head_ref, ref)) {
> + decoration = decoration->next;
> + continue;
> + }
This is just rewriting the existing if statement which has nothing to do with the stated aim of this patch - lets leave it as it was.
> +
> + strbuf_reset(&update_ref);
> + resolved_ref = refs_resolve_ref_unsafe(refs, ref,
> + RESOLVE_REF_READING |
> + RESOLVE_REF_NO_RECURSE,
Why are we passing RESOLVE_REF_NO_RECURSE here? I'd have thought we want to resolve the whole chain of symbolic refs to find out which ref is actually going to be updated.
> + NULL, &flags);
> + if ((flags & REF_ISSYMREF) && resolved_ref) {
I think it is generally safer to check the return value before using any of the "out" parameters from a function call. In this case the function unconditionally clears flags at the beginning so it is safe.
> + if (!starts_with(resolved_ref, "refs/heads/")) {
> + decoration = decoration->next;
> + continue;
This is the opposite of what I was expecting - if the decoration is a symref that resolves to a branch then that branch will also be in the list of decorations and so will be updated. If the decoration is a symref that resolves outside "refs/heads/" then we want to add the decoration to the list of refs to update to keep the current behavior.
If we do that then we skip all symbolic refs that point to another branch, instead of just skipping those that match HEAD and we don't need any of the changes below here.
Thanks
Phillip
> + }
> +
> + strbuf_addstr(&update_ref, resolved_ref);
> + ref = update_ref.buf;
> + is_symref = 1;
> + }
> +
> + if ((is_symref && resolved_head_ref &&
> + !strcmp(resolved_head_ref, ref)) ||
> + string_list_has_string(&ctx->refs_to_oids, ref)) {
> decoration = decoration->next;
> continue;
> }
> @@ -6478,19 +6517,19 @@ static int add_decorations_to_list(const struct commit *commit,
> memset(item, 0, sizeof(*item));
> > /* If the branch is checked out, then leave a comment instead. */
> - if ((path = branch_checked_out(decoration->name))) {
> + if ((path = branch_checked_out(ref))) {
> item->command = TODO_COMMENT;
> strbuf_commented_addf(ctx->buf, comment_line_str,
> "Ref %s checked out at '%s'\n",
> - decoration->name, path);
> + ref, path);
> } else {
> struct string_list_item *sti;
> item->command = TODO_UPDATE_REF;
> - strbuf_addf(ctx->buf, "%s\n", decoration->name);
> + strbuf_addf(ctx->buf, "%s\n", ref);
> > sti = string_list_insert(&ctx->refs_to_oids,
> - decoration->name);
> - sti->util = init_update_ref_record(decoration->name);
> + ref);
> + sti->util = init_update_ref_record(ref);
> }
> > item->offset_in_buf = base_offset;
> @@ -6501,6 +6540,8 @@ static int add_decorations_to_list(const struct commit *commit,
> decoration = decoration->next;
> }
> > + strbuf_release(&update_ref);
> + free(resolved_head_ref);
> return 0;
> }
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 42ba8cc313..29447c0fc3 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1978,7 +1978,7 @@ test_expect_success '--update-refs ignores non-branch decorations' '
> test_cmp expect actual
> '
> > -test_expect_failure '--update-refs skips branch symrefs to current branch' '
> +test_expect_success '--update-refs skips branch symrefs to current branch' '
> test_when_finished "
> test_might_fail git rebase --abort &&
> git checkout primary &&There was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): 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().
> + 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.
> + 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.There was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): 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 <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().
>> + 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.
> There was a problem hiding this comment. Erik Cervin-Edin wrote on the Git mailing list (how to reply to this email): I ran into a minor regression that I tracked down to v2 of this patch
series. When I was running
git rebase --interactive --update-refs
I got a comment in my git-rebase-todo for the branch I was rebasing,
even though it's just the current branch in the current worktree:
# Ref refs/heads/main checked out at '/private/tmp/gittest'
where I'd expect no entry at all, since the branch is updated by the
rebase itself. After applying v3 of the series, the regression went
away. Nevertheless, I thought I ought to share what I found.
On 26/07/24 10:55AM, Phillip Wood wrote:
> > > + if (head_ref && !strcmp(head_ref, decoration->name)) {
> > > + free(resolved_ref);
> > > decoration = decoration->next;
> > > continue;
> > > }
> > > +
> > > + path = branch_checked_out(decoration->name);
> >
> > 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.
branch_checked_out() can't tell "checked out in another worktree"
apart from "checked out right here", so `path` is never NULL for the
branch actually being rebased. In v2, the check above was instead:
if (!path && head_ref && !strcmp(head_ref, decoration->name))
continue;
which made it a no-op for exactly that branch -- the regression I
observed. v3 drops the `!path` gate and moves `path` below it, which
is why it's fixed.
> > As topic2 is checked out in the worktree where the rebase is running
> > why did this line appear before?
This might be the same symptom from another cause: on master, head_ref
comes from refs_resolve_ref_unsafe(), and as you note its buffer is
overwritten inside branch_checked_out(). So by the time topic2 is
compared, wt-topic's lookup may already have clobbered head_ref,
letting topic2 fall through to the comment. I haven't run the test to
confirm, though.
Thanks,
ErikThere was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
>> 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.
>> ...
>> 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().
Makes sense. Thanks for raising a possible alternative and then
clarifying that it is not quite workable.
>>> + /*
>>> + * 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.
Yeah, decoration is very much end-user facing and if we can make
behavioural decision based on a more stable resolved_ref that would
make it easier to reason about.
But stepping back a bit, is having a HEAD that is a symref and
points outside "refs/heads/" an invalid state? Why are we catering
to such a configuration to begin with?There was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 26/07/2026 16:42, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >>> 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.
>>> ...
>>> 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().
> > Makes sense. Thanks for raising a possible alternative and then
> clarifying that it is not quite workable.
> >>>> + /*
>>>> + * 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.
> > Yeah, decoration is very much end-user facing and if we can make
> behavioural decision based on a more stable resolved_ref that would
> make it easier to reason about.
> > But stepping back a bit, is having a HEAD that is a symref and
> points outside "refs/heads/" an invalid state? Why are we catering
> to such a configuration to begin with?
We allow HEAD to point to anything below "refs/" - see e9cc02f0e4 (symbolic-ref: allow refs/<whatever> in HEAD, 2009-02-13). I've not tested it but looking at the code I think rebase handles a non-branch HEAD correctly.
It would be nice if we didn't have to worry about non-branch HEADs, recently Caleb reported on discord being surprised that git allows HEAD to point to a tag.
Thanks
Phillip
There was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
>> But stepping back a bit, is having a HEAD that is a symref and
>> points outside "refs/heads/" an invalid state? Why are we catering
>> to such a configuration to begin with?
>
> We allow HEAD to point to anything below "refs/" - see e9cc02f0e4
> (symbolic-ref: allow refs/<whatever> in HEAD, 2009-02-13).
But that was about a low level mechanism that must be more lenient
to be usable as repair tools to recover from such a broken state,
no? I thought the end-user facing commands like "git checkout" have
been tightened long ago to stop users from getting into a situation
that needs repairing in the first place. And that was why I asked.
There was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 28/07/2026 15:23, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >>> But stepping back a bit, is having a HEAD that is a symref and
>>> points outside "refs/heads/" an invalid state? Why are we catering
>>> to such a configuration to begin with?
>>
>> We allow HEAD to point to anything below "refs/" - see e9cc02f0e4
>> (symbolic-ref: allow refs/<whatever> in HEAD, 2009-02-13).
> > But that was about a low level mechanism that must be more lenient
> to be usable as repair tools to recover from such a broken state,
> no?
It checks the new value of HEAD, not the old one so I don't think so. The commit message talks about topgit using "git symbolic-ref" to set head outside "refs/heads/" - peff had previously tried to tighten it to reject non-branch refs but that broke topgit. I've just had a quick look at the topgit code and still sets HEAD to point to "refs/top-bases/..." by default[1], although there are plans to start using "refs/heads/{top-bases}/..." instead.
> I thought the end-user facing commands like "git checkout" have
> been tightened long ago to stop users from getting into a situation
> that needs repairing in the first place. And that was why I asked.
Yes "git checkout" detaches HEAD if you give a non-branch ref, but external tools can still use "git symbolic-ref" to bypass that. It appears topgit's rebase command is built around "git rebase"[2] so I think we need to continue to support rebasing a non-branch HEAD.
Thanks
Phillip
[1] https://github.com/mackyle/topgit/blob/master/tg.sh#L2683
[2] https://github.com/mackyle/topgit/blob/master/tg-rebase.sh#L56
There was a problem hiding this comment. Junio C Hamano wrote on the Git mailing list (how to reply to this email): Phillip Wood <phillip.wood123@gmail.com> writes:
>> But that was about a low level mechanism that must be more lenient
>> to be usable as repair tools to recover from such a broken state,
>> no?
>
> It checks the new value of HEAD, not the old one so I don't think so.
> The commit message talks about topgit using "git symbolic-ref" to set
> head outside "refs/heads/" - peff had previously tried to tighten it to
> reject non-branch refs but that broke topgit. I've just had a quick look
> at the topgit code and still sets HEAD to point to "refs/top-bases/..."
> by default[1], although there are plans to start using
> "refs/heads/{top-bases}/..." instead.
Ah, that name vaguely rings a bell. Is it still in use, and now
they prevent us from forbidding funny characters like {} in the
refname? Sigh...
> ... It
> appears topgit's rebase command is built around "git rebase"[2] so I
> think we need to continue to support rebasing a non-branch HEAD.
Sigh, again, but OK.
Thanks.There was a problem hiding this comment. Phillip Wood wrote on the Git mailing list (how to reply to this email): On 29/07/2026 15:26, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >>> But that was about a low level mechanism that must be more lenient
>>> to be usable as repair tools to recover from such a broken state,
>>> no?
>>
>> It checks the new value of HEAD, not the old one so I don't think so.
>> The commit message talks about topgit using "git symbolic-ref" to set
>> head outside "refs/heads/" - peff had previously tried to tighten it to
>> reject non-branch refs but that broke topgit. I've just had a quick look
>> at the topgit code and still sets HEAD to point to "refs/top-bases/..."
>> by default[1], although there are plans to start using
>> "refs/heads/{top-bases}/..." instead.
> > Ah, that name vaguely rings a bell. Is it still in use, and now
> they prevent us from forbidding funny characters like {} in the
> refname? Sigh...
Yes, it still seems to be maintained, I guess they chose the funny characters to try and avoid name collisions because no-one would want them in a "normal" branch name.
Thanks
Phillip
>> ... It
>> appears topgit's rebase command is built around "git rebase"[2] so I
>> think we need to continue to support rebasing a non-branch HEAD.
> > Sigh, again, but OK.
> > Thanks.
> |
||
| { | ||
| const struct name_decoration *decoration = get_name_decoration(&commit->object); | ||
| 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; | ||
| const char *checked_ref; | ||
| char *resolved_ref; | ||
| int flags = 0; | ||
| size_t base_offset = ctx->buf->len; | ||
|
|
||
| /* | ||
| * Non-branch decorations may be present if the pretty format | ||
| * includes "%d", which would have loaded all refs | ||
| * into the global decoration table. | ||
| */ | ||
| if (decoration->type != DECORATION_REF_LOCAL) { | ||
| decoration = decoration->next; | ||
| continue; | ||
| } | ||
|
|
||
| /* | ||
| * A symref to another local branch is only an alias. The | ||
| * target branch has its own decoration, so only queue the | ||
| * concrete branch. | ||
| */ | ||
| 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; | ||
| } | ||
|
|
||
| /* | ||
| * If the branch or its referent is the current HEAD, then it | ||
| * will be updated by the default rebase behavior. | ||
| */ | ||
| if (head_ref && resolved_ref && | ||
| !strcmp(head_ref, resolved_ref)) { | ||
| free(resolved_ref); | ||
| decoration = decoration->next; | ||
| continue; | ||
| } | ||
|
|
||
| path = branch_checked_out(decoration->name); | ||
| if (!path && resolved_ref && (flags & REF_ISSYMREF)) { | ||
| checked_ref = resolved_ref; | ||
| path = branch_checked_out(checked_ref); | ||
| } | ||
| if (!path && resolved_ref && (flags & REF_ISSYMREF) && | ||
| string_list_has_string(&ctx->symref_update_targets, | ||
| resolved_ref)) { | ||
| free(resolved_ref); | ||
| decoration = decoration->next; | ||
| continue; | ||
| } | ||
|
|
@@ -6498,13 +6535,17 @@ static int add_decorations_to_list(const struct commit *commit, | |
| memset(item, 0, sizeof(*item)); | ||
|
|
||
| /* If the branch is checked out, then leave a comment instead. */ | ||
| if (path) { | ||
| item->command = TODO_COMMENT; | ||
| strbuf_commented_addf(ctx->buf, comment_line_str, | ||
| "Ref %s checked out at '%s'\n", | ||
| decoration->name, path); | ||
| } else { | ||
| struct string_list_item *sti; | ||
|
|
||
| if (resolved_ref && (flags & REF_ISSYMREF)) | ||
| string_list_insert(&ctx->symref_update_targets, | ||
| resolved_ref); | ||
| item->command = TODO_UPDATE_REF; | ||
| strbuf_addf(ctx->buf, "%s\n", decoration->name); | ||
|
|
||
|
|
@@ -6518,9 +6559,11 @@ static int add_decorations_to_list(const struct commit *commit, | |
| item->arg_len = ctx->buf->len - base_offset; | ||
| ctx->items_nr++; | ||
|
|
||
| free(resolved_ref); | ||
| decoration = decoration->next; | ||
| } | ||
|
|
||
| free(head_ref); | ||
| return 0; | ||
| } | ||
|
|
||
|
|
@@ -6534,6 +6577,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list) | |
| struct todo_add_branch_context ctx = { | ||
| .buf = &todo_list->buf, | ||
| .refs_to_oids = STRING_LIST_INIT_DUP, | ||
| .symref_update_targets = STRING_LIST_INIT_DUP, | ||
| }; | ||
|
|
||
| ctx.items_alloc = 2 * todo_list->nr + 1; | ||
|
|
@@ -6559,6 +6603,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list) | |
| res = write_update_refs_state(&ctx.refs_to_oids); | ||
|
|
||
| string_list_clear(&ctx.refs_to_oids, 1); | ||
| string_list_clear(&ctx.symref_update_targets, 0); | ||
|
|
||
| if (res) { | ||
| /* we failed, so clean up the new list. */ | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Kristoffer Haugsbakk" wrote on the Git mailing list (how to reply to this email):