Dragan Simic <dsimic@xxxxxxxxxxx> writes:
> if ($smtp) {
> print __("Result: "), $smtp->code, ' ',
> - ($smtp->message =~ /\n([^\n]+\n)$/s), "\n";
> + ($smtp->message =~ /\n([^\n]+\n)$/s);
> } else {
> - print __("Result: OK\n");
> + print __("Result: OK");
> }
> + # Make it easy to discern the messages for each patch
I do not think we want this comment.
Before printing the "Result: OK" line, we do not give an obvious and
pointless comment e.g., "# Report success". What this comment says
something similarly obvious. Both choices in the preceding if/else
emit an incomplete line, hence it is clear that we need to terminate
the line here, and this is the last line of output about the message
we just processed.
> }
>
> return 1;
You didn't ran t9001 before sending this version (or any of the
previous rounds) out, did you? Among ~200 tests 10 of them now fail
with this patch applied.
Do we know when we are sending either the first or the last message
of a sequence at this point in the code? It would be superb if you
can make this extra blank line a separator, not a terminator [*], as
there needs no extra blank line after emitting the last message.
[Side note]
* When showing a list of 3 things A B C, you can separate them by
inserting a gap between A and B, and another gap between B and C,
and you are using the "separator" (this is similar to how "git
log --pretty=format:..." works). But you can be lazy and instead
append a gap after each element, in which case you are using the
"terminator" (similar to how "git log --pretty=tformat:..."
works).
But it is harder to do separator correctly if the output is
conditional (e.g., you have 5 input messages, but you may somehow
skip some messages depending on conditions---now your code to decide
if you emit an extra newline needs to take it into account. After
sending the 4th message and showing an extra newline, because you
expect that there is another message to be sent and it needs a gap
before, you may realize that some logic causes you to drop 5th and
final message but then it is too late for you to take that extra
blank line back), and obviously a buggy separator implementation is
worse than a terminator implementation.
Here is my attempt on top of your patch to implement the separator
semantics. After showing a message, we remember that fact, and
before showing the next message, we emit an extra blank line. With
it, all tests in t9001 pass, but you may want to double check how
different ways to leave send_message() would affect the output. I
just randomly decided that there needs no extra blank line before
emitting the message that was edited and that is why in the
following patch, I assign 0 to $want_separator_before_send_message
in that case, but it may not be the right choice (I never "edit"
inside send-email myself, so I would be a wrong person to decide
without second opinion), for example.
git-send-email.perl | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git c/git-send-email.perl w/git-send-email.perl
index ac0c691d3a..d4bbc73f1f 100755
--- c/git-send-email.perl
+++ w/git-send-email.perl
@@ -1689,8 +1689,7 @@ sub send_message {
} else {
print __("Result: OK");
}
- # Make it easy to discern the messages for each patch
- print "\n\n";
+ print "\n";
}
return 1;
@@ -1918,16 +1917,21 @@ sub pre_process_file {
# Prepares the email, prompts the user, and sends it out
# Returns 0 if an edit was done and the function should be called again, or 1
# on the email being successfully sent out.
+my $want_separator_before_send_message = 0;
+
sub process_file {
my ($t) = @_;
pre_process_file($t, $quiet);
+ print "\n" if ($want_separator_before_send_message);
my $message_was_sent = send_message();
if ($message_was_sent == -1) {
do_edit($t);
+ $want_separator_before_send_message = 0;
return 0;
}
+ $want_separator_before_send_message = $message_was_sent;
# set up for the next message
if ($thread) {
