| Message ID | 8a9f4927aab96f2f62e2467e59fb6150d7e931fc.1712367983.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] send-email: make it easy to discern the messages for each patch | expand |
Dragan Simic <dsimic@manjaro.org> writes: > Following the approach of making the produced output more readable, also > emit additional vertical whitespace after the "Send this email [y/n/...]?" > prompt. Hmph. I'd prefer to see you try not to endlessly extend the scope of a topic. By including the above change, the patch no longer is small and focused enough, which was the reason why we said that the "let's move the final newline out of the translatable string" can be done as a "while at it" change. Besides, because of the switch to separator semantics, that hunk lost the reason to exist as part of the "use a blank line between output for each message"---the change no longer is needed to support the feature. Even though it is a good change to have, and it deserves to be justified by its merit alone. The whole thing deserves to be a three-patch series, the first one being a preliminarly "let's move the final newline out of the translatable string" step, followed by "let's have a gap between output for each patch sent out". Perhaps another "even during sending a single patch, we may want extra blank lines when use of editor and other user interation is involved" patch on top. I haven't formed an opinion on that last step, and I do not think I can spend any time to think about that new part of the feature for some time (others can review that part and give their opinion on it, of course, while I'll be working on other topics). It would mean you are adding yet another feature to delay the base improvement to stabilize. You really do not want to do that. In any case, this [v4], as a single ball of wax, is not something I can confidently say "I reviewed this and looks OK". Thanks.
On 2024-04-06 07:40, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> Following the approach of making the produced output more readable, >> also >> emit additional vertical whitespace after the "Send this email >> [y/n/...]?" >> prompt. > > Hmph. I'd prefer to see you try not to endlessly extend the scope > of a topic. > > By including the above change, the patch no longer is small and > focused enough, which was the reason why we said that the "let's > move the final newline out of the translatable string" can be done > as a "while at it" change. > > Besides, because of the switch to separator semantics, that hunk > lost the reason to exist as part of the "use a blank line between > output for each message"---the change no longer is needed to support > the feature. > > Even though it is a good change to have, and it deserves to be > justified by its merit alone. > > The whole thing deserves to be a three-patch series, the first one > being a preliminarly "let's move the final newline out of the > translatable string" step, followed by "let's have a gap between > output for each patch sent out". Perhaps another "even during > sending a single patch, we may want extra blank lines when use of > editor and other user interation is involved" patch on top. > > I haven't formed an opinion on that last step, and I do not think I > can spend any time to think about that new part of the feature for > some time (others can review that part and give their opinion on it, > of course, while I'll be working on other topics). It would mean > you are adding yet another feature to delay the base improvement to > stabilize. You really do not want to do that. Quite frankly, I think all these changes are small enough and understandable enough to be fine for being squashed into a single patch. See, I love perfection and I'm also kind of a perfectionist, but such an approach can sometimes actually become counterproductive. That's what I usually refer to as being pragmatic, in the sense of making things a bit less perfect but still fine, in the interest of "getting things done", so to speak. I hope it makes sense. However, if you insist I'm also perfectly fine with splitting this patch into a three-patch series. That would make it perfect, there's no doubt about it, but would it be a pragmatic approach, worth the additional time and effort? Perhaps not. > In any case, this [v4], as a single ball of wax, is not something I > can confidently say "I reviewed this and looks OK".
On 2024-04-06 10:56, Dragan Simic wrote: > On 2024-04-06 07:40, Junio C Hamano wrote: >> Dragan Simic <dsimic@manjaro.org> writes: >> >>> Following the approach of making the produced output more readable, >>> also >>> emit additional vertical whitespace after the "Send this email >>> [y/n/...]?" >>> prompt. >> >> Hmph. I'd prefer to see you try not to endlessly extend the scope >> of a topic. >> >> By including the above change, the patch no longer is small and >> focused enough, which was the reason why we said that the "let's >> move the final newline out of the translatable string" can be done >> as a "while at it" change. >> >> Besides, because of the switch to separator semantics, that hunk >> lost the reason to exist as part of the "use a blank line between >> output for each message"---the change no longer is needed to support >> the feature. >> >> Even though it is a good change to have, and it deserves to be >> justified by its merit alone. >> >> The whole thing deserves to be a three-patch series, the first one >> being a preliminarly "let's move the final newline out of the >> translatable string" step, followed by "let's have a gap between >> output for each patch sent out". Perhaps another "even during >> sending a single patch, we may want extra blank lines when use of >> editor and other user interation is involved" patch on top. >> >> I haven't formed an opinion on that last step, and I do not think I >> can spend any time to think about that new part of the feature for >> some time (others can review that part and give their opinion on it, >> of course, while I'll be working on other topics). It would mean >> you are adding yet another feature to delay the base improvement to >> stabilize. You really do not want to do that. > > Quite frankly, I think all these changes are small enough and > understandable enough to be fine for being squashed into a single > patch. See, I love perfection and I'm also kind of a perfectionist, > but such an approach can sometimes actually become counterproductive. > That's what I usually refer to as being pragmatic, in the sense of > making things a bit less perfect but still fine, in the interest > of "getting things done", so to speak. I hope it makes sense. > > However, if you insist I'm also perfectly fine with splitting this > patch into a three-patch series. That would make it perfect, there's > no doubt about it, but would it be a pragmatic approach, worth the > additional time and effort? Perhaps not. After thinking a bit more about it, I agree that splitting this patch into a three-patch series is the right thing to do. As you described it above, changes introduced in some versions of the patch made the original assumptions about squashing the changes together no longer apply. I'll split this patch into three separate patches and send those over. As a note, I think that making the outputs of "git send-mail" more readable is quite important, because we've seen people complaining about the whole process of sending patches to mailing lists. Making the outputs more readable can only help, if you agree. >> In any case, this [v4], as a single ball of wax, is not something I >> can confidently say "I reviewed this and looks OK".
Junio C Hamano <gitster@pobox.com> writes: > The whole thing deserves to be a three-patch series, the first one > being a preliminarly "let's move the final newline out of the > translatable string" step, followed by "let's have a gap between > output for each patch sent out". Perhaps another "even during > sending a single patch, we may want extra blank lines when use of > editor and other user interation is involved" patch on top. Or the latter two could be done in a single patch. I'll have to re-review the thing (if I were the only reviewer of the topic) so doing so would delay the completion of the topic, though.
On 2024-04-06 19:05, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> The whole thing deserves to be a three-patch series, the first one >> being a preliminarly "let's move the final newline out of the >> translatable string" step, followed by "let's have a gap between >> output for each patch sent out". Perhaps another "even during >> sending a single patch, we may want extra blank lines when use of >> editor and other user interation is involved" patch on top. > > Or the latter two could be done in a single patch. I'll have to > re-review the thing (if I were the only reviewer of the topic) so > doing so would delay the completion of the topic, though. Huh, I've already separated this patch into three patches, and IMHO they look nice and make everything less error prone. Would you agree with the three-patch approach, please?
Dragan Simic <dsimic@manjaro.org> writes: > On 2024-04-06 19:05, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >>> The whole thing deserves to be a three-patch series, the first one >>> being a preliminarly "let's move the final newline out of the >>> translatable string" step, followed by "let's have a gap between >>> output for each patch sent out". Perhaps another "even during >>> sending a single patch, we may want extra blank lines when use of >>> editor and other user interation is involved" patch on top. >> Or the latter two could be done in a single patch. I'll have to >> re-review the thing (if I were the only reviewer of the topic) so >> doing so would delay the completion of the topic, though. > > Huh, I've already separated this patch into three patches, and IMHO > they look nice and make everything less error prone. Would you agree > with the three-patch approach, please? My "or the latter two could be done in a single patch" was "alternatively you can", so either way is fine as long as the result is well structured. I know how to explain "insert a gap between patches" well. I do not know which one is easier to explain, between (1) now we have "insert a gap between patches" with patch [2/3], but when editor invocation and confirmation prompts are involved, there are these three cases where we want to tweak the logic to show gaps. Here in patch [3/3] I explain how each of these three affect the logic from the previous step. or (2) We want to insert a gap before showing the second and subsequent patches, unless in such and such cases. We also want to insert a gap when we do this and that. We do all of these in this patch [2/2]. So doing it in three-patches results in a series easier to understand by readers, by all means, please do. Thanks.
Hello Junio, On 2024-04-08 18:01, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> On 2024-04-06 19:05, Junio C Hamano wrote: >>> Junio C Hamano <gitster@pobox.com> writes: >>> >>>> The whole thing deserves to be a three-patch series, the first one >>>> being a preliminarly "let's move the final newline out of the >>>> translatable string" step, followed by "let's have a gap between >>>> output for each patch sent out". Perhaps another "even during >>>> sending a single patch, we may want extra blank lines when use of >>>> editor and other user interation is involved" patch on top. >>> Or the latter two could be done in a single patch. I'll have to >>> re-review the thing (if I were the only reviewer of the topic) so >>> doing so would delay the completion of the topic, though. >> >> Huh, I've already separated this patch into three patches, and IMHO >> they look nice and make everything less error prone. Would you agree >> with the three-patch approach, please? > > My "or the latter two could be done in a single patch" was > "alternatively you can", so either way is fine as long as the result > is well structured. Great, thanks. I really appreciate your highly detailed approach to reviewing the patches. > I know how to explain "insert a gap between > patches" well. I do not know which one is easier to explain, > between > > (1) now we have "insert a gap between patches" with patch [2/3], > but when editor invocation and confirmation prompts are > involved, there are these three cases where we want to tweak > the logic to show gaps. Here in patch [3/3] I explain how each > of these three affect the logic from the previous step. In my opinion, the insertion of vertical whitespace can also be seen rather independently when it comes to separating the patches, and separating the prompts from the patches. > (2) We want to insert a gap before showing the second and > subsequent patches, unless in such and such cases. We also > want to insert a gap when we do this and that. We do all of > these in this patch [2/2]. > > So doing it in three-patches results in a series easier to > understand by readers, by all means, please do. As I mentioned above, the rather independent nature of the insertion of the two "classes" of vertical whitespace makes the three-patch approach look more suitable to me. I think it's also a better option if some regression is discovered later, because the patches can be applied and tested separately. I already went ahead and submitted the three-patch series, [1] please have a look. I hope you'll like it. [1] https://lore.kernel.org/git/cover.1712486910.git.dsimic@manjaro.org/T/#u
diff --git a/git-send-email.perl b/git-send-email.perl index 821b2b3a135a..a09bc7fd6b96 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -212,6 +212,7 @@ sub format_2822_time { my $compose_filename; my $force = 0; my $dump_aliases = 0; +my $needs_separator = 0; # Variables to prevent short format-patch options from being captured # as abbreviated send-email options @@ -1361,7 +1362,6 @@ sub smtp_host_string { # Returns 1 if authentication succeeded or was not necessary # (smtp_user was not specified), and 0 otherwise. - sub smtp_auth_maybe { if (!defined $smtp_authuser || $auth || (defined $smtp_auth && $smtp_auth eq "none")) { return 1; @@ -1510,6 +1510,7 @@ sub gen_header { sub send_message { my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header(); my @recipients = @$recipients_ref; + my $prompt_separator = 0; my @sendmail_parameters = ('-i', @recipients); my $raw_from = $sender; @@ -1554,7 +1555,11 @@ sub send_message { exit(0); } elsif (/^a/i) { $confirm = 'never'; + $needs_separator = 1; } + $prompt_separator = 1; + } else { + $needs_separator = 1; } unshift (@sendmail_parameters, @smtp_server_options); @@ -1576,7 +1581,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.") } @@ -1663,6 +1667,7 @@ sub send_message { $smtp->dataend() or die $smtp->message; $smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"), $subject).$smtp->message; } + print "\n" if ($prompt_separator); if ($quiet) { printf($dry_run ? __("Dry-Sent %s\n") : __("Sent %s\n"), $subject); } else { @@ -1686,10 +1691,11 @@ 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"); } + print "\n"; } return 1; @@ -1920,7 +1926,8 @@ sub pre_process_file { sub process_file { my ($t) = @_; - pre_process_file($t, $quiet); + pre_process_file($t, $quiet); + print "\n" if ($needs_separator); my $message_was_sent = send_message(); if ($message_was_sent == -1) {
When sending multiple patches at once, without 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, except after the last one, to make the outputs more readable, as visible in the sample output excerpt below, produced after the addition of vertical whitespace: ... MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Result: 250 OK. Log says: ... These changes don't emit additional vertical whitespace after the result status produced for the last processed patch, i.e. the vertical whitespace is treated as a separator. This follows the Git's general approach of not wasting the vertical screen space whenever possible. The way the additional vertical separation between the messages produced for each patch is introduced to the source code also moves the already existing newline characters out of the couple of translatable strings, which should help a bit with the translation efforts. Following the approach of making the produced output more readable, also emit additional vertical whitespace after the "Send this email [y/n/...]?" prompt. The subsequent produced messages were also bunched together with this prompt, as visible in the excerpt right below, which also contributed to making the discerning of the produced outputs unnecessarily hard. ... Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y OK. Log says: ... While there, remove a couple of spotted stray newlines in the source code and convert one indentation from spaces to tabs, for consistency. The associated test, t9001, requires no updates to cover these changes. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- Notes: * send-email: make produced outputs more readable by separating the result statuses and the prompts from the subsequent outputs Changes in v4: - Dropped the changes to the styling of the produced prompts, as reasonably requested by Junio, [1] because it extended the scope of the patch with no good reason, and may also create issues on some platforms, whose Perl packages may actually not support the "->ornaments()" feature of Term::ReadLine - Updated the patch description and the "what's cooking" summary to cover the changes Changes in v3: - Removed a redundant comment, as suggested by Junio [2] - Did the right thing and made the vertical separators emitted as message separators, instead of having them emitted as message terminators, as suggested by Junio [2] - Additional vertical whitespace is now also emitted after the prompt for sending emails, to "de-bunch" it from the subsequent messages and make discerning the messages easier - The above-mentioned prompt no longer uses underlined text, to make it significantly easier on the eyes - Fixed one indentation by spaces to use tabs and removed one stray newline in the source code, as spotted - Updated and extended the patch description to cover the changes - Updated the "what's cooking" summary to cover the changes - Cleaned up the older notes a bit Changes in v2: - Improved the way additional newline separators are introduced to the source code, as suggested by Junio, [3], 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 [3] - Added a Helped-by tag Notes for v1: - This is a resubmission of the patch I submitted about a week and a half ago; [4] 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 Link to v2: https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/T/#u Link to v3: https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u [1] https://lore.kernel.org/git/xmqq8r1rs39g.fsf@gitster.g/ [2] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/ [3] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/ [4] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/ Range-diff against v3: 1: 21168cead42a ! 1: 8a9f4927aab9 send-email: make it easy to discern the messages for each patch @@ Commit message OK. Log says: ... - As the final touch, make the above-mentioned prompt emitted without using - underlined text, which also applies to any other produced prompts, which made - them somewhat hard on the eyes, especially because the prompt's tailing space - character was also underlined. - While there, remove a couple of spotted stray newlines in the source code and convert one indentation from spaces to tabs, for consistency. The associated test, t9001, requires no updates to cover these changes. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Dragan Simic <dsimic@manjaro.org> ## Notes ## * send-email: make produced outputs more readable by separating - the result statuses and the prompts from the subsequent patch - outputs, and by no longer using underlined text for the prompts + the result statuses and the prompts from the subsequent outputs + + Changes in v4: + - Dropped the changes to the styling of the produced prompts, as + reasonably requested by Junio, [1] because it extended the scope + of the patch with no good reason, and may also create issues on + some platforms, whose Perl packages may actually not support the + "->ornaments()" feature of Term::ReadLine + - Updated the patch description and the "what's cooking" summary + to cover the changes Changes in v3: - - Removed a redundant comment, as suggested by Junio [1] + - Removed a redundant comment, as suggested by Junio [2] - Did the right thing and made the vertical separators emitted as message separators, instead of having them emitted as message - terminators, as suggested by Junio [1] + terminators, as suggested by Junio [2] - Additional vertical whitespace is now also emitted after the prompt for sending emails, to "de-bunch" it from the subsequent messages and make discerning the messages easier - The above-mentioned prompt no longer uses underlined text, to make it significantly easier on the eyes - Fixed one indentation by spaces to use tabs and removed one stray newline in the source code, as spotted - Updated and extended the patch description to cover the changes - Updated the "what's cooking" summary to cover the changes - Cleaned up the older notes a bit Changes in v2: - Improved the way additional newline separators are introduced to - the source code, as suggested by Junio, [2], to help a bit with + the source code, as suggested by Junio, [3], 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 [2] + additional information, as suggested by Junio [3] - Added a Helped-by tag Notes for v1: - This is a resubmission of the patch I submitted about a week and - a half ago; [3] the patch subject in the original submission was + a half ago; [4] 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 Link to v2: https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/T/#u + Link to v3: https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/T/#u - [1] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/ - [2] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/ - [3] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/ + [1] https://lore.kernel.org/git/xmqq8r1rs39g.fsf@gitster.g/ + [2] https://lore.kernel.org/git/xmqqzfu8yc40.fsf@gitster.g/ + [3] https://lore.kernel.org/git/xmqqy19tylrm.fsf@gitster.g/ + [4] https://lore.kernel.org/git/6ee28707b9eb8bd8fdfc8756c351455c6bc3bb62.1711447365.git.dsimic@manjaro.org/ ## git-send-email.perl ## @@ git-send-email.perl: sub format_2822_time { my $compose_filename; my $force = 0; my $dump_aliases = 0; +my $needs_separator = 0; # Variables to prevent short format-patch options from being captured # as abbreviated send-email options -@@ git-send-email.perl: sub get_patch_subject { - $term = $ENV{"GIT_SEND_EMAIL_NOTTY"} - ? Term::ReadLine->new('git-send-email', \*STDIN, \*STDOUT) - : Term::ReadLine->new('git-send-email'); -+ $term->ornaments(0); - } - return $term; - } @@ git-send-email.perl: sub smtp_host_string { # Returns 1 if authentication succeeded or was not necessary git-send-email.perl | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)