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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-04-05 00:52, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>  		if ($smtp) {
>>  			print __("Result: "), $smtp->code, ' ',
>> -				($smtp->message =~ /\n([^\n]+\n)$/s), "\n";
>> +				($smtp->message =~ /\n([^\n]+\n)$/s);
>>  		} else {
>> -			print __("Result: OK\n");
>> +			print __("Result: OK");
>>  		}
>> +		# Make it easy to discern the messages for each patch
> 
> I do not think we want this comment.
> 
> Before printing the "Result: OK" line, we do not give an obvious and
> pointless comment e.g., "# Report success".  What this comment says
> something similarly obvious.  Both choices in the preceding if/else
> emit an incomplete line, hence it is clear that we need to terminate
> the line here, and this is the last line of output about the message
> we just processed.

Hmm, I thought that including this comment might actually be helpful
to someone reading the code later, but with your explanation above,
I agree that it doesn't fit very well.

>>  	}
>> 
>>  	return 1;
> 
> You didn't ran t9001 before sending this version (or any of the
> previous rounds) out, did you?  Among ~200 tests 10 of them now fail
> with this patch applied.

My bad, sorry.  I totally missed that test. :(

> Do we know when we are sending either the first or the last message
> of a sequence at this point in the code?  It would be superb if you
> can make this extra blank line a separator, not a terminator [*], as
> there needs no extra blank line after emitting the last message.
> 
>     [Side note]
> 
>      * When showing a list of 3 things A B C, you can separate them by
>        inserting a gap between A and B, and another gap between B and 
> C,
>        and you are using the "separator" (this is similar to how "git
>        log --pretty=format:..." works).  But you can be lazy and 
> instead
>        append a gap after each element, in which case you are using the
>        "terminator" (similar to how "git log --pretty=tformat:..."
>        works).

Well, I actually wasn't lazy, but instead I wanted to give this patch
better chances to be accepted, by introducing as a small amount of
changes to the code as possible.  This was a lesson not to take that
route, but to do "the right thing" instead.

> But it is harder to do separator correctly if the output is
> conditional (e.g., you have 5 input messages, but you may somehow
> skip some messages depending on conditions---now your code to decide
> if you emit an extra newline needs to take it into account.  After
> sending the 4th message and showing an extra newline, because you
> expect that there is another message to be sent and it needs a gap
> before, you may realize that some logic causes you to drop 5th and
> final message but then it is too late for you to take that extra
> blank line back), and obviously a buggy separator implementation is
> worse than a terminator implementation.

Yup, it's tricky to do it right.

> Here is my attempt on top of your patch to implement the separator
> semantics.  After showing a message, we remember that fact, and
> before showing the next message, we emit an extra blank line.  With
> it, all tests in t9001 pass, but you may want to double check how
> different ways to leave send_message() would affect the output.  I
> just randomly decided that there needs no extra blank line before
> emitting the message that was edited and that is why in the
> following patch, I assign 0 to $want_separator_before_send_message
> in that case, but it may not be the right choice (I never "edit"
> inside send-email myself, so I would be a wrong person to decide
> without second opinion), for example.
> 
> 
>  git-send-email.perl | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git c/git-send-email.perl w/git-send-email.perl
> index ac0c691d3a..d4bbc73f1f 100755
> --- c/git-send-email.perl
> +++ w/git-send-email.perl
> @@ -1689,8 +1689,7 @@ sub send_message {
>  		} else {
>  			print __("Result: OK");
>  		}
> -		# Make it easy to discern the messages for each patch
> -		print "\n\n";
> +		print "\n";
>  	}
> 
>  	return 1;
> @@ -1918,16 +1917,21 @@ sub pre_process_file {
>  # Prepares the email, prompts the user, and sends it out
>  # Returns 0 if an edit was done and the function should be called 
> again, or 1
>  # on the email being successfully sent out.
> +my $want_separator_before_send_message = 0;
> +
>  sub process_file {
>  	my ($t) = @_;
> 
>          pre_process_file($t, $quiet);
> 
> +	print "\n" if ($want_separator_before_send_message);
>  	my $message_was_sent = send_message();
>  	if ($message_was_sent == -1) {
>  		do_edit($t);
> +		$want_separator_before_send_message = 0;
>  		return 0;
>  	}
> +	$want_separator_before_send_message = $message_was_sent;
> 
>  	# set up for the next message
>  	if ($thread) {

Thanks for this patch, I'll go through it, and make sure that it works
as expected.  I'll see also to correct the current lack of vertical
separation between the "send the message [y/n/q/a/...]" prompt and the
subsequent output, which also unnecessarily makes the output a bit hard
to parse by humans.

There's also seemingly something wrong with the typeface highlighting
selected for the above-mentioned prompt.  In my environment, it ends up
being displayed as underlined bold text, which looks a bit ugly and,
more importantly, a bit hard on the eyes.