Rubén Justo <rjusto@xxxxxxxxx> 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.
