{"thread":{"id":"61279","subject":"[PATCH v4] send-email: make it easy to discern the messages for each patch","startedAt":"2024-04-06T01:48:33Z","lastAt":"2024-04-08T16:15:52Z","messageCount":8,"participants":["Dragan Simic","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"492360","messageId":"8a9f4927aab96f2f62e2467e59fb6150d7e931fc.1712367983.git.dsimic@manjaro.org","threadId":"61279","inReplyTo":null,"subject":"[PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-06T01:48:28Z","receivedAt":"2024-04-06T01:48:33Z","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, except after\nthe last one, to make the outputs more readable, as visible in the sample\noutput excerpt below, produced 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.  This follows the Git's general approach of not\nwasting the vertical screen space whenever possible.\n\nThe way the additional vertical separation between the messages produced for\neach patch is introduced to the source code also moves the already existing\nnewline characters out of the couple of translatable strings, which should\nhelp a bit with the translation efforts.\n\nFollowing the approach of making the produced output more readable, also\nemit additional vertical whitespace after the \"Send this email [y/n/...]?\"\nprompt.  The subsequent produced messages were also bunched together with\nthis prompt, as visible in the excerpt right below, which also contributed\nto making the discerning of the produced outputs unnecessarily hard.\n\n    ...\n    Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y\n    OK. Log says:\n    ...\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\nNotes:\n     * send-email: make produced outputs more readable by separating\n       the result statuses and the prompts from the subsequent outputs\n    \n    Changes in v4:\n        - Dropped the changes to the styling of the produced prompts, as\n          reasonably requested by Junio, [1] 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    \n    Changes in v3:\n        - Removed a redundant comment, as suggested by Junio [2]\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 [2]\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    \n    Changes in v2:\n        - Improved the way additional newline separators are introduced to\n          the source code, as suggested by Junio, [3], 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 [3]\n        - Added a Helped-by tag\n    \n    Notes for v1:\n        - This is a resubmission of the patch I submitted about a week and\n          a half ago; [4]  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    Link to v2: https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/T/#u\n    Link to v3: https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u\n    \n    [1] https://lore.kernel.org/git/xmqq8r1rs39g.fsf@gitster.g/\n    [2] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/\n    [3] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/\n    [4] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/\n\nRange-diff against v3:\n1:  21168cead42a ! 1:  8a9f4927aab9 send-email: make it easy to discern the messages for each patch\n    @@ Commit message\n             OK. Log says:\n             ...\n     \n    -    As the final touch, make the above-mentioned prompt emitted without using\n    -    underlined text, which also applies to any other produced prompts, which made\n    -    them somewhat hard on the eyes, especially because the prompt's tailing space\n    -    character was also underlined.\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     \n         Helped-by: Junio C Hamano <gitster@pobox.com>\n         Signed-off-by: Dragan Simic <dsimic@manjaro.org>\n     \n     \n      ## Notes ##\n          * send-email: make produced outputs more readable by separating\n    -       the result statuses and the prompts from the subsequent patch\n    -       outputs, and by no longer using underlined text for the prompts\n    +       the result statuses and the prompts from the subsequent outputs\n    +\n    +    Changes in v4:\n    +        - Dropped the changes to the styling of the produced prompts, as\n    +          reasonably requested by Junio, [1] 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     \n         Changes in v3:\n    -        - Removed a redundant comment, as suggested by Junio [1]\n    +        - Removed a redundant comment, as suggested by Junio [2]\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 [1]\n    +          terminators, as suggested by Junio [2]\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     \n         Changes in v2:\n             - Improved the way additional newline separators are introduced to\n    -          the source code, as suggested by Junio, [2], to help a bit with\n    +          the source code, as suggested by Junio, [3], 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 [2]\n    +          additional information, as suggested by Junio [3]\n             - Added a Helped-by tag\n     \n         Notes for v1:\n             - This is a resubmission of the patch I submitted about a week and\n    -          a half ago; [3]  the patch subject in the original submission was\n    +          a half ago; [4]  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         Link to v2: https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/T/#u\n    +    Link to v3: https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u\n     \n    -    [1] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/\n    -    [2] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/\n    -    [3] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/\n    +    [1] https://lore.kernel.org/git/xmqq8r1rs39g.fsf@gitster.g/\n    +    [2] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/\n    +    [3] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/\n    +    [4] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/\n     \n      ## git-send-email.perl ##\n     @@ git-send-email.perl: 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    -@@ git-send-email.perl: sub get_patch_subject {\n    - \t\t\t$term = $ENV{\"GIT_SEND_EMAIL_NOTTY\"}\n    - \t\t\t\t? Term::ReadLine->new('git-send-email', \\*STDIN, \\*STDOUT)\n    - \t\t\t\t: Term::ReadLine->new('git-send-email');\n    -+\t\t\t$term->ornaments(0);\n    - \t\t}\n    - \t\treturn $term;\n    - \t}\n     @@ git-send-email.perl: sub smtp_host_string {\n      \n      # Returns 1 if authentication succeeded or was not necessary\n\n git-send-email.perl | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 821b2b3a135a..a09bc7fd6b96 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@@ -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@@ -1554,7 +1555,11 @@ 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\t$prompt_separator = 1;\n+\t} else {\n+\t\t$needs_separator = 1;\n \t}\n \n \tunshift (@sendmail_parameters, @smtp_server_options);\n@@ -1576,7 +1581,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@@ -1663,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@@ -1686,10 +1691,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@@ -1920,7 +1926,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":"492371","messageId":"xmqqwmpbm4lp.fsf@gitster.g","threadId":"61279","inReplyTo":"8a9f4927aab96f2f62e2467e59fb6150d7e931fc.1712367983.git.dsimic@manjaro.org","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T05:40:02Z","receivedAt":"2024-04-06T05:40:05Z","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> Following the approach of making the produced output more readable, also\n> emit additional vertical whitespace after the \"Send this email [y/n/...]?\"\n> prompt.\n\nHmph.  I'd prefer to see you try not to endlessly extend the scope\nof a topic.\n\nBy including the above change, the patch no longer is small and\nfocused enough, which was the reason why we said that the \"let's\nmove the final newline out of the translatable string\" can be done\nas a \"while at it\" change.\n\nBesides, because of the switch to separator semantics, that hunk\nlost the reason to exist as part of the \"use a blank line between\noutput for each message\"---the change no longer is needed to support\nthe feature.\n\nEven though it is a good change to have, and it deserves to be\njustified by its merit alone.\n\nThe whole thing deserves to be a three-patch series, the first one\nbeing a preliminarly \"let's move the final newline out of the\ntranslatable string\" step, followed by \"let's have a gap between\noutput for each patch sent out\".  Perhaps another \"even during\nsending a single patch, we may want extra blank lines when use of\neditor and other user interation is involved\" patch on top.\n\nI haven't formed an opinion on that last step, and I do not think I\ncan spend any time to think about that new part of the feature for\nsome time (others can review that part and give their opinion on it,\nof course, while I'll be working on other topics).  It would mean\nyou are adding yet another feature to delay the base improvement to\nstabilize.  You really do not want to do that.\n\nIn any case, this [v4], as a single ball of wax, is not something I\ncan confidently say \"I reviewed this and looks OK\".\n\nThanks.\n"},{"id":"492376","messageId":"e90f6c80be33b2b54e20992f19e417dc@manjaro.org","threadId":"61279","inReplyTo":"xmqqwmpbm4lp.fsf@gitster.g","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-06T08:56:07Z","receivedAt":"2024-04-06T08:56:09Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-06 07:40, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> Following the approach of making the produced output more readable, \n>> also\n>> emit additional vertical whitespace after the \"Send this email \n>> [y/n/...]?\"\n>> prompt.\n> \n> Hmph.  I'd prefer to see you try not to endlessly extend the scope\n> of a topic.\n> \n> By including the above change, the patch no longer is small and\n> focused enough, which was the reason why we said that the \"let's\n> move the final newline out of the translatable string\" can be done\n> as a \"while at it\" change.\n> \n> Besides, because of the switch to separator semantics, that hunk\n> lost the reason to exist as part of the \"use a blank line between\n> output for each message\"---the change no longer is needed to support\n> the feature.\n> \n> Even though it is a good change to have, and it deserves to be\n> justified by its merit alone.\n> \n> The whole thing deserves to be a three-patch series, the first one\n> being a preliminarly \"let's move the final newline out of the\n> translatable string\" step, followed by \"let's have a gap between\n> output for each patch sent out\".  Perhaps another \"even during\n> sending a single patch, we may want extra blank lines when use of\n> editor and other user interation is involved\" patch on top.\n> \n> I haven't formed an opinion on that last step, and I do not think I\n> can spend any time to think about that new part of the feature for\n> some time (others can review that part and give their opinion on it,\n> of course, while I'll be working on other topics).  It would mean\n> you are adding yet another feature to delay the base improvement to\n> stabilize.  You really do not want to do that.\n\nQuite frankly, I think all these changes are small enough and\nunderstandable enough to be fine for being squashed into a single\npatch.  See, I love perfection and I'm also kind of a perfectionist,\nbut such an approach can sometimes actually become counterproductive.\nThat's what I usually refer to as being pragmatic, in the sense of\nmaking things a bit less perfect but still fine, in the interest\nof \"getting things done\", so to speak.  I hope it makes sense.\n\nHowever, if you insist I'm also perfectly fine with splitting this\npatch into a three-patch series.  That would make it perfect, there's\nno doubt about it, but would it be a pragmatic approach, worth the\nadditional time and effort?  Perhaps not.\n\n> In any case, this [v4], as a single ball of wax, is not something I\n> can confidently say \"I reviewed this and looks OK\".\n"},{"id":"492379","messageId":"713c28cfc9dff2d01159105ddd2fd0f5@manjaro.org","threadId":"61279","inReplyTo":"e90f6c80be33b2b54e20992f19e417dc@manjaro.org","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-06T10:18:31Z","receivedAt":"2024-04-06T10:18:33Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-06 10:56, Dragan Simic wrote:\n> On 2024-04-06 07:40, Junio C Hamano wrote:\n>> Dragan Simic <dsimic@manjaro.org> writes:\n>> \n>>> Following the approach of making the produced output more readable, \n>>> also\n>>> emit additional vertical whitespace after the \"Send this email \n>>> [y/n/...]?\"\n>>> prompt.\n>> \n>> Hmph.  I'd prefer to see you try not to endlessly extend the scope\n>> of a topic.\n>> \n>> By including the above change, the patch no longer is small and\n>> focused enough, which was the reason why we said that the \"let's\n>> move the final newline out of the translatable string\" can be done\n>> as a \"while at it\" change.\n>> \n>> Besides, because of the switch to separator semantics, that hunk\n>> lost the reason to exist as part of the \"use a blank line between\n>> output for each message\"---the change no longer is needed to support\n>> the feature.\n>> \n>> Even though it is a good change to have, and it deserves to be\n>> justified by its merit alone.\n>> \n>> The whole thing deserves to be a three-patch series, the first one\n>> being a preliminarly \"let's move the final newline out of the\n>> translatable string\" step, followed by \"let's have a gap between\n>> output for each patch sent out\".  Perhaps another \"even during\n>> sending a single patch, we may want extra blank lines when use of\n>> editor and other user interation is involved\" patch on top.\n>> \n>> I haven't formed an opinion on that last step, and I do not think I\n>> can spend any time to think about that new part of the feature for\n>> some time (others can review that part and give their opinion on it,\n>> of course, while I'll be working on other topics).  It would mean\n>> you are adding yet another feature to delay the base improvement to\n>> stabilize.  You really do not want to do that.\n> \n> Quite frankly, I think all these changes are small enough and\n> understandable enough to be fine for being squashed into a single\n> patch.  See, I love perfection and I'm also kind of a perfectionist,\n> but such an approach can sometimes actually become counterproductive.\n> That's what I usually refer to as being pragmatic, in the sense of\n> making things a bit less perfect but still fine, in the interest\n> of \"getting things done\", so to speak.  I hope it makes sense.\n> \n> However, if you insist I'm also perfectly fine with splitting this\n> patch into a three-patch series.  That would make it perfect, there's\n> no doubt about it, but would it be a pragmatic approach, worth the\n> additional time and effort?  Perhaps not.\n\nAfter thinking a bit more about it, I agree that splitting this\npatch into a three-patch series is the right thing to do.  As\nyou described it above, changes introduced in some versions of\nthe patch made the original assumptions about squashing the changes\ntogether no longer apply.  I'll split this patch into three separate\npatches and send those over.\n\nAs a note, I think that making the outputs of \"git send-mail\" more\nreadable is quite important, because we've seen people complaining\nabout the whole process of sending patches to mailing lists.  Making\nthe outputs more readable can only help, if you agree.\n\n>> In any case, this [v4], as a single ball of wax, is not something I\n>> can confidently say \"I reviewed this and looks OK\".\n"},{"id":"492397","messageId":"xmqq5xwul8vn.fsf@gitster.g","threadId":"61279","inReplyTo":"xmqqwmpbm4lp.fsf@gitster.g","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T17:05:16Z","receivedAt":"2024-04-06T17:05:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> The whole thing deserves to be a three-patch series, the first one\n> being a preliminarly \"let's move the final newline out of the\n> translatable string\" step, followed by \"let's have a gap between\n> output for each patch sent out\".  Perhaps another \"even during\n> sending a single patch, we may want extra blank lines when use of\n> editor and other user interation is involved\" patch on top.\n\nOr the latter two could be done in a single patch.  I'll have to\nre-review the thing (if I were the only reviewer of the topic) so\ndoing so would delay the completion of the topic, though.\n"},{"id":"492399","messageId":"6cd48321180195e7070825dbc3f49578@manjaro.org","threadId":"61279","inReplyTo":"xmqq5xwul8vn.fsf@gitster.g","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-06T17:08:55Z","receivedAt":"2024-04-06T17:08:57Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-06 19:05, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> The whole thing deserves to be a three-patch series, the first one\n>> being a preliminarly \"let's move the final newline out of the\n>> translatable string\" step, followed by \"let's have a gap between\n>> output for each patch sent out\".  Perhaps another \"even during\n>> sending a single patch, we may want extra blank lines when use of\n>> editor and other user interation is involved\" patch on top.\n> \n> Or the latter two could be done in a single patch.  I'll have to\n> re-review the thing (if I were the only reviewer of the topic) so\n> doing so would delay the completion of the topic, though.\n\nHuh, I've already separated this patch into three patches, and IMHO\nthey look nice and make everything less error prone.  Would you agree\nwith the three-patch approach, please?\n"},{"id":"492547","messageId":"xmqqttkbetd3.fsf@gitster.g","threadId":"61279","inReplyTo":"6cd48321180195e7070825dbc3f49578@manjaro.org","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T16:01:28Z","receivedAt":"2024-04-08T16:01:33Z","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> On 2024-04-06 19:05, Junio C Hamano wrote:\n>> Junio C Hamano <gitster@pobox.com> writes:\n>> \n>>> The whole thing deserves to be a three-patch series, the first one\n>>> being a preliminarly \"let's move the final newline out of the\n>>> translatable string\" step, followed by \"let's have a gap between\n>>> output for each patch sent out\".  Perhaps another \"even during\n>>> sending a single patch, we may want extra blank lines when use of\n>>> editor and other user interation is involved\" patch on top.\n>> Or the latter two could be done in a single patch.  I'll have to\n>> re-review the thing (if I were the only reviewer of the topic) so\n>> doing so would delay the completion of the topic, though.\n>\n> Huh, I've already separated this patch into three patches, and IMHO\n> they look nice and make everything less error prone.  Would you agree\n> with the three-patch approach, please?\n\nMy \"or the latter two could be done in a single patch\" was\n\"alternatively you can\", so either way is fine as long as the result\nis well structured.  I know how to explain \"insert a gap between\npatches\" well.  I do not know which one is easier to explain,\nbetween \n\n (1) now we have \"insert a gap between patches\" with patch [2/3],\n     but when editor invocation and confirmation prompts are\n     involved, there are these three cases where we want to tweak\n     the logic to show gaps.  Here in patch [3/3] I explain how each\n     of these three affect the logic from the previous step.\n\nor\n\n (2) We want to insert a gap before showing the second and\n     subsequent patches, unless in such and such cases.  We also\n     want to insert a gap when we do this and that.  We do all of\n     these in this patch [2/2].\n\nSo doing it in three-patches results in a series easier to\nunderstand by readers, by all means, please do.\n\nThanks.\n"},{"id":"492549","messageId":"d334e0cc3a681d7ec5b06edac7d21232@manjaro.org","threadId":"61279","inReplyTo":"xmqqttkbetd3.fsf@gitster.g","subject":"Re: [PATCH v4] send-email: make it easy to discern the messages for each patch","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-08T16:15:48Z","receivedAt":"2024-04-08T16:15:52Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Junio,\n\nOn 2024-04-08 18:01, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> On 2024-04-06 19:05, Junio C Hamano wrote:\n>>> Junio C Hamano <gitster@pobox.com> writes:\n>>> \n>>>> The whole thing deserves to be a three-patch series, the first one\n>>>> being a preliminarly \"let's move the final newline out of the\n>>>> translatable string\" step, followed by \"let's have a gap between\n>>>> output for each patch sent out\".  Perhaps another \"even during\n>>>> sending a single patch, we may want extra blank lines when use of\n>>>> editor and other user interation is involved\" patch on top.\n>>> Or the latter two could be done in a single patch.  I'll have to\n>>> re-review the thing (if I were the only reviewer of the topic) so\n>>> doing so would delay the completion of the topic, though.\n>> \n>> Huh, I've already separated this patch into three patches, and IMHO\n>> they look nice and make everything less error prone.  Would you agree\n>> with the three-patch approach, please?\n> \n> My \"or the latter two could be done in a single patch\" was\n> \"alternatively you can\", so either way is fine as long as the result\n> is well structured.\n\nGreat, thanks.  I really appreciate your highly detailed approach\nto reviewing the patches.\n\n> I know how to explain \"insert a gap between\n> patches\" well.  I do not know which one is easier to explain,\n> between\n> \n>  (1) now we have \"insert a gap between patches\" with patch [2/3],\n>      but when editor invocation and confirmation prompts are\n>      involved, there are these three cases where we want to tweak\n>      the logic to show gaps.  Here in patch [3/3] I explain how each\n>      of these three affect the logic from the previous step.\n\nIn my opinion, the insertion of vertical whitespace can also be\nseen rather independently when it comes to separating the patches,\nand separating the prompts from the patches.\n\n>  (2) We want to insert a gap before showing the second and\n>      subsequent patches, unless in such and such cases.  We also\n>      want to insert a gap when we do this and that.  We do all of\n>      these in this patch [2/2].\n> \n> So doing it in three-patches results in a series easier to\n> understand by readers, by all means, please do.\n\nAs I mentioned above, the rather independent nature of the insertion\nof the two \"classes\" of vertical whitespace makes the three-patch\napproach look more suitable to me.  I think it's also a better option\nif some regression is discovered later, because the patches can be\napplied and tested separately.\n\nI already went ahead and submitted the three-patch series, [1] please\nhave a look.  I hope you'll like it.\n\n[1] \nhttps://lore.kernel.org/git/cover.1712486910.git.dsimic@manjaro.org/T/#u\n"}]}