{"thread":{"id":"59018","subject":"[PATCH 0/2] branch: operations on orphan branches","startedAt":"2022-12-30T22:59:07Z","lastAt":"2023-05-01T22:19:05Z","messageCount":49,"participants":["Rubén Justo","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"469694","messageId":"ffd675e9-8a64-ae05-fc3b-36ae99092735@gmail.com","threadId":"59018","inReplyTo":null,"subject":"[PATCH 0/2] branch: operations on orphan branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-12-30T22:59:02Z","receivedAt":"2022-12-30T22:59:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Avoid some confusing errors operating on orphan branches when\nworking with worktrees\n\n\nRubén Justo (2):\n branch: description for orphan branch errors\n branch: rename orphan branches in any worktree\n\n builtin/branch.c | 10 +++++-----\n t/t3200-branch.sh | 12 ++++++++++++\n t/t3202-show-branch.sh | 36 ++++++++++++++++++++++++++++++++++++\n 3 files changed, 53 insertions(+), 5 deletions(-)\n\n-- \n2.39.0\n\n"},{"id":"469695","messageId":"dd86016b-3232-563b-940e-03bc36af917a@gmail.com","threadId":"59018","inReplyTo":"ffd675e9-8a64-ae05-fc3b-36ae99092735@gmail.com","subject":"[PATCH 1/2] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-12-30T23:04:51Z","receivedAt":"2022-12-30T23:04:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bcfc82bd48 (branch: description for non-existent branch errors,\n2022-10-08) we used \"strcmp(head, branch)\" to check if we're asked to\noperate in a branch that is the current HEAD, and\n\"ref_exists(branch_ref)\" to check if it points to a valid ref, i.e. it\nis an orphan branch. We are doing this with the intention of avoiding\nthe confusing error: \"No branch named...\"\n\nIf we're asked to operate with an orphan branch, but it is on a\ndifferent worktree, \"strcmp(head, branch)\" is not going to match:\n\n\t$ git worktree add orphan-worktree --detach\n\t$ git -C orphan-worktree checkout --orphan orpha-branch\n\t$ git branch -m orpha-branch orphan-branch\n\tfatal: No branch named 'orpha-branch'.\n\nLet's do the check for HEAD in any worktree in the repository, removing\nthe \"strcmp\" and using the helper introduced in 31ad6b61bd (branch: add\nbranch_checked_out() helper, 2022-06-15)\n\nThis commit also extends the tests introduced on bcfc82bd48, in\nt3202-show-branch, to cover not just the initial branch but any orphan\nbranch.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 8 ++++----\n t/t3202-show-branch.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..954008e51d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -528,8 +528,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n-\t\tif (copy && !strcmp(head, oldname))\n+\tif ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {\n+\t\tif (copy && branch_checked_out(oldref.buf))\n \t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n \t\telse\n \t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n-\t\t\terror((!argc || !strcmp(head, branch_name))\n+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n \t\t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t\t : _(\"No branch named '%s'.\"),\n \t\t\t branch_name);\n@@ -851,7 +851,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (!ref_exists(branch->refname)) {\n-\t\t\tif (!argc || !strcmp(head, branch->name))\n+\t\t\tif (!argc || branch_checked_out(branch->refname))\n \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n \t\t}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..acaedac34e 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -221,4 +221,40 @@ test_expect_success 'fatal descriptions on non-existent branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'error descriptions on orphan or unborn-yet branch' '\n+\tcat >expect <<-EOF &&\n+\terror: No commit on branch '\\''orphan-branch'\\'' yet.\n+\tEOF\n+\ttest_when_finished git worktree remove -f orphan-worktree &&\n+\tgit worktree add orphan-worktree --detach &&\n+\tgit -C orphan-worktree checkout --orphan orphan-branch &&\n+\ttest_must_fail git -C orphan-worktree branch --edit-description 2>actual && # implicit branch\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git -C orphan-worktree branch --edit-description orphan-branch 2>actual && # explicit branch\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git branch --edit-description orphan-branch 2>actual && # different worktree\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'fatal descriptions on orphan or unborn-yet branch' '\n+\tcat >expect <<-EOF &&\n+\tfatal: No commit on branch '\\''orphan-branch'\\'' yet.\n+\tEOF\n+\ttest_when_finished git worktree remove -f orphan-worktree &&\n+\tgit worktree add orphan-worktree --detach &&\n+\tgit -C orphan-worktree checkout --orphan orphan-branch &&\n+\ttest_must_fail git -C orphan-worktree branch --set-upstream-to=non-existent 2>actual && # implicit branch\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git -C orphan-worktree branch --set-upstream-to=non-existent orphan-branch 2>actual && # explicit branch\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git branch --set-upstream-to=non-existent orphan-branch 2>actual && # different worktree\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git -C orphan-worktree branch -c new-branch 2>actual && # implicit branch\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git -C orphan-worktree branch -c orphan-branch new-branch 2>actual && # explicit branch\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git branch -c orphan-branch new-branch 2>actual && # different worktree\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.39.0\n"},{"id":"469696","messageId":"bce24c2f-47c9-30cf-12b7-48beaf112bdb@gmail.com","threadId":"59018","inReplyTo":"ffd675e9-8a64-ae05-fc3b-36ae99092735@gmail.com","subject":"[PATCH 2/2] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-12-30T23:12:04Z","receivedAt":"2022-12-30T23:12:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\nwe added support for renaming an orphan branch in the current working\ntree, avoiding the rename_ref() error because an orphan branch does not\npoint to any ref.\n\nWe did this with two checks (note that head cannot be null due to a\nprecondition in cmd_branch):\n\n \t if (branch_is_not_head)\n \t \t// Only HEAD can be orphan. Do the rename_ref\n \t else if (head_ref_is_valid)\n \t\t// There is a valid ref. Do the rename_ref\n \t else\n \t\t// Nothing to do here. Continue\n\nWith worktrees, we can try to rename an orphan branch while we are in a\ndifferent worktree, and so the orphan branch we want to rename is not\nthe HEAD in the current worktree:\n\n\t$ git worktree add orphan-worktree --detach\n\t$ git -C orphan-worktree checkout --orphan orphn-branch\n\t$ git branch -m orphn-branch orphan-branch\n\tfatal: No branch named 'orphn-branch'.\n\nLets fix this considering all HEADs in all worktrees in the repository,\nchanging the checks introduced in cfaff3aac8 to:\n\n \t if (branch_is_not_head_in_any_worktree)\n \t \t// Only HEADs can be orphan. Do the rename_ref\n \t else if (branch_ref_is_valid)\n \t\t// There is a valid ref. Do the rename_ref\n \t else\n \t\t// Nothing to do here. Continue\n\nLet's also add to t3200-branch tests for this and for normal renames of\norphan branches (other than the initial branch) for which we did not yet\nhave tests.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 2 +-\n t/t3200-branch.sh | 12 ++++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 954008e51d..151541e1c2 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -559,7 +559,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\t oldref.buf, newref.buf);\n \n \tif (!copy &&\n-\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n+\t (!branch_checked_out(oldref.buf) || ref_exists(oldref.buf)) &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a169b68d6..01d616af05 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,18 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -m should work with orphan branches' '\n+\ttest_when_finished git worktree remove -f orphan-worktree &&\n+\ttest_when_finished git checkout - &&\n+\tgit worktree add orphan-worktree --detach &&\n+\tgit -C orphan-worktree checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar-wt &&\n+\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n+\tgit checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar &&\n+\ttest orphan-bar=$(git branch --show-current)\n+'\n+\n test_expect_success 'git branch -d on orphan HEAD (merged)' '\n \ttest_when_finished git checkout main &&\n \tgit checkout --orphan orphan &&\n-- \n2.39.0\n"},{"id":"469721","messageId":"xmqqy1qmhq8k.fsf@gitster.g","threadId":"59018","inReplyTo":"dd86016b-3232-563b-940e-03bc36af917a@gmail.com","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-01T03:45:47Z","receivedAt":"2023-01-01T03:45:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> In bcfc82bd48 (branch: description for non-existent branch errors,\n> 2022-10-08) we used \"strcmp(head, branch)\" to check if we're asked to\n> operate in a branch that is the current HEAD, and\n> \"ref_exists(branch_ref)\" to check if it points to a valid ref, i.e. it\n> is an orphan branch. We are doing this with the intention of avoiding\n> the confusing error: \"No branch named...\"\n\nI agree that it is good to notice \"the user thinks the branch should\nalready exist, for they have 'checked out' that branch, but there is\nno commit on the branch yet\". I am not sure checked out on a separate\nworktree as an unborn branch is always the indication that the user\nthinks the branch should exist (e.g. \"you allowed somebody else, or\nyourself, prepare a separate worktree to work on something a few\nweeks ago, but no single commit has been made there and you forgot\nabout the worktree---should the branch still exist?\"), but that is a\nseparate topic. Let's assume that the two are equivalent.\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index f63fd45edb..954008e51d 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -528,8 +528,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n> \t}\n> \n> -\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n> -\t\tif (copy && !strcmp(head, oldname))\n> +\tif ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {\n> +\t\tif (copy && branch_checked_out(oldref.buf))\n> \t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n> \t\telse\n> \t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n\nIsn't branch_checked_out() a fairly heavyweight operation when you\nhave multiple worktrees? The original went to the filesystem\n(i.e. check ref_exists()) only after seeing that a condition that\ncan be computed using only in-core data holds (i.e. the branch names\nare the same or we are doing a copy), which is an optimum order to\navoid doing unnecessary work in most common cases, but I am not sure\nif the order the checks are done in the updated code optimizes for\nthe common case. If branch_checked_out() is more expensive than a\ncall to ref_exists() for a single brnch, that would change the\nequation. Calling such a heavyweight operation twice would make it\neven more expensive but that is a perfectly fine thing to do in the\nerror codepath, inside the block that is entered after we noticed an\nerror condition.\n\n> +test_expect_success 'error descriptions on orphan or unborn-yet branch' '\n> +\tcat >expect <<-EOF &&\n> +\terror: No commit on branch '\\''orphan-branch'\\'' yet.\n> +\tEOF\n> ...\n> +'\n> +\n> +test_expect_success 'fatal descriptions on orphan or unborn-yet branch' '\n> +\tcat >expect <<-EOF &&\n> +\tfatal: No commit on branch '\\''orphan-branch'\\'' yet.\n> +\tEOF\n> ...\n> +'\n\nDo we already cover existing \"No branch named\" case the same way in\nthis test script, so that it is OK for these new tests to cover only\nthe \"not yet\" cases? I am asking because if we have existing\ncoverage, before and after the change to the C code in this patch,\nsome of the existing tests would change the behaviour (i.e. they\nwould have said \"No branch named X\" when somebody else created an\nunborn branch in a separate worktree, but now they would say \"No\ncommit on branch X yet\"), but I see no such change in the test. If\nwe lack existing coverage, we probably should --- otherwise we would\nnot notice when somebody breaks the command to say \"No commit on\nbranch X yet\" when it should say \"No such branch X\".\n\n"},{"id":"469756","messageId":"18ca1e65-3e26-8352-cabd-daebdd0cf7f2@gmail.com","threadId":"59018","inReplyTo":"xmqqy1qmhq8k.fsf@gitster.g","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-03T01:15:37Z","receivedAt":"2023-01-03T01:15:47Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 01-ene-2023 12:45:47, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> Isn't branch_checked_out() a fairly heavyweight operation when you\n> have multiple worktrees? The original went to the filesystem\n> (i.e. check ref_exists()) only after seeing that a condition that\n> can be computed using only in-core data holds (i.e. the branch names\n> are the same or we are doing a copy), which is an optimum order to\n> avoid doing unnecessary work in most common cases, but I am not sure\n> if the order the checks are done in the updated code optimizes for\n> the common case. If branch_checked_out() is more expensive than a\n> call to ref_exists() for a single brnch, that would change the\n> equation. Calling such a heavyweight operation twice would make it\n> even more expensive but that is a perfectly fine thing to do in the\n> error codepath, inside the block that is entered after we noticed an\n> error condition.\n\nI share your concern, I thought about it.\n\nMy thoughts evaluating the change were more or less:\n\n - presumably there should be many more references than worktrees, and\n more repositories with 0 or 1 workdirs than more, so, arbitrarily,\n calling ref_exists() last still sounds sensible\n\n - strcmp() to branch_checked_out() introduces little change in the\n logic\n\n - I like branch_checked_out(), it expresses better what we want there\n\n - branch_checked_out() considers refs, strcmp considers branch names\n (we have a corner case with @{-1} pointing to HEAD, that this\n resolves)\n\n - finally, perhaps branch_checked_out() has optimization possibilities.\n Maybe in the common case we can get close to the amount of work we\n are doing now. Here we can alleviate a bit by removing the\n unconditional resolve_refdup(HEAD) we are doing at the beginning of\n cmd_branch().\n\nIn the end, it seems to me that we have some places where we are\nconsidering HEAD and we may need to consider HEADs.\n\nAnd again, I agree, the change has somewhat profound implications.\n\n> \n> > +test_expect_success 'error descriptions on orphan or unborn-yet branch' '\n> > +\tcat >expect <<-EOF &&\n> > +\terror: No commit on branch '\\''orphan-branch'\\'' yet.\n> > +\tEOF\n> > ...\n> > +'\n> > +\n> > +test_expect_success 'fatal descriptions on orphan or unborn-yet branch' '\n> > +\tcat >expect <<-EOF &&\n> > +\tfatal: No commit on branch '\\''orphan-branch'\\'' yet.\n> > +\tEOF\n> > ...\n> > +'\n> \n> Do we already cover existing \"No branch named\" case the same way in\n> this test script, so that it is OK for these new tests to cover only\n> the \"not yet\" cases? I am asking because if we have existing\n> coverage, before and after the change to the C code in this patch,\n> some of the existing tests would change the behaviour (i.e. they\n> would have said \"No branch named X\" when somebody else created an\n> unborn branch in a separate worktree, but now they would say \"No\n> commit on branch X yet\"), but I see no such change in the test. If\n> we lack existing coverage, we probably should --- otherwise we would\n> not notice when somebody breaks the command to say \"No commit on\n> branch X yet\" when it should say \"No such branch X\".\n> \n\nI think we do, bcfc82bd (branch: description for non-existent branch\nerrors). We have some pending changes to follow the CodingGuideLines in\nthis messages that maybe we can resume:\n\nhttps://lore.kernel.org/git/eb3c689e-efeb-4468-a10f-dd32bc0ee37b@gmail.com/\n\n\n\nThank you for reading the change this way.\n"},{"id":"469781","messageId":"xmqqr0wau6px.fsf@gitster.g","threadId":"59018","inReplyTo":"18ca1e65-3e26-8352-cabd-daebdd0cf7f2@gmail.com","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-04T06:58:02Z","receivedAt":"2023-01-04T06:58:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> On 01-ene-2023 12:45:47, Junio C Hamano wrote:\n>> Rubén Justo writes:\n>> \n>> Isn't branch_checked_out() a fairly heavyweight operation when you\n>> have multiple worktrees? The original went to the filesystem\n>> (i.e. check ref_exists()) only after seeing that a condition that\n>> can be computed using only in-core data holds (i.e. the branch names\n>> are the same or we are doing a copy), which is an optimum order to\n>> avoid doing unnecessary work in most common cases, but I am not sure\n>> if the order the checks are done in the updated code optimizes for\n>> the common case. If branch_checked_out() is more expensive than a\n>> call to ref_exists() for a single brnch, that would change the\n>> equation. Calling such a heavyweight operation twice would make it\n>> even more expensive but that is a perfectly fine thing to do in the\n>> error codepath, inside the block that is entered after we noticed an\n>> error condition.\n>\n> I share your concern, I thought about it.\n>\n> My thoughts evaluating the change were more or less:\n>\n> - presumably there should be many more references than worktrees, and\n> more repositories with 0 or 1 workdirs than more, so, arbitrarily,\n> calling ref_exists() last still sounds sensible\n>\n> - strcmp() to branch_checked_out() introduces little change in the\n> logic\n>\n> - I like branch_checked_out(), it expresses better what we want there\n>\n> - branch_checked_out() considers refs, strcmp considers branch names\n> (we have a corner case with @{-1} pointing to HEAD, that this\n> resolves)\n>\n> - finally, perhaps branch_checked_out() has optimization possibilities.\n> Maybe in the common case we can get close to the amount of work we\n> are doing now. Here we can alleviate a bit by removing the\n> unconditional resolve_refdup(HEAD) we are doing at the beginning of\n> cmd_branch().\n>\n> In the end, it seems to me that we have some places where we are\n> considering HEAD and we may need to consider HEADs.\n\nI am not sure what you share with me after reading the above,\nthough. As I already said, I am not opposed to the use of\nbranch_checked_out(), or checking the state of other worktrees\nconnected to the same repository, at all.\n\nI was merely looking at this:\n\n> -\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n> -\t\tif (copy && !strcmp(head, oldname))\n> +\tif ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {\n\nand wondering if the evaluation order to call branch_checked_out()\nunconditionally and then calling ref_exists() still makes sense, or\nnow the strcmp() part of the original has become so much more\ncostly, if we are better off doing the same thing in a different\norder, e.g.\n\n\tif (!ref_exists(oldref.buf) && (copy || !branch_checked_out(oldref.buf))) {\n\n>> Do we already cover existing \"No branch named\" case the same way in\n>> this test script, so that it is OK for these new tests to cover only\n>> the \"not yet\" cases? I am asking because if we have existing\n>> coverage, before and after the change to the C code in this patch,\n>> some of the existing tests would change the behaviour (i.e. they\n>> would have said \"No branch named X\" when somebody else created an\n>> unborn branch in a separate worktree, but now they would say \"No\n>> commit on branch X yet\"), but I see no such change in the test. If\n>> we lack existing coverage, we probably should --- otherwise we would\n>> not notice when somebody breaks the command to say \"No commit on\n>> branch X yet\" when it should say \"No such branch X\".\n>\n> I think we do, bcfc82bd (branch: description for non-existent branch\n> errors).\n\nIf we already have checks that current code triggers the \"no branch\nnamed X\" warning, and because the patch is changing the code to give\nthat warning to instead say \"branch X has no commits yet\" in some\ncases, if the existing test coverage were thorough, some of the\nexisting tests that expect \"no branch named X\" warning should now\nexpect \"branch X has no commits yet\" warning. Your patch did not\nhave any such change in the tests, which was an indication that the\nexisting test coverage was lacking, no?\n\nAnd given that, do we now have a complete test coverage for all\ncases with the patch we are discussing?\n\nThanks.\n\n"},{"id":"469858","messageId":"db348ac3-571c-f6ba-c889-9f476f4ae04b@gmail.com","threadId":"59018","inReplyTo":"xmqqr0wau6px.fsf@gitster.g","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-06T23:39:16Z","receivedAt":"2023-01-06T23:39:26Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 04-ene-2023 15:58:02, Junio C Hamano wrote:\n> \n> > -\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n> > -\t\tif (copy && !strcmp(head, oldname))\n> > +\tif ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {\n> \n> and wondering if the evaluation order to call branch_checked_out()\n> unconditionally and then calling ref_exists() still makes sense, or\n> now the strcmp() part of the original has become so much more\n> costly, if we are better off doing the same thing in a different\n> order, e.g.\n> \n> \tif (!ref_exists(oldref.buf) && (copy || !branch_checked_out(oldref.buf))) {\n> \n\nThinking of this as a whole, perhaps after this series we can add:\n\n-- >8 --\nSubject: [PATCH] branch: copy_or_rename_branch() unconditionally calls\n\nIn previous commits we have introduced changes to\ncopy_or_rename_branch() that lead to unconditionally calling\nref_exists(), twice in some circumstances.\n\nOptimize copy_or_rename_branch() so that it only calls ref_exists() once\nand reorder some conditionals to consider ref_exists() first and avoid\nunnecessarily calling other expensive functions.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c14a7a42e6..6e70377a1a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -515,7 +515,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0;\n+\tint recovery = 0, oldref_exists;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -523,12 +523,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t * ref that we used to allow to be created by accident.\n \t\t */\n \t\tif (ref_exists(oldref.buf))\n-\t\t\trecovery = 1;\n+\t\t\toldref_exists = recovery = 1;\n \t\telse\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n-\t}\n+\t} else\n+\t\toldref_exists = ref_exists(oldref.buf);\n \n-\tif ((copy || !branch_checked_out(oldref.buf)) && !ref_exists(oldref.buf)) {\n+\tif (!oldref_exists && (copy || !branch_checked_out(oldref.buf))) {\n \t\tif (copy && branch_checked_out(oldref.buf))\n \t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n \t\telse\n@@ -558,8 +559,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n-\tif (!copy &&\n-\t (!branch_checked_out(oldref.buf) || ref_exists(oldref.buf)) &&\n+\tif (!copy && oldref_exists &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n\nbase-commit: 64b4d8c0eb1938fa10477b9bd9aead2773456e3e\n--\n\n> >> Do we already cover existing \"No branch named\" case the same way in\n> >> this test script, so that it is OK for these new tests to cover only\n> >> the \"not yet\" cases? I am asking because if we have existing\n> >> coverage, before and after the change to the C code in this patch,\n> >> some of the existing tests would change the behaviour (i.e. they\n> >> would have said \"No branch named X\" when somebody else created an\n> >> unborn branch in a separate worktree, but now they would say \"No\n> >> commit on branch X yet\"), but I see no such change in the test. If\n> >> we lack existing coverage, we probably should --- otherwise we would\n> >> not notice when somebody breaks the command to say \"No commit on\n> >> branch X yet\" when it should say \"No such branch X\".\n> >\n> > I think we do, bcfc82bd (branch: description for non-existent branch\n> > errors).\n> \n> If we already have checks that current code triggers the \"no branch\n> named X\" warning, and because the patch is changing the code to give\n> that warning to instead say \"branch X has no commits yet\" in some\n> cases, if the existing test coverage were thorough, some of the\n> existing tests that expect \"no branch named X\" warning should now\n> expect \"branch X has no commits yet\" warning. Your patch did not\n> have any such change in the tests, which was an indication that the\n> existing test coverage was lacking, no?\n\nYes. We did not have a test for 'No branch named' that implied an\norphan branch. I think if we had tried that, we would have ended\nup doing what we're doing now.\n\n> \n> And given that, do we now have a complete test coverage for all\n> cases with the patch we are discussing?\n\nConsidering 1/2 and 2/2, I think so. But if you're asking maybe\nyou're realizing something...\n"},{"id":"469861","messageId":"xmqq8rifp63p.fsf@gitster.g","threadId":"59018","inReplyTo":"db348ac3-571c-f6ba-c889-9f476f4ae04b@gmail.com","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-06T23:59:22Z","receivedAt":"2023-01-07T00:00:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Thinking of this as a whole, perhaps after this series we can add:\n\nWhy \"after\"? If we already know that the existing patches are\nmaking things worse and need to fix the regression with a future\npatch to make it usable again, why introduce a regression in the\nfirst place?\n"},{"id":"469862","messageId":"xmqq4jt3p629.fsf@gitster.g","threadId":"59018","inReplyTo":"db348ac3-571c-f6ba-c889-9f476f4ae04b@gmail.com","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-07T00:00:14Z","receivedAt":"2023-01-07T00:01:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Considering 1/2 and 2/2, I think so. But if you're asking maybe\n> you're realizing something...\n\nNo, I am trying to make sure that you have thought it through.\n"},{"id":"469863","messageId":"9a050344-27ea-4f26-e66a-2795596eb11d@gmail.com","threadId":"59018","inReplyTo":"xmqq8rifp63p.fsf@gitster.g","subject":"Re: [PATCH 1/2] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-07T00:35:51Z","receivedAt":"2023-01-07T00:37:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 7/1/23 0:59, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n>> Thinking of this as a whole, perhaps after this series we can add:\n> \n> Why \"after\"? If we already know that the existing patches are\n> making things worse and need to fix the regression with a future\n> patch to make it usable again, why introduce a regression in the\n> first place?\n> \n\nI'm not sure if it is so worse, and if the optimization is a fix.\n\nWe're actually paying for worktrees twice in:\nreject_rebase_or_bisedt_branch() and\nreplace_each_worktree_head_symref().\n\nMaking the change this way makes more obvious IMHO what we are moving\nand why.\n\nStart moving the ref_exists() in 1/2, easily leads to 2/1 and this patch\nsquashed with 1/2, for little gain (IMHO) and worse history.\n\nThis is why I think it's a good sequence. But I understand your point\nand I'm not opposed to doing it as you suggest if you think the current\nway doesn't pay off.\n"},{"id":"470411","messageId":"34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com","threadId":"59018","inReplyTo":"ffd675e9-8a64-ae05-fc3b-36ae99092735@gmail.com","subject":"[PATCH v2 0/3] branch: operations on orphan branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-15T23:54:58Z","receivedAt":"2023-01-15T23:55:04Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Avoid some confusing errors operating on orphan branches when\nworking with worktrees.\n\nThanks.\n\n\n\nRubén Justo (3):\n branch: avoid unnecessary worktrees traversing\n branch: description for orphan branch errors\n branch: rename orphan branches in any worktree\n\n builtin/branch.c | 40 ++++++++++++++++++++--------------------\n t/t3200-branch.sh | 16 ++++++++++++++++\n t/t3202-show-branch.sh | 18 ++++++++++++++++++\n 3 files changed, 54 insertions(+), 20 deletions(-)\n\n-- \n2.39.0\n"},{"id":"470412","messageId":"bc82dd95-c968-146d-0dea-f82631b74765@gmail.com","threadId":"59018","inReplyTo":"34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com","subject":"[PATCH v2 1/3] avoid unnecessary worktrees traversing","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-16T00:00:10Z","receivedAt":"2023-01-16T00:00:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\"reject_rebase_or_bisect_branch()\" was introduced [1] to prevent a\nbranch under bisect or rebase from being renamed or copied. It\ntraverses all worktrees in the repository and dies if the specified\nbranch is being rebased or bisected in any them.\n\n\"replace_each_worktree_head_symref()\" was introduced [2] to adjust the\nHEAD in all worktrees after a branch rename succeeded. It traverses all\nworktrees in the repository and if any of them have their HEAD pointing\nto the renamed ref, it adjusts it.\n\nIf we could know in advance if the renamed branch is not HEAD in any\nworktree we could avoid calling \"replace_each_worktree_head_symref()\".\n\nLet's have \"reject_rebase_or_bisect_branch()\" check and return whether\nthe specified branch is HEAD in any worktree, and use this information\nto avoid unnecessary calls to \"replace_each_worktree_head_symref()\".\n\n 1.- 14ace5b (branch: do not rename a branch under bisect or rebase,\n 2016-04-22)\n\n 2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..a1ee728ca3 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -486,14 +486,17 @@ static void print_current_branch_name(void)\n \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n }\n \n-static void reject_rebase_or_bisect_branch(const char *target)\n+static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n {\n \tstruct worktree **worktrees = get_worktrees();\n-\tint i;\n+\tint i, ret = 0;\n \n \tfor (i = 0; worktrees[i]; i++) {\n \t\tstruct worktree *wt = worktrees[i];\n \n+\t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n+\t\t\tret = 1;\n+\n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n \n@@ -507,6 +510,7 @@ static void reject_rebase_or_bisect_branch(const char *target)\n \t}\n \n \tfree_worktrees(worktrees);\n+\treturn ret;\n }\n \n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n@@ -515,7 +519,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0;\n+\tint recovery = 0, ishead;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -544,7 +548,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\treject_rebase_or_bisect_branch(oldref.buf);\n+\tishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);\n \n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n@@ -574,7 +578,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\t\tinterpreted_oldname);\n \t}\n \n-\tif (!copy &&\n+\tif (!copy && ishead &&\n \t replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n \n-- \n2.39.0\n\n"},{"id":"470413","messageId":"1d866847-7535-8905-a8ad-a1feb158fa36@gmail.com","threadId":"59018","inReplyTo":"34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com","subject":"[PATCH v2 2/3] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-16T00:02:45Z","receivedAt":"2023-01-16T00:02:54Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bcfc82bd48 (branch: description for non-existent branch errors,\n2022-10-08) we check the current HEAD to detect if the branch to operate\nwith is an orphan branch, to avoid the confusing error: \"No branch\nnamed...\".\n\nIf we are asked to operate with an orphan branch in a different working\ntree than the current one, we need to check the HEAD in that different\nworking tree.\n\nLet's extend the check we did in bcfc82db48, to all HEADs in the\nrepository, using the helper introduced in 31ad6b61bd (branch: add\nbranch_checked_out() helper, 2022-06-15)\n\n\"ishead_reject_rebase_or_bised_branch()\" already returns whether the\nbranch to operate with is HEAD in any working tree in the repository.\nLet's use this information in \"copy_or_rename_branch()\", instead of the\nhelper.\n\nNote that we now call reject_rebase_or_bisect_branch() earlier, which\nintroduces a change in the error displayed when a combination of\nunsupported conditions occur simultaneously: the desired destination\nname is invalid, and the branch to operate on is being overrun or\nbisected. With \"foo\" being rebased or bisected, this:\n\n\t$ git branch -m foo HEAD\n\tfatal: 'HEAD' is not a valid branch name.\n\n... becomes:\n\n\t$ git branch -m foo HEAD\n\tfatal: Branch refs/heads/foo is being ...\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 25 +++++++++++--------------\n t/t3202-show-branch.sh | 18 ++++++++++++++++++\n 2 files changed, 29 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex a1ee728ca3..6bb5f50950 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -532,12 +532,12 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n-\t\tif (copy && !strcmp(head, oldname))\n-\t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n-\t\telse\n-\t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n-\t}\n+\tishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);\n+\n+\tif ((copy || !ishead) && !ref_exists(oldref.buf))\n+\t\tdie(ishead\n+\t\t ? _(\"No commit on branch '%s' yet.\")\n+\t\t : _(\"No branch named '%s'.\"), oldname);\n \n \t/*\n \t * A command like \"git branch -M currentbranch currentbranch\" cannot\n@@ -548,8 +548,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\tishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);\n-\n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n \t\tBUG(\"expected prefix missing for refs\");\n@@ -810,7 +808,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n-\t\t\terror((!argc || !strcmp(head, branch_name))\n+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n \t\t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t\t : _(\"No branch named '%s'.\"),\n \t\t\t branch_name);\n@@ -854,11 +852,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \t\t}\n \n-\t\tif (!ref_exists(branch->refname)) {\n-\t\t\tif (!argc || !strcmp(head, branch->name))\n-\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n-\t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n-\t\t}\n+\t\tif (!ref_exists(branch->refname))\n+\t\t\tdie((!argc || branch_checked_out(branch->refname))\n+\t\t\t ? _(\"No commit on branch '%s' yet.\")\n+\t\t\t : _(\"branch '%s' does not exist\"), branch->name);\n \n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..be20ebe1d5 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'error descriptions on orphan branch' '\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\tgit -C wt checkout --orphan orphan-branch &&\n+\ttest_branch_op_in_wt() {\n+\t\ttest_orphan_error() {\n+\t\t\ttest_must_fail git $* 2>actual &&\n+\t\t\ttest_i18ngrep \"No commit on branch .orphan-branch. yet.$\" actual\n+\t\t} &&\n+\t\ttest_orphan_error -C wt branch $1 $2 && # implicit branch\n+\t\ttest_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch\n+\t\ttest_orphan_error branch $1 orphan-branch $2 # different worktree\n+\t} &&\n+\ttest_branch_op_in_wt --edit-description &&\n+\ttest_branch_op_in_wt --set-upstream-to=ne &&\n+\ttest_branch_op_in_wt -c new-branch\n+'\n+\n test_done\n-- \n2.39.0\n\n"},{"id":"470414","messageId":"a47ff192-db67-dc4c-ded3-cd1e7c197726@gmail.com","threadId":"59018","inReplyTo":"34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com","subject":"[PATCH 3/3] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-16T00:04:26Z","receivedAt":"2023-01-16T00:04:35Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\nwe added support for renaming an orphan branch, skipping rename_ref() if\nthe branch to rename is an orphan branch, checking the current HEAD.\n\nBut the orphan branch to be renamed can be an orphan branch in a\ndifferent working tree than the current one, i.e. not the current HEAD.\n\nLet's make \"ishead_reject_rebase_or_bisect_branch()\" return a flag\nindicating if the returned value refers to an orphan branch, and use it\nto extend what we did in cfaff3aac, to all HEADs in the repository.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 5 ++---\n t/t3200-branch.sh | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6bb5f50950..7e6baa291a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -495,7 +495,7 @@ static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n \t\tstruct worktree *wt = worktrees[i];\n \n \t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n-\t\t\tret = 1;\n+\t\t\tret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);\n \n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n@@ -560,8 +560,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n-\tif (!copy &&\n-\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n+\tif (!copy && !(ishead > 1) &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a169b68d6..966583dc7d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,22 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -m should work with orphan branches' '\n+\ttest_when_finished git checkout - &&\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\n+\t# rename orphan in another worktreee\n+\tgit -C wt checkout --orphan orphan-foo-wt &&\n+\tgit branch -m orphan-foo-wt orphan-bar-wt &&\n+\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n+\n+\t# rename orphan in the current worktree\n+\tgit checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar &&\n+\ttest orphan-bar=$(git branch --show-current)\n+'\n+\n test_expect_success 'git branch -d on orphan HEAD (merged)' '\n \ttest_when_finished git checkout main &&\n \tgit checkout --orphan orphan &&\n-- \n2.39.0\n"},{"id":"470415","messageId":"4601f80b-e504-c5a2-6374-7a2609236a27@gmail.com","threadId":"59018","inReplyTo":"34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com","subject":"[PATCH v2 3/3] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-16T00:06:33Z","receivedAt":"2023-01-16T00:06:40Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\nwe added support for renaming an orphan branch, skipping rename_ref() if\nthe branch to rename is an orphan branch, checking the current HEAD.\n\nBut the orphan branch to be renamed can be an orphan branch in a\ndifferent working tree than the current one, i.e. not the current HEAD.\n\nLet's make \"ishead_reject_rebase_or_bisect_branch()\" return a flag\nindicating if the returned value refers to an orphan branch, and use it\nto extend what we did in cfaff3aac, to all HEADs in the repository.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 5 ++---\n t/t3200-branch.sh | 16 ++++++++++++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6bb5f50950..7e6baa291a 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -495,7 +495,7 @@ static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n \t\tstruct worktree *wt = worktrees[i];\n \n \t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n-\t\t\tret = 1;\n+\t\t\tret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);\n \n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n@@ -560,8 +560,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n-\tif (!copy &&\n-\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n+\tif (!copy && !(ishead > 1) &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a169b68d6..966583dc7d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,22 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -m should work with orphan branches' '\n+\ttest_when_finished git checkout - &&\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\n+\t# rename orphan in another worktreee\n+\tgit -C wt checkout --orphan orphan-foo-wt &&\n+\tgit branch -m orphan-foo-wt orphan-bar-wt &&\n+\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n+\n+\t# rename orphan in the current worktree\n+\tgit checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar &&\n+\ttest orphan-bar=$(git branch --show-current)\n+'\n+\n test_expect_success 'git branch -d on orphan HEAD (merged)' '\n \ttest_when_finished git checkout main &&\n \tgit checkout --orphan orphan &&\n-- \n2.39.0\n"},{"id":"470735","messageId":"xmqqilh21ale.fsf@gitster.g","threadId":"59018","inReplyTo":"bc82dd95-c968-146d-0dea-f82631b74765@gmail.com","subject":"Re: [PATCH v2 1/3] avoid unnecessary worktrees traversing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-19T21:24:45Z","receivedAt":"2023-01-19T21:31:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> -static void reject_rebase_or_bisect_branch(const char *target)\n> +static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n\nThe original name was already horrible but it became much worse by\nacquiring a non-word \"ishead\" as part of it X-<.\n\nAt least let's replace \"rebase or bisect\" with something a bit more\ngeneric, extensible, and shorter phrase. For example, isn't the\npoint of having the function was to give us a mechansim to see if\nthe branch with the given name is not to be modified because it is\nbeing worked on elsewhere? \"The branch is in use\" would be a good\nphrase to express such a concept, so die_if_branch_is_in_use() or\nsomething along that line may be easier to grok.\n"},{"id":"470736","messageId":"xmqqedrq1a7h.fsf@gitster.g","threadId":"59018","inReplyTo":"a47ff192-db67-dc4c-ded3-cd1e7c197726@gmail.com","subject":"Re: [PATCH 3/3] branch: rename orphan branches in any worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-19T21:33:06Z","receivedAt":"2023-01-19T21:51:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> +\tif (!copy && !(ishead > 1) &&\n\nLogically it might be necessary to be able to extend \"is that branch\nwhat we have checked out, yes or no?\" bool into something else that\ncan be something other than 0 or 1, but as soon as you did so,\n\"is_head\" is no longer a Boolean \"is it a HEAD, yes or no?\".\n\nNow what does that value really _mean_? Please rename the variable\nand helper function appropriately to make it clear what is going on.\n"},{"id":"470752","messageId":"f369f68e-5921-095f-e3e8-3d324d1688f4@gmail.com","threadId":"59018","inReplyTo":"xmqqilh21ale.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] avoid unnecessary worktrees traversing","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-19T23:26:59Z","receivedAt":"2023-01-19T23:27:11Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 19-ene-2023 13:24:45, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > -static void reject_rebase_or_bisect_branch(const char *target)\n> > +static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n> \n> The original name was already horrible but it became much worse by\n> acquiring a non-word \"ishead\" as part of it X-<.\n> \n> At least let's replace \"rebase or bisect\" with something a bit more\n> generic, extensible, and shorter phrase. For example, isn't the\n> point of having the function was to give us a mechansim to see if\n> the branch with the given name is not to be modified because it is\n> being worked on elsewhere? \"The branch is in use\" would be a good\n> phrase to express such a concept, so die_if_branch_is_in_use() or\n> something along that line may be easier to grok.\n\nI agree, the naming is ugly.\n\nThe idea is to return, if not die(), from the iteration we are doing in\nthat function, whether the branch is checked out in any worktree. That\ninformation allows us later, if we know in advance that no HEAD needs to\nbe adjusted, to avoid calling replace_each_worktree_head_symref(),\nsaving us a new and unnecessary traversal of the worktrees.\n\nThere is a second idea to, in next commits, return also if the branch\nis an unborn branch.\n\ndie_if_branch_is_in_use() is a better name for reject_rebase_or_... but\ndon't know how it fits with these ideas. I'm open to suggestions.\n\nI'll reroll with a better approach.\n\nThank you.\n"},{"id":"470753","messageId":"527f7315-be7b-7ec0-04fc-d07da7d4fefa@gmail.com","threadId":"59018","inReplyTo":"xmqqedrq1a7h.fsf@gitster.g","subject":"Re: [PATCH 3/3] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-01-19T23:34:13Z","receivedAt":"2023-01-19T23:34:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 19-ene-2023 13:33:06, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > +\tif (!copy && !(ishead > 1) &&\n> \n> Logically it might be necessary to be able to extend \"is that branch\n> what we have checked out, yes or no?\" bool into something else that\n> can be something other than 0 or 1, but as soon as you did so,\n> \"is_head\" is no longer a Boolean \"is it a HEAD, yes or no?\".\n> \n> Now what does that value really _mean_? Please rename the variable\n> and helper function appropriately to make it clear what is going on.\n\nThe idea is that an unborn branch needs to be a HEAD, so (head > 1)\ncodifies that information.\n\nAs I said in another reply in this thread, I'm going to reroll. I hope\nto make it clearer then.\n\nThank you.\n"},{"id":"471635","messageId":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","threadId":"59018","inReplyTo":"34a58449-4f2e-66ef-ea01-119186aebd23@gmail.com","subject":"[PATCH v3 0/3] branch: operations on orphan branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-06T23:01:27Z","receivedAt":"2023-02-06T23:02:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Avoid some confusing errors operating with orphan branches when\nworking with worktrees.\n\nChanges from v2:\n\n - Renamed \"ishead_and_reject_rebase_or_bisect_branch()\" to\n \"die_if_branch_is_being_rebased_or_bisected()\"\n\n A proposed name \"die_if_branch_is_is_use()\" has not been used because\n it could lead to confusion. We don't yet support copying or renaming\n a branch being rebased or bisected, but we do under other uses.\n\n - Comments added and variables renamed to make more clear the intention and\n the functioning.\n\n\nRubén Justo (3):\n branch: avoid unnecessary worktrees traversals\n branch: description for orphan branch errors\n branch: rename orphan branches in any worktree\n\n builtin/branch.c | 47 ++++++++++++++++++++++++------------------\n t/t3200-branch.sh | 14 +++++++++++++\n t/t3202-show-branch.sh | 18 ++++++++++++++++\n 3 files changed, 59 insertions(+), 20 deletions(-)\n\nRange-diff against v2:\n1: d16819bc61 ! 1: 50fa7ed076 avoid unnecessary worktrees traversing\n @@ Metadata\n Author: Rubén Justo \n \n ## Commit message ##\n - avoid unnecessary worktrees traversing\n + branch: avoid unnecessary worktrees traversals\n \n \"reject_rebase_or_bisect_branch()\" was introduced [1] to prevent a\n branch under bisect or rebase from being renamed or copied. It\n traverses all worktrees in the repository and dies if the specified\n - branch is being rebased or bisected in any them.\n + branch is being rebased or bisected in any of them.\n \n \"replace_each_worktree_head_symref()\" was introduced [2] to adjust the\n HEAD in all worktrees after a branch rename succeeded. It traverses all\n @@ Commit message\n to the renamed ref, it adjusts it.\n \n If we could know in advance if the renamed branch is not HEAD in any\n - worktree we could avoid calling \"replace_each_worktree_head_symref()\".\n + worktree we could avoid calling \"replace_each_worktree_head_symref()\",\n + and so avoid that unnecessary second traversing.\n \n - Let's have \"reject_rebase_or_bisect_branch()\" check and return whether\n - the specified branch is HEAD in any worktree, and use this information\n - to avoid unnecessary calls to \"replace_each_worktree_head_symref()\".\n + Let's rename \"reject_rebase_or_bisect_branch()\" to a more meaningful\n + name \"die_if_branch_is_being_rebased_or_bisected()\" and make it return,\n + if it does not die(), if the specified branch is HEAD in any worktree.\n + Use this new information to avoid unnecessary calls to\n + \"replace_each_worktree_head_symref()\".\n \n 1.- 14ace5b (branch: do not rename a branch under bisect or rebase,\n 2016-04-22)\n @@ builtin/branch.c: static void print_current_branch_name(void)\n }\n \n -static void reject_rebase_or_bisect_branch(const char *target)\n -+static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n ++/*\n ++ * Dies if the specified branch is being rebased or bisected. Otherwise returns\n ++ * 0 or, if the branch is HEAD in any worktree, returns 1.\n ++ */\n ++static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n {\n \tstruct worktree **worktrees = get_worktrees();\n -\tint i;\n @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n -\tint recovery = 0;\n -+\tint recovery = 0, ishead;\n ++\tint recovery = 0, oldref_is_head;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n -\treject_rebase_or_bisect_branch(oldref.buf);\n -+\tishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);\n ++\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n \n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c\n \t}\n \n -\tif (!copy &&\n -+\tif (!copy && ishead &&\n ++\tif (!copy && oldref_is_head &&\n \t replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n \n2: bc0ac43932 ! 2: ab277e5fcb branch: description for orphan branch errors\n @@ Commit message\n branch: description for orphan branch errors\n \n In bcfc82bd48 (branch: description for non-existent branch errors,\n - 2022-10-08) we check the current HEAD to detect if the branch to operate\n - with is an orphan branch, to avoid the confusing error: \"No branch\n - named...\".\n + 2022-10-08) we checked the current HEAD to detect if the branch to\n + operate with is an orphan branch, so as to avoid the confusing error:\n + \"No branch named...\".\n \n If we are asked to operate with an orphan branch in a different working\n tree than the current one, we need to check the HEAD in that different\n @@ Commit message\n repository, using the helper introduced in 31ad6b61bd (branch: add\n branch_checked_out() helper, 2022-06-15)\n \n - \"ishead_reject_rebase_or_bised_branch()\" already returns whether the\n - branch to operate with is HEAD in any working tree in the repository.\n + \"die_if_branch_is_being_rebased_or_bisected()\" already returns whether\n + the branch to operate with is HEAD in any worktree in the repository.\n Let's use this information in \"copy_or_rename_branch()\", instead of the\n helper.\n \n - Note that we now call reject_rebase_or_bisect_branch() earlier, which\n - introduces a change in the error displayed when a combination of\n - unsupported conditions occur simultaneously: the desired destination\n - name is invalid, and the branch to operate on is being overrun or\n - bisected. With \"foo\" being rebased or bisected, this:\n + Note that we now call \"die_if_branch_is_being_rebased_or_bisected()\"\n + earlier, which introduces a change in the error displayed when a\n + combination of unsupported conditions occur simultaneously: the desired\n + destination name is invalid, and the branch to operate with is being\n + rebased or bisected. i.e. with \"foo\" being rebased or bisected, this:\n \n - $ git branch -m foo HEAD\n - fatal: 'HEAD' is not a valid branch name.\n + $ git branch -m foo /\n + fatal: '/' is not a valid branch name.\n \n ... becomes:\n \n - $ git branch -m foo HEAD\n + $ git branch -m foo /\n fatal: Branch refs/heads/foo is being ...\n \n Signed-off-by: Rubén Justo \n @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c\n -\t\telse\n -\t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n -\t}\n -+\tishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);\n ++\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n +\n -+\tif ((copy || !ishead) && !ref_exists(oldref.buf))\n -+\t\tdie(ishead\n ++\tif ((copy || !oldref_is_head) && !ref_exists(oldref.buf))\n ++\t\tdie(oldref_is_head\n +\t\t ? _(\"No commit on branch '%s' yet.\")\n +\t\t : _(\"No branch named '%s'.\"), oldname);\n \n @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const c\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n --\tishead = ishead_and_reject_rebase_or_bisect_branch(oldref.buf);\n +-\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n -\n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n3: 29f8b6044d ! 3: 9742b4ff1f branch: rename orphan branches in any worktree\n @@ Commit message\n \n In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\n we added support for renaming an orphan branch, skipping rename_ref() if\n - the branch to rename is an orphan branch, checking the current HEAD.\n + the branch being renamed is an orphan branch; checking the current HEAD.\n \n But the orphan branch to be renamed can be an orphan branch in a\n - different working tree than the current one, i.e. not the current HEAD.\n + different worktree than the current one, i.e. not the current HEAD.\n \n - Let's make \"ishead_reject_rebase_or_bisect_branch()\" return a flag\n - indicating if the returned value refers to an orphan branch, and use it\n - to extend what we did in cfaff3aac, to all HEADs in the repository.\n + Let's make \"die_if_branch_is_being_rebased_or_bisected()\" return if the\n + specified branch is HEAD and orphan, and use it to extend what we did in\n + cfaff3aac, to check all HEADs in the repository.\n \n Signed-off-by: Rubén Justo \n \n ## builtin/branch.c ##\n -@@ builtin/branch.c: static int ishead_and_reject_rebase_or_bisect_branch(const char *target)\n +@@ builtin/branch.c: static void print_current_branch_name(void)\n + \n + /*\n + * Dies if the specified branch is being rebased or bisected. Otherwise returns\n +- * 0 or, if the branch is HEAD in any worktree, returns 1.\n ++ * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD\n ++ * and also orphan, returns 2.\n + */\n + static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n + {\n +@@ builtin/branch.c: static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n \t\tstruct worktree *wt = worktrees[i];\n \n \t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n -\t\t\tret = 1;\n -+\t\t\tret = 1 + (is_null_oid(&wt->head_oid) ? 1 : 0);\n ++\t\t\tret = is_null_oid(&wt->head_oid) ? 2 : 1;\n \n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n +@@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const char *newname, int\n + \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n + \tconst char *interpreted_oldname = NULL;\n + \tconst char *interpreted_newname = NULL;\n +-\tint recovery = 0, oldref_is_head;\n ++\tint recovery = 0, oldref_is_head, oldref_is_orphan;\n + \n + \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n + \t\t/*\n +@@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const char *newname, int\n + \t}\n + \n + \toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n ++\toldref_is_orphan = (oldref_is_head > 1);\n + \n +-\tif ((copy || !oldref_is_head) && !ref_exists(oldref.buf))\n +-\t\tdie(oldref_is_head\n ++\tif ((copy || !oldref_is_head) &&\n ++\t (oldref_is_orphan || !ref_exists(oldref.buf)))\n ++\t\tdie(oldref_is_orphan\n + \t\t ? _(\"No commit on branch '%s' yet.\")\n + \t\t : _(\"No branch named '%s'.\"), oldname);\n + \n @@ builtin/branch.c: static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n -\tif (!copy &&\n -\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n -+\tif (!copy && !(ishead > 1) &&\n ++\tif (!copy && !oldref_is_orphan &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n @@ t/t3200-branch.sh: test_expect_success 'git branch -M and -C fail on detached HE\n +\ttest_when_finished git checkout - &&\n +\ttest_when_finished git worktree remove -f wt &&\n +\tgit worktree add wt --detach &&\n -+\n +\t# rename orphan in another worktreee\n +\tgit -C wt checkout --orphan orphan-foo-wt &&\n +\tgit branch -m orphan-foo-wt orphan-bar-wt &&\n +\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n -+\n +\t# rename orphan in the current worktree\n +\tgit checkout --orphan orphan-foo &&\n +\tgit branch -m orphan-foo orphan-bar &&\n-- \n2.39.0\n"},{"id":"471637","messageId":"f284e163-5476-0c38-106c-094080340f71@gmail.com","threadId":"59018","inReplyTo":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","subject":"[PATCH v3 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-06T23:06:11Z","receivedAt":"2023-02-06T23:06:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\"reject_rebase_or_bisect_branch()\" was introduced [1] to prevent a\nbranch under bisect or rebase from being renamed or copied. It\ntraverses all worktrees in the repository and dies if the specified\nbranch is being rebased or bisected in any of them.\n\n\"replace_each_worktree_head_symref()\" was introduced [2] to adjust the\nHEAD in all worktrees after a branch rename succeeded. It traverses all\nworktrees in the repository and if any of them have their HEAD pointing\nto the renamed ref, it adjusts it.\n\nIf we could know in advance if the renamed branch is not HEAD in any\nworktree we could avoid calling \"replace_each_worktree_head_symref()\",\nand so avoid that unnecessary second traversing.\n\nLet's rename \"reject_rebase_or_bisect_branch()\" to a more meaningful\nname \"die_if_branch_is_being_rebased_or_bisected()\" and make it return,\nif it does not die(), if the specified branch is HEAD in any worktree.\nUse this new information to avoid unnecessary calls to\n\"replace_each_worktree_head_symref()\".\n\n 1.- 14ace5b (branch: do not rename a branch under bisect or rebase,\n 2016-04-22)\n\n 2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..89198fa5bf 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -486,14 +486,21 @@ static void print_current_branch_name(void)\n \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n }\n \n-static void reject_rebase_or_bisect_branch(const char *target)\n+/*\n+ * Dies if the specified branch is being rebased or bisected. Otherwise returns\n+ * 0 or, if the branch is HEAD in any worktree, returns 1.\n+ */\n+static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n {\n \tstruct worktree **worktrees = get_worktrees();\n-\tint i;\n+\tint i, ret = 0;\n \n \tfor (i = 0; worktrees[i]; i++) {\n \t\tstruct worktree *wt = worktrees[i];\n \n+\t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n+\t\t\tret = 1;\n+\n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n \n@@ -507,6 +514,7 @@ static void reject_rebase_or_bisect_branch(const char *target)\n \t}\n \n \tfree_worktrees(worktrees);\n+\treturn ret;\n }\n \n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n@@ -515,7 +523,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0;\n+\tint recovery = 0, oldref_is_head;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -544,7 +552,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\treject_rebase_or_bisect_branch(oldref.buf);\n+\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n \n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n@@ -574,7 +582,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\t\tinterpreted_oldname);\n \t}\n \n-\tif (!copy &&\n+\tif (!copy && oldref_is_head &&\n \t replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n \n-- \n2.39.0\n"},{"id":"471638","messageId":"6ba2caa4-1584-ac7f-c547-d7787be2930d@gmail.com","threadId":"59018","inReplyTo":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","subject":"[PATCH v3 2/3] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-06T23:06:24Z","receivedAt":"2023-02-06T23:06:32Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bcfc82bd48 (branch: description for non-existent branch errors,\n2022-10-08) we checked the current HEAD to detect if the branch to\noperate with is an orphan branch, so as to avoid the confusing error:\n\"No branch named...\".\n\nIf we are asked to operate with an orphan branch in a different working\ntree than the current one, we need to check the HEAD in that different\nworking tree.\n\nLet's extend the check we did in bcfc82db48, to all HEADs in the\nrepository, using the helper introduced in 31ad6b61bd (branch: add\nbranch_checked_out() helper, 2022-06-15)\n\n\"die_if_branch_is_being_rebased_or_bisected()\" already returns whether\nthe branch to operate with is HEAD in any worktree in the repository.\nLet's use this information in \"copy_or_rename_branch()\", instead of the\nhelper.\n\nNote that we now call \"die_if_branch_is_being_rebased_or_bisected()\"\nearlier, which introduces a change in the error displayed when a\ncombination of unsupported conditions occur simultaneously: the desired\ndestination name is invalid, and the branch to operate with is being\nrebased or bisected. i.e. with \"foo\" being rebased or bisected, this:\n\n\t$ git branch -m foo /\n\tfatal: '/' is not a valid branch name.\n\n... becomes:\n\n\t$ git branch -m foo /\n\tfatal: Branch refs/heads/foo is being ...\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 25 +++++++++++--------------\n t/t3202-show-branch.sh | 18 ++++++++++++++++++\n 2 files changed, 29 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 89198fa5bf..28cec344ad 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -536,12 +536,12 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n-\t\tif (copy && !strcmp(head, oldname))\n-\t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n-\t\telse\n-\t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n-\t}\n+\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n+\n+\tif ((copy || !oldref_is_head) && !ref_exists(oldref.buf))\n+\t\tdie(oldref_is_head\n+\t\t ? _(\"No commit on branch '%s' yet.\")\n+\t\t : _(\"No branch named '%s'.\"), oldname);\n \n \t/*\n \t * A command like \"git branch -M currentbranch currentbranch\" cannot\n@@ -552,8 +552,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n-\n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n \t\tBUG(\"expected prefix missing for refs\");\n@@ -814,7 +812,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n-\t\t\terror((!argc || !strcmp(head, branch_name))\n+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n \t\t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t\t : _(\"No branch named '%s'.\"),\n \t\t\t branch_name);\n@@ -858,11 +856,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \t\t}\n \n-\t\tif (!ref_exists(branch->refname)) {\n-\t\t\tif (!argc || !strcmp(head, branch->name))\n-\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n-\t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n-\t\t}\n+\t\tif (!ref_exists(branch->refname))\n+\t\t\tdie((!argc || branch_checked_out(branch->refname))\n+\t\t\t ? _(\"No commit on branch '%s' yet.\")\n+\t\t\t : _(\"branch '%s' does not exist\"), branch->name);\n \n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..be20ebe1d5 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'error descriptions on orphan branch' '\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\tgit -C wt checkout --orphan orphan-branch &&\n+\ttest_branch_op_in_wt() {\n+\t\ttest_orphan_error() {\n+\t\t\ttest_must_fail git $* 2>actual &&\n+\t\t\ttest_i18ngrep \"No commit on branch .orphan-branch. yet.$\" actual\n+\t\t} &&\n+\t\ttest_orphan_error -C wt branch $1 $2 && # implicit branch\n+\t\ttest_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch\n+\t\ttest_orphan_error branch $1 orphan-branch $2 # different worktree\n+\t} &&\n+\ttest_branch_op_in_wt --edit-description &&\n+\ttest_branch_op_in_wt --set-upstream-to=ne &&\n+\ttest_branch_op_in_wt -c new-branch\n+'\n+\n test_done\n-- \n2.39.0\n"},{"id":"471639","messageId":"ec027400-db8f-434d-2045-8a0ba0a35117@gmail.com","threadId":"59018","inReplyTo":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","subject":"[PATCH v3 3/3] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-06T23:06:39Z","receivedAt":"2023-02-06T23:06:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\nwe added support for renaming an orphan branch, skipping rename_ref() if\nthe branch being renamed is an orphan branch; checking the current HEAD.\n\nBut the orphan branch to be renamed can be an orphan branch in a\ndifferent worktree than the current one, i.e. not the current HEAD.\n\nLet's make \"die_if_branch_is_being_rebased_or_bisected()\" return if the\nspecified branch is HEAD and orphan, and use it to extend what we did in\ncfaff3aac, to check all HEADs in the repository.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 16 +++++++++-------\n t/t3200-branch.sh | 14 ++++++++++++++\n 2 files changed, 23 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 28cec344ad..7efda62224 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -488,7 +488,8 @@ static void print_current_branch_name(void)\n \n /*\n * Dies if the specified branch is being rebased or bisected. Otherwise returns\n- * 0 or, if the branch is HEAD in any worktree, returns 1.\n+ * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD\n+ * and also orphan, returns 2.\n */\n static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n {\n@@ -499,7 +500,7 @@ static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n \t\tstruct worktree *wt = worktrees[i];\n \n \t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n-\t\t\tret = 1;\n+\t\t\tret = is_null_oid(&wt->head_oid) ? 2 : 1;\n \n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n@@ -523,7 +524,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0, oldref_is_head;\n+\tint recovery = 0, oldref_is_head, oldref_is_orphan;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -537,9 +538,11 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t}\n \n \toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n+\toldref_is_orphan = (oldref_is_head > 1);\n \n-\tif ((copy || !oldref_is_head) && !ref_exists(oldref.buf))\n-\t\tdie(oldref_is_head\n+\tif ((copy || !oldref_is_head) &&\n+\t (oldref_is_orphan || !ref_exists(oldref.buf)))\n+\t\tdie(oldref_is_orphan\n \t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t : _(\"No branch named '%s'.\"), oldname);\n \n@@ -564,8 +567,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n-\tif (!copy &&\n-\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n+\tif (!copy && !oldref_is_orphan &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a169b68d6..5aef00efde 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -m should work with orphan branches' '\n+\ttest_when_finished git checkout - &&\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\t# rename orphan in another worktreee\n+\tgit -C wt checkout --orphan orphan-foo-wt &&\n+\tgit branch -m orphan-foo-wt orphan-bar-wt &&\n+\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n+\t# rename orphan in the current worktree\n+\tgit checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar &&\n+\ttest orphan-bar=$(git branch --show-current)\n+'\n+\n test_expect_success 'git branch -d on orphan HEAD (merged)' '\n \ttest_when_finished git checkout main &&\n \tgit checkout --orphan orphan &&\n-- \n2.39.0\n"},{"id":"471665","messageId":"xmqqy1pa1ghm.fsf@gitster.g","threadId":"59018","inReplyTo":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","subject":"Re: [PATCH v3 0/3] branch: operations on orphan branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-07T00:11:49Z","receivedAt":"2023-02-07T00:11:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Avoid some confusing errors operating with orphan branches when\n> working with worktrees.\n>\n> Changes from v2:\n>\n> - Renamed \"ishead_and_reject_rebase_or_bisect_branch()\" to\n> \"die_if_branch_is_being_rebased_or_bisected()\"\n>\n> A proposed name \"die_if_branch_is_is_use()\" has not been used because\n> it could lead to confusion. We don't yet support copying or renaming\n> a branch being rebased or bisected, but we do under other uses.\n\nQuite sensible.\n\n> - Comments added and variables renamed to make more clear the intention and\n> the functioning.\n\nOK.\n\nWill update what has been queued. Let's see if we get positive\nreviews on this round and start merging it down.\n\nThanks.\n"},{"id":"471671","messageId":"230207.86cz6l501v.gmgdl@evledraar.gmail.com","threadId":"59018","inReplyTo":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","subject":"Re: [PATCH v3 0/3] branch: operations on orphan branches","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-07T08:33:39Z","receivedAt":"2023-02-07T08:53:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 07 2023, Rubén Justo wrote:\n\n> Avoid some confusing errors operating with orphan branches when\n> working with worktrees.\n>\n> Changes from v2:\n>\n> - Renamed \"ishead_and_reject_rebase_or_bisect_branch()\" to\n> \"die_if_branch_is_being_rebased_or_bisected()\"\n\nLooking this over holistically, I think this is a great example of where\nfactoring something out into a function is just making readbility\nworse. This function is only used in copy_or_rename_branch(), and the\noverloaded name & semantics are making things quite confusing.\n\nWhereas if we just start by pulling it into its only caller I think this\ngets much better, at the end of this message is a diff-on-top these\nthree patches where I do that (I kept the \"target\" variable to minimize\nthe diff with the move detection, but we probalby want the strbuf\ndirectly instead).\n\n> A proposed name \"die_if_branch_is_is_use()\" has not been used because\n> it could lead to confusion. We don't yet support copying or renaming\n> a branch being rebased or bisected, but we do under other uses.\n\nAnother thing that I think could be improved in this series is if you\nskip the refactoring-while-at-it of changing the existing\n\"if/if/die/die\" into a \"if/die/?:\".\n\nIn the below diff I have that proposed change on top, but this snippet\nhere shows the diff to \"origin/master\":\n\t\n\t@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n\t \n\t \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n\t \t\tif (!ref_exists(branch_ref.buf))\n\t-\t\t\terror((!argc || !strcmp(head, branch_name))\n\t+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n\t \t\t\t ? _(\"No commit on branch '%s' yet.\")\n\t \t\t\t : _(\"No branch named '%s'.\"),\n\t \t\t\t branch_name);\n\t@@ -851,10 +851,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n\t \t\t}\n\t \n\t \t\tif (!ref_exists(branch->refname)) {\n\t-\t\t\tif (!argc || !strcmp(head, branch->name))\n\t+\t\t\tif (!argc || branch_checked_out(branch->refname))\n\t \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n\t \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n\t \t\t}\n\nI.e. your refactoring of this in 2/3 turns out to in the end have just\nbeen inflating the code change, for no functional benefit.\n\nI wouldn't mind if this were in some pre-cleanup, or if it actually made\nthe code easier to read, but IMO this pattern of using a ternary to\nselect the format to \"error\" or \"die\" makes things worse for\nreadability. It's a few bytes less code, but makes things harder to follow overall.\n\nAnd even if you disagree with that as far as the end state is concerned,\nI think it's unarguable that it makes the 2/3 harder to follow, since\nit's sticking a refactoring that's not neede dfor the end-goal here into\nan otherwise functional change.\n\nI'm aware that some of the code in the context uses this pattern, and\nyou probably changed the \"if\" block you modified to be consistent with\nthe code above, but I think in this case it's better not to follow the\nexisting style (which is used in that function, but is a rare exception\noverall in this codebase).\n\nThe diff-on-top, mentioned above:\n\n== BEGIN\n\t\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7efda622241..dc7a3e3dde1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -486,45 +486,16 @@ static void print_current_branch_name(void)\n \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n }\n \n-/*\n- * Dies if the specified branch is being rebased or bisected. Otherwise returns\n- * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD\n- * and also orphan, returns 2.\n- */\n-static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n-{\n-\tstruct worktree **worktrees = get_worktrees();\n-\tint i, ret = 0;\n-\n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct worktree *wt = worktrees[i];\n-\n-\t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n-\t\t\tret = is_null_oid(&wt->head_oid) ? 2 : 1;\n-\n-\t\tif (!wt->is_detached)\n-\t\t\tcontinue;\n-\n-\t\tif (is_worktree_being_rebased(wt, target))\n-\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n-\t\t\t target, wt->path);\n-\n-\t\tif (is_worktree_being_bisected(wt, target))\n-\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n-\t\t\t target, wt->path);\n-\t}\n-\n-\tfree_worktrees(worktrees);\n-\treturn ret;\n-}\n-\n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0, oldref_is_head, oldref_is_orphan;\n+\tint recovery = 0, oldref_is_head = 0, oldref_is_orphan = 0;\n+\tstruct worktree **worktrees;\n+\tint i;\n+\tconst char *target;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -537,8 +508,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n-\toldref_is_orphan = (oldref_is_head > 1);\n+\tworktrees = get_worktrees();\n+\ttarget = oldref.buf;\n+\tfor (i = 0; worktrees[i]; i++) {\n+\t\tstruct worktree *wt = worktrees[i];\n+\n+\t\tif (wt->head_ref && !strcmp(target, wt->head_ref)) {\n+\t\t\toldref_is_head = 1;\n+\t\t\tif (is_null_oid(&wt->head_oid))\n+\t\t\t\toldref_is_orphan = 1;\n+\t\t}\n+\n+\t\tif (!wt->is_detached)\n+\t\t\tcontinue;\n+\n+\t\tif (is_worktree_being_rebased(wt, target))\n+\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n+\t\t\t target, wt->path);\n+\n+\t\tif (is_worktree_being_bisected(wt, target))\n+\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n+\t\t\t target, wt->path);\n+\t}\n+\tfree_worktrees(worktrees);\n \n \tif ((copy || !oldref_is_head) &&\n \t (oldref_is_orphan || !ref_exists(oldref.buf)))\n@@ -858,10 +850,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \t\t}\n \n-\t\tif (!ref_exists(branch->refname))\n-\t\t\tdie((!argc || branch_checked_out(branch->refname))\n-\t\t\t ? _(\"No commit on branch '%s' yet.\")\n-\t\t\t : _(\"branch '%s' does not exist\"), branch->name);\n+\t\tif (!ref_exists(branch->refname)) {\n+\t\t\tif (!argc || branch_checked_out(branch->refname))\n+\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n+\t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n+\t\t}\n+\t\t\n \n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n\n== END\n\nP.S. if I were refactoring those ?: for style in that function I'd\nprobably go for this on-top. The N_() followed by _() pattern is\nprobably overdoing it, but included to show that one way out of this\nsort of thing with i18n is that you can pre-mark the string with N_(),\nthen use it with _() to emit the message (right now the code uses\n\"copy?\" over \"copy ?\" instead to align them):\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex dc7a3e3dde1..e42f9bc4900 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -805,31 +805,35 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n-\t\tif (!ref_exists(branch_ref.buf))\n-\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n-\t\t\t ? _(\"No commit on branch '%s' yet.\")\n-\t\t\t : _(\"No branch named '%s'.\"),\n-\t\t\t branch_name);\n-\t\telse if (!edit_branch_description(branch_name))\n+\t\tif (!ref_exists(branch_ref.buf)) {\n+\t\t\tif (!argc || branch_checked_out(branch_ref.buf))\n+\t\t\t\terror(_(\"No commit on branch '%s' yet.\"),\n+\t\t\t\t branch_name);\n+\t\t\telse\n+\t\t\t\terror(_(\"No branch named '%s'.\"), branch_name);\n+\t\t} else if (!edit_branch_description(branch_name)) {\n \t\t\tret = 0; /* happy */\n+\t\t}\n \n \t\tstrbuf_release(&branch_ref);\n \t\tstrbuf_release(&buf);\n \n \t\treturn ret;\n \t} else if (copy || rename) {\n+\t\tstatic const char *cannot_copy = N_(\"cannot copy the current branch while not on any.\");\n+\t\tstatic const char *cannot_rename = N_(\"cannot rename the current branch while not on any.\");\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n \t\telse if ((argc == 1) && filter.detached)\n-\t\t\tdie(copy? _(\"cannot copy the current branch while not on any.\")\n-\t\t\t\t: _(\"cannot rename the current branch while not on any.\"));\n+\t\t\tdie(\"%s\", copy ? _(cannot_copy) : _(cannot_rename));\n \t\telse if (argc == 1)\n \t\t\tcopy_or_rename_branch(head, argv[0], copy, copy + rename > 1);\n \t\telse if (argc == 2)\n \t\t\tcopy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);\n+\t\telse if (copy)\n+\t\t\tdie(_(\"too many branches for a copy operation\"));\n \t\telse\n-\t\t\tdie(copy? _(\"too many branches for a copy operation\")\n-\t\t\t\t: _(\"too many arguments for a rename operation\"));\n+\t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n \t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n"},{"id":"471745","messageId":"1450c94f-b0e4-a424-edd4-8c88b72febf5@gmail.com","threadId":"59018","inReplyTo":"230207.86cz6l501v.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/3] branch: operations on orphan branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-08T00:35:26Z","receivedAt":"2023-02-08T00:35:34Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 07-feb-2023 09:33:39, Ævar Arnfjörð Bjarmason wrote:\n\nThank you reviewing this.\n\n> \n> On Tue, Feb 07 2023, Rubén Justo wrote:\n> \n> > Avoid some confusing errors operating with orphan branches when\n> > working with worktrees.\n> >\n> > Changes from v2:\n> >\n> > - Renamed \"ishead_and_reject_rebase_or_bisect_branch()\" to\n> > \"die_if_branch_is_being_rebased_or_bisected()\"\n> \n> Looking this over holistically, I think this is a great example of where\n> factoring something out into a function is just making readbility\n> worse. This function is only used in copy_or_rename_branch(), and the\n> overloaded name & semantics are making things quite confusing.\n> \n> Whereas if we just start by pulling it into its only caller I think this\n> gets much better, at the end of this message is a diff-on-top these\n> three patches where I do that (I kept the \"target\" variable to minimize\n> the diff with the move detection, but we probalby want the strbuf\n> directly instead).\n\nI'm sorry, but I don't agree. copy_or_rename_branch() already does some\nheterogeneous things. IMHO, making it also iterate worktrees makes\nthings worse, in terms of readability. I could agree with maybe the\nfunction die_if_branch_is_being_rebased_or_bisected() could be part of a\nmore general use, maybe something in worktree.h.\n \n> > A proposed name \"die_if_branch_is_is_use()\" has not been used because\n> > it could lead to confusion. We don't yet support copying or renaming\n> > a branch being rebased or bisected, but we do under other uses.\n> \n> Another thing that I think could be improved in this series is if you\n> skip the refactoring-while-at-it of changing the existing\n> \"if/if/die/die\" into a \"if/die/?:\".\n\nI'm sorry, but I don't agree with this reasoning either. The ternary op\nhere allows the code to be more clear, IMHO, in the intention: there is\nno conditional die() or error(), the conditional is for the message.\n\n> \n> In the below diff I have that proposed change on top, but this snippet\n> here shows the diff to \"origin/master\":\n> \t\n> \t@@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t \n> \t \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n> \t \t\tif (!ref_exists(branch_ref.buf))\n> \t-\t\t\terror((!argc || !strcmp(head, branch_name))\n> \t+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n> \t \t\t\t ? _(\"No commit on branch '%s' yet.\")\n> \t \t\t\t : _(\"No branch named '%s'.\"),\n> \t \t\t\t branch_name);\n> \t@@ -851,10 +851,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t \t\t}\n> \t \n> \t \t\tif (!ref_exists(branch->refname)) {\n> \t-\t\t\tif (!argc || !strcmp(head, branch->name))\n> \t+\t\t\tif (!argc || branch_checked_out(branch->refname))\n> \t \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n> \t \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n> \t \t\t}\n> \n> I.e. your refactoring of this in 2/3 turns out to in the end have just\n> been inflating the code change, for no functional benefit.\n> \n> I wouldn't mind if this were in some pre-cleanup, or if it actually made\n> the code easier to read, but IMO this pattern of using a ternary to\n> select the format to \"error\" or \"die\" makes things worse for\n> readability. It's a few bytes less code, but makes things harder to follow overall.\n> \n> And even if you disagree with that as far as the end state is concerned,\n> I think it's unarguable that it makes the 2/3 harder to follow, since\n> it's sticking a refactoring that's not neede dfor the end-goal here into\n> an otherwise functional change.\n> \n> I'm aware that some of the code in the context uses this pattern, and\n> you probably changed the \"if\" block you modified to be consistent with\n> the code above, but I think in this case it's better not to follow the\n> existing style (which is used in that function, but is a rare exception\n> overall in this codebase).\n> \n> The diff-on-top, mentioned above:\n> \n> == BEGIN\n> \t\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 7efda622241..dc7a3e3dde1 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -486,45 +486,16 @@ static void print_current_branch_name(void)\n> \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n> }\n> \n> -/*\n> - * Dies if the specified branch is being rebased or bisected. Otherwise returns\n> - * 0 or, if the branch is HEAD in any worktree, returns 1. If the branch is HEAD\n> - * and also orphan, returns 2.\n> - */\n> -static int die_if_branch_is_being_rebased_or_bisected(const char *target)\n> -{\n> -\tstruct worktree **worktrees = get_worktrees();\n> -\tint i, ret = 0;\n> -\n> -\tfor (i = 0; worktrees[i]; i++) {\n> -\t\tstruct worktree *wt = worktrees[i];\n> -\n> -\t\tif (wt->head_ref && !strcmp(target, wt->head_ref))\n> -\t\t\tret = is_null_oid(&wt->head_oid) ? 2 : 1;\n> -\n> -\t\tif (!wt->is_detached)\n> -\t\t\tcontinue;\n> -\n> -\t\tif (is_worktree_being_rebased(wt, target))\n> -\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n> -\t\t\t target, wt->path);\n> -\n> -\t\tif (is_worktree_being_bisected(wt, target))\n> -\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n> -\t\t\t target, wt->path);\n> -\t}\n> -\n> -\tfree_worktrees(worktrees);\n> -\treturn ret;\n> -}\n> -\n> static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n> {\n> \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n> \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n> \tconst char *interpreted_oldname = NULL;\n> \tconst char *interpreted_newname = NULL;\n> -\tint recovery = 0, oldref_is_head, oldref_is_orphan;\n> +\tint recovery = 0, oldref_is_head = 0, oldref_is_orphan = 0;\n> +\tstruct worktree **worktrees;\n> +\tint i;\n> +\tconst char *target;\n> \n> \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n> \t\t/*\n> @@ -537,8 +508,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n> \t}\n> \n> -\toldref_is_head = die_if_branch_is_being_rebased_or_bisected(oldref.buf);\n> -\toldref_is_orphan = (oldref_is_head > 1);\n> +\tworktrees = get_worktrees();\n> +\ttarget = oldref.buf;\n> +\tfor (i = 0; worktrees[i]; i++) {\n> +\t\tstruct worktree *wt = worktrees[i];\n> +\n> +\t\tif (wt->head_ref && !strcmp(target, wt->head_ref)) {\n> +\t\t\toldref_is_head = 1;\n> +\t\t\tif (is_null_oid(&wt->head_oid))\n> +\t\t\t\toldref_is_orphan = 1;\n> +\t\t}\n> +\n> +\t\tif (!wt->is_detached)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (is_worktree_being_rebased(wt, target))\n> +\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n> +\t\t\t target, wt->path);\n> +\n> +\t\tif (is_worktree_being_bisected(wt, target))\n> +\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n> +\t\t\t target, wt->path);\n> +\t}\n> +\tfree_worktrees(worktrees);\n> \n> \tif ((copy || !oldref_is_head) &&\n> \t (oldref_is_orphan || !ref_exists(oldref.buf)))\n> @@ -858,10 +850,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n> \t\t}\n> \n> -\t\tif (!ref_exists(branch->refname))\n> -\t\t\tdie((!argc || branch_checked_out(branch->refname))\n> -\t\t\t ? _(\"No commit on branch '%s' yet.\")\n> -\t\t\t : _(\"branch '%s' does not exist\"), branch->name);\n> +\t\tif (!ref_exists(branch->refname)) {\n> +\t\t\tif (!argc || branch_checked_out(branch->refname))\n> +\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n> +\t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n> +\t\t}\n> +\t\t\n> \n> \t\tdwim_and_setup_tracking(the_repository, branch->name,\n> \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n> \n> == END\n> \n> P.S. if I were refactoring those ?: for style in that function I'd\n> probably go for this on-top. The N_() followed by _() pattern is\n> probably overdoing it, but included to show that one way out of this\n> sort of thing with i18n is that you can pre-mark the string with N_(),\n> then use it with _() to emit the message (right now the code uses\n> \"copy?\" over \"copy ?\" instead to align them):\n> \n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index dc7a3e3dde1..e42f9bc4900 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -805,31 +805,35 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \n> \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n> -\t\tif (!ref_exists(branch_ref.buf))\n> -\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n> -\t\t\t ? _(\"No commit on branch '%s' yet.\")\n> -\t\t\t : _(\"No branch named '%s'.\"),\n> -\t\t\t branch_name);\n> -\t\telse if (!edit_branch_description(branch_name))\n> +\t\tif (!ref_exists(branch_ref.buf)) {\n> +\t\t\tif (!argc || branch_checked_out(branch_ref.buf))\n> +\t\t\t\terror(_(\"No commit on branch '%s' yet.\"),\n> +\t\t\t\t branch_name);\n> +\t\t\telse\n> +\t\t\t\terror(_(\"No branch named '%s'.\"), branch_name);\n> +\t\t} else if (!edit_branch_description(branch_name)) {\n> \t\t\tret = 0; /* happy */\n> +\t\t}\n> \n> \t\tstrbuf_release(&branch_ref);\n> \t\tstrbuf_release(&buf);\n> \n> \t\treturn ret;\n> \t} else if (copy || rename) {\n> +\t\tstatic const char *cannot_copy = N_(\"cannot copy the current branch while not on any.\");\n> +\t\tstatic const char *cannot_rename = N_(\"cannot rename the current branch while not on any.\");\n> \t\tif (!argc)\n> \t\t\tdie(_(\"branch name required\"));\n> \t\telse if ((argc == 1) && filter.detached)\n> -\t\t\tdie(copy? _(\"cannot copy the current branch while not on any.\")\n> -\t\t\t\t: _(\"cannot rename the current branch while not on any.\"));\n> +\t\t\tdie(\"%s\", copy ? _(cannot_copy) : _(cannot_rename));\n> \t\telse if (argc == 1)\n> \t\t\tcopy_or_rename_branch(head, argv[0], copy, copy + rename > 1);\n> \t\telse if (argc == 2)\n> \t\t\tcopy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);\n> +\t\telse if (copy)\n> +\t\t\tdie(_(\"too many branches for a copy operation\"));\n> \t\telse\n> -\t\t\tdie(copy? _(\"too many branches for a copy operation\")\n> -\t\t\t\t: _(\"too many arguments for a rename operation\"));\n> +\t\t\tdie(_(\"too many arguments for a rename operation\"));\n> \t} else if (new_upstream) {\n> \t\tstruct branch *branch;\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \n"},{"id":"471786","messageId":"xmqqa61oroko.fsf@gitster.g","threadId":"59018","inReplyTo":"230207.86cz6l501v.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 0/3] branch: operations on orphan branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-08T18:37:11Z","receivedAt":"2023-02-08T18:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> - Renamed \"ishead_and_reject_rebase_or_bisect_branch()\" to\n>> \"die_if_branch_is_being_rebased_or_bisected()\"\n>\n> Looking this over holistically, I think this is a great example of where\n> factoring something out into a function is just making readbility\n> worse. This function is only used in copy_or_rename_branch(), and the\n> overloaded name & semantics are making things quite confusing.\n>\n> Whereas if we just start by pulling it into its only caller I think this\n> gets much better,...\n\nHmph, I hadn't considered it, but with only a single caller that\nbecomes a viable alternative.\n\n> Another thing that I think could be improved in this series is if you\n> skip the refactoring-while-at-it of changing the existing\n> \"if/if/die/die\" into a \"if/die/?:\".\n> ...\n> I.e. your refactoring of this in 2/3 turns out to in the end have just\n> been inflating the code change, for no functional benefit.\n>\n> I wouldn't mind if this were in some pre-cleanup, or if it actually made\n> the code easier to read, but IMO this pattern of using a ternary to\n> select the format to \"error\" or \"die\" makes things worse for\n> readability. It's a few bytes less code, but makes things harder to follow overall.\n\nGood.\n\nThanks for carefully reading.\n"},{"id":"471971","messageId":"20230211041644.1848341-1-jonathantanmy@google.com","threadId":"59018","inReplyTo":"f284e163-5476-0c38-106c-094080340f71@gmail.com","subject":"Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-02-11T04:16:44Z","receivedAt":"2023-02-11T04:16:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Rubén Justo writes:\n> \"reject_rebase_or_bisect_branch()\" was introduced [1] to prevent a\n> branch under bisect or rebase from being renamed or copied. It\n> traverses all worktrees in the repository and dies if the specified\n> branch is being rebased or bisected in any of them.\n> \n> \"replace_each_worktree_head_symref()\" was introduced [2] to adjust the\n> HEAD in all worktrees after a branch rename succeeded. It traverses all\n> worktrees in the repository and if any of them have their HEAD pointing\n> to the renamed ref, it adjusts it.\n\nThanks for the references to why these were introduced!\n\n> If we could know in advance if the renamed branch is not HEAD in any\n> worktree we could avoid calling \"replace_each_worktree_head_symref()\",\n> and so avoid that unnecessary second traversing.\n\nWhen I first read this paragraph, I thought that the traversing involved\nwas just a loop through an in-memory data structure, which is not that\ncostly. It turns out that a travesal involves not only constructing\nsaid data structure but also reading from disk to get the necessary\ninformation, which indeed is very costly. I would include that in the\ncommit message, but won't insist on that (perhaps it's clear to others\nwhat is meant by traversal).\n\n> Let's rename \"reject_rebase_or_bisect_branch()\" to a more meaningful\n> name \"die_if_branch_is_being_rebased_or_bisected()\" and make it return,\n> if it does not die(), if the specified branch is HEAD in any worktree.\n> Use this new information to avoid unnecessary calls to\n> \"replace_each_worktree_head_symref()\".\n\nIn later patches, I see that the return value can also indicate that a\nbranch is an orphan, and that for the sake of code clarity, the calling\nfunction had to have a variable assignment of the form oldref_is_orphan\n= (oldref_is_head > 1). If this is so, it is probably better to have\nthis function return something with names. So something like\n\n #define IS_HEAD 4\n #define IS_ORPHAN 8\n int get_branch_usage_in_worktrees(...) {...}\n\nand then the caller can use these constants whenever it needs to know\nwhat kind of branch this is.\n\nI also see in patch 2 that we're changing what the user sees under\ncertain inputs. That can be avoided if we move the dying to the caller,\nand have this function merely return when the branch is being rebased\nor bisected.\n\n #define IS_BISECTED 1\n #define IS_REBASED 2\n\nor something like that. I would prefer if user-visible behavior didn't\nchange unnecessarily, and this does not seem like a necessary case.\n\nOther than that, everything looks good.\n"},{"id":"472166","messageId":"b9464970-1ba5-e2f3-58c6-145ca45b1095@gmail.com","threadId":"59018","inReplyTo":"20230211041644.1848341-1-jonathantanmy@google.com","subject":"Re: [PATCH v3 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-15T22:00:39Z","receivedAt":"2023-02-15T22:00:53Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 10-feb-2023 20:16:44, Jonathan Tan wrote:\n\n> > If we could know in advance if the renamed branch is not HEAD in any\n> > worktree we could avoid calling \"replace_each_worktree_head_symref()\",\n> > and so avoid that unnecessary second traversing.\n> \n> When I first read this paragraph, I thought that the traversing involved\n> was just a loop through an in-memory data structure, which is not that\n> costly. It turns out that a travesal involves not only constructing\n> said data structure but also reading from disk to get the necessary\n> information, which indeed is very costly. I would include that in the\n> commit message, but won't insist on that (perhaps it's clear to others\n> what is meant by traversal).\n\nSorry, I should have included details about why it's costly. I'll\ninclude some in the message.\n\n> \n> > Let's rename \"reject_rebase_or_bisect_branch()\" to a more meaningful\n> > name \"die_if_branch_is_being_rebased_or_bisected()\" and make it return,\n> > if it does not die(), if the specified branch is HEAD in any worktree.\n> > Use this new information to avoid unnecessary calls to\n> > \"replace_each_worktree_head_symref()\".\n> \n> In later patches, I see that the return value can also indicate that a\n> branch is an orphan, and that for the sake of code clarity, the calling\n> function had to have a variable assignment of the form oldref_is_orphan\n> = (oldref_is_head > 1). If this is so, it is probably better to have\n> this function return something with names. So something like\n> \n> #define IS_HEAD 4\n> #define IS_ORPHAN 8\n\nOK. I'll use names.\n\n> int get_branch_usage_in_worktrees(...) {...}\n> \n> and then the caller can use these constants whenever it needs to know\n> what kind of branch this is.\n> \n> I also see in patch 2 that we're changing what the user sees under\n> certain inputs. That can be avoided if we move the dying to the caller,\n> and have this function merely return when the branch is being rebased\n> or bisected.\n> \n> #define IS_BISECTED 1\n> #define IS_REBASED 2\n> \n> or something like that. I would prefer if user-visible behavior didn't\n> change unnecessarily, and this does not seem like a necessary case.\n\nOK.\n\n> \n> Other than that, everything looks good.\n\nThanks for your review and suggestions!\n"},{"id":"472458","messageId":"139555f1-21ab-d987-a58e-c7f04ce222d3@gmail.com","threadId":"59018","inReplyTo":"2193a4ed-b263-068e-92f8-847dcb053f8c@gmail.com","subject":"[PATCH v4 0/3] branch: operations on orphan branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-22T22:50:01Z","receivedAt":"2023-02-22T22:50:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Avoid some confusing errors operating with orphan branches when working\nwith worktrees.\n\nChanges from v3:\n\nThis is a major refactoring based on the comments from the reviews of\nv3. The intention is to make no functional changes in this iteration,\nexcept a change to maintain unchanged the user-visible behavior, as\nJonathan suggested.\n\nSummary of changes:\n\n - Inlined reject_rebase_or_bisect_branch() and\n replace_each_head_symref() into copy_or_rename_branch(). \n\n - Used names to define HEAD states.\n\n - Moved die(\"Branch foo is being rebased/bisected...\") after the call\n to validate_branchname(), to maintain the user-visible behavior.\n\n - Removed the use of ternary operators to avoid introducing unneeded\n noise in the patch.\n \n\nRubén Justo (3):\n branch: avoid unnecessary worktrees traversals\n branch: description for orphan branch errors\n branch: rename orphan branches in any worktree\n\n branch.c | 27 ------------\n branch.h | 8 ----\n builtin/branch.c | 99 ++++++++++++++++++++++++++++--------------\n t/t3200-branch.sh | 14 ++++++\n t/t3202-show-branch.sh | 18 ++++++++\n 5 files changed, 98 insertions(+), 68 deletions(-)\n\n-- \n2.34.1\n\n"},{"id":"472459","messageId":"50dd7c7a-5656-e010-1c0b-819a40a1f1a0@gmail.com","threadId":"59018","inReplyTo":"139555f1-21ab-d987-a58e-c7f04ce222d3@gmail.com","subject":"[PATCH v4 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-22T22:52:51Z","receivedAt":"2023-02-22T22:53:12Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\"reject_rebase_or_bisect_branch()\" was introduced [1] to prevent a\nbranch under bisect or rebase from being renamed or copied. It\ntraverses all worktrees in the repository and dies if the specified\nbranch is being rebased or bisected in any of them.\n\n\"replace_each_worktree_head_symref()\" was introduced [2] to adjust the\nHEAD in all worktrees after a branch rename succeeded. It traverses all\nworktrees in the repository and if any of them have their HEAD pointing\nto the renamed ref, it adjusts it.\n\nConsidering that both functions are only called from within\ncopy_or_rename_branch() and each of them traverses all worktrees, which\nmight involve disk access and resolving multiple references, inlining\nthese two functions to do the traversing once, makes sense.\n\n 1.- 14ace5b (branch: do not rename a branch under bisect or rebase,\n 2016-04-22)\n\n 2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 27 --------------------\n branch.h | 8 ------\n builtin/branch.c | 64 ++++++++++++++++++++++++++++--------------------\n 3 files changed, 38 insertions(+), 61 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex e5614b53b3..f64062be71 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -830,30 +830,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n \n \tfree_worktrees(worktrees);\n }\n-\n-int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n-\t\t\t\t const char *logmsg)\n-{\n-\tint ret = 0;\n-\tstruct worktree **worktrees = get_worktrees();\n-\tint i;\n-\n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct ref_store *refs;\n-\n-\t\tif (worktrees[i]->is_detached)\n-\t\t\tcontinue;\n-\t\tif (!worktrees[i]->head_ref)\n-\t\t\tcontinue;\n-\t\tif (strcmp(oldref, worktrees[i]->head_ref))\n-\t\t\tcontinue;\n-\n-\t\trefs = get_worktree_ref_store(worktrees[i]);\n-\t\tif (refs_create_symref(refs, \"HEAD\", newref, logmsg))\n-\t\t\tret = error(_(\"HEAD of working tree %s is not updated\"),\n-\t\t\t\t worktrees[i]->path);\n-\t}\n-\n-\tfree_worktrees(worktrees);\n-\treturn ret;\n-}\ndiff --git a/branch.h b/branch.h\nindex ef56103c05..30c01aed73 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);\n */\n void die_if_checked_out(const char *branch, int ignore_current_worktree);\n \n-/*\n- * Update all per-worktree HEADs pointing at the old ref to point the new ref.\n- * This will be used when renaming a branch. Returns 0 if successful, non-zero\n- * otherwise.\n- */\n-int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n-\t\t\t\t const char *logmsg);\n-\n #endif\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..a32ae64006 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -486,28 +486,6 @@ static void print_current_branch_name(void)\n \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n }\n \n-static void reject_rebase_or_bisect_branch(const char *target)\n-{\n-\tstruct worktree **worktrees = get_worktrees();\n-\tint i;\n-\n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct worktree *wt = worktrees[i];\n-\n-\t\tif (!wt->is_detached)\n-\t\t\tcontinue;\n-\n-\t\tif (is_worktree_being_rebased(wt, target))\n-\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n-\t\t\t target, wt->path);\n-\n-\t\tif (is_worktree_being_bisected(wt, target))\n-\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n-\t\t\t target, wt->path);\n-\t}\n-\n-\tfree_worktrees(worktrees);\n-}\n \n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n@@ -516,6 +494,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n+\tstruct worktree **worktrees = get_worktrees();\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -544,7 +523,20 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\treject_rebase_or_bisect_branch(oldref.buf);\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tstruct worktree *wt = worktrees[i];\n+\n+\t\tif (!wt->is_detached)\n+\t\t\tcontinue;\n+\n+\t\tif (is_worktree_being_rebased(wt, oldref.buf))\n+\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n+\t\t\t oldref.buf, wt->path);\n+\n+\t\tif (is_worktree_being_bisected(wt, oldref.buf))\n+\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n+\t\t\t oldref.buf, wt->path);\n+\t}\n \n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n@@ -574,9 +566,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\t\tinterpreted_oldname);\n \t}\n \n-\tif (!copy &&\n-\t replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n-\t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n+\tif (!copy) {\n+\t\t/*\n+\t\t * Update all per-worktree HEADs pointing at the old ref to\n+\t\t * point the new ref.\n+\t\t */\n+\t\tfor (int i = 0; worktrees[i]; i++) {\n+\t\t\tstruct ref_store *refs;\n+\n+\t\t\tif (worktrees[i]->is_detached)\n+\t\t\t\tcontinue;\n+\t\t\tif (!worktrees[i]->head_ref)\n+\t\t\t\tcontinue;\n+\t\t\tif (strcmp(oldref.buf, worktrees[i]->head_ref))\n+\t\t\t\tcontinue;\n+\n+\t\t\trefs = get_worktree_ref_store(worktrees[i]);\n+\t\t\tif (refs_create_symref(refs, \"HEAD\", newref.buf, logmsg.buf))\n+\t\t\t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"),\n+\t\t\t\t\tnewname);\n+\t\t}\n+\t}\n+\n+\tfree_worktrees(worktrees);\n \n \tstrbuf_release(&logmsg);\n \n-- \n2.34.1\n"},{"id":"472460","messageId":"76bac570-147e-7c74-c18c-1da88bc3d342@gmail.com","threadId":"59018","inReplyTo":"139555f1-21ab-d987-a58e-c7f04ce222d3@gmail.com","subject":"[PATCH v4 2/3] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-22T22:55:02Z","receivedAt":"2023-02-22T22:55:17Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bcfc82bd48 (branch: description for non-existent branch errors,\n2022-10-08) we checked the current HEAD to detect if the branch to\noperate with is an orphan branch, so as to avoid the confusing error:\n\"No branch named...\".\n\nIf we are asked to operate with an orphan branch in a different working\ntree than the current one, we need to check the HEAD in that different\nworking tree.\n\nLet's extend the check we did in bcfc82bd48, to all HEADs in the\nrepository, using the helper introduced in 31ad6b61bd (branch: add\nbranch_checked_out() helper, 2022-06-15)\n\nWe are already traversing all worktrees in \"copy_or_rename_branch()\"\nchecking if the branch to be copied or renamed is being bisected or\nrebased. Let's include also a check for being HEAD, and use this\ninformation within the function rather than the helper. This implies\ndoing the worktrees iteration earlier in the function; to keep the\nuser-visible behavior unchanged lets maintain the die(\"Branch foo is\nbeing rebased/bisected...\") in the same position within the function. \n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 53 ++++++++++++++++++++++++++++--------------\n t/t3202-show-branch.sh | 18 ++++++++++++++\n 2 files changed, 53 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex a32ae64006..aecf009993 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -486,6 +486,9 @@ static void print_current_branch_name(void)\n \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n }\n \n+#define IS_BISECTED 1\n+#define IS_REBASED 2\n+#define IS_HEAD 4\n \n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n@@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0;\n+\tint recovery = 0, oldref_usage = 0;\n \tstruct worktree **worktrees = get_worktrees();\n+\tstruct worktree *oldref_wt = NULL;\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -507,8 +511,28 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n-\t\tif (copy && !strcmp(head, oldname))\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tstruct worktree *wt = worktrees[i];\n+\n+\t\tif (wt->head_ref && !strcmp(oldref.buf, wt->head_ref))\n+\t\t\toldref_usage |= IS_HEAD;\n+\n+\t\tif (!wt->is_detached)\n+\t\t\tcontinue;\n+\n+\t\tif (is_worktree_being_rebased(wt, oldref.buf)) {\n+\t\t\toldref_usage |= IS_REBASED;\n+\t\t\toldref_wt = wt;\n+\t\t}\n+\n+\t\tif (is_worktree_being_bisected(wt, oldref.buf)) {\n+\t\t\toldref_usage |= IS_BISECTED;\n+\t\t\toldref_wt = wt;\n+\t\t}\n+\t}\n+\n+\tif ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {\n+\t\tif (oldref_usage & IS_HEAD)\n \t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n \t\telse\n \t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n@@ -523,20 +547,13 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\tfor (int i = 0; worktrees[i]; i++) {\n-\t\tstruct worktree *wt = worktrees[i];\n+\tif (oldref_usage & IS_BISECTED)\n+\t\tdie(_(\"Branch %s is being rebased at %s\"),\n+\t\t oldref.buf, oldref_wt->path);\n \n-\t\tif (!wt->is_detached)\n-\t\t\tcontinue;\n-\n-\t\tif (is_worktree_being_rebased(wt, oldref.buf))\n-\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n-\t\t\t oldref.buf, wt->path);\n-\n-\t\tif (is_worktree_being_bisected(wt, oldref.buf))\n-\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n-\t\t\t oldref.buf, wt->path);\n-\t}\n+\tif (oldref_usage & IS_REBASED)\n+\t\tdie(_(\"Branch %s is being bisected at %s\"),\n+\t\t oldref.buf, oldref_wt->path);\n \n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n@@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n-\t\t\terror((!argc || !strcmp(head, branch_name))\n+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n \t\t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t\t : _(\"No branch named '%s'.\"),\n \t\t\t branch_name);\n@@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (!ref_exists(branch->refname)) {\n-\t\t\tif (!argc || !strcmp(head, branch->name))\n+\t\t\tif (!argc || branch_checked_out(branch->refname))\n \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n \t\t}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..be20ebe1d5 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'error descriptions on orphan branch' '\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\tgit -C wt checkout --orphan orphan-branch &&\n+\ttest_branch_op_in_wt() {\n+\t\ttest_orphan_error() {\n+\t\t\ttest_must_fail git $* 2>actual &&\n+\t\t\ttest_i18ngrep \"No commit on branch .orphan-branch. yet.$\" actual\n+\t\t} &&\n+\t\ttest_orphan_error -C wt branch $1 $2 && # implicit branch\n+\t\ttest_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch\n+\t\ttest_orphan_error branch $1 orphan-branch $2 # different worktree\n+\t} &&\n+\ttest_branch_op_in_wt --edit-description &&\n+\ttest_branch_op_in_wt --set-upstream-to=ne &&\n+\ttest_branch_op_in_wt -c new-branch\n+'\n+\n test_done\n-- \n2.34.1\n"},{"id":"472461","messageId":"3d567892-bdd0-9ad9-a30e-1a67e7face11@gmail.com","threadId":"59018","inReplyTo":"139555f1-21ab-d987-a58e-c7f04ce222d3@gmail.com","subject":"[PATCH v4 3/3] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-22T22:56:31Z","receivedAt":"2023-02-22T22:56:37Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\nwe added support for renaming an orphan branch, skipping rename_ref() if\nthe branch being renamed is an orphan branch; checking the current HEAD.\n\nHowever the branch to be renamed can be an orphan branch in a different\nworktree than the current one, i.e. not the current HEAD.\n\nIn \"copy_or_rename_branch()\" we are traversing the worktrees checking if\nthe branch to be renamed is HEAD in any worktree. Let's include also a\ncheck for HEAD being NULL, which is the indication of an orphan branch,\nand use it to extend what we did in cfaff3aac, to all HEADs in the\nrepository.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 16 ++++++++++------\n t/t3200-branch.sh | 14 ++++++++++++++\n 2 files changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex aecf009993..24cd66bae7 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -489,6 +489,7 @@ static void print_current_branch_name(void)\n #define IS_BISECTED 1\n #define IS_REBASED 2\n #define IS_HEAD 4\n+#define IS_ORPHAN 8\n \n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n@@ -514,8 +515,11 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tfor (int i = 0; worktrees[i]; i++) {\n \t\tstruct worktree *wt = worktrees[i];\n \n-\t\tif (wt->head_ref && !strcmp(oldref.buf, wt->head_ref))\n+\t\tif (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) {\n \t\t\toldref_usage |= IS_HEAD;\n+\t\t\tif (is_null_oid(&wt->head_oid))\n+\t\t\t\toldref_usage |= IS_ORPHAN;\n+\t\t}\n \n \t\tif (!wt->is_detached)\n \t\t\tcontinue;\n@@ -531,8 +535,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t}\n \t}\n \n-\tif ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {\n-\t\tif (oldref_usage & IS_HEAD)\n+\tif ((copy || !(oldref_usage & IS_HEAD)) &&\n+\t ((oldref_usage & IS_ORPHAN) || !ref_exists(oldref.buf))) {\n+\t\tif (oldref_usage & IS_ORPHAN)\n \t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n \t\telse\n \t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n@@ -567,8 +572,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n-\tif (!copy &&\n-\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n+\tif (!copy && !(oldref_usage & IS_ORPHAN) &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\n@@ -583,7 +587,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\t\tinterpreted_oldname);\n \t}\n \n-\tif (!copy) {\n+\tif (!copy && (oldref_usage & IS_HEAD)) {\n \t\t/*\n \t\t * Update all per-worktree HEADs pointing at the old ref to\n \t\t * point the new ref.\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a169b68d6..5aef00efde 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -279,6 +279,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -m should work with orphan branches' '\n+\ttest_when_finished git checkout - &&\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\t# rename orphan in another worktreee\n+\tgit -C wt checkout --orphan orphan-foo-wt &&\n+\tgit branch -m orphan-foo-wt orphan-bar-wt &&\n+\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n+\t# rename orphan in the current worktree\n+\tgit checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar &&\n+\ttest orphan-bar=$(git branch --show-current)\n+'\n+\n test_expect_success 'git branch -d on orphan HEAD (merged)' '\n \ttest_when_finished git checkout main &&\n \tgit checkout --orphan orphan &&\n-- \n2.34.1\n"},{"id":"472722","messageId":"4e6aeffe-98ef-b4d7-7c8f-782e0a617653@gmail.com","threadId":"59018","inReplyTo":"50dd7c7a-5656-e010-1c0b-819a40a1f1a0@gmail.com","subject":"Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-25T15:08:06Z","receivedAt":"2023-02-25T15:08:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 22-feb-2023 23:52:51, Rubén Justo wrote:\n> \"reject_rebase_or_bisect_branch()\" was introduced [1] to prevent a\n> branch under bisect or rebase from being renamed or copied. It\n> traverses all worktrees in the repository and dies if the specified\n> branch is being rebased or bisected in any of them.\n> \n> \"replace_each_worktree_head_symref()\" was introduced [2] to adjust the\n> HEAD in all worktrees after a branch rename succeeded. It traverses all\n> worktrees in the repository and if any of them have their HEAD pointing\n> to the renamed ref, it adjusts it.\n> \n> Considering that both functions are only called from within\n> copy_or_rename_branch() and each of them traverses all worktrees, which\n> might involve disk access and resolving multiple references, inlining\n> these two functions to do the traversing once, makes sense.\n> \n> 1.- 14ace5b (branch: do not rename a branch under bisect or rebase,\n> 2016-04-22)\n> \n> 2.- 70999e9 (branch -m: update all per-worktree HEADs, 2016-03-27)\n> \n> Signed-off-by: Rubén Justo \n> ---\n> branch.c | 27 --------------------\n> branch.h | 8 ------\n> builtin/branch.c | 64 ++++++++++++++++++++++++++++--------------------\n> 3 files changed, 38 insertions(+), 61 deletions(-)\n> \n> diff --git a/branch.c b/branch.c\n> index e5614b53b3..f64062be71 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -830,30 +830,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n> \n> \tfree_worktrees(worktrees);\n> }\n> -\n> -int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n> -\t\t\t\t const char *logmsg)\n> -{\n> -\tint ret = 0;\n> -\tstruct worktree **worktrees = get_worktrees();\n> -\tint i;\n> -\n> -\tfor (i = 0; worktrees[i]; i++) {\n> -\t\tstruct ref_store *refs;\n> -\n> -\t\tif (worktrees[i]->is_detached)\n> -\t\t\tcontinue;\n> -\t\tif (!worktrees[i]->head_ref)\n> -\t\t\tcontinue;\n> -\t\tif (strcmp(oldref, worktrees[i]->head_ref))\n> -\t\t\tcontinue;\n> -\n> -\t\trefs = get_worktree_ref_store(worktrees[i]);\n> -\t\tif (refs_create_symref(refs, \"HEAD\", newref, logmsg))\n> -\t\t\tret = error(_(\"HEAD of working tree %s is not updated\"),\n> -\t\t\t\t worktrees[i]->path);\n> -\t}\n> -\n> -\tfree_worktrees(worktrees);\n> -\treturn ret;\n> -}\n> diff --git a/branch.h b/branch.h\n> index ef56103c05..30c01aed73 100644\n> --- a/branch.h\n> +++ b/branch.h\n> @@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);\n> */\n> void die_if_checked_out(const char *branch, int ignore_current_worktree);\n> \n> -/*\n> - * Update all per-worktree HEADs pointing at the old ref to point the new ref.\n> - * This will be used when renaming a branch. Returns 0 if successful, non-zero\n> - * otherwise.\n> - */\n> -int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n> -\t\t\t\t const char *logmsg);\n> -\n> #endif\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index f63fd45edb..a32ae64006 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -486,28 +486,6 @@ static void print_current_branch_name(void)\n> \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n> }\n> \n> -static void reject_rebase_or_bisect_branch(const char *target)\n> -{\n> -\tstruct worktree **worktrees = get_worktrees();\n> -\tint i;\n> -\n> -\tfor (i = 0; worktrees[i]; i++) {\n> -\t\tstruct worktree *wt = worktrees[i];\n> -\n> -\t\tif (!wt->is_detached)\n> -\t\t\tcontinue;\n> -\n> -\t\tif (is_worktree_being_rebased(wt, target))\n> -\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n> -\t\t\t target, wt->path);\n> -\n> -\t\tif (is_worktree_being_bisected(wt, target))\n> -\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n> -\t\t\t target, wt->path);\n> -\t}\n> -\n> -\tfree_worktrees(worktrees);\n> -}\n> \n> static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n> {\n> @@ -516,6 +494,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \tconst char *interpreted_oldname = NULL;\n> \tconst char *interpreted_newname = NULL;\n> \tint recovery = 0;\n> +\tstruct worktree **worktrees = get_worktrees();\n> \n> \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n> \t\t/*\n> @@ -544,7 +523,20 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \telse\n> \t\tvalidate_new_branchname(newname, &newref, force);\n> \n> -\treject_rebase_or_bisect_branch(oldref.buf);\n> +\tfor (int i = 0; worktrees[i]; i++) {\n> +\t\tstruct worktree *wt = worktrees[i];\n> +\n> +\t\tif (!wt->is_detached)\n> +\t\t\tcontinue;\n> +\n> +\t\tif (is_worktree_being_rebased(wt, oldref.buf))\n> +\t\t\tdie(_(\"Branch %s is being rebased at %s\"),\n> +\t\t\t oldref.buf, wt->path);\n> +\n> +\t\tif (is_worktree_being_bisected(wt, oldref.buf))\n> +\t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n> +\t\t\t oldref.buf, wt->path);\n> +\t}\n> \n> \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n> \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n> @@ -574,9 +566,29 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \t\t\t\tinterpreted_oldname);\n> \t}\n> \n> -\tif (!copy &&\n> -\t replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n> -\t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n> +\tif (!copy) {\n> +\t\t/*\n> +\t\t * Update all per-worktree HEADs pointing at the old ref to\n> +\t\t * point the new ref.\n> +\t\t */\n> +\t\tfor (int i = 0; worktrees[i]; i++) {\n> +\t\t\tstruct ref_store *refs;\n> +\n> +\t\t\tif (worktrees[i]->is_detached)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (!worktrees[i]->head_ref)\n> +\t\t\t\tcontinue;\n> +\t\t\tif (strcmp(oldref.buf, worktrees[i]->head_ref))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\trefs = get_worktree_ref_store(worktrees[i]);\n> +\t\t\tif (refs_create_symref(refs, \"HEAD\", newref.buf, logmsg.buf))\n> +\t\t\t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"),\n> +\t\t\t\t\tnewname);\n\nReviewing this, I noticed I made a mistake here. The original code\ndoesn't stop iterating whenever refs_create_symref() fails; it continues\ntrying to update the remaining worktrees. When the iteration ends, if\nany of the updates failed, then die(). Also, the error message \"HEAD of\nworking tree %s is not updated\" is lost. I'll reroll with this errors\nfixed but maybe some review is already underway, so I'll wait some\ntime. Sorry for the inconvenience.\n"},{"id":"472816","messageId":"20230227193012.2410973-1-jonathantanmy@google.com","threadId":"59018","inReplyTo":"4e6aeffe-98ef-b4d7-7c8f-782e0a617653@gmail.com","subject":"Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-02-27T19:30:12Z","receivedAt":"2023-02-27T19:30:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Rubén Justo writes:\n> Reviewing this, I noticed I made a mistake here. The original code\n> doesn't stop iterating whenever refs_create_symref() fails; it continues\n> trying to update the remaining worktrees. When the iteration ends, if\n> any of the updates failed, then die(). Also, the error message \"HEAD of\n> working tree %s is not updated\" is lost. \n\nAh yes, I noticed this too.\n\nBesides that, a reviewer, upon reading the commit message, might ask:\nwhy not take the worktrees as a parameter then, if we're so worried\nabout performance? I think that the real reason for inlining is that the\ncode being inlined needs to communicate more information to its calling\nfunction in subsequent patches; the performance improvement is only a\nbeneficial side effect.\n\n"},{"id":"472817","messageId":"20230227193839.2416545-1-jonathantanmy@google.com","threadId":"59018","inReplyTo":"76bac570-147e-7c74-c18c-1da88bc3d342@gmail.com","subject":"Re: [PATCH v4 2/3] branch: description for orphan branch errors","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-02-27T19:38:39Z","receivedAt":"2023-02-27T19:39:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Firstly, the subject could be more precise. Maybe \"branch: check all\nworktrees for orphan branches\" (47 characters) or something like that.\n\nRubén Justo writes:\n> In bcfc82bd48 (branch: description for non-existent branch errors,\n> 2022-10-08) we checked the current HEAD\n\nProbably clearer to say \"HEAD in the current worktree\" instead of\n\"current HEAD\".\n\n> to detect if the branch to\n> operate with is an orphan branch, so as to avoid the confusing error:\n> \"No branch named...\".\n> \n> If we are asked to operate with an orphan branch in a different working\n> tree than the current one, we need to check the HEAD in that different\n> working tree.\n\nProbably clearer to just say \"But there might be orphan branches in\nother worktrees\".\n\n> Let's extend the check we did in bcfc82bd48, to all HEADs in the\n> repository, using the helper introduced in 31ad6b61bd (branch: add\n> branch_checked_out() helper, 2022-06-15)\n\ns/HEADs/worktrees/\n\n> @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n> \tconst char *interpreted_oldname = NULL;\n> \tconst char *interpreted_newname = NULL;\n> -\tint recovery = 0;\n> +\tint recovery = 0, oldref_usage = 0;\n> \tstruct worktree **worktrees = get_worktrees();\n> +\tstruct worktree *oldref_wt = NULL;\n\nBetter to have 2 variables (one for rebased, and one for bisected) to\navoid the situation in which the last problematic worktree seen was a\nbisected one, but a prior one was a rebased one.\n\n> @@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \n> \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n> \t\tif (!ref_exists(branch_ref.buf))\n> -\t\t\terror((!argc || !strcmp(head, branch_name))\n> +\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n> \t\t\t ? _(\"No commit on branch '%s' yet.\")\n> \t\t\t : _(\"No branch named '%s'.\"),\n> \t\t\t branch_name);\n> @@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \n> \t\tif (!ref_exists(branch->refname)) {\n> -\t\t\tif (!argc || !strcmp(head, branch->name))\n> +\t\t\tif (!argc || branch_checked_out(branch->refname))\n> \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n> \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n> \t\t}\n\nWhat is the relevance of these changes?\n\n"},{"id":"472819","messageId":"20230227194116.2417165-1-jonathantanmy@google.com","threadId":"59018","inReplyTo":"3d567892-bdd0-9ad9-a30e-1a67e7face11@gmail.com","subject":"Re: [PATCH v4 3/3] branch: rename orphan branches in any worktree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2023-02-27T19:41:16Z","receivedAt":"2023-02-27T19:41:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Rubén Justo writes:\n> @@ -583,7 +587,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n> \t\t\t\tinterpreted_oldname);\n> \t}\n> \n> -\tif (!copy) {\n> +\tif (!copy && (oldref_usage & IS_HEAD)) {\n> \t\t/*\n> \t\t * Update all per-worktree HEADs pointing at the old ref to\n> \t\t * point the new ref.\n\nWhat is the relevance of this change?\n"},{"id":"472825","messageId":"xmqq5ybmsrgu.fsf@gitster.g","threadId":"59018","inReplyTo":"20230227193839.2416545-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 2/3] branch: description for orphan branch errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-27T21:56:01Z","receivedAt":"2023-02-27T21:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Firstly, the subject could be more precise. Maybe \"branch: check all\n> worktrees for orphan branches\" (47 characters) or something like that.\n>\n> Rubén Justo writes:\n>> In bcfc82bd48 (branch: description for non-existent branch errors,\n>> 2022-10-08) we checked the current HEAD\n>\n> Probably clearer to say \"HEAD in the current worktree\" instead of\n> \"current HEAD\".\n>\n>> to detect if the branch to\n>> operate with is an orphan branch, so as to avoid the confusing error:\n>> \"No branch named...\".\n>> \n>> If we are asked to operate with an orphan branch in a different working\n>> tree than the current one, we need to check the HEAD in that different\n>> working tree.\n>\n> Probably clearer to just say \"But there might be orphan branches in\n> other worktrees\".\n>\n>> Let's extend the check we did in bcfc82bd48, to all HEADs in the\n>> repository, using the helper introduced in 31ad6b61bd (branch: add\n>> branch_checked_out() helper, 2022-06-15)\n>\n> s/HEADs/worktrees/\n>\n>> @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n>> \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n>> \tconst char *interpreted_oldname = NULL;\n>> \tconst char *interpreted_newname = NULL;\n>> -\tint recovery = 0;\n>> +\tint recovery = 0, oldref_usage = 0;\n>> \tstruct worktree **worktrees = get_worktrees();\n>> +\tstruct worktree *oldref_wt = NULL;\n>\n> Better to have 2 variables (one for rebased, and one for bisected) to\n> avoid the situation in which the last problematic worktree seen was a\n> bisected one, but a prior one was a rebased one.\n\nAll good suggestions.\n\n>> @@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> \n>> \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n>> \t\tif (!ref_exists(branch_ref.buf))\n>> -\t\t\terror((!argc || !strcmp(head, branch_name))\n>> +\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n>> \t\t\t ? _(\"No commit on branch '%s' yet.\")\n>> \t\t\t : _(\"No branch named '%s'.\"),\n>> \t\t\t branch_name);\n>> @@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> \t\t}\n>> \n>> \t\tif (!ref_exists(branch->refname)) {\n>> -\t\t\tif (!argc || !strcmp(head, branch->name))\n>> +\t\t\tif (!argc || branch_checked_out(branch->refname))\n>> \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n>> \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n>> \t\t}\n>\n> What is the relevance of these changes?\n\nThanks for reading very carefully and asking clarification for\nrelevant parts of the changes.\n\n\n\n"},{"id":"472833","messageId":"dee6ca9e-af3b-bc79-a09c-6bc0f0245017@gmail.com","threadId":"59018","inReplyTo":"20230227193012.2410973-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 1/3] branch: avoid unnecessary worktrees traversals","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-28T00:11:51Z","receivedAt":"2023-02-28T00:12:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 27/2/23 20:30, Jonathan Tan wrote:\n> Rubén Justo writes:\n>> Reviewing this, I noticed I made a mistake here. The original code\n>> doesn't stop iterating whenever refs_create_symref() fails; it continues\n>> trying to update the remaining worktrees. When the iteration ends, if\n>> any of the updates failed, then die(). Also, the error message \"HEAD of\n>> working tree %s is not updated\" is lost. \n> \n> Ah yes, I noticed this too.\n\nI'll re-roll with the fix in the next iteration.\n\nThank you for reading carefully.\n\n> \n> Besides that, a reviewer, upon reading the commit message, might ask:\n> why not take the worktrees as a parameter then, if we're so worried\n> about performance? I think that the real reason for inlining is that the\n> code being inlined needs to communicate more information to its calling\n> function in subsequent patches; the performance improvement is only a\n> beneficial side effect.\n> \n\nCertainly, that's a way to see the modification within this series.\nActually, this modification wasn't present in v1, but once it was\nintroduced in v2, it made sense on its own: to eliminate the redundant\ntraversals of worktrees when renaming a branch, even if the branch isn't\nHEAD in any worktree.\n\nTherefore, I think the change can also be seen in another way: the\nincreased ease in the next modifications is a beneficial side effect\nof this patch.\n"},{"id":"472834","messageId":"d29bb653-7d3e-1753-8805-aa48b35320d9@gmail.com","threadId":"59018","inReplyTo":"20230227193839.2416545-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 2/3] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-28T00:22:55Z","receivedAt":"2023-02-28T00:23:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Thank you for your review!\n\nOn 27/2/23 20:38, Jonathan Tan wrote:\n> Firstly, the subject could be more precise. Maybe \"branch: check all\n> worktrees for orphan branches\" (47 characters) or something like that.\n\nThe main intention in this series is to stop giving the user a confusing\nerror \"No branch named...\" for a branch he may have just created. I\nthink the current subject states that better. But I'm open to change it\nin that direction.\n\n> Rubén Justo writes:\n>> In bcfc82bd48 (branch: description for non-existent branch errors,\n>> 2022-10-08) we checked the current HEAD\n> \n> Probably clearer to say \"HEAD in the current worktree\" instead of\n> \"current HEAD\".\n\nOK. I'll reword with that.\n\n>> to detect if the branch to\n>> operate with is an orphan branch, so as to avoid the confusing error:\n>> \"No branch named...\".\n>>\n>> If we are asked to operate with an orphan branch in a different working\n>> tree than the current one, we need to check the HEAD in that different\n>> working tree.\n> \n> Probably clearer to just say \"But there might be orphan branches in\n> other worktrees\".\n\nThat loses important details IMHO, the intention: \"avoid the\nconfusing..\", and the reasoning on why we need to check HEAD in all\nworktrees.\n\n>> Let's extend the check we did in bcfc82bd48, to all HEADs in the\n>> repository, using the helper introduced in 31ad6b61bd (branch: add\n>> branch_checked_out() helper, 2022-06-15)\n> \n> s/HEADs/worktrees/\n\nI understand your suggestion, but my intention along the message is to\nmaintain the reasoning on the \"HEAD\", due to an orphan branch is a HEAD\npointing to a non-existing ref. Maybe \"the HEADs in all worktrees\"\ncould be better?\n\n>> @@ -493,8 +496,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n>> \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n>> \tconst char *interpreted_oldname = NULL;\n>> \tconst char *interpreted_newname = NULL;\n>> -\tint recovery = 0;\n>> +\tint recovery = 0, oldref_usage = 0;\n>> \tstruct worktree **worktrees = get_worktrees();\n>> +\tstruct worktree *oldref_wt = NULL;\n> \n> Better to have 2 variables (one for rebased, and one for bisected) to\n> avoid the situation in which the last problematic worktree seen was a\n> bisected one, but a prior one was a rebased one.\n\nWell seen. Thanks for reading carefully.\n\nI'll re-roll with that.\n\n>> @@ -818,7 +835,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> \n>> \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n>> \t\tif (!ref_exists(branch_ref.buf))\n>> -\t\t\terror((!argc || !strcmp(head, branch_name))\n>> +\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n>> \t\t\t ? _(\"No commit on branch '%s' yet.\")\n>> \t\t\t : _(\"No branch named '%s'.\"),\n>> \t\t\t branch_name);\n>> @@ -863,7 +880,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> \t\t}\n>> \n>> \t\tif (!ref_exists(branch->refname)) {\n>> -\t\t\tif (!argc || !strcmp(head, branch->name))\n>> +\t\t\tif (!argc || branch_checked_out(branch->refname))\n>> \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n>> \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n>> \t\t}\n> \n> What is the relevance of these changes?\n> \n\nThis is the main intention in the patch: not showing the confusing error\n\"No branch named...\" for orphan branches. I'm not sure if I understand\nyour question...\n"},{"id":"472835","messageId":"fc3a5058-134a-3fcc-a59a-cd072f2d0258@gmail.com","threadId":"59018","inReplyTo":"20230227194116.2417165-1-jonathantanmy@google.com","subject":"Re: [PATCH v4 3/3] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-02-28T00:23:54Z","receivedAt":"2023-02-28T00:24:05Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 27/2/23 20:41, Jonathan Tan wrote:\n> Rubén Justo writes:\n>> @@ -583,7 +587,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n>> \t\t\t\tinterpreted_oldname);\n>> \t}\n>> \n>> -\tif (!copy) {\n>> +\tif (!copy && (oldref_usage & IS_HEAD)) {\n>> \t\t/*\n>> \t\t * Update all per-worktree HEADs pointing at the old ref to\n>> \t\t * point the new ref.\n> \n> What is the relevance of this change?\n> \n\nMaybe this change makes more sense in 2/3?\n"},{"id":"474200","messageId":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","threadId":"59018","inReplyTo":"139555f1-21ab-d987-a58e-c7f04ce222d3@gmail.com","subject":"[PATCH v5 0/5] branch: operations on orphan branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-03-26T22:19:36Z","receivedAt":"2023-03-26T22:19:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The initial and main intention in this series is to avoid some confusing\nerrors operating with orphan branches, when working with worktrees.\n\nIt's been a while since the previous iteration, sorry for that. Here is\na quick overview of the previous iterations:\n\nIn v2 a change was introduced, to avoid some unnecessary worktrees\ntraversals, which implies disk access.\n\nIn v3 and v4 some refactorings were done, such as making some names\nmore meaningful and, mainly, inlining some helper functions in its only\none caller. No functional changes were expected. However, those\nrefactorings introduced some undesired behavior changes; some were noted\nin the round of reviews, some others during the preparation of this new\niteration.\n\nIn this iteration, v5, I've reverted some of those major refactorings,\nmainly the inlining, because it ended up introducing unnecessary\ncomplexity for minimal benefit in this series. Maybe those refactorings\nmake more sense in future series.\n\nA new test has been introduced, in 1/5, to notice if a behavior change\nsimilar to that observed in v4, is reintroduced in the future.\n\nOther than that, no functional changes are expected from v2.\n\nRubén Justo (5):\n branch: test for failures while renaming branches\n branch: use get_worktrees() in copy_or_rename_branch()\n branch: description for orphan branch errors\n branch: rename orphan branches in any worktree\n branch: avoid unnecessary worktrees traversals\n\n branch.c | 27 ----------------\n branch.h | 8 -----\n builtin/branch.c | 71 ++++++++++++++++++++++++++++++++++--------\n t/t3200-branch.sh | 29 +++++++++++++++++\n t/t3202-show-branch.sh | 18 +++++++++++\n 5 files changed, 105 insertions(+), 48 deletions(-)\n\n-- \n2.39.2\n"},{"id":"474201","messageId":"2c22e77e-314f-1025-3355-740e19b743fa@gmail.com","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"[PATCH v5 1/5] branch: test for failures while renaming branches","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-03-26T22:33:02Z","receivedAt":"2023-03-26T22:33:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we introduced replace_each_worktree_head_symref() in 70999e9cec\n(branch -m: update all per-worktree HEADs, 2016-03-27), we implemented a\nbest effort approach.\n\nIf we are asked to rename a branch that is simultaneously checked out in\nmultiple worktrees, we try to update all of those worktrees. If we fail\nupdating any of them, we die() as a signal that something has gone\nwrong. However, at this point, the branch ref has already been renamed\nand also updated the HEADs of the successfully updated worktrees.\nDespite returning an error, we do not try to rollback those changes.\n\nLet's add a test to notice if we change this behavior in the future.\n\nIn next commits we will change replace_each_worktree_head_symref() to\nwork more closely with its only caller, copy_or_rename_branch(). Let's\nmove the former closer to its caller, to facilitate those changes.\n\nSigned-off-by: Rubén Justo \n---\n branch.c | 27 ---------------------------\n branch.h | 8 --------\n builtin/branch.c | 32 ++++++++++++++++++++++++++++++++\n t/t3200-branch.sh | 15 +++++++++++++++\n 4 files changed, 47 insertions(+), 35 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex eacef62b7c..b45454593a 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -835,30 +835,3 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)\n \n \tfree_worktrees(worktrees);\n }\n-\n-int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n-\t\t\t\t const char *logmsg)\n-{\n-\tint ret = 0;\n-\tstruct worktree **worktrees = get_worktrees();\n-\tint i;\n-\n-\tfor (i = 0; worktrees[i]; i++) {\n-\t\tstruct ref_store *refs;\n-\n-\t\tif (worktrees[i]->is_detached)\n-\t\t\tcontinue;\n-\t\tif (!worktrees[i]->head_ref)\n-\t\t\tcontinue;\n-\t\tif (strcmp(oldref, worktrees[i]->head_ref))\n-\t\t\tcontinue;\n-\n-\t\trefs = get_worktree_ref_store(worktrees[i]);\n-\t\tif (refs_create_symref(refs, \"HEAD\", newref, logmsg))\n-\t\t\tret = error(_(\"HEAD of working tree %s is not updated\"),\n-\t\t\t\t worktrees[i]->path);\n-\t}\n-\n-\tfree_worktrees(worktrees);\n-\treturn ret;\n-}\ndiff --git a/branch.h b/branch.h\nindex ef56103c05..30c01aed73 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -155,12 +155,4 @@ int read_branch_desc(struct strbuf *, const char *branch_name);\n */\n void die_if_checked_out(const char *branch, int ignore_current_worktree);\n \n-/*\n- * Update all per-worktree HEADs pointing at the old ref to point the new ref.\n- * This will be used when renaming a branch. Returns 0 if successful, non-zero\n- * otherwise.\n- */\n-int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n-\t\t\t\t const char *logmsg);\n-\n #endif\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f63fd45edb..c7ace2f2da 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -509,6 +509,38 @@ static void reject_rebase_or_bisect_branch(const char *target)\n \tfree_worktrees(worktrees);\n }\n \n+/*\n+ * Update all per-worktree HEADs pointing at the old ref to point the new ref.\n+ * This will be used when renaming a branch. Returns 0 if successful, non-zero\n+ * otherwise.\n+ */\n+static int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n+\t\t\t\t\t const char *logmsg)\n+{\n+\tint ret = 0;\n+\tstruct worktree **worktrees = get_worktrees();\n+\tint i;\n+\n+\tfor (i = 0; worktrees[i]; i++) {\n+\t\tstruct ref_store *refs;\n+\n+\t\tif (worktrees[i]->is_detached)\n+\t\t\tcontinue;\n+\t\tif (!worktrees[i]->head_ref)\n+\t\t\tcontinue;\n+\t\tif (strcmp(oldref, worktrees[i]->head_ref))\n+\t\t\tcontinue;\n+\n+\t\trefs = get_worktree_ref_store(worktrees[i]);\n+\t\tif (refs_create_symref(refs, \"HEAD\", newref, logmsg))\n+\t\t\tret = error(_(\"HEAD of working tree %s is not updated\"),\n+\t\t\t\t worktrees[i]->path);\n+\t}\n+\n+\tfree_worktrees(worktrees);\n+\treturn ret;\n+}\n+\n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 5a8a48287c..7abd938e15 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -239,6 +239,21 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou\n \tgit worktree prune\n '\n \n+test_expect_success 'git branch -M fails if updating any linked working tree fails' '\n+\tgit worktree add -b baz bazdir1 &&\n+\tgit worktree add -f bazdir2 baz &&\n+\ttouch .git/worktrees/bazdir1/HEAD.lock &&\n+\ttest_must_fail git branch -M baz bam &&\n+\ttest $(git -C bazdir2 rev-parse --abbrev-ref HEAD) = bam &&\n+\tgit branch -M bam baz &&\n+\trm .git/worktrees/bazdir1/HEAD.lock &&\n+\ttouch .git/worktrees/bazdir2/HEAD.lock &&\n+\ttest_must_fail git branch -M baz bam &&\n+\ttest $(git -C bazdir1 rev-parse --abbrev-ref HEAD) = bam &&\n+\trm -rf bazdir1 bazdir2 &&\n+\tgit worktree prune\n+'\n+\n test_expect_success 'git branch -M baz bam should succeed within a worktree in which baz is checked out' '\n \tgit checkout -b baz &&\n \tgit worktree add -f bazdir baz &&\n-- \n2.39.2\n"},{"id":"474202","messageId":"4b1386fd-1bf7-6fdf-5c0b-49b564b1a7f4@gmail.com","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"[PATCH v5 2/5] branch: use get_worktrees() in copy_or_rename_branch()","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-03-26T22:33:09Z","receivedAt":"2023-03-26T22:33:29Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Obtaining the list of worktrees, using get_worktrees(), is not a\nlightweight operation, because it involves reading from disk.\n\nLet's stop calling get_worktrees() in reject_rebase_or_bisect_branch()\nand in replace_each_worktree_head_symref(). Make them receive the list\nof worktrees from their only caller, copy_or_rename_branch().\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex c7ace2f2da..bac67c27d5 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -486,9 +486,9 @@ static void print_current_branch_name(void)\n \t\tdie(_(\"HEAD (%s) points outside of refs/heads/\"), refname);\n }\n \n-static void reject_rebase_or_bisect_branch(const char *target)\n+static void reject_rebase_or_bisect_branch(struct worktree **worktrees,\n+\t\t\t\t\t const char *target)\n {\n-\tstruct worktree **worktrees = get_worktrees();\n \tint i;\n \n \tfor (i = 0; worktrees[i]; i++) {\n@@ -505,8 +505,6 @@ static void reject_rebase_or_bisect_branch(const char *target)\n \t\t\tdie(_(\"Branch %s is being bisected at %s\"),\n \t\t\t target, wt->path);\n \t}\n-\n-\tfree_worktrees(worktrees);\n }\n \n /*\n@@ -514,11 +512,11 @@ static void reject_rebase_or_bisect_branch(const char *target)\n * This will be used when renaming a branch. Returns 0 if successful, non-zero\n * otherwise.\n */\n-static int replace_each_worktree_head_symref(const char *oldref, const char *newref,\n+static int replace_each_worktree_head_symref(struct worktree **worktrees,\n+\t\t\t\t\t const char *oldref, const char *newref,\n \t\t\t\t\t const char *logmsg)\n {\n \tint ret = 0;\n-\tstruct worktree **worktrees = get_worktrees();\n \tint i;\n \n \tfor (i = 0; worktrees[i]; i++) {\n@@ -537,7 +535,6 @@ static int replace_each_worktree_head_symref(const char *oldref, const char *new\n \t\t\t\t worktrees[i]->path);\n \t}\n \n-\tfree_worktrees(worktrees);\n \treturn ret;\n }\n \n@@ -548,6 +545,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n+\tstruct worktree **worktrees = get_worktrees();\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n@@ -576,7 +574,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \telse\n \t\tvalidate_new_branchname(newname, &newref, force);\n \n-\treject_rebase_or_bisect_branch(oldref.buf);\n+\treject_rebase_or_bisect_branch(worktrees, oldref.buf);\n \n \tif (!skip_prefix(oldref.buf, \"refs/heads/\", &interpreted_oldname) ||\n \t !skip_prefix(newref.buf, \"refs/heads/\", &interpreted_newname)) {\n@@ -607,7 +605,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t}\n \n \tif (!copy &&\n-\t replace_each_worktree_head_symref(oldref.buf, newref.buf, logmsg.buf))\n+\t replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,\n+\t\t\t\t\t logmsg.buf))\n \t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n \n \tstrbuf_release(&logmsg);\n@@ -622,6 +621,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tstrbuf_release(&newref);\n \tstrbuf_release(&oldsection);\n \tstrbuf_release(&newsection);\n+\tfree_worktrees(worktrees);\n }\n \n static GIT_PATH_FUNC(edit_description, \"EDIT_DESCRIPTION\")\n-- \n2.39.2\n"},{"id":"474203","messageId":"e22b661c-6f79-a980-23c6-4fe0200ead4d@gmail.com","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"[PATCH v5 3/5] branch: description for orphan branch errors","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-03-26T22:33:17Z","receivedAt":"2023-03-26T22:33:29Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In bcfc82bd48 (branch: description for non-existent branch errors,\n2022-10-08) we checked the HEAD in the current worktree to detect if the\nbranch to operate with is an orphan branch, so as to avoid the confusing\nerror: \"No branch named...\".\n\nIf we are asked to operate with an orphan branch in a different working\ntree than the current one, we need to check the HEAD in that different\nworking tree.\n\nLet's extend the check we did in bcfc82bd48, to check the HEADs in all\nworktrees linked to the current repository, using the helper introduced\nin 31ad6b61bd (branch: add branch_checked_out() helper, 2022-06-15).\n\nThe helper, branch_checked_out(), does its work obtaining internally a\nlist of worktrees linked to the current repository. Obtaining that list\nis not a lightweight work because it implies disk access.\n\nIn copy_or_rename_branch() we already have a list of worktrees. Let's\nuse that already obtained list, and avoid using here the helper.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 21 ++++++++++++++++-----\n t/t3202-show-branch.sh | 18 ++++++++++++++++++\n 2 files changed, 34 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex bac67c27d5..90dcbb0c6e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -538,13 +538,15 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees,\n \treturn ret;\n }\n \n+#define IS_HEAD 1\n+\n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tconst char *interpreted_oldname = NULL;\n \tconst char *interpreted_newname = NULL;\n-\tint recovery = 0;\n+\tint recovery = 0, oldref_usage = 0;\n \tstruct worktree **worktrees = get_worktrees();\n \n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n@@ -558,8 +560,17 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tif ((copy || strcmp(head, oldname)) && !ref_exists(oldref.buf)) {\n-\t\tif (copy && !strcmp(head, oldname))\n+\tfor (int i = 0; worktrees[i]; i++) {\n+\t\tstruct worktree *wt = worktrees[i];\n+\n+\t\tif (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) {\n+\t\t\toldref_usage |= IS_HEAD;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif ((copy || !(oldref_usage & IS_HEAD)) && !ref_exists(oldref.buf)) {\n+\t\tif (oldref_usage & IS_HEAD)\n \t\t\tdie(_(\"No commit on branch '%s' yet.\"), oldname);\n \t\telse\n \t\t\tdie(_(\"No branch named '%s'.\"), oldname);\n@@ -838,7 +849,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf))\n-\t\t\terror((!argc || !strcmp(head, branch_name))\n+\t\t\terror((!argc || branch_checked_out(branch_ref.buf))\n \t\t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t\t : _(\"No branch named '%s'.\"),\n \t\t\t branch_name);\n@@ -883,7 +894,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (!ref_exists(branch->refname)) {\n-\t\t\tif (!argc || !strcmp(head, branch->name))\n+\t\t\tif (!argc || branch_checked_out(branch->refname))\n \t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), branch->name);\n \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n \t\t}\ndiff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh\nindex ea7cfd1951..be20ebe1d5 100755\n--- a/t/t3202-show-branch.sh\n+++ b/t/t3202-show-branch.sh\n@@ -221,4 +221,22 @@ test_expect_success 'fatal descriptions on non-existent branch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'error descriptions on orphan branch' '\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\tgit -C wt checkout --orphan orphan-branch &&\n+\ttest_branch_op_in_wt() {\n+\t\ttest_orphan_error() {\n+\t\t\ttest_must_fail git $* 2>actual &&\n+\t\t\ttest_i18ngrep \"No commit on branch .orphan-branch. yet.$\" actual\n+\t\t} &&\n+\t\ttest_orphan_error -C wt branch $1 $2 && # implicit branch\n+\t\ttest_orphan_error -C wt branch $1 orphan-branch $2 && # explicit branch\n+\t\ttest_orphan_error branch $1 orphan-branch $2 # different worktree\n+\t} &&\n+\ttest_branch_op_in_wt --edit-description &&\n+\ttest_branch_op_in_wt --set-upstream-to=ne &&\n+\ttest_branch_op_in_wt -c new-branch\n+'\n+\n test_done\n-- \n2.39.2\n"},{"id":"474204","messageId":"cc4cf795-5424-c56b-aaec-2032cb969152@gmail.com","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"[PATCH v5 4/5] branch: rename orphan branches in any worktree","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-03-26T22:33:27Z","receivedAt":"2023-03-26T22:33:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In cfaff3aac (branch -m: allow renaming a yet-unborn branch, 2020-12-13)\nwe added support for renaming an orphan branch when that branch is\nchecked out in the current worktree.\n\nLet's also allow renaming an orphan branch checked out in a worktree\ndifferent than the current one.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 6 ++++--\n t/t3200-branch.sh | 14 ++++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 90dcbb0c6e..a93b9fc0ab 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -539,6 +539,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees,\n }\n \n #define IS_HEAD 1\n+#define IS_ORPHAN 2\n \n static void copy_or_rename_branch(const char *oldname, const char *newname, int copy, int force)\n {\n@@ -565,6 +566,8 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \n \t\tif (wt->head_ref && !strcmp(oldref.buf, wt->head_ref)) {\n \t\t\toldref_usage |= IS_HEAD;\n+\t\t\tif (is_null_oid(&wt->head_oid))\n+\t\t\t\toldref_usage |= IS_ORPHAN;\n \t\t\tbreak;\n \t\t}\n \t}\n@@ -599,8 +602,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t\t oldref.buf, newref.buf);\n \n-\tif (!copy &&\n-\t (!head || strcmp(oldname, head) || !is_null_oid(&head_oid)) &&\n+\tif (!copy && !(oldref_usage & IS_ORPHAN) &&\n \t rename_ref(oldref.buf, newref.buf, logmsg.buf))\n \t\tdie(_(\"Branch rename failed\"));\n \tif (copy && copy_existing_ref(oldref.buf, newref.buf, logmsg.buf))\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7abd938e15..98b6c8ac34 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -298,6 +298,20 @@ test_expect_success 'git branch -M and -C fail on detached HEAD' '\n \ttest_cmp expect err\n '\n \n+test_expect_success 'git branch -m should work with orphan branches' '\n+\ttest_when_finished git checkout - &&\n+\ttest_when_finished git worktree remove -f wt &&\n+\tgit worktree add wt --detach &&\n+\t# rename orphan in another worktreee\n+\tgit -C wt checkout --orphan orphan-foo-wt &&\n+\tgit branch -m orphan-foo-wt orphan-bar-wt &&\n+\ttest orphan-bar-wt=$(git -C orphan-worktree branch --show-current) &&\n+\t# rename orphan in the current worktree\n+\tgit checkout --orphan orphan-foo &&\n+\tgit branch -m orphan-foo orphan-bar &&\n+\ttest orphan-bar=$(git branch --show-current)\n+'\n+\n test_expect_success 'git branch -d on orphan HEAD (merged)' '\n \ttest_when_finished git checkout main &&\n \tgit checkout --orphan orphan &&\n-- \n2.39.2\n"},{"id":"474205","messageId":"c54e35a5-3887-9192-0c9d-8673feaa10ba@gmail.com","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"[PATCH v5 5/5] branch: avoid unnecessary worktrees traversals","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-03-26T22:33:40Z","receivedAt":"2023-03-26T22:33:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we rename a branch ref, we need to update any worktree that have\nits HEAD pointing to the branch ref being renamed, so to make it use the\nnew ref name.\n\nIf we know in advance that we're renaming a branch that is not currently\nchecked out in any worktree, we can skip this step entirely. Let's do\nit so.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex a93b9fc0ab..6a564b22b0 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -617,7 +617,7 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \t\t\t\tinterpreted_oldname);\n \t}\n \n-\tif (!copy &&\n+\tif (!copy && (oldref_usage & IS_HEAD) &&\n \t replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,\n \t\t\t\t\t logmsg.buf))\n \t\tdie(_(\"Branch renamed to %s, but HEAD is not updated!\"), newname);\n-- \n2.39.2\n"},{"id":"474256","messageId":"xmqq7cv29dpi.fsf@gitster.g","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"Re: [PATCH v5 0/5] branch: operations on orphan branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-27T19:49:45Z","receivedAt":"2023-03-27T19:50:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> In this iteration, v5, I've reverted some of those major refactorings,\n> mainly the inlining, because it ended up introducing unnecessary\n> complexity for minimal benefit in this series. Maybe those refactorings\n> make more sense in future series.\n\nThe previous one has been cooking in 'next' already for some time\n(even before the feature freeze for 2.40), so we'd usually do not\ntake such a rewrite and rather go with incremental refinements, but\nlet's discard it out of 'next' and queue this intereation instead.\n\nWe're probably overdue to rewind and rebuild 'next' this round\nanyway.\n\nThanks for an updated version. Will queue.\n\n"},{"id":"476389","messageId":"xmqqednz3dbf.fsf@gitster.g","threadId":"59018","inReplyTo":"f8e6447e-5cd3-98fa-f567-39e1c60dacb0@gmail.com","subject":"Re: [PATCH v5 0/5] branch: operations on orphan branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-01T22:19:00Z","receivedAt":"2023-05-01T22:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> The initial and main intention in this series is to avoid some confusing\n> errors operating with orphan branches, when working with worktrees.\n> ...\n> In this iteration, v5, I've reverted some of those major refactorings,\n> mainly the inlining, because it ended up introducing unnecessary\n> complexity for minimal benefit in this series. Maybe those refactorings\n> make more sense in future series.\n>\n> A new test has been introduced, in 1/5, to notice if a behavior change\n> similar to that observed in v4, is reintroduced in the future.\n>\n> Other than that, no functional changes are expected from v2.\n\nThis has not seen any activities since it was posted; presumably the\nissues raised during the review of the previous round have all been\naddressed?\n\nIs everybody happy to see us declare victory and merge it down to\n'next'? I see everybody who commented on earlier iterations of the\nseries are CC'ed, so let's hear from them (and others who may be\ninterested).\n\nThanks.\n"}]}