Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 19, 2023, 14:21 UTC
Message-ID: <6a0499d4-ad6e-56b7-de72-0f8a3697b534@dunelm.org.uk>
In-Reply-To: <20230119055325.1013-1-carenas@gmail.com>

Hi Carlo

Thanks for working on this

On 19/01/2023 05:53, Carlo Marcelo Arenas Belón wrote:

Show 20 quoted lines

> Commands `git switch -C` and `git checkout -B` neglect to check whether
> the provided branch is already checked out in some other worktree, as
> shown by the following:
> 
>    $ git worktree list
>    .../foo    beefb00f [main]
>    $ git worktree add ../other
>    Preparing worktree (new branch 'other')
>    HEAD is now at beefb00f first
>    $ cd ../other
>    $ git switch -C main
>    Switched to and reset branch 'main'
>    $ git worktree list
>    .../foo    beefb00f [main]
>    .../other  beefb00f [main]
> 
> Fix this problem by teaching `git switch -C` and `git checkout -B` to
> check whether the branch in question is already checked out elsewhere
> by expanding on the existing checks that are being used by their non
> force variants.

Show 6 quoted lines

> Unlike what it is done for `git switch` and `git checkout`, that have
> an historical exception to ignore other workspaces if the branch to
> check is the current one (as required when called as part of other
> tools), the logic implemented is more strict and will require the user
> to invoke the command with `--ignore-other-worktrees` to explicitly
> indicate they want the risky behaviour.

> This matches the current behaviour of `git branch -f` and is safer, for
> more details see the tests in t2400.

I think it would be helpful to spell out the behavior of

	git checkout $current_branch
	git checkout -B $current_branch [<commit>]
	git checkout -B $current_branch --ignore-other-worktrees [<commit>]

when the current branch is and is not checked out in another worktree as the tests are hard to follow because they rely on worktrees set up previous tests.

Show 18 quoted lines

> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> @@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>   		strbuf_release(&buf);
>   	}
>   
> -	if (opts->patch_mode || opts->pathspec.nr)
> +	if (opts->patch_mode || opts->pathspec.nr) {
> +		free(check_branch_path);
>   		return checkout_paths(opts, new_branch_info);
> +	}
>   	else
> -		return checkout_branch(opts, new_branch_info);
> +		return checkout_branch(opts, new_branch_info, check_branch_path);
>   }

I found the ownership of check_branch_path confusing here. I think it would be clearer to do

	if (opts->patch_mode || opts->pathspec.nr)
		ret = checkout_path(...);
	else
		ret = checkout_branch(...);
	free(check_branch_path);
	return ret;

 > [...]

> +test_expect_success 'but die if using force without --ignore-other-worktrees' '

I'm not sure from the title what this test is checking. Having added "git worktree list" and run it is checking that when the current branch is checked out elsewhere we require --ignore-other-worktrees when resetting the current branch.

Show 6 quoted lines

> +	(
> +		cd there &&
> +		test_must_fail git checkout -B newmain &&
> +		test_must_fail git switch -C newmain &&
> +		git checkout --ignore-other-worktrees -B newmain &&
> +		git switch --ignore-other-worktrees -C newmaain >   	)

I tried running

	git switch -C main newbranch

from the main worktree (which is the only worktree that has branch 'main' checked out) to check that we did not error out when the branch is not checked out elsewhere and was surprised it failed with

	fatal: 'newbranch' is already checked out at '/dev/shm/trash 
directory.t2400-worktree-add/short-hand'

It works as expected without this patch so it looks like there is a bug somewhere.

Best Wishes

Phillip