Re: [PATCH] branch: error and informative messages

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 25, 2022, 19:28 UTC
Message-ID: <xmqqzgdjn1ne.fsf@gitster.g>
In-Reply-To: <faf7a985-f6ef-f20a-3857-031396124d60@gmail.com>

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

Show 8 quoted lines

>>> 	- "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'.

Show 8 quoted lines

>>>  - "%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.

Show 10 quoted lines

>>> 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.

Show 8 quoted lines

>> 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.

Show 7 quoted lines

>> 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.