git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH RESEND] send-email: make produced outputs more readable

From
DSDragan Simic <dsimic@manjaro.org>
Date
Apr 4, 2024, 19:40 UTC
Message-ID
<44ed7af52e2968993d9b2b99faec1f64@manjaro.org>
In-Reply-To
<xmqqy19tylrm.fsf@gitster.g>
Hello Junio,
On 2024-04-04 21:23, Junio C Hamano wrote:
Show 21 quoted lines
> 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.
Show 52 quoted lines
>> 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.

Previous: Junio C Hamano
Message 3 of 3 in “send-email: make produced outputs more readable”
  1. send-email: make produced outputs more readableDragan Simic, Apr 4, 2024
  2. Junio C HamanoApr 4, 2024
  3. Dragan SimicApr 4, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.