From: Dragan Simic <dsimic@manjaro.org>
Date: Thu, 04 Apr 2024 19:40:32 GMT
Subject: Re: [PATCH RESEND] send-email: make produced outputs more readable
Message-ID: <44ed7af52e2968993d9b2b99faec1f64@manjaro.org>
In-Reply-To: <xmqqy19tylrm.fsf@gitster.g>

Hello Junio,

On 2024-04-04 21:23, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> Notes:
>>      * send-email: make produced outputs more readable by separating
>>        the result statuses from the subsequent patch outputs
>> 
>>     This is a resubmission of the patch I submitted about a week and a 
>> half
>>     ago. [1]  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.
> 
> I tried to cram a bit more information than "output more readable"
> that lacks in what way the result is easier to read.
> 
>     send-email: make boundaries between messages easier to spot
> 
> perhaps?

Looking good to me, will use that as the patch subject in v2.

>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 821b2b3a135a..62505ab2707c 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -1576,7 +1576,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.")
>>  		}
>> @@ -1686,9 +1685,9 @@ 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), "\n\n";
>>  		} else {
>> -			print __("Result: OK\n");
>> +			print __("Result: OK\n\n");
>>  		}
> 
> It would be nicer to instead add a single separate
> 
> 		print "\n";
> 
> after these if/else alternatives, without touching the existing
> message lines, I would think.  That way, existing message
> translations do not have to change.
> 
> If we were to change the translatable string anyway, it would be
> even better to remove the newline from the translatable part of the
> message, rendering the thing to:
> 
> 	if ($smtp) {
> 		print __("Result: "), ..., ($smtp->message =~ /.../);
>   	} else {
> 		print __("Result: OK");
> 	}
> 	print "\n\n";
> 
> Strictly speaking, that is an orthogonal clean-up, so it may have to
> make it into two patch series, one for preliminary clean-up "to
> excise terminating newline out of translatable strings" patch that
> adds a separate print that adds a single newline, plus the "make it
> easier to spot where a message ends and another one starts" patch
> that makes the new print statement that adds a single newline to
> instead add two.  In a patch as simple as this one, however, I think
> killing two birds with a stone, i.e., directly go to the "if we were
> to change the translatable string anyway" final shape in a single
> patch, would be fine.

Thank you very much for such a detailed review!  I also think that
putting everything into a single patch is a better option, because
the changes are rather small.  Will do that in v2.