Re: [PATCH] branch: error and informative messages

From: Rubén Justo <rjusto@gmail.com>
Date: Oct 25, 2022, 23:52 UTC
Message-ID: <9ceaa13d-5ab3-a36f-b03d-003dbaf3f112@gmail.com>
In-Reply-To: <xmqqzgdjn1ne.fsf@gitster.g>

On 25/10/22 21:28, Junio C Hamano wrote:

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

The reasoning I do is:
- an option is a literal, cannot change in the message, with unexpected chars
  for example
- this makes it is well delimited.
- the dashes clearly differentiate from a simple word
- we do not use quotes for options other in places, like the documentation

But looks like we have a mix:

$ git grep '\(die\|error\|warning\).*"[^"'\'']*--[a-z]' \*.c | head -12

apply.c: return error(_("options '%s' and '%s' cannot be used together"), "--reject", "--3way"); apply.c: return error(_("'%s' outside a repository"), "--index"); apply.c: return error(_("'%s' outside a repository"), "--cached"); apply.c: error(_("No valid patches in input (allow with \"--allow-empty\")")); archive.c: die(_("Unexpected option --remote")); archive.c: die(_("the option '%s' requires '%s'"), "--exec", "--remote"); archive.c: die(_("Unexpected option --output")); archive.c: die(_("options '%s' and '%s' cannot be used together"), "--add-file", "--remote"); blame.c: die(_("--contents and --reverse do not blend well.")); blame.c: die(_("cannot use --contents with final commit object name")); blame.c: die(_("--reverse and --first-parent together require specified latest commit")); blame.c: die(_("--reverse --first-parent together require range along first-parent chain"));

I prefer the unquoted form, because of the previous reasons. But I don't have a strong opinion on that, beyond using the same criteria in the file :-).

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

OK. I just sent a patch for this.

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

OK. I will separate this in a preparatory step.

Show 10 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".

OK. I just sent a patch for this.

I'll wait a few days before sending a new version, to give others time to comment.

Thanks.