From: Dragan Simic <dsimic@manjaro.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH RESEND] send-email: make produced outputs more readable
Date: Thu, 04 Apr 2024 21:40:32 +0200 [thread overview]
Message-ID: <44ed7af52e2968993d9b2b99faec1f64@manjaro.org> (raw)
In-Reply-To: <xmqqy19tylrm.fsf@gitster.g>
Hello Junio,
On 2024-04-04 21:23, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Notes:
>> * send-email: make produced outputs more readable by separating
>> the result statuses from the subsequent patch outputs
>>
>> This is a resubmission of the patch I submitted about a week and a
>> half
>> ago. [1] The patch subject in the original submission was
>> selected in
>> a bit unfortunate way, which this submission corrects, and also
>> improves
>> the patch description a bit. There are no changes to the patch
>> itself.
>
> I tried to cram a bit more information than "output more readable"
> that lacks in what way the result is easier to read.
>
> send-email: make boundaries between messages easier to spot
>
> perhaps?
Looking good to me, will use that as the patch subject in v2.
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 821b2b3a135a..62505ab2707c 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1576,7 +1576,6 @@ sub send_message {
>> print $sm "$header\n$message";
>> close $sm or die $!;
>> } else {
>> -
>> if (!defined $smtp_server) {
>> die __("The required SMTP server is not properly defined.")
>> }
>> @@ -1686,9 +1685,9 @@ sub send_message {
>> print $header, "\n";
>> if ($smtp) {
>> print __("Result: "), $smtp->code, ' ',
>> - ($smtp->message =~ /\n([^\n]+\n)$/s), "\n";
>> + ($smtp->message =~ /\n([^\n]+\n)$/s), "\n\n";
>> } else {
>> - print __("Result: OK\n");
>> + print __("Result: OK\n\n");
>> }
>
> It would be nicer to instead add a single separate
>
> print "\n";
>
> after these if/else alternatives, without touching the existing
> message lines, I would think. That way, existing message
> translations do not have to change.
>
> If we were to change the translatable string anyway, it would be
> even better to remove the newline from the translatable part of the
> message, rendering the thing to:
>
> if ($smtp) {
> print __("Result: "), ..., ($smtp->message =~ /.../);
> } else {
> print __("Result: OK");
> }
> print "\n\n";
>
> Strictly speaking, that is an orthogonal clean-up, so it may have to
> make it into two patch series, one for preliminary clean-up "to
> excise terminating newline out of translatable strings" patch that
> adds a separate print that adds a single newline, plus the "make it
> easier to spot where a message ends and another one starts" patch
> that makes the new print statement that adds a single newline to
> instead add two. In a patch as simple as this one, however, I think
> killing two birds with a stone, i.e., directly go to the "if we were
> to change the translatable string anyway" final shape in a single
> patch, would be fine.
Thank you very much for such a detailed review! I also think that
putting everything into a single patch is a better option, because
the changes are rather small. Will do that in v2.
prev parent reply other threads:[~2024-04-04 19:41 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-04 17:07 [PATCH RESEND] send-email: make produced outputs more readable Dragan Simic
2024-04-04 19:23 ` Junio C Hamano
2024-04-04 19:40 ` Dragan Simic [this message]
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=44ed7af52e2968993d9b2b99faec1f64@manjaro.org \
--to=dsimic@manjaro.org \
--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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).