git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox

git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox

[Willem Verstraeten]

# What did you do before the bug happened? (Steps to reproduce your issue)

Clone a repo
Create an additional worktree for that clone
Use `git checkout -B branch-of-primary-clone ...` to checkout the
branch that is already checked out in the primary clone

For example, with the pathfinder repo on GitHub:

    git clone https://github.com/servo/pathfinder.git primary
    cd primary
    git worktree add -b metal ../secondary origin/metal
    cd ../secondary
    git checkout -b main #reports a fatal error, as expected
    git checkout -f main origin/main #also reports a fatal error, as expected
    git checkout -B main origin/main # ----> this succeeds, which is
unexpected <----

# What did you expect to happen? (Expected behavior)

I expected a fatal error stating that the branch could not be checked
out since it was already checked out in my primary worktree

In `git checkout --help`, it is documented that `git checkout -B` is
the atomic equivalent of `git branch -f <branch> <commit> ; git
checkout <branch>` :

> If -B is given, <new-branch> is created if it doesn’t exist; otherwise, it is reset. This is the transactional equivalent of
>
>     $ git branch -f <branch> [<start-point>]
>     $ git checkout <branch>

# What happened instead? (Actual behavior)

The branch was checked out in the secondary worktree. If I then work
and make commits in this secondary worktree, the status of my primary
worktree gets clobbered as well.

What's different between what you expected and what actually happened?

The checkout in the secondary worktree is allowed, but it shouldn't be


[System Info]
git version:
git version 2.41.0
cpu: arm64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul  5 22:21:53
PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T6020 arm64
compiler info: clang: 14.0.3 (clang-1403.0.22.14.1)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]

Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox

[Junio C Hamano]

Willem Verstraeten <willem.verstraeten@gmail.com> writes:

>     git checkout -b main #reports a fatal error, as expected

This is expected because "main" already exists, not because "main"
is checked out elsewhere.

>     git checkout -f main origin/main #also reports a fatal error, as expected

This is expected because origin/main is taken as pathspec, and it is
a request to checkout the paths that match the pathspec out of the
named tree-ish (i.e., "main"), even when these paths have local
changes, but you do not have paths that match "origin/main".  The
failure is not because "main" is checked out elsewhere.

A slight variant of the command

    git checkout -f -b main origin/main

still fails for the same reason as the first of your examples above.

It is a tangent, but I suspect this failure may be a bit unexpected.
In this example, "-f"orce could be overriding the usual failure from
"-b" to switch to a branch that already exists, but that is what
"-B" does, and "-f -b" does not work as a synonym for "-B".

In any case, these example you marked "fail as expected" do fail as
expected, but they fail for reasons that have nothing to do with the
protection of branches that are used in other worktrees.

>     git checkout -B main origin/main # ----> this succeeds, which is
> unexpected <----

I agree this may be undesirable.

But it makes sort of sense, because "-B" is a forced form of "-b"
(i.e., it tells git: even when "-b" would fail, take necessary
measures to make it work), and we can view that it is part of
"forcing" to override the protection over branches that are used
elsewhere.

I guess we could change the behaviour so that

    git checkout -B <branch> [<start-point>]

fails when <branch> is an existing branch that is in use in another
worktree, and allow "-f" to be used to override the safety, i.e.,

    git checkout -f -B <branch> [<start-point>]

would allow the <branch> to be repointed to <start-point> (or HEAD)
even when it is used elsewhere.

Thoughts, whether they agree or disagree with what I just said, by
other experienced contributors are very much welcome, before I can
say "patches welcome" ;-).

Willem, thanks for raising the issue.

Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I guess we could change the behaviour so that
>
>     git checkout -B <branch> [<start-point>]
>
> fails when <branch> is an existing branch that is in use in another
> worktree, and allow "-f" to be used to override the safety, i.e.,
>
>     git checkout -f -B <branch> [<start-point>]
>
> would allow the <branch> to be repointed to <start-point> (or HEAD)
> even when it is used elsewhere.

It turns out that for some reason "-f" is not how we decided to
override this one---there is "--ignore-other-worktrees" option.

I'll attach the first step (preparatory refactoring) to this message
below, and follow it up with the second step to implement and test
the change.

--- >8 ---
From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 23 Nov 2023 14:11:41 +0900
Subject: [PATCH 1/2] checkout: refactor die_if_checked_out() caller

There is a bit dense logic to make a call to "die_if_checked_out()"
while trying to check out a branch.  Extract it into a helper
function and give it a bit of comment to describe what is going on.

The most important part of the refactoring is the separation of the
guarding logic before making the call to die_if_checked_out() into
the caller specific part (e.g., the logic that decides that the
caller is trying to check out an existing branch) and the bypass due
to the "--ignore-other-worktrees" option.  The latter will be common
no matter how the current or future callers decides they need this
protection.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc15..b4ab972c5a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1516,6 +1516,26 @@ static void die_if_some_operation_in_progress(void)
 	wt_status_state_free_buffers(&state);
 }
 
+/*
+ * die if attempting to checkout an existing branch that is in use
+ * in another worktree, unless ignore-other-wortrees option is given.
+ * The check is bypassed when the branch is already the current one,
+ * as it will not make things any worse.
+ */
+static void die_if_switching_to_a_branch_in_use(struct checkout_opts *opts,
+						const char *full_ref)
+{
+	int flags;
+	char *head_ref;
+
+	if (opts->ignore_other_worktrees)
+		return;
+	head_ref = resolve_refdup("HEAD", 0, NULL, &flags);
+	if (head_ref && (!(flags & REF_ISSYMREF) || strcmp(head_ref, full_ref)))
+		die_if_checked_out(full_ref, 1);
+	free(head_ref);
+}
+
 static int checkout_branch(struct checkout_opts *opts,
 			   struct branch_info *new_branch_info)
 {
@@ -1576,15 +1596,9 @@ static int checkout_branch(struct checkout_opts *opts,
 	if (!opts->can_switch_when_in_progress)
 		die_if_some_operation_in_progress();
 
-	if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
-	    !opts->ignore_other_worktrees) {
-		int flag;
-		char *head_ref = resolve_refdup("HEAD", 0, NULL, &flag);
-		if (head_ref &&
-		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))
-			die_if_checked_out(new_branch_info->path, 1);
-		free(head_ref);
-	}
+	/* "git checkout <branch>" */
+	if (new_branch_info->path && !opts->force_detach && !opts->new_branch)
+		die_if_switching_to_a_branch_in_use(opts, new_branch_info->path);
 
 	if (!new_branch_info->commit && opts->new_branch) {
 		struct object_id rev;
-- 
2.43.0

[PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Junio C Hamano]

"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(+)

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) &&
-- 
2.43.0

Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox

[Willem Verstraeten]

> >     git checkout -f main origin/main #also reports a fatal error, as expected
>
> This is expected because origin/main is taken as pathspec, and it is
> a request to checkout the paths that match the pathspec out of the
> named tree-ish (i.e., "main"), even when these paths have local
> changes, but you do not have paths that match "origin/main".  The
> failure is not because "main" is checked out elsewhere.
>

My mistake: I meant to do `git branch -f main origin/main`, as
documented for `git checkout -B main origin/main`

So, for completeness' sake, this is my revised reproduction scenario
that actually demonstrates the problem I have:

    ~/temp> git clone https://github.com/servo/pathfinder.git primary
    Cloning into 'primary'...
    ....

    ~/temp> cd primary

    ~/temp/primary> git worktree add -b metal ../secondary origin/metal
    Preparing worktree (new branch 'metal')
    branch 'metal' set up to track 'origin/metal'.
    HEAD is now at 1cdcc209 wip

    ~/temp/primary> cd ..\secondary\

    ~/temp/secondary> git checkout main
    fatal: 'main' is already used by worktree at ../primary'

    ~/temp/secondary> git branch -f main origin/main
    fatal: cannot force update the branch 'main' used by worktree at
'../primary'

    ~/temp/secondary> git checkout -B main origin/main
    Switched to and reset branch 'main'
    branch 'main' set up to track 'origin/main'.
    Your branch is up to date with 'origin/main'.

I would expect that last `git checkout -B ...` to fail with a similar
error as the `git branch -f ...` command right before that, since the
documentation for `git checkout -B <branch> <start-point>` states that
it is the atomic equivalent of `git branch -f <branch> <start-point> ;
git checkout <branch>`


> I guess we could change the behaviour so that
>
>     git checkout -B <branch> [<start-point>]
>
> fails when <branch> is an existing branch that is in use in another
> worktree, and allow "-f" to be used to override the safety, i.e.,
>
>     git checkout -f -B <branch> [<start-point>]

I would be very much in favor of that, indeed.

However, as you noted in your follow-up mail, the
--ignore-other-worktrees option would be better suited than the -f
flag.

> It turns out that for some reason "-f" is not how we decided to
> override this one---there is "--ignore-other-worktrees" option.

This means it would look like this then, if you decide to tackle this?

    ~/temp/secondary> git checkout -B metal origin/metal
    fatal: cannot force update the branch 'main' used by worktree at
'../primary'

    ~/temp/secondary> git checkout --ignore-other-worktrees -B metal
origin/metal
    Switched to and reset branch 'metal'
    branch 'metal' set up to track 'origin/metal'.
    Your branch is up to date with 'origin/metal'.

My thoughts as an experienced user, though not an experienced
contributor, admittedly :)

Kind regards,
Willem Verstraeten

On Thu, 23 Nov 2023 at 02:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Willem Verstraeten <willem.verstraeten@gmail.com> writes:
>
> >     git checkout -b main #reports a fatal error, as expected
>
> This is expected because "main" already exists, not because "main"
> is checked out elsewhere.
>
> >     git checkout -f main origin/main #also reports a fatal error, as expected
>
> This is expected because origin/main is taken as pathspec, and it is
> a request to checkout the paths that match the pathspec out of the
> named tree-ish (i.e., "main"), even when these paths have local
> changes, but you do not have paths that match "origin/main".  The
> failure is not because "main" is checked out elsewhere.
>
> A slight variant of the command
>
>     git checkout -f -b main origin/main
>
> still fails for the same reason as the first of your examples above.
>
> It is a tangent, but I suspect this failure may be a bit unexpected.
> In this example, "-f"orce could be overriding the usual failure from
> "-b" to switch to a branch that already exists, but that is what
> "-B" does, and "-f -b" does not work as a synonym for "-B".
>
> In any case, these example you marked "fail as expected" do fail as
> expected, but they fail for reasons that have nothing to do with the
> protection of branches that are used in other worktrees.
>
> >     git checkout -B main origin/main # ----> this succeeds, which is
> > unexpected <----
>
> I agree this may be undesirable.
>
> But it makes sort of sense, because "-B" is a forced form of "-b"
> (i.e., it tells git: even when "-b" would fail, take necessary
> measures to make it work), and we can view that it is part of
> "forcing" to override the protection over branches that are used
> elsewhere.
>
> I guess we could change the behaviour so that
>
>     git checkout -B <branch> [<start-point>]
>
> fails when <branch> is an existing branch that is in use in another
> worktree, and allow "-f" to be used to override the safety, i.e.,
>
>     git checkout -f -B <branch> [<start-point>]
>
> would allow the <branch> to be repointed to <start-point> (or HEAD)
> even when it is used elsewhere.
>
> Thoughts, whether they agree or disagree with what I just said, by
> other experienced contributors are very much welcome, before I can
> say "patches welcome" ;-).
>
> Willem, thanks for raising the issue.
>
>
>

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Phillip Wood]

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) &&

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Eric Sunshine]

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.

Re: git checkout -B <branch> lets you checkout a branch that is already checked out in another worktree Inbox

[Andy Koppe]

On 23/11/2023 05:58, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> I guess we could change the behaviour so that
>>
>>      git checkout -B <branch> [<start-point>]
>>
>> fails when <branch> is an existing branch that is in use in another
>> worktree, and allow "-f" to be used to override the safety, i.e.,
>>
>>      git checkout -f -B <branch> [<start-point>]
>>
>> would allow the <branch> to be repointed to <start-point> (or HEAD)
>> even when it is used elsewhere.
> 
> It turns out that for some reason "-f" is not how we decided to
> override this one---there is "--ignore-other-worktrees" option.

Presumably that's because -f means throwing away any local changes that 
are in the way of checking out the new HEAD, which you wouldn't 
necessarily want when trying to replace an existing branch.

Andy

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Junio C Hamano]

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.

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Junio C Hamano]

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.

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Jeff King]

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

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Phillip Wood]

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.

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Willem Verstraeten]

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.

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Eric Sunshine]

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/

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Junio C Hamano]

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
 	)
 '
 

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Willem Verstraeten]

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
>         )
>  '
>

Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere

[Junio C Hamano]

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.