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
* 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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.