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

[Dragan Simic <dsimic@manjaro.org>]

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.