Re: [PATCH] branch: error and informative messages

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

>>> 	- "no such branch '%s'"
>>> 	- "branch '%s' does not exist"
>>> 	- "invalid branch name: '%s'"
>>> 	+ "no branch named '%s'"
>
> Yes, I had doubts with the third.  The error is referring to the source branch
> in the copy/rename operation, so I think it makes sense to say that the branch
> doesn't exist, even if it couldn't.

OK.  As long as it refers to a branch that ought to exist, then
using the fourth one is perfectly fine.

> I prefer a single term like 'modify' as I find it less confuse than 'set or
> unset'.

OK.

Some folks find it unsure and confusing if 'modification' includes
'deleting/unsetting', and that was why I brought up 'set or unset'.

>>>  - "%s" and "'%s'" was used to format a branch name in different
>>>    messages.  "'%s'" has been used to normalize as it's the more
>>>    frequently used in this file and very common in the rest of the
>>>    codebase.  The opposite has been done for options: "-a" used vs
>>>    "'-a'".
> ...
> Same reasoning as above.  It is a system-chosen term, but the message
> has not a placeholder to put a value, we're using a literal.

I doubt that "same reasoning" is sensible. I'll welcome input from
others, but 

    $ git grep '"[^"'\'']*'\''--[a-z]' \*.c

looked very reasonable, and after imagining the output with them
losing the single quote around the option name, I would think they
are better with the quotes around them.

>>> Finally, let's change the return code on error for --edit-description,
>>> from -1 to 1.
>> 
>> OK.  That last one may be better to be a separate patch, as these
>> wording changes are subject to discussion and bikeshedding.
>
> Mmm, I thought about that.  This change is one that we've been delaying because
> it might break something due to a change in the way we report errors.  We're
> specifically changing this here and the change is small, so I found appropriate
> to do it here.

Not really.  Nobody reads error messages, but programs can react to
exit codes.  It is more important to get the latter right.

>> This does not fall into any of the categories the proposed log
>> message discussed.  Rather, it looks more like "the code
>> subjectively looks better this way".  It happens to much my
>> subjective taste, but that does not change the fact that we
>> shouldn't distract reviewers with such an unrelated change in the
>> same patch.
>  
> It certainly looks subjectively better, and in less lines...

As I said, it does not matter.  It is outside the scope of "improve
error messages" and should be done outside the series, or at lesat
as a separate step in the series.

>> And that should be a separate patch, that can be reviewed and
>> applied regardless of the rest of "error messages cleanup" topic.
>
> Good point.  I didn't think about that and it also goes in the line of
> the previous patches in this file.  I'll review that.  Also gives a good
> opportunity to fix that repeated code /... if (copy) ... else if
> (rename)/.

OK.  But again, that is outside the topic of "improve error
messages".

Thanks.