| Message ID | 0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] send-email: make it easy to discern the messages for each patch | expand |
Dragan Simic <dsimic@manjaro.org> 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) {
On 2024-04-05 00:52, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> 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.
Hi On Thu, Apr 4, 2024, at 22:34, Dragan Simic wrote: > may be seen as redundant. Though, it doesn't look too bad, and making that > last newline not displayed would make the code much more complex, which would > not be worth neither the time and effort now, nor the additional maintenance > burden in the future. “Not worth neither” looks like a double negative. A double negative in mainstream English leads to cancellation, unlike in some regional dialects where it acts as an intensifier of sorts.
Hello Kristoffer, On 2024-04-05 08:52, Kristoffer Haugsbakk wrote: > On Thu, Apr 4, 2024, at 22:34, Dragan Simic wrote: >> may be seen as redundant. Though, it doesn't look too bad, and making >> that >> last newline not displayed would make the code much more complex, >> which would >> not be worth neither the time and effort now, nor the additional >> maintenance >> burden in the future. > > “Not worth neither” looks like a double negative. A double negative in > mainstream English leads to cancellation, unlike in some regional > dialects where it acts as an intensifier of sorts. Good point! :) However, that part of the patch description has already been deleted for the future v3 of this patch, because it treats the vertical whitespace as separators, instead of as terminators.
Dragan Simic <dsimic@manjaro.org> writes: > 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. The lesson you should be taking is a bit different here, though. As we discussed, doing the separator correctly is harder and doing the terminator correctly is much easier and less error prone. So if we chose to do terminator semantics and punt on doing separator semantics, the right thing could have been to adjust the existing tests to make sure that the new output with unnecessary trailing blank line is expected. Either that, or we should fix the code not to break the current expectation. Doing neither of these is not quite acceptable--- that is the lesson from this episode, I would think. Thanks.
Hello Junio, On 2024-04-05 18:44, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> 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. > > The lesson you should be taking is a bit different here, though. > > As we discussed, doing the separator correctly is harder and doing > the terminator correctly is much easier and less error prone. So if > we chose to do terminator semantics and punt on doing separator > semantics, the right thing could have been to adjust the existing > tests to make sure that the new output with unnecessary trailing > blank line is expected. > > Either that, or we should fix the code not to break the current > expectation. Doing neither of these is not quite acceptable--- > that is the lesson from this episode, I would think. Of course, it was my mistake not to either ensure that the tests still pass after the introduced changes, or to adjust the test(s) accordingly. However, making the vertical whitespace a separator also follows the Git's general approach of not wasting the vertical screen space whenever possible, so taking the separator route is actually better, if you agree.
Dragan Simic <dsimic@manjaro.org> writes: > However, making the vertical whitespace a separator also follows > the Git's general approach of not wasting the vertical screen > space whenever possible, so taking the separator route is actually > better, if you agree. Of course. If the topic came from a contributor with a longer track record and used terminator semantics, I would have insisted to consider separator semantics a lot more strongly. With only two commits in this history of any tagged releases, you are getting a bit more lenient treatment ;-)
On 2024-04-05 19:13, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> However, making the vertical whitespace a separator also follows >> the Git's general approach of not wasting the vertical screen >> space whenever possible, so taking the separator route is actually >> better, if you agree. > > Of course. > > If the topic came from a contributor with a longer track record and > used terminator semantics, I would have insisted to consider > separator semantics a lot more strongly. With only two commits in > this history of any tagged releases, you are getting a bit more > lenient treatment ;-) Ah, makes sense, thanks. :) However, please don't hesitate to be more strict, and I'll also make sure not to take any "shortcuts" in the future.
diff --git a/git-send-email.perl b/git-send-email.perl index 821b2b3a135a..ac0c691d3a46 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1576,7 +1576,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.") } @@ -1686,10 +1685,12 @@ sub send_message { print $header, "\n"; 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 + print "\n\n"; } return 1;
When sending multiple patches at once, without confirming the sending of each patch separately, the displayed result statuses of sending 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 above, bunching the "Result: <status-code>" lines together with the messages produced for the subsequent patch makes the output unreadable. Thus, let's add a newline after each displayed result status, to make the outputs more readable, as visible in the sample output excerpt below: ... MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Result: 250 OK. Log says: ... This change also adds a newline after the last produced result status, which may be seen as redundant. Though, it doesn't look too bad, and making that last newline not displayed would make the code much more complex, which would not be worth neither the time and effort now, nor the additional maintenance burden in the future. The way the additional newline separators are introduced to the source code also moves the already existing newline characters out of the translatable strings, which should help a bit with the translation efforts. While there, remove one spotted stray newline in the source code. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- Notes: * send-email: make produced outputs more readable by separating the result statuses from the subsequent patch outputs Changes in v2: - Improved the way additional newline separators are introduced to the source code, as suggested by Junio, [1], to help a bit with the translation efforts - Improved the patch subject and description a bit, to provide some additional information, as suggested by Junio [1] Notes for v1: - This is a resubmission of the patch I submitted about a week and a half ago. [2] The patch subject in the original submission was selected in a bit unfortunate way, which this submission corrects, and also improves the patch description a bit. - There are no changes to the patch itself, vs. the original patch. Link to v1: https://lore.kernel.org/git/62553db377c28458883b66bcdc0c58cc0f32d15b.1712250366.git.dsimic@manjaro.org/T/#u [1] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/ [2] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/ git-send-email.perl | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)