{"thread":{"id":"59097","subject":"[PATCH] worktree: teach find_shared_symref to ignore current worktree","startedAt":"2023-01-17T00:36:49Z","lastAt":"2023-02-27T00:00:42Z","messageCount":40,"participants":["Rubén Justo","Junio C Hamano","Phillip Wood","Ævar Arnfjörð Bjarmason","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470459","messageId":"eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com","threadId":"59097","inReplyTo":null,"subject":"[PATCH] worktree: teach find_shared_symref to ignore current worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-17T00:36:41Z","receivedAt":"2023-01-17T00:36:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"We prevent some operations from being executed on a branch checked out\nin a worktree other than the current one.  An example of this was\nintroduced in b5cabb4 (rebase: refuse to switch to branch already\nchecked out elsewhere, 2020-02-23).\n\n\"find_shared_symref()\" is sometimes used to find the worktree in which a\nbranch is checked out.  It performs its search starting with the current\nworktree.\n\nAs we allow to have the same branch checked out in multiple worktrees\nsimultaneously...\n\n\t$ git worktree add foo\n\t$ git worktree add -f bar foo\n\t$ git checkout --ignore-other-worktrees foo\n\n... if the branch checked out in the current worktree is also checked\nout in another worktree, with \"find_shared_symref()\" we will not notice\nthis \"other\" working tree.\n\nLet's teach \"find_shared_symref()\" to ignore the current worktree in the\nsearch, based on the caller's needs.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n branch.c               | 4 ++--\n builtin/notes.c        | 2 +-\n builtin/receive-pack.c | 2 +-\n t/t3400-rebase.sh      | 3 +++\n worktree.c             | 6 +++++-\n worktree.h             | 3 ++-\n 6 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d182756827..2508e94add 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -822,8 +822,8 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n \tstruct worktree **worktrees = get_worktrees();\n \tconst struct worktree *wt;\n \n-\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n-\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n+\twt = find_shared_symref(worktrees, \"HEAD\", branch, ignore_current_worktree);\n+\tif (wt) {\n \t\tskip_prefix(branch, \"refs/heads/\", &branch);\n \t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n \t}\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 80d9dfd25c..80326bdaab 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -869,7 +869,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\t/* Store ref-to-be-updated into .git/NOTES_MERGE_REF */\n \t\tworktrees = get_worktrees();\n \t\twt = find_shared_symref(worktrees, \"NOTES_MERGE_REF\",\n-\t\t\t\t\tdefault_notes_ref());\n+\t\t\t\t\tdefault_notes_ref(), 0);\n \t\tif (wt)\n \t\t\tdie(_(\"a notes merge into %s is already in-progress at %s\"),\n \t\t\t    default_notes_ref(), wt->path);\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a90af30363..18d400101c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1460,7 +1460,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)\n \tint do_update_worktree = 0;\n \tstruct worktree **worktrees = get_worktrees();\n \tconst struct worktree *worktree =\n-\t\tfind_shared_symref(worktrees, \"HEAD\", name);\n+\t\tfind_shared_symref(worktrees, \"HEAD\", name, 0);\n \n \t/* only refs/... are allowed */\n \tif (!starts_with(name, \"refs/\") || check_refname_format(name + 5, 0)) {\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex d5a8ee39fc..874cfff8fe 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -407,6 +407,9 @@ test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n \ttest_must_fail git -C wt rebase main main 2>err &&\n+\ttest_i18ngrep \"already checked out\" err &&\n+\tgit worktree add --force wt2 main &&\n+\ttest_must_fail git rebase main main &&\n \ttest_i18ngrep \"already checked out\" err\n '\n \ndiff --git a/worktree.c b/worktree.c\nindex aa43c64119..3d686137ef 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -405,7 +405,8 @@ int is_worktree_being_bisected(const struct worktree *wt,\n  */\n const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n-\t\t\t\t\t  const char *target)\n+\t\t\t\t\t  const char *target,\n+\t\t\t\t\t  int ignore_current_worktree)\n {\n \tconst struct worktree *existing = NULL;\n \tint i = 0;\n@@ -416,6 +417,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\tstruct ref_store *refs;\n \t\tint flags;\n \n+\t\tif (wt->is_current && ignore_current_worktree)\n+\t\t\tcontinue;\n+\n \t\tif (wt->is_bare)\n \t\t\tcontinue;\n \ndiff --git a/worktree.h b/worktree.h\nindex 9dcea6fc8c..a9f35ee990 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -147,7 +147,8 @@ void free_worktrees(struct worktree **);\n  */\n const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n-\t\t\t\t\t  const char *target);\n+\t\t\t\t\t  const char *target,\n+\t\t\t\t\t  int ignore_current_worktree);\n \n /*\n  * Similar to head_ref() for all HEADs _except_ one from the current\n-- \n2.39.0\n"},{"id":"470565","messageId":"xmqqilh491y6.fsf@gitster.g","threadId":"59097","inReplyTo":"eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com","subject":"Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-17T23:27:29Z","receivedAt":"2023-01-18T00:11:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> We prevent some operations from being executed on a branch checked out\n> in a worktree other than the current one.  An example of this was\n> introduced in b5cabb4 (rebase: refuse to switch to branch already\n> checked out elsewhere, 2020-02-23).\n>\n> \"find_shared_symref()\" is sometimes used to find the worktree in which a\n> branch is checked out.  It performs its search starting with the current\n> worktree.\n\n\"starting with the current\" may be a correct statement of the fact,\nbut it is totally unclear what the relevance it has to the problem\nbeing solved. Rather, it is unclear what problem you are solving.\n\nIs it \n\n - We search through the worktrees, starting with the current one,\n   and stop at the first one found.\n\n - If the current branch the the current worktree is checked out in\n   a different worktree, we get the current worktree back.\n\n - There are callers that want to know ONLY about other worktrees;\n   they check the returned value and when they see it is the current\n   one, they happily ignores the fact that it might be checked out\n   elsewhere as well.\n\n> As we allow to have the same branch checked out in multiple worktrees\n> simultaneously...\n>\n> \t$ git worktree add foo\n> \t$ git worktree add -f bar foo\n> \t$ git checkout --ignore-other-worktrees foo\n>\n> ... if the branch checked out in the current worktree is also checked\n> out in another worktree, with \"find_shared_symref()\" we will not notice\n> this \"other\" working tree.\n\nIt is somewhat disturbing that your solution only needs to \"ignore\"\nthe current one.  Whatever problem you are seeing by the current\ncode not ignoring the current worktree, wouldn't we have a similar\nproblem if two non-current worktrees checked out the same branch?\nWould it not be a problem because any non-current worktree returned\nby the function triggers the \"already checked-out\" safety mechanism?\n\n"},{"id":"470685","messageId":"f209958f-e824-181b-f9d5-6e4bc8e53646@gmail.com","threadId":"59097","inReplyTo":"xmqqilh491y6.fsf@gitster.g","subject":"Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-18T23:50:34Z","receivedAt":"2023-01-18T23:51:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 17-ene-2023 15:27:29, Junio C Hamano wrote:\n\n> being solved. Rather, it is unclear what problem you are solving.\n\nSorry, I didn't explain the motivation well in the message.\n\nFirst, I'm not sure if \"ignore_current_worktree\" is correct, maybe needs\nto be \"prefer_current_worktree\".  Having said that, let me use an example.\n\nWith...\n\n\t$ git worktree add wt1 -b main\n\t$ git worktree add wt2 -f main\n\n... we get confuse results:\n\n\t$ git -C wt1 rebase main main\n\tCurrent branch main is up to date.\n\t$ git -C wt2 rebase main main\n\tfatal: 'main' is already checked out...\n\nThe problem I'm trying to solve is that find_shared_symref() returns the\nfirst matching worktree, and a possible matching \"current worktree\"\nmight not be the first matching one.  That's why die_if_checked_out()\ndies with \"git -C wt2\".\n\nfind_shared_symref() searches through the list of worktrees that\nget_worktrees() composes: first the main worktree and then, as getdir()\nreturns them, those in .git/worktrees/*.  The search is sequential and\nonce a match is found, it is returned.  And so die_if_checked_out(),\nwhen asked to \"ignore_current_worktree\", is going to consider for\n\"is_current\" the worktree which may or may not be the \"current\" one,\ndepending on the sequence from get_worktree(), and getdir() ultimately.\n\nIf we want to disallow operations on a worktree with a checked out\nbranch also on another worktree, we need the \"ignore_current_worktree\".\nBut, and now I'm more in favor of this, if we prefer to allow the\noperation, we need a \"prefer_current_worktree\", to induce\nfind_shared_symref() to return the \"current\" one if it matches, or any\nother one that matches.\n"},{"id":"470707","messageId":"cedc93ec-aa6f-65bf-65be-0dca3d4d0186@dunelm.org.uk","threadId":"59097","inReplyTo":"f209958f-e824-181b-f9d5-6e4bc8e53646@gmail.com","subject":"Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-19T10:48:27Z","receivedAt":"2023-01-19T10:49:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 18/01/2023 23:50, Rubén Justo wrote:\n> On 17-ene-2023 15:27:29, Junio C Hamano wrote:\n> \n>> being solved. Rather, it is unclear what problem you are solving.\n> \n> Sorry, I didn't explain the motivation well in the message.\n\nI'm afraid I'm still not totally clear from this message exactly what \nthe problem is. Having looked at die_if_checked_out() I think it maybe \nthe second of Junio's options\n\n  - If the current branch the the current worktree is checked out in\n    a different worktree, we get the current worktree back.\n\nHere is the function:\n\nvoid die_if_checked_out(const char *branch, int ignore_current_worktree)\n{\n\tstruct worktree **worktrees = get_worktrees();\n\tconst struct worktree *wt;\n\n\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n\t}\n\n\tfree_worktrees(worktrees);\n}\n\n\nIt takes a flag to ignore the current worktree but uses \nfind_shared_symref() which does not have such a flag to see if the \nbranch is checked out. This means that if a branch is checkout out twice \nfind_shared_symref() may return the current worktree rather than the one \nwe're interested in.\n\nIf that is the problem you're trying to solve I think it would be better \nto keep the signature of find_shared_symref() the same and add a helper \nfunction that is called by die_if_checked_out() and find_shared_symref() \nwhich can optionally ignore the current worktree. The commit message for \nsuch a patch should explain you're fixing a bug rather than trying to \nchange the existing behavior.\n\nBest Wishes\n\nPhillip\n\n\n> First, I'm not sure if \"ignore_current_worktree\" is correct, maybe needs\n> to be \"prefer_current_worktree\".  Having said that, let me use an example.\n> \n> With...\n> \n> \t$ git worktree add wt1 -b main\n> \t$ git worktree add wt2 -f main\n> \n> ... we get confuse results:\n> \n> \t$ git -C wt1 rebase main main\n> \tCurrent branch main is up to date.\n> \t$ git -C wt2 rebase main main\n> \tfatal: 'main' is already checked out...\n> \n> The problem I'm trying to solve is that find_shared_symref() returns the\n> first matching worktree, and a possible matching \"current worktree\"\n> might not be the first matching one.  That's why die_if_checked_out()\n> dies with \"git -C wt2\".\n> \n> find_shared_symref() searches through the list of worktrees that\n> get_worktrees() composes: first the main worktree and then, as getdir()\n> returns them, those in .git/worktrees/*.  The search is sequential and\n> once a match is found, it is returned.  And so die_if_checked_out(),\n> when asked to \"ignore_current_worktree\", is going to consider for\n> \"is_current\" the worktree which may or may not be the \"current\" one,\n> depending on the sequence from get_worktree(), and getdir() ultimately.\n> \n> If we want to disallow operations on a worktree with a checked out\n> branch also on another worktree, we need the \"ignore_current_worktree\".\n> But, and now I'm more in favor of this, if we prefer to allow the\n> operation, we need a \"prefer_current_worktree\", to induce\n> find_shared_symref() to return the \"current\" one if it matches, or any\n> other one that matches.\n"},{"id":"470751","messageId":"71a7b8d3-3c53-3407-56f0-b805f7cccc1f@gmail.com","threadId":"59097","inReplyTo":"cedc93ec-aa6f-65bf-65be-0dca3d4d0186@dunelm.org.uk","subject":"Re: [PATCH] worktree: teach find_shared_symref to ignore current worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-19T23:18:23Z","receivedAt":"2023-01-19T23:20:11Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 19-ene-2023 10:48:27, Phillip Wood wrote:\n\n> I'm afraid I'm still not totally clear from this message exactly what the\n> problem is. Having looked at die_if_checked_out() I think it maybe the\n\nI'm going to reroll, I hope it makes sense then.\n\n> If that is the problem you're trying to solve I think it would be better to\n> keep the signature of find_shared_symref() the same and add a helper\n> function that is called by die_if_checked_out() and find_shared_symref()\n\nOK.\n\n> which can optionally ignore the current worktree. The commit message for\n> such a patch should explain you're fixing a bug rather than trying to change\n> the existing behavior.\n> \n\nYes, the message is not clear that the commit is fixing a bug.  I'll add\nthat.\n\nThank you. \n"},{"id":"470851","messageId":"f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com","threadId":"59097","inReplyTo":"eeb0c778-af0a-235c-f009-bca3abafdb15@gmail.com","subject":"[PATCH v2 0/3] fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-22T01:20:41Z","receivedAt":"2023-01-22T01:20:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Changes:\n- The message explains the change as a bug fix.\n- Introduced a new helper is_shared_symref() to be used from\n  find_shared_symref() and die_if_checked_out().\n- Tests for rebase and switch.\n\n\nRubén Justo (3):\n  branch: fix die_if_checked_out() when ignore_current_worktree\n  rebase: refuse to switch to a branch already checked out elsewhere (test)\n  switch: reject if the branch is already checked out elsewhere (test)\n\n branch.c          | 16 +++++++++-----\n t/t2060-switch.sh | 29 +++++++++++++++++++++++++\n t/t3400-rebase.sh | 14 ++++++++++++\n worktree.c        | 54 +++++++++++++++++++++++++----------------------\n worktree.h        |  6 ++++++\n 5 files changed, 89 insertions(+), 30 deletions(-)\n\n-- \n2.39.0\n"},{"id":"470852","messageId":"17f267b1-7f5e-2fb6-fb14-1c37ec355e65@gmail.com","threadId":"59097","inReplyTo":"f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com","subject":"[PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-22T01:23:53Z","receivedAt":"2023-01-22T01:23:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 8d9fdd7 (worktree.c: check whether branch is rebased in another\nworktree, 2016-04-22) die_if_checked_out() learned a new\noption ignore_current_worktree, to modify the operation from \"die() if\nthe branch is checked out in any worktree\" to \"die() if the branch\nis checked out in any worktree other than the current one\".\n\nUnfortunately we implemented it by checking the flag is_current in the\nworktree that find_shared_symref() returns.\n\nWhen the same branch is checked out in several worktrees simultaneously,\nfind_shared_symref() will return the first matching worktree in the list\ncomposed by get_worktrees().  If one of the worktrees with the checked\nout branch is the current worktree, find_shared_symref() may or may not\nreturn it, depending on the order of the list.\n\nLet's stop using find_shared_symref() in die_if_checked_out(), to handle\ncorrectly ignore_current_worktree.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n branch.c   | 16 +++++++++++-----\n worktree.c | 54 +++++++++++++++++++++++++++++-------------------------\n worktree.h |  6 ++++++\n 3 files changed, 46 insertions(+), 30 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d182756827..2378368415 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)\n void die_if_checked_out(const char *branch, int ignore_current_worktree)\n {\n \tstruct worktree **worktrees = get_worktrees();\n-\tconst struct worktree *wt;\n+\tint i;\n+\n+\tfor (i = 0; worktrees[i]; i++)\n+\t{\n+\t\tif (worktrees[i]->is_current && ignore_current_worktree)\n+\t\t\tcontinue;\n \n-\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n-\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n-\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n-\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n+\t\tif (is_shared_symref(worktrees[i], \"HEAD\", branch)) {\n+\t\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n+\t\t\tdie(_(\"'%s' is already checked out at '%s'\"),\n+\t\t\t\tbranch, worktrees[i]->path);\n+\t\t}\n \t}\n \n \tfree_worktrees(worktrees);\ndiff --git a/worktree.c b/worktree.c\nindex aa43c64119..d500d69e4c 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,\n  * bisect). New commands that do similar things should update this\n  * function as well.\n  */\n+int is_shared_symref(const struct worktree *wt, const char *symref,\n+\t\t     const char *target)\n+{\n+\tconst char *symref_target;\n+\tstruct ref_store *refs;\n+\tint flags;\n+\n+\tif (wt->is_bare)\n+\t\treturn 0;\n+\n+\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n+\t\tif (is_worktree_being_rebased(wt, target))\n+\t\t\treturn 1;\n+\t\tif (is_worktree_being_bisected(wt, target))\n+\t\t\treturn 1;\n+\t}\n+\n+\trefs = get_worktree_ref_store(wt);\n+\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n+\t\t\t\t\t\tNULL, &flags);\n+\tif ((flags & REF_ISSYMREF) &&\n+\t    symref_target && !strcmp(symref_target, target))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n \t\t\t\t\t  const char *target)\n@@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \tint i = 0;\n \n \tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct worktree *wt = worktrees[i];\n-\t\tconst char *symref_target;\n-\t\tstruct ref_store *refs;\n-\t\tint flags;\n-\n-\t\tif (wt->is_bare)\n-\t\t\tcontinue;\n-\n-\t\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n-\t\t\tif (is_worktree_being_rebased(wt, target)) {\n-\t\t\t\texisting = wt;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (is_worktree_being_bisected(wt, target)) {\n-\t\t\t\texisting = wt;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n-\n-\t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n-\t\t\t\t\t\t\tNULL, &flags);\n-\t\tif ((flags & REF_ISSYMREF) &&\n-\t\t    symref_target && !strcmp(symref_target, target)) {\n-\t\t\texisting = wt;\n+\t\tif (is_shared_symref(worktrees[i], symref, target)) {\n+\t\t\texisting = worktrees[i];\n \t\t\tbreak;\n \t\t}\n \t}\ndiff --git a/worktree.h b/worktree.h\nindex 9dcea6fc8c..7889c4761d 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n \t\t\t\t\t  const char *target);\n \n+/*\n+ * Returns true if a symref points to a ref in a worktree.\n+ */\n+int is_shared_symref(const struct worktree *wt,\n+\t\t     const char *symref, const char *target);\n+\n /*\n  * Similar to head_ref() for all HEADs _except_ one from the current\n  * worktree, which is covered by head_ref().\n-- \n2.39.0\n"},{"id":"470853","messageId":"3452517b-1efa-4ea9-a7d9-ed27715c750c@gmail.com","threadId":"59097","inReplyTo":"f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com","subject":"[PATCH v2 2/3] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-22T01:28:10Z","receivedAt":"2023-01-22T01:28:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In b5cabb4a9 (rebase: refuse to switch to branch already checked out\nelsewhere, 2020-02-23) we add a condition to prevent a rebase operation\ninvolving a switch to a branch that is already checked out in another\nworktree.\n\nA bug has recently been fixed that caused this to not work as expected.\n\nLet's add a test to notice if this changes in the future.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t3400-rebase.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex d5a8ee39fc..3ce918fdb8 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '\n \tgit rebase main main\n '\n \n+test_expect_success 'switch to branch checked out elsewhere fails' '\n+\ttest_when_finished \"\n+\t\tgit worktree remove wt1 &&\n+\t\tgit worktree remove wt2 &&\n+\t\tgit branch -d shared\n+\t\" &&\n+\tgit worktree add wt1 -b shared &&\n+\tgit worktree add wt2 -f shared &&\n+\t# we test in both worktrees to ensure that works\n+\t# as expected with \"first\" and \"next\" worktrees\n+\ttest_must_fail git -C wt1 rebase shared shared &&\n+\ttest_must_fail git -C wt2 rebase shared shared\n+'\n+\n test_expect_success 'switch to branch not checked out' '\n \tgit checkout main &&\n \tgit branch other &&\n-- \n2.39.0\n"},{"id":"470854","messageId":"1c98cbe8-8bca-e202-ef52-ce100307c7f3@gmail.com","threadId":"59097","inReplyTo":"f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com","subject":"[PATCH v2 3/3] switch: reject if the branch is already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-22T01:28:20Z","receivedAt":"2023-01-22T01:28:25Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since 5883034 (checkout: reject if the branch is already checked out\nelsewhere) in normal use, we do not allow multiple worktrees having the\nsame checked out branch.\n\nA bug has recently been fixed that caused this to not work as expected.\n\nLet's add a test to notice if this changes in the future.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex 5a7caf958c..7bea95dba2 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '\n \ttest_cmp_config \"\" --default \"\" branch.main2.merge\n '\n \n+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '\n+\ttest_when_finished \"\n+\t\tgit worktree remove wt1 &&\n+\t\tgit worktree remove wt2 &&\n+\t\tgit branch -d shared\n+\t\tgit checkout -\n+\t\" &&\n+\tgit checkout -b shared &&\n+\ttest_commit shared-first &&\n+\tHASH1=$(git rev-parse --verify HEAD) &&\n+\ttest_commit shared-second &&\n+\ttest_commit shared-third &&\n+\tHASH2=$(git rev-parse --verify HEAD) &&\n+\tgit worktree add wt1 -f shared &&\n+\tgit -C wt1 bisect start &&\n+\tgit -C wt1 bisect good $HASH1 &&\n+\tgit -C wt1 bisect bad $HASH2 &&\n+\tgit worktree add wt2 -f shared &&\n+\tgit -C wt2 bisect start &&\n+\tgit -C wt2 bisect good $HASH1 &&\n+\tgit -C wt2 bisect bad $HASH2 &&\n+\t# we test in both worktrees to ensure that works\n+\t# as expected with \"first\" and \"next\" worktrees\n+\ttest_must_fail git -C wt1 switch shared &&\n+\tgit -C wt1 switch --ignore-other-worktrees shared &&\n+\ttest_must_fail git -C wt2 switch shared &&\n+\tgit -C wt2 switch --ignore-other-worktrees shared\n+'\n+\n test_done\n-- \n2.39.0\n"},{"id":"470856","messageId":"xmqqbkmruykg.fsf@gitster.g","threadId":"59097","inReplyTo":"17f267b1-7f5e-2fb6-fb14-1c37ec355e65@gmail.com","subject":"Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-22T01:50:55Z","receivedAt":"2023-01-22T01:50:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Let's stop using find_shared_symref() in die_if_checked_out(), to handle\n> correctly ignore_current_worktree.\n\nThis says what the code stops using, but does not say what it uses\ninstead.\n\nFactoring is_shared_symref() out of find_shared_symref() is probably\na good idea that can stand alone without any other change in this\npatch as a single step, and then a second step can update\ndie_if_checked_out() using the new function.\n\nAs the proposed log message explained, updating die_if_checked_out()\nwith this patch would fix a bug---can we demonstrate the existing\nbreakage and protect the fix from future breakages by adding a test\nor two?\n\nOther than that, it looks like it is going in the right direction.\nThanks for working on the topic.\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>  branch.c   | 16 +++++++++++-----\n>  worktree.c | 54 +++++++++++++++++++++++++++++-------------------------\n>  worktree.h |  6 ++++++\n>  3 files changed, 46 insertions(+), 30 deletions(-)\n>\n> diff --git a/branch.c b/branch.c\n> index d182756827..2378368415 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)\n>  void die_if_checked_out(const char *branch, int ignore_current_worktree)\n>  {\n>  \tstruct worktree **worktrees = get_worktrees();\n> -\tconst struct worktree *wt;\n> +\tint i;\n> +\n> +\tfor (i = 0; worktrees[i]; i++)\n> +\t{\n\nStyle.  WRite the above on a single line, i.e.\n\n\tfor (i = 0; worktrees[i]; i++) {\n\nOptionally, we can lose the separate declaration of \"i\" by using C99\nvariable declaration, i.e.\n\n\tfor (int i = 0; worktrees[i]; i++) {\n\n> diff --git a/worktree.c b/worktree.c\n> index aa43c64119..d500d69e4c 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,\n>   * bisect). New commands that do similar things should update this\n>   * function as well.\n>   */\n\nThe above comment is about find_shared_symref() which iterates over\nworktrees and find the one that uses the named symref.  Now the\ncomment appears to apply to is_shared_symref() which does not\niterate but takes one specific worktree instance.  Do their\ndifferences necessitate some updates to the comment?\n\n> +int is_shared_symref(const struct worktree *wt, const char *symref,\n> +\t\t     const char *target)\n> +{\n\nWhat this function does sound more like \"is target in use in this\nparticular worktree by being pointed at by the symref?\"  IOW, I do\nnot see where \"shared\" comes into its name from.\n\n\"HEAD\" that is tentatively detached while bisecting or rebasing the\n\"target\" branch is still considered to point at the \"target\", so\nperhaps symref_points_at_target() or something?\n\n>  const struct worktree *find_shared_symref(struct worktree **worktrees,\n>  \t\t\t\t\t  const char *symref,\n>  \t\t\t\t\t  const char *target)\n> @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n>  \tint i = 0;\n>  \n>  \tfor (i = 0; worktrees[i]; i++) {\n\nNot a new problem, but the initialization on the declaration of \"i\"\nis redundant and unnecessary.  Again, we can use the C99 style, i.e.\n\n\tconst struct worktree *existing = NULL;\n-\tint i = 0;\n-\n-\tfor (i = 0; worktrees[i]; i++) {\n+\tfor (int i = 0; worktrees[i]; i++) {\n\n> +\t\tif (is_shared_symref(worktrees[i], symref, target)) {\n> +\t\t\texisting = worktrees[i];\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n> diff --git a/worktree.h b/worktree.h\n> index 9dcea6fc8c..7889c4761d 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n>  \t\t\t\t\t  const char *symref,\n>  \t\t\t\t\t  const char *target);\n>  \n> +/*\n> + * Returns true if a symref points to a ref in a worktree.\n> + */\n\nMake it clear that what you called \"a ref\" in the above is what is\ncalled \"target\" below.\n\n> +int is_shared_symref(const struct worktree *wt,\n> +\t\t     const char *symref, const char *target);\n> +\n>  /*\n>   * Similar to head_ref() for all HEADs _except_ one from the current\n>   * worktree, which is covered by head_ref().\n"},{"id":"470877","messageId":"766b25e1-2d7a-7b5c-10a9-43e545a57dba@gmail.com","threadId":"59097","inReplyTo":"xmqqbkmruykg.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-22T11:51:02Z","receivedAt":"2023-01-22T11:51:11Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 21-ene-2023 17:50:55, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > Let's stop using find_shared_symref() in die_if_checked_out(), to handle\n> > correctly ignore_current_worktree.\n> \n> This says what the code stops using, but does not say what it uses\n> instead.\n\nI thought the code for that was a better and clearer description.  I'll add\nsome details to the message.\n\n> Factoring is_shared_symref() out of find_shared_symref() is probably\n> a good idea that can stand alone without any other change in this\n> patch as a single step, and then a second step can update\n> die_if_checked_out() using the new function.\n\nOK.  I'll split that in two.\n\n> As the proposed log message explained, updating die_if_checked_out()\n> with this patch would fix a bug---can we demonstrate the existing\n> breakage and protect the fix from future breakages by adding a test\n> or two?\n\n2/3 and 3/3, I think makes more sense on its own commit.\n\n> > -\tconst struct worktree *wt;\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; worktrees[i]; i++)\n> > +\t{\n> \n> Style.  WRite the above on a single line, i.e.\n> \n> \tfor (i = 0; worktrees[i]; i++) {\n\nSorry.  I'll fix that.\n\n> \n> Optionally, we can lose the separate declaration of \"i\" by using C99\n> variable declaration, i.e.\n> \n> \tfor (int i = 0; worktrees[i]; i++) {\n\nOK.  Yes, I was playing with this, changed my mind and ended up with this and\nthe other style below, mess.\n\n> \n> > diff --git a/worktree.c b/worktree.c\n> > index aa43c64119..d500d69e4c 100644\n> > --- a/worktree.c\n> > +++ b/worktree.c\n> > @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,\n> >   * bisect). New commands that do similar things should update this\n> >   * function as well.\n> >   */\n> \n> The above comment is about find_shared_symref() which iterates over\n> worktrees and find the one that uses the named symref.  Now the\n> comment appears to apply to is_shared_symref() which does not\n> iterate but takes one specific worktree instance.  Do their\n> differences necessitate some updates to the comment?\n\nI think the comment still makes sense as is for the new function, both the\ndescription and the recommendation.  I will review it again.\n\n> \n> > +int is_shared_symref(const struct worktree *wt, const char *symref,\n> > +\t\t     const char *target)\n> > +{\n> \n> What this function does sound more like \"is target in use in this\n> particular worktree by being pointed at by the symref?\"  IOW, I do\n> not see where \"shared\" comes into its name from.\n> \n> \"HEAD\" that is tentatively detached while bisecting or rebasing the\n> \"target\" branch is still considered to point at the \"target\", so\n> perhaps symref_points_at_target() or something?\n> \n\nI tried to maintain the terms as much as possible.  I'll think about the name\nyou suggest.\n\n> >  const struct worktree *find_shared_symref(struct worktree **worktrees,\n> >  \t\t\t\t\t  const char *symref,\n> >  \t\t\t\t\t  const char *target)\n> > @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n> >  \tint i = 0;\n> >  \n> >  \tfor (i = 0; worktrees[i]; i++) {\n> \n> Not a new problem, but the initialization on the declaration of \"i\"\n> is redundant and unnecessary.  Again, we can use the C99 style, i.e.\n> \n> \tconst struct worktree *existing = NULL;\n> -\tint i = 0;\n> -\n> -\tfor (i = 0; worktrees[i]; i++) {\n> +\tfor (int i = 0; worktrees[i]; i++) {\n\nI'll fix this.\n\n> \n> > +\t\tif (is_shared_symref(worktrees[i], symref, target)) {\n> > +\t\t\texisting = worktrees[i];\n> >  \t\t\tbreak;\n> >  \t\t}\n> >  \t}\n> > diff --git a/worktree.h b/worktree.h\n> > index 9dcea6fc8c..7889c4761d 100644\n> > --- a/worktree.h\n> > +++ b/worktree.h\n> > @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n> >  \t\t\t\t\t  const char *symref,\n> >  \t\t\t\t\t  const char *target);\n> >  \n> > +/*\n> > + * Returns true if a symref points to a ref in a worktree.\n> > + */\n> \n> Make it clear that what you called \"a ref\" in the above is what is\n> called \"target\" below.\n> \n\nAgain, that was an attempt to maintain the terms from find_shared_symref().\n\n\nThank you for your review.\n"},{"id":"470889","messageId":"xmqqk01eqr3m.fsf@gitster.g","threadId":"59097","inReplyTo":"766b25e1-2d7a-7b5c-10a9-43e545a57dba@gmail.com","subject":"Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-22T19:58:05Z","receivedAt":"2023-01-22T19:58:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> As the proposed log message explained, updating die_if_checked_out()\n>> with this patch would fix a bug---can we demonstrate the existing\n>> breakage and protect the fix from future breakages by adding a test\n>> or two?\n>\n> 2/3 and 3/3, I think makes more sense on its own commit.\n\nHmph, how so?  Especially once you split 1/3 into the preliminary\nrefactoring and the real fix, the fix becomes fairly small and\nclear.  And the tests to protect the fix would go best in the same\ncommit.\n\n>> The above comment is about find_shared_symref() which iterates over\n>> worktrees and find the one that uses the named symref.  Now the\n>> comment appears to apply to is_shared_symref() which does not\n>> iterate but takes one specific worktree instance.  Do their\n>> differences necessitate some updates to the comment?\n>\n> I think the comment still makes sense as is for the new function, both the\n> description and the recommendation.  I will review it again.\n\nOK.  Thanks.\n\n>> > +int is_shared_symref(const struct worktree *wt, const char *symref,\n>> > +\t\t     const char *target)\n>> > +{\n>> \n>> What this function does sound more like \"is target in use in this\n>> particular worktree by being pointed at by the symref?\"  IOW, I do\n>> not see where \"shared\" comes into its name from.\n>> \n>> \"HEAD\" that is tentatively detached while bisecting or rebasing the\n>> \"target\" branch is still considered to point at the \"target\", so\n>> perhaps symref_points_at_target() or something?\n>\n> I tried to maintain the terms as much as possible.  I'll think about the name\n> you suggest.\n\nWhen you did not change a thing in such a way that it does not\nchange the relationship between that thing and other things, it\nmakes perfect sense to keep the same term to refer to the thing.\nOtherwise, once the thing starts playing different roles in the\nworld, there may be a better word to refer to the updated and\nimproved thing.\n"},{"id":"470893","messageId":"d61a2393-64c8-da49-fe13-00bc4a52d5e3@gmail.com","threadId":"59097","inReplyTo":"xmqqk01eqr3m.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-22T23:21:31Z","receivedAt":"2023-01-22T23:28:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 22-ene-2023 11:58:05, Junio C Hamano wrote:\n\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> >> As the proposed log message explained, updating die_if_checked_out()\n> >> with this patch would fix a bug---can we demonstrate the existing\n> >> breakage and protect the fix from future breakages by adding a test\n> >> or two?\n> >\n> > 2/3 and 3/3, I think makes more sense on its own commit.\n> \n> Hmph, how so?  Especially once you split 1/3 into the preliminary\n> refactoring and the real fix, the fix becomes fairly small and\n> clear.  And the tests to protect the fix would go best in the same\n> commit.\n\nMy intention is to protect rebase (2/3) and switch (3/3).  If any of\nthose tests break, even if die_if_checked_out() is no longer used by\nthem, I try to make the original intent clear with that in there.\n\ndie_if_checked_out() was initially fine, the ignore_current_worktree was\nunfortunately introduced.  I haven't checked, but other callers not\naffected by the change, i.e. ignore_current_worktree = 0, his tests\nshould have protected them by the change.\n\nYou are right, in a future reroll, split 1/3 could leave a fairly small\ncommit, maybe not a bad thing.  Definitely this need a reroll, because\nof the style issues, but I will wait some time for other reviewers. \n\n> >> The above comment is about find_shared_symref() which iterates over\n> >> worktrees and find the one that uses the named symref.  Now the\n> >> comment appears to apply to is_shared_symref() which does not\n> >> iterate but takes one specific worktree instance.  Do their\n> >> differences necessitate some updates to the comment?\n> >\n> > I think the comment still makes sense as is for the new function, both the\n> > description and the recommendation.  I will review it again.\n> \n> OK.  Thanks.\n> \n> >> > +int is_shared_symref(const struct worktree *wt, const char *symref,\n> >> > +\t\t     const char *target)\n> >> > +{\n> >> \n> >> What this function does sound more like \"is target in use in this\n> >> particular worktree by being pointed at by the symref?\"  IOW, I do\n> >> not see where \"shared\" comes into its name from.\n> >> \n> >> \"HEAD\" that is tentatively detached while bisecting or rebasing the\n> >> \"target\" branch is still considered to point at the \"target\", so\n> >> perhaps symref_points_at_target() or something?\n> >\n> > I tried to maintain the terms as much as possible.  I'll think about the name\n> > you suggest.\n> \n> When you did not change a thing in such a way that it does not\n> change the relationship between that thing and other things, it\n> makes perfect sense to keep the same term to refer to the thing.\n> Otherwise, once the thing starts playing different roles in the\n> world, there may be a better word to refer to the updated and\n> improved thing.\n\nI tried to maintain the relationship and the role, too.  Just introduce\nthe helper, as Phillip suggested and I think it is a good idea. \n\nThank you.\n"},{"id":"470965","messageId":"1a7fa327-3833-8da3-46d7-60bfe8dae82c@dunelm.org.uk","threadId":"59097","inReplyTo":"d61a2393-64c8-da49-fe13-00bc4a52d5e3@gmail.com","subject":"Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-24T10:35:26Z","receivedAt":"2023-01-24T10:36:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 22/01/2023 23:21, Rubén Justo wrote:\n> I tried to maintain the relationship and the role, too.  Just introduce\n> the helper, as Phillip suggested and I think it is a good idea.\n\nWhen I suggested adding a helper I was thinking of something like\n\nstatic const struct worktree *do_find_shared_symref(struct worktree \n**worktrees,\n  \t\t\t\t\t  const char *symref,\n  \t\t\t\t\t  const char *target,\n\t\t\t\t\t  int ignore_current)\n{\n\t/*\n\t * Body moved from find_share_symref() with a couple\n\t * of lines added to support ignore_current\n\t /*\n}\n\nconst struct worktree *find_shared_symref(struct worktree **worktrees,\n  \t\t\t\t\t  const char *symref,\n  \t\t\t\t\t  const char *target)\n{\n\treturn do_find_shared_symref(worktrees, symref, target, 0)\n}\n\nvoid die_if_checked_out(const char *branch, int ignore_current_worktree)\n{\n\tstruct worktree **worktrees = get_worktrees();\n\tconst struct worktree *wt;\n\n\twt = do_find_shared_symref(worktrees, \"HEAD\", branch,\n\t\t\t\t   ignore_current_worktree);\n\t/* rest unchanged */\n}\n\nThe aim was to avoid changing the public api\n\nBest Wishes\n\nPhillip\n\n"},{"id":"471049","messageId":"e7943994-3a7a-f877-8cf2-71146def9076@gmail.com","threadId":"59097","inReplyTo":"1a7fa327-3833-8da3-46d7-60bfe8dae82c@dunelm.org.uk","subject":"Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-26T03:07:03Z","receivedAt":"2023-01-26T03:07:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 24-ene-2023 10:35:26, Phillip Wood wrote:\n\n> On 22/01/2023 23:21, Rubén Justo wrote:\n> > I tried to maintain the relationship and the role, too.  Just introduce\n> > the helper, as Phillip suggested and I think it is a good idea.\n> \n> When I suggested adding a helper I was thinking of something like\n> \n> static const struct worktree *do_find_shared_symref(struct worktree\n> **worktrees,\n>  \t\t\t\t\t  const char *symref,\n>  \t\t\t\t\t  const char *target,\n> \t\t\t\t\t  int ignore_current)\n> {\n> \t/*\n> \t * Body moved from find_share_symref() with a couple\n> \t * of lines added to support ignore_current\n> \t /*\n> }\n> \n> const struct worktree *find_shared_symref(struct worktree **worktrees,\n>  \t\t\t\t\t  const char *symref,\n>  \t\t\t\t\t  const char *target)\n> {\n> \treturn do_find_shared_symref(worktrees, symref, target, 0)\n> }\n> \n> void die_if_checked_out(const char *branch, int ignore_current_worktree)\n> {\n> \tstruct worktree **worktrees = get_worktrees();\n> \tconst struct worktree *wt;\n> \n> \twt = do_find_shared_symref(worktrees, \"HEAD\", branch,\n> \t\t\t\t   ignore_current_worktree);\n> \t/* rest unchanged */\n> }\n> \n> The aim was to avoid changing the public api\n\nI thought about a solution like the one you suggest.  Also another one based on\niterations, something like wt_head_refs()....  I ended up with\nis_shared_symref(), it adds some value, I think.\n\nThe public API remains unchanged, and I like that the comment for\nfind_shared_ref(), which is an important note, is moved to is_shared_symref(),\nwhich contains the essential work related to the comment.\n\ndie_if_checked_out() needs to iterate (here was the wt_heads_refs()), but my\nintuition makes me think it's a good step since we might need another level,\nI'm not sure yet but, like \"die_if_if_checked_out(allow_if_current)\".\n\nI'm going to send a v3 addressing the issues Junio commented, still with the\nis_shared_symref().  If the above reasoning doesn't work for you or if the\nchange as-is introduces any problem, I have no objection to\n\"do_find_shared_symref()\".\n\nThank you.\n"},{"id":"471505","messageId":"02a15ebb-b927-1048-db2e-576abef9538b@gmail.com","threadId":"59097","inReplyTo":"f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com","subject":"[PATCH v3 0/4] fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-04T23:19:01Z","receivedAt":"2023-02-04T23:19:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Changes from v2:\n\n - is_shared_symref() is introduced in its own commit\n - code style fixes\n\nRubén Justo (4):\n  worktree: introduce is_shared_symref()\n  branch: fix die_if_checked_out() when ignore_current_worktree\n  rebase: refuse to switch to a branch already checked out elsewhere (test)\n  switch: reject if the branch is already checked out elsewhere (test)\n\n branch.c          | 14 +++++++----\n t/t2060-switch.sh | 29 ++++++++++++++++++++++\n t/t3400-rebase.sh | 14 +++++++++++\n worktree.c        | 63 +++++++++++++++++++++++------------------------\n worktree.h        |  6 +++++\n 5 files changed, 89 insertions(+), 37 deletions(-)\n\nRange-diff against v2:\n1:  cacdc022f8 < -:  ---------- branch: fix die_if_checked_out() when ignore_current_worktree\n-:  ---------- > 1:  6bbe05f452 worktree: introduce is_shared_symref()\n-:  ---------- > 2:  d787afe77f branch: fix die_if_checked_out() when ignore_current_worktree\n2:  6e9ed45f4e = 3:  4c418d37f8 rebase: refuse to switch to a branch already checked out elsewhere (test)\n3:  a66e58e7b2 = 4:  00b075af6a switch: reject if the branch is already checked out elsewhere (test)\n\nDiff against v2:\ndiff --git a/branch.c b/branch.c\nindex aa854fa65f..64b7dbfd17 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -820,10 +820,8 @@ void remove_branch_state(struct repository *r, int verbose)\n void die_if_checked_out(const char *branch, int ignore_current_worktree)\n {\n \tstruct worktree **worktrees = get_worktrees();\n-\tint i;\n \n-\tfor (i = 0; worktrees[i]; i++)\n-\t{\n+\tfor (int i = 0; worktrees[i]; i++) {\n \t\tif (worktrees[i]->is_current && ignore_current_worktree)\n \t\t\tcontinue;\n \ndiff --git a/worktree.c b/worktree.c\nindex d500d69e4c..34043d8fe0 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -434,17 +434,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n \t\t\t\t\t  const char *target)\n {\n-\tconst struct worktree *existing = NULL;\n-\tint i = 0;\n \n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tif (is_shared_symref(worktrees[i], symref, target)) {\n-\t\t\texisting = worktrees[i];\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\tfor (int i = 0; worktrees[i]; i++)\n+\t\tif (is_shared_symref(worktrees[i], symref, target))\n+\t\t\treturn worktrees[i];\n \n-\treturn existing;\n+\treturn NULL;\n }\n \n int submodule_uses_worktrees(const char *path)\n-- \n2.39.0\n"},{"id":"471506","messageId":"d62842c2-1b27-4a40-f5bb-fc1dca5d2d77@gmail.com","threadId":"59097","inReplyTo":"02a15ebb-b927-1048-db2e-576abef9538b@gmail.com","subject":"[PATCH v3 1/4] worktree: introduce is_shared_symref()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-04T23:25:43Z","receivedAt":"2023-02-04T23:25:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a new function, is_shared_symref(), which contains the heart of\nfind_shared_symref().  Refactor find_shared_symref() to use the new\nfunction is_shared_symref().\n\nSoon, we will use is_shared_symref() to search for symref beyond\nthe first worktree that matches.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n worktree.c | 62 +++++++++++++++++++++++++++---------------------------\n worktree.h |  6 ++++++\n 2 files changed, 37 insertions(+), 31 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex aa43c64119..40cb9874b7 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -403,44 +403,44 @@ int is_worktree_being_bisected(const struct worktree *wt,\n  * bisect). New commands that do similar things should update this\n  * function as well.\n  */\n-const struct worktree *find_shared_symref(struct worktree **worktrees,\n-\t\t\t\t\t  const char *symref,\n-\t\t\t\t\t  const char *target)\n+int is_shared_symref(const struct worktree *wt, const char *symref,\n+\t\t     const char *target)\n {\n-\tconst struct worktree *existing = NULL;\n-\tint i = 0;\n+\tconst char *symref_target;\n+\tstruct ref_store *refs;\n+\tint flags;\n \n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct worktree *wt = worktrees[i];\n-\t\tconst char *symref_target;\n-\t\tstruct ref_store *refs;\n-\t\tint flags;\n+\tif (wt->is_bare)\n+\t\treturn 0;\n \n-\t\tif (wt->is_bare)\n-\t\t\tcontinue;\n+\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n+\t\tif (is_worktree_being_rebased(wt, target))\n+\t\t\treturn 1;\n+\t\tif (is_worktree_being_bisected(wt, target))\n+\t\t\treturn 1;\n+\t}\n \n-\t\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n-\t\t\tif (is_worktree_being_rebased(wt, target)) {\n-\t\t\t\texisting = wt;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (is_worktree_being_bisected(wt, target)) {\n-\t\t\t\texisting = wt;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n+\trefs = get_worktree_ref_store(wt);\n+\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n+\t\t\t\t\t\tNULL, &flags);\n+\tif ((flags & REF_ISSYMREF) &&\n+\t    symref_target && !strcmp(symref_target, target))\n+\t\treturn 1;\n \n-\t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n-\t\t\t\t\t\t\tNULL, &flags);\n-\t\tif ((flags & REF_ISSYMREF) &&\n-\t\t    symref_target && !strcmp(symref_target, target)) {\n-\t\t\texisting = wt;\n-\t\t\tbreak;\n-\t\t}\n+\treturn 0;\n+}\n+\n+const struct worktree *find_shared_symref(struct worktree **worktrees,\n+\t\t\t\t\t  const char *symref,\n+\t\t\t\t\t  const char *target)\n+{\n+\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tif (is_shared_symref(worktrees[i], symref, target))\n+\t\t\treturn worktrees[i];\n \t}\n \n-\treturn existing;\n+\treturn NULL;\n }\n \n int submodule_uses_worktrees(const char *path)\ndiff --git a/worktree.h b/worktree.h\nindex 9dcea6fc8c..7889c4761d 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n \t\t\t\t\t  const char *target);\n \n+/*\n+ * Returns true if a symref points to a ref in a worktree.\n+ */\n+int is_shared_symref(const struct worktree *wt,\n+\t\t     const char *symref, const char *target);\n+\n /*\n  * Similar to head_ref() for all HEADs _except_ one from the current\n  * worktree, which is covered by head_ref().\n-- \n2.39.0\n"},{"id":"471507","messageId":"8670d6c6-b5cd-a1e3-8fbf-b948cb687388@gmail.com","threadId":"59097","inReplyTo":"02a15ebb-b927-1048-db2e-576abef9538b@gmail.com","subject":"[PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-04T23:25:53Z","receivedAt":"2023-02-04T23:26:01Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 8d9fdd7 (worktree.c: check whether branch is rebased in another\nworktree, 2016-04-22) die_if_checked_out() learned a new option\nignore_current_worktree, to modify the operation from \"die() if the\nbranch is checked out in any worktree\" to \"die() if the branch is\nchecked out in any worktree other than the current one\".\n\nUnfortunately we implemented it by checking the flag is_current in the\nworktree that find_shared_symref() returns.\n\nWhen the same branch is checked out in several worktrees simultaneously,\nfind_shared_symref() will return the first matching worktree in the list\ncomposed by get_worktrees().  If one of the worktrees with the checked\nout branch is the current worktree, find_shared_symref() may or may not\nreturn it, depending on the order in the list.\n\nInstead of find_shared_symref(), let's do the search using use the\nrecently introduced API is_shared_symref(), and consider\nignore_current_worktree when necessary.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n branch.c   | 14 +++++++++-----\n worktree.c |  3 +--\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex e5614b53b3..64b7dbfd17 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -820,12 +820,16 @@ void remove_branch_state(struct repository *r, int verbose)\n void die_if_checked_out(const char *branch, int ignore_current_worktree)\n {\n \tstruct worktree **worktrees = get_worktrees();\n-\tconst struct worktree *wt;\n \n-\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n-\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n-\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n-\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tif (worktrees[i]->is_current && ignore_current_worktree)\n+\t\t\tcontinue;\n+\n+\t\tif (is_shared_symref(worktrees[i], \"HEAD\", branch)) {\n+\t\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n+\t\t\tdie(_(\"'%s' is already checked out at '%s'\"),\n+\t\t\t\tbranch, worktrees[i]->path);\n+\t\t}\n \t}\n \n \tfree_worktrees(worktrees);\ndiff --git a/worktree.c b/worktree.c\nindex 40cb9874b7..34043d8fe0 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *target)\n {\n \n-\tfor (int i = 0; worktrees[i]; i++) {\n+\tfor (int i = 0; worktrees[i]; i++)\n \t\tif (is_shared_symref(worktrees[i], symref, target))\n \t\t\treturn worktrees[i];\n-\t}\n \n \treturn NULL;\n }\n-- \n2.39.0\n"},{"id":"471508","messageId":"5b0d5b6e-5055-6323-1b6c-fe98137e81f6@gmail.com","threadId":"59097","inReplyTo":"02a15ebb-b927-1048-db2e-576abef9538b@gmail.com","subject":"[PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-04T23:26:06Z","receivedAt":"2023-02-04T23:26:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In b5cabb4a9 (rebase: refuse to switch to branch already checked out\nelsewhere, 2020-02-23) we add a condition to prevent a rebase operation\ninvolving a switch to a branch that is already checked out in another\nworktree.\n\nA bug has recently been fixed that caused this to not work as expected.\n\nLet's add a test to notice if this changes in the future.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t3400-rebase.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex d5a8ee39fc..3ce918fdb8 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '\n \tgit rebase main main\n '\n \n+test_expect_success 'switch to branch checked out elsewhere fails' '\n+\ttest_when_finished \"\n+\t\tgit worktree remove wt1 &&\n+\t\tgit worktree remove wt2 &&\n+\t\tgit branch -d shared\n+\t\" &&\n+\tgit worktree add wt1 -b shared &&\n+\tgit worktree add wt2 -f shared &&\n+\t# we test in both worktrees to ensure that works\n+\t# as expected with \"first\" and \"next\" worktrees\n+\ttest_must_fail git -C wt1 rebase shared shared &&\n+\ttest_must_fail git -C wt2 rebase shared shared\n+'\n+\n test_expect_success 'switch to branch not checked out' '\n \tgit checkout main &&\n \tgit branch other &&\n-- \n2.39.0\n"},{"id":"471509","messageId":"b18a5710-2791-f892-8460-dda7ea41d88a@gmail.com","threadId":"59097","inReplyTo":"02a15ebb-b927-1048-db2e-576abef9538b@gmail.com","subject":"[PATCH v3 4/4] switch: reject if the branch is already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-04T23:26:20Z","receivedAt":"2023-02-04T23:26:28Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since 5883034 (checkout: reject if the branch is already checked out\nelsewhere) in normal use, we do not allow multiple worktrees having the\nsame checked out branch.\n\nA bug has recently been fixed that caused this to not work as expected.\n\nLet's add a test to notice if this changes in the future.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex 5a7caf958c..7bea95dba2 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '\n \ttest_cmp_config \"\" --default \"\" branch.main2.merge\n '\n \n+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '\n+\ttest_when_finished \"\n+\t\tgit worktree remove wt1 &&\n+\t\tgit worktree remove wt2 &&\n+\t\tgit branch -d shared\n+\t\tgit checkout -\n+\t\" &&\n+\tgit checkout -b shared &&\n+\ttest_commit shared-first &&\n+\tHASH1=$(git rev-parse --verify HEAD) &&\n+\ttest_commit shared-second &&\n+\ttest_commit shared-third &&\n+\tHASH2=$(git rev-parse --verify HEAD) &&\n+\tgit worktree add wt1 -f shared &&\n+\tgit -C wt1 bisect start &&\n+\tgit -C wt1 bisect good $HASH1 &&\n+\tgit -C wt1 bisect bad $HASH2 &&\n+\tgit worktree add wt2 -f shared &&\n+\tgit -C wt2 bisect start &&\n+\tgit -C wt2 bisect good $HASH1 &&\n+\tgit -C wt2 bisect bad $HASH2 &&\n+\t# we test in both worktrees to ensure that works\n+\t# as expected with \"first\" and \"next\" worktrees\n+\ttest_must_fail git -C wt1 switch shared &&\n+\tgit -C wt1 switch --ignore-other-worktrees shared &&\n+\ttest_must_fail git -C wt2 switch shared &&\n+\tgit -C wt2 switch --ignore-other-worktrees shared\n+'\n+\n test_done\n-- \n2.39.0\n"},{"id":"471566","messageId":"230206.861qn27mt9.gmgdl@evledraar.gmail.com","threadId":"59097","inReplyTo":"8670d6c6-b5cd-a1e3-8fbf-b948cb687388@gmail.com","subject":"Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-06T16:56:55Z","receivedAt":"2023-02-06T16:58:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 05 2023, Rubén Justo wrote:\n\n> -\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n> -\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n> -\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n> -\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n> +\tfor (int i = 0; worktrees[i]; i++) {\n\nI see that there are existing \"int i\" for counting worktrees in\nworktree.c, FWIW for new code I wouldn't mind if it's \"size_t i\"\ninstead, to make it future proof (and to eventually get rid of cast\nwarnings as we move more things from \"int\" to \"size_t\").\n> @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n>  \t\t\t\t\t  const char *target)\n>  {\n>  \n> -\tfor (int i = 0; worktrees[i]; i++) {\n> +\tfor (int i = 0; worktrees[i]; i++)\n>  \t\tif (is_shared_symref(worktrees[i], symref, target))\n>  \t\t\treturn worktrees[i];\n> -\t}\n\nYou added this function in the last commit, let's just skip adding the\nbraces to begin with, rather than this style-fix after the fact.\n"},{"id":"471567","messageId":"230206.86wn4u6859.gmgdl@evledraar.gmail.com","threadId":"59097","inReplyTo":"5b0d5b6e-5055-6323-1b6c-fe98137e81f6@gmail.com","subject":"Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-06T16:59:11Z","receivedAt":"2023-02-06T17:01:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 05 2023, Rubén Justo wrote:\n\n> In b5cabb4a9 (rebase: refuse to switch to branch already checked out\n> elsewhere, 2020-02-23) we add a condition to prevent a rebase operation\n> involving a switch to a branch that is already checked out in another\n> worktree.\n>\n> A bug has recently been fixed that caused this to not work as expected.\n\nPresumably in 1/4 and 2/4, if so saying \"in the preceding commit(s)\"\nwould be better.\n\n> Let's add a test to notice if this changes in the future.\n\nI for one would find this series much easier to follow if you started\nwith this test, possibly with a test_expect_failure, and as we fix the\nrelevant code flip them (both this and the subsequent one) to run\nsuccessfully, and include them as part of the commit that fixes the\nbug).\n\nMaybe there's reasons for why that's tricky to do in this case, so\nplease ignore this if so.\n"},{"id":"471660","messageId":"ab032813-39dd-2b45-0e3a-84fb6babe89c@gmail.com","threadId":"59097","inReplyTo":"230206.861qn27mt9.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-06T23:09:14Z","receivedAt":"2023-02-06T23:10:33Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 06-feb-2023 17:56:55, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Feb 05 2023, Rubén Justo wrote:\n> \n> > -\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n> > -\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n> > -\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n> > -\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n> > +\tfor (int i = 0; worktrees[i]; i++) {\n> \n> I see that there are existing \"int i\" for counting worktrees in\n> worktree.c, FWIW for new code I wouldn't mind if it's \"size_t i\"\n> instead, to make it future proof (and to eventually get rid of cast\n> warnings as we move more things from \"int\" to \"size_t\").\n\nOK.\n\n> > @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n> >  \t\t\t\t\t  const char *target)\n> >  {\n> >  \n> > -\tfor (int i = 0; worktrees[i]; i++) {\n> > +\tfor (int i = 0; worktrees[i]; i++)\n> >  \t\tif (is_shared_symref(worktrees[i], symref, target))\n> >  \t\t\treturn worktrees[i];\n> > -\t}\n> \n> You added this function in the last commit, let's just skip adding the\n> braces to begin with, rather than this style-fix after the fact.\n\nOK. Thanks.\n"},{"id":"471661","messageId":"4580b825-b117-4581-4ea2-ab30e350b6ad@gmail.com","threadId":"59097","inReplyTo":"230206.86wn4u6859.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-06T23:16:32Z","receivedAt":"2023-02-06T23:16:38Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:\n\n> > Let's add a test to notice if this changes in the future.\n> \n> I for one would find this series much easier to follow if you started\n> with this test, possibly with a test_expect_failure, and as we fix the\n> relevant code flip them (both this and the subsequent one) to run\n> successfully, and include them as part of the commit that fixes the\n> bug).\n> \n> Maybe there's reasons for why that's tricky to do in this case, so\n> please ignore this if so.\n\nI'll give it try, I like the idea.  Thanks.\n"},{"id":"471674","messageId":"fc671499-5650-0473-0850-41449d85e1c3@dunelm.org.uk","threadId":"59097","inReplyTo":"d62842c2-1b27-4a40-f5bb-fc1dca5d2d77@gmail.com","subject":"Re: [PATCH v3 1/4] worktree: introduce is_shared_symref()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-07T10:44:59Z","receivedAt":"2023-02-07T10:45:07Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nI'm still not really sure why we want to make this new function public \nbut the conversion looks fine. It does mean that the next patch has to \nadd a loop to die_if_checked_out() though which we wouldn't need to do \nif we just changed find_shared_symref() to take an extra parameter to \nignore the current worktree.\n\nBest Wishes\n\nPhillip\n\nOn 04/02/2023 23:25, Rubén Justo wrote:\n> Add a new function, is_shared_symref(), which contains the heart of\n> find_shared_symref().  Refactor find_shared_symref() to use the new\n> function is_shared_symref().\n> \n> Soon, we will use is_shared_symref() to search for symref beyond\n> the first worktree that matches.\n> \n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>   worktree.c | 62 +++++++++++++++++++++++++++---------------------------\n>   worktree.h |  6 ++++++\n>   2 files changed, 37 insertions(+), 31 deletions(-)\n> \n> diff --git a/worktree.c b/worktree.c\n> index aa43c64119..40cb9874b7 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -403,44 +403,44 @@ int is_worktree_being_bisected(const struct worktree *wt,\n>    * bisect). New commands that do similar things should update this\n>    * function as well.\n>    */\n> -const struct worktree *find_shared_symref(struct worktree **worktrees,\n> -\t\t\t\t\t  const char *symref,\n> -\t\t\t\t\t  const char *target)\n> +int is_shared_symref(const struct worktree *wt, const char *symref,\n> +\t\t     const char *target)\n>   {\n> -\tconst struct worktree *existing = NULL;\n> -\tint i = 0;\n> +\tconst char *symref_target;\n> +\tstruct ref_store *refs;\n> +\tint flags;\n>   \n> -\tfor (i = 0; worktrees[i]; i++) {\n> -\t\tstruct worktree *wt = worktrees[i];\n> -\t\tconst char *symref_target;\n> -\t\tstruct ref_store *refs;\n> -\t\tint flags;\n> +\tif (wt->is_bare)\n> +\t\treturn 0;\n>   \n> -\t\tif (wt->is_bare)\n> -\t\t\tcontinue;\n> +\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n> +\t\tif (is_worktree_being_rebased(wt, target))\n> +\t\t\treturn 1;\n> +\t\tif (is_worktree_being_bisected(wt, target))\n> +\t\t\treturn 1;\n> +\t}\n>   \n> -\t\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n> -\t\t\tif (is_worktree_being_rebased(wt, target)) {\n> -\t\t\t\texisting = wt;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t\tif (is_worktree_being_bisected(wt, target)) {\n> -\t\t\t\texisting = wt;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t}\n> +\trefs = get_worktree_ref_store(wt);\n> +\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n> +\t\t\t\t\t\tNULL, &flags);\n> +\tif ((flags & REF_ISSYMREF) &&\n> +\t    symref_target && !strcmp(symref_target, target))\n> +\t\treturn 1;\n>   \n> -\t\trefs = get_worktree_ref_store(wt);\n> -\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n> -\t\t\t\t\t\t\tNULL, &flags);\n> -\t\tif ((flags & REF_ISSYMREF) &&\n> -\t\t    symref_target && !strcmp(symref_target, target)) {\n> -\t\t\texisting = wt;\n> -\t\t\tbreak;\n> -\t\t}\n> +\treturn 0;\n> +}\n> +\n> +const struct worktree *find_shared_symref(struct worktree **worktrees,\n> +\t\t\t\t\t  const char *symref,\n> +\t\t\t\t\t  const char *target)\n> +{\n> +\n> +\tfor (int i = 0; worktrees[i]; i++) {\n> +\t\tif (is_shared_symref(worktrees[i], symref, target))\n> +\t\t\treturn worktrees[i];\n>   \t}\n>   \n> -\treturn existing;\n> +\treturn NULL;\n>   }\n>   \n>   int submodule_uses_worktrees(const char *path)\n> diff --git a/worktree.h b/worktree.h\n> index 9dcea6fc8c..7889c4761d 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n>   \t\t\t\t\t  const char *symref,\n>   \t\t\t\t\t  const char *target);\n>   \n> +/*\n> + * Returns true if a symref points to a ref in a worktree.\n> + */\n> +int is_shared_symref(const struct worktree *wt,\n> +\t\t     const char *symref, const char *target);\n> +\n>   /*\n>    * Similar to head_ref() for all HEADs _except_ one from the current\n>    * worktree, which is covered by head_ref().\n"},{"id":"471676","messageId":"1da499d6-3de3-ba34-1ca0-6f39e528b80b@dunelm.org.uk","threadId":"59097","inReplyTo":"230206.861qn27mt9.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-07T10:50:12Z","receivedAt":"2023-02-07T10:50:32Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Feb 05 2023, Rubén Justo wrote:\n> \n>> -\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n>> -\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n>> -\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n>> -\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n>> +\tfor (int i = 0; worktrees[i]; i++) {\n> \n> I see that there are existing \"int i\" for counting worktrees in\n> worktree.c, FWIW for new code I wouldn't mind if it's \"size_t i\"\n> instead, to make it future proof (and to eventually get rid of cast\n> warnings as we move more things from \"int\" to \"size_t\").\n\nThis seems to be different from the usual worries about int/size_t \ncomparisons/truncation. worktrees is NULL terminated so there is no \nsigned/unsigned comparison here as we're not comparing it to anything. \nThe only concern would be that there are more than INT_MAX which seems \nunlikely.\n\nBest Wishes\n\nPhillip\n\n>> @@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n>>   \t\t\t\t\t  const char *target)\n>>   {\n>>   \n>> -\tfor (int i = 0; worktrees[i]; i++) {\n>> +\tfor (int i = 0; worktrees[i]; i++)\n>>   \t\tif (is_shared_symref(worktrees[i], symref, target))\n>>   \t\t\treturn worktrees[i];\n>> -\t}\n> \n> You added this function in the last commit, let's just skip adding the\n> braces to begin with, rather than this style-fix after the fact.\n"},{"id":"471677","messageId":"f4be3c97-eb15-790e-9746-96d0c9bbc5a7@dunelm.org.uk","threadId":"59097","inReplyTo":"4580b825-b117-4581-4ea2-ab30e350b6ad@gmail.com","subject":"Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-07T10:52:39Z","receivedAt":"2023-02-07T10:52:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Rubén\n\nOn 06/02/2023 23:16, Rubén Justo wrote:\n> On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:\n> \n>>> Let's add a test to notice if this changes in the future.\n>>\n>> I for one would find this series much easier to follow if you started\n>> with this test, possibly with a test_expect_failure, and as we fix the\n>> relevant code flip them (both this and the subsequent one) to run\n>> successfully, and include them as part of the commit that fixes the\n>> bug).\n>>\n>> Maybe there's reasons for why that's tricky to do in this case, so\n>> please ignore this if so.\n> \n> I'll give it try, I like the idea.  Thanks.\n\nSquashing the last three commits together so that the tests are \nintroduced in the same commit as the fix as Junio suggested in his \ncomments on the previous round would be very welcome.\n\nBest Wishes\n\nPhillip\n"},{"id":"471682","messageId":"230207.86wn4t37k6.gmgdl@evledraar.gmail.com","threadId":"59097","inReplyTo":"1da499d6-3de3-ba34-1ca0-6f39e528b80b@dunelm.org.uk","subject":"Re: [PATCH v3 2/4] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-07T12:58:26Z","receivedAt":"2023-02-07T13:54:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 07 2023, Phillip Wood wrote:\n\n> On 06/02/2023 16:56, Ævar Arnfjörð Bjarmason wrote:\n>> On Sun, Feb 05 2023, Rubén Justo wrote:\n>> \n>>> -\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n>>> -\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n>>> -\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n>>> -\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n>>> +\tfor (int i = 0; worktrees[i]; i++) {\n>> I see that there are existing \"int i\" for counting worktrees in\n>> worktree.c, FWIW for new code I wouldn't mind if it's \"size_t i\"\n>> instead, to make it future proof (and to eventually get rid of cast\n>> warnings as we move more things from \"int\" to \"size_t\").\n>\n> This seems to be different from the usual worries about int/size_t\n> comparisons/truncation. worktrees is NULL terminated so there is no\n> signed/unsigned comparison here as we're not comparing it to\n> anything.\n\nHaving looked at this again I think using \"int\" for now is the right\nthing, sorry about the noise.\n\n> The only concern would be that there are more than INT_MAX\n> which seems unlikely.\n\nMy general concern isn't just with the code that we can prove doesn't\noverflow in such cases, but that by having different types for such\nthings (which isn't the case here, I thought our \"struct worktrees **\"\nwould be alloc'd with a \"size_t\") we end up with coercion warnings.\n\nThose warnings are so prevalent in this codebase that we've had to\nsuppress them, and consequently we make it harder to spot the real\nissues.\n"},{"id":"471746","messageId":"19e3e0d9-406e-ac2f-c43a-b4e994035529@gmail.com","threadId":"59097","inReplyTo":"f4be3c97-eb15-790e-9746-96d0c9bbc5a7@dunelm.org.uk","subject":"Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-08T00:43:45Z","receivedAt":"2023-02-08T00:43:52Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 07-feb-2023 10:52:39, Phillip Wood wrote:\n> Hi Rubén\n> \n> On 06/02/2023 23:16, Rubén Justo wrote:\n> > On 06-feb-2023 17:59:11, Ævar Arnfjörð Bjarmason wrote:\n> > \n> > > > Let's add a test to notice if this changes in the future.\n> > > \n> > > I for one would find this series much easier to follow if you started\n> > > with this test, possibly with a test_expect_failure, and as we fix the\n> > > relevant code flip them (both this and the subsequent one) to run\n> > > successfully, and include them as part of the commit that fixes the\n> > > bug).\n> > > \n> > > Maybe there's reasons for why that's tricky to do in this case, so\n> > > please ignore this if so.\n> > \n> > I'll give it try, I like the idea.  Thanks.\n> \n> Squashing the last three commits together so that the tests are introduced\n> in the same commit as the fix as Junio suggested in his comments on the\n> previous round would be very welcome.\n\nYes, I considered that, but I think that keeping the tests in their own\ncommit is reasonable.  The tests protect against a malfunction that we\ndid not notice when the implementation was done.  The commits reference\n(and use similar subjects) to the original commit where we should have\nintroduced the tests.  If the tests fail, I think it will be easier and\nless confusing to reach the original commit where the implementation was\ndone if we keep them separated, rather than combining all three commits.\n\nI'm going to reorder the commits and change to use test_expect_failure().\nThis way the commit with the fix will also be linked.\n"},{"id":"471752","messageId":"xmqqk00svimy.fsf@gitster.g","threadId":"59097","inReplyTo":"19e3e0d9-406e-ac2f-c43a-b4e994035529@gmail.com","subject":"Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T05:19:33Z","receivedAt":"2023-02-08T05:19:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> ...  If the tests fail, I think it will be easier and\n> less confusing to reach the original commit where the implementation was\n> done if we keep them separated, rather than combining all three commits.\n\nNo, if the test were in the same commit as the implementation, then\nupon a future test breakage it is easier to see what code the test\nwas meant to protect, exactly because it is part of the same commit.\n\n> I'm going to reorder the commits and change to use test_expect_failure().\n\nDo not do that.  Adding a failing test and flipping the\nexpect_failure to expect_success is a sure way to make the series\nharder to review.\n\n"},{"id":"471808","messageId":"c1b3dc07-f3d3-273e-6486-9aa709b1c269@gmail.com","threadId":"59097","inReplyTo":"xmqqk00svimy.fsf@gitster.g","subject":"Re: [PATCH v3 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-08T22:09:29Z","receivedAt":"2023-02-08T22:09:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 07-feb-2023 21:19:33, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > ...  If the tests fail, I think it will be easier and\n> > less confusing to reach the original commit where the implementation was\n> > done if we keep them separated, rather than combining all three commits.\n> \n> No, if the test were in the same commit as the implementation, then\n> upon a future test breakage it is easier to see what code the test\n> was meant to protect, exactly because it is part of the same commit.\n\nYes, but my reasoning was that those tests protect what we did in 8d9fdd7\n(worktree.c: check whether branch is rebased in another worktree, 2016-04-22)\nand in b5cabb4a9 (rebase: refuse to switch to branch already checked out\nelsewhere, 2020-02-23), not die_if_checked_out().\n\n> > I'm going to reorder the commits and change to use test_expect_failure().\n> \n> Do not do that.  Adding a failing test and flipping the\n> expect_failure to expect_success is a sure way to make the series\n> harder to review.\n\nHmm\n"},{"id":"472118","messageId":"CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com","threadId":"59097","inReplyTo":"b18a5710-2791-f892-8460-dda7ea41d88a@gmail.com","subject":"Re: [PATCH v3 4/4] switch: reject if the branch is already checked out elsewhere (test)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-15T04:17:42Z","receivedAt":"2023-02-15T04:20:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 4, 2023 at 6:33 PM Rubén Justo <rjusto@gmail.com> wrote:\n> Since 5883034 (checkout: reject if the branch is already checked out\n> elsewhere) in normal use, we do not allow multiple worktrees having the\n> same checked out branch.\n>\n> A bug has recently been fixed that caused this to not work as expected.\n>\n> Let's add a test to notice if this changes in the future.\n>\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n> diff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\n> @@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '\n> +test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '\n> +       test_when_finished \"\n> +               git worktree remove wt1 &&\n> +               git worktree remove wt2 &&\n> +               git branch -d shared\n> +               git checkout -\n> +       \" &&\n> +       git checkout -b shared &&\n\nA few comments...\n\nThe test_when_finished() call has an odd mix of &&-chain and missing &&-chain.\n\nIf you do use &&-chaining inside test_when_finished(), you have to be\ncareful that _none_ of the cleanup commands fail, otherwise\ntest_when_finished() itself will fail, making the entire test fail,\n_even if_ the test body succeeded. So, &&-chaining the commands in\nthis test_when_finished() is undesirable since any of the `git\nworktree remove` and `git branch -d` commands could potentially fail.\nBetter would be to drop the &&-chain and ensure that the final command\nsucceeds.\n\nIt may be a good idea to use `||:` at the end of each command as\ndocumentation for readers that any of the cleanup commands might fail,\nwhich could happen if something in the body of the test fails.\n\nThe `git branch -d shared` cleanup command fails unconditionally\nbecause it's still the checked-out branch in the directory. You need\nto swap the `git branch -d shared` and `git checkout -` commands.\n\nTaking all the above points into account, a possible rewrite would be:\n\n  test_when_finished \"\n    git worktree remove wt1 ||:\n    git worktree remove wt2 ||:\n    git checkout - ||:\n    git branch -d shared ||:\n  \" &&\n  git checkout -b shared &&\n\n> +       test_commit shared-first &&\n> +       HASH1=$(git rev-parse --verify HEAD) &&\n> +       test_commit shared-second &&\n> +       test_commit shared-third &&\n> +       HASH2=$(git rev-parse --verify HEAD) &&\n> +       git worktree add wt1 -f shared &&\n> +       git -C wt1 bisect start &&\n> +       git -C wt1 bisect good $HASH1 &&\n> +       git -C wt1 bisect bad $HASH2 &&\n> +       git worktree add wt2 -f shared &&\n> +       git -C wt2 bisect start &&\n> +       git -C wt2 bisect good $HASH1 &&\n> +       git -C wt2 bisect bad $HASH2 &&\n> +       # we test in both worktrees to ensure that works\n> +       # as expected with \"first\" and \"next\" worktrees\n> +       test_must_fail git -C wt1 switch shared &&\n> +       git -C wt1 switch --ignore-other-worktrees shared &&\n> +       test_must_fail git -C wt2 switch shared &&\n> +       git -C wt2 switch --ignore-other-worktrees shared\n> +'\n"},{"id":"472168","messageId":"c4dcfd83-593a-0c91-4f5d-04b172c160f3@gmail.com","threadId":"59097","inReplyTo":"CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com","subject":"Re: [PATCH v3 4/4] switch: reject if the branch is already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-15T22:17:50Z","receivedAt":"2023-02-15T22:17:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 14-feb-2023 23:17:42, Eric Sunshine wrote:\n\n> > +test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '\n> > +       test_when_finished \"\n> > +               git worktree remove wt1 &&\n> > +               git worktree remove wt2 &&\n> > +               git branch -d shared\n> > +               git checkout -\n> > +       \" &&\n> > +       git checkout -b shared &&\n> \n> The test_when_finished() call has an odd mix of &&-chain and missing &&-chain.\n\nOops, thanks, that was unintentional and hides an error ...\n\n> \n> If you do use &&-chaining inside test_when_finished(), you have to be\n> careful that _none_ of the cleanup commands fail, otherwise\n> test_when_finished() itself will fail, making the entire test fail,\n> _even if_ the test body succeeded. So, &&-chaining the commands in\n> this test_when_finished() is undesirable since any of the `git\n> worktree remove` and `git branch -d` commands could potentially fail.\n> Better would be to drop the &&-chain and ensure that the final command\n> succeeds.\n> \n> It may be a good idea to use `||:` at the end of each command as\n> documentation for readers that any of the cleanup commands might fail,\n> which could happen if something in the body of the test fails.\n> \n> The `git branch -d shared` cleanup command fails unconditionally\n> because it's still the checked-out branch in the directory. You need\n> to swap the `git branch -d shared` and `git checkout -` commands.\n\n...  in fact, that needs to be \"git branch -D shared\" to successfully\ndelete that unmerged branch.  Considering also, what you pointed out,\nswapping the two commands.\n\nAbout the `||:`, maybe it's a good idea to keep the '&&' and let the\ntest fail if any command fails in the cleanup.  However, if that is also\npart of the test, maybe must not be in the cleanup... so I don't have a\nstrong opinion on that.\n\nI'll wait some time, if there is no argument against the change, I'll\nreroll with the fix and using '||:' as you suggested.\n\nThank you for your review and comments.\n"},{"id":"472717","messageId":"6fed3b1b-1c4e-9298-19b6-7ad9c04c87dd@gmail.com","threadId":"59097","inReplyTo":"f7f45f54-9261-45ea-3399-8ba8dee6832b@gmail.com","subject":"[PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-25T14:14:14Z","receivedAt":"2023-02-25T14:14:22Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since v3, two errors in 4/4 have been fixed:\n  - \"checkout -\" must be done before \"branch -d\"\n  - \"branch -D\" instead of \"branch -d\" because \"shared\" is a non-merged\n    branch.\n\nAlso, '||:' has been used to chain commands in the test_when_finished()\nblock.\n\nRubén Justo (4):\n  worktree: introduce is_shared_symref()\n  branch: fix die_if_checked_out() when ignore_current_worktree\n  rebase: refuse to switch to a branch already checked out elsewhere (test)\n  switch: reject if the branch is already checked out elsewhere (test)\n\n branch.c          | 14 +++++++----\n t/t2060-switch.sh | 29 ++++++++++++++++++++++\n t/t3400-rebase.sh | 14 +++++++++++\n worktree.c        | 63 +++++++++++++++++++++++------------------------\n worktree.h        |  6 +++++\n 5 files changed, 89 insertions(+), 37 deletions(-)\n\nRange-diff against v3:\n1:  40b5ea54d3 = 1:  22d0944aad worktree: introduce is_shared_symref()\n2:  2f1479b354 = 2:  e7ba7b6fdd branch: fix die_if_checked_out() when ignore_current_worktree\n3:  81a5b619c3 = 3:  8de8319d0e rebase: refuse to switch to a branch already checked out elsewhere (test)\n4:  6559e58344 ! 4:  27d6ae2755 switch: reject if the branch is already checked out elsewhere (test)\n    @@ t/t2060-switch.sh: test_expect_success 'tracking info copied with autoSetupMerge\n      \n     +test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '\n     +\ttest_when_finished \"\n    -+\t\tgit worktree remove wt1 &&\n    -+\t\tgit worktree remove wt2 &&\n    -+\t\tgit branch -d shared\n    -+\t\tgit checkout -\n    ++\t\tgit worktree remove wt1 ||:\n    ++\t\tgit worktree remove wt2 ||:\n    ++\t\tgit checkout - ||:\n    ++\t\tgit branch -D shared ||:\n     +\t\" &&\n     +\tgit checkout -b shared &&\n     +\ttest_commit shared-first &&\n-- \n2.39.2\n"},{"id":"472718","messageId":"d23997f9-240c-000d-a93e-7e0d0c5adbe6@gmail.com","threadId":"59097","inReplyTo":"6fed3b1b-1c4e-9298-19b6-7ad9c04c87dd@gmail.com","subject":"[PATCH v4 1/4] worktree: introduce is_shared_symref()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-25T14:21:51Z","receivedAt":"2023-02-25T14:21:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a new function, is_shared_symref(), which contains the heart of\nfind_shared_symref().  Refactor find_shared_symref() to use the new\nfunction is_shared_symref().\n\nSoon, we will use is_shared_symref() to search for symref beyond\nthe first worktree that matches.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n worktree.c | 62 +++++++++++++++++++++++++++---------------------------\n worktree.h |  6 ++++++\n 2 files changed, 37 insertions(+), 31 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex aa43c64119..40cb9874b7 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -403,44 +403,44 @@ int is_worktree_being_bisected(const struct worktree *wt,\n  * bisect). New commands that do similar things should update this\n  * function as well.\n  */\n-const struct worktree *find_shared_symref(struct worktree **worktrees,\n-\t\t\t\t\t  const char *symref,\n-\t\t\t\t\t  const char *target)\n+int is_shared_symref(const struct worktree *wt, const char *symref,\n+\t\t     const char *target)\n {\n-\tconst struct worktree *existing = NULL;\n-\tint i = 0;\n+\tconst char *symref_target;\n+\tstruct ref_store *refs;\n+\tint flags;\n \n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct worktree *wt = worktrees[i];\n-\t\tconst char *symref_target;\n-\t\tstruct ref_store *refs;\n-\t\tint flags;\n+\tif (wt->is_bare)\n+\t\treturn 0;\n \n-\t\tif (wt->is_bare)\n-\t\t\tcontinue;\n+\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n+\t\tif (is_worktree_being_rebased(wt, target))\n+\t\t\treturn 1;\n+\t\tif (is_worktree_being_bisected(wt, target))\n+\t\t\treturn 1;\n+\t}\n \n-\t\tif (wt->is_detached && !strcmp(symref, \"HEAD\")) {\n-\t\t\tif (is_worktree_being_rebased(wt, target)) {\n-\t\t\t\texisting = wt;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (is_worktree_being_bisected(wt, target)) {\n-\t\t\t\texisting = wt;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n+\trefs = get_worktree_ref_store(wt);\n+\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n+\t\t\t\t\t\tNULL, &flags);\n+\tif ((flags & REF_ISSYMREF) &&\n+\t    symref_target && !strcmp(symref_target, target))\n+\t\treturn 1;\n \n-\t\trefs = get_worktree_ref_store(wt);\n-\t\tsymref_target = refs_resolve_ref_unsafe(refs, symref, 0,\n-\t\t\t\t\t\t\tNULL, &flags);\n-\t\tif ((flags & REF_ISSYMREF) &&\n-\t\t    symref_target && !strcmp(symref_target, target)) {\n-\t\t\texisting = wt;\n-\t\t\tbreak;\n-\t\t}\n+\treturn 0;\n+}\n+\n+const struct worktree *find_shared_symref(struct worktree **worktrees,\n+\t\t\t\t\t  const char *symref,\n+\t\t\t\t\t  const char *target)\n+{\n+\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tif (is_shared_symref(worktrees[i], symref, target))\n+\t\t\treturn worktrees[i];\n \t}\n \n-\treturn existing;\n+\treturn NULL;\n }\n \n int submodule_uses_worktrees(const char *path)\ndiff --git a/worktree.h b/worktree.h\nindex 9dcea6fc8c..7889c4761d 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *symref,\n \t\t\t\t\t  const char *target);\n \n+/*\n+ * Returns true if a symref points to a ref in a worktree.\n+ */\n+int is_shared_symref(const struct worktree *wt,\n+\t\t     const char *symref, const char *target);\n+\n /*\n  * Similar to head_ref() for all HEADs _except_ one from the current\n  * worktree, which is covered by head_ref().\n-- \n2.39.2\n"},{"id":"472719","messageId":"356b96bf-4130-6a91-4ce5-4228b071e183@gmail.com","threadId":"59097","inReplyTo":"6fed3b1b-1c4e-9298-19b6-7ad9c04c87dd@gmail.com","subject":"[PATCH v4 2/4] branch: fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-25T14:22:02Z","receivedAt":"2023-02-25T14:22:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In 8d9fdd7 (worktree.c: check whether branch is rebased in another\nworktree, 2016-04-22) die_if_checked_out() learned a new option\nignore_current_worktree, to modify the operation from \"die() if the\nbranch is checked out in any worktree\" to \"die() if the branch is\nchecked out in any worktree other than the current one\".\n\nUnfortunately we implemented it by checking the flag is_current in the\nworktree that find_shared_symref() returns.\n\nWhen the same branch is checked out in several worktrees simultaneously,\nfind_shared_symref() will return the first matching worktree in the list\ncomposed by get_worktrees().  If one of the worktrees with the checked\nout branch is the current worktree, find_shared_symref() may or may not\nreturn it, depending on the order in the list.\n\nInstead of find_shared_symref(), let's do the search using use the\nrecently introduced API is_shared_symref(), and consider\nignore_current_worktree when necessary.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n branch.c   | 14 +++++++++-----\n worktree.c |  3 +--\n 2 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex e5614b53b3..64b7dbfd17 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -820,12 +820,16 @@ void remove_branch_state(struct repository *r, int verbose)\n void die_if_checked_out(const char *branch, int ignore_current_worktree)\n {\n \tstruct worktree **worktrees = get_worktrees();\n-\tconst struct worktree *wt;\n \n-\twt = find_shared_symref(worktrees, \"HEAD\", branch);\n-\tif (wt && (!ignore_current_worktree || !wt->is_current)) {\n-\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n-\t\tdie(_(\"'%s' is already checked out at '%s'\"), branch, wt->path);\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tif (worktrees[i]->is_current && ignore_current_worktree)\n+\t\t\tcontinue;\n+\n+\t\tif (is_shared_symref(worktrees[i], \"HEAD\", branch)) {\n+\t\t\tskip_prefix(branch, \"refs/heads/\", &branch);\n+\t\t\tdie(_(\"'%s' is already checked out at '%s'\"),\n+\t\t\t\tbranch, worktrees[i]->path);\n+\t\t}\n \t}\n \n \tfree_worktrees(worktrees);\ndiff --git a/worktree.c b/worktree.c\nindex 40cb9874b7..34043d8fe0 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -435,10 +435,9 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,\n \t\t\t\t\t  const char *target)\n {\n \n-\tfor (int i = 0; worktrees[i]; i++) {\n+\tfor (int i = 0; worktrees[i]; i++)\n \t\tif (is_shared_symref(worktrees[i], symref, target))\n \t\t\treturn worktrees[i];\n-\t}\n \n \treturn NULL;\n }\n-- \n2.39.2\n"},{"id":"472720","messageId":"6ff38da4-4465-5ffb-36ec-d73a09f3c900@gmail.com","threadId":"59097","inReplyTo":"6fed3b1b-1c4e-9298-19b6-7ad9c04c87dd@gmail.com","subject":"[PATCH v4 3/4] rebase: refuse to switch to a branch already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-25T14:22:13Z","receivedAt":"2023-02-25T14:22:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In b5cabb4a9 (rebase: refuse to switch to branch already checked out\nelsewhere, 2020-02-23) we add a condition to prevent a rebase operation\ninvolving a switch to a branch that is already checked out in another\nworktree.\n\nA bug has recently been fixed that caused this to not work as expected.\n\nLet's add a test to notice if this changes in the future.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t3400-rebase.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex d5a8ee39fc..3ce918fdb8 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -388,6 +388,20 @@ test_expect_success 'switch to branch checked out here' '\n \tgit rebase main main\n '\n \n+test_expect_success 'switch to branch checked out elsewhere fails' '\n+\ttest_when_finished \"\n+\t\tgit worktree remove wt1 &&\n+\t\tgit worktree remove wt2 &&\n+\t\tgit branch -d shared\n+\t\" &&\n+\tgit worktree add wt1 -b shared &&\n+\tgit worktree add wt2 -f shared &&\n+\t# we test in both worktrees to ensure that works\n+\t# as expected with \"first\" and \"next\" worktrees\n+\ttest_must_fail git -C wt1 rebase shared shared &&\n+\ttest_must_fail git -C wt2 rebase shared shared\n+'\n+\n test_expect_success 'switch to branch not checked out' '\n \tgit checkout main &&\n \tgit branch other &&\n-- \n2.39.2\n"},{"id":"472721","messageId":"8f335d0e-144d-2795-06b5-004f6752b632@gmail.com","threadId":"59097","inReplyTo":"6fed3b1b-1c4e-9298-19b6-7ad9c04c87dd@gmail.com","subject":"[PATCH v4 4/4] switch: reject if the branch is already checked out elsewhere (test)","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-25T14:22:23Z","receivedAt":"2023-02-25T14:22:39Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since 5883034 (checkout: reject if the branch is already checked out\nelsewhere) in normal use, we do not allow multiple worktrees having the\nsame checked out branch.\n\nA bug has recently been fixed that caused this to not work as expected.\n\nLet's add a test to notice if this changes in the future.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t2060-switch.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t2060-switch.sh b/t/t2060-switch.sh\nindex 5a7caf958c..e247a4735b 100755\n--- a/t/t2060-switch.sh\n+++ b/t/t2060-switch.sh\n@@ -146,4 +146,33 @@ test_expect_success 'tracking info copied with autoSetupMerge=inherit' '\n \ttest_cmp_config \"\" --default \"\" branch.main2.merge\n '\n \n+test_expect_success 'switch back when temporarily detached and checked out elsewhere ' '\n+\ttest_when_finished \"\n+\t\tgit worktree remove wt1 ||:\n+\t\tgit worktree remove wt2 ||:\n+\t\tgit checkout - ||:\n+\t\tgit branch -D shared ||:\n+\t\" &&\n+\tgit checkout -b shared &&\n+\ttest_commit shared-first &&\n+\tHASH1=$(git rev-parse --verify HEAD) &&\n+\ttest_commit shared-second &&\n+\ttest_commit shared-third &&\n+\tHASH2=$(git rev-parse --verify HEAD) &&\n+\tgit worktree add wt1 -f shared &&\n+\tgit -C wt1 bisect start &&\n+\tgit -C wt1 bisect good $HASH1 &&\n+\tgit -C wt1 bisect bad $HASH2 &&\n+\tgit worktree add wt2 -f shared &&\n+\tgit -C wt2 bisect start &&\n+\tgit -C wt2 bisect good $HASH1 &&\n+\tgit -C wt2 bisect bad $HASH2 &&\n+\t# we test in both worktrees to ensure that works\n+\t# as expected with \"first\" and \"next\" worktrees\n+\ttest_must_fail git -C wt1 switch shared &&\n+\tgit -C wt1 switch --ignore-other-worktrees shared &&\n+\ttest_must_fail git -C wt2 switch shared &&\n+\tgit -C wt2 switch --ignore-other-worktrees shared\n+'\n+\n test_done\n-- \n2.39.2\n"},{"id":"472734","messageId":"xmqqbklhuzq8.fsf@gitster.g","threadId":"59097","inReplyTo":"6fed3b1b-1c4e-9298-19b6-7ad9c04c87dd@gmail.com","subject":"Re: [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-25T22:50:07Z","receivedAt":"2023-02-25T22:50:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>      +\ttest_when_finished \"\n>     -+\t\tgit worktree remove wt1 &&\n>     -+\t\tgit worktree remove wt2 &&\n>     -+\t\tgit branch -d shared\n>     -+\t\tgit checkout -\n>     ++\t\tgit worktree remove wt1 ||:\n>     ++\t\tgit worktree remove wt2 ||:\n>     ++\t\tgit checkout - ||:\n>     ++\t\tgit branch -D shared ||:\n>      +\t\" &&\n\nSorry, but I do not get the point of this construct.  The\ntest_cleanup variable that accumulates test_when_finished scripts is\nevaled without -e shopt set, so you can just remove all these ||:\nand add a single \"true\" at the end, like\n\n\t...\n\tgit checkout -\n\tgit branch -D shared\n\t:\n    \" &&\n\nfor exactly the same effect, no?\n\n\n"},{"id":"472767","messageId":"7a8dc2ee-a8a3-b59e-f2ea-a7720320fb7c@gmail.com","threadId":"59097","inReplyTo":"xmqqbklhuzq8.fsf@gitster.g","subject":"Re: [PATCH v4 0/4] fix die_if_checked_out() when ignore_current_worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-27T00:00:35Z","receivedAt":"2023-02-27T00:00:42Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 25-feb-2023 14:50:07, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> >      +\ttest_when_finished \"\n> >     -+\t\tgit worktree remove wt1 &&\n> >     -+\t\tgit worktree remove wt2 &&\n> >     -+\t\tgit branch -d shared\n> >     -+\t\tgit checkout -\n> >     ++\t\tgit worktree remove wt1 ||:\n> >     ++\t\tgit worktree remove wt2 ||:\n> >     ++\t\tgit checkout - ||:\n> >     ++\t\tgit branch -D shared ||:\n> >      +\t\" &&\n> \n> Sorry, but I do not get the point of this construct.  The\n> test_cleanup variable that accumulates test_when_finished scripts is\n> evaled without -e shopt set, so you can just remove all these ||:\n> and add a single \"true\" at the end, like\n> \n> \t...\n> \tgit checkout -\n> \tgit branch -D shared\n> \t:\n>     \" &&\n> \n> for exactly the same effect, no?\n\nYes, of course.\n\nI agree that all of that '||:' can be confusing, but I'm not sure if a\nfinal ':' is much better.\n\nAs I said in my response to Eric, I don't have a clear opinion here.\n\ntest_when_finished() was introduced in 2bf78867 (test-lib: Let tests\nspecify commands to be run at end of test, 2010-05-02) to allow\nindicating some actions to be executed after the test block finishes,\neven if the test fails, to leave a repository or overall situation\nhealthy for the next tests.\n\nWith that in mind, I ask myself, is it worth removing the worktrees\nhere?  Is it worth reporting a possible error in that removal?  If, for\nexample, in the future we deny the removal of a worktree under bisect,\nis it worth reporting the failure here?  What about the \"shared\"\nbranch? ...\n\nI Dunno.\n\nThank you for reviewing this patch.\n"}]}