Re: [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2024, 21:08 UTC
Message-ID: <xmqq7ch7a7gt.fsf@gitster.g>
In-Reply-To: <7f87383089011a98b0347d885b3b9d76cfddb91d.1712486910.git.dsimic@manjaro.org>

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.

Show 5 quoted lines

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

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

Show 9 quoted lines

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

Other than the above points, very well done.

Show 15 quoted lines

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

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