From: Dragan Simic <dsimic@manjaro.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, code@khaugsbakk.name
Subject: Re: [PATCH v6 2/2] send-email: make it easy to discern the messages for each patch
Date: Sat, 27 Apr 2024 19:27:26 +0200 [thread overview]
Message-ID: <0216a0e8369b8a3592dda90e5680be31@manjaro.org> (raw)
In-Reply-To: <7dcc6f23cc7cb823cb19ec63c69c60e4@manjaro.org>
Hello Junio,
Just checking, is there something I can do to get this patch
series moving forward?
On 2024-04-13 08:27, Dragan Simic wrote:
> On 2024-04-10 18:28, Junio C Hamano wrote:
>> Dragan Simic <dsimic@manjaro.org> writes:
>>
>>> When sending one or multiple patches at once, the displayed result
>>> statuses
>>> for each patch and the "Send this email [y/n/a/...]?" confirmation
>>> prompts
>>> become bunched together with the messages produced for the subsequent
>>> patch,
>>> or with the produced SMTP trace, respectively.
>>>
>>> This makes reading the outputs unnecessarily harder, as visible in a
>>> couple
>>> of excerpts from a sample output below:
>>
>> It is unclear where the boundaries between the messages in the
>> example are, though.
>>
>>> ...
>>> MIME-Version: 1.0
>>> Content-Transfer-Encoding: 8bit
>>>
>>> Result: 250
>>
>> Is this where one message ends, and the next line "OK. Log says:" is
>> the beginning of the next message?
>>
>>> OK. Log says:
>>> Server: smtp.example.com
>>> MAIL FROM:<test@example.com>
>>> ...
>>>
>>> ...
>>> MIME-Version: 1.0
>>> Content-Transfer-Encoding: 8bit
>>
>> Is the above about a single (i.e. the second) message ...
>>
>>> Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>>
>> ... and the user is asked about that message?
>>
>>> OK. Log says:
>>> Server: smtp.example.com
>>> MAIL FROM:<test@example.com>
>>> ...
>
> Huh, I understand your confusion and those are all valid remarks.
> However, my intention was to include excerpts that are long enough
> to illustrate the points to someone already familiar enough with
> the outputs produced by "git send-mail".
>
> If that isn't good enough for the intended audience of the Git
> repository log, I unfortunately see no good way to provide excerpts
> that are long enough to eliminate any doubts. Such excerpts would
> need to be half a dozen screens long, which would turn the patch
> description into a monster.
>
> With all that in mind, perhaps it's the best to simply delete all
> excerpts from the patch description, if you agree?
>
>> And is this about a separate (i.e. the third) message? Without
>> making these clear, it is hard to agree or disagree with the claim
>> that the current presentation is hard to read.
>>
>>> MIME-Version: 1.0
>>> Content-Transfer-Encoding: 8bit
>>>
>>> Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
>>>
>>> OK. Log says:
>>> Server: smtp.example.com
>>> MAIL FROM:<test@example.com>
>>> ...
>>
>> This is obviously in the realm of subjective preference, but I find
>> that the prompt line is distinct enough among all other output that
>> we do not need an extra blank line to locate them.
>
> Basically, I went with a rather simple reasoning: the confirmation
> prompts, just like the SMTP statuses, aren't part of the emitted SMTP
> traces and patch descriptions. They're different kinds of emitted
> messages, if you agree.
>
> Thus, separating the prompts with vertical whitespace is actually
> consistent, and should help with the overall readability, by taking
> the prompts visually out of the other produced messages. In other
> words, it's about keeping different kinds of emitted messages
> separate, with the focus on the SMTP traces and patch descriptions,
> instead of making the prompts locatable.
>
>>> diff --git a/git-send-email.perl b/git-send-email.perl
>>> index f0be4b4560f7..1d6712a44e95 100755
>>> --- a/git-send-email.perl
>>> +++ b/git-send-email.perl
>>> @@ -1361,7 +1361,6 @@ sub smtp_host_string {
>>>
>>> # Returns 1 if authentication succeeded or was not necessary
>>> # (smtp_user was not specified), and 0 otherwise.
>>> -
>>> sub smtp_auth_maybe {
>>> if (!defined $smtp_authuser || $auth || (defined $smtp_auth &&
>>> $smtp_auth eq "none")) {
>>> return 1;
>>> @@ -1510,6 +1509,7 @@ sub gen_header {
>>> sub send_message {
>>> my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline,
>>> $header) = gen_header();
>>> my @recipients = @$recipients_ref;
>>> + my $confirm_shown = 0;
>>>
>>> my @sendmail_parameters = ('-i', @recipients);
>>> my $raw_from = $sender;
>>> @@ -1555,6 +1555,7 @@ sub send_message {
>>> } elsif (/^a/i) {
>>> $confirm = 'never';
>>> }
>>> + $confirm_shown = 1;
>>> }
>>>
>>> unshift (@sendmail_parameters, @smtp_server_options);
>>> @@ -1576,7 +1577,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.")
>>> }
>>> @@ -1664,9 +1664,11 @@ sub send_message {
>>> $smtp->code =~ /250|200/ or die sprintf(__("Failed to send %s\n"),
>>> $subject).$smtp->message;
>>> }
>>> if ($quiet) {
>>> + print "\n" if ($confirm_shown);
>>> printf($dry_run ? __("Dry-Sent %s") : __("Sent %s"), $subject);
>>> print "\n";
>>> } else {
>>> + print "\n";
>>> print($dry_run ? __("Dry-OK. Log says:") : __("OK. Log says:"));
>>> print "\n";
>>> if (!defined $sendmail_cmd &&
>>> !file_name_is_absolute($smtp_server)) {
>>> @@ -1923,7 +1925,7 @@ sub pre_process_file {
>>> sub process_file {
>>> my ($t) = @_;
>>>
>>> - pre_process_file($t, $quiet);
>>> + pre_process_file($t, $quiet);
>>>
>>> my $message_was_sent = send_message();
>>> if ($message_was_sent == -1) {
>>
>> I'll let others comment as the "blank around prompt" smells quite
>> subjective and do not want to be the sole reviewer on it.
>>
>> Thanks, will queue.
next prev parent reply other threads:[~2024-04-27 17:27 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-10 7:01 [PATCH v6 0/2] send-email: make produced outputs more readable Dragan Simic
2024-04-10 7:01 ` [PATCH v6 1/2] send-email: move newline characters out of a few translatable strings Dragan Simic
2024-04-10 16:12 ` Junio C Hamano
2024-04-13 6:12 ` Dragan Simic
2024-04-10 7:01 ` [PATCH v6 2/2] send-email: make it easy to discern the messages for each patch Dragan Simic
2024-04-10 16:28 ` Junio C Hamano
2024-04-10 22:59 ` Eric Sunshine
2024-04-13 6:10 ` Dragan Simic
2024-04-13 6:27 ` Dragan Simic
2024-04-27 17:27 ` Dragan Simic [this message]
2024-04-27 17:41 ` Junio C Hamano
2024-04-27 17:49 ` Dragan Simic
2024-04-27 18:06 ` Junio C Hamano
2024-04-27 18:18 ` Junio C Hamano
2024-04-28 3:03 ` Dragan Simic
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0216a0e8369b8a3592dda90e5680be31@manjaro.org \
--to=dsimic@manjaro.org \
--cc=code@khaugsbakk.name \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).