Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees
From: Phillip Wood <hidden>
Date: 2023-01-19 14:21:19
Hi Carlo Thanks for working on this On 19/01/2023 05:53, Carlo Marcelo Arenas Belón wrote:
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.
>
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.
quoted hunk ↗ jump to hunk
Reported-by: Jinwook Jeong <redacted> Helped-by: Rubén Justo [off-list ref] Helped-by: Eric Sunshine [off-list ref] Signed-off-by: Carlo Marcelo Arenas Belón <redacted> ---@@ -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.
+ ( + 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