Thread (13 messages) 13 messages, 4 authors, 2023-05-14

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help