{"thread":{"id":"59095","subject":"[PATCH] builtin/checkout: check the branch used in -B with worktrees","startedAt":"2023-01-16T17:50:00Z","lastAt":"2023-05-14T20:24:42Z","messageCount":24,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Rubén Justo","Carlo Arenas","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470445","messageId":"20230116172824.93218-1-carenas@gmail.com","threadId":"59095","inReplyTo":null,"subject":"[PATCH] builtin/checkout: check the branch used in -B with worktrees","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-16T17:28:24Z","receivedAt":"2023-01-16T17:50:00Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When multiple worktrees are being used, checkout/switch check\nthat the target branch is not already checked out with code that\nevolved from 8d9fdd7087 (worktree.c: check whether branch is rebased\nin another worktree, 2016-04-22), but that logic wasn't valid for\n-B/-C\n\nAvoid reusing the same `branch_info` structure for the checks and\nassumming that it will be rejected later if is a new branch that\nalready exists as that doesn't apply to -B/-C.\n\nReported-by: Jinwook Jeong \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/checkout.c | 22 ++++++++++++++++------\n t/t2400-worktree-add.sh | 14 ++++++++++++++\n 2 files changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3fa29a08ee..94dcd617ef 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)\n }\n \n static int checkout_branch(struct checkout_opts *opts,\n-\t\t\t struct branch_info *new_branch_info)\n+\t\t\t struct branch_info *new_branch_info,\n+\t\t\t struct branch_info *check_branch_info)\n {\n \tif (opts->pathspec.nr)\n \t\tdie(_(\"paths cannot be used with switching branches\"));\n@@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,\n \tif (!opts->can_switch_when_in_progress)\n \t\tdie_if_some_operation_in_progress();\n \n-\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n-\t !opts->ignore_other_worktrees) {\n+\tif (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {\n \t\tint flag;\n \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n \t\tif (head_ref &&\n-\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n-\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n+\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))\n+\t\t\tdie_if_checked_out(check_branch_info->path, 1);\n \t\tfree(head_ref);\n \t}\n \n@@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t struct branch_info *new_branch_info)\n {\n \tint parseopt_flags = 0;\n+\tstruct branch_info check_branch_info = { 0 };\n \n \topts->overwrite_ignore = 1;\n \topts->prefix = prefix;\n@@ -1739,6 +1740,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t!opts->new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n \t\t\t\t\t new_branch_info, opts, &rev);\n+\t\tcheck_branch_info = *new_branch_info;\n \t\targv += n;\n \t\targc -= n;\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n@@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t\t\t\t opts, &rev,\n \t\t\t\t\t\t opts->from_treeish);\n \n+\t\tcheck_branch_info = *new_branch_info;\n \t\tif (!opts->source_tree)\n \t\t\tdie(_(\"reference is not a tree: %s\"), opts->from_treeish);\n+\t} else if (opts->new_branch) {\n+\t\tstruct object_id rev;\n+\n+\t\tif (!get_oid_mb(opts->new_branch, &rev))\n+\t\t\tsetup_new_branch_info_and_source_tree(&check_branch_info,\n+\t\t\t\t\t\t\topts, &rev,\n+\t\t\t\t\t\t\topts->new_branch);\n \t}\n \n \tif (argc) {\n@@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \tif (opts->patch_mode || opts->pathspec.nr)\n \t\treturn checkout_paths(opts, new_branch_info);\n \telse\n-\t\treturn checkout_branch(opts, new_branch_info);\n+\t\treturn checkout_branch(opts, new_branch_info, &check_branch_info);\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex d587e0b20d..283ba7607e 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '\n \t)\n '\n \n+test_expect_success 'die the same branch is already checked out (checkout -B)' '\n+\t(\n+\t\tcd here &&\n+\t\ttest_must_fail git checkout -B newmain\n+\t)\n+'\n+\n test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '\n \thead=$(git -C there rev-parse --git-path HEAD) &&\n \tref=$(git -C there symbolic-ref HEAD) &&\n@@ -147,6 +154,13 @@ test_expect_success 'not die on re-checking out current branch' '\n \t)\n '\n \n+test_expect_success 'not die on re-checking out current branch (checkout -B)' '\n+\t(\n+\t\tcd there &&\n+\t\tgit checkout -B newmain\n+\t)\n+'\n+\n test_expect_success '\"add\" from a bare repo' '\n \t(\n \t\tgit clone --bare . bare &&\n-- \n2.39.0.1.g119e9c6876.dirty\n\n"},{"id":"470458","messageId":"CAPig+cR3=wDFJPr8ViUTVFDx-AvaJUGWNUnqndJ2edQPL5smVw@mail.gmail.com","threadId":"59095","inReplyTo":"20230116172824.93218-1-carenas@gmail.com","subject":"Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-16T22:18:04Z","receivedAt":"2023-01-16T22:18:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 16, 2023 at 12:30 PM Carlo Marcelo Arenas Belón\n wrote:\n> builtin/checkout: check the branch used in -B with worktrees\n\nThanks for working on this and coming up with a fix. As mentioned\nearlier, I had started looking into it[1], but lacked the time to\ndisentangle the logic, so I'm glad to see a patch arrive so quickly.\n\n> When multiple worktrees are being used, checkout/switch check\n> that the target branch is not already checked out with code that\n> evolved from 8d9fdd7087 (worktree.c: check whether branch is rebased\n> in another worktree, 2016-04-22), but that logic wasn't valid for\n> -B/-C\n>\n> Avoid reusing the same `branch_info` structure for the checks and\n> assumming that it will be rejected later if is a new branch that\n> already exists as that doesn't apply to -B/-C.\n\nEven though I'm familiar with the bug report[2] which sparked this\npatch, I find the above description somewhat hard to digest; the\nhigh-level problem it is addressing doesn't jump off the page at me.\nPerhaps it can be rewritten something like this:\n\n checkout/switch: disallow checking out same branch in multiple worktrees\n\n Commands `git switch -C` and `git checkout -B` neglect to check\n whether the branch being forcibly created is already checked out\n in some other worktree, which can result in the undesirable\n situation of the same branch being checked out in multiple\n worktrees. For instance:\n\n $ git worktree list\n .../foo beefb00f [main]\n $ git worktree add ../other\n Preparing worktree (new branch 'other')\n HEAD is now at beefb00f first\n $ cd ../other\n $ git switch -C main\n Switched to and reset branch 'main'\n $ git worktree list\n .../foo beefb00f [main]\n .../other beefb00f [main]\n $\n\n Fix this problem by teaching `git switch -C` and `git checkout -B`\n to check whether the branch in question is already checked out\n elsewhere.\n\nafter which you might include some details which you wrote about initially.\n\n> Reported-by: Jinwook Jeong \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,\n> - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> - !opts->ignore_other_worktrees) {\n> + if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {\n> int flag;\n> char *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n> if (head_ref &&\n> - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n> - die_if_checked_out(new_branch_info->path, 1);\n> + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))\n> + die_if_checked_out(check_branch_info->path, 1);\n\nThis variable name change (`new_branch_info` => `check_branch_info`)\nhelps make the code clearer. Good. (I had found it more than a little\nconfusing to have similar named variables `new_branch_info` and\n`opts->new_branch` even though they are unrelated and have very\ndifferent purposes.)\n\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> @@ -125,6 +125,13 @@ test_expect_success 'die the same branch is already checked out' '\n> +test_expect_success 'die the same branch is already checked out (checkout -B)' '\n> + (\n> + cd here &&\n> + test_must_fail git checkout -B newmain\n> + )\n> +'\n\nAlthough `git switch` and `git checkout` currently share\nimplementation, that might not always be the case going forward. As\nsuch, this test could be made a bit more robust by testing both\ncommands rather than just `git-checkout`. So, perhaps:\n\n test_expect_success 'die the same branch is already checked out' '\n (\n cd here &&\n test_must_fail git checkout -B newmain &&\n test_must_fail git switch -C newmain\n )\n '\n\n> +test_expect_success 'not die on re-checking out current branch (checkout -B)' '\n> + (\n> + cd there &&\n> + git checkout -B newmain\n> + )\n> +'\n\nGood to see you considered this case too. (I had tested it myself\nmanually when trying out your patch.)\n\n[1]: https://lore.kernel.org/git/CAPig+cQc1+D9gH7BAC-r03bGKWx3a9jpPyLuP-ehH-X2P+fV6Q@mail.gmail.com/\n[2]: https://lore.kernel.org/git/CAA3Q-aaO=vcZd9VLFr8UP-g06be80eUWN_GjygfyGkYmrLx9yQ@mail.gmail.com/\n"},{"id":"470460","messageId":"45cb1b38-1284-ddc9-a2d8-0a45f10abafb@gmail.com","threadId":"59095","inReplyTo":"20230116172824.93218-1-carenas@gmail.com","subject":"Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-17T00:53:23Z","receivedAt":"2023-01-17T00:53:32Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Hi Carlo.\n\nThank you for working on this.\n\nOn 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:\n\n> @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,\n> \tif (!opts->can_switch_when_in_progress)\n> \t\tdie_if_some_operation_in_progress();\n> \n> -\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> -\t !opts->ignore_other_worktrees) {\n> +\tif (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {\n> \t\tint flag;\n> \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n> \t\tif (head_ref &&\n> -\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n> -\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n> +\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))\n> +\t\t\tdie_if_checked_out(check_branch_info->path, 1);\n\n\nYou adjusted this block to accommodate the problem with \"checkout -B\",\nwhich is sensible, but we may need to do something different here.\n\nWhat we are doing here, if I understand it correctly, is dying if the\nbranch is _not checked out in the current worktree_ and _checked out in\nany other worktree_. Which is OK for normal checkout/switch.\n\nBut IMHO with \"checkout -B\" we have to die if the branch is checked out\nin any other worktree, regardless of whether or not it is checked out in\nthe current working tree.\n\nPerhaps the scenario where the user has the same branch checked out in\nmultiple worktrees and tries to reset in one of them, is one of those\nwhere we could use the \"if it hurts, don't do it\". But we are providing\nthe \"--ignore-other-worktrees\" modifier, so I think we should disallow\nthe \"-B\" if the branch is checked out in any other worktree, and let\nthe user use \"--ignore-other-worktrees\" if he wants to go wild.\n\nBut.. die_if_checked_out() does not correctly honor the\n\"ignore_current_worktree\" in this situation. I have submitted a patch\nto fix this, in case you want to consider all of this.\n\nUn saludo.\n"},{"id":"470580","messageId":"CAPUEsphrbPVZtZi_GV9=8OqOqjk+SZ1JJf1bU_HWpUnT1H6YzA@mail.gmail.com","threadId":"59095","inReplyTo":"45cb1b38-1284-ddc9-a2d8-0a45f10abafb@gmail.com","subject":"Re: [PATCH] builtin/checkout: check the branch used in -B with worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-18T05:44:16Z","receivedAt":"2023-01-18T05:44:35Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Jan 16, 2023 at 4:53 PM Rubén Justo wrote:\n> On 16/1/23 18:28, Carlo Marcelo Arenas Belón wrote:\n>\n> > @@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,\n> > if (!opts->can_switch_when_in_progress)\n> > die_if_some_operation_in_progress();\n> >\n> > - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> > - !opts->ignore_other_worktrees) {\n> > + if (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {\n> > int flag;\n> > char *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n> > if (head_ref &&\n> > - (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n> > - die_if_checked_out(new_branch_info->path, 1);\n> > + (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path)))\n> > + die_if_checked_out(check_branch_info->path, 1);\n>\n> What we are doing here, if I understand it correctly, is dying if the\n> branch is _not checked out in the current worktree_ and _checked out in\n> any other worktree_. Which is OK for normal checkout/switch.\n\nI think the exception was added to `checkout` only, where it is\ndefinitely needed, but switch probably should be more strict as it is\nnot \"plumbing\" and as you pointed out there is already a UI option to\noverride its safety.\n\n> But IMHO with \"checkout -B\" we have to die if the branch is checked out\n> in any other worktree, regardless of whether or not it is checked out in\n> the current working tree.\n\nI have to admit, I thought about that too, but then avoided making a\nchange as checkout behaviour affects a lot of other places, but in\nretrospect I think that in this case it might be worth the change of\nbehaviour, since it is connected with the bugfix.\n\nBefore, the operation was allowed and the logic never tried to prevent\nit (which is why in my first look, I thought it might have been\nintentional), but once Eric pointed out it was a bug, then the obvious\nconclusion would be to prevent it with the extended logic, as you\npointed out.\n\n> Perhaps the scenario where the user has the same branch checked out in\n> multiple worktrees and tries to reset in one of them, is one of those\n> where we could use the \"if it hurts, don't do it\". But we are providing\n> the \"--ignore-other-worktrees\" modifier, so I think we should disallow\n> the \"-B\" if the branch is checked out in any other worktree, and let\n> the user use \"--ignore-other-worktrees\" if he wants to go wild.\n\nv2 includes this and AFAIK nothing is broken yet.\n\nCarlo\n\nPS. As explained before, tightening `switch` might be a good idea, but\nit is obviously punted for this change and will be addressed later.\n"},{"id":"470581","messageId":"20230118061527.76218-1-carenas@gmail.com","threadId":"59095","inReplyTo":"20230116172824.93218-1-carenas@gmail.com","subject":"[PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-18T06:15:27Z","receivedAt":"2023-01-18T06:29:36Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Commands `git switch -C` and `git checkout -B` neglect to check whether\nthe provided branch is already checked out in some other worktree, as\nshown by the following example:\n\n $ git worktree list\n .../foo beefb00f [main]\n $ git worktree add ../other\n Preparing worktree (new branch 'other')\n HEAD is now at beefb00f first\n $ cd ../other\n $ git switch -C main\n Switched to and reset branch 'main'\n $ git worktree list\n .../foo beefb00f [main]\n .../other beefb00f [main]\n\nFix this problem by teaching `git switch -C` and `git checkout -B` to\ncheck whether the branch in question is already checked out elsewhere\nby expanding on the existing checks that are being used by their non\nforce variants.\n\nAs reflected on the tests, this will change the behaviour of those\ncommands when they are invoked in a worktree that has that requested\nbranch checked out, as that matches the logic used by branch, is safer\n(assuming both commands are user facing) and can be overriden with an\nexisting flag.\n\nReported-by: Jinwook Jeong \nHelped-by: Rubén Justo \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nChanges since v1\n* A much better commit message\n* Changes to the tests as suggested by Eric\n* Changes to the logic as suggested by Rubén\n\n builtin/checkout.c | 24 +++++++++++++++++-------\n t/t2400-worktree-add.sh | 18 ++++++++++++++++--\n 2 files changed, 33 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3fa29a08ee..58a956392b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)\n }\n \n static int checkout_branch(struct checkout_opts *opts,\n-\t\t\t struct branch_info *new_branch_info)\n+\t\t\t struct branch_info *new_branch_info,\n+\t\t\t struct branch_info *check_branch_info)\n {\n \tif (opts->pathspec.nr)\n \t\tdie(_(\"paths cannot be used with switching branches\"));\n@@ -1533,13 +1534,12 @@ static int checkout_branch(struct checkout_opts *opts,\n \tif (!opts->can_switch_when_in_progress)\n \t\tdie_if_some_operation_in_progress();\n \n-\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n-\t !opts->ignore_other_worktrees) {\n+\tif (check_branch_info->path && !opts->force_detach && !opts->ignore_other_worktrees) {\n \t\tint flag;\n \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n-\t\tif (head_ref &&\n-\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n-\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n+\t\tif (opts->new_branch_force || (head_ref &&\n+\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_info->path))))\n+\t\t\tdie_if_checked_out(check_branch_info->path, 1);\n \t\tfree(head_ref);\n \t}\n \n@@ -1628,6 +1628,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t struct branch_info *new_branch_info)\n {\n \tint parseopt_flags = 0;\n+\tstruct branch_info check_branch_info = { 0 };\n \n \topts->overwrite_ignore = 1;\n \topts->prefix = prefix;\n@@ -1739,6 +1740,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t!opts->new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n \t\t\t\t\t new_branch_info, opts, &rev);\n+\t\tcheck_branch_info = *new_branch_info;\n \t\targv += n;\n \t\targc -= n;\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n@@ -1751,8 +1753,16 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t\t\t\t opts, &rev,\n \t\t\t\t\t\t opts->from_treeish);\n \n+\t\tcheck_branch_info = *new_branch_info;\n \t\tif (!opts->source_tree)\n \t\t\tdie(_(\"reference is not a tree: %s\"), opts->from_treeish);\n+\t} else if (opts->new_branch) {\n+\t\tstruct object_id rev;\n+\n+\t\tif (!get_oid_mb(opts->new_branch, &rev))\n+\t\t\tsetup_new_branch_info_and_source_tree(&check_branch_info,\n+\t\t\t\t\t\t\topts, &rev,\n+\t\t\t\t\t\t\topts->new_branch);\n \t}\n \n \tif (argc) {\n@@ -1819,7 +1829,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \tif (opts->patch_mode || opts->pathspec.nr)\n \t\treturn checkout_paths(opts, new_branch_info);\n \telse\n-\t\treturn checkout_branch(opts, new_branch_info);\n+\t\treturn checkout_branch(opts, new_branch_info, &check_branch_info);\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex d587e0b20d..a66f9bb30c 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -121,7 +121,10 @@ test_expect_success '\"add\" worktree creating new branch' '\n test_expect_success 'die the same branch is already checked out' '\n \t(\n \t\tcd here &&\n-\t\ttest_must_fail git checkout newmain\n+\t\ttest_must_fail git checkout newmain &&\n+\t\ttest_must_fail git checkout -B newmain &&\n+\t\ttest_must_fail git switch newmain &&\n+\t\ttest_must_fail git switch -C newmain\n \t)\n '\n \n@@ -143,7 +146,18 @@ test_expect_success 'not die the same branch is already checked out' '\n test_expect_success 'not die on re-checking out current branch' '\n \t(\n \t\tcd there &&\n-\t\tgit checkout newmain\n+\t\tgit checkout newmain &&\n+\t\tgit switch newmain\n+\t)\n+'\n+\n+test_expect_success 'but die if using force without --ignore-other-worktrees' '\n+\t(\n+\t\tcd there &&\n+\t\ttest_must_fail git checkout -B newmain &&\n+\t\ttest_must_fail git switch -C newmain &&\n+\t\tgit checkout --ignore-other-worktrees -B newmain &&\n+\t\tgit switch --ignore-other-worktrees -C newmain\n \t)\n '\n \n-- \n2.37.1 (Apple Git-137.1)\n\n"},{"id":"470584","messageId":"xmqq5yd48hcb.fsf@gitster.g","threadId":"59095","inReplyTo":"20230118061527.76218-1-carenas@gmail.com","subject":"Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-18T06:52:36Z","receivedAt":"2023-01-18T07:36:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> As reflected on the tests, this will change the behaviour of those\n> commands when they are invoked in a worktree that has that requested\n> branch checked out, as that matches the logic used by branch, is safer\n> (assuming both commands are user facing) and can be overriden with an\n> existing flag.\n\n... meaning you can \"--force\", or something else? Allowing an\nexisting option to be used as the safety valve does make sense,\nespecially if the option is something users are already familiar\nwith (like \"--force\") and naturally expected to work.\n\nThere might need an documentation update. Back when \"checkout -b\"\nand \"branch\" was written, there wasn't \"multiple worktrees connected\nto a single repository\" hence there was no need to provide safety\nagainst checking out the same branch in two different places. \"git\nbranch\" might have learned th give that safety while \"git checkout\n-b\", which _ought_ to be equivalent to \"git branch\" followed by \"git\ncheckout\", might have forgot to do so. After this change, it may\nstill be correct to say that \"checkout -b\" is equivalent to \"branch\"\nfollowed by \"checkout\", but if the documentation to \"branch\" talks\nabout this safety, it probably deserves to be mentioned in the\ndocumentation to \"checkout -b\", as well, if only to give an appropriate\nplace to talk about how to override it \"with an existing flag\".\n\nThanks.\n"},{"id":"470588","messageId":"CAPUEspjmFJUjZmNBH=f_-TY3hYnOtgVjBY-YtWuo15eg3a+5cQ@mail.gmail.com","threadId":"59095","inReplyTo":"xmqq5yd48hcb.fsf@gitster.g","subject":"Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-18T07:58:35Z","receivedAt":"2023-01-18T08:49:36Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jan 17, 2023 at 10:52 PM Junio C Hamano wrote:\n>\n> Carlo Marcelo Arenas Belón writes:\n>\n> > As reflected on the tests, this will change the behaviour of those\n> > commands when they are invoked in a worktree that has that requested\n> > branch checked out, as that matches the logic used by branch, is safer\n> > (assuming both commands are user facing) and can be overriden with an\n> > existing flag.\n>\n> ... meaning you can \"--force\", or something else? Allowing an\n> existing option to be used as the safety valve does make sense,\n> especially if the option is something users are already familiar\n> with (like \"--force\") and naturally expected to work.\n\nthe following is the way to override:\n\n$ git checkout --ignore-other-worktrees -B foo\n\n> There might need an documentation update. Back when \"checkout -b\"\n> and \"branch\" was written, there wasn't \"multiple worktrees connected\n> to a single repository\" hence there was no need to provide safety\n> against checking out the same branch in two different places. \"git\n> branch\" might have learned to give that safety while \"git checkout\n> -b\", which _ought_ to be equivalent to \"git branch\" followed by \"git\n> checkout\", might have forgot to do so.\n\nNot sure if it was originally forgotten, but it is definitely working now;\nthis change only fixes the uppercase (-B) version.\n\n> After this change, it may\n> still be correct to say that \"checkout -b\" is equivalent to \"branch\"\n> followed by \"checkout\", but if the documentation to \"branch\" talks\n> about this safety, it probably deserves to be mentioned in the\n> documentation to \"checkout -b\", as well, if only to give an appropriate\n> place to talk about how to override it \"with an existing flag\".\n\nInterestingly, when the flag was added in 1d0fa898ea (checkout: add\n--ignore-other-wortrees, 2015-01-03), it was only added to `checkout`.\n\n`git branch` has no flag and will die even when `-f` is used\n\nCarlo\n"},{"id":"470637","messageId":"xmqqilh36cxd.fsf@gitster.g","threadId":"59095","inReplyTo":"CAPUEspjmFJUjZmNBH=f_-TY3hYnOtgVjBY-YtWuo15eg3a+5cQ@mail.gmail.com","subject":"Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-18T16:10:54Z","receivedAt":"2023-01-18T16:15:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> the following is the way to override:\n>\n> $ git checkout --ignore-other-worktrees -B foo\n\nMy points were that (1) the option is way too unintuitive to\nanybody, other than those who stared at the implementation of the\nproblematic logic for too long, and that (2) it wasn't mentioned in\nthe proposed log message or documentation updates. If \"--force\" is\nmade to mean that, it might be easier to discover to the users, but\nI have no strong opinion on that (meaning: there may be downsides to\nallow use of \"--force\" to override this safety, given that \"-B\" is\nalready considered as a \"forcing\" version of \"-b\").\n\n> `git branch` has no flag and will die even when `-f` is used\n\nIf \"--force\" does not force it, I suspect it should be considered a\nbug.\n\nThanks.\n"},{"id":"470678","messageId":"xmqqbkmv4fnb.fsf@gitster.g","threadId":"59095","inReplyTo":"20230118061527.76218-1-carenas@gmail.com","subject":"Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-18T22:55:04Z","receivedAt":"2023-01-18T22:55:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> Changes since v1\n> * A much better commit message\n> * Changes to the tests as suggested by Eric\n> * Changes to the logic as suggested by Rubén\n\nI queued this topic at the tip of 'seen' as 2fe0b4e3 (Merge branch\n'cb/checkout-same-branch-twice' into seen, 2023-01-18), on top of\n4ea8693b (Merge branch 'mc/credential-helper-auth-headers' into\nseen, 2023-01-18).\n\n - 4ea8693b - https://github.com/git/git/actions/runs/3952916442\n - 2fe0b4e3 - https://github.com/git/git/actions/runs/3953521066\n\nComparing these two runs, inclusion of this topic seems to introduce\nnew leaks, as t1408 and t2018 (neither of which was touched by this\ntopic) that used to pass are now failing.\n\n> builtin/checkout.c | 24 +++++++++++++++++-------\n> t/t2400-worktree-add.sh | 18 ++++++++++++++++--\n> 2 files changed, 33 insertions(+), 9 deletions(-)\n\nThanks. \n"},{"id":"470700","messageId":"20230119055325.1013-1-carenas@gmail.com","threadId":"59095","inReplyTo":"20230118061527.76218-1-carenas@gmail.com","subject":"[PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-19T05:53:24Z","receivedAt":"2023-01-19T05:53:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Commands `git switch -C` and `git checkout -B` neglect to check whether\nthe provided branch is already checked out in some other worktree, as\nshown by the following:\n\n $ git worktree list\n .../foo beefb00f [main]\n $ git worktree add ../other\n Preparing worktree (new branch 'other')\n HEAD is now at beefb00f first\n $ cd ../other\n $ git switch -C main\n Switched to and reset branch 'main'\n $ git worktree list\n .../foo beefb00f [main]\n .../other beefb00f [main]\n\nFix this problem by teaching `git switch -C` and `git checkout -B` to\ncheck whether the branch in question is already checked out elsewhere\nby expanding on the existing checks that are being used by their non\nforce variants.\n\nUnlike what it is done for `git switch` and `git checkout`, that have\nan historical exception to ignore other workspaces if the branch to\ncheck is the current one (as required when called as part of other\ntools), the logic implemented is more strict and will require the user\nto invoke the command with `--ignore-other-worktrees` to explicitly\nindicate they want the risky behaviour.\n\nThis matches the current behaviour of `git branch -f` and is safer, for\nmore details see the tests in t2400.\n\nReported-by: Jinwook Jeong \nHelped-by: Rubén Justo \nHelped-by: Eric Sunshine \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nChanges since v2\n* A leak free implementation\n* More details in commit as suggested by Junio\n\nChanges since v1\n* A much better commit message\n* Changes to the tests as suggested by Eric\n* Changes to the logic as suggested by Rubén\n\n builtin/checkout.c | 31 +++++++++++++++++++++++--------\n t/t2400-worktree-add.sh | 18 ++++++++++++++++--\n 2 files changed, 39 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5963e1b74b..467e194701 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1476,7 +1476,8 @@ static void die_if_some_operation_in_progress(void)\n }\n \n static int checkout_branch(struct checkout_opts *opts,\n-\t\t\t struct branch_info *new_branch_info)\n+\t\t\t struct branch_info *new_branch_info,\n+\t\t\t char *check_branch_path)\n {\n \tif (opts->pathspec.nr)\n \t\tdie(_(\"paths cannot be used with switching branches\"));\n@@ -1535,15 +1536,15 @@ static int checkout_branch(struct checkout_opts *opts,\n \tif (!opts->can_switch_when_in_progress)\n \t\tdie_if_some_operation_in_progress();\n \n-\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n-\t !opts->ignore_other_worktrees) {\n+\tif (check_branch_path && !opts->force_detach && !opts->ignore_other_worktrees) {\n \t\tint flag;\n \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n-\t\tif (head_ref &&\n-\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n-\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n+\t\tif (opts->new_branch_force || (head_ref &&\n+\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))\n+\t\t\tdie_if_checked_out(check_branch_path, 1);\n \t\tfree(head_ref);\n \t}\n+\tfree(check_branch_path);\n \n \tif (!new_branch_info->commit && opts->new_branch) {\n \t\tstruct object_id rev;\n@@ -1630,6 +1631,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t struct branch_info *new_branch_info)\n {\n \tint parseopt_flags = 0;\n+\tchar *check_branch_path = NULL;\n \n \topts->overwrite_ignore = 1;\n \topts->prefix = prefix;\n@@ -1741,6 +1743,7 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t!opts->new_branch;\n \t\tint n = parse_branchname_arg(argc, argv, dwim_ok,\n \t\t\t\t\t new_branch_info, opts, &rev);\n+\t\tcheck_branch_path = xstrdup_or_null(new_branch_info->path);\n \t\targv += n;\n \t\targc -= n;\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n@@ -1753,8 +1756,18 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t\t\t\t opts, &rev,\n \t\t\t\t\t\t opts->from_treeish);\n \n+\t\tcheck_branch_path = xstrdup_or_null(new_branch_info->path);\n \t\tif (!opts->source_tree)\n \t\t\tdie(_(\"reference is not a tree: %s\"), opts->from_treeish);\n+\t} else if (opts->new_branch && !opts->ignore_other_worktrees) {\n+\t\tstruct object_id rev;\n+\n+\t\tif (!get_oid_mb(opts->new_branch, &rev)) {\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tstrbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);\n+\t\t\tstrbuf_splice(&buf, 0, 0, \"refs/heads/\", 11);\n+\t\t\tcheck_branch_path = strbuf_detach(&buf, NULL);\n+\t\t}\n \t}\n \n \tif (argc) {\n@@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\tstrbuf_release(&buf);\n \t}\n \n-\tif (opts->patch_mode || opts->pathspec.nr)\n+\tif (opts->patch_mode || opts->pathspec.nr) {\n+\t\tfree(check_branch_path);\n \t\treturn checkout_paths(opts, new_branch_info);\n+\t}\n \telse\n-\t\treturn checkout_branch(opts, new_branch_info);\n+\t\treturn checkout_branch(opts, new_branch_info, check_branch_path);\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex d587e0b20d..a66f9bb30c 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -121,7 +121,10 @@ test_expect_success '\"add\" worktree creating new branch' '\n test_expect_success 'die the same branch is already checked out' '\n \t(\n \t\tcd here &&\n-\t\ttest_must_fail git checkout newmain\n+\t\ttest_must_fail git checkout newmain &&\n+\t\ttest_must_fail git checkout -B newmain &&\n+\t\ttest_must_fail git switch newmain &&\n+\t\ttest_must_fail git switch -C newmain\n \t)\n '\n \n@@ -143,7 +146,18 @@ test_expect_success 'not die the same branch is already checked out' '\n test_expect_success 'not die on re-checking out current branch' '\n \t(\n \t\tcd there &&\n-\t\tgit checkout newmain\n+\t\tgit checkout newmain &&\n+\t\tgit switch newmain\n+\t)\n+'\n+\n+test_expect_success 'but die if using force without --ignore-other-worktrees' '\n+\t(\n+\t\tcd there &&\n+\t\ttest_must_fail git checkout -B newmain &&\n+\t\ttest_must_fail git switch -C newmain &&\n+\t\tgit checkout --ignore-other-worktrees -B newmain &&\n+\t\tgit switch --ignore-other-worktrees -C newmain\n \t)\n '\n \n-- \n2.37.1 (Apple Git-137.1)\n\n"},{"id":"470702","messageId":"xmqqa62f2dj1.fsf_-_@gitster.g","threadId":"59095","inReplyTo":"20230119055325.1013-1-carenas@gmail.com","subject":"Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-19T07:23:46Z","receivedAt":"2023-01-19T07:24:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> Changes since v2\n> * A leak free implementation\n\n> * More details in commit as suggested by Junio\n\nI meant to say we may need more details in the documentation, but\nafter reading the existing documentation, we say that\n\n - \"-B \" is equivalent to run \"branch -f \", which is\n sufficient to hint that it will fail to recreate and check out an\n existing branch that is checked out elsewhere, because \"branch\n -f\" would fail in such a situation.\n\nand that\n\n - \"--ignore-other-worktrees\" defeats the safety that makes \"git\n checkout refuses when the wanted ref is already checked out\".\n\nso the existing documentation of \"git checkout\" may already be OK.\n\nAs long as it is well known that \"git branch -f\" still fails in the\nsituation, that is. After re-reading \"git branch --help\" twice,\nhowever, I am not sure if it is so clear, though.\n\nHow about adding something like this, as an independent\ndocumentation improvement?\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] branch: document `-f` and linked worktree behaviour\n\n\"git branch -f name start\" forces to recreate the named branch, but\nthe forcing does not defeat the \"do not touch a branch that is\nchecked out elsewhere\" safety valve.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/git-branch.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git c/Documentation/git-branch.txt w/Documentation/git-branch.txt\nindex aa2f78c4c2..b12e7940d3 100644\n--- c/Documentation/git-branch.txt\n+++ w/Documentation/git-branch.txt\n@@ -123,6 +123,10 @@ OPTIONS\n \tpoints to a valid commit. In combination with\n \t`-m` (or `--move`), allow renaming the branch even if the new\n \tbranch name already exists, the same applies for `-c` (or `--copy`).\n++\n+Note that 'git branch -f []' refuses to change\n+an existing branch `` that is checked out in another worktree\n+linked to the same repository.\n \n -m::\n --move::\n"},{"id":"470703","messageId":"CAPUEsphr5hhszLmXc84_87gUq3Fc2yQo3_oxaP+UdODaCXHDeA@mail.gmail.com","threadId":"59095","inReplyTo":"xmqqa62f2dj1.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-19T07:41:29Z","receivedAt":"2023-01-19T07:41:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Thanks, that should round up the documentation for this behaviour nicely.\n\nCarlo\n"},{"id":"470708","messageId":"6a0499d4-ad6e-56b7-de72-0f8a3697b534@dunelm.org.uk","threadId":"59095","inReplyTo":"20230119055325.1013-1-carenas@gmail.com","subject":"Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-19T14:21:05Z","receivedAt":"2023-01-19T14:21:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nThanks for working on this\n\nOn 19/01/2023 05:53, Carlo Marcelo Arenas Belón wrote:\n> Commands `git switch -C` and `git checkout -B` neglect to check whether\n> the provided branch is already checked out in some other worktree, as\n> shown by the following:\n> \n> $ git worktree list\n> .../foo beefb00f [main]\n> $ git worktree add ../other\n> Preparing worktree (new branch 'other')\n> HEAD is now at beefb00f first\n> $ cd ../other\n> $ git switch -C main\n> Switched to and reset branch 'main'\n> $ git worktree list\n> .../foo beefb00f [main]\n> .../other beefb00f [main]\n> \n> Fix this problem by teaching `git switch -C` and `git checkout -B` to\n> check whether the branch in question is already checked out elsewhere\n> by expanding on the existing checks that are being used by their non\n> force variants.\n >\n> Unlike what it is done for `git switch` and `git checkout`, that have\n> an historical exception to ignore other workspaces if the branch to\n> check is the current one (as required when called as part of other\n> tools), the logic implemented is more strict and will require the user\n> to invoke the command with `--ignore-other-worktrees` to explicitly\n> indicate they want the risky behaviour.\n\n> This matches the current behaviour of `git branch -f` and is safer, for\n> more details see the tests in t2400.\n\nI think it would be helpful to spell out the behavior of\n\n\tgit checkout $current_branch\n\tgit checkout -B $current_branch []\n\tgit checkout -B $current_branch --ignore-other-worktrees []\n\nwhen the current branch is and is not checked out in another worktree \nas the tests are hard to follow because they rely on worktrees set up \nprevious tests.\n\n> Reported-by: Jinwook Jeong \n> Helped-by: Rubén Justo \n> Helped-by: Eric Sunshine \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> @@ -1818,10 +1831,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\tstrbuf_release(&buf);\n> \t}\n> \n> -\tif (opts->patch_mode || opts->pathspec.nr)\n> +\tif (opts->patch_mode || opts->pathspec.nr) {\n> +\t\tfree(check_branch_path);\n> \t\treturn checkout_paths(opts, new_branch_info);\n> +\t}\n> \telse\n> -\t\treturn checkout_branch(opts, new_branch_info);\n> +\t\treturn checkout_branch(opts, new_branch_info, check_branch_path);\n> }\n\nI found the ownership of check_branch_path confusing here. I think it \nwould be clearer to do\n\n\tif (opts->patch_mode || opts->pathspec.nr)\n\t\tret = checkout_path(...);\n\telse\n\t\tret = checkout_branch(...);\n\tfree(check_branch_path);\n\treturn ret;\n\n > [...]\n> +test_expect_success 'but die if using force without --ignore-other-worktrees' '\n\nI'm not sure from the title what this test is checking. Having added \n\"git worktree list\" and run it is checking that when the current branch \nis checked out elsewhere we require --ignore-other-worktrees when \nresetting the current branch.\n\n> +\t(\n> +\t\tcd there &&\n> +\t\ttest_must_fail git checkout -B newmain &&\n> +\t\ttest_must_fail git switch -C newmain &&\n> +\t\tgit checkout --ignore-other-worktrees -B newmain &&\n> +\t\tgit switch --ignore-other-worktrees -C newmaain > \t)\n\nI tried running\n\n\tgit switch -C main newbranch\n\nfrom the main worktree (which is the only worktree that has branch \n'main' checked out) to check that we did not error out when the branch \nis not checked out elsewhere and was surprised it failed with\n\n\tfatal: 'newbranch' is already checked out at '/dev/shm/trash \ndirectory.t2400-worktree-add/short-hand'\n\nIt works as expected without this patch so it looks like there is a bug \nsomewhere.\n\nBest Wishes\n\nPhillip\n"},{"id":"470763","messageId":"xmqqa62dzyst.fsf@gitster.g","threadId":"59095","inReplyTo":"20230119055325.1013-1-carenas@gmail.com","subject":"Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T03:10:10Z","receivedAt":"2023-01-20T03:10:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Merging this on top of 'seen' that used to pass the CI tests\n\n https://github.com/git/git/actions/runs/3963948890\n\nnow fails many tests\n\n https://github.com/git/git/actions/runs/3964312300\n\nFunny thing is that nothing fails locally for me on x86_64 Linux\nwith gcc-12. Anything rings a bell?\n\nThanks.\n"},{"id":"470764","messageId":"CAPUEsph5phu8z=nYJt=zRh7q40-BwKuzhVQ-F=iLQppd9gR5hA@mail.gmail.com","threadId":"59095","inReplyTo":"xmqqa62dzyst.fsf@gitster.g","subject":"Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-20T03:53:31Z","receivedAt":"2023-01-20T03:53:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"I think it only fails its own test, and I can't reproduce locally\neither with macOS and clang.\n\nI was going to propose a newer version but it seems to be similarly affected:\n\n https://github.com/carenas/git/actions/runs/3964409976\n\nIt will be better to drop it for now; will produce one without the\nheisenbug, somehow\n\nCarlo\n"},{"id":"470766","messageId":"CAPUEspikKmUMxCDYY7RC5Krozx3_v56sPN-uTBvjHDjWmvZjig@mail.gmail.com","threadId":"59095","inReplyTo":"CAPUEsph5phu8z=nYJt=zRh7q40-BwKuzhVQ-F=iLQppd9gR5hA@mail.gmail.com","subject":"Re: [PATCH v3] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-20T04:39:41Z","receivedAt":"2023-01-20T04:46:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"I should have known; the test is affected by the bug Rubén is trying to fix in:\n\n https://lore.kernel.org/git/eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com/\n\nWhere the order that the worktrees are found is not deterministic and\nso the ignore_current_worktree flag to die_if_checked_out() might\ntrigger incorrectly.\n\nCarlo\n"},{"id":"470778","messageId":"20230120113553.24655-1-carenas@gmail.com","threadId":"59095","inReplyTo":"20230119055325.1013-1-carenas@gmail.com","subject":"[PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-20T11:35:53Z","receivedAt":"2023-01-20T11:36:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Commands `git switch -C` and `git checkout -B` neglect to check whether\nthe provided branch is already checked out in some other worktree, as\nshown by the following:\n\n $ git worktree list\n .../foo beefb00f [main]\n $ git worktree add ../other\n Preparing worktree (new branch 'other')\n HEAD is now at beefb00f first\n $ cd ../other\n $ git switch -C main\n Switched to and reset branch 'main'\n $ git worktree list\n .../foo beefb00f [main]\n .../other beefb00f [main]\n\nFix this problem by teaching `git switch -C` and `git checkout -B` to\ncheck whether the branch in question is already checked out elsewhere.\n\nUnlike what it is done for `git switch` and `git checkout`, that have\nan historical exception to ignore other worktrees if the branch to\ncheck is the current one (as required when called as part of other\ntools), the logic implemented is more strict and will require the user\nto invoke the command with `--ignore-other-worktrees` to explicitly\nindicate they want the risky behaviour.\n\nThis matches the current behaviour of `git branch -f` and is safer; for\nmore details see the tests in t2400.\n\nReported-by: Jinwook Jeong \nHelped-by: Eric Sunshine \nHelped-by: Rubén Justo \nHelped-by: Phillip Wood \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nChanges since v3\n* Code and Tests improvements as suggested by Phillip\n* Disable unreliable test that triggers a known bug\n\nChanges since v2\n* A leak free implementation\n* More details in commit as suggested by Junio\n\nChanges since v1\n* A much better commit message\n* Changes to the tests as suggested by Eric\n* Changes to the logic as suggested by Rubén\n\n\n builtin/checkout.c | 32 ++++++++++++++++++++++++--------\n t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------\n 2 files changed, 51 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3fa29a08ee..0688652f99 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)\n }\n \n static int checkout_branch(struct checkout_opts *opts,\n-\t\t\t struct branch_info *new_branch_info)\n+\t\t\t struct branch_info *new_branch_info,\n+\t\t\t char *check_branch_path)\n {\n \tif (opts->pathspec.nr)\n \t\tdie(_(\"paths cannot be used with switching branches\"));\n@@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,\n \tif (!opts->can_switch_when_in_progress)\n \t\tdie_if_some_operation_in_progress();\n \n-\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n-\t !opts->ignore_other_worktrees) {\n+\tif (!opts->ignore_other_worktrees && !opts->force_detach &&\n+\t check_branch_path && ref_exists(check_branch_path)) {\n \t\tint flag;\n \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n-\t\tif (head_ref &&\n-\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n-\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n+\t\tif (opts->new_branch_force || (head_ref &&\n+\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))\n+\t\t\tdie_if_checked_out(check_branch_path, 1);\n \t\tfree(head_ref);\n \t}\n \n@@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t const char * const usagestr[],\n \t\t\t struct branch_info *new_branch_info)\n {\n+\tint ret;\n \tint parseopt_flags = 0;\n+\tchar *check_branch_path = NULL;\n \n \topts->overwrite_ignore = 1;\n \topts->prefix = prefix;\n@@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\topts->new_branch = argv0 + 1;\n \t}\n \n+\tif (opts->new_branch && !opts->ignore_other_worktrees) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tstrbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);\n+\t\tstrbuf_splice(&buf, 0, 0, \"refs/heads/\", 11);\n+\t\tcheck_branch_path = strbuf_detach(&buf, NULL);\n+\t}\n \t/*\n \t * Extract branch name from command line arguments, so\n \t * all that is left is pathspecs.\n@@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t\t\t\t\t new_branch_info, opts, &rev);\n \t\targv += n;\n \t\targc -= n;\n+\n+\t\tif (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n+\t\t\tcheck_branch_path = xstrdup(new_branch_info->path);\n \t} else if (!opts->accept_ref && opts->from_treeish) {\n \t\tstruct object_id rev;\n \n@@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \t}\n \n \tif (opts->patch_mode || opts->pathspec.nr)\n-\t\treturn checkout_paths(opts, new_branch_info);\n+\t\tret = checkout_paths(opts, new_branch_info);\n \telse\n-\t\treturn checkout_branch(opts, new_branch_info);\n+\t\tret = checkout_branch(opts, new_branch_info, check_branch_path);\n+\n+\tfree(check_branch_path);\n+\treturn ret;\n }\n \n int cmd_checkout(int argc, const char **argv, const char *prefix)\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex d587e0b20d..7ab7e87440 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -118,14 +118,17 @@ test_expect_success '\"add\" worktree creating new branch' '\n \t)\n '\n \n-test_expect_success 'die the same branch is already checked out' '\n+test_expect_success 'die if the same branch is already checked out' '\n \t(\n \t\tcd here &&\n-\t\ttest_must_fail git checkout newmain\n+\t\ttest_must_fail git checkout newmain &&\n+\t\ttest_must_fail git checkout -B newmain &&\n+\t\ttest_must_fail git switch newmain &&\n+\t\ttest_must_fail git switch -C newmain\n \t)\n '\n \n-test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '\n+test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '\n \thead=$(git -C there rev-parse --git-path HEAD) &&\n \tref=$(git -C there symbolic-ref HEAD) &&\n \trm \"$head\" &&\n@@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin\n \ttest_must_fail git -C here checkout newmain\n '\n \n-test_expect_success 'not die the same branch is already checked out' '\n+test_expect_success 'allow creating multiple worktrees for same branch with force' '\n+\tgit worktree add --force anothernewmain newmain\n+'\n+\n+test_expect_success 'allow checkout/reset from the conflicted branch' '\n \t(\n \t\tcd here &&\n-\t\tgit worktree add --force anothernewmain newmain\n+\t\tgit checkout -b conflictedmain newmain &&\n+\t\tgit checkout -B conflictedmain newmain &&\n+\t\tgit switch -C conflictedmain newmain\n+\t)\n+'\n+\n+test_expect_success 'and not die on re-checking out current branch even if conflicted' '\n+\t(\n+\t\tcd there &&\n+\t\tgit checkout newmain &&\n+\t\tgit switch newmain\n \t)\n '\n \n-test_expect_success 'not die on re-checking out current branch' '\n+test_expect_failure 'unless using force without --ignore-other-worktrees' '\n \t(\n \t\tcd there &&\n-\t\tgit checkout newmain\n+\t\ttest_must_fail git checkout -B newmain &&\n+\t\ttest_must_fail git switch -C newmain &&\n+\t\tgit checkout --ignore-other-worktrees -B newmain &&\n+\t\tgit switch --ignore-other-worktrees -C newmain\n \t)\n '\n \n-- \n2.37.1 (Apple Git-137.1)\n\n"},{"id":"470783","messageId":"8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk","threadId":"59095","inReplyTo":"20230120113553.24655-1-carenas@gmail.com","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-20T15:08:02Z","receivedAt":"2023-01-20T15:08:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:\n> Commands `git switch -C` and `git checkout -B` neglect to check whether\n> the provided branch is already checked out in some other worktree, as\n> shown by the following:\n> \n> $ git worktree list\n> .../foo beefb00f [main]\n> $ git worktree add ../other\n> Preparing worktree (new branch 'other')\n> HEAD is now at beefb00f first\n> $ cd ../other\n> $ git switch -C main\n> Switched to and reset branch 'main'\n> $ git worktree list\n> .../foo beefb00f [main]\n> .../other beefb00f [main]\n> \n> Fix this problem by teaching `git switch -C` and `git checkout -B` to\n> check whether the branch in question is already checked out elsewhere.\n> \n> Unlike what it is done for `git switch` and `git checkout`, that have\n> an historical exception to ignore other worktrees if the branch to\n> check is the current one (as required when called as part of other\n> tools), the logic implemented is more strict and will require the user\n> to invoke the command with `--ignore-other-worktrees` to explicitly\n> indicate they want the risky behaviour.\n> \n> This matches the current behaviour of `git branch -f` and is safer; for\n> more details see the tests in t2400.\n\nAs I said before, it would be much easier for everyone else to \nunderstand the changes if you wrote out what they were rather than \nsaying \"look at the tests\". I do appreciate the improved test \ndescriptions though - thanks for that.\n\n> Reported-by: Jinwook Jeong \n> Helped-by: Eric Sunshine \n> Helped-by: Rubén Justo \n> Helped-by: Phillip Wood \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Changes since v3\n> * Code and Tests improvements as suggested by Phillip\n> * Disable unreliable test that triggers a known bug\n> \n> Changes since v2\n> * A leak free implementation\n> * More details in commit as suggested by Junio\n> \n> Changes since v1\n> * A much better commit message\n> * Changes to the tests as suggested by Eric\n> * Changes to the logic as suggested by Rubén\n> \n> \n> builtin/checkout.c | 32 ++++++++++++++++++++++++--------\n> t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------\n> 2 files changed, 51 insertions(+), 15 deletions(-)\n> \n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 3fa29a08ee..0688652f99 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)\n> }\n> \n> static int checkout_branch(struct checkout_opts *opts,\n> -\t\t\t struct branch_info *new_branch_info)\n> +\t\t\t struct branch_info *new_branch_info,\n> +\t\t\t char *check_branch_path)\n> {\n> \tif (opts->pathspec.nr)\n> \t\tdie(_(\"paths cannot be used with switching branches\"));\n> @@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,\n> \tif (!opts->can_switch_when_in_progress)\n> \t\tdie_if_some_operation_in_progress();\n> \n> -\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> -\t !opts->ignore_other_worktrees) {\n> +\tif (!opts->ignore_other_worktrees && !opts->force_detach &&\n> +\t check_branch_path && ref_exists(check_branch_path)) {\n\nI think check_branch_path is NULL if opts->ignore_other_worktrees is set \nso we could maybe lose \"!opts->ignore_other_worktrees\" here (or possibly \nbelow where you set check_branch_path).\n\n> \t\tint flag;\n> \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n> -\t\tif (head_ref &&\n> -\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n> -\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n> +\t\tif (opts->new_branch_force || (head_ref &&\n> +\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))\n> +\t\t\tdie_if_checked_out(check_branch_path, 1);\n\nI don't think it is worth a re-roll but the reformatting here is \nunfortunate. If you add the new condition at the end it is clearer what \nis being changed.\n\n\t\tif ((head_ref &&\n\t\t (!(flag & REF_IS_YMREF) || strcmp(head_ref, check_branch_path))) ||\n\t\t opts->new_branch_force)\n\npreserves the original code structure so one can see we've added a new \ncondition and done s/new_branch_info->path/check_branch_path/\n\n> \t\tfree(head_ref);\n> \t}\n> \n> @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\t\t const char * const usagestr[],\n> \t\t\t struct branch_info *new_branch_info)\n> {\n> +\tint ret;\n> \tint parseopt_flags = 0;\n> +\tchar *check_branch_path = NULL;\n> \n> \topts->overwrite_ignore = 1;\n> \topts->prefix = prefix;\n> @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\topts->new_branch = argv0 + 1;\n> \t}\n> \n> +\tif (opts->new_branch && !opts->ignore_other_worktrees) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tstrbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);\n> +\t\tstrbuf_splice(&buf, 0, 0, \"refs/heads/\", 11);\n> +\t\tcheck_branch_path = strbuf_detach(&buf, NULL);\n> +\t}\n\nThis block will run whenever -b/-B is given which is good\n\n> \t/*\n> \t * Extract branch name from command line arguments, so\n> \t * all that is left is pathspecs.\n> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\t\t\t\t new_branch_info, opts, &rev);\n> \t\targv += n;\n> \t\targc -= n;\n> +\n> +\t\tif (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n> +\t\t\tcheck_branch_path = xstrdup(new_branch_info->path);\n\nI'm a bit confused what this is doing.\n\n> \t} else if (!opts->accept_ref && opts->from_treeish) {\n> \t\tstruct object_id rev;\n> \n> @@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t}\n> \n> \tif (opts->patch_mode || opts->pathspec.nr)\n> -\t\treturn checkout_paths(opts, new_branch_info);\n> +\t\tret = checkout_paths(opts, new_branch_info);\n> \telse\n> -\t\treturn checkout_branch(opts, new_branch_info);\n> +\t\tret = checkout_branch(opts, new_branch_info, check_branch_path);\n> +\n> +\tfree(check_branch_path);\n> +\treturn ret;\n\nThis is clearer now - thanks\n\n> }\n> \n> int cmd_checkout(int argc, const char **argv, const char *prefix)\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> index d587e0b20d..7ab7e87440 100755\n> --- a/t/t2400-worktree-add.sh\n> +++ b/t/t2400-worktree-add.sh\n> @@ -118,14 +118,17 @@ test_expect_success '\"add\" worktree creating new branch' '\n> \t)\n> '\n> \n> -test_expect_success 'die the same branch is already checked out' '\n> +test_expect_success 'die if the same branch is already checked out' '\n> \t(\n> \t\tcd here &&\n> -\t\ttest_must_fail git checkout newmain\n> +\t\ttest_must_fail git checkout newmain &&\n> +\t\ttest_must_fail git checkout -B newmain &&\n> +\t\ttest_must_fail git switch newmain &&\n> +\t\ttest_must_fail git switch -C newmain\n> \t)\n> '\n> \n> -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '\n> +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '\n> \thead=$(git -C there rev-parse --git-path HEAD) &&\n> \tref=$(git -C there symbolic-ref HEAD) &&\n> \trm \"$head\" &&\n> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin\n> \ttest_must_fail git -C here checkout newmain\n> '\n> \n> -test_expect_success 'not die the same branch is already checked out' '\n> +test_expect_success 'allow creating multiple worktrees for same branch with force' '\n> +\tgit worktree add --force anothernewmain newmain\n> +'\n> +\n> +test_expect_success 'allow checkout/reset from the conflicted branch' '\n\nI'm not sure what \"the conflicted branch\" means (it reminds we of merge \nconflicts). Is this just testing that \"checkout -b/B \n\" works?\n\n> \t(\n> \t\tcd here &&\n> -\t\tgit worktree add --force anothernewmain newmain\n> +\t\tgit checkout -b conflictedmain newmain &&\n> +\t\tgit checkout -B conflictedmain newmain &&\n> +\t\tgit switch -C conflictedmain newmain\n> +\t)\n> +'\n> +\n> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '\n\nI think 'allow re-checking out ...' would be clearer, again I'm not sure \nwhat's conflicted here.\n\n> +\t(\n> +\t\tcd there &&\n> +\t\tgit checkout newmain &&\n> +\t\tgit switch newmain\n> \t)\n> '\n> \n> -test_expect_success 'not die on re-checking out current branch' '\n> +test_expect_failure 'unless using force without --ignore-other-worktrees' '\n\nThis test passes for me - what's the reason for changing from \ntest_expect_success to test_expect_failure?\n\nThanks for working on this\n\nPhillip\n\n> \t(\n> \t\tcd there &&\n> -\t\tgit checkout newmain\n> +\t\ttest_must_fail git checkout -B newmain &&\n> +\t\ttest_must_fail git switch -C newmain &&\n> +\t\tgit checkout --ignore-other-worktrees -B newmain &&\n> +\t\tgit switch --ignore-other-worktrees -C newmain\n> \t)\n> '\n> \n"},{"id":"470823","messageId":"CAPUEspjuXSncRxo5DMj1pA5zcYvn4Y6epdijYL6HJRGhk_6q7g@mail.gmail.com","threadId":"59095","inReplyTo":"8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-20T22:12:21Z","receivedAt":"2023-01-20T22:12:37Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood wrote:\n>\n> On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:\n> > Commands `git switch -C` and `git checkout -B` neglect to check whether\n> > the provided branch is already checked out in some other worktree, as\n> > shown by the following:\n> >\n> > $ git worktree list\n> > .../foo beefb00f [main]\n> > $ git worktree add ../other\n> > Preparing worktree (new branch 'other')\n> > HEAD is now at beefb00f first\n> > $ cd ../other\n> > $ git switch -C main\n> > Switched to and reset branch 'main'\n> > $ git worktree list\n> > .../foo beefb00f [main]\n> > .../other beefb00f [main]\n> >\n> > Fix this problem by teaching `git switch -C` and `git checkout -B` to\n> > check whether the branch in question is already checked out elsewhere.\n> >\n> > Unlike what it is done for `git switch` and `git checkout`, that have\n> > an historical exception to ignore other worktrees if the branch to\n> > check is the current one (as required when called as part of other\n> > tools), the logic implemented is more strict and will require the user\n> > to invoke the command with `--ignore-other-worktrees` to explicitly\n> > indicate they want the risky behaviour.\n> >\n> > This matches the current behaviour of `git branch -f` and is safer; for\n> > more details see the tests in t2400.\n>\n> As I said before, it would be much easier for everyone else to\n> understand the changes if you wrote out what they were rather than\n> saying \"look at the tests\". I do appreciate the improved test\n> descriptions though - thanks for that.\n\nApologies on that, I tried to come up with something that would\ndescribe the change of behaviour other than the paragraph above and\ncouldn't come out with a better explanation except reading the tests\n(which I know is complicated by the fact they are interlinked).\n\nThe behaviour I am changing is also not documented (other than by the\ntest) and might have been added as a quirk to keep the scripted rebase\nand bisect going when confronted with branches that were checked out\nin multiple worktrees, and as such might had not be intended for\n`switch`, and might not be needed anymore either.\n\nUsing`checkout` for simplicity, but also applies to `switch`,\n\n % git worktree list\n .../base 6a45aba [main]\n % git worktree add -f ../other main\n Preparing worktree (checking out 'main')\n HEAD is now at 6a45aba init\n % cd ../other\n % git checkout main\n Already on 'main'\n % git checkout -B main\n fatal: 'main' is already checked out at '.../base'\n % git checkout --ignore-other-worktrees -B main\n Already on 'main'\n\nThe change of behaviour only applies to -B and it actually matches the\ndocumentation better.\n\n> > @@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,\n> > if (!opts->can_switch_when_in_progress)\n> > die_if_some_operation_in_progress();\n> >\n> > - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> > - !opts->ignore_other_worktrees) {\n> > + if (!opts->ignore_other_worktrees && !opts->force_detach &&\n> > + check_branch_path && ref_exists(check_branch_path)) {\n>\n> I think check_branch_path is NULL if opts->ignore_other_worktrees is set\n> so we could maybe lose \"!opts->ignore_other_worktrees\" here (or possibly\n> below where you set check_branch_path).\n\nopts->ignore_other_worktrees was kept from the original expression;\nyou are correct that is not needed anymore, but I thought it didn't\nhurt and made the code intention clearer (meaning it is obvious to\nanyone new to the code that this code will be skipped if that flag is\nset), would using an assert or a comment be a better option?\n\n> > /*\n> > * Extract branch name from command line arguments, so\n> > * all that is left is pathspecs.\n> > @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> > new_branch_info, opts, &rev);\n> > argv += n;\n> > argc -= n;\n> > +\n> > + if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n> > + check_branch_path = xstrdup(new_branch_info->path);\n>\n> I'm a bit confused what this is doing.\n\nThe branch we are interested in might come from 2 places, either it is\na parameter to -b, which was picked up before, or it is the argument\nto the command itself, which was detected above.\n\nIf both are provided, we want to make sure to use the one from -b, or\nwill have the bug you sharply spotted before, which was frankly\nembarrassing.\n\n> > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> > index d587e0b20d..7ab7e87440 100755\n> > @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin\n> > test_must_fail git -C here checkout newmain\n> > '\n> >\n> > -test_expect_success 'not die the same branch is already checked out' '\n> > +test_expect_success 'allow creating multiple worktrees for same branch with force' '\n> > + git worktree add --force anothernewmain newmain\n> > +'\n> > +\n> > +test_expect_success 'allow checkout/reset from the conflicted branch' '\n>\n> I'm not sure what \"the conflicted branch\" means (it reminds we of merge\n> conflicts).\n\nby \"conflicted\" I meant one that is checked out in more than one worktree\n\n> Is this just testing that \"checkout -b/B \n> \" works?\n\nyes, but most importantly that we chose the right branch to check if\nboth are provided and is also a branch\n\n> > (\n> > cd here &&\n> > - git worktree add --force anothernewmain newmain\n> > + git checkout -b conflictedmain newmain &&\n> > + git checkout -B conflictedmain newmain &&\n> > + git switch -C conflictedmain newmain\n> > + )\n> > +'\n> > +\n> > +test_expect_success 'and not die on re-checking out current branch even if conflicted' '\n>\n> I think 'allow re-checking out ...' would be clearer, again I'm not sure\n> what's conflicted here.\n\nok\n\n> > + (\n> > + cd there &&\n> > + git checkout newmain &&\n> > + git switch newmain\n> > )\n> > '\n> >\n> > -test_expect_success 'not die on re-checking out current branch' '\n> > +test_expect_failure 'unless using force without --ignore-other-worktrees' '\n>\n> This test passes for me - what's the reason for changing from\n> test_expect_success to test_expect_failure?\n\nIt also works for me, and for Junio, but somehow it didn't in the last\nruns from the CI and you could reproduce locally by going to the tree\ncreated above in the example I provided and doing:\n\n % cd ../base\n % git checkout -B main\n\nwhich should fail after finding that main is already checked out in\n`other`, but does not because when looking at the worktrees would\nfirst find the current one and not die, never aware there is another\nworktree with that same branch.\n\nthe bug is the same one that Rubén is trying to address for rebase and\nthat you commented on as well and that was mentioned before in this\nthread:\n\n https://lore.kernel.org/git/eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com/\n\nCarlo\n"},{"id":"471087","messageId":"a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk","threadId":"59095","inReplyTo":"CAPUEspjuXSncRxo5DMj1pA5zcYvn4Y6epdijYL6HJRGhk_6q7g@mail.gmail.com","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-27T14:46:45Z","receivedAt":"2023-01-27T14:46:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Carlo\n\nOn 20/01/2023 22:12, Carlo Arenas wrote:\n> On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood wrote:\n>>\n>> On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:\n>>> Commands `git switch -C` and `git checkout -B` neglect to check whether\n>>> the provided branch is already checked out in some other worktree, as\n>>> shown by the following:\n>>>\n>>> $ git worktree list\n>>> .../foo beefb00f [main]\n>>> $ git worktree add ../other\n>>> Preparing worktree (new branch 'other')\n>>> HEAD is now at beefb00f first\n>>> $ cd ../other\n>>> $ git switch -C main\n>>> Switched to and reset branch 'main'\n>>> $ git worktree list\n>>> .../foo beefb00f [main]\n>>> .../other beefb00f [main]\n>>>\n>>> Fix this problem by teaching `git switch -C` and `git checkout -B` to\n>>> check whether the branch in question is already checked out elsewhere.\n>>>\n>>> Unlike what it is done for `git switch` and `git checkout`, that have\n>>> an historical exception to ignore other worktrees if the branch to\n>>> check is the current one (as required when called as part of other\n>>> tools), the logic implemented is more strict and will require the user\n>>> to invoke the command with `--ignore-other-worktrees` to explicitly\n>>> indicate they want the risky behaviour.\n>>>\n>>> This matches the current behaviour of `git branch -f` and is safer; for\n>>> more details see the tests in t2400.\n>>\n>> As I said before, it would be much easier for everyone else to\n>> understand the changes if you wrote out what they were rather than\n>> saying \"look at the tests\". I do appreciate the improved test\n>> descriptions though - thanks for that.\n> \n> Apologies on that, I tried to come up with something that would\n> describe the change of behaviour other than the paragraph above and\n> couldn't come out with a better explanation except reading the tests\n> (which I know is complicated by the fact they are interlinked).\n> \n> The behaviour I am changing is also not documented (other than by the\n> test) and might have been added as a quirk to keep the scripted rebase\n> and bisect going when confronted with branches that were checked out\n> in multiple worktrees, and as such might had not be intended for\n> `switch`, and might not be needed anymore either.\n> \n> Using`checkout` for simplicity, but also applies to `switch`,\n> \n> % git worktree list\n> .../base 6a45aba [main]\n> % git worktree add -f ../other main\n> Preparing worktree (checking out 'main')\n> HEAD is now at 6a45aba init\n> % cd ../other\n> % git checkout main\n> Already on 'main'\n> % git checkout -B main\n> fatal: 'main' is already checked out at '.../base'\n\nThanks for explaining that. If there is no given we don't \nreset the branch so it seems a bit harsh to error out here. For \"git \ncheckout -B \" when is checked out in \nanother worktree requiring --ignore-other-worktrees makes sense.\n\n> % git checkout --ignore-other-worktrees -B main\n> Already on 'main'\n> \n> The change of behaviour only applies to -B and it actually matches the\n> documentation better.\n> \n>>> @@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,\n>>> if (!opts->can_switch_when_in_progress)\n>>> die_if_some_operation_in_progress();\n>>>\n>>> - if (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n>>> - !opts->ignore_other_worktrees) {\n>>> + if (!opts->ignore_other_worktrees && !opts->force_detach &&\n>>> + check_branch_path && ref_exists(check_branch_path)) {\n>>\n>> I think check_branch_path is NULL if opts->ignore_other_worktrees is set\n>> so we could maybe lose \"!opts->ignore_other_worktrees\" here (or possibly\n>> below where you set check_branch_path).\n> \n> opts->ignore_other_worktrees was kept from the original expression;\n> you are correct that is not needed anymore, but I thought it didn't\n> hurt and made the code intention clearer (meaning it is obvious to\n> anyone new to the code that this code will be skipped if that flag is\n> set), would using an assert or a comment be a better option?\n\nIt's a good point that it makes the intention clearer, maybe we should \njust leave it as it is.\n\n>>> /*\n>>> * Extract branch name from command line arguments, so\n>>> * all that is left is pathspecs.\n>>> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n>>> new_branch_info, opts, &rev);\n>>> argv += n;\n>>> argc -= n;\n>>> +\n>>> + if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n>>> + check_branch_path = xstrdup(new_branch_info->path);\n>>\n>> I'm a bit confused what this is doing.\n> \n> The branch we are interested in might come from 2 places, either it is\n> a parameter to -b, which was picked up before, or it is the argument\n> to the command itself, which was detected above.\n\nOh, of course. I was so focused on the -b that I'd forgotten we need the \nsame check when we're checking out an existing branch - thanks for \nputting me right.\n\n> If both are provided, we want to make sure to use the one from -b, or\n> will have the bug you sharply spotted before, which was frankly\n> embarrassing.\n> \n>>> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n>>> index d587e0b20d..7ab7e87440 100755\n>>> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin\n>>> test_must_fail git -C here checkout newmain\n>>> '\n>>>\n>>> -test_expect_success 'not die the same branch is already checked out' '\n>>> +test_expect_success 'allow creating multiple worktrees for same branch with force' '\n>>> + git worktree add --force anothernewmain newmain\n>>> +'\n>>> +\n>>> +test_expect_success 'allow checkout/reset from the conflicted branch' '\n>>\n>> I'm not sure what \"the conflicted branch\" means (it reminds we of merge\n>> conflicts).\n> \n> by \"conflicted\" I meant one that is checked out in more than one worktree\n\nI think it would be clearer so say that rather than \"conflicted\" which \nhas a strong association with merge conflicts.\n\nBest Wishes\n\nPhillip\n"},{"id":"473974","messageId":"xmqq355wbaar.fsf@gitster.g","threadId":"59095","inReplyTo":"8f24fc3c-c30f-dc70-5a94-5ee4ed3de102@dunelm.org.uk","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T00:06:52Z","receivedAt":"2023-03-23T00:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Carlo\n> ...\n> As I said before, it would be much easier for everyone else to\n> understand the changes if you wrote out what they were rather than\n> saying \"look at the tests\"....\n> ...\n>> +\tif (!opts->ignore_other_worktrees && !opts->force_detach &&\n>> +\t check_branch_path && ref_exists(check_branch_path)) {\n>\n> I think check_branch_path is NULL if opts->ignore_other_worktrees is\n> set so we could maybe lose \"!opts->ignore_other_worktrees\" here (or\n> possibly below where you set check_branch_path).\n> ...\n>> ...\n>> +\t\tif (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n>> +\t\t\tcheck_branch_path = xstrdup(new_branch_info->path);\n>\n> I'm a bit confused what this is doing.\n> ...\n>> +test_expect_success 'allow checkout/reset from the conflicted branch' '\n>\n> I'm not sure what \"the conflicted branch\" means (it reminds we of\n> merge conflicts). Is this just testing that \"checkout -b/B \n> \" works?\n> ...\n>> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '\n>\n> I think 'allow re-checking out ...' would be clearer, again I'm not\n> sure what's conflicted here.\n> ...\n>> -test_expect_success 'not die on re-checking out current branch' '\n>> +test_expect_failure 'unless using force without --ignore-other-worktrees' '\n>\n> This test passes for me - what's the reason for changing from\n> test_expect_success to test_expect_failure?\n>\n> Thanks for working on this\n\nThis topic has been dormant for a full two months. Aside from\ncomments by Phillip (quoted above), are there remaining things\nto be done and issues to be resolved before we can see v5?\n\nThanks.\n"},{"id":"474071","messageId":"CAPUEspj_Bh+LgYLnWfeBdcq_uV5Cbou-7H51GLFjzSa5Qzby9w@mail.gmail.com","threadId":"59095","inReplyTo":"xmqq355wbaar.fsf@gitster.g","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-03-24T03:49:55Z","receivedAt":"2023-03-24T03:50:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Mar 22, 2023 at 5:06 PM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > Hi Carlo\n> > ...\n> > As I said before, it would be much easier for everyone else to\n> > understand the changes if you wrote out what they were rather than\n> > saying \"look at the tests\"....\n> > ...\n> >> + if (!opts->ignore_other_worktrees && !opts->force_detach &&\n> >> + check_branch_path && ref_exists(check_branch_path)) {\n> >\n> > I think check_branch_path is NULL if opts->ignore_other_worktrees is\n> > set so we could maybe lose \"!opts->ignore_other_worktrees\" here (or\n> > possibly below where you set check_branch_path).\n> > ...\n> >> ...\n> >> + if (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n> >> + check_branch_path = xstrdup(new_branch_info->path);\n> >\n> > I'm a bit confused what this is doing.\n> > ...\n> >> +test_expect_success 'allow checkout/reset from the conflicted branch' '\n> >\n> > I'm not sure what \"the conflicted branch\" means (it reminds we of\n> > merge conflicts). Is this just testing that \"checkout -b/B \n> > \" works?\n> > ...\n> >> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '\n> >\n> > I think 'allow re-checking out ...' would be clearer, again I'm not\n> > sure what's conflicted here.\n> > ...\n> >> -test_expect_success 'not die on re-checking out current branch' '\n> >> +test_expect_failure 'unless using force without --ignore-other-worktrees' '\n> >\n> > This test passes for me - what's the reason for changing from\n> > test_expect_success to test_expect_failure?\n> >\n> > Thanks for working on this\n>\n> are there remaining things\n> to be done and issues to be resolved before we can see v5?\n\nit interacted with another branch\n(rj/avoid-switching-to-already-used-branch) that was just recently\nmerged to master.\n\nit also was probably too aggressive as pointed[1] by Phillip after my\nexplanation of the change of behaviour as quoted:\n\n>> Using`checkout` for simplicity, but also applies to `switch`,\n>>\n>> % git worktree list\n>> .../base 6a45aba [main]\n>> % git worktree add -f ../other main\n>> Preparing worktree (checking out 'main')\n>> HEAD is now at 6a45aba init\n>> % cd ../other\n>> % git checkout main\n>> Already on 'main'\n>> % git checkout -B main\n>> fatal: 'main' is already checked out at '.../base'\n>\n> Thanks for explaining that. If there is no given we don't\n> reset the branch so it seems a bit harsh to error out here. For \"git\n> checkout -B \" when is checked out in\n> another worktree requiring --ignore-other-worktrees makes sense.\n\nI wasn't sure on how to proceed from there, but will come with a v5 to\ndiscuss further.\n\nCarlo\n\n[1] https://lore.kernel.org/git/a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk/\n"},{"id":"477267","messageId":"31ab5d7c-d310-1659-5d56-2fd341c44275@gmail.com","threadId":"59095","inReplyTo":"a848b7d5-fd40-b043-7ed9-1672f65312e6@dunelm.org.uk","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-05-14T20:21:21Z","receivedAt":"2023-05-14T20:21:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 27-ene-2023 14:46:45, Phillip Wood wrote:\n> Hi Carlo\n> \n> On 20/01/2023 22:12, Carlo Arenas wrote:\n> > On Fri, Jan 20, 2023 at 7:08 AM Phillip Wood wrote:\n> > > \n> > > On 20/01/2023 11:35, Carlo Marcelo Arenas Belón wrote:\n> > > > Commands `git switch -C` and `git checkout -B` neglect to check whether\n> > > > the provided branch is already checked out in some other worktree, as\n> > > > shown by the following:\n> > > > \n> > > > $ git worktree list\n> > > > .../foo beefb00f [main]\n> > > > $ git worktree add ../other\n> > > > Preparing worktree (new branch 'other')\n> > > > HEAD is now at beefb00f first\n> > > > $ cd ../other\n> > > > $ git switch -C main\n> > > > Switched to and reset branch 'main'\n> > > > $ git worktree list\n> > > > .../foo beefb00f [main]\n> > > > .../other beefb00f [main]\n> > > > \n> > > > Fix this problem by teaching `git switch -C` and `git checkout -B` to\n> > > > check whether the branch in question is already checked out elsewhere.\n> > > > \n> > > > Unlike what it is done for `git switch` and `git checkout`, that have\n> > > > an historical exception to ignore other worktrees if the branch to\n> > > > check is the current one (as required when called as part of other\n> > > > tools), the logic implemented is more strict and will require the user\n> > > > to invoke the command with `--ignore-other-worktrees` to explicitly\n> > > > indicate they want the risky behaviour.\n> > > > \n> > > > This matches the current behaviour of `git branch -f` and is safer; for\n> > > > more details see the tests in t2400.\n> > > \n> > > As I said before, it would be much easier for everyone else to\n> > > understand the changes if you wrote out what they were rather than\n> > > saying \"look at the tests\". I do appreciate the improved test\n> > > descriptions though - thanks for that.\n> > \n> > Apologies on that, I tried to come up with something that would\n> > describe the change of behaviour other than the paragraph above and\n> > couldn't come out with a better explanation except reading the tests\n> > (which I know is complicated by the fact they are interlinked).\n> > \n> > The behaviour I am changing is also not documented (other than by the\n> > test) and might have been added as a quirk to keep the scripted rebase\n> > and bisect going when confronted with branches that were checked out\n> > in multiple worktrees, and as such might had not be intended for\n> > `switch`, and might not be needed anymore either.\n> > \n> > Using`checkout` for simplicity, but also applies to `switch`,\n> > \n> > % git worktree list\n> > .../base 6a45aba [main]\n> > % git worktree add -f ../other main\n> > Preparing worktree (checking out 'main')\n> > HEAD is now at 6a45aba init\n> > % cd ../other\n> > % git checkout main\n> > Already on 'main'\n> > % git checkout -B main\n> > fatal: 'main' is already checked out at '.../base'\n> \n> Thanks for explaining that. If there is no given we don't\n> reset the branch so it seems a bit harsh to error out here.\n\nWe say in the documentation:\n\n +\n If `-B` is given, `` is created if it doesn't exist; otherwise, it\n is reset. This is the transactional equivalent of\n +\n ------------\n $ git branch -f []\n $ git checkout \n ------------\n +\n\nand, since 55c4a67307 (Prevent force-updating of the current branch,\n2011-08-20), we return with error on \"git branch -f [...]\".\n\nTherefore, when the current branch is checked out in multiple worktrees, it\nseems consequent to error on \"git checkout -B [...]\".\n\nBut we want to allow \"git checkout -B \", without ,\nas a way to say \"git reset --keep\", see: 39bd6f7261 (Allow checkout -B\n to update the current branch, 2011-11-26).\n\nYour suggestion not to error is not only reasonable, but also seems desirable.\n\nThanks.\n"},{"id":"477268","messageId":"6dfbdc94-6a5c-c99a-3435-08f86f0e1fef@gmail.com","threadId":"59095","inReplyTo":"20230120113553.24655-1-carenas@gmail.com","subject":"Re: [PATCH v4] checkout/switch: disallow checking out same branch in multiple worktrees","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-05-14T20:24:33Z","receivedAt":"2023-05-14T20:24:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 20/1/23 12:35, Carlo Marcelo Arenas Belón wrote:\n> Commands `git switch -C` and `git checkout -B` neglect to check whether\n> the provided branch is already checked out in some other worktree, as\n> shown by the following:\n>\n> $ git worktree list\n> .../foo beefb00f [main]\n> $ git worktree add ../other\n> Preparing worktree (new branch 'other')\n> HEAD is now at beefb00f first\n> $ cd ../other\n> $ git switch -C main\n> Switched to and reset branch 'main'\n> $ git worktree list\n> .../foo beefb00f [main]\n> .../other beefb00f [main]\n>\n> Fix this problem by teaching `git switch -C` and `git checkout -B` to\n> check whether the branch in question is already checked out elsewhere.\n>\n> Unlike what it is done for `git switch` and `git checkout`, that have\n> an historical exception to ignore other worktrees if the branch to\n> check is the current one (as required when called as part of other\n> tools), the logic implemented is more strict and will require the user\n> to invoke the command with `--ignore-other-worktrees` to explicitly\n> indicate they want the risky behaviour.\n>\n> This matches the current behaviour of `git branch -f` and is safer; for\n> more details see the tests in t2400.\n>\n> Reported-by: Jinwook Jeong \n> Helped-by: Eric Sunshine \n> Helped-by: Rubén Justo \n> Helped-by: Phillip Wood \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Changes since v3\n> * Code and Tests improvements as suggested by Phillip\n> * Disable unreliable test that triggers a known bug\n>\n> Changes since v2\n> * A leak free implementation\n> * More details in commit as suggested by Junio\n>\n> Changes since v1\n> * A much better commit message\n> * Changes to the tests as suggested by Eric\n> * Changes to the logic as suggested by Rubén\n>\n>\n> builtin/checkout.c | 32 ++++++++++++++++++++++++--------\n> t/t2400-worktree-add.sh | 34 +++++++++++++++++++++++++++-------\n> 2 files changed, 51 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 3fa29a08ee..0688652f99 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1474,7 +1474,8 @@ static void die_if_some_operation_in_progress(void)\n> }\n> \n> static int checkout_branch(struct checkout_opts *opts,\n> -\t\t\t struct branch_info *new_branch_info)\n> +\t\t\t struct branch_info *new_branch_info,\n> +\t\t\t char *check_branch_path)\n> {\n> \tif (opts->pathspec.nr)\n> \t\tdie(_(\"paths cannot be used with switching branches\"));\n> @@ -1533,13 +1534,13 @@ static int checkout_branch(struct checkout_opts *opts,\n> \tif (!opts->can_switch_when_in_progress)\n> \t\tdie_if_some_operation_in_progress();\n> \n> -\tif (new_branch_info->path && !opts->force_detach && !opts->new_branch &&\n> -\t !opts->ignore_other_worktrees) {\n> +\tif (!opts->ignore_other_worktrees && !opts->force_detach &&\n> +\t check_branch_path && ref_exists(check_branch_path)) {\n> \t\tint flag;\n> \t\tchar *head_ref = resolve_refdup(\"HEAD\", 0, NULL, &flag);\n> -\t\tif (head_ref &&\n> -\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, new_branch_info->path)))\n> -\t\t\tdie_if_checked_out(new_branch_info->path, 1);\n> +\t\tif (opts->new_branch_force || (head_ref &&\n> +\t\t (!(flag & REF_ISSYMREF) || strcmp(head_ref, check_branch_path))))\n> +\t\t\tdie_if_checked_out(check_branch_path, 1);\n> \t\tfree(head_ref);\n> \t}\n> \n> @@ -1627,7 +1628,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\t\t const char * const usagestr[],\n> \t\t\t struct branch_info *new_branch_info)\n> {\n> +\tint ret;\n> \tint parseopt_flags = 0;\n> +\tchar *check_branch_path = NULL;\n> \n> \topts->overwrite_ignore = 1;\n> \topts->prefix = prefix;\n> @@ -1717,6 +1720,13 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\topts->new_branch = argv0 + 1;\n> \t}\n> \n> +\tif (opts->new_branch && !opts->ignore_other_worktrees) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\t\tstrbuf_branchname(&buf, opts->new_branch, INTERPRET_BRANCH_LOCAL);\n> +\t\tstrbuf_splice(&buf, 0, 0, \"refs/heads/\", 11);\n> +\t\tcheck_branch_path = strbuf_detach(&buf, NULL);\n> +\t}\n> \t/*\n> \t * Extract branch name from command line arguments, so\n> \t * all that is left is pathspecs.\n> @@ -1741,6 +1751,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t\t\t\t\t new_branch_info, opts, &rev);\n> \t\targv += n;\n> \t\targc -= n;\n> +\n> +\t\tif (!opts->ignore_other_worktrees && !check_branch_path && new_branch_info->path)\n> +\t\t\tcheck_branch_path = xstrdup(new_branch_info->path);\n> \t} else if (!opts->accept_ref && opts->from_treeish) {\n> \t\tstruct object_id rev;\n> \n> @@ -1817,9 +1830,12 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n> \t}\n> \n> \tif (opts->patch_mode || opts->pathspec.nr)\n> -\t\treturn checkout_paths(opts, new_branch_info);\n> +\t\tret = checkout_paths(opts, new_branch_info);\n> \telse\n> -\t\treturn checkout_branch(opts, new_branch_info);\n> +\t\tret = checkout_branch(opts, new_branch_info, check_branch_path);\n> +\n> +\tfree(check_branch_path);\n> +\treturn ret;\n> }\n> \n> int cmd_checkout(int argc, const char **argv, const char *prefix)\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> index d587e0b20d..7ab7e87440 100755\n> --- a/t/t2400-worktree-add.sh\n> +++ b/t/t2400-worktree-add.sh\n> @@ -118,14 +118,17 @@ test_expect_success '\"add\" worktree creating new branch' '\n> \t)\n> '\n> \n> -test_expect_success 'die the same branch is already checked out' '\n> +test_expect_success 'die if the same branch is already checked out' '\n> \t(\n> \t\tcd here &&\n> -\t\ttest_must_fail git checkout newmain\n> +\t\ttest_must_fail git checkout newmain &&\n> +\t\ttest_must_fail git checkout -B newmain &&\n> +\t\ttest_must_fail git switch newmain &&\n> +\t\ttest_must_fail git switch -C newmain\n> \t)\n> '\n> \n> -test_expect_success SYMLINKS 'die the same branch is already checked out (symlink)' '\n> +test_expect_success SYMLINKS 'die if the same branch is already checked out (symlink)' '\n> \thead=$(git -C there rev-parse --git-path HEAD) &&\n> \tref=$(git -C there symbolic-ref HEAD) &&\n> \trm \"$head\" &&\n> @@ -133,17 +136,34 @@ test_expect_success SYMLINKS 'die the same branch is already checked out (symlin\n> \ttest_must_fail git -C here checkout newmain\n> '\n> \n> -test_expect_success 'not die the same branch is already checked out' '\n> +test_expect_success 'allow creating multiple worktrees for same branch with force' '\n> +\tgit worktree add --force anothernewmain newmain\n> +'\n> +\n> +test_expect_success 'allow checkout/reset from the conflicted branch' '\n> \t(\n> \t\tcd here &&\n> -\t\tgit worktree add --force anothernewmain newmain\n> +\t\tgit checkout -b conflictedmain newmain &&\n> +\t\tgit checkout -B conflictedmain newmain &&\n> +\t\tgit switch -C conflictedmain newmain\n> +\t)\n> +'\n> +\n> +test_expect_success 'and not die on re-checking out current branch even if conflicted' '\n> +\t(\n> +\t\tcd there &&\n> +\t\tgit checkout newmain &&\n> +\t\tgit switch newmain\n> \t)\n> '\n> \n> -test_expect_success 'not die on re-checking out current branch' '\n> +test_expect_failure 'unless using force without --ignore-other-worktrees' '\n\nThe fix to what caused this test to occasionally fail is already in\n'master'. This test should now succeed consistently.\n\n> \t(\n> \t\tcd there &&\n> -\t\tgit checkout newmain\n> +\t\ttest_must_fail git checkout -B newmain &&\n> +\t\ttest_must_fail git switch -C newmain &&\n> +\t\tgit checkout --ignore-other-worktrees -B newmain &&\n> +\t\tgit switch --ignore-other-worktrees -C newmain\n> \t)\n> '\n> \n>\n\nThanks for continuing to work on this.\n"}]}