Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Junio C Hamano <hidden>
Date: 2023-12-08 17:13:23
Eric Sunshine [off-list ref] writes:
Needs review and documentation updates. I'm not sure if the "Needs review" comment is still applicable since the patch did get some review comments, however, the mentioned documentation update is probably still needed for this series to graduate.
Thanks. I think "-B" being defined as "branch -f <branch>" followed by "checkout <branch>" makes it technically unnecessary to add any new documentation (because "checkout <branch>" will refuse, so it naturally follows that "checkout -B <branch>" should), but giving the failure mode a bit more explicit mention would be more helpful to readers. Here is to illustrate what I have in mind. The mention of the "transactional" was already in the documentation for the "checkout" back when switch was described at d787d311 (checkout: split part of it to new command 'switch', 2019-03-29), but somehow was left out in the documentation of the "switch". While it is not incorrect to say that it is a convenient short-cut, it is more important to say what happens when one of them fails, so I am tempted to port that description over to the "switch" command, and give the "used elsewhere" as a sample failure mode. The test has been also enhanced to check the "transactional" nature. Documentation/git-checkout.txt | 4 +++- Documentation/git-switch.txt | 9 +++++++-- t/t2400-worktree-add.sh | 18 ++++++++++++++++-- 3 files changed, 26 insertions(+), 5 deletions(-)
diff --git c/Documentation/git-checkout.txt w/Documentation/git-checkout.txt
index 240c54639e..55a50b5b23 100644
--- c/Documentation/git-checkout.txt
+++ w/Documentation/git-checkout.txt@@ -63,7 +63,9 @@ $ git checkout <branch> ------------ + that is to say, the branch is not reset/created unless "git checkout" is -successful. +successful (e.g., when the branch is in use in another worktree, not +just the current branch stays the same, but the branch is not reset to +the start-point, either). 'git checkout' --detach [<branch>]:: 'git checkout' [--detach] <commit>::
diff --git c/Documentation/git-switch.txt w/Documentation/git-switch.txt
index c60fc9c138..6137421ede 100644
--- c/Documentation/git-switch.txt
+++ w/Documentation/git-switch.txt@@ -59,13 +59,18 @@ out at most one of `A` and `B`, in which case it defaults to `HEAD`. -c <new-branch>:: --create <new-branch>:: Create a new branch named `<new-branch>` starting at - `<start-point>` before switching to the branch. This is a - convenient shortcut for: + `<start-point>` before switching to the branch. This is the + transactional equivalent of + ------------ $ git branch <new-branch> $ git switch <new-branch> ------------ ++ +that is to say, the branch is not reset/created unless "git switch" is +successful (e.g., when the branch is in use in another worktree, not +just the current branch stays the same, but the branch is not reset to +the start-point, either). -C <new-branch>:: --force-create <new-branch>::
diff --git c/t/t2400-worktree-add.sh w/t/t2400-worktree-add.sh
index bbcb2d3419..5d5064e63d 100755
--- c/t/t2400-worktree-add.sh
+++ w/t/t2400-worktree-add.sh@@ -129,8 +129,22 @@ 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 + + # we know we are on detached HEAD but just in case ... + git checkout --detach HEAD && + git rev-parse --verify HEAD >old.head && + + git rev-parse --verify refs/heads/newmain >old.branch && + test_must_fail git checkout -B newmain 2>error && + git rev-parse --verify refs/heads/newmain >new.branch && + git rev-parse --verify HEAD >new.head && + + grep "already used by worktree at" error && + test_cmp old.branch new.branch && + test_cmp old.head new.head && + + # and we must be still on the same detached HEAD state + test_must_fail git symbolic-ref HEAD ) '