Skip to content

rebase: handle --update-refs branch symrefs #2126

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sluongng wants to merge 1 commit into
base: master
Choose a base branch
from
+46 −12
Open

rebase: handle --update-refs branch symrefs #2126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions sequencer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6445,28 +6445,46 @@ static int add_decorations_to_list(const struct commit *commit,
struct todo_add_branch_context *ctx)
Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot May 28, 2026

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):

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

Copy link
Copy Markdown

@gitgitgadget gitgitgadget Bot Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 &&

{
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;
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;
}

path = branch_checked_out(decoration->name);

/*
* 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, unless it is checked out and
* needs a comment in the todo list.
*/
if (!path && head_ref && !strcmp(head_ref, decoration->name)) {
decoration = decoration->next;
continue;
}

resolved_ref = refs_resolve_ref_unsafe(refs, decoration->name,
RESOLVE_REF_READING,
NULL, &flags);
if (!path && resolved_ref && (flags & REF_ISSYMREF) &&
starts_with(resolved_ref, "refs/heads/")) {
decoration = decoration->next;
continue;
}
Expand All @@ -6478,7 +6496,7 @@ 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) {
item->command = TODO_COMMENT;
strbuf_commented_addf(ctx->buf, comment_line_str,
"Ref %s checked out at '%s'\n",
Expand All @@ -6501,6 +6519,7 @@ static int add_decorations_to_list(const struct commit *commit,
decoration = decoration->next;
}

free(head_ref);
return 0;
}

Expand Down
15 changes: 15 additions & 0 deletions t/t3404-rebase-interactive.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1979,20 +1979,35 @@ test_expect_success '--update-refs ignores non-branch decorations' '
'

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 &&
git rebase -i --autosquash --update-refs primary 2>err &&
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 &&
q_to_tab >expect <<-\EOF &&
Successfully rebased and updated refs/heads/update-refs.
Expand Down
Loading