Re: [PATCH] branch: fix funny-sounding error message

From: Alex Henrie <alexhenrie24@gmail.com>
Date: May 5, 2015, 19:42 UTC
Message-ID: <CAMMLpeTYhLwGmy11uGzZAVugQPO6qN6F6pxZ6BiUZKeKnir+zw@mail.gmail.com>
In-Reply-To: <xmqq8ud5usi6.fsf@gitster.dls.corp.google.com>

2015-05-03 17:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:

Show 46 quoted lines

> Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>> ---
>>  builtin/branch.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 1d15037..c0b4bae 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -972,7 +972,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>
>>               if (!branch) {
>>                       if (!argc || !strcmp(argv[0], "HEAD"))
>> -                             die(_("could not set upstream of HEAD to %s when "
>> +                             die(_("could not set upstream of HEAD to %s because "
>>                                     "it does not point to any branch."),
>>                                   new_upstream);
>>                       die(_("no such branch '%s'"), argv[0]);
>
> Thanks.
>
> To me neither sounds so funny, but both sound somewhat awkward,
> primarily because it is unclear in the first reading what "it" in
> "it does not point at any branch" refers to.
>
> Perhaps if you explain in the log message to illustrate why you
> found it funny (and the update text is not), it might help, e.g.
>
>     "git branch", ran with <this and that options>, when the current
>     branch is <in what state>, dies with
>
>         fatal: could not set upstream of HEAD to frotz when it does not
>         point to any branch.
>
>     which is funny <because of such and such reasons>.  Saying "because"
>     makes it <better beause of such and such reasons>.
>
> I suspect that this message is about a nonsense attempt to set an
> upstream for a detached HEAD perhaps?  Then
>
>     fatal: cannot set upstream for a detached HEAD
>
> may be shorter and more directly points at the root cause of the
> error?

I don't really understand what this code is doing. It just sounds funny to use "when" to join a phrase in the past tense with a phrase in the present tense.

This error message can be triggered by running `git branch --set-upstream-to=origin/master` from a detached head. But if running from a detached head is the only way to trigger the error message, why does the code use strcmp instead of if (detached) { ... } as other code in that function does?

Also, I missed the complementary error message "could not unset upstream of HEAD when it does not point to any branch." We should either change the "when" to a "because" in the second message too or rewrite both of these messages entirely.

-Alex