rebase: handle --update-refs branch symrefs#2126
Open
sluongng wants to merge 1 commit into
Open
Conversation
sluongng
force-pushed
the
sl/rebase-update-refs-symrefs
branch
from
9938d7e to
0ab0a71
Compare
Author
|
/preview |
|
Preview email sent as pull.2126.git.1779908024.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2126.git.1779943633.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2126.git.1779943735.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2126.git.1779944585.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2126.git.1779946921.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -6445,28 +6445,67 @@ static int add_decorations_to_list(const struct commit *commit, | |||
| struct todo_add_branch_context *ctx) | |||
There was a problem hiding this comment.
"Kristoffer Haugsbakk" wrote on the Git mailing list (how to reply to this email):
On Thu, May 28, 2026, at 07:42, Son Luong Ngoc via GitGitGadget wrote:
>[snip]
> -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 &&
This style of fixing a bug by:
• Add failing test `test_expect_failure` in the first commit
• Fix the bug in the next commit and flip to `test_expect_success`
Is legitimate and makes it easier to verify that the test really
exercises the regression. But in this project it is preferred to
just do the bug fix + regression test in one patch.
See https://lore.kernel.org/git/xmqqfrdk3aqy.fsf@gitster.g/
> --
> gitgitgadget
|
User |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Son Luong Ngoc via GitGitGadget" <gitgitgadget@gmail.com> writes:
> git rebase --update-refs can fail after the normal rebase path has
> successfully updated the current branch when another local branch is a
> symbolic ref to it.
>
> One practical way to arrive at that setup is a default branch rename from
> master to main. While the migration is in progress, a user may keep
> refs/heads/main as a symbolic ref to refs/heads/master so that both names
> continue to work locally.
>
> If pull.rebase is enabled, a plain git pull can then finish the rebase of
> master and still fail while trying to update the main alias. The reported
> failure looked like this, with line breaks adjusted for the cover letter:
>
> Successfully rebased and updated refs/heads/master.
> error: update_ref failed for ref 'refs/heads/main':
> cannot lock ref 'refs/heads/main':
> is at fc2c7bd5f17abec7861ef759edcd33a1e16662a1
> but expected 531cabdfb49098d6ffa502ed4bf91d1b35edfcfa
> Updated the following refs with --update-refs:
> Failed to update the following refs with --update-refs:
> refs/heads/main
I vaguely recall we saw a different topic that dealt with a
situation somewhat similar to this topic (I think it was about
'describe' giving a name that is not a branch). How would this mesh
with what the other topic wanted to do? Instead of filtering out
non-branch names (which the other topic did), here we want to filter
out names that are not concrete branches but pointers to something
else. Would it mean that the logic here is more broad (i.e., both
wants to filter out names of non-branches), making the other topic
unnecessary?
|
|
This patch series was integrated into seen via git@d871d43. |
sluongng
force-pushed
the
sl/rebase-update-refs-symrefs
branch
from
0ab0a71 to
c1e8828
Compare
Author
|
/preview |
|
Preview email sent as pull.2126.v2.git.1780062147590.gitgitgadget@gmail.com |
sluongng
force-pushed
the
sl/rebase-update-refs-symrefs
branch
from
c1e8828 to
b59eb4e
Compare
Author
|
/preview |
|
Preview email sent as pull.2126.v2.git.1780068269833.gitgitgadget@gmail.com |
|
This branch is now known as |
|
This patch series was integrated into seen via git@eb72804. |
|
This patch series was integrated into seen via git@9ced507. |
| ) && | ||
| grep ^update-ref todo >actual && | ||
| test_write_lines "update-ref refs/heads/no-conflict-branch" >expect && | ||
| test_cmp expect actual |
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 queues local branch decorations by their literal
> refnames. When a branch such as refs/heads/main is a symbolic ref to
> the current branch, the normal rebase path first updates the current
> branch and the queued symref update later tries to update the same
> referent with the old value it recorded before the rebase.
> > Add a known-breakage test that exercises this case so that the fix can
> flip it to test_expect_success. The expected behavior is that the branch
> symref keeps pointing at the rebased current branch.
Thanks for adding a test, I'd find it easier to review this series if the test was added in the same patch as the fix which is our usual practice.
> +test_expect_failure '--update-refs skips branch symrefs to current branch' '
> + test_when_finished "
> + test_might_fail git rebase --abort &&
> + git checkout primary &&
> + test_might_fail git symbolic-ref -d refs/heads/update-refs-symref-alias &&
> + test_might_fail git branch -D update-refs-symref update-refs-symref-base
> + " &&
> + git checkout -B update-refs-symref-base primary &&
> + test_commit --no-tag update-refs-symref-base symref-base.t &&
> + git checkout -B update-refs-symref &&
> + test_commit --no-tag update-refs-symref-topic symref-topic.t &&
> + git checkout update-refs-symref-base &&
> + test_commit --no-tag update-refs-symref-newbase symref-newbase.t &&
> + git checkout update-refs-symref &&
> + git symbolic-ref refs/heads/update-refs-symref-alias refs/heads/update-refs-symref &&
I think we want to test a symref that does not match HEAD as well. Rather than adding a new test, can we instead add a couple of symref branches to the test "--update-refs updates refs correctly"?
Thanks
Phillip
> +
> + git rebase --update-refs update-refs-symref-base 2>err &&
> +
> + test_cmp_rev update-refs-symref-base update-refs-symref^ &&
> + test_cmp_rev refs/heads/update-refs-symref refs/heads/update-refs-symref-alias &&
> + test_write_lines refs/heads/update-refs-symref >expect &&
> + git symbolic-ref refs/heads/update-refs-symref-alias >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success '--update-refs updates refs correctly' '
> git checkout -B update-refs no-conflict-branch &&
> git branch -f base HEAD~4 &&
|
User |
| @@ -6445,28 +6445,67 @@ 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 &&
|
This patch series was integrated into seen via git@b977dbc. |
|
This patch series was integrated into seen via git@389dbe0. |
|
There was a status update in the "New Topics" section about the branch "git rebase --update-refs" has been taught to resolve local branch symrefs to their referents before queuing updates. This correctly skips aliases of the current branch and avoids duplicate updates for underlying real branches, fixing failures when branch aliases (like a default branch rename) are present. Waiting for response(s) to review comment(s). cf. <xmqqwlwni7vk.fsf@gitster.g> cf. <f1b662d5-f9bb-4274-ad42-3a2227d2a060@gmail.com> source: <pull.2126.git.1779946921.gitgitgadget@gmail.com> |
sluongng
force-pushed
the
sl/rebase-update-refs-symrefs
branch
from
b59eb4e to
4b6c5ca
Compare
Author
|
/preview |
|
Preview email sent as pull.2126.v2.git.1780474806447.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2126.v2.git.1780474941165.gitgitgadget@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 106b688 (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. 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. Signed-off-by: Son Luong Ngoc <sluongng@gmail.com>
sluongng
force-pushed
the
sl/rebase-update-refs-symrefs
branch
from
4b6c5ca to
68f6982
Compare
Author
|
/preview |
|
Preview email sent as pull.2126.v2.git.1780478721315.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2126.v2.git.1780482436865.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Changes since v1:
symrefs that are not checked out when their targets resolve to
refs/heads/*, while preserving the existing behavior for symrefs to
non-branch targets.
skip, so worktrees still get the existing todo-list warning instead of
an update-ref command.
"--update-refs updates refs correctly" test, covering both an alias of
HEAD and an alias of another branch.
decorations are already filtered, and this patch handles the remaining
local branch decorations that are symref aliases.
cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com
cc: Phillip Wood phillip.wood123@gmail.com