[PATCH] builtin/checkout: check the branch used in -B with worktrees

[PATCH] builtin/checkout: check the branch used in -B with worktrees

[Carlo Marcelo Arenas Belón]

When multiple worktrees are being used, checkout/switch check
that the target branch is not already checked out with code that
evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased
in another worktree, 2016-04-22), but that logic wasn't valid for
-B/-C

Avoid reusing the same `branch_info` structure for the checks and
assumming that it will be rejected later if is a new branch that
already exists as that doesn't apply to -B/-C.

Reported-by: Jinwook Jeong <vustthat@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/checkout.c      | 22 ++++++++++++++++------
 t/t2400-worktree-add.sh | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3fa29a08ee..94dcd617ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
 }
 
 static int checkout_branch(struct checkout_opts *opts,
-			   struct branch_info *new_branch_info)
+			   struct branch_info *new_branch_info,
+			   struct branch_info *check_branch_info)
 {
 	if (opts->pathspec.nr)
 		die(_("paths cannot be used with switching branches"));
@@ -1533,13 +1534,12 @@ 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) {
+	if (check_branch_info->path && !opts->force_detach && !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);
+		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
+			die_if_checked_out(check_branch_info->path, 1);
 		free(head_ref);
 	}
 
@@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			 struct branch_info *new_branch_info)
 {
 	int parseopt_flags = 0;
+	struct branch_info check_branch_info = { 0 };
 
 	opts->overwrite_ignore = 1;
 	opts->prefix = prefix;
@@ -1739,6 +1740,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			!opts->new_branch;
 		int n = parse_branchname_arg(argc, argv, dwim_ok,
 					     new_branch_info, opts, &rev);
+		check_branch_info = *new_branch_info;
 		argv += n;
 		argc -= n;
 	} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 						      opts, &rev,
 						      opts->from_treeish);
 
+		check_branch_info = *new_branch_info;
 		if (!opts->source_tree)
 			die(_("reference is not a tree: %s"), opts->from_treeish);
+	} else if (opts->new_branch) {
+		struct object_id rev;
+
+		if (!get_oid_mb(opts->new_branch, &rev))
+			setup_new_branch_info_and_source_tree(&check_branch_info,
+							opts, &rev,
+							opts->new_branch);
 	}
 
 	if (argc) {
@@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	if (opts->patch_mode || opts->pathspec.nr)
 		return checkout_paths(opts, new_branch_info);
 	else
-		return checkout_branch(opts, new_branch_info);
+		return checkout_branch(opts, new_branch_info, &check_branch_info);
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..283ba7607e 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '
 	)
 '
 
+test_expect_success 'die the same branch is already checked out (checkout -B)' '
+	(
+		cd here &&
+		test_must_fail git checkout -B newmain
+	)
+'
+
 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) &&
@@ -147,6 +154,13 @@ test_expect_success 'not die on re-checking out current branch' '
 	)
 '
 
+test_expect_success 'not die on re-checking out current branch (checkout -B)' '
+	(
+		cd there &&
+		git checkout -B newmain
+	)
+'
+
 test_expect_success '"add" from a bare repo' '
 	(
 		git clone --bare . bare &&
-- 
2.39.0.1.g119e9c6876.dirty

Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees

[Eric Sunshine]

On Mon, Jan 16, 2023 at 12:30 PM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> builtin/checkout: check the branch used in -B with worktrees

Thanks for working on this and coming up with a fix. As mentioned
earlier, I had started looking into it[1], but lacked the time to
disentangle the logic, so I'm glad to see a patch arrive so quickly.

> When multiple worktrees are being used, checkout/switch check
> that the target branch is not already checked out with code that
> evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased
> in another worktree, 2016-04-22), but that logic wasn't valid for
> -B/-C
>
> Avoid reusing the same `branch_info` structure for the checks and
> assumming that it will be rejected later if is a new branch that
> already exists as that doesn't apply to -B/-C.

Even though I'm familiar with the bug report[2] which sparked this
patch, I find the above description somewhat hard to digest; the
high-level problem it is addressing doesn't jump off the page at me.
Perhaps it can be rewritten something like this:

    checkout/switch: disallow checking out same branch in multiple worktrees

    Commands `git switch -C` and `git checkout -B` neglect to check
    whether the branch being forcibly created is already checked out
    in some other worktree, which can result in the undesirable
    situation of the same branch being checked out in multiple
    worktrees. For instance:

        $ 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.

after which you might include some details which you wrote about initially.

> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,
> -       if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&
> -           !opts->ignore_other_worktrees) {
> +       if (check_branch_info->path && !opts->force_detach && !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);
> +                   (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> +                       die_if_checked_out(check_branch_info->path, 1);

This variable name change (`new_branch_info` => `check_branch_info`)
helps make the code clearer. Good. (I had found it more than a little
confusing to have similar named variables `new_branch_info` and
`opts->new_branch` even though they are unrelated and have very
different purposes.)

> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '
> +test_expect_success 'die the same branch is already checked out (checkout -B)' '
> +       (
> +               cd here &&
> +               test_must_fail git checkout -B newmain
> +       )
> +'

Although `git switch` and `git checkout` currently share
implementation, that might not always be the case going forward. As
such, this test could be made a bit more robust by testing both
commands rather than just `git-checkout`. So, perhaps:

    test_expect_success 'die the same branch is already checked out' '
        (
            cd here &&
            test_must_fail git checkout -B newmain &&
            test_must_fail git switch -C newmain
        )
    '

> +test_expect_success 'not die on re-checking out current branch (checkout -B)' '
> +       (
> +               cd there &&
> +               git checkout -B newmain
> +       )
> +'

Good to see you considered this case too. (I had tested it myself
manually when trying out your patch.)

[1]: https://lore.kernel.org/git/CAPig+cQc1+D9gH7BAC-r03bGKWx3a9jpPyLuP-ehH-X2P+fV6Q@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAA3Q-aaO=vcZd9VLFr8UP-g06be80eUWN_GjygfyGkYmrLx9yQ@mail.gmail.com/

Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees

[Rubén Justo]

Hi Carlo.

Thank you for working on this.

On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:

> @@ -1533,13 +1534,12 @@ 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) {
> +	if (check_branch_info->path && !opts->force_detach && !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);
> +		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> +			die_if_checked_out(check_branch_info->path, 1);


You adjusted this block to accommodate the problem with "checkout -B",
which is sensible, but we may need to do something different here.

What we are doing here, if I understand it correctly, is dying if the
branch is _not checked out in the current worktree_ and _checked out in
any other worktree_.  Which is OK for normal checkout/switch.

But IMHO with "checkout -B" we have to die if the branch is checked out
in any other worktree, regardless of whether or not it is checked out in
the current working tree.

Perhaps the scenario where the user has the same branch checked out in
multiple worktrees and tries to reset in one of them, is one of those
where we could use the "if it hurts, don't do it". But we are providing
the "--ignore-other-worktrees" modifier, so I think we should disallow
the "-B" if the branch is checked out in any other worktree, and let
the user use "--ignore-other-worktrees" if he wants to go wild.

But.. die_if_checked_out() does not correctly honor the
"ignore_current_worktree" in this situation.  I have submitted a patch
to fix this, in case you want to consider all of this.

Un saludo.

Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees

[Carlo Arenas]

On Mon, Jan 16, 2023 at 4:53 PM Rubén Justo <rjusto@gmail.com> wrote:
> On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:
>
> > @@ -1533,13 +1534,12 @@ 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) {
> > +     if (check_branch_info->path && !opts->force_detach && !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);
> > +                 (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))
> > +                     die_if_checked_out(check_branch_info->path, 1);
>
> What we are doing here, if I understand it correctly, is dying if the
> branch is _not checked out in the current worktree_ and _checked out in
> any other worktree_.  Which is OK for normal checkout/switch.

I think the exception was added to `checkout` only, where it is
definitely needed, but switch probably should be more strict as it is
not "plumbing" and as you pointed out there is already a UI option to
override its safety.

> But IMHO with "checkout -B" we have to die if the branch is checked out
> in any other worktree, regardless of whether or not it is checked out in
> the current working tree.

I have to admit, I thought about that too, but then avoided making a
change as checkout behaviour affects a lot of other places, but in
retrospect I think that in this case it might be worth the change of
behaviour, since it is connected with the bugfix.

Before, the operation was allowed and the logic never tried to prevent
it (which is why in my first look, I thought it might have been
intentional), but once Eric pointed out it was a bug, then the obvious
conclusion would be to prevent it with the extended logic, as you
pointed out.

> Perhaps the scenario where the user has the same branch checked out in
> multiple worktrees and tries to reset in one of them, is one of those
> where we could use the "if it hurts, don't do it". But we are providing
> the "--ignore-other-worktrees" modifier, so I think we should disallow
> the "-B" if the branch is checked out in any other worktree, and let
> the user use "--ignore-other-worktrees" if he wants to go wild.

v2 includes this and AFAIK nothing is broken yet.

Carlo

PS. As explained before, tightening `switch` might be a good idea, but
it is obviously punted for this change and will be addressed later.

[PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Marcelo Arenas Belón]

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 example:

  $ 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.

As reflected on the tests, this will change the behaviour of those
commands when they are invoked in a worktree that has that requested
branch checked out, as that matches the logic used by branch, is safer
(assuming both commands are user facing) and can be overriden with an
existing flag.

Reported-by: Jinwook Jeong <vustthat@gmail.com>
Helped-by: Rubén Justo <rjusto@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Changes since v1
* A much better commit message
* Changes to the tests as suggested by Eric
* Changes to the logic as suggested by Rubén

 builtin/checkout.c      | 24 +++++++++++++++++-------
 t/t2400-worktree-add.sh | 18 ++++++++++++++++--
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3fa29a08ee..58a956392b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
 }
 
 static int checkout_branch(struct checkout_opts *opts,
-			   struct branch_info *new_branch_info)
+			   struct branch_info *new_branch_info,
+			   struct branch_info *check_branch_info)
 {
 	if (opts->pathspec.nr)
 		die(_("paths cannot be used with switching branches"));
@@ -1533,13 +1534,12 @@ 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) {
+	if (check_branch_info->path && !opts->force_detach && !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);
+		if (opts->new_branch_force || (head_ref &&
+		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path))))
+			die_if_checked_out(check_branch_info->path, 1);
 		free(head_ref);
 	}
 
@@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			 struct branch_info *new_branch_info)
 {
 	int parseopt_flags = 0;
+	struct branch_info check_branch_info = { 0 };
 
 	opts->overwrite_ignore = 1;
 	opts->prefix = prefix;
@@ -1739,6 +1740,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			!opts->new_branch;
 		int n = parse_branchname_arg(argc, argv, dwim_ok,
 					     new_branch_info, opts, &rev);
+		check_branch_info = *new_branch_info;
 		argv += n;
 		argc -= n;
 	} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 						      opts, &rev,
 						      opts->from_treeish);
 
+		check_branch_info = *new_branch_info;
 		if (!opts->source_tree)
 			die(_("reference is not a tree: %s"), opts->from_treeish);
+	} else if (opts->new_branch) {
+		struct object_id rev;
+
+		if (!get_oid_mb(opts->new_branch, &rev))
+			setup_new_branch_info_and_source_tree(&check_branch_info,
+							opts, &rev,
+							opts->new_branch);
 	}
 
 	if (argc) {
@@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	if (opts->patch_mode || opts->pathspec.nr)
 		return checkout_paths(opts, new_branch_info);
 	else
-		return checkout_branch(opts, new_branch_info);
+		return checkout_branch(opts, new_branch_info, &check_branch_info);
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..a66f9bb30c 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -121,7 +121,10 @@ test_expect_success '"add" worktree creating new branch' '
 test_expect_success 'die the same branch is already checked out' '
 	(
 		cd here &&
-		test_must_fail git checkout newmain
+		test_must_fail git checkout newmain &&
+		test_must_fail git checkout -B newmain &&
+		test_must_fail git switch newmain &&
+		test_must_fail git switch -C newmain
 	)
 '
 
@@ -143,7 +146,18 @@ test_expect_success 'not die the same branch is already checked out' '
 test_expect_success 'not die on re-checking out current branch' '
 	(
 		cd there &&
-		git checkout newmain
+		git checkout newmain &&
+		git switch newmain
+	)
+'
+
+test_expect_success 'but die if using force without --ignore-other-worktrees' '
+	(
+		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 newmain
 	)
 '
 
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> As reflected on the tests, this will change the behaviour of those
> commands when they are invoked in a worktree that has that requested
> branch checked out, as that matches the logic used by branch, is safer
> (assuming both commands are user facing) and can be overriden with an
> existing flag.

... meaning you can "--force", or something else?  Allowing an
existing option to be used as the safety valve does make sense,
especially if the option is something users are already familiar
with (like "--force") and naturally expected to work.

There might need an documentation update.  Back when "checkout -b"
and "branch" was written, there wasn't "multiple worktrees connected
to a single repository" hence there was no need to provide safety
against checking out the same branch in two different places.  "git
branch" might have learned th give that safety while "git checkout
-b", which _ought_ to be equivalent to "git branch" followed by "git
checkout", might have forgot to do so.  After this change, it may
still be correct to say that "checkout -b" is equivalent to "branch"
followed by "checkout", but if the documentation to "branch" talks
about this safety, it probably deserves to be mentioned in the
documentation to "checkout -b", as well, if only to give an appropriate
place to talk about how to override it "with an existing flag".

Thanks.

Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Arenas]

On Tue, Jan 17, 2023 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
>
> > As reflected on the tests, this will change the behaviour of those
> > commands when they are invoked in a worktree that has that requested
> > branch checked out, as that matches the logic used by branch, is safer
> > (assuming both commands are user facing) and can be overriden with an
> > existing flag.
>
> ... meaning you can "--force", or something else?  Allowing an
> existing option to be used as the safety valve does make sense,
> especially if the option is something users are already familiar
> with (like "--force") and naturally expected to work.

the following is the way to override:

$ git checkout --ignore-other-worktrees -B foo

> There might need an documentation update.  Back when "checkout -b"
> and "branch" was written, there wasn't "multiple worktrees connected
> to a single repository" hence there was no need to provide safety
> against checking out the same branch in two different places.  "git
> branch" might have learned to give that safety while "git checkout
> -b", which _ought_ to be equivalent to "git branch" followed by "git
> checkout", might have forgot to do so.

Not sure if it was originally forgotten, but it is definitely working now;
this change only fixes the uppercase (-B) version.

> After this change, it may
> still be correct to say that "checkout -b" is equivalent to "branch"
> followed by "checkout", but if the documentation to "branch" talks
> about this safety, it probably deserves to be mentioned in the
> documentation to "checkout -b", as well, if only to give an appropriate
> place to talk about how to override it "with an existing flag".

Interestingly, when the flag was added in 1d0fa898ea (checkout: add
--ignore-other-wortrees, 2015-01-03), it was only added to `checkout`.

`git branch` has no flag and will die even when `-f` is used

Carlo

Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> the following is the way to override:
>
> $ git checkout --ignore-other-worktrees -B foo

My points were that (1) the option is way too unintuitive to
anybody, other than those who stared at the implementation of the
problematic logic for too long, and that (2) it wasn't mentioned in
the proposed log message or documentation updates.  If "--force" is
made to mean that, it might be easier to discover to the users, but
I have no strong opinion on that (meaning: there may be downsides to
allow use of "--force" to override this safety, given that "-B" is
already considered as a "forcing" version of "-b").

> `git branch` has no flag and will die even when `-f` is used

If "--force" does not force it, I suspect it should be considered a
bug.

Thanks.

Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> Changes since v1
> * A much better commit message
> * Changes to the tests as suggested by Eric
> * Changes to the logic as suggested by Rubén

I queued this topic at the tip of 'seen' as 2fe0b4e3 (Merge branch
'cb/checkout-same-branch-twice' into seen, 2023-01-18), on top of
4ea8693b (Merge branch 'mc/credential-helper-auth-headers' into
seen, 2023-01-18).

 - 4ea8693b - https://github.com/git/git/actions/runs/3952916442
 - 2fe0b4e3 - https://github.com/git/git/actions/runs/3953521066

Comparing these two runs, inclusion of this topic seems to introduce
new leaks, as t1408 and t2018 (neither of which was touched by this
topic) that used to pass are now failing.

>  builtin/checkout.c      | 24 +++++++++++++++++-------
>  t/t2400-worktree-add.sh | 18 ++++++++++++++++--
>  2 files changed, 33 insertions(+), 9 deletions(-)

Thanks.  

[PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Marcelo Arenas Belón]

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.

Reported-by: Jinwook Jeong <vustthat@gmail.com>
Helped-by: Rubén Justo <rjusto@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Changes since v2
* A leak free implementation
* More details in commit as suggested by Junio

Changes since v1
* A much better commit message
* Changes to the tests as suggested by Eric
* Changes to the logic as suggested by Rubén

 builtin/checkout.c      | 31 +++++++++++++++++++++++--------
 t/t2400-worktree-add.sh | 18 ++++++++++++++++--
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5963e1b74b..467e194701 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1476,7 +1476,8 @@ static void die_if_some_operation_in_progress(void)
 }
 
 static int checkout_branch(struct checkout_opts *opts,
-			   struct branch_info *new_branch_info)
+			   struct branch_info *new_branch_info,
+			   char *check_branch_path)
 {
 	if (opts->pathspec.nr)
 		die(_("paths cannot be used with switching branches"));
@@ -1535,15 +1536,15 @@ 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) {
+	if (check_branch_path && !opts->force_detach && !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);
+		if (opts->new_branch_force || (head_ref &&
+		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))
+			die_if_checked_out(check_branch_path, 1);
 		free(head_ref);
 	}
+	free(check_branch_path);
 
 	if (!new_branch_info->commit && opts->new_branch) {
 		struct object_id rev;
@@ -1630,6 +1631,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			 struct branch_info *new_branch_info)
 {
 	int parseopt_flags = 0;
+	char *check_branch_path = NULL;
 
 	opts->overwrite_ignore = 1;
 	opts->prefix = prefix;
@@ -1741,6 +1743,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			!opts->new_branch;
 		int n = parse_branchname_arg(argc, argv, dwim_ok,
 					     new_branch_info, opts, &rev);
+		check_branch_path = xstrdup_or_null(new_branch_info->path);
 		argv += n;
 		argc -= n;
 	} else if (!opts->accept_ref && opts->from_treeish) {
@@ -1753,8 +1756,18 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 						      opts, &rev,
 						      opts->from_treeish);
 
+		check_branch_path = xstrdup_or_null(new_branch_info->path);
 		if (!opts->source_tree)
 			die(_("reference is not a tree: %s"), opts->from_treeish);
+	} else if (opts->new_branch && !opts->ignore_other_worktrees) {
+		struct object_id rev;
+
+		if (!get_oid_mb(opts->new_branch, &rev)) {
+			struct strbuf buf = STRBUF_INIT;
+			strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);
+			strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
+			check_branch_path = strbuf_detach(&buf, NULL);
+		}
 	}
 
 	if (argc) {
@@ -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);
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..a66f9bb30c 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -121,7 +121,10 @@ test_expect_success '"add" worktree creating new branch' '
 test_expect_success 'die the same branch is already checked out' '
 	(
 		cd here &&
-		test_must_fail git checkout newmain
+		test_must_fail git checkout newmain &&
+		test_must_fail git checkout -B newmain &&
+		test_must_fail git switch newmain &&
+		test_must_fail git switch -C newmain
 	)
 '
 
@@ -143,7 +146,18 @@ test_expect_success 'not die the same branch is already checked out' '
 test_expect_success 'not die on re-checking out current branch' '
 	(
 		cd there &&
-		git checkout newmain
+		git checkout newmain &&
+		git switch newmain
+	)
+'
+
+test_expect_success 'but die if using force without --ignore-other-worktrees' '
+	(
+		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 newmain
 	)
 '
 
-- 
2.37.1 (Apple Git-137.1)

Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> Changes since v2
> * A leak free implementation

> * More details in commit as suggested by Junio

I meant to say we may need more details in the documentation, but
after reading the existing documentation, we say that

 - "-B <name>" is equivalent to run "branch -f <name>", which is
   sufficient to hint that it will fail to recreate and check out an
   existing branch that is checked out elsewhere, because "branch
   -f" would fail in such a situation.

and that

 - "--ignore-other-worktrees" defeats the safety that makes "git
   checkout refuses when the wanted ref is already checked out".

so the existing documentation of "git checkout" may already be OK.

As long as it is well known that "git branch -f" still fails in the
situation, that is.  After re-reading "git branch --help" twice,
however, I am not sure if it is so clear, though.

How about adding something like this, as an independent
documentation improvement?

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] branch: document `-f` and linked worktree behaviour

"git branch -f name start" forces to recreate the named branch, but
the forcing does not defeat the "do not touch a branch that is
checked out elsewhere" safety valve.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-branch.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git c/Documentation/git-branch.txt w/Documentation/git-branch.txt
index aa2f78c4c2..b12e7940d3 100644
--- c/Documentation/git-branch.txt
+++ w/Documentation/git-branch.txt
@@ -123,6 +123,10 @@ OPTIONS
 	points to a valid commit. In combination with
 	`-m` (or `--move`), allow renaming the branch even if the new
 	branch name already exists, the same applies for `-c` (or `--copy`).
++
+Note that 'git branch -f <branchname> [<start-point>]' refuses to change
+an existing branch `<branchname>` that is checked out in another worktree
+linked to the same repository.
 
 -m::
 --move::

Re: Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Arenas]

Thanks, that should round up the documentation for this behaviour nicely.

Carlo

Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Phillip Wood]

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.

> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> @@ -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

Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Junio C Hamano]

Merging this on top of 'seen' that used to pass the CI tests

    https://github.com/git/git/actions/runs/3963948890

now fails many tests

    https://github.com/git/git/actions/runs/3964312300

Funny thing is that nothing fails locally for me on x86_64 Linux
with gcc-12.  Anything rings a bell?

Thanks.

Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Arenas]

I think it only fails its own test, and I can't reproduce locally
either with macOS and clang.

I was going to propose a newer version but it seems to be similarly affected:

  https://github.com/carenas/git/actions/runs/3964409976

It will be better to drop it for now; will produce one without the
heisenbug, somehow

Carlo

Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Arenas]

I should have known; the test is affected by the bug Rubén is trying to fix in:

  https://lore.kernel.org/git/eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com/

Where the order that the worktrees are found is not deterministic and
so the ignore_current_worktree flag to die_if_checked_out() might
trigger incorrectly.

Carlo

[PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Marcelo Arenas Belón]

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.

Unlike what it is done for `git switch` and `git checkout`, that have
an historical exception to ignore other worktrees 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.

Reported-by: Jinwook Jeong <vustthat@gmail.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Rubén Justo <rjusto@gmail.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Changes since v3
* Code and Tests improvements as suggested by Phillip
* Disable unreliable test that triggers a known bug

Changes since v2
* A leak free implementation
* More details in commit as suggested by Junio

Changes since v1
* A much better commit message
* Changes to the tests as suggested by Eric
* Changes to the logic as suggested by Rubén


 builtin/checkout.c      | 32 ++++++++++++++++++++++++--------
 t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------
 2 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3fa29a08ee..0688652f99 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
 }
 
 static int checkout_branch(struct checkout_opts *opts,
-			   struct branch_info *new_branch_info)
+			   struct branch_info *new_branch_info,
+			   char *check_branch_path)
 {
 	if (opts->pathspec.nr)
 		die(_("paths cannot be used with switching branches"));
@@ -1533,13 +1534,13 @@ 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) {
+	if (!opts->ignore_other_worktrees && !opts->force_detach &&
+	    check_branch_path && ref_exists(check_branch_path)) {
 		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);
+		if (opts->new_branch_force || (head_ref &&
+		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))
+			die_if_checked_out(check_branch_path, 1);
 		free(head_ref);
 	}
 
@@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			 const char * const usagestr[],
 			 struct branch_info *new_branch_info)
 {
+	int ret;
 	int parseopt_flags = 0;
+	char *check_branch_path = NULL;
 
 	opts->overwrite_ignore = 1;
 	opts->prefix = prefix;
@@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 		opts->new_branch = argv0 + 1;
 	}
 
+	if (opts->new_branch && !opts->ignore_other_worktrees) {
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);
+		strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
+		check_branch_path = strbuf_detach(&buf, NULL);
+	}
 	/*
 	 * Extract branch name from command line arguments, so
 	 * all that is left is pathspecs.
@@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 					     new_branch_info, opts, &rev);
 		argv += n;
 		argc -= n;
+
+		if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
+			check_branch_path = xstrdup(new_branch_info->path);
 	} else if (!opts->accept_ref && opts->from_treeish) {
 		struct object_id rev;
 
@@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 	}
 
 	if (opts->patch_mode || opts->pathspec.nr)
-		return checkout_paths(opts, new_branch_info);
+		ret = checkout_paths(opts, new_branch_info);
 	else
-		return checkout_branch(opts, new_branch_info);
+		ret = checkout_branch(opts, new_branch_info, check_branch_path);
+
+	free(check_branch_path);
+	return ret;
 }
 
 int cmd_checkout(int argc, const char **argv, const char *prefix)
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index d587e0b20d..7ab7e87440 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' '
 	)
 '
 
-test_expect_success 'die the same branch is already checked out' '
+test_expect_success 'die if the same branch is already checked out' '
 	(
 		cd here &&
-		test_must_fail git checkout newmain
+		test_must_fail git checkout newmain &&
+		test_must_fail git checkout -B newmain &&
+		test_must_fail git switch newmain &&
+		test_must_fail git switch -C newmain
 	)
 '
 
-test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
+test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '
 	head=$(git -C there rev-parse --git-path HEAD) &&
 	ref=$(git -C there symbolic-ref HEAD) &&
 	rm "$head" &&
@@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
 	test_must_fail git -C here checkout newmain
 '
 
-test_expect_success 'not die the same branch is already checked out' '
+test_expect_success 'allow creating multiple worktrees for same branch with force' '
+	git worktree add --force anothernewmain newmain
+'
+
+test_expect_success 'allow checkout/reset from the conflicted branch' '
 	(
 		cd here &&
-		git worktree add --force anothernewmain newmain
+		git checkout -b conflictedmain newmain &&
+		git checkout -B conflictedmain newmain &&
+		git switch -C conflictedmain newmain
+	)
+'
+
+test_expect_success 'and not die on re-checking out current branch even if conflicted' '
+	(
+		cd there &&
+		git checkout newmain &&
+		git switch newmain
 	)
 '
 
-test_expect_success 'not die on re-checking out current branch' '
+test_expect_failure 'unless using force without --ignore-other-worktrees' '
 	(
 		cd there &&
-		git checkout newmain
+		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 newmain
 	)
 '
 
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Phillip Wood]

Hi Carlo

On 20/01/2023 11:35, 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.
> 
> Unlike what it is done for `git switch` and `git checkout`, that have
> an historical exception to ignore other worktrees 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.

As I said before, it would be much easier for everyone else to 
understand the changes if you wrote out what they were rather than 
saying "look at the tests". I do appreciate the improved test 
descriptions though - thanks for that.

> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> Changes since v3
> * Code and Tests improvements as suggested by Phillip
> * Disable unreliable test that triggers a known bug
> 
> Changes since v2
> * A leak free implementation
> * More details in commit as suggested by Junio
> 
> Changes since v1
> * A much better commit message
> * Changes to the tests as suggested by Eric
> * Changes to the logic as suggested by Rubén
> 
> 
>   builtin/checkout.c      | 32 ++++++++++++++++++++++++--------
>   t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------
>   2 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3fa29a08ee..0688652f99 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
>   }
>   
>   static int checkout_branch(struct checkout_opts *opts,
> -			   struct branch_info *new_branch_info)
> +			   struct branch_info *new_branch_info,
> +			   char *check_branch_path)
>   {
>   	if (opts->pathspec.nr)
>   		die(_("paths cannot be used with switching branches"));
> @@ -1533,13 +1534,13 @@ 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) {
> +	if (!opts->ignore_other_worktrees && !opts->force_detach &&
> +	    check_branch_path && ref_exists(check_branch_path)) {

I think check_branch_path is NULL if opts->ignore_other_worktrees is set 
so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly 
below where you set check_branch_path).

>   		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);
> +		if (opts->new_branch_force || (head_ref &&
> +		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))
> +			die_if_checked_out(check_branch_path, 1);

I don't think it is worth a re-roll but the reformatting here is 
unfortunate. If you add the new condition at the end it is clearer what 
is being changed.

		if ((head_ref &&
		    (!(flag & REF_IS_YMREF) || strcmp(head_ref, check_branch_path))) ||
		    opts->new_branch_force)

preserves the original code structure so one can see we've added a new 
condition and done s/new_branch_info->path/check_branch_path/

>   		free(head_ref);
>   	}
>   
> @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>   			 const char * const usagestr[],
>   			 struct branch_info *new_branch_info)
>   {
> +	int ret;
>   	int parseopt_flags = 0;
> +	char *check_branch_path = NULL;
>   
>   	opts->overwrite_ignore = 1;
>   	opts->prefix = prefix;
> @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>   		opts->new_branch = argv0 + 1;
>   	}
>   
> +	if (opts->new_branch && !opts->ignore_other_worktrees) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);
> +		strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
> +		check_branch_path = strbuf_detach(&buf, NULL);
> +	}

This block will run whenever -b/-B is given which is good

>   	/*
>   	 * Extract branch name from command line arguments, so
>   	 * all that is left is pathspecs.
> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>   					     new_branch_info, opts, &rev);
>   		argv += n;
>   		argc -= n;
> +
> +		if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
> +			check_branch_path = xstrdup(new_branch_info->path);

I'm a bit confused what this is doing.

>   	} else if (!opts->accept_ref && opts->from_treeish) {
>   		struct object_id rev;
>   
> @@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>   	}
>   
>   	if (opts->patch_mode || opts->pathspec.nr)
> -		return checkout_paths(opts, new_branch_info);
> +		ret = checkout_paths(opts, new_branch_info);
>   	else
> -		return checkout_branch(opts, new_branch_info);
> +		ret = checkout_branch(opts, new_branch_info, check_branch_path);
> +
> +	free(check_branch_path);
> +	return ret;

This is clearer now - thanks

>   }
>   
>   int cmd_checkout(int argc, const char **argv, const char *prefix)
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index d587e0b20d..7ab7e87440 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' '
>   	)
>   '
>   
> -test_expect_success 'die the same branch is already checked out' '
> +test_expect_success 'die if the same branch is already checked out' '
>   	(
>   		cd here &&
> -		test_must_fail git checkout newmain
> +		test_must_fail git checkout newmain &&
> +		test_must_fail git checkout -B newmain &&
> +		test_must_fail git switch newmain &&
> +		test_must_fail git switch -C newmain
>   	)
>   '
>   
> -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
> +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '
>   	head=$(git -C there rev-parse --git-path HEAD) &&
>   	ref=$(git -C there symbolic-ref HEAD) &&
>   	rm "$head" &&
> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
>   	test_must_fail git -C here checkout newmain
>   '
>   
> -test_expect_success 'not die the same branch is already checked out' '
> +test_expect_success 'allow creating multiple worktrees for same branch with force' '
> +	git worktree add --force anothernewmain newmain
> +'
> +
> +test_expect_success 'allow checkout/reset from the conflicted branch' '

I'm not sure what "the conflicted branch" means (it reminds we of merge 
conflicts). Is this just testing that "checkout -b/B <branch> 
<start-point>" works?

>   	(
>   		cd here &&
> -		git worktree add --force anothernewmain newmain
> +		git checkout -b conflictedmain newmain &&
> +		git checkout -B conflictedmain newmain &&
> +		git switch -C conflictedmain newmain
> +	)
> +'
> +
> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '

I think 'allow re-checking out ...' would be clearer, again I'm not sure 
what's conflicted here.

> +	(
> +		cd there &&
> +		git checkout newmain &&
> +		git switch newmain
>   	)
>   '
>   
> -test_expect_success 'not die on re-checking out current branch' '
> +test_expect_failure 'unless using force without --ignore-other-worktrees' '

This test passes for me - what's the reason for changing from 
test_expect_success to test_expect_failure?

Thanks for working on this

Phillip

>   	(
>   		cd there &&
> -		git checkout newmain
> +		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 newmain
>   	)
>   '
>   

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Arenas]

On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 20/01/2023 11:35, 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.
> >
> > Unlike what it is done for `git switch` and `git checkout`, that have
> > an historical exception to ignore other worktrees 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.
>
> As I said before, it would be much easier for everyone else to
> understand the changes if you wrote out what they were rather than
> saying "look at the tests". I do appreciate the improved test
> descriptions though - thanks for that.

Apologies on that, I tried to come up with something that would
describe the change of behaviour other than the paragraph above and
couldn't come out with a better explanation except reading the tests
(which I know is complicated by the fact they are interlinked).

The behaviour I am changing is also not documented (other than by the
test) and might have been added as a quirk to keep the scripted rebase
and bisect going when confronted with branches that were checked out
in multiple worktrees, and as such might had not be intended for
`switch`, and might not be needed anymore either.

Using`checkout` for simplicity, but also applies to `switch`,

  % git worktree list
  .../base  6a45aba [main]
  % git worktree add -f ../other main
  Preparing worktree (checking out 'main')
  HEAD is now at 6a45aba init
  % cd ../other
  % git checkout main
  Already on 'main'
  % git checkout -B main
  fatal: 'main' is already checked out at '.../base'
  % git checkout --ignore-other-worktrees -B main
  Already on 'main'

The change of behaviour only applies to -B and it actually matches the
documentation better.

> > @@ -1533,13 +1534,13 @@ 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) {
> > +     if (!opts->ignore_other_worktrees && !opts->force_detach &&
> > +         check_branch_path && ref_exists(check_branch_path)) {
>
> I think check_branch_path is NULL if opts->ignore_other_worktrees is set
> so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly
> below where you set check_branch_path).

opts->ignore_other_worktrees was kept from the original expression;
you are correct that is not needed anymore, but I thought it didn't
hurt and made the code intention clearer (meaning it is obvious to
anyone new to the code that this code will be skipped if that flag is
set), would using an assert or a comment be a better option?

> >       /*
> >        * Extract branch name from command line arguments, so
> >        * all that is left is pathspecs.
> > @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
> >                                            new_branch_info, opts, &rev);
> >               argv += n;
> >               argc -= n;
> > +
> > +             if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
> > +                     check_branch_path = xstrdup(new_branch_info->path);
>
> I'm a bit confused what this is doing.

The branch we are interested in might come from 2 places, either it is
a parameter to -b, which was picked up before, or it is the argument
to the command itself, which was detected above.

If both are provided, we want to make sure to use the one from -b, or
will have the bug you sharply spotted before, which was frankly
embarrassing.

> > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> > index d587e0b20d..7ab7e87440 100755
> > @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
> >       test_must_fail git -C here checkout newmain
> >   '
> >
> > -test_expect_success 'not die the same branch is already checked out' '
> > +test_expect_success 'allow creating multiple worktrees for same branch with force' '
> > +     git worktree add --force anothernewmain newmain
> > +'
> > +
> > +test_expect_success 'allow checkout/reset from the conflicted branch' '
>
> I'm not sure what "the conflicted branch" means (it reminds we of merge
> conflicts).

by "conflicted" I meant one that is checked out in more than one worktree

> Is this just testing that "checkout -b/B <branch>
> <start-point>" works?

yes, but most importantly that we chose the right branch to check if
both are provided and <start-point> is also a branch

> >       (
> >               cd here &&
> > -             git worktree add --force anothernewmain newmain
> > +             git checkout -b conflictedmain newmain &&
> > +             git checkout -B conflictedmain newmain &&
> > +             git switch -C conflictedmain newmain
> > +     )
> > +'
> > +
> > +test_expect_success 'and not die on re-checking out current branch even if conflicted' '
>
> I think 'allow re-checking out ...' would be clearer, again I'm not sure
> what's conflicted here.

ok

> > +     (
> > +             cd there &&
> > +             git checkout newmain &&
> > +             git switch newmain
> >       )
> >   '
> >
> > -test_expect_success 'not die on re-checking out current branch' '
> > +test_expect_failure 'unless using force without --ignore-other-worktrees' '
>
> This test passes for me - what's the reason for changing from
> test_expect_success to test_expect_failure?

It also works for me, and for Junio, but somehow it didn't in the last
runs from the CI and you could reproduce locally by going to the tree
created above in the example I provided and doing:

  % cd ../base
  % git checkout -B main

which should fail after finding that main is already checked out in
`other`, but does not because when looking at the worktrees would
first find the current one and not die, never aware there is another
worktree with that same branch.

the bug is the same one that Rubén is trying to address for rebase and
that you commented on as well and that was mentioned before in this
thread:

  https://lore.kernel.org/git/eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com/

Carlo

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Phillip Wood]

Hi Carlo

On 20/01/2023 22:12, Carlo Arenas wrote:
> On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 20/01/2023 11:35, 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.
>>>
>>> Unlike what it is done for `git switch` and `git checkout`, that have
>>> an historical exception to ignore other worktrees 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.
>>
>> As I said before, it would be much easier for everyone else to
>> understand the changes if you wrote out what they were rather than
>> saying "look at the tests". I do appreciate the improved test
>> descriptions though - thanks for that.
> 
> Apologies on that, I tried to come up with something that would
> describe the change of behaviour other than the paragraph above and
> couldn't come out with a better explanation except reading the tests
> (which I know is complicated by the fact they are interlinked).
> 
> The behaviour I am changing is also not documented (other than by the
> test) and might have been added as a quirk to keep the scripted rebase
> and bisect going when confronted with branches that were checked out
> in multiple worktrees, and as such might had not be intended for
> `switch`, and might not be needed anymore either.
> 
> Using`checkout` for simplicity, but also applies to `switch`,
> 
>    % git worktree list
>    .../base  6a45aba [main]
>    % git worktree add -f ../other main
>    Preparing worktree (checking out 'main')
>    HEAD is now at 6a45aba init
>    % cd ../other
>    % git checkout main
>    Already on 'main'
>    % git checkout -B main
>    fatal: 'main' is already checked out at '.../base'

Thanks for explaining that. If there is no <start-point> given we don't 
reset the branch so it seems a bit harsh to error out here. For "git 
checkout -B <branch> <start-point>" when <branch> is checked out in 
another worktree requiring --ignore-other-worktrees makes sense.

>    % git checkout --ignore-other-worktrees -B main
>    Already on 'main'
> 
> The change of behaviour only applies to -B and it actually matches the
> documentation better.
> 
>>> @@ -1533,13 +1534,13 @@ 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) {
>>> +     if (!opts->ignore_other_worktrees && !opts->force_detach &&
>>> +         check_branch_path && ref_exists(check_branch_path)) {
>>
>> I think check_branch_path is NULL if opts->ignore_other_worktrees is set
>> so we could maybe lose "!opts->ignore_other_worktrees" here (or possibly
>> below where you set check_branch_path).
> 
> opts->ignore_other_worktrees was kept from the original expression;
> you are correct that is not needed anymore, but I thought it didn't
> hurt and made the code intention clearer (meaning it is obvious to
> anyone new to the code that this code will be skipped if that flag is
> set), would using an assert or a comment be a better option?

It's a good point that it makes the intention clearer, maybe we should 
just leave it as it is.

>>>        /*
>>>         * Extract branch name from command line arguments, so
>>>         * all that is left is pathspecs.
>>> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>>>                                             new_branch_info, opts, &rev);
>>>                argv += n;
>>>                argc -= n;
>>> +
>>> +             if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
>>> +                     check_branch_path = xstrdup(new_branch_info->path);
>>
>> I'm a bit confused what this is doing.
> 
> The branch we are interested in might come from 2 places, either it is
> a parameter to -b, which was picked up before, or it is the argument
> to the command itself, which was detected above.

Oh, of course. I was so focused on the -b that I'd forgotten we need the 
same check when we're checking out an existing branch - thanks for 
putting me right.

> If both are provided, we want to make sure to use the one from -b, or
> will have the bug you sharply spotted before, which was frankly
> embarrassing.
> 
>>> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
>>> index d587e0b20d..7ab7e87440 100755
>>> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
>>>        test_must_fail git -C here checkout newmain
>>>    '
>>>
>>> -test_expect_success 'not die the same branch is already checked out' '
>>> +test_expect_success 'allow creating multiple worktrees for same branch with force' '
>>> +     git worktree add --force anothernewmain newmain
>>> +'
>>> +
>>> +test_expect_success 'allow checkout/reset from the conflicted branch' '
>>
>> I'm not sure what "the conflicted branch" means (it reminds we of merge
>> conflicts).
> 
> by "conflicted" I meant one that is checked out in more than one worktree

I think it would be clearer so say that rather than "conflicted" which 
has a strong association with merge conflicts.

Best Wishes

Phillip

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Carlo
> ...
> As I said before, it would be much easier for everyone else to
> understand the changes if you wrote out what they were rather than
> saying "look at the tests"....
> ...
>> +	if (!opts->ignore_other_worktrees && !opts->force_detach &&
>> +	    check_branch_path && ref_exists(check_branch_path)) {
>
> I think check_branch_path is NULL if opts->ignore_other_worktrees is
> set so we could maybe lose "!opts->ignore_other_worktrees" here (or
> possibly below where you set check_branch_path).
> ...
>> ...
>> +		if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
>> +			check_branch_path = xstrdup(new_branch_info->path);
>
> I'm a bit confused what this is doing.
> ...
>> +test_expect_success 'allow checkout/reset from the conflicted branch' '
>
> I'm not sure what "the conflicted branch" means (it reminds we of
> merge conflicts). Is this just testing that "checkout -b/B <branch>
> <start-point>" works?
> ...
>> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '
>
> I think 'allow re-checking out ...' would be clearer, again I'm not
> sure what's conflicted here.
> ...
>>   -test_expect_success 'not die on re-checking out current branch' '
>> +test_expect_failure 'unless using force without --ignore-other-worktrees' '
>
> This test passes for me - what's the reason for changing from
> test_expect_success to test_expect_failure?
>
> Thanks for working on this

This topic has been dormant for a full two months.  Aside from
comments by Phillip (quoted above), are there remaining things
to be done and issues to be resolved before we can see v5?

Thanks.

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Carlo Arenas]

On Wed, Mar 22, 2023 at 5:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > Hi Carlo
> > ...
> > As I said before, it would be much easier for everyone else to
> > understand the changes if you wrote out what they were rather than
> > saying "look at the tests"....
> > ...
> >> +    if (!opts->ignore_other_worktrees && !opts->force_detach &&
> >> +        check_branch_path && ref_exists(check_branch_path)) {
> >
> > I think check_branch_path is NULL if opts->ignore_other_worktrees is
> > set so we could maybe lose "!opts->ignore_other_worktrees" here (or
> > possibly below where you set check_branch_path).
> > ...
> >> ...
> >> +            if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
> >> +                    check_branch_path = xstrdup(new_branch_info->path);
> >
> > I'm a bit confused what this is doing.
> > ...
> >> +test_expect_success 'allow checkout/reset from the conflicted branch' '
> >
> > I'm not sure what "the conflicted branch" means (it reminds we of
> > merge conflicts). Is this just testing that "checkout -b/B <branch>
> > <start-point>" works?
> > ...
> >> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '
> >
> > I think 'allow re-checking out ...' would be clearer, again I'm not
> > sure what's conflicted here.
> > ...
> >>   -test_expect_success 'not die on re-checking out current branch' '
> >> +test_expect_failure 'unless using force without --ignore-other-worktrees' '
> >
> > This test passes for me - what's the reason for changing from
> > test_expect_success to test_expect_failure?
> >
> > Thanks for working on this
>
> are there remaining things
> to be done and issues to be resolved before we can see v5?

it interacted with another branch
(rj/avoid-switching-to-already-used-branch) that was just recently
merged to master.

it also was probably too aggressive as pointed[1] by Phillip after my
explanation of the change of behaviour as quoted:

>> Using`checkout` for simplicity, but also applies to `switch`,
>>
>>    % git worktree list
>>    .../base  6a45aba [main]
>>    % git worktree add -f ../other main
>>    Preparing worktree (checking out 'main')
>>    HEAD is now at 6a45aba init
>>    % cd ../other
>>    % git checkout main
>>    Already on 'main'
>>    % git checkout -B main
>>    fatal: 'main' is already checked out at '.../base'
>
> Thanks for explaining that. If there is no <start-point> given we don't
> reset the branch so it seems a bit harsh to error out here. For "git
> checkout -B <branch> <start-point>" when <branch> is checked out in
> another worktree requiring --ignore-other-worktrees makes sense.

I wasn't sure on how to proceed from there, but will come with a v5 to
discuss further.

Carlo

[1] https://lore.kernel.org/git/a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk/

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Rubén Justo]

On 27-ene-2023 14:46:45, Phillip Wood wrote:
> Hi Carlo
> 
> On 20/01/2023 22:12, Carlo Arenas wrote:
> > On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > > 
> > > On 20/01/2023 11:35, 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.
> > > > 
> > > > Unlike what it is done for `git switch` and `git checkout`, that have
> > > > an historical exception to ignore other worktrees 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.
> > > 
> > > As I said before, it would be much easier for everyone else to
> > > understand the changes if you wrote out what they were rather than
> > > saying "look at the tests". I do appreciate the improved test
> > > descriptions though - thanks for that.
> > 
> > Apologies on that, I tried to come up with something that would
> > describe the change of behaviour other than the paragraph above and
> > couldn't come out with a better explanation except reading the tests
> > (which I know is complicated by the fact they are interlinked).
> > 
> > The behaviour I am changing is also not documented (other than by the
> > test) and might have been added as a quirk to keep the scripted rebase
> > and bisect going when confronted with branches that were checked out
> > in multiple worktrees, and as such might had not be intended for
> > `switch`, and might not be needed anymore either.
> > 
> > Using`checkout` for simplicity, but also applies to `switch`,
> > 
> >    % git worktree list
> >    .../base  6a45aba [main]
> >    % git worktree add -f ../other main
> >    Preparing worktree (checking out 'main')
> >    HEAD is now at 6a45aba init
> >    % cd ../other
> >    % git checkout main
> >    Already on 'main'
> >    % git checkout -B main
> >    fatal: 'main' is already checked out at '.../base'
> 
> Thanks for explaining that. If there is no <start-point> given we don't
> reset the branch so it seems a bit harsh to error out here.

We say in the documentation:

   +
   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>
   ------------
   +

and, since 55c4a67307 (Prevent force-updating of the current branch,
2011-08-20), we return with error on "git branch -f <current-branch> [...]".

Therefore, when the current branch is checked out in multiple worktrees, it
seems consequent to error on "git checkout -B <current_branch> [...]".

But we want to allow "git checkout -B <current_branch>", without <start-point>,
as a way to say "git reset --keep", see: 39bd6f7261 (Allow checkout -B
<current-branch> to update the current branch, 2011-11-26).

Your suggestion not to error is not only reasonable, but also seems desirable.

Thanks.

Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees

[Rubén Justo]

On 20/1/23 12:35, 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.
>
> Unlike what it is done for `git switch` and `git checkout`, that have
> an historical exception to ignore other worktrees 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.
>
> Reported-by: Jinwook Jeong <vustthat@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Helped-by: Rubén Justo <rjusto@gmail.com>
> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> Changes since v3
> * Code and Tests improvements as suggested by Phillip
> * Disable unreliable test that triggers a known bug
>
> Changes since v2
> * A leak free implementation
> * More details in commit as suggested by Junio
>
> Changes since v1
> * A much better commit message
> * Changes to the tests as suggested by Eric
> * Changes to the logic as suggested by Rubén
>
>
>  builtin/checkout.c      | 32 ++++++++++++++++++++++++--------
>  t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------
>  2 files changed, 51 insertions(+), 15 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3fa29a08ee..0688652f99 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)
>  }
>  
>  static int checkout_branch(struct checkout_opts *opts,
> -			   struct branch_info *new_branch_info)
> +			   struct branch_info *new_branch_info,
> +			   char *check_branch_path)
>  {
>  	if (opts->pathspec.nr)
>  		die(_("paths cannot be used with switching branches"));
> @@ -1533,13 +1534,13 @@ 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) {
> +	if (!opts->ignore_other_worktrees && !opts->force_detach &&
> +	    check_branch_path && ref_exists(check_branch_path)) {
>  		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);
> +		if (opts->new_branch_force || (head_ref &&
> +		    (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))
> +			die_if_checked_out(check_branch_path, 1);
>  		free(head_ref);
>  	}
>  
> @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  			 const char * const usagestr[],
>  			 struct branch_info *new_branch_info)
>  {
> +	int ret;
>  	int parseopt_flags = 0;
> +	char *check_branch_path = NULL;
>  
>  	opts->overwrite_ignore = 1;
>  	opts->prefix = prefix;
> @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  		opts->new_branch = argv0 + 1;
>  	}
>  
> +	if (opts->new_branch && !opts->ignore_other_worktrees) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		strbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);
> +		strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
> +		check_branch_path = strbuf_detach(&buf, NULL);
> +	}
>  	/*
>  	 * Extract branch name from command line arguments, so
>  	 * all that is left is pathspecs.
> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  					     new_branch_info, opts, &rev);
>  		argv += n;
>  		argc -= n;
> +
> +		if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)
> +			check_branch_path = xstrdup(new_branch_info->path);
>  	} else if (!opts->accept_ref && opts->from_treeish) {
>  		struct object_id rev;
>  
> @@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
>  	}
>  
>  	if (opts->patch_mode || opts->pathspec.nr)
> -		return checkout_paths(opts, new_branch_info);
> +		ret = checkout_paths(opts, new_branch_info);
>  	else
> -		return checkout_branch(opts, new_branch_info);
> +		ret = checkout_branch(opts, new_branch_info, check_branch_path);
> +
> +	free(check_branch_path);
> +	return ret;
>  }
>  
>  int cmd_checkout(int argc, const char **argv, const char *prefix)
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index d587e0b20d..7ab7e87440 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -118,14 +118,17 @@ test_expect_success '"add" worktree creating new branch' '
>  	)
>  '
>  
> -test_expect_success 'die the same branch is already checked out' '
> +test_expect_success 'die if the same branch is already checked out' '
>  	(
>  		cd here &&
> -		test_must_fail git checkout newmain
> +		test_must_fail git checkout newmain &&
> +		test_must_fail git checkout -B newmain &&
> +		test_must_fail git switch newmain &&
> +		test_must_fail git switch -C newmain
>  	)
>  '
>  
> -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '
> +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '
>  	head=$(git -C there rev-parse --git-path HEAD) &&
>  	ref=$(git -C there symbolic-ref HEAD) &&
>  	rm "$head" &&
> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin
>  	test_must_fail git -C here checkout newmain
>  '
>  
> -test_expect_success 'not die the same branch is already checked out' '
> +test_expect_success 'allow creating multiple worktrees for same branch with force' '
> +	git worktree add --force anothernewmain newmain
> +'
> +
> +test_expect_success 'allow checkout/reset from the conflicted branch' '
>  	(
>  		cd here &&
> -		git worktree add --force anothernewmain newmain
> +		git checkout -b conflictedmain newmain &&
> +		git checkout -B conflictedmain newmain &&
> +		git switch -C conflictedmain newmain
> +	)
> +'
> +
> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '
> +	(
> +		cd there &&
> +		git checkout newmain &&
> +		git switch newmain
>  	)
>  '
>  
> -test_expect_success 'not die on re-checking out current branch' '
> +test_expect_failure 'unless using force without --ignore-other-worktrees' '

The fix to what caused this test to occasionally fail is already in
'master'.  This test should now succeed consistently.

>  	(
>  		cd there &&
> -		git checkout newmain
> +		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 newmain
>  	)
>  '
>  
>

Thanks for continuing to work on this.