{"thread":{"id":"58691","subject":"[PATCH] branch: error copying or renaming a detached HEAD","startedAt":"2022-10-25T23:01:37Z","lastAt":"2022-10-25T23:50:47Z","messageCount":2,"participants":["Rubén Justo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"465741","messageId":"0ac8cd48-08d7-9bdd-b074-c8d5ded522f6@gmail.com","threadId":"58691","inReplyTo":null,"subject":"[PATCH] branch: error copying or renaming a detached HEAD","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-25T23:01:29Z","receivedAt":"2022-10-25T23:01:37Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In c847f53712 (Detached HEAD (experimental), 2007-01-01) an error\ncondition was introduced in rename_branch() to prevent renaming, later\nalso copying, a detached HEAD.\n\nThe condition used was checking for NULL in oldname, the source branch\nto rename/copy.  That condition cannot be satisfied because if no source\nbranch is specified, HEAD is going to be used in the call.\n\nThe error issued instead is:\n\n\tfatal: Invalid branch name: 'HEAD'\n\nLet's remove the condition in copy_or_rename_branch() (the current\nfunction name) and check for HEAD before calling it, dying with the\noriginal intended error if we're in a detached HEAD.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/branch.c  | 28 ++++++++--------------------\n t/t3200-branch.sh | 11 +++++++++++\n 2 files changed, 19 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 407517ba68..c964ac7bb4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -520,13 +520,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n \n-\tif (!oldname) {\n-\t\tif (copy)\n-\t\t\tdie(_(\"cannot copy the current branch while not on any.\"));\n-\t\telse\n-\t\t\tdie(_(\"cannot rename the current branch while not on any.\"));\n-\t}\n-\n \tif (strbuf_check_branch_ref(&oldref, oldname)) {\n \t\t/*\n \t\t * Bad name --- this could be an attempt to rename a\n@@ -827,24 +820,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \n \t\treturn ret;\n-\t} else if (copy) {\n-\t\tif (!argc)\n-\t\t\tdie(_(\"branch name required\"));\n-\t\telse if (argc == 1)\n-\t\t\tcopy_or_rename_branch(head, argv[0], 1, copy > 1);\n-\t\telse if (argc == 2)\n-\t\t\tcopy_or_rename_branch(argv[0], argv[1], 1, copy > 1);\n-\t\telse\n-\t\t\tdie(_(\"too many branches for a copy operation\"));\n-\t} else if (rename) {\n+\t} else if (copy || rename) {\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\telse if (argc == 1)\n-\t\t\tcopy_or_rename_branch(head, argv[0], 0, rename > 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], 0, rename > 1);\n+\t\t\tcopy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);\n \t\telse\n-\t\t\tdie(_(\"too many arguments for a rename operation\"));\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} else if (new_upstream) {\n \t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 7d8edff9c3..38c57de71b 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -268,6 +268,17 @@ test_expect_success 'git branch -M topic topic should work when main is checked\n \tgit branch -M topic topic\n '\n \n+test_expect_success 'git branch -M and -C fail on detached HEAD' '\n+\tgit checkout HEAD^{} &&\n+\ttest_when_finished git checkout - &&\n+\techo \"fatal: cannot rename the current branch while not on any.\" >expect &&\n+\ttest_must_fail git branch -M must-fail 2>err &&\n+\ttest_cmp expect err &&\n+\techo \"fatal: cannot copy the current branch while not on any.\" >expect &&\n+\ttest_must_fail git branch -C must-fail 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n \tgit rev-parse --verify refs/heads/t &&\n-- \n2.36.1\n\n"},{"id":"465744","messageId":"xmqqmt9jmpim.fsf@gitster.g","threadId":"58691","inReplyTo":"0ac8cd48-08d7-9bdd-b074-c8d5ded522f6@gmail.com","subject":"Re: [PATCH] branch: error copying or renaming a detached HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-25T23:50:41Z","receivedAt":"2022-10-25T23:50:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Let's remove the condition in copy_or_rename_branch() (the current\n> function name) and check for HEAD before calling it, dying with the\n> original intended error if we're in a detached HEAD.\n\nMakes sense.\n\n> @@ -827,24 +820,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\tstrbuf_release(&buf);\n>  \n>  \t\treturn ret;\n> -\t} else if (copy) {\n> -\t\tif (!argc)\n> -\t\t\tdie(_(\"branch name required\"));\n> -\t\telse if (argc == 1)\n> -\t\t\tcopy_or_rename_branch(head, argv[0], 1, copy > 1);\n> -\t\telse if (argc == 2)\n> -\t\t\tcopy_or_rename_branch(argv[0], argv[1], 1, copy > 1);\n> -\t\telse\n> -\t\t\tdie(_(\"too many branches for a copy operation\"));\n> -\t} else if (rename) {\n> +\t} else if (copy || rename) {\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\nMissing \" \" before \"?\".\n\n>  \t\telse if (argc == 1)\n> -\t\t\tcopy_or_rename_branch(head, argv[0], 0, rename > 1);\n> +\t\t\tcopy_or_rename_branch(head, argv[0], copy, copy + rename > 1);\n\nThe third argument being 'copy' makes sense (as the original has 1\nfor copy and 0 for rename).  As we reject if rename and copy are\nboth set, here we have either copy > 0 or rename > 0 but not both,\nso the fourth argument makes sense, too.\n\n>  \t\telse if (argc == 2)\n> -\t\t\tcopy_or_rename_branch(argv[0], argv[1], 0, rename > 1);\n> +\t\t\tcopy_or_rename_branch(argv[0], argv[1], copy, copy + rename > 1);\n>  \t\telse\n> -\t\t\tdie(_(\"too many arguments for a rename operation\"));\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\nDitto.\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index 7d8edff9c3..38c57de71b 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -268,6 +268,17 @@ test_expect_success 'git branch -M topic topic should work when main is checked\n>  \tgit branch -M topic topic\n>  '\n>  \n> +test_expect_success 'git branch -M and -C fail on detached HEAD' '\n> +\tgit checkout HEAD^{} &&\n> +\ttest_when_finished git checkout - &&\n> +\techo \"fatal: cannot rename the current branch while not on any.\" >expect &&\n> +\ttest_must_fail git branch -M must-fail 2>err &&\n> +\ttest_cmp expect err &&\n> +\techo \"fatal: cannot copy the current branch while not on any.\" >expect &&\n> +\ttest_must_fail git branch -C must-fail 2>err &&\n> +\ttest_cmp expect err\n> +'\n\nExcellent.\n\nI'll do the whitespace tweak while queueing, so there is no need to\nresend only to fix them.\n\nThanks.\n"}]}