From: Junio C Hamano <gitster@pobox.com>
To: Dragan Simic <dsimic@manjaro.org>
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: Mon, 08 Apr 2024 14:08:02 -0700 [thread overview]
Message-ID: <xmqq7ch7a7gt.fsf@gitster.g> (raw)
In-Reply-To: <7f87383089011a98b0347d885b3b9d76cfddb91d.1712486910.git.dsimic@manjaro.org> (Dragan Simic's message of "Sun, 7 Apr 2024 12:48:51 +0200")
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.
> 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.
> 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.
> @@ -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.
Other than the above points, very well done.
> 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 ;-)
> + print "\n" if ($needs_separator);
>
> my $message_was_sent = send_message();
> if ($message_was_sent == -1) {
next prev parent reply other threads:[~2024-04-08 21:08 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 [this message]
2024-04-09 3:37 ` Dragan Simic
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=xmqq7ch7a7gt.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=code@khaugsbakk.name \
--cc=dsimic@manjaro.org \
--cc=git@vger.kernel.org \
/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).