From: Dragan Simic <dsimic@manjaro.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, code@khaugsbakk.name
Subject: Re: [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch
Date: Tue, 09 Apr 2024 05:37:43 +0200 [thread overview]
Message-ID: <2b24b9e8f0fcb4f6df595c1b0be06359@manjaro.org> (raw)
In-Reply-To: <xmqq7ch7a7gt.fsf@gitster.g>
On 2024-04-08 23:08, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> ... To make the produced outputs more readable, add vertical
>> whitespace (more precisely, a newline) between the displayed result
>> statuses
>> and the subsequent messages, as visible in ...
>
> The above feels a bit roundabout way to say "the logic is that we
> need to add a gap before showing the next message, if we did things
> that cause the smtp traces to be shown", but OK.
Yeah, the wording I used isn't perfect, but I think it's still
understandable. I'll see to possibly improve it in the v6.
>> These changes don't emit additional vertical whitespace after the
>> result
>> status produced for the last processed patch, i.e. the vertical
>> whitespace
>> is treated as a separator between the groups of produced messages, not
>> as
>> their terminator. This follows the Git's general approach of not
>> wasting
>> the vertical screen space whenever reasonably possible.
>
> I do not see this paragraph is relevant to the target audience. It
> may be a good advice to give to a reader who attempts to solve the
> problem this patch solved themselves, botches the attempt and ends
> up with a code with the terminator semantics. But for other readers
> of "git log" and reviewers of the patch, "I did not make a silly
> mistake, and instead correctly chose to use the separator semantics"
> is not something worth boasting about.
Makes sense, will delete that paragraph in the v6.
>> While there, remove a couple of spotted stray newlines in the source
>> code
>> and convert one indentation from spaces to tabs, for consistency.
>>
>> The associated test, t9001, requires no updates to cover these
>> changes.
>
> These are worth recording.
Thanks.
>> @@ -1554,7 +1554,10 @@ sub send_message {
>> exit(0);
>> } elsif (/^a/i) {
>> $confirm = 'never';
>> + $needs_separator = 1;
>> }
>> + } else {
>> + $needs_separator = 1;
>> }
>
> If you do not add this "else" clause to the outer "are we doing
> confirmation?" if statement, and instead just set $needs_separator
> *after* it, it would make it even more obvious what is going on.
> The codeflow would become
>
> sub send_message {
> do bunch of things that do not yet send e-mail
> and possibly return or die
>
> $needs_separator = 1;
>
> do things that cause the smtp exchange and trace
> to be emitted
> }
>
> That makes it obvious that the purpose of $needs_separator is to
> record the fact that "this" message has already been sent and we
> need to add a "gap" before attempting to send the "next" message.
Very good point, thanks! I've somehow managed to miss that while
iterating through a few code variants and the associated testing.
Will be improved in the v6.
> Other than the above points, very well done.
Thank you! :)
>> unshift (@sendmail_parameters, @smtp_server_options);
>> @@ -1576,7 +1579,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.")
>> }
>> @@ -1921,7 +1923,8 @@ sub pre_process_file {
>> sub process_file {
>> my ($t) = @_;
>>
>> - pre_process_file($t, $quiet);
>> + pre_process_file($t, $quiet);
>
> nice ;-)
It had to be fixed, IMHO. :)
>> + print "\n" if ($needs_separator);
>>
>> my $message_was_sent = send_message();
>> if ($message_was_sent == -1) {
next prev parent reply other threads:[~2024-04-09 3:37 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-07 10:48 [PATCH v5 0/3] send-email: make produced outputs more readable Dragan Simic
2024-04-07 10:48 ` [PATCH v5 1/3] send-email: move newline character out of a translatable string Dragan Simic
2024-04-07 10:48 ` [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch Dragan Simic
2024-04-08 21:08 ` Junio C Hamano
2024-04-09 3:37 ` Dragan Simic [this message]
2024-04-07 10:48 ` [PATCH v5 3/3] send-email: separate the confirmation prompts from the messages Dragan Simic
2024-04-08 21:21 ` Junio C Hamano
2024-04-09 3:25 ` Dragan Simic
2024-04-10 3:53 ` Dragan Simic
2024-04-10 6:07 ` Dragan Simic
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=2b24b9e8f0fcb4f6df595c1b0be06359@manjaro.org \
--to=dsimic@manjaro.org \
--cc=code@khaugsbakk.name \
--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).