Re: [PATCH v5 2/3] send-email: make it easy to discern the messages for each patch
- From
- Dragan Simic <dsimic@manjaro.org>
- Date
- Apr 9, 2024, 03:37 UTC
- Message-ID
- <2b24b9e8f0fcb4f6df595c1b0be06359@manjaro.org>
- In-Reply-To
- <xmqq7ch7a7gt.fsf@gitster.g>
On 2024-04-08 23:08, Junio C Hamano wrote:
Show 10 quoted lines
> 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.
Show 17 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.
Makes sense, will delete that paragraph in the v6.
Show 8 quoted lines
>> 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.
Show 28 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.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! :)
Show 17 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 ;-)It had to be fixed, IMHO. :)
>> + print "\n" if ($needs_separator);
>>
>> my $message_was_sent = send_message();
>> if ($message_was_sent == -1) {