{"thread":{"id":"61288","subject":"[PATCH v5 0/3] send-email: make produced outputs more readable","startedAt":"2024-04-07T10:49:00Z","lastAt":"2024-04-10T06:07:44Z","messageCount":10,"participants":["Dragan Simic","Junio C Hamano"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"492456","messageId":"cover.1712486910.git.dsimic@manjaro.org","threadId":"61288","inReplyTo":null,"subject":"[PATCH v5 0/3] send-email: make produced outputs more readable","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T10:48:49Z","receivedAt":"2024-04-07T10:49:00Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":" * send-email: make produced outputs more readable by separating\n   the result statuses and prompts from the subsequent messages\n\nThis series makes the outputs of \"git send-mail\" more readable, by\nadding vertical whitespace to make discerning the messages produced\nfor each patch easy, and by adding vertical whitespace to separate the\nemitted prompts from the other produced messages.  Having the prompts\nand some of the produced messages bunched together made the navigation\nthrough the produced outputs unnecessarily hard.\n\nMaking the outputs of \"git send-mail\" more readable is quite important,\nbecause the Git users rather regularly complain about the workflows that\nrequire project patches to be sent to various mailing lists.  Making the\nproduced outputs more readable can only help there.\n\nChanges in v5:\n    - Split into a three-patch series, because changes introduced in\n      some versions of this patch made the original assumptions about\n      squashing the changes together no longer apply [1]\n    - Updated and extended the patch descriptions, to hopefully describe\n      the changes performed in each patch a bit better\n\nChanges in v4:\n    - Dropped the changes to the styling of the produced prompts, as\n      reasonably requested by Junio, [2] because it extended the scope\n      of the patch with no good reason, and may also create issues on\n      some platforms, whose Perl packages may actually not support the\n      \"->ornaments()\" feature of Term::ReadLine\n    - Updated the patch description and the \"what's cooking\" summary\n      to cover the changes\n\nChanges in v3:\n    - Removed a redundant comment, as suggested by Junio [3]\n    - Did the right thing and made the vertical separators emitted as\n      message separators, instead of having them emitted as message\n      terminators, as suggested by Junio [3]\n    - Additional vertical whitespace is now also emitted after the\n      prompt for sending emails, to \"de-bunch\" it from the subsequent\n      messages and make discerning the messages easier\n    - The above-mentioned prompt no longer uses underlined text, to make\n      it significantly easier on the eyes\n    - Fixed one indentation by spaces to use tabs and removed one stray\n      newline in the source code, as spotted\n    - Updated and extended the patch description to cover the changes\n    - Updated the \"what's cooking\" summary to cover the changes\n    - Cleaned up the older notes a bit\n\nChanges in v2:\n    - Improved the way additional newline separators are introduced to\n      the source code, as suggested by Junio, [4], 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 [4]\n    - Added a Helped-by tag\n\nNotes for v1:\n    - This is a resubmission of the patch I submitted about a week and\n      a half ago; [5]  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\nLink to v1: https://lore.kernel.org/git/62553db377c28458883b66bcdc0c58cc0f32d15b.1712250366.git.dsimic@manjaro.org/T/#u\nLink to v2: https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/T/#u\nLink to v3: https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u\nLink to v4: https://lore.kernel.org/git/8a9f4927aab96f2f62e2467e59fb6150d7e931fc.1712367983.git.dsimic@manjaro.org/T/#u\n\n[1] https://lore.kernel.org/git/713c28cfc9dff2d01159105ddd2fd0f5@manjaro.org/\n[2] https://lore.kernel.org/git/xmqq8r1rs39g.fsf@gitster.g/\n[3] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/\n[4] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/\n[5] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/\n\nDragan Simic (3):\n  send-email: move newline character out of a translatable string\n  send-email: make it easy to discern the messages for each patch\n  send-email: separate the confirmation prompts from the messages\n\n git-send-email.perl | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\n"},{"id":"492457","messageId":"d2d456edcb5bdf7ec254542ed94fed56337f1856.1712486910.git.dsimic@manjaro.org","threadId":"61288","inReplyTo":"cover.1712486910.git.dsimic@manjaro.org","subject":"[PATCH v5 1/3] send-email: move newline character out of a translatable string","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T10:48:50Z","receivedAt":"2024-04-07T10:49:01Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Move the already existing newline character out of a translatable string,\nto help a bit with the translation efforts.\n\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n git-send-email.perl | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 821b2b3a135a..a22f299ba051 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1686,10 +1686,11 @@ 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\tprint \"\\n\";\n \t}\n \n \treturn 1;\n"},{"id":"492458","messageId":"7f87383089011a98b0347d885b3b9d76cfddb91d.1712486910.git.dsimic@manjaro.org","threadId":"61288","inReplyTo":"cover.1712486910.git.dsimic@manjaro.org","subject":"[PATCH v5 2/3] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T10:48:51Z","receivedAt":"2024-04-07T10:49:01Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"When sending multiple patches at once, without prompting the user to confirm\nthe sending of each patch separately, the displayed result statuses for each\npatch become bunched together with the messages produced for the subsequent\npatch.  This unnecessarily makes discerning each of the result statuses a bit\ndifficult, 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    OK. Log says:\n    ...\n\nAs visible in the excerpt above, bunching the \"Result: <status-code>\" lines\ntogether with the messages produced for the subsequent patch makes the output\nunreadable, which actually becomes worse as the number of patches sent at\nonce increases.  To make the produced outputs more readable, add vertical\nwhitespace (more precisely, a newline) between the displayed result statuses\nand the subsequent messages, as visible in the sample output excerpt below,\nproduced after the addition of vertical whitespace:\n\n    ...\n    MIME-Version: 1.0\n    Content-Transfer-Encoding: 8bit\n\n    Result: 250\n\n    OK. Log says:\n    ...\n\nThese changes don't emit additional vertical whitespace after the result\nstatus produced for the last processed patch, i.e. the vertical whitespace\nis treated as a separator between the groups of produced messages, not as\ntheir terminator.  This follows the Git's general approach of not wasting\nthe vertical screen space whenever reasonably possible.\n\nWhile there, remove a couple of spotted stray newlines in the source code\nand convert one indentation from spaces to tabs, for consistency.\n\nThe associated test, t9001, requires no updates to cover these changes.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n git-send-email.perl | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex a22f299ba051..4127fbe6b936 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -212,6 +212,7 @@ sub format_2822_time {\n my $compose_filename;\n my $force = 0;\n my $dump_aliases = 0;\n+my $needs_separator = 0;\n \n # Variables to prevent short format-patch options from being captured\n # as abbreviated send-email options\n@@ -1361,7 +1362,6 @@ sub smtp_host_string {\n \n # Returns 1 if authentication succeeded or was not necessary\n # (smtp_user was not specified), and 0 otherwise.\n-\n sub smtp_auth_maybe {\n \tif (!defined $smtp_authuser || $auth || (defined $smtp_auth && $smtp_auth eq \"none\")) {\n \t\treturn 1;\n@@ -1554,7 +1554,10 @@ sub send_message {\n \t\t\texit(0);\n \t\t} elsif (/^a/i) {\n \t\t\t$confirm = 'never';\n+\t\t\t$needs_separator = 1;\n \t\t}\n+\t} else {\n+\t\t$needs_separator = 1;\n \t}\n \n \tunshift (@sendmail_parameters, @smtp_server_options);\n@@ -1576,7 +1579,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@@ -1921,7 +1923,8 @@ sub pre_process_file {\n sub process_file {\n \tmy ($t) = @_;\n \n-        pre_process_file($t, $quiet);\n+\tpre_process_file($t, $quiet);\n+\tprint \"\\n\" if ($needs_separator);\n \n \tmy $message_was_sent = send_message();\n \tif ($message_was_sent == -1) {\n"},{"id":"492459","messageId":"7b99e5c7c0b0c8d3d7eaaae169e650ebe81964a1.1712486910.git.dsimic@manjaro.org","threadId":"61288","inReplyTo":"cover.1712486910.git.dsimic@manjaro.org","subject":"[PATCH v5 3/3] send-email: separate the confirmation prompts from the messages","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T10:48:52Z","receivedAt":"2024-04-07T10:49:01Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Emit additional vertical whitespace after the \"Send this email [y/n/...]?\"\nconfirmation prompts, more specifically after each confirmed email is sent,\nbut before the subsequent messages are emitted, to make the produced output\nmore readable.  The subsequent produced messages were bunched together with\nthe confirmation prompts, as visible in the sample output excerpt below,\nwhich made discerning the outputs unnecessarily harder.\n\n    ...\n    Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y\n    OK. Log says:\n    ...\n\nThe introduced changes don't emit additional vertical whitespace after the\nconfirmation prompt if the user selects to skip sending the email they were\nasked about, or if the user selects to quit the procedure entirely.  This\nfollows the Git's general approach of not wasting the vertical screen space\nwhenever reasonably possible.\n\nThe associated test, t9001, requires no updates to cover these changes.\n\nSigned-off-by: Dragan Simic <dsimic@manjaro.org>\n---\n git-send-email.perl | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 4127fbe6b936..a09bc7fd6b96 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1510,6 +1510,7 @@ sub gen_header {\n sub send_message {\n \tmy ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();\n \tmy @recipients = @$recipients_ref;\n+\tmy $prompt_separator = 0;\n \n \tmy @sendmail_parameters = ('-i', @recipients);\n \tmy $raw_from = $sender;\n@@ -1556,6 +1557,7 @@ sub send_message {\n \t\t\t$confirm = 'never';\n \t\t\t$needs_separator = 1;\n \t\t}\n+\t\t$prompt_separator = 1;\n \t} else {\n \t\t$needs_separator = 1;\n \t}\n@@ -1665,6 +1667,7 @@ sub send_message {\n \t\t$smtp->dataend() or die $smtp->message;\n \t\t$smtp->code =~ /250|200/ or die sprintf(__(\"Failed to send %s\\n\"), $subject).$smtp->message;\n \t}\n+\tprint \"\\n\" if ($prompt_separator);\n \tif ($quiet) {\n \t\tprintf($dry_run ? __(\"Dry-Sent %s\\n\") : __(\"Sent %s\\n\"), $subject);\n \t} else {\n"},{"id":"492584","messageId":"xmqq7ch7a7gt.fsf@gitster.g","threadId":"61288","inReplyTo":"7f87383089011a98b0347d885b3b9d76cfddb91d.1712486910.git.dsimic@manjaro.org","subject":"Re: [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T21:08:02Z","receivedAt":"2024-04-08T21:08:07Z","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> ...  To make the produced outputs more readable, add vertical\n> whitespace (more precisely, a newline) between the displayed result statuses\n> and the subsequent messages, as visible in ...\n\nThe above feels a bit roundabout way to say \"the logic is that we\nneed to add a gap before showing the next message, if we did things\nthat cause the smtp traces to be shown\", but OK.\n\n> These changes don't emit additional vertical whitespace after the result\n> status produced for the last processed patch, i.e. the vertical whitespace\n> is treated as a separator between the groups of produced messages, not as\n> their terminator.  This follows the Git's general approach of not wasting\n> the vertical screen space whenever reasonably possible.\n\nI do not see this paragraph is relevant to the target audience.  It\nmay be a good advice to give to a reader who attempts to solve the\nproblem this patch solved themselves, botches the attempt and ends\nup with a code with the terminator semantics.  But for other readers\nof \"git log\" and reviewers of the patch, \"I did not make a silly\nmistake, and instead correctly chose to use the separator semantics\"\nis not something worth boasting about.\n\n> While there, remove a couple of spotted stray newlines in the source code\n> and convert one indentation from spaces to tabs, for consistency.\n>\n> The associated test, t9001, requires no updates to cover these changes.\n\nThese are worth recording.\n\n> @@ -1554,7 +1554,10 @@ sub send_message {\n>  \t\t\texit(0);\n>  \t\t} elsif (/^a/i) {\n>  \t\t\t$confirm = 'never';\n> +\t\t\t$needs_separator = 1;\n>  \t\t}\n> +\t} else {\n> +\t\t$needs_separator = 1;\n>  \t}\n\nIf you do not add this \"else\" clause to the outer \"are we doing\nconfirmation?\" if statement, and instead just set $needs_separator\n*after* it, it would make it even more obvious what is going on.\nThe codeflow would become\n\n\tsub send_message {\n\t\tdo bunch of things that do not yet send e-mail\n\t        and possibly return or die\n\n\t\t$needs_separator = 1;\n\n\t\tdo things that cause the smtp exchange and trace\n\t\tto be emitted\n\t}\n\nThat makes it obvious that the purpose of $needs_separator is to\nrecord the fact that \"this\" message has already been sent and we\nneed to add a \"gap\" before attempting to send the \"next\" message.\n\nOther than the above points, very well done.\n\n>  \tunshift (@sendmail_parameters, @smtp_server_options);\n> @@ -1576,7 +1579,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> @@ -1921,7 +1923,8 @@ sub pre_process_file {\n>  sub process_file {\n>  \tmy ($t) = @_;\n>  \n> -        pre_process_file($t, $quiet);\n> +\tpre_process_file($t, $quiet);\n\nnice ;-)\n\n> +\tprint \"\\n\" if ($needs_separator);\n>  \n>  \tmy $message_was_sent = send_message();\n>  \tif ($message_was_sent == -1) {\n"},{"id":"492585","messageId":"xmqq1q7fa6u7.fsf@gitster.g","threadId":"61288","inReplyTo":"7b99e5c7c0b0c8d3d7eaaae169e650ebe81964a1.1712486910.git.dsimic@manjaro.org","subject":"Re: [PATCH v5 3/3] send-email: separate the confirmation prompts from the messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T21:21:36Z","receivedAt":"2024-04-08T21:21:39Z","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> Emit additional vertical whitespace after the \"Send this email [y/n/...]?\"\n> confirmation prompts, more specifically after each confirmed email is sent,\n> but before the subsequent messages are emitted, to make the produced output\n> more readable.  The subsequent produced messages were bunched together with\n> the confirmation prompts, as visible in the sample output excerpt below,\n> which made discerning the outputs unnecessarily harder.\n>\n>     ...\n>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y\n>     OK. Log says:\n>     ...\n\nWhat comes before \"send this email\" prompt needs to be shown to make\nthe argument more convincing, but with \"...\" there is no cue to decide\nif the output is hard to read.\n\n>  sub send_message {\n>  \tmy ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();\n>  \tmy @recipients = @$recipients_ref;\n> +\tmy $prompt_separator = 0;\n>  \n>  \tmy @sendmail_parameters = ('-i', @recipients);\n>  \tmy $raw_from = $sender;\n> @@ -1556,6 +1557,7 @@ sub send_message {\n>  \t\t\t$confirm = 'never';\n>  \t\t\t$needs_separator = 1;\n>  \t\t}\n> +\t\t$prompt_separator = 1;\n>  \t} else {\n>  \t\t$needs_separator = 1;\n>  \t}\n> @@ -1665,6 +1667,7 @@ sub send_message {\n>  \t\t$smtp->dataend() or die $smtp->message;\n>  \t\t$smtp->code =~ /250|200/ or die sprintf(__(\"Failed to send %s\\n\"), $subject).$smtp->message;\n>  \t}\n> +\tprint \"\\n\" if ($prompt_separator);\n\n\"prompt separator\" sounds more like a separator that separates\nprompts, but that is not what is going on, no?\n\nDo we even need that new varible in the first place?  I am wondering\nif you can just do the print \"\\n\" right where you assign to that\nvariable.\n\nWhen $confirm is set to 'never', you have both $needs_separtor and\n$prompt_separator set.  Would it mean you'd have two extra blank\nlines for that message?\n\nAll these questions you should have been able to avoided with a bit\nmore helpful explanation in the proposed log message, I hope.\n\nThanks.\n\n>  \tif ($quiet) {\n>  \t\tprintf($dry_run ? __(\"Dry-Sent %s\\n\") : __(\"Sent %s\\n\"), $subject);\n>  \t} else {\n"},{"id":"492614","messageId":"bc3998b1c9bbdbc75287735b42013f52@manjaro.org","threadId":"61288","inReplyTo":"xmqq1q7fa6u7.fsf@gitster.g","subject":"Re: [PATCH v5 3/3] send-email: separate the confirmation prompts from the messages","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-09T03:25:32Z","receivedAt":"2024-04-09T03:25:34Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Junio,\n\nOn 2024-04-08 23:21, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> Emit additional vertical whitespace after the \"Send this email \n>> [y/n/...]?\"\n>> confirmation prompts, more specifically after each confirmed email is \n>> sent,\n>> but before the subsequent messages are emitted, to make the produced \n>> output\n>> more readable.  The subsequent produced messages were bunched together \n>> with\n>> the confirmation prompts, as visible in the sample output excerpt \n>> below,\n>> which made discerning the outputs unnecessarily harder.\n>> \n>>     ...\n>>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y\n>>     OK. Log says:\n>>     ...\n> \n> What comes before \"send this email\" prompt needs to be shown to make\n> the argument more convincing, but with \"...\" there is no cue to decide\n> if the output is hard to read.\n\nBelieve it or not, I also saw the provided sample excerpt as\ntoo short.  However, when I tried to make it longer and more\nobvious, it turned out that simply too many lines needed to be\nincluded.  I'll give it some more though, and possibly delete\nthe sample entirely.\n\n>>  sub send_message {\n>>  \tmy ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) \n>> = gen_header();\n>>  \tmy @recipients = @$recipients_ref;\n>> +\tmy $prompt_separator = 0;\n>> \n>>  \tmy @sendmail_parameters = ('-i', @recipients);\n>>  \tmy $raw_from = $sender;\n>> @@ -1556,6 +1557,7 @@ sub send_message {\n>>  \t\t\t$confirm = 'never';\n>>  \t\t\t$needs_separator = 1;\n>>  \t\t}\n>> +\t\t$prompt_separator = 1;\n>>  \t} else {\n>>  \t\t$needs_separator = 1;\n>>  \t}\n>> @@ -1665,6 +1667,7 @@ sub send_message {\n>>  \t\t$smtp->dataend() or die $smtp->message;\n>>  \t\t$smtp->code =~ /250|200/ or die sprintf(__(\"Failed to send %s\\n\"), \n>> $subject).$smtp->message;\n>>  \t}\n>> +\tprint \"\\n\" if ($prompt_separator);\n> \n> \"prompt separator\" sounds more like a separator that separates\n> prompts, but that is not what is going on, no?\n\nYup, I already wanted to rename that variable, because its name\nisn't really great, but I didn't do that because it went as such\nin earlier versions of the patch.  Will rename it in the v6.\n\n> Do we even need that new varible in the first place?  I am wondering\n> if you can just do the print \"\\n\" right where you assign to that\n> variable.\n\nThe reason why the newline isn't emitted right away is because\nsending the message takes some time, and if we print it right\naway, there's an additional empty line staring at the user while\nthey wait for the message to be sent.  If we emit the newline\na bit later, using that variable, it gets displayed together\nwith the printed message, making the displaying of the output\nmuch smoother.\n\nI already tried to describe that behavior in the patch description.\nI'll try to improve that description in the v6.\n\n> When $confirm is set to 'never', you have both $needs_separtor and\n> $prompt_separator set.  Would it mean you'd have two extra blank\n> lines for that message?\n\nActually, there are no unneeded blank lines in that case, which\nI've also tested before sending the patches.  One of the blank\nlines (the one for $needs_separator) goes after the patch messages,\nand the other one (the one for $prompt_separator) goes after the\nprompt, which is displayed before the patch messages.\n\n> All these questions you should have been able to avoided with a bit\n> more helpful explanation in the proposed log message, I hope.\n\nI already tried to do that, but it seems it needed more detailed\nexplanations in the patch description.  Will be improved in the v6.\n\n>>  \tif ($quiet) {\n>>  \t\tprintf($dry_run ? __(\"Dry-Sent %s\\n\") : __(\"Sent %s\\n\"), $subject);\n>>  \t} else {\n"},{"id":"492615","messageId":"2b24b9e8f0fcb4f6df595c1b0be06359@manjaro.org","threadId":"61288","inReplyTo":"xmqq7ch7a7gt.fsf@gitster.g","subject":"Re: [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-09T03:37:43Z","receivedAt":"2024-04-09T03:37:45Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-08 23:08, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> ...  To make the produced outputs more readable, add vertical\n>> whitespace (more precisely, a newline) between the displayed result \n>> statuses\n>> and the subsequent messages, as visible in ...\n> \n> The above feels a bit roundabout way to say \"the logic is that we\n> need to add a gap before showing the next message, if we did things\n> that cause the smtp traces to be shown\", but OK.\n\nYeah, the wording I used isn't perfect, but I think it's still\nunderstandable.  I'll see to possibly improve it in the v6.\n\n>> These changes don't emit additional vertical whitespace after the \n>> result\n>> status produced for the last processed patch, i.e. the vertical \n>> whitespace\n>> is treated as a separator between the groups of produced messages, not \n>> as\n>> their terminator.  This follows the Git's general approach of not \n>> wasting\n>> the vertical screen space whenever reasonably possible.\n> \n> I do not see this paragraph is relevant to the target audience.  It\n> may be a good advice to give to a reader who attempts to solve the\n> problem this patch solved themselves, botches the attempt and ends\n> up with a code with the terminator semantics.  But for other readers\n> of \"git log\" and reviewers of the patch, \"I did not make a silly\n> mistake, and instead correctly chose to use the separator semantics\"\n> is not something worth boasting about.\n\nMakes sense, will delete that paragraph in the v6.\n\n>> While there, remove a couple of spotted stray newlines in the source \n>> code\n>> and convert one indentation from spaces to tabs, for consistency.\n>> \n>> The associated test, t9001, requires no updates to cover these \n>> changes.\n> \n> These are worth recording.\n\nThanks.\n\n>> @@ -1554,7 +1554,10 @@ sub send_message {\n>>  \t\t\texit(0);\n>>  \t\t} elsif (/^a/i) {\n>>  \t\t\t$confirm = 'never';\n>> +\t\t\t$needs_separator = 1;\n>>  \t\t}\n>> +\t} else {\n>> +\t\t$needs_separator = 1;\n>>  \t}\n> \n> If you do not add this \"else\" clause to the outer \"are we doing\n> confirmation?\" if statement, and instead just set $needs_separator\n> *after* it, it would make it even more obvious what is going on.\n> The codeflow would become\n> \n> \tsub send_message {\n> \t\tdo bunch of things that do not yet send e-mail\n> \t        and possibly return or die\n> \n> \t\t$needs_separator = 1;\n> \n> \t\tdo things that cause the smtp exchange and trace\n> \t\tto be emitted\n> \t}\n> \n> That makes it obvious that the purpose of $needs_separator is to\n> record the fact that \"this\" message has already been sent and we\n> need to add a \"gap\" before attempting to send the \"next\" message.\n\nVery good point, thanks!  I've somehow managed to miss that while\niterating through a few code variants and the associated testing.\nWill be improved in the v6.\n\n> Other than the above points, very well done.\n\nThank you! :)\n\n>>  \tunshift (@sendmail_parameters, @smtp_server_options);\n>> @@ -1576,7 +1579,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>> @@ -1921,7 +1923,8 @@ sub pre_process_file {\n>>  sub process_file {\n>>  \tmy ($t) = @_;\n>> \n>> -        pre_process_file($t, $quiet);\n>> +\tpre_process_file($t, $quiet);\n> \n> nice ;-)\n\nIt had to be fixed, IMHO. :)\n\n>> +\tprint \"\\n\" if ($needs_separator);\n>> \n>>  \tmy $message_was_sent = send_message();\n>>  \tif ($message_was_sent == -1) {\n"},{"id":"492677","messageId":"feabd3a4b4fc6f03d31b59340bc238cc@manjaro.org","threadId":"61288","inReplyTo":"bc3998b1c9bbdbc75287735b42013f52@manjaro.org","subject":"Re: [PATCH v5 3/3] send-email: separate the confirmation prompts from the messages","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-10T03:53:35Z","receivedAt":"2024-04-10T03:53:38Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-09 05:25, Dragan Simic wrote:\n> On 2024-04-08 23:21, Junio C Hamano wrote:\n>> When $confirm is set to 'never', you have both $needs_separtor and\n>> $prompt_separator set.  Would it mean you'd have two extra blank\n>> lines for that message?\n> \n> Actually, there are no unneeded blank lines in that case, which\n> I've also tested before sending the patches.  One of the blank\n> lines (the one for $needs_separator) goes after the patch messages,\n> and the other one (the one for $prompt_separator) goes after the\n> prompt, which is displayed before the patch messages.\n\nActually, there's a much, _much_ simpler solution for everything,\nwhich also works as expected when running \"git send-email --quiet\"\nwith sendmail.confirm set to \"auto\" or \"never\".\n\nI need to test it a bit more, and I'll send the updated patches.\n"},{"id":"492679","messageId":"a70e1098f177cc84f8834fac0ac6f253@manjaro.org","threadId":"61288","inReplyTo":"feabd3a4b4fc6f03d31b59340bc238cc@manjaro.org","subject":"Re: [PATCH v5 3/3] send-email: separate the confirmation prompts from the messages","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-10T06:07:42Z","receivedAt":"2024-04-10T06:07:44Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-10 05:53, Dragan Simic wrote:\n> On 2024-04-09 05:25, Dragan Simic wrote:\n>> On 2024-04-08 23:21, Junio C Hamano wrote:\n>>> When $confirm is set to 'never', you have both $needs_separtor and\n>>> $prompt_separator set.  Would it mean you'd have two extra blank\n>>> lines for that message?\n>> \n>> Actually, there are no unneeded blank lines in that case, which\n>> I've also tested before sending the patches.  One of the blank\n>> lines (the one for $needs_separator) goes after the patch messages,\n>> and the other one (the one for $prompt_separator) goes after the\n>> prompt, which is displayed before the patch messages.\n> \n> Actually, there's a much, _much_ simpler solution for everything,\n> which also works as expected when running \"git send-email --quiet\"\n> with sendmail.confirm set to \"auto\" or \"never\".\n> \n> I need to test it a bit more, and I'll send the updated patches.\n\nThis new, simplified approach works well, but I've spotted some\nmore scenarios that also require addition of newlines.  Though,\nbased on previous lessons, :) I'll leave that to the follow-up\npatches.\n"}]}