From: "Rubén Justo" <rjusto@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] branch: error and informative messages
Date: Wed, 26 Oct 2022 01:52:02 +0200 [thread overview]
Message-ID: <9ceaa13d-5ab3-a36f-b03d-003dbaf3f112@gmail.com> (raw)
In-Reply-To: <xmqqzgdjn1ne.fsf@gitster.g>
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.
next prev parent reply other threads:[~2022-10-25 23:52 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-23 22:29 [PATCH] branch: error and informative messages Rubén Justo
2022-10-24 0:20 ` Junio C Hamano
2022-10-24 23:39 ` Rubén Justo
2022-10-25 19:28 ` Junio C Hamano
2022-10-25 23:52 ` Rubén Justo [this message]
2022-11-06 21:51 ` [PATCH v2] " Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9ceaa13d-5ab3-a36f-b03d-003dbaf3f112@gmail.com \
--to=rjusto@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).