Re: [PATCH] sequencer: don't clobber HEAD ref name in worktree lookup

[Junio C Hamano <gitster@pobox.com>]

Andy Koppe <andy.koppe@gmail.com> writes:

> add_decorations_to_list() skips the branch that HEAD points to, because
> the rebase updates it anyway, as per aa37f3e1d89 (sequencer: ignore HEAD
> ref under --update-refs, 2022-07-19).
>
> However, it keeps the name of that branch in the static buffer returned
> by refs_resolve_ref_unsafe(). If another branch on the same commit is
> checked out in a different worktree, branch_checked_out() overwrites
> that buffer. If the current branch comes after the other branch in the
> list of decorations, it gets a "checked out" comment in the todo list
> instead of being skipped, and the branch whose name ended up in the
> buffer is skipped in its place.

So obviously broken original, so cleanly corrected.  I very much
like it.

Will queue.  thanks.

>
> Decorations are processed in reverse alphabetical order, so this depends
> on the branch names. The test added in 94304b9f48e (sequencer: comment
> checked-out branch properly, 2024-11-25) happens to trigger the problem
> and expects its result.
>
> Fix it by using refs_resolve_refdup(), correct that test's expectation,
> and add a test with checked-out branches sorting both before and after
> the current branch.
>
> Signed-off-by: Andy Koppe <andy.koppe@gmail.com>
> ---
>  sequencer.c       | 11 ++++++-----
>  t/t3400-rebase.sh | 21 ++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 6dae43e4db..c672b6e127 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6577,11 +6577,11 @@ 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);
> +	char *head_ref = refs_resolve_refdup(get_main_ref_store(the_repository),
> +					     "HEAD",
> +					     RESOLVE_REF_READING,
> +					     NULL,
> +					     NULL);
>  
>  	while (decoration) {
>  		struct todo_item *item;
> @@ -6633,6 +6633,7 @@ static int add_decorations_to_list(const struct commit *commit,
>  		decoration = decoration->next;
>  	}
>  
> +	free(head_ref);
>  	return 0;
>  }
>  
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index accaaba4d4..0e7ef33eb1 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -483,7 +483,26 @@ test_expect_success 'git rebase --update-ref with core.commentChar and branch on
>  	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" actual
> +'
> +
> +test_expect_success 'git rebase --update-refs ignores current branch with branches on worktrees' '
> +	test_when_finished git branch -D base current a-topic z-topic &&
> +	test_when_finished git checkout main &&
> +	test_when_finished git worktree remove z-topic &&
> +	test_when_finished git worktree remove a-topic &&
> +	git checkout -b base main &&
> +	git checkout -b current &&
> +	test_commit msg4 &&
> +	git worktree add a-topic &&
> +	git worktree add z-topic &&
> +	git checkout base &&
> +	test_commit msg5 &&
> +	git checkout current &&
> +	GIT_SEQUENCE_EDITOR="cat >actual" git rebase -i --update-refs base &&
> +	test_grep "# Ref refs/heads/a-topic checked out at" actual &&
> +	test_grep "# Ref refs/heads/z-topic checked out at" actual &&
> +	test_grep ! "refs/heads/current" actual
>  '
>  
>  test_done