| Message ID | 7f87383089011a98b0347d885b3b9d76cfddb91d.1712486910.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | send-email: make produced outputs more readable | expand |
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) {
On 2024-04-08 23:08, Junio C Hamano wrote: > 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. 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) {
diff --git a/git-send-email.perl b/git-send-email.perl index a22f299ba051..4127fbe6b936 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -212,6 +212,7 @@ sub format_2822_time { my $compose_filename; my $force = 0; my $dump_aliases = 0; +my $needs_separator = 0; # Variables to prevent short format-patch options from being captured # as abbreviated send-email options @@ -1361,7 +1362,6 @@ sub smtp_host_string { # Returns 1 if authentication succeeded or was not necessary # (smtp_user was not specified), and 0 otherwise. - sub smtp_auth_maybe { if (!defined $smtp_authuser || $auth || (defined $smtp_auth && $smtp_auth eq "none")) { return 1; @@ -1554,7 +1554,10 @@ sub send_message { exit(0); } elsif (/^a/i) { $confirm = 'never'; + $needs_separator = 1; } + } else { + $needs_separator = 1; } 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); + print "\n" if ($needs_separator); my $message_was_sent = send_message(); if ($message_was_sent == -1) {
When sending multiple patches at once, without prompting the user to confirm the sending of each patch separately, the displayed result statuses for each patch become bunched together with the messages produced for the subsequent patch. This unnecessarily makes discerning each of the result statuses a bit difficult, as visible in the sample output excerpt below: ... MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Result: 250 OK. Log says: ... As visible in the excerpt above, bunching the "Result: <status-code>" lines together with the messages produced for the subsequent patch makes the output unreadable, which actually becomes worse as the number of patches sent at once increases. 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 sample output excerpt below, produced after the addition of vertical whitespace: ... MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Result: 250 OK. Log says: ... 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. 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. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- git-send-email.perl | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)