{"thread":{"id":"61269","subject":"[PATCH v2] send-email: make it easy to discern the messages for each patch","startedAt":"2024-04-04T20:34:34Z","lastAt":"2024-04-05T17:18:59Z","messageCount":9,"participants":["Dragan Simic","Junio C Hamano","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"492268","messageId":"0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org","threadId":"61269","inReplyTo":null,"subject":"[PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-04T20:34:28Z","receivedAt":"2024-04-04T20:34:34Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"When sending multiple patches at once, without confirming the sending of each\npatch separately, the displayed result statuses of sending each patch become\nbunched together with the messages produced for the subsequent patch.  This\nunnecessarily makes discerning each of the result statuses a bit difficult,\nas visible in the sample output excerpt below:\n\n    ...\n    MIME-Version: 1.0\n    Content-Transfer-Encoding: 8bit\n\n    Result: 250\n    OK. Log says:\n    ...\n\nAs visible above, bunching the \"Result: <status-code>\" lines together with\nthe messages produced for the subsequent patch makes the output unreadable.\nThus, let's add a newline after each displayed result status, to make the\noutputs more readable, as visible in the sample output excerpt below:\n\n    ...\n    MIME-Version: 1.0\n    Content-Transfer-Encoding: 8bit\n\n    Result: 250\n\n    OK. Log says:\n    ...\n\nThis change also adds a newline after the last produced result status, which\nmay be seen as redundant.  Though, it doesn't look too bad, and making that\nlast newline not displayed would make the code much more complex, which would\nnot be worth neither the time and effort now, nor the additional maintenance\nburden in the future.\n\nThe way the additional newline separators are introduced to the source code\nalso moves the already existing newline characters out of the translatable\nstrings, which should help a bit with the translation efforts.\n\nWhile there, remove one spotted stray newline in the source code.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n\nNotes:\n     * send-email: make produced outputs more readable by separating\n       the result statuses from the subsequent patch outputs\n    \n    Changes in v2:\n        - Improved the way additional newline separators are introduced to\n          the source code, as suggested by Junio, [1], to help a bit with\n          the translation efforts\n        - Improved the patch subject and description a bit, to provide some\n          additional information, as suggested by Junio [1]\n    \n    Notes for v1:\n        - This is a resubmission of the patch I submitted about a week and\n          a half ago. [2]  The patch subject in the original submission was\n          selected in a bit unfortunate way, which this submission corrects,\n          and also improves the patch description a bit.\n        - There are no changes to the patch itself, vs. the original patch.\n    \n    Link to v1: https://lore.kernel.org/git/62553db377c28458883b66bcdc0c58cc0f32d15b.1712250366.git.dsimic@manjaro.org/T/#u\n    \n    [1] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/\n    [2] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/\n\n git-send-email.perl | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 821b2b3a135a..ac0c691d3a46 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1576,7 +1576,6 @@ sub send_message {\n \t\tprint $sm \"$header\\n$message\";\n \t\tclose $sm or die $!;\n \t} else {\n-\n \t\tif (!defined $smtp_server) {\n \t\t\tdie __(\"The required SMTP server is not properly defined.\")\n \t\t}\n@@ -1686,10 +1685,12 @@ sub send_message {\n \t\tprint $header, \"\\n\";\n \t\tif ($smtp) {\n \t\t\tprint __(\"Result: \"), $smtp->code, ' ',\n-\t\t\t\t($smtp->message =~ /\\n([^\\n]+\\n)$/s), \"\\n\";\n+\t\t\t\t($smtp->message =~ /\\n([^\\n]+\\n)$/s);\n \t\t} else {\n-\t\t\tprint __(\"Result: OK\\n\");\n+\t\t\tprint __(\"Result: OK\");\n \t\t}\n+\t\t# Make it easy to discern the messages for each patch\n+\t\tprint \"\\n\\n\";\n \t}\n \n \treturn 1;\n"},{"id":"492274","messageId":"xmqqzfu8yc40.fsf@gitster.g","threadId":"61269","inReplyTo":"0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-04T22:52:31Z","receivedAt":"2024-04-04T22:52:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n>  \t\tif ($smtp) {\n>  \t\t\tprint __(\"Result: \"), $smtp->code, ' ',\n> -\t\t\t\t($smtp->message =~ /\\n([^\\n]+\\n)$/s), \"\\n\";\n> +\t\t\t\t($smtp->message =~ /\\n([^\\n]+\\n)$/s);\n>  \t\t} else {\n> -\t\t\tprint __(\"Result: OK\\n\");\n> +\t\t\tprint __(\"Result: OK\");\n>  \t\t}\n> +\t\t# Make it easy to discern the messages for each patch\n\nI do not think we want this comment.  \n\nBefore printing the \"Result: OK\" line, we do not give an obvious and\npointless comment e.g., \"# Report success\".  What this comment says\nsomething similarly obvious.  Both choices in the preceding if/else\nemit an incomplete line, hence it is clear that we need to terminate\nthe line here, and this is the last line of output about the message\nwe just processed.\n\n>  \t}\n>  \n>  \treturn 1;\n\nYou didn't ran t9001 before sending this version (or any of the\nprevious rounds) out, did you?  Among ~200 tests 10 of them now fail\nwith this patch applied.\n\nDo we know when we are sending either the first or the last message\nof a sequence at this point in the code?  It would be superb if you\ncan make this extra blank line a separator, not a terminator [*], as\nthere needs no extra blank line after emitting the last message.\n\n    [Side note]\n\n     * When showing a list of 3 things A B C, you can separate them by\n       inserting a gap between A and B, and another gap between B and C,\n       and you are using the \"separator\" (this is similar to how \"git\n       log --pretty=format:...\" works).  But you can be lazy and instead\n       append a gap after each element, in which case you are using the\n       \"terminator\" (similar to how \"git log --pretty=tformat:...\"\n       works).\n\nBut it is harder to do separator correctly if the output is\nconditional (e.g., you have 5 input messages, but you may somehow\nskip some messages depending on conditions---now your code to decide\nif you emit an extra newline needs to take it into account.  After\nsending the 4th message and showing an extra newline, because you\nexpect that there is another message to be sent and it needs a gap\nbefore, you may realize that some logic causes you to drop 5th and\nfinal message but then it is too late for you to take that extra\nblank line back), and obviously a buggy separator implementation is\nworse than a terminator implementation.\n\nHere is my attempt on top of your patch to implement the separator\nsemantics.  After showing a message, we remember that fact, and\nbefore showing the next message, we emit an extra blank line.  With\nit, all tests in t9001 pass, but you may want to double check how\ndifferent ways to leave send_message() would affect the output.  I\njust randomly decided that there needs no extra blank line before\nemitting the message that was edited and that is why in the\nfollowing patch, I assign 0 to $want_separator_before_send_message\nin that case, but it may not be the right choice (I never \"edit\"\ninside send-email myself, so I would be a wrong person to decide\nwithout second opinion), for example.\n\n\n git-send-email.perl | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git c/git-send-email.perl w/git-send-email.perl\nindex ac0c691d3a..d4bbc73f1f 100755\n--- c/git-send-email.perl\n+++ w/git-send-email.perl\n@@ -1689,8 +1689,7 @@ sub send_message {\n \t\t} else {\n \t\t\tprint __(\"Result: OK\");\n \t\t}\n-\t\t# Make it easy to discern the messages for each patch\n-\t\tprint \"\\n\\n\";\n+\t\tprint \"\\n\";\n \t}\n \n \treturn 1;\n@@ -1918,16 +1917,21 @@ sub pre_process_file {\n # Prepares the email, prompts the user, and sends it out\n # Returns 0 if an edit was done and the function should be called again, or 1\n # on the email being successfully sent out.\n+my $want_separator_before_send_message = 0;\n+\n sub process_file {\n \tmy ($t) = @_;\n \n         pre_process_file($t, $quiet);\n \n+\tprint \"\\n\" if ($want_separator_before_send_message);\n \tmy $message_was_sent = send_message();\n \tif ($message_was_sent == -1) {\n \t\tdo_edit($t);\n+\t\t$want_separator_before_send_message = 0;\n \t\treturn 0;\n \t}\n+\t$want_separator_before_send_message = $message_was_sent;\n \n \t# set up for the next message\n \tif ($thread) {\n"},{"id":"492277","messageId":"8d47bd687f2ad80bbc1e1c86ae337327@manjaro.org","threadId":"61269","inReplyTo":"xmqqzfu8yc40.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-04T23:44:42Z","receivedAt":"2024-04-04T23:44:44Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-05 00:52, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>>  \t\tif ($smtp) {\n>>  \t\t\tprint __(\"Result: \"), $smtp->code, ' ',\n>> -\t\t\t\t($smtp->message =~ /\\n([^\\n]+\\n)$/s), \"\\n\";\n>> +\t\t\t\t($smtp->message =~ /\\n([^\\n]+\\n)$/s);\n>>  \t\t} else {\n>> -\t\t\tprint __(\"Result: OK\\n\");\n>> +\t\t\tprint __(\"Result: OK\");\n>>  \t\t}\n>> +\t\t# Make it easy to discern the messages for each patch\n> \n> I do not think we want this comment.\n> \n> Before printing the \"Result: OK\" line, we do not give an obvious and\n> pointless comment e.g., \"# Report success\".  What this comment says\n> something similarly obvious.  Both choices in the preceding if/else\n> emit an incomplete line, hence it is clear that we need to terminate\n> the line here, and this is the last line of output about the message\n> we just processed.\n\nHmm, I thought that including this comment might actually be helpful\nto someone reading the code later, but with your explanation above,\nI agree that it doesn't fit very well.\n\n>>  \t}\n>> \n>>  \treturn 1;\n> \n> You didn't ran t9001 before sending this version (or any of the\n> previous rounds) out, did you?  Among ~200 tests 10 of them now fail\n> with this patch applied.\n\nMy bad, sorry.  I totally missed that test. :(\n\n> Do we know when we are sending either the first or the last message\n> of a sequence at this point in the code?  It would be superb if you\n> can make this extra blank line a separator, not a terminator [*], as\n> there needs no extra blank line after emitting the last message.\n> \n>     [Side note]\n> \n>      * When showing a list of 3 things A B C, you can separate them by\n>        inserting a gap between A and B, and another gap between B and \n> C,\n>        and you are using the \"separator\" (this is similar to how \"git\n>        log --pretty=format:...\" works).  But you can be lazy and \n> instead\n>        append a gap after each element, in which case you are using the\n>        \"terminator\" (similar to how \"git log --pretty=tformat:...\"\n>        works).\n\nWell, I actually wasn't lazy, but instead I wanted to give this patch\nbetter chances to be accepted, by introducing as a small amount of\nchanges to the code as possible.  This was a lesson not to take that\nroute, but to do \"the right thing\" instead.\n\n> But it is harder to do separator correctly if the output is\n> conditional (e.g., you have 5 input messages, but you may somehow\n> skip some messages depending on conditions---now your code to decide\n> if you emit an extra newline needs to take it into account.  After\n> sending the 4th message and showing an extra newline, because you\n> expect that there is another message to be sent and it needs a gap\n> before, you may realize that some logic causes you to drop 5th and\n> final message but then it is too late for you to take that extra\n> blank line back), and obviously a buggy separator implementation is\n> worse than a terminator implementation.\n\nYup, it's tricky to do it right.\n\n> Here is my attempt on top of your patch to implement the separator\n> semantics.  After showing a message, we remember that fact, and\n> before showing the next message, we emit an extra blank line.  With\n> it, all tests in t9001 pass, but you may want to double check how\n> different ways to leave send_message() would affect the output.  I\n> just randomly decided that there needs no extra blank line before\n> emitting the message that was edited and that is why in the\n> following patch, I assign 0 to $want_separator_before_send_message\n> in that case, but it may not be the right choice (I never \"edit\"\n> inside send-email myself, so I would be a wrong person to decide\n> without second opinion), for example.\n> \n> \n>  git-send-email.perl | 8 ++++++--\n>  1 file changed, 6 insertions(+), 2 deletions(-)\n> \n> diff --git c/git-send-email.perl w/git-send-email.perl\n> index ac0c691d3a..d4bbc73f1f 100755\n> --- c/git-send-email.perl\n> +++ w/git-send-email.perl\n> @@ -1689,8 +1689,7 @@ sub send_message {\n>  \t\t} else {\n>  \t\t\tprint __(\"Result: OK\");\n>  \t\t}\n> -\t\t# Make it easy to discern the messages for each patch\n> -\t\tprint \"\\n\\n\";\n> +\t\tprint \"\\n\";\n>  \t}\n> \n>  \treturn 1;\n> @@ -1918,16 +1917,21 @@ sub pre_process_file {\n>  # Prepares the email, prompts the user, and sends it out\n>  # Returns 0 if an edit was done and the function should be called \n> again, or 1\n>  # on the email being successfully sent out.\n> +my $want_separator_before_send_message = 0;\n> +\n>  sub process_file {\n>  \tmy ($t) = @_;\n> \n>          pre_process_file($t, $quiet);\n> \n> +\tprint \"\\n\" if ($want_separator_before_send_message);\n>  \tmy $message_was_sent = send_message();\n>  \tif ($message_was_sent == -1) {\n>  \t\tdo_edit($t);\n> +\t\t$want_separator_before_send_message = 0;\n>  \t\treturn 0;\n>  \t}\n> +\t$want_separator_before_send_message = $message_was_sent;\n> \n>  \t# set up for the next message\n>  \tif ($thread) {\n\nThanks for this patch, I'll go through it, and make sure that it works\nas expected.  I'll see also to correct the current lack of vertical\nseparation between the \"send the message [y/n/q/a/...]\" prompt and the\nsubsequent output, which also unnecessarily makes the output a bit hard\nto parse by humans.\n\nThere's also seemingly something wrong with the typeface highlighting\nselected for the above-mentioned prompt.  In my environment, it ends up\nbeing displayed as underlined bold text, which looks a bit ugly and,\nmore importantly, a bit hard on the eyes.\n"},{"id":"492280","messageId":"70c97d51-b26a-43f3-9856-af405f396576@app.fastmail.com","threadId":"61269","inReplyTo":"0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-04-05T06:52:02Z","receivedAt":"2024-04-05T06:52:23Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi\n\nOn Thu, Apr 4, 2024, at 22:34, Dragan Simic wrote:\n> may be seen as redundant.  Though, it doesn't look too bad, and making that\n> last newline not displayed would make the code much more complex, which would\n> not be worth neither the time and effort now, nor the additional maintenance\n> burden in the future.\n\n“Not worth neither” looks like a double negative. A double negative in\nmainstream English leads to cancellation, unlike in some regional\ndialects where it acts as an intensifier of sorts.\n"},{"id":"492282","messageId":"37827a9d31e494074310024b86b242c0@manjaro.org","threadId":"61269","inReplyTo":"70c97d51-b26a-43f3-9856-af405f396576@app.fastmail.com","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-05T07:07:32Z","receivedAt":"2024-04-05T07:07:35Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Kristoffer,\n\nOn 2024-04-05 08:52, Kristoffer Haugsbakk wrote:\n> On Thu, Apr 4, 2024, at 22:34, Dragan Simic wrote:\n>> may be seen as redundant.  Though, it doesn't look too bad, and making \n>> that\n>> last newline not displayed would make the code much more complex, \n>> which would\n>> not be worth neither the time and effort now, nor the additional \n>> maintenance\n>> burden in the future.\n> \n> “Not worth neither” looks like a double negative. A double negative in\n> mainstream English leads to cancellation, unlike in some regional\n> dialects where it acts as an intensifier of sorts.\n\nGood point! :)  However, that part of the patch description has already\nbeen deleted for the future v3 of this patch, because it treats the \nvertical\nwhitespace as separators, instead of as terminators.\n"},{"id":"492297","messageId":"xmqqjzlbwyhr.fsf@gitster.g","threadId":"61269","inReplyTo":"8d47bd687f2ad80bbc1e1c86ae337327@manjaro.org","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-05T16:44:16Z","receivedAt":"2024-04-05T16:44:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> Well, I actually wasn't lazy, but instead I wanted to give this patch\n> better chances to be accepted, by introducing as a small amount of\n> changes to the code as possible.  This was a lesson not to take that\n> route, but to do \"the right thing\" instead.\n\nThe lesson you should be taking is a bit different here, though.\n\nAs we discussed, doing the separator correctly is harder and doing\nthe terminator correctly is much easier and less error prone.  So if\nwe chose to do terminator semantics and punt on doing separator\nsemantics, the right thing could have been to adjust the existing\ntests to make sure that the new output with unnecessary trailing\nblank line is expected.\n\nEither that, or we should fix the code not to break the current\nexpectation.  Doing neither of these is not quite acceptable---\nthat is the lesson from this episode, I would think.\n\nThanks.\n"},{"id":"492300","messageId":"fa91b134e6cb18578d9c499f584ae3c5@manjaro.org","threadId":"61269","inReplyTo":"xmqqjzlbwyhr.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-05T16:51:55Z","receivedAt":"2024-04-05T16:51:57Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Junio,\n\nOn 2024-04-05 18:44, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> Well, I actually wasn't lazy, but instead I wanted to give this patch\n>> better chances to be accepted, by introducing as a small amount of\n>> changes to the code as possible.  This was a lesson not to take that\n>> route, but to do \"the right thing\" instead.\n> \n> The lesson you should be taking is a bit different here, though.\n> \n> As we discussed, doing the separator correctly is harder and doing\n> the terminator correctly is much easier and less error prone.  So if\n> we chose to do terminator semantics and punt on doing separator\n> semantics, the right thing could have been to adjust the existing\n> tests to make sure that the new output with unnecessary trailing\n> blank line is expected.\n> \n> Either that, or we should fix the code not to break the current\n> expectation.  Doing neither of these is not quite acceptable---\n> that is the lesson from this episode, I would think.\n\nOf course, it was my mistake not to either ensure that the tests\nstill pass after the introduced changes, or to adjust the test(s)\naccordingly.\n\nHowever, making the vertical whitespace a separator also follows\nthe Git's general approach of not wasting the vertical screen\nspace whenever possible, so taking the separator route is actually\nbetter, if you agree.\n"},{"id":"492304","messageId":"xmqqplv3vijx.fsf@gitster.g","threadId":"61269","inReplyTo":"fa91b134e6cb18578d9c499f584ae3c5@manjaro.org","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-05T17:13:54Z","receivedAt":"2024-04-05T17:13:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> However, making the vertical whitespace a separator also follows\n> the Git's general approach of not wasting the vertical screen\n> space whenever possible, so taking the separator route is actually\n> better, if you agree.\n\nOf course.\n\nIf the topic came from a contributor with a longer track record and\nused terminator semantics, I would have insisted to consider\nseparator semantics a lot more strongly.  With only two commits in\nthis history of any tagged releases, you are getting a bit more\nlenient treatment ;-)\n\n"},{"id":"492305","messageId":"fd70ae57ab668e6789210ad6d1ff0e80@manjaro.org","threadId":"61269","inReplyTo":"xmqqplv3vijx.fsf@gitster.g","subject":"Re: [PATCH v2] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-05T17:18:51Z","receivedAt":"2024-04-05T17:18:59Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-05 19:13, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> However, making the vertical whitespace a separator also follows\n>> the Git's general approach of not wasting the vertical screen\n>> space whenever possible, so taking the separator route is actually\n>> better, if you agree.\n> \n> Of course.\n> \n> If the topic came from a contributor with a longer track record and\n> used terminator semantics, I would have insisted to consider\n> separator semantics a lot more strongly.  With only two commits in\n> this history of any tagged releases, you are getting a bit more\n> lenient treatment ;-)\n\nAh, makes sense, thanks. :)  However, please don't hesitate to be\nmore strict, and I'll also make sure not to take any \"shortcuts\"\nin the future.\n"}]}