Re: [PATCH 2/2] rebase: skip branch symref aliases

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 1, 2026, 14:10 UTC
Message-ID: <f1b662d5-f9bb-4274-ad42-3a2227d2a060@gmail.com>
In-Reply-To: <0ab0a717441e9fc7c494da194065a948a35a7f01.1779946921.git.gitgitgadget@gmail.com>

On 28/05/2026 06:42, Son Luong Ngoc via GitGitGadget wrote:

Show 7 quoted lines

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

Show 25 quoted lines

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

Show 36 quoted lines

> +	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.

Show 5 quoted lines

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

Show 60 quoted lines

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