Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Nov 23, 2023, 16:33 UTC
Message-ID: <bf848477-b4dd-49d3-8e4b-de0fc3948570@gmail.com>
In-Reply-To: <xmqqpm01au0w.fsf_-_@gitster.g>

Hi Junio

On 23/11/2023 06:00, Junio C Hamano wrote:

Show 12 quoted lines

> "git checkout -B <branch> [<start-point>]", being a "forced" version
> of "-b", switches to the <branch>, after optionally resetting its
> tip to the <start-point>, even if the <branch> is in use in another
> worktree, which is somewhat unexpected.
> 
> Protect the <branch> using the same logic that forbids "git checkout
> <branch>" from touching a branch that is in use elsewhere.
> 
> This is a breaking change that may deserve backward compatibliity
> warning in the Release Notes.  The "--ignore-other-worktrees" option
> can be used as an escape hatch if the finger memory of existing
> users depend on the current behaviour of "-B".

I think this change makes sense and I found the implementation here much easier to understand than a previous attempt at https://lore.kernel.org/git/20230120113553.24655-1-carenas@gmail.com/

Show 5 quoted lines

> Reported-by: Willem Verstraeten <willem.verstraeten@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>   * The documentation might also need updates, but I didn't look at.

This option is documented as an atomic version of

	git branch -f <branch> [<start-point>]
	git checkout <branch>

However "git branch -f <branch>" will fail if the branch is checked out in the current worktree whereas "git checkout -B" succeeds. I think allowing the checkout in that case makes sense for "git checkout -B" but it does mean that description is not strictly accurate. I'm not sure it matters that much though.

The documentation for "switch -C" is a bit lacking compared to "checkout -B" but that is a separate problem.

Show 19 quoted lines

> 
>   builtin/checkout.c      | 7 +++++++
>   t/t2060-switch.sh       | 2 ++
>   t/t2400-worktree-add.sh | 8 ++++++++
>   3 files changed, 17 insertions(+)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index b4ab972c5a..8a8ad23e98 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1600,6 +1600,13 @@ static int checkout_branch(struct checkout_opts *opts,
>   	if (new_branch_info->path && !opts->force_detach && !opts->new_branch)
>   		die_if_switching_to_a_branch_in_use(opts, new_branch_info->path);
>   
> +	/* "git checkout -B <branch>" */
> +	if (opts->new_branch_force) {
> +		char *full_ref = xstrfmt("refs/heads/%s", opts->new_branch);
> +		die_if_switching_to_a_branch_in_use(opts, full_ref);
> +		free(full_ref);

At the moment this is academic as neither of the test scripts changed by this patch are leak free and so I don't think we need to worry about it but it raises an interesting question about how we should handle memory leaks when dying. Leaving the leak when dying means that a test script that tests an expected failure will never be leak free but using UNLEAK() would mean we miss a leak being introduced in the successful case should the call to "free()" ever be removed. We could of course rename die_if_checked_out() to error_if_checked_out() and return an error instead of dying but that seems like a lot of churn just to keep the leak checker happy.

Best Wishes

Phillip

Show 39 quoted lines

> +	}
> +
>   	if (!new_branch_info->commit && opts->new_branch) {
>   		struct object_id rev;
>   		int flag;
> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh
> index e247a4735b..c91c4db936 100755
> --- a/t/t2060-switch.sh
> +++ b/t/t2060-switch.sh
> @@ -170,8 +170,10 @@ test_expect_success 'switch back when temporarily detached and checked out elsew
>   	# we test in both worktrees to ensure that works
>   	# as expected with "first" and "next" worktrees
>   	test_must_fail git -C wt1 switch shared &&
> +	test_must_fail git -C wt1 switch -C shared &&
>   	git -C wt1 switch --ignore-other-worktrees shared &&
>   	test_must_fail git -C wt2 switch shared &&
> +	test_must_fail git -C wt2 switch -C shared &&
>   	git -C wt2 switch --ignore-other-worktrees shared
>   '
>   
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index df4aff7825..bbcb2d3419 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -126,6 +126,14 @@ test_expect_success 'die the same branch is already checked out' '
>   	)
>   '
>   
> +test_expect_success 'refuse to reset a branch in use elsewhere' '
> +	(
> +		cd here &&
> +		test_must_fail git checkout -B newmain 2>actual &&
> +		grep "already used by worktree at" actual
> +	)
> +'
> +
>   test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
>   	head=$(git -C there rev-parse --git-path HEAD) &&
>   	ref=$(git -C there symbolic-ref HEAD) &&