From: Dragan Simic <dsimic@manjaro.org>
Date: Tue, 09 Apr 2024 03:37:43 GMT
Subject: Re: [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch
Message-ID: <2b24b9e8f0fcb4f6df595c1b0be06359@manjaro.org>
In-Reply-To: <xmqq7ch7a7gt.fsf@gitster.g>

On 2024-04-08 23:08, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> ...  To make the produced outputs more readable, add vertical
>> whitespace (more precisely, a newline) between the displayed result 
>> statuses
>> and the subsequent messages, as visible in ...
> 
> The above feels a bit roundabout way to say "the logic is that we
> need to add a gap before showing the next message, if we did things
> that cause the smtp traces to be shown", but OK.

Yeah, the wording I used isn't perfect, but I think it's still
understandable.  I'll see to possibly improve it in the v6.

>> 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 between the groups of produced messages, not 
>> as
>> their terminator.  This follows the Git's general approach of not 
>> wasting
>> the vertical screen space whenever reasonably possible.
> 
> I do not see this paragraph is relevant to the target audience.  It
> may be a good advice to give to a reader who attempts to solve the
> problem this patch solved themselves, botches the attempt and ends
> up with a code with the terminator semantics.  But for other readers
> of "git log" and reviewers of the patch, "I did not make a silly
> mistake, and instead correctly chose to use the separator semantics"
> is not something worth boasting about.

Makes sense, will delete that paragraph in the v6.

>> 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.
> 
> These are worth recording.

Thanks.

>> @@ -1554,7 +1554,10 @@ sub send_message {
>>  			exit(0);
>>  		} elsif (/^a/i) {
>>  			$confirm = 'never';
>> +			$needs_separator = 1;
>>  		}
>> +	} else {
>> +		$needs_separator = 1;
>>  	}
> 
> If you do not add this "else" clause to the outer "are we doing
> confirmation?" if statement, and instead just set $needs_separator
> *after* it, it would make it even more obvious what is going on.
> The codeflow would become
> 
> 	sub send_message {
> 		do bunch of things that do not yet send e-mail
> 	        and possibly return or die
> 
> 		$needs_separator = 1;
> 
> 		do things that cause the smtp exchange and trace
> 		to be emitted
> 	}
> 
> That makes it obvious that the purpose of $needs_separator is to
> record the fact that "this" message has already been sent and we
> need to add a "gap" before attempting to send the "next" message.

Very good point, thanks!  I've somehow managed to miss that while
iterating through a few code variants and the associated testing.
Will be improved in the v6.

> Other than the above points, very well done.

Thank you! :)

>>  	unshift (@sendmail_parameters, @smtp_server_options);
>> @@ -1576,7 +1579,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.")
>>  		}
>> @@ -1921,7 +1923,8 @@ sub pre_process_file {
>>  sub process_file {
>>  	my ($t) = @_;
>> 
>> -        pre_process_file($t, $quiet);
>> +	pre_process_file($t, $quiet);
> 
> nice ;-)

It had to be fixed, IMHO. :)

>> +	print "\n" if ($needs_separator);
>> 
>>  	my $message_was_sent = send_message();
>>  	if ($message_was_sent == -1) {