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

> 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.

>>  	}
>> 
>>  	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.

> 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).

> 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.

> 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) {