| Message ID | xmqqpm01au0w.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi Junio On 23/11/2023 06:00, Junio C Hamano wrote: > "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/ > 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. > > 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 > + } > + > 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) &&
On Thu, Nov 23, 2023 at 11:33 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 23/11/2023 06:00, Junio C Hamano wrote: > > "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/ Thanks for digging up this link. Upon reading the problem report, I felt certain that we had seen this issue reported previously and that patches had been proposed, but I was unable to find the conversation (despite having taken part in it). I agree, also, that this two-patch series is simple to digest.
Eric Sunshine <sunshine@sunshineco.com> writes: > Thanks for digging up this link. Upon reading the problem report, I > felt certain that we had seen this issue reported previously and that > patches had been proposed, but I was unable to find the conversation > (despite having taken part in it). I am surprised that I did not remember that old discussion at all, and I am doubly surprised that I still do not, even though I clearly recognise my writing in the thread. > I agree, also, that this two-patch series is simple to digest. OK.
Phillip Wood <phillip.wood123@gmail.com> writes: >> 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. Is there a leak here? The piece of memory is pointed at by an on-stack variable full_ref when leak sanitizer starts scanning the heap and the stack just before the process exits due to die, so I do not see a reason to worry about this particular variable over all the other on stack variables we accumulated before the control reached this point of the code. Are you worried about optimizing compilers that behave more cleverly than their own good to somehow lose the on-stack reference to full_ref while calling die_if_switching_to_a_branch_in_use()? We might need to squelch them with UNLEAK() but that does not mean we have to remove the free() we see above, and I suspect a more productive use of our time to solve that issue is ensure that our leak-sanitizing build will not triger such an unwanted optimization anyway. Thanks.
On Mon, Nov 27, 2023 at 10:51:00AM +0900, Junio C Hamano wrote: > >> + 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. > > Is there a leak here? The piece of memory is pointed at by an on-stack > variable full_ref when leak sanitizer starts scanning the heap and > the stack just before the process exits due to die, so I do not see > a reason to worry about this particular variable over all the other > on stack variables we accumulated before the control reached this > point of the code. Right, I think the only reasonable approach here is to not consider this a leak. We've discussed this in the past. Here's a link into a relevant thread for reference, but I don't think it's really worth anybody's time to re-visit: https://lore.kernel.org/git/Y0+i1G5ybdhUGph2@coredump.intra.peff.net/ > Are you worried about optimizing compilers that behave more cleverly > than their own good to somehow lose the on-stack reference to > full_ref while calling die_if_switching_to_a_branch_in_use()? We > might need to squelch them with UNLEAK() but that does not mean we > have to remove the free() we see above, and I suspect a more > productive use of our time to solve that issue is ensure that our > leak-sanitizing build will not triger such an unwanted optimization > anyway. We did have that problem, but it should no longer be the case after d3775de074 (Makefile: force -O0 when compiling with SANITIZE=leak, 2022-10-18). If that is not sufficient for some compiler/code combo, I'd be interested to hear about it. -Peff
Hi Junio On 27/11/2023 01:51, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> 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. > > Is there a leak here? The piece of memory is pointed at by an on-stack > variable full_ref when leak sanitizer starts scanning the heap and > the stack just before the process exits due to die, so I do not see > a reason to worry about this particular variable over all the other > on stack variables we accumulated before the control reached this > point of the code. Oh, good point. I was thinking "we exit without calling free() so it is leaked" but as you say the leak checker (thankfully) does not consider it a leak as there is still a reference to the allocation on the stack. Sorry for the noise Phillip > Are you worried about optimizing compilers that behave more cleverly > than their own good to somehow lose the on-stack reference to > full_ref while calling die_if_switching_to_a_branch_in_use()? We > might need to squelch them with UNLEAK() but that does not mean we > have to remove the free() we see above, and I suspect a more > productive use of our time to solve that issue is ensure that our > leak-sanitizing build will not triger such an unwanted optimization > anyway. > > Thanks.
Hi everyone, It's not clear for me from the email thread what the status is of this bug report, and whether there is still something expected from me. Is the current consensus that this is a real issue that needs fixing? If so, does the current patch-set fix the issue, and how does the fix get into (one of) the next release(s)? Do I still need to do something? Kind regards, Willem On Thu, 30 Nov 2023 at 16:22, Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Junio > > On 27/11/2023 01:51, Junio C Hamano wrote: > > Phillip Wood <phillip.wood123@gmail.com> writes: > > > >> 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. > > > > Is there a leak here? The piece of memory is pointed at by an on-stack > > variable full_ref when leak sanitizer starts scanning the heap and > > the stack just before the process exits due to die, so I do not see > > a reason to worry about this particular variable over all the other > > on stack variables we accumulated before the control reached this > > point of the code. > > Oh, good point. I was thinking "we exit without calling free() so it is > leaked" but as you say the leak checker (thankfully) does not consider > it a leak as there is still a reference to the allocation on the stack. > > Sorry for the noise > > Phillip > > > Are you worried about optimizing compilers that behave more cleverly > > than their own good to somehow lose the on-stack reference to > > full_ref while calling die_if_switching_to_a_branch_in_use()? We > > might need to squelch them with UNLEAK() but that does not mean we > > have to remove the free() we see above, and I suspect a more > > productive use of our time to solve that issue is ensure that our > > leak-sanitizing build will not triger such an unwanted optimization > > anyway. > > > > Thanks.
On Mon, Dec 4, 2023 at 7:21 AM Willem Verstraeten <willem.verstraeten@gmail.com> wrote: > It's not clear for me from the email thread what the status is of this > bug report, and whether there is still something expected from me. > > Is the current consensus that this is a real issue that needs fixing? > If so, does the current patch-set fix the issue, and how does the fix > get into (one of) the next release(s)? > > Do I still need to do something? According to Junio's latest "What's cooking"[1], the status of this patch series is: * jc/checkout-B-branch-in-use (2023-11-23) 2 commits - checkout: forbid "-B <branch>" from touching a branch used elsewhere - checkout: refactor die_if_checked_out() caller "git checkout -B <branch> [<start-point>]" allowed a branch that is in use in another worktree to be updated and checked out, which might be a bit unexpected. The rule has been tightened, which is a breaking change. "--ignore-other-worktrees" option is required to unbreak you, if you are used to the current behaviour that "-B" overrides the safety. 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. I can't speak for what Junio had in mind, but perhaps sufficient would be to add a side-note to the description of the -B option saying that it historically (accidentally) would succeed even if the named branch was checked out in another worktree, but now requires --ignore-other-worktrees. To move the series forward, someone will probably need to make the necessary documentation update. That someone could be you, if you're interested, either by rerolling Junio's series and modifying patch [2/2] to also make the necessary documentation update, or by posting a patch, [3/2] atop his series which updates the documentation. [1]: https://lore.kernel.org/git/xmqq8r6j1dgt.fsf@gitster.g/
Eric Sunshine <sunshine@sunshineco.com> 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 ) '
Hi all, Sorry for dropping out of the conversation, but I see that the changes landed on the master, ready for the 2.44.0 release. Thank you very much! On Fri, 8 Dec 2023 at 18:13, Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> 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 > ) > ' >
Willem Verstraeten <willem.verstraeten@gmail.com> writes: > Sorry for dropping out of the conversation, but I see that the changes > landed on the master, ready for the 2.44.0 release. > > Thank you very much! Thanks for your initial report that led to the update.
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); + } + 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) &&
"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". 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. builtin/checkout.c | 7 +++++++ t/t2060-switch.sh | 2 ++ t/t2400-worktree-add.sh | 8 ++++++++ 3 files changed, 17 insertions(+)