On 2024-04-05 00:52, Junio C Hamano wrote:
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.
Hmm, I thought that including this comment might actually be helpful
to someone reading the code later, but with your explanation above,
I agree that it doesn't fit very well.
}
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.
My bad, sorry. I totally missed that test. :(
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).
Well, I actually wasn't lazy, but instead I wanted to give this patch
better chances to be accepted, by introducing as a small amount of
changes to the code as possible. This was a lesson not to take that
route, but to do "the right thing" instead.
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.
Yup, it's tricky to do it right.
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) {
Thanks for this patch, I'll go through it, and make sure that it works
as expected. I'll see also to correct the current lack of vertical
separation between the "send the message [y/n/q/a/...]" prompt and the
subsequent output, which also unnecessarily makes the output a bit hard
to parse by humans.
There's also seemingly something wrong with the typeface highlighting
selected for the above-mentioned prompt. In my environment, it ends up
being displayed as underlined bold text, which looks a bit ugly and,
more importantly, a bit hard on the eyes.
