diff mbox series

[v6,2/2] send-email: make it easy to discern the messages for each patch

Message ID c78b043b5a6cf0de712d36e6e000804bd6e1316d.1712732383.git.dsimic@manjaro.org (mailing list archive)
State New
Headers show
Series send-email: make produced outputs more readable | expand

Commit Message

Dragan Simic April 10, 2024, 7:01 a.m. UTC
When sending one or multiple patches at once, the displayed result statuses
for each patch and the "Send this email [y/n/a/...]?" confirmation prompts
become bunched together with the messages produced for the subsequent patch,
or with the produced SMTP trace, respectively.

This makes reading the outputs unnecessarily harder, as visible in a couple
of excerpts from a sample output below:

    ...
    MIME-Version: 1.0
    Content-Transfer-Encoding: 8bit

    Result: 250
    OK. Log says:
    Server: smtp.example.com
    MAIL FROM:<test@example.com>
    ...

    ...
    MIME-Version: 1.0
    Content-Transfer-Encoding: 8bit

    Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
    OK. Log says:
    Server: smtp.example.com
    MAIL FROM:<test@example.com>
    ...

As visible in the excerpts above, bunching the "Result: <status-code>" lines
or the "Send this email [y/n/a/...]?" confirmation prompts together with the
other messages makes the outputs a bit unreadable, which actually becomes
worse as the number of patches sent at once increases.

To make the produced outputs more readable, ensure that vertical whitespace
(more precisely, single newlines) exist before the displayed result statuses
and after the confirmation prompts, as visible in the two updated excerpts
from a sample output below:

    ...
    MIME-Version: 1.0
    Content-Transfer-Encoding: 8bit

    Result: 250

    OK. Log says:
    Server: smtp.example.com
    MAIL FROM:<test@example.com>
    ...

    ...
    MIME-Version: 1.0
    Content-Transfer-Encoding: 8bit

    Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y

    OK. Log says:
    Server: smtp.example.com
    MAIL FROM:<test@example.com>
    ...

While there, remove a couple of spotted stray newlines in the source code
and convert one indentation from spaces to tabs, for consistency.

Update the associated test, t9001, by including additional newlines into
the expected outputs of separate tests affected by these changes.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 git-send-email.perl   |  8 +++++---
 t/t9001-send-email.sh | 10 ++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Junio C Hamano April 10, 2024, 4:28 p.m. UTC | #1
Dragan Simic <dsimic@manjaro.org> writes:

> When sending one or multiple patches at once, the displayed result statuses
> for each patch and the "Send this email [y/n/a/...]?" confirmation prompts
> become bunched together with the messages produced for the subsequent patch,
> or with the produced SMTP trace, respectively.
>
> This makes reading the outputs unnecessarily harder, as visible in a couple
> of excerpts from a sample output below:

It is unclear where the boundaries between the messages in the
example are, though.

>     ...
>     MIME-Version: 1.0
>     Content-Transfer-Encoding: 8bit
>
>     Result: 250

Is this where one message ends, and the next line "OK. Log says:" is
the beginning of the next message?

>     OK. Log says:
>     Server: smtp.example.com
>     MAIL FROM:<test@example.com>
>     ...
>
>     ...
>     MIME-Version: 1.0
>     Content-Transfer-Encoding: 8bit

Is the above about a single (i.e. the second) message ...

>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y

... and the user is asked about that message?

>     OK. Log says:
>     Server: smtp.example.com
>     MAIL FROM:<test@example.com>
>     ...

And is this about a separate (i.e. the third) message?  Without
making these clear, it is hard to agree or disagree with the claim
that the current presentation is hard to read.

>     MIME-Version: 1.0
>     Content-Transfer-Encoding: 8bit
>
>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>
>     OK. Log says:
>     Server: smtp.example.com
>     MAIL FROM:<test@example.com>
>     ...

This is obviously in the realm of subjective preference, but I find
that the prompt line is distinct enough among all other output that
we do not need an extra blank line to locate them.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index f0be4b4560f7..1d6712a44e95 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1361,7 +1361,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 +1509,7 @@ sub gen_header {
>  sub send_message {
>  	my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
>  	my @recipients = @$recipients_ref;
> +	my $confirm_shown = 0;
>  
>  	my @sendmail_parameters = ('-i', @recipients);
>  	my $raw_from = $sender;
> @@ -1555,6 +1555,7 @@ sub send_message {
>  		} elsif (/^a/i) {
>  			$confirm = 'never';
>  		}
> +		$confirm_shown = 1;
>  	}
>  
>  	unshift (@sendmail_parameters, @smtp_server_options);
> @@ -1576,7 +1577,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.")
>  		}
> @@ -1664,9 +1664,11 @@ sub send_message {
>  		$smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"), $subject).$smtp->message;
>  	}
>  	if ($quiet) {
> +		print "\n" if ($confirm_shown);
>  		printf($dry_run ? __("Dry-Sent %s") : __("Sent %s"), $subject);
>  		print "\n";
>  	} else {
> +		print "\n";
>  		print($dry_run ? __("Dry-OK. Log says:") : __("OK. Log says:"));
>  		print "\n";
>  		if (!defined $sendmail_cmd && !file_name_is_absolute($smtp_server)) {
> @@ -1923,7 +1925,7 @@ sub pre_process_file {
>  sub process_file {
>  	my ($t) = @_;
>  
> -        pre_process_file($t, $quiet);
> +	pre_process_file($t, $quiet);
>  
>  	my $message_was_sent = send_message();
>  	if ($message_was_sent == -1) {

I'll let others comment as the "blank around prompt" smells quite
subjective and do not want to be the sole reviewer on it.

Thanks, will queue.
Eric Sunshine April 10, 2024, 10:59 p.m. UTC | #2
On Wed, Apr 10, 2024 at 12:28 PM Junio C Hamano <gitster@pobox.com> wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> >     MIME-Version: 1.0
> >     Content-Transfer-Encoding: 8bit
> >
> >     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
> >
> >     OK. Log says:
> >     Server: smtp.example.com
>
> This is obviously in the realm of subjective preference, but I find
> that the prompt line is distinct enough among all other output that
> we do not need an extra blank line to locate them.
>
> I'll let others comment as the "blank around prompt" smells quite
> subjective and do not want to be the sole reviewer on it.

For what it's worth, I had the same reaction about the extra blank
line related to the prompt being unnecessary, but I didn't feel
strongly enough to say anything about it, and I still don't feel
strongly enough to suggest dropping this change.
Dragan Simic April 13, 2024, 6:10 a.m. UTC | #3
Hello Eric,

On 2024-04-11 00:59, Eric Sunshine wrote:
> On Wed, Apr 10, 2024 at 12:28 PM Junio C Hamano <gitster@pobox.com> 
> wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> >     MIME-Version: 1.0
>> >     Content-Transfer-Encoding: 8bit
>> >
>> >     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>> >
>> >     OK. Log says:
>> >     Server: smtp.example.com
>> 
>> This is obviously in the realm of subjective preference, but I find
>> that the prompt line is distinct enough among all other output that
>> we do not need an extra blank line to locate them.
>> 
>> I'll let others comment as the "blank around prompt" smells quite
>> subjective and do not want to be the sole reviewer on it.
> 
> For what it's worth, I had the same reaction about the extra blank
> line related to the prompt being unnecessary, but I didn't feel
> strongly enough to say anything about it, and I still don't feel
> strongly enough to suggest dropping this change.

Basically, I went with a rather simple reasoning:  the confirmation
prompts, just like the SMTP statuses, aren't part of the emitted SMTP
traces and patch descriptions.  As such, separating the prompts with
vertical whitespace is actually consistent, and should help with the
overall readability, by taking the prompts visually out of the other
produced messages.
Dragan Simic April 13, 2024, 6:27 a.m. UTC | #4
On 2024-04-10 18:28, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> When sending one or multiple patches at once, the displayed result 
>> statuses
>> for each patch and the "Send this email [y/n/a/...]?" confirmation 
>> prompts
>> become bunched together with the messages produced for the subsequent 
>> patch,
>> or with the produced SMTP trace, respectively.
>> 
>> This makes reading the outputs unnecessarily harder, as visible in a 
>> couple
>> of excerpts from a sample output below:
> 
> It is unclear where the boundaries between the messages in the
> example are, though.
> 
>>     ...
>>     MIME-Version: 1.0
>>     Content-Transfer-Encoding: 8bit
>> 
>>     Result: 250
> 
> Is this where one message ends, and the next line "OK. Log says:" is
> the beginning of the next message?
> 
>>     OK. Log says:
>>     Server: smtp.example.com
>>     MAIL FROM:<test@example.com>
>>     ...
>> 
>>     ...
>>     MIME-Version: 1.0
>>     Content-Transfer-Encoding: 8bit
> 
> Is the above about a single (i.e. the second) message ...
> 
>>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
> 
> ... and the user is asked about that message?
> 
>>     OK. Log says:
>>     Server: smtp.example.com
>>     MAIL FROM:<test@example.com>
>>     ...

Huh, I understand your confusion and those are all valid remarks.
However, my intention was to include excerpts that are long enough
to illustrate the points to someone already familiar enough with
the outputs produced by "git send-mail".

If that isn't good enough for the intended audience of the Git
repository log, I unfortunately see no good way to provide excerpts
that are long enough to eliminate any doubts.  Such excerpts would
need to be half a dozen screens long, which would turn the patch
description into a monster.

With all that in mind, perhaps it's the best to simply delete all
excerpts from the patch description, if you agree?

> And is this about a separate (i.e. the third) message?  Without
> making these clear, it is hard to agree or disagree with the claim
> that the current presentation is hard to read.
> 
>>     MIME-Version: 1.0
>>     Content-Transfer-Encoding: 8bit
>> 
>>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>> 
>>     OK. Log says:
>>     Server: smtp.example.com
>>     MAIL FROM:<test@example.com>
>>     ...
> 
> This is obviously in the realm of subjective preference, but I find
> that the prompt line is distinct enough among all other output that
> we do not need an extra blank line to locate them.

Basically, I went with a rather simple reasoning:  the confirmation
prompts, just like the SMTP statuses, aren't part of the emitted SMTP
traces and patch descriptions.  They're different kinds of emitted
messages, if you agree.

Thus, separating the prompts with vertical whitespace is actually
consistent, and should help with the overall readability, by taking
the prompts visually out of the other produced messages.  In other
words, it's about keeping different kinds of emitted messages
separate, with the focus on the SMTP traces and patch descriptions,
instead of making the prompts locatable.

>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index f0be4b4560f7..1d6712a44e95 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1361,7 +1361,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 +1509,7 @@ sub gen_header {
>>  sub send_message {
>>  	my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) 
>> = gen_header();
>>  	my @recipients = @$recipients_ref;
>> +	my $confirm_shown = 0;
>> 
>>  	my @sendmail_parameters = ('-i', @recipients);
>>  	my $raw_from = $sender;
>> @@ -1555,6 +1555,7 @@ sub send_message {
>>  		} elsif (/^a/i) {
>>  			$confirm = 'never';
>>  		}
>> +		$confirm_shown = 1;
>>  	}
>> 
>>  	unshift (@sendmail_parameters, @smtp_server_options);
>> @@ -1576,7 +1577,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.")
>>  		}
>> @@ -1664,9 +1664,11 @@ sub send_message {
>>  		$smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"), 
>> $subject).$smtp->message;
>>  	}
>>  	if ($quiet) {
>> +		print "\n" if ($confirm_shown);
>>  		printf($dry_run ? __("Dry-Sent %s") : __("Sent %s"), $subject);
>>  		print "\n";
>>  	} else {
>> +		print "\n";
>>  		print($dry_run ? __("Dry-OK. Log says:") : __("OK. Log says:"));
>>  		print "\n";
>>  		if (!defined $sendmail_cmd && !file_name_is_absolute($smtp_server)) 
>> {
>> @@ -1923,7 +1925,7 @@ sub pre_process_file {
>>  sub process_file {
>>  	my ($t) = @_;
>> 
>> -        pre_process_file($t, $quiet);
>> +	pre_process_file($t, $quiet);
>> 
>>  	my $message_was_sent = send_message();
>>  	if ($message_was_sent == -1) {
> 
> I'll let others comment as the "blank around prompt" smells quite
> subjective and do not want to be the sole reviewer on it.
> 
> Thanks, will queue.
Dragan Simic April 27, 2024, 5:27 p.m. UTC | #5
Hello Junio,

Just checking, is there something I can do to get this patch
series moving forward?

On 2024-04-13 08:27, Dragan Simic wrote:
> On 2024-04-10 18:28, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> When sending one or multiple patches at once, the displayed result 
>>> statuses
>>> for each patch and the "Send this email [y/n/a/...]?" confirmation 
>>> prompts
>>> become bunched together with the messages produced for the subsequent 
>>> patch,
>>> or with the produced SMTP trace, respectively.
>>> 
>>> This makes reading the outputs unnecessarily harder, as visible in a 
>>> couple
>>> of excerpts from a sample output below:
>> 
>> It is unclear where the boundaries between the messages in the
>> example are, though.
>> 
>>>     ...
>>>     MIME-Version: 1.0
>>>     Content-Transfer-Encoding: 8bit
>>> 
>>>     Result: 250
>> 
>> Is this where one message ends, and the next line "OK. Log says:" is
>> the beginning of the next message?
>> 
>>>     OK. Log says:
>>>     Server: smtp.example.com
>>>     MAIL FROM:<test@example.com>
>>>     ...
>>> 
>>>     ...
>>>     MIME-Version: 1.0
>>>     Content-Transfer-Encoding: 8bit
>> 
>> Is the above about a single (i.e. the second) message ...
>> 
>>>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>> 
>> ... and the user is asked about that message?
>> 
>>>     OK. Log says:
>>>     Server: smtp.example.com
>>>     MAIL FROM:<test@example.com>
>>>     ...
> 
> Huh, I understand your confusion and those are all valid remarks.
> However, my intention was to include excerpts that are long enough
> to illustrate the points to someone already familiar enough with
> the outputs produced by "git send-mail".
> 
> If that isn't good enough for the intended audience of the Git
> repository log, I unfortunately see no good way to provide excerpts
> that are long enough to eliminate any doubts.  Such excerpts would
> need to be half a dozen screens long, which would turn the patch
> description into a monster.
> 
> With all that in mind, perhaps it's the best to simply delete all
> excerpts from the patch description, if you agree?
> 
>> And is this about a separate (i.e. the third) message?  Without
>> making these clear, it is hard to agree or disagree with the claim
>> that the current presentation is hard to read.
>> 
>>>     MIME-Version: 1.0
>>>     Content-Transfer-Encoding: 8bit
>>> 
>>>     Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>>> 
>>>     OK. Log says:
>>>     Server: smtp.example.com
>>>     MAIL FROM:<test@example.com>
>>>     ...
>> 
>> This is obviously in the realm of subjective preference, but I find
>> that the prompt line is distinct enough among all other output that
>> we do not need an extra blank line to locate them.
> 
> Basically, I went with a rather simple reasoning:  the confirmation
> prompts, just like the SMTP statuses, aren't part of the emitted SMTP
> traces and patch descriptions.  They're different kinds of emitted
> messages, if you agree.
> 
> Thus, separating the prompts with vertical whitespace is actually
> consistent, and should help with the overall readability, by taking
> the prompts visually out of the other produced messages.  In other
> words, it's about keeping different kinds of emitted messages
> separate, with the focus on the SMTP traces and patch descriptions,
> instead of making the prompts locatable.
> 
>>> diff --git a/git-send-email.perl b/git-send-email.perl
>>> index f0be4b4560f7..1d6712a44e95 100755
>>> --- a/git-send-email.perl
>>> +++ b/git-send-email.perl
>>> @@ -1361,7 +1361,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 +1509,7 @@ sub gen_header {
>>>  sub send_message {
>>>  	my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, 
>>> $header) = gen_header();
>>>  	my @recipients = @$recipients_ref;
>>> +	my $confirm_shown = 0;
>>> 
>>>  	my @sendmail_parameters = ('-i', @recipients);
>>>  	my $raw_from = $sender;
>>> @@ -1555,6 +1555,7 @@ sub send_message {
>>>  		} elsif (/^a/i) {
>>>  			$confirm = 'never';
>>>  		}
>>> +		$confirm_shown = 1;
>>>  	}
>>> 
>>>  	unshift (@sendmail_parameters, @smtp_server_options);
>>> @@ -1576,7 +1577,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.")
>>>  		}
>>> @@ -1664,9 +1664,11 @@ sub send_message {
>>>  		$smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"), 
>>> $subject).$smtp->message;
>>>  	}
>>>  	if ($quiet) {
>>> +		print "\n" if ($confirm_shown);
>>>  		printf($dry_run ? __("Dry-Sent %s") : __("Sent %s"), $subject);
>>>  		print "\n";
>>>  	} else {
>>> +		print "\n";
>>>  		print($dry_run ? __("Dry-OK. Log says:") : __("OK. Log says:"));
>>>  		print "\n";
>>>  		if (!defined $sendmail_cmd && 
>>> !file_name_is_absolute($smtp_server)) {
>>> @@ -1923,7 +1925,7 @@ sub pre_process_file {
>>>  sub process_file {
>>>  	my ($t) = @_;
>>> 
>>> -        pre_process_file($t, $quiet);
>>> +	pre_process_file($t, $quiet);
>>> 
>>>  	my $message_was_sent = send_message();
>>>  	if ($message_was_sent == -1) {
>> 
>> I'll let others comment as the "blank around prompt" smells quite
>> subjective and do not want to be the sole reviewer on it.
>> 
>> Thanks, will queue.
Junio C Hamano April 27, 2024, 5:41 p.m. UTC | #6
Dragan Simic <dsimic@manjaro.org> writes:

> Just checking, is there something I can do to get this patch
> series moving forward?

Get it into a shape that is more palatable to people?  That's easy
to say and hard to define X-<.

I didn't see anybody enthused about the change, and I didn't get
enthused about it, either.
Dragan Simic April 27, 2024, 5:49 p.m. UTC | #7
On 2024-04-27 19:41, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> Just checking, is there something I can do to get this patch
>> series moving forward?
> 
> Get it into a shape that is more palatable to people?  That's easy
> to say and hard to define X-<.
> 
> I didn't see anybody enthused about the change, and I didn't get
> enthused about it, either.

Hmm, I was under impression that the amount of discussion that
already went into the different versions of this series clearly
showed at least some interest in these patches.  We've reached
the sixth iteration, which shows something.
Junio C Hamano April 27, 2024, 6:06 p.m. UTC | #8
Dragan Simic <dsimic@manjaro.org> writes:

> On 2024-04-27 19:41, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> Just checking, is there something I can do to get this patch
>>> series moving forward?
>> Get it into a shape that is more palatable to people?  That's easy
>> to say and hard to define X-<.
>> I didn't see anybody enthused about the change, and I didn't get
>> enthused about it, either.
>
> Hmm, I was under impression that the amount of discussion that
> already went into the different versions of this series clearly
> showed at least some interest in these patches.  We've reached
> the sixth iteration, which shows something.

Well, v1 and v3-v5 saw comments only from me.  There was a comment
on v2 by Kristoffer Haugsbakk about phrasing in proposed log
message.  v6 had Eric's "Meh, either way".

I was not enthused in the topic myself, but was reviewing and
suggesting improvements merely to help you turn the patch into a
shape that would interest others, but the thing is, if the current
output is good enough, it is perfectly normal that there is no more
room to make material improvements, and it is not your fault.


[References]

v1. https://lore.kernel.org/git/62553db377c28458883b66bcdc0c58cc0f32d15b.1712250366.git.dsimic@manjaro.org/
v2. https://lore.kernel.org/git/0e087ed992def0746f3d437253248904c2126464.1712262791.git.dsimic@manjaro.org/
v3. https://lore.kernel.org/git/e3212c0a4ad331685c68c13afcdbced20982ab32.1712364420.git.dsimic@manjaro.org/
v4. https://lore.kernel.org/git/8a9f4927aab96f2f62e2467e59fb6150d7e931fc.1712367983.git.dsimic@manjaro.org/
v5. https://lore.kernel.org/git/cover.1712486910.git.dsimic@manjaro.org/
v6. https://lore.kernel.org/git/cover.1712732383.git.dsimic@manjaro.org/
Junio C Hamano April 27, 2024, 6:18 p.m. UTC | #9
Junio C Hamano <gitster@pobox.com> writes:

> Well, v1 and v3-v5 saw comments only from me.  There was a comment
> on v2 by Kristoffer Haugsbakk about phrasing in proposed log
> message.  v6 had Eric's "Meh, either way".

FWIW, somewhere between v2 and v3 may have been a good place to
stop.

"After calling send_message(), give an extra blank line before the
next message" was extremely easy to see why it was a good idea.
Mixing it with "oh, during the process of a single invocation of
send_message(), there may be other places that want extra blank
lines" made the changes less focused.  Personally I lost interest
in the series at around that point.
Dragan Simic April 28, 2024, 3:03 a.m. UTC | #10
On 2024-04-27 20:18, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Well, v1 and v3-v5 saw comments only from me.  There was a comment
>> on v2 by Kristoffer Haugsbakk about phrasing in proposed log
>> message.  v6 had Eric's "Meh, either way".
> 
> FWIW, somewhere between v2 and v3 may have been a good place to
> stop.
> 
> "After calling send_message(), give an extra blank line before the
> next message" was extremely easy to see why it was a good idea.
> Mixing it with "oh, during the process of a single invocation of
> send_message(), there may be other places that want extra blank
> lines" made the changes less focused.  Personally I lost interest
> in the series at around that point.

Thank you for your additional feedback.

Admittedly, the whole endeavor could have been handled better from
my side, with fewer "oh, let's do it differently and a bit better"
iterations that inevitably made the whole thing harder to follow and
become interested with.

However, I find improving the way "git send-mail" works rather 
important,
because if it bothers me, it must be bothering other people, who may
simply not be on the mailing list.  As we know, a lot of people quite
regularly complain through different channels about sending patches by
email being not user-friendly, which should make any attempts to make
it more user-friendly quite important, if you agree.

I have some more plans for improving "git send-mail" and adding some new
features, but I need to get everything into the final shape I'm pretty
much 100% satisfied with, both from the design and the implementation
standpoints, before sending the patches over.
diff mbox series

Patch

diff --git a/git-send-email.perl b/git-send-email.perl
index f0be4b4560f7..1d6712a44e95 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1361,7 +1361,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 +1509,7 @@  sub gen_header {
 sub send_message {
 	my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
 	my @recipients = @$recipients_ref;
+	my $confirm_shown = 0;
 
 	my @sendmail_parameters = ('-i', @recipients);
 	my $raw_from = $sender;
@@ -1555,6 +1555,7 @@  sub send_message {
 		} elsif (/^a/i) {
 			$confirm = 'never';
 		}
+		$confirm_shown = 1;
 	}
 
 	unshift (@sendmail_parameters, @smtp_server_options);
@@ -1576,7 +1577,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.")
 		}
@@ -1664,9 +1664,11 @@  sub send_message {
 		$smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"), $subject).$smtp->message;
 	}
 	if ($quiet) {
+		print "\n" if ($confirm_shown);
 		printf($dry_run ? __("Dry-Sent %s") : __("Sent %s"), $subject);
 		print "\n";
 	} else {
+		print "\n";
 		print($dry_run ? __("Dry-OK. Log says:") : __("OK. Log says:"));
 		print "\n";
 		if (!defined $sendmail_cmd && !file_name_is_absolute($smtp_server)) {
@@ -1923,7 +1925,7 @@  sub pre_process_file {
 sub process_file {
 	my ($t) = @_;
 
-        pre_process_file($t, $quiet);
+	pre_process_file($t, $quiet);
 
 	my $message_was_sent = send_message();
 	if ($message_was_sent == -1) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 5a771000c9ca..a5cb198a5c5f 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -208,6 +208,7 @@  cat >expected-show-all-headers <<\EOF
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -821,6 +822,7 @@  cat >expected-suppress-sob <<\EOF
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -869,6 +871,7 @@  cat >expected-suppress-sob <<\EOF
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -904,6 +907,7 @@  cat >expected-suppress-cccmd <<\EOF
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
 (body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -940,6 +944,7 @@  test_expect_success $PREREQ 'sendemail.cccmd' '
 test_expect_success $PREREQ 'setup expect' '
 cat >expected-suppress-all <<\EOF
 0001-Second.patch
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -968,6 +973,7 @@  cat >expected-suppress-body <<\EOF
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
 (cc-cmd) Adding cc: cc-cmd@example.com from: './cccmd'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -1003,6 +1009,7 @@  cat >expected-suppress-body-cccmd <<\EOF
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -1036,6 +1043,7 @@  cat >expected-suppress-sob <<\EOF
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -1071,6 +1079,7 @@  cat >expected-suppress-bodycc <<\EOF
 (mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
 (mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
 (body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>
@@ -1105,6 +1114,7 @@  cat >expected-suppress-cc <<\EOF
 0001-Second.patch
 (mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
 (body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
+
 Dry-OK. Log says:
 Server: relay.example.com
 MAIL FROM:<from@example.com>