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.
> 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) {
