{"thread":{"id":"39220","subject":"[PATCH] branch: fix funny-sounding error message","startedAt":"2015-05-02T03:12:40Z","lastAt":"2015-05-05T23:06:30Z","messageCount":5,"participants":["Alex Henrie","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"260412","messageId":"1430536360-21901-1-git-send-email-alexhenrie24@gmail.com","threadId":"39220","inReplyTo":null,"subject":"[PATCH] branch: fix funny-sounding error message","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2015-05-02T03:12:40Z","receivedAt":"2015-05-02T03:12:40Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie \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 1d15037..c0b4bae 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -972,7 +972,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\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\tdie(_(\"could not set upstream of HEAD to %s because \"\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-- \n2.3.7\n"},{"id":"260497","messageId":"xmqq8ud5usi6.fsf@gitster.dls.corp.google.com","threadId":"39220","inReplyTo":"1430536360-21901-1-git-send-email-alexhenrie24@gmail.com","subject":"Re: [PATCH] branch: fix funny-sounding error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-03T23:54:25Z","receivedAt":"2015-05-03T23:54:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> Signed-off-by: Alex Henrie \n> ---\n> builtin/branch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 1d15037..c0b4bae 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -972,7 +972,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\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\tdie(_(\"could not set upstream of HEAD to %s because \"\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\nThanks.\n\nTo me neither sounds so funny, but both sound somewhat awkward,\nprimarily because it is unclear in the first reading what \"it\" in\n\"it does not point at any branch\" refers to.\n\nPerhaps if you explain in the log message to illustrate why you\nfound it funny (and the update text is not), it might help, e.g.\n\n \"git branch\", ran with , when the current\n branch is , dies with\n\n \tfatal: could not set upstream of HEAD to frotz when it does not\n \tpoint to any branch.\n\n which is funny . Saying \"because\"\n makes it .\n\nI suspect that this message is about a nonsense attempt to set an\nupstream for a detached HEAD perhaps? Then\n\n fatal: cannot set upstream for a detached HEAD\n\nmay be shorter and more directly points at the root cause of the\nerror?\n"},{"id":"260617","messageId":"CAMMLpeTYhLwGmy11uGzZAVugQPO6qN6F6pxZ6BiUZKeKnir+zw@mail.gmail.com","threadId":"39220","inReplyTo":"xmqq8ud5usi6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] branch: fix funny-sounding error message","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2015-05-05T19:42:11Z","receivedAt":"2015-05-05T19:42:11Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2015-05-03 17:54 GMT-06:00 Junio C Hamano :\n> Alex Henrie writes:\n>\n>> Signed-off-by: Alex Henrie \n>> ---\n>> builtin/branch.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> index 1d15037..c0b4bae 100644\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -972,7 +972,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>>\n>> if (!branch) {\n>> if (!argc || !strcmp(argv[0], \"HEAD\"))\n>> - die(_(\"could not set upstream of HEAD to %s when \"\n>> + die(_(\"could not set upstream of HEAD to %s because \"\n>> \"it does not point to any branch.\"),\n>> new_upstream);\n>> die(_(\"no such branch '%s'\"), argv[0]);\n>\n> Thanks.\n>\n> To me neither sounds so funny, but both sound somewhat awkward,\n> primarily because it is unclear in the first reading what \"it\" in\n> \"it does not point at any branch\" refers to.\n>\n> Perhaps if you explain in the log message to illustrate why you\n> found it funny (and the update text is not), it might help, e.g.\n>\n> \"git branch\", ran with , when the current\n> branch is , dies with\n>\n> fatal: could not set upstream of HEAD to frotz when it does not\n> point to any branch.\n>\n> which is funny . Saying \"because\"\n> makes it .\n>\n> I suspect that this message is about a nonsense attempt to set an\n> upstream for a detached HEAD perhaps? Then\n>\n> fatal: cannot set upstream for a detached HEAD\n>\n> may be shorter and more directly points at the root cause of the\n> error?\n\nI don't really understand what this code is doing. It just sounds\nfunny to use \"when\" to join a phrase in the past tense with a phrase\nin the present tense.\n\nThis error message can be triggered by running `git branch\n--set-upstream-to=origin/master` from a detached head. But if running\nfrom a detached head is the only way to trigger the error message, why\ndoes the code use strcmp instead of if (detached) { ... } as other\ncode in that function does?\n\nAlso, I missed the complementary error message \"could not unset\nupstream of HEAD when it does not point to any branch.\" We should\neither change the \"when\" to a \"because\" in the second message too or\nrewrite both of these messages entirely.\n\n-Alex\n"},{"id":"260623","messageId":"xmqqvbg6d6v3.fsf@gitster.dls.corp.google.com","threadId":"39220","inReplyTo":"CAMMLpeTYhLwGmy11uGzZAVugQPO6qN6F6pxZ6BiUZKeKnir+zw@mail.gmail.com","subject":"Re: [PATCH] branch: fix funny-sounding error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-05T21:58:24Z","receivedAt":"2015-05-05T21:58:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Henrie writes:\n\n> 2015-05-03 17:54 GMT-06:00 Junio C Hamano :\n>> Alex Henrie writes:\n>>\n>>> Signed-off-by: Alex Henrie \n>>> ---\n>>> builtin/branch.c | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/builtin/branch.c b/builtin/branch.c\n>>> index 1d15037..c0b4bae 100644\n>>> --- a/builtin/branch.c\n>>> +++ b/builtin/branch.c\n>>> @@ -972,7 +972,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>>>\n>>> if (!branch) {\n>>> if (!argc || !strcmp(argv[0], \"HEAD\"))\n>>> - die(_(\"could not set upstream of HEAD to %s when \"\n>>> + die(_(\"could not set upstream of HEAD to %s because \"\n>>> \"it does not point to any branch.\"),\n>>> new_upstream);\n>>> die(_(\"no such branch '%s'\"), argv[0]);\n>>\n>> Thanks.\n>>\n>> To me neither sounds so funny, but both sound somewhat awkward,\n>> primarily because it is unclear in the first reading what \"it\" in\n>> \"it does not point at any branch\" refers to.\n>>\n>> Perhaps if you explain in the log message to illustrate why you\n>> found it funny (and the update text is not), it might help, e.g.\n>>\n>> \"git branch\", ran with , when the current\n>> branch is , dies with\n>>\n>> fatal: could not set upstream of HEAD to frotz when it does not\n>> point to any branch.\n>>\n>> which is funny . Saying \"because\"\n>> makes it .\n>>\n>> I suspect that this message is about a nonsense attempt to set an\n>> upstream for a detached HEAD perhaps? Then\n>>\n>> fatal: cannot set upstream for a detached HEAD\n>>\n>> may be shorter and more directly points at the root cause of the\n>> error?\n>\n> I don't really understand what this code is doing.\n\nTo be honest, I didn't know what was going on at the first glance,\neither. AND I felt that it is unfair to say \"when\" was funny,\nwithout understanding what it is trying to say---perhaps it may be\nimplying that upstream of HEAD could be set to \"%s\" when \"it\" does\npoint at some branch, or something, even though, as I said, \"it\" is\nunclear without really knowing the context.\n\nI think this \"branch is NULL\" condition is when an earlier call to\nbranch_get() returned a NULL, and _one_ way to make that happen is\nto detach the HEAD. There may be others, but I didn't check.\n\n> This error message can be triggered by running `git branch\n> --set-upstream-to=origin/master` from a detached head. But if running\n> from a detached head is the only way to trigger the error message, why\n> does the code use strcmp instead of if (detached) { ... } as other\n> code in that function does?\n\nThe 'detached' merely is \"Is the _current_ HEAD detached?\", not \"Is\nthe branch the user asked us to do something about the detached\nHEAD?\", I think.\n\nIf you are getting a NULL out of branch_get() but the user did\n\"branch -u foo thisbranch\", then that is an indication that we\nshouldn't be saying that \"nono, detached HEAD will never have its\nown upstream\", because that is not what the user asked us to do. So\n\"no argument (implying, we defaulted to HEAD) or the argument is\nspelled HEAD\" I think is the right thing to check.\n\nAnd the other error message \"no such branch\" hints (again, I didn't\nfollow the codepath) that we may get a NULL in branch if you did\n\"git branch -u foo no-such-branch\".\n"},{"id":"260629","messageId":"20150505230630.GC18817@peff.net","threadId":"39220","inReplyTo":"xmqqvbg6d6v3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] branch: fix funny-sounding error message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-05T23:06:30Z","receivedAt":"2015-05-05T23:06:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 05, 2015 at 02:58:24PM -0700, Junio C Hamano wrote:\n\n> I think this \"branch is NULL\" condition is when an earlier call to\n> branch_get() returned a NULL, and _one_ way to make that happen is\n> to detach the HEAD. There may be others, but I didn't check.\n\nFWIW, since I have just been looking through the branch code,\nbranch_get() should only return NULL when:\n\n 1. We passed NULL or \"HEAD\" to it (i.e., we asked for the current branch). If\n you ask for a branch by name, you will always get a non-NULL return,\n even if that branch does not exist.\n\n 2. There is no current branch (i.e., a detached HEAD).\n\nI think the \"no such branch\" error message here:\n\n if (!branch) {\n if (!argc || !strcmp(argv[0], \"HEAD\"))\n die(_(\"could not unset upstream of HEAD when \"\n \"it does not point to any branch.\"));\n\t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n\t }\n\ncannot actually be triggered (and I double-checked that the test suite\ndoes not trigger it by replacing it with \"exit(141)\", which should cause\neven test_must_fail to complain).\n\n-Peff\n"}]}