On 2024-04-08 23:08, Junio C Hamano wrote:
Dragan Simic <dsimic@xxxxxxxxxxx> 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) {
