On 25/10/22 21:28, Junio C Hamano wrote:
>>>> - "%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 :-).
>>>> 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.
>>> 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.
>>> 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.
