{"thread":{"id":"32958","subject":"\"git branch HEAD\" dumps core when on detached head (NULL pointer dereference)","startedAt":"2013-02-21T12:27:25Z","lastAt":"2013-02-23T20:01:56Z","messageCount":10,"participants":["Per Cederqvist","Duy Nguyen","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"209967","messageId":"512612AD.4000609@opera.com","threadId":"32958","inReplyTo":null,"subject":"\"git branch HEAD\" dumps core when on detached head (NULL pointer dereference)","fromName":"Per Cederqvist","fromEmail":"cederp@opera.com","sentAt":"2013-02-21T12:27:25Z","receivedAt":"2013-02-21T12:27:25Z","isPatch":false,"sender":{"key":"cederp@opera.com","avatar":"https://gravatar.com/avatar/008fe28caedea0ba34042cd5157705267934f4ba7a9b29fb248ae18ec5b3055e?d=mp&s=160"},"body":"Running \"git branch HEAD\" may be a stupid thing to do. It actually\nwas a mistake on my part. Still, I don't think git should dereference\na NULL pointer.\n\nTested using git 1.8.1.4 adn 1.8.1.1.\n\nRepeat by:\n\n mkdir branchcrash || exit 1\n cd branchcrash\n git init\n touch a; git add a; git commit -m\"Added a\".\n touch b; git add b; git commit -m\"Added b\".\n git checkout HEAD^\n git branch HEAD\n\nThe last command dumps core. gdb session:\n\ngdb /usr/local/bin/git core\nGNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04\nCopyright (C) 2012 Free Software Foundation, Inc.\nLicense GPLv3+: GNU GPL version 3 or later \n\nThis is free software: you are free to change and redistribute it.\nThere is NO WARRANTY, to the extent permitted by law. Type \"show copying\"\nand \"show warranty\" for details.\nThis GDB was configured as \"x86_64-linux-gnu\".\nFor bug reporting instructions, please see:\n...\nReading symbols from /usr/local/bin/git...done.\n[New LWP 7174]\n\nwarning: Can't read pathname for load map: Input/output error.\n[Thread debugging using libthread_db enabled]\nUsing host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\nCore was generated by `git branch HEAD'.\nProgram terminated with signal 11, Segmentation fault.\n#0 cmd_branch (argc=1, argv=0x7fffe6e2a0f0, prefix=)\n at builtin/branch.c:919\n919\t\t\tstrbuf_addf(&buf, \"refs/remotes/%s\", branch->name);\n(gdb) bt\n#0 cmd_branch (argc=1, argv=0x7fffe6e2a0f0, prefix=)\n at builtin/branch.c:919\n#1 0x00000000004046ac in run_builtin (argv=0x7fffe6e2a0f0, argc=2,\n p=) at git.c:273\n#2 handle_internal_command (argc=2, argv=0x7fffe6e2a0f0) at git.c:434\n#3 0x0000000000404df3 in run_argv (argv=0x7fffe6e29f90, \nargcp=0x7fffe6e29f9c)\n at git.c:480\n#4 main (argc=2, argv=0x7fffe6e2a0f0) at git.c:555\n(gdb) p branch\n$1 = (struct branch *) 0x0\n(gdb) quit\n\n /ceder\n"},{"id":"209968","messageId":"CACsJy8CuRvwsbXbcKr7dHneEDF7UmoG4ioCxfDi_7qWDD-4wgQ@mail.gmail.com","threadId":"32958","inReplyTo":"512612AD.4000609@opera.com","subject":"Re: \"git branch HEAD\" dumps core when on detached head (NULL pointer dereference)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-21T12:50:19Z","receivedAt":"2013-02-21T12:50:19Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 21, 2013 at 7:27 PM, Per Cederqvist wrote:\n> Running \"git branch HEAD\" may be a stupid thing to do. It actually\n> was a mistake on my part. Still, I don't think git should dereference\n> a NULL pointer.\n\nWe should not. Can you make a patch to fix it (with test cases)? You\nmay want to fix the two preceding blocks, \"if (new_upstream)\" and \"if\n(unset_upstream)\", as well. They don't check for NULL branch either. I\nthink we can say something like \"detached HEAD is not valid for this\noperation\" before exit.\n\n>\n> Tested using git 1.8.1.4 adn 1.8.1.1.\n>\n> Repeat by:\n>\n> mkdir branchcrash || exit 1\n> cd branchcrash\n> git init\n> touch a; git add a; git commit -m\"Added a\".\n> touch b; git add b; git commit -m\"Added b\".\n> git checkout HEAD^\n> git branch HEAD\n>\n> The last command dumps core. gdb session:\n>\n> gdb /usr/local/bin/git core\n> GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04\n> Copyright (C) 2012 Free Software Foundation, Inc.\n> License GPLv3+: GNU GPL version 3 or later\n> \n> This is free software: you are free to change and redistribute it.\n> There is NO WARRANTY, to the extent permitted by law. Type \"show copying\"\n> and \"show warranty\" for details.\n> This GDB was configured as \"x86_64-linux-gnu\".\n> For bug reporting instructions, please see:\n> ...\n> Reading symbols from /usr/local/bin/git...done.\n> [New LWP 7174]\n>\n> warning: Can't read pathname for load map: Input/output error.\n> [Thread debugging using libthread_db enabled]\n> Using host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n> Core was generated by `git branch HEAD'.\n> Program terminated with signal 11, Segmentation fault.\n> #0 cmd_branch (argc=1, argv=0x7fffe6e2a0f0, prefix=)\n> at builtin/branch.c:919\n> 919 strbuf_addf(&buf, \"refs/remotes/%s\", branch->name);\n> (gdb) bt\n> #0 cmd_branch (argc=1, argv=0x7fffe6e2a0f0, prefix=)\n> at builtin/branch.c:919\n> #1 0x00000000004046ac in run_builtin (argv=0x7fffe6e2a0f0, argc=2,\n> p=) at git.c:273\n> #2 handle_internal_command (argc=2, argv=0x7fffe6e2a0f0) at git.c:434\n> #3 0x0000000000404df3 in run_argv (argv=0x7fffe6e29f90,\n> argcp=0x7fffe6e29f9c)\n> at git.c:480\n> #4 main (argc=2, argv=0x7fffe6e2a0f0) at git.c:555\n> (gdb) p branch\n> $1 = (struct branch *) 0x0\n> (gdb) quit\n>\n> /ceder\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nDuy\n"},{"id":"209971","messageId":"51261FF9.2090304@opera.com","threadId":"32958","inReplyTo":"CACsJy8CuRvwsbXbcKr7dHneEDF7UmoG4ioCxfDi_7qWDD-4wgQ@mail.gmail.com","subject":"Re: \"git branch HEAD\" dumps core when on detached head (NULL pointer dereference)","fromName":"Per Cederqvist","fromEmail":"cederp@opera.com","sentAt":"2013-02-21T13:24:09Z","receivedAt":"2013-02-21T13:24:09Z","isPatch":false,"sender":{"key":"cederp@opera.com","avatar":"https://gravatar.com/avatar/008fe28caedea0ba34042cd5157705267934f4ba7a9b29fb248ae18ec5b3055e?d=mp&s=160"},"body":"On 02/21/13 13:50, Duy Nguyen wrote:\n> On Thu, Feb 21, 2013 at 7:27 PM, Per Cederqvist wrote:\n>> Running \"git branch HEAD\" may be a stupid thing to do. It actually\n>> was a mistake on my part. Still, I don't think git should dereference\n>> a NULL pointer.\n>\n> We should not. Can you make a patch to fix it (with test cases)? You\n> may want to fix the two preceding blocks, \"if (new_upstream)\" and \"if\n> (unset_upstream)\", as well. They don't check for NULL branch either. I\n> think we can say something like \"detached HEAD is not valid for this\n> operation\" before exit.\n\nSorry, but isolating the issue reporting it here is about as much time\nas I can spend on this issue. Learning the coding standard of Git and\nhow to write test cases is not something I'm prepared to do, at least\nnot at the moment. I hope there is a place for users and reporters of\nbugs in the Git community.\n\n /ceder\n\n>>\n>> Tested using git 1.8.1.4 adn 1.8.1.1.\n>>\n>> Repeat by:\n>>\n>> mkdir branchcrash || exit 1\n>> cd branchcrash\n>> git init\n>> touch a; git add a; git commit -m\"Added a\".\n>> touch b; git add b; git commit -m\"Added b\".\n>> git checkout HEAD^\n>> git branch HEAD\n>>\n>> The last command dumps core. gdb session:\n>>\n>> gdb /usr/local/bin/git core\n>> GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04\n>> Copyright (C) 2012 Free Software Foundation, Inc.\n>> License GPLv3+: GNU GPL version 3 or later\n>> \n>> This is free software: you are free to change and redistribute it.\n>> There is NO WARRANTY, to the extent permitted by law. Type \"show copying\"\n>> and \"show warranty\" for details.\n>> This GDB was configured as \"x86_64-linux-gnu\".\n>> For bug reporting instructions, please see:\n>> ...\n>> Reading symbols from /usr/local/bin/git...done.\n>> [New LWP 7174]\n>>\n>> warning: Can't read pathname for load map: Input/output error.\n>> [Thread debugging using libthread_db enabled]\n>> Using host libthread_db library \"/lib/x86_64-linux-gnu/libthread_db.so.1\".\n>> Core was generated by `git branch HEAD'.\n>> Program terminated with signal 11, Segmentation fault.\n>> #0 cmd_branch (argc=1, argv=0x7fffe6e2a0f0, prefix=)\n>> at builtin/branch.c:919\n>> 919 strbuf_addf(&buf, \"refs/remotes/%s\", branch->name);\n>> (gdb) bt\n>> #0 cmd_branch (argc=1, argv=0x7fffe6e2a0f0, prefix=)\n>> at builtin/branch.c:919\n>> #1 0x00000000004046ac in run_builtin (argv=0x7fffe6e2a0f0, argc=2,\n>> p=) at git.c:273\n>> #2 handle_internal_command (argc=2, argv=0x7fffe6e2a0f0) at git.c:434\n>> #3 0x0000000000404df3 in run_argv (argv=0x7fffe6e29f90,\n>> argcp=0x7fffe6e29f9c)\n>> at git.c:480\n>> #4 main (argc=2, argv=0x7fffe6e2a0f0) at git.c:555\n>> (gdb) p branch\n>> $1 = (struct branch *) 0x0\n>> (gdb) quit\n>>\n>> /ceder\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>\n>\n>\n"},{"id":"209972","messageId":"CACsJy8B_jfgvneRtF0u=G_bLbb_SKEi4s=T8OPUtcV9a3mK_vw@mail.gmail.com","threadId":"32958","inReplyTo":"51261FF9.2090304@opera.com","subject":"Re: \"git branch HEAD\" dumps core when on detached head (NULL pointer dereference)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-21T13:32:27Z","receivedAt":"2013-02-21T13:32:27Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 21, 2013 at 8:24 PM, Per Cederqvist wrote:\n> Sorry, but isolating the issue reporting it here is about as much time\n> as I can spend on this issue. Learning the coding standard of Git and\n> how to write test cases is not something I'm prepared to do, at least\n> not at the moment. I hope there is a place for users and reporters of\n> bugs in the Git community.\n\nSure. No problem. I just thought you might want to finish it off. I'll\nlook into it.\n-- \nDuy\n"},{"id":"209973","messageId":"1361456285-29611-1-git-send-email-pclouds@gmail.com","threadId":"32958","inReplyTo":"512612AD.4000609@opera.com","subject":"[PATCH] branch: segfault fixes and validation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-21T14:18:05Z","receivedAt":"2013-02-21T14:18:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"branch_get() can return NULL (so far on detached HEAD only) but some\ncode paths in builtin/branch.c cannot deal with that and cause\nsegfaults. Fix it.\n\nWhile at there, make sure to bail out when the user gives 2 or more\narguments, but only the first one is processed.\n\nReported-by: Per Cederqvist \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/branch.c | 20 ++++++++++++++++++++\n t/t3200-branch.sh | 21 +++++++++++++++++++++\n 2 files changed, 41 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6371bf9..c1d688e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -889,6 +889,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (new_upstream) {\n \t\tstruct branch *branch = branch_get(argv[0]);\n \n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"too many branches to set new upstream\"));\n+\n+\t\tif (!branch)\n+\t\t\tdie(_(\"could not figure out the branch name from '%s'\"),\n+\t\t\t argc == 1 ? argv[0] : \"HEAD\");\n+\n \t\tif (!ref_exists(branch->refname))\n \t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n \n@@ -901,6 +908,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstruct branch *branch = branch_get(argv[0]);\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"too many branches to unset upstream\"));\n+\n+\t\tif (!branch)\n+\t\t\tdie(_(\"could not figure out the branch name from '%s'\"),\n+\t\t\t argc == 1 ? argv[0] : \"HEAD\");\n+\n \t\tif (!branch_has_merge_config(branch)) {\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \t\t}\n@@ -916,6 +930,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tint branch_existed = 0, remote_tracking = 0;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (!strcmp(argv[0], \"HEAD\"))\n+\t\t\tdie(_(\"it does not make sense to create 'HEAD' manually\"));\n+\n+\t\tif (!branch)\n+\t\t\tdie(_(\"could not figure out the branch name from '%s'\"), argv[0]);\n+\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex f3e0e4a..12f1e4a 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -42,6 +42,10 @@ test_expect_success \\\n 'git branch a/b/c should create a branch' \\\n 'git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c'\n \n+test_expect_success \\\n+ 'git branch HEAD should fail' \\\n+ 'test_must_fail git branch HEAD'\n+\n cat >expect < 1117150200 +0000\tbranch: Created from master\n EOF\n@@ -388,6 +392,14 @@ test_expect_success \\\n 'git tag foobar &&\n test_must_fail git branch --track my11 foobar'\n \n+test_expect_success '--set-upstream-to fails on multiple branches' \\\n+ 'test_must_fail git branch --set-upstream-to master a b c'\n+\n+test_expect_success '--set-upstream-to fails on detached HEAD' \\\n+ 'git checkout HEAD^{} &&\n+ test_must_fail git branch --set-upstream-to master &&\n+ git checkout -'\n+\n test_expect_success 'use --set-upstream-to modify HEAD' \\\n 'test_config branch.master.remote foo &&\n test_config branch.master.merge foo &&\n@@ -417,6 +429,15 @@ test_expect_success 'test --unset-upstream on HEAD' \\\n test_must_fail git branch --unset-upstream\n '\n \n+test_expect_success '--unset-upstream should fail on multiple branches' \\\n+ 'test_must_fail git branch --unset-upstream a b c'\n+\n+test_expect_success '--unset-upstream should fail on detached HEAD' \\\n+ 'git checkout HEAD^{} &&\n+ test_must_fail git branch --unset-upstream &&\n+ git checkout -\n+'\n+\n test_expect_success 'test --unset-upstream on a particular branch' \\\n 'git branch my15\n git branch --set-upstream-to master my14 &&\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"209981","messageId":"7vvc9lv9rt.fsf@alter.siamese.dyndns.org","threadId":"32958","inReplyTo":"1361456285-29611-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] branch: segfault fixes and validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T17:47:50Z","receivedAt":"2013-02-21T17:47:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> branch_get() can return NULL (so far on detached HEAD only)...\n\nDo you anticipate any other cases where the API call should validly\nreturn NULL? I offhand do not, ...\n\n> but some\n> code paths in builtin/branch.c cannot deal with that and cause\n> segfaults. Fix it.\n>\n> While at there, make sure to bail out when the user gives 2 or more\n> arguments, but only the first one is processed.\n>\n> Reported-by: Per Cederqvist \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> builtin/branch.c | 20 ++++++++++++++++++++\n> t/t3200-branch.sh | 21 +++++++++++++++++++++\n> 2 files changed, 41 insertions(+)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 6371bf9..c1d688e 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -889,6 +889,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t} else if (new_upstream) {\n> \t\tstruct branch *branch = branch_get(argv[0]);\n> \n> +\t\tif (argc > 1)\n> +\t\t\tdie(_(\"too many branches to set new upstream\"));\n> +\n> +\t\tif (!branch)\n> +\t\t\tdie(_(\"could not figure out the branch name from '%s'\"),\n> +\t\t\t argc == 1 ? argv[0] : \"HEAD\");\n\n... and find this \"could not figure out\" very unfriendly to the\nuser. Is it a bug in the implementation, silly Git failing to find\nwhat branch the user meant? What recourse does the user have at\nthis point?\n\nOr is it a user error to ask Git to operate on the branch pointed at\nby HEAD, when HEAD does not refer to any branch? If that is the\ncase, then the message should say that there is no current branch to\noperate on because the user is on a detached HEAD. That would point\nthe user in the right direction to correct the user error, no?\n\nOf course, argv[0] may not be HEAD and in that case you may have to\nsay \"no such branch %s\" % argv[0] or something. The point is that\n\"could not figure out\" feels a bit too lazy.\n\n> @@ -901,6 +908,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\tstruct branch *branch = branch_get(argv[0]);\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \n> +\t\tif (argc > 1)\n> +\t\t\tdie(_(\"too many branches to unset upstream\"));\n> +\n> +\t\tif (!branch)\n> +\t\t\tdie(_(\"could not figure out the branch name from '%s'\"),\n> +\t\t\t argc == 1 ? argv[0] : \"HEAD\");\n\nLikewise.\n\n> @@ -916,6 +930,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\tint branch_existed = 0, remote_tracking = 0;\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \n> +\t\tif (!strcmp(argv[0], \"HEAD\"))\n> +\t\t\tdie(_(\"it does not make sense to create 'HEAD' manually\"));\n> +\n> +\t\tif (!branch)\n> +\t\t\tdie(_(\"could not figure out the branch name from '%s'\"), argv[0]);\n\nLikewise.\n\nThanks.\n"},{"id":"210024","messageId":"1361533663-3172-1-git-send-email-pclouds@gmail.com","threadId":"32958","inReplyTo":"7vvc9lv9rt.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] branch: segfault fixes and validation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-22T11:47:43Z","receivedAt":"2013-02-22T11:47:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"branch_get() can return NULL (so far on detached HEAD only) but some\ncode paths in builtin/branch.c cannot deal with that and cause\nsegfaults. While at there, make sure we bail out when the user gives 2\nor more arguments, but we only process the first one and silently\nignore the rest.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Fri, Feb 22, 2013 at 12:47 AM, Junio C Hamano wrote:\n > Nguyễn Thái Ngọc Duy writes:\n >\n >> branch_get() can return NULL (so far on detached HEAD only)...\n >\n > Do you anticipate any other cases where the API call should validly\n > return NULL?\n \n No. But I do not see any guarantee that it may never do that in\n future either. Which is why I was deliberately vague with \"could not\n figure out...\". But you also correctly observed my laziness there. So\n how about this? It makes a special case for HEAD but not insist on\n detached HEAD as the only cause.\n\n builtin/branch.c | 24 ++++++++++++++++++++++++\n t/t3200-branch.sh | 21 +++++++++++++++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6371bf9..82ed337 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -889,6 +889,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (new_upstream) {\n \t\tstruct branch *branch = branch_get(argv[0]);\n \n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"too many branches to set new upstream\"));\n+\n+\t\tif (!branch) {\n+\t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n+\t\t\t\tdie(_(\"HEAD does not point to any branch. Is it detached?\"));\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(_(\"branch '%s' does not exist\"), branch->name);\n \n@@ -901,6 +910,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstruct branch *branch = branch_get(argv[0]);\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"too many branches to unset upstream\"));\n+\n+\t\tif (!branch) {\n+\t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n+\t\t\t\tdie(_(\"HEAD does not point to any branch. Is it detached?\"));\n+\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n+\t\t}\n+\n \t\tif (!branch_has_merge_config(branch)) {\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \t\t}\n@@ -916,6 +934,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tint branch_existed = 0, remote_tracking = 0;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (!strcmp(argv[0], \"HEAD\"))\n+\t\t\tdie(_(\"it does not make sense to create 'HEAD' manually\"));\n+\n+\t\tif (!branch)\n+\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n+\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex f3e0e4a..12f1e4a 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -42,6 +42,10 @@ test_expect_success \\\n 'git branch a/b/c should create a branch' \\\n 'git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c'\n \n+test_expect_success \\\n+ 'git branch HEAD should fail' \\\n+ 'test_must_fail git branch HEAD'\n+\n cat >expect < 1117150200 +0000\tbranch: Created from master\n EOF\n@@ -388,6 +392,14 @@ test_expect_success \\\n 'git tag foobar &&\n test_must_fail git branch --track my11 foobar'\n \n+test_expect_success '--set-upstream-to fails on multiple branches' \\\n+ 'test_must_fail git branch --set-upstream-to master a b c'\n+\n+test_expect_success '--set-upstream-to fails on detached HEAD' \\\n+ 'git checkout HEAD^{} &&\n+ test_must_fail git branch --set-upstream-to master &&\n+ git checkout -'\n+\n test_expect_success 'use --set-upstream-to modify HEAD' \\\n 'test_config branch.master.remote foo &&\n test_config branch.master.merge foo &&\n@@ -417,6 +429,15 @@ test_expect_success 'test --unset-upstream on HEAD' \\\n test_must_fail git branch --unset-upstream\n '\n \n+test_expect_success '--unset-upstream should fail on multiple branches' \\\n+ 'test_must_fail git branch --unset-upstream a b c'\n+\n+test_expect_success '--unset-upstream should fail on detached HEAD' \\\n+ 'git checkout HEAD^{} &&\n+ test_must_fail git branch --unset-upstream &&\n+ git checkout -\n+'\n+\n test_expect_success 'test --unset-upstream on a particular branch' \\\n 'git branch my15\n git branch --set-upstream-to master my14 &&\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"210052","messageId":"7vvc9kccwa.fsf@alter.siamese.dyndns.org","threadId":"32958","inReplyTo":"1361533663-3172-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] branch: segfault fixes and validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T20:27:33Z","receivedAt":"2013-02-22T20:27:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> branch_get() can return NULL (so far on detached HEAD only) but some\n> code paths in builtin/branch.c cannot deal with that and cause\n> segfaults. While at there, make sure we bail out when the user gives 2\n> or more arguments, but we only process the first one and silently\n> ignore the rest.\n\nExplain \"2 or more arguments\" in what context, perhaps? Otherwise\nit makes it sound as if \"git branch foo bar baz\" is covered with\nthis patch, no?\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> On Fri, Feb 22, 2013 at 12:47 AM, Junio C Hamano wrote:\n> > Nguyễn Thái Ngọc Duy writes:\n> >\n> >> branch_get() can return NULL (so far on detached HEAD only)...\n> >\n> > Do you anticipate any other cases where the API call should validly\n> > return NULL?\n> \n> No. But I do not see any guarantee that it may never do that in\n> future either. Which is why I was deliberately vague with \"could not\n> figure out...\". But you also correctly observed my laziness there. So\n> how about this? It makes a special case for HEAD but not insist on\n> detached HEAD as the only cause.\n\nSure. It looks better.\n\nWhat you can do is to have a single helper function that can explain\nwhy branch_get() returned NULL (or extend branch_get() to serve that\npurpose as well); then you do not have to duplicate the logic twice\non the caller's side (and there may be other callers that want to do\nthe same).\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 6371bf9..82ed337 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -889,6 +889,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t} else if (new_upstream) {\n> \t\tstruct branch *branch = branch_get(argv[0]);\n> \n> +\t\tif (argc > 1)\n> +\t\t\tdie(_(\"too many branches to set new upstream\"));\n> +\n> +\t\tif (!branch) {\n> +\t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n> +\t\t\t\tdie(_(\"HEAD does not point to any branch. Is it detached?\"));\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(_(\"branch '%s' does not exist\"), branch->name);\n\nThe latter part of the new code triggers when branch_get() returns\nNULL while doing \"git branch --set-upstream-to=X [Y]\". When \"Y\" is\nstring \"HEAD\" or missing, the first die() is triggered and says a\nfunny thing. If HEAD does not point to any branch, by definition it\nis detached. The user may say \"Yes, I know it is detached.\" but the\nmessage does not help the user to figure out what to do next.\n\nInstead of asking \"Is it detached?\", perhaps we can say something\nlike \"You told me to set the upstream of HEAD to branch X, but \" in\nfront? At least, that will be a better explanation for the reason\nwhy the operation is failing.\n\nThe existing test might be wrong, by the way. Your HEAD may point\nat a branch Y but you may not have any commit on it yet, and you may\nwant to allow setting the upstream of that to-be-born branch to\nanother branch X with \"branch --set-upstream-to=X [Y|HEAD]\".\n\nWhile I think it is insane to do anything before creating the first\ncommit on your current branch (or using \"checkout --orphan\" in\ngeneral) and it may not be worth our time to babysit users who do\nso, but the following sequence may feel natural to them:\n\n\tgit checkout --orphan X\n git branch --set-upstream-to=master\n\n\t... perhaps create an initial commit, perhaps not ...\n\n\tgit merge @{upstream}\n\nFor that to work sanely, perhaps the pattern\n\n\tbranch = branch_get();\n if (!branch)\n\t\tdie due to no branch;\n\tif (!ref_exists(branch->refname))\n\t\tdie due to typo in branch name\n\nmay need to be fixed globally, replacing ref_exists(branch->refname)\nwith branch_exists(branch) that returns true if branch->refname is\nan existing ref, or the branch in question was obtained by checking\nwith current_branch (in remote.c), or something like that.\n"},{"id":"210086","messageId":"1361622147-20921-1-git-send-email-pclouds@gmail.com","threadId":"32958","inReplyTo":"7vvc9kccwa.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] branch: segfault fixes and validation","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-02-23T12:22:27Z","receivedAt":"2013-02-23T12:22:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"branch_get() can return NULL (so far on detached HEAD only) but some\ncode paths in builtin/branch.c cannot deal with that and cause\nsegfaults.\n\nWhile at there, make sure to bail out when the user gives 2 or more\nbranches with --set-upstream-to or --unset-upstream, where only the\nfirst branch is processed and the rest silently dropped.\n\nReported-by: Per Cederqvist \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n On Sat, Feb 23, 2013 at 3:27 AM, Junio C Hamano wrote:\n > Instead of asking \"Is it detached?\", perhaps we can say something\n > like \"You told me to set the upstream of HEAD to branch X, but \" in\n > front? At least, that will be a better explanation for the reason\n > why the operation is failing.\n\n Fixed.\n\n > What you can do is to have a single helper function that can explain\n > why branch_get() returned NULL (or extend branch_get() to serve that\n > purpose as well); then you do not have to duplicate the logic twice\n > on the caller's side (and there may be other callers that want to do\n > the same).\n\n The explanation mentions about the failed operation, which makes a\n helper less useful. We could still do the helper, but it may lead to\n i18n legos. So no helper in this version.\n\n > The existing test might be wrong, by the way. Your HEAD may point\n > at a branch Y but you may not have any commit on it yet, and you may\n > want to allow setting the upstream of that to-be-born branch to\n > another branch X with \"branch --set-upstream-to=X [Y|HEAD]\".\n\n It sounds complicated. I think we can revisit it when a user actually\n complains about it.\n\n builtin/branch.c | 27 +++++++++++++++++++++++++++\n t/t3200-branch.sh | 21 +++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6371bf9..00d17d2 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -889,6 +889,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (new_upstream) {\n \t\tstruct branch *branch = branch_get(argv[0]);\n \n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"too many branches to set new upstream\"));\n+\n+\t\tif (!branch) {\n+\t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n+\t\t\t\tdie(_(\"could not set upstream of HEAD to %s when \"\n+\t\t\t\t \"it does not point to any branch.\"),\n+\t\t\t\t new_upstream);\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(_(\"branch '%s' does not exist\"), branch->name);\n \n@@ -901,6 +912,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstruct branch *branch = branch_get(argv[0]);\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (argc > 1)\n+\t\t\tdie(_(\"too many branches to unset upstream\"));\n+\n+\t\tif (!branch) {\n+\t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n+\t\t\t\tdie(_(\"could not unset upstream of HEAD when \"\n+\t\t\t\t \"it does not point to any branch.\"));\n+\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n+\t\t}\n+\n \t\tif (!branch_has_merge_config(branch)) {\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \t\t}\n@@ -916,6 +937,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tint branch_existed = 0, remote_tracking = 0;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n+\t\tif (!strcmp(argv[0], \"HEAD\"))\n+\t\t\tdie(_(\"it does not make sense to create 'HEAD' manually\"));\n+\n+\t\tif (!branch)\n+\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n+\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex f3e0e4a..12f1e4a 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -42,6 +42,10 @@ test_expect_success \\\n 'git branch a/b/c should create a branch' \\\n 'git branch a/b/c && test_path_is_file .git/refs/heads/a/b/c'\n \n+test_expect_success \\\n+ 'git branch HEAD should fail' \\\n+ 'test_must_fail git branch HEAD'\n+\n cat >expect < 1117150200 +0000\tbranch: Created from master\n EOF\n@@ -388,6 +392,14 @@ test_expect_success \\\n 'git tag foobar &&\n test_must_fail git branch --track my11 foobar'\n \n+test_expect_success '--set-upstream-to fails on multiple branches' \\\n+ 'test_must_fail git branch --set-upstream-to master a b c'\n+\n+test_expect_success '--set-upstream-to fails on detached HEAD' \\\n+ 'git checkout HEAD^{} &&\n+ test_must_fail git branch --set-upstream-to master &&\n+ git checkout -'\n+\n test_expect_success 'use --set-upstream-to modify HEAD' \\\n 'test_config branch.master.remote foo &&\n test_config branch.master.merge foo &&\n@@ -417,6 +429,15 @@ test_expect_success 'test --unset-upstream on HEAD' \\\n test_must_fail git branch --unset-upstream\n '\n \n+test_expect_success '--unset-upstream should fail on multiple branches' \\\n+ 'test_must_fail git branch --unset-upstream a b c'\n+\n+test_expect_success '--unset-upstream should fail on detached HEAD' \\\n+ 'git checkout HEAD^{} &&\n+ test_must_fail git branch --unset-upstream &&\n+ git checkout -\n+'\n+\n test_expect_success 'test --unset-upstream on a particular branch' \\\n 'git branch my15\n git branch --set-upstream-to master my14 &&\n-- \n1.8.1.2.536.gf441e6d\n"},{"id":"210103","messageId":"7vtxp27qa3.fsf@alter.siamese.dyndns.org","threadId":"32958","inReplyTo":"1361622147-20921-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3] branch: segfault fixes and validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-23T20:01:56Z","receivedAt":"2013-02-23T20:01:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> > What you can do is to have a single helper function that can explain\n> > why branch_get() returned NULL (or extend branch_get() to serve that\n> > purpose as well); then you do not have to duplicate the logic twice\n> > on the caller's side (and there may be other callers that want to do\n> > the same).\n>\n> The explanation mentions about the failed operation, which makes a\n> helper less useful. We could still do the helper, but it may lead to\n> i18n legos. So no helper in this version.\n\nWho said explanation has to be conveyed in human language to the\ncaller of the helper? Since there are only two possible cases, at\nleast for now, and I do not think there will be countless many in\nthe future, for the branch_get() function to fail, you can still do:\n\n \tint explain_error;\n\tstruct branch *branch = branch_get(argv[0], &explain_error);\n\n switch (explain_error) {\n\tcase DEFAULT_HEAD_DETACHED:\n case GIVEN_HEAD_DETACHED:\n\t\tdie(_(\"could not make %s upstream of the current branch \"\n\t\t\t\"because you do not have one\"), new_upstream);\n\t\tbreak;\n\tdefault:\n\t\tbreak;\n }\n\nand we could even fold the !ref_exists() check that is there for\ntypo-detection purposes into the framework, e.g.\n\n\tcase GIVEN_BRANCH_DOES_NOT_EXIST:\n\t\tdie(_(\"you told me to make %s upstream of %s \"\n\t\t\t\"but the latter does not exist yet\"),\n\t\t\tnew_upstream, argv[0]);\n\nif we wanted to preserve what the current test does, no?\n\n> > The existing test might be wrong, by the way. Your HEAD may point\n> > at a branch Y but you may not have any commit on it yet, and you may\n> > want to allow setting the upstream of that to-be-born branch to\n> > another branch X with \"branch --set-upstream-to=X [Y|HEAD]\".\n>\n> It sounds complicated. I think we can revisit it when a user actually\n> complains about it.\n\nYeah, will replace the previous one and queue this version.\n\nThanks.\n"}]}