From: Gregory Anders <greg@gpanders.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] git-send-email: add sendmailCommand option
Date: Wed, 12 May 2021 07:12:17 -0600 [thread overview]
Message-ID: <YJvUMTAVKJqPZF2t@gpanders.com> (raw)
In-Reply-To: <609b8a5a65826_6e0fc2084c@natae.notmuch>
On Wed, 12 May 2021 02:57 -0500, Felipe Contreras wrote:
>Gregory Anders wrote:
>I'm not against these kinds of changes but it took me one minute to
>figure out all you did was change the format.
>
>This belongs in a separate patch.
Yes this was pointed out by the other reviewers as well, I'll omit it in
subsequent revisions.
>
>> +--sendmail-cmd=<command>::
>
>Oh no no no. Don't do shortcuts.
>
>If you think --sendmail-command is too long, then address that problem
>head on, don't try to hide it.
>
>I do think it's too long, which is why I suggested --command (especially
>since it's obvious which command we are talking about), but I wouldn't
>suggest --sdm-command, or something of that sort. We have to own our
>decisions.
>
> 1. --command
> 2. --sendmail
> 3. --sendmail-command
>
>We have to pick one. I suggest #1.
>
>To try to make #3 shorter is just shoving the problem under the rug.
The intention behind `--sendmail-cmd` was consistency with `--to-cmd`
and `--cc-cmd`. Though both of those options also use the condensed
'cmd' form in their configuration options as well, so I suppose I should
also change this one to 'sendemail.sendmailcmd'.
I'm not opposed to just '--sendmail' and 'sendemail.sendmail' either. I
personally believe --sendmail-cmd is the clearest, even if it's verbose,
but I'll concede to whatever consensus we arrive at.
>
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -70,6 +70,7 @@ sub usage {
>>
>> Sending:
>> --envelope-sender <str> * Email envelope sender.
>> + --sendmail-cmd <str> * Shell command to run to send email.
>> --smtp-server <str:int> * Outgoing SMTP server to use. The port
>> is optional. Default 'localhost'.
>> --smtp-server-option <str> * Outgoing SMTP server option to use.
>> @@ -252,6 +253,7 @@ sub do_edit {
>> my (@suppress_cc);
>> my ($auto_8bit_encoding);
>> my ($compose_encoding);
>> +my ($sendmail_command);
>> # Variables with corresponding config settings & hardcoded defaults
>> my ($debug_net_smtp) = 0; # Net::SMTP, see send_message()
>> my $thread = 1;
>> @@ -299,6 +301,7 @@ sub do_edit {
>> "assume8bitencoding" => \$auto_8bit_encoding,
>> "composeencoding" => \$compose_encoding,
>> "transferencoding" => \$target_xfer_encoding,
>> + "sendmailcommand" => \$sendmail_command,
>> );
>>
>> my %config_path_settings = (
>> @@ -432,6 +435,7 @@ sub read_config {
>> "no-bcc" => \$no_bcc,
>> "chain-reply-to!" => \$chain_reply_to,
>> "no-chain-reply-to" => sub {$chain_reply_to = 0},
>> + "sendmail-cmd=s" => \$sendmail_command,
>
>Isn't it interesting that to make the code readable you picked
>$sendmail_command, but you don't want users to type so much, even if
>it's more readable?
See my note above.
>
>> --- a/t/t9001-send-email.sh
>> +++ b/t/t9001-send-email.sh
>> @@ -57,7 +57,7 @@ test_no_confirm () {
>> git send-email \
>> --from="Example <from@example.com>" \
>> --to=nobody@example.com \
>> - --smtp-server="$(pwd)/fake.sendmail" \
>> + --sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
>
>People are already using --smpt-server=$cmd, we need to keep testing
>that.
>
>Yes, eventually we would want them to move to --sendmail-cmd (or
>--command, or whatever), but that won't happen tomorrow. Therefore our
>primary tests need to be focused on --smtp-server.
>
>We need new *additional* tests for --sendmail-cmd, but those should not
>override the current tests. At least not right now.
I will add a test case for the absolute path form of --smtp-server;
however, if we are introducing an option for specifying a sendmail-like
command, surely that is the one to use when using "fake.sendmail", no?
If we leave the test cases as-is for now, we introduce a split that
someone will eventually need to come back and update anyway. Instead of
kicking that can down the road, I thought it best to go ahead and do it
now.
Thanks for your feedback.
Greg
next prev parent reply other threads:[~2021-05-12 13:12 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-12 3:30 [PATCH] git-send-email: add sendmailCommand option Gregory Anders
2021-05-12 4:19 ` Junio C Hamano
2021-05-12 13:03 ` Gregory Anders
2021-05-12 7:57 ` Felipe Contreras
2021-05-12 13:12 ` Gregory Anders [this message]
2021-05-12 17:21 ` Felipe Contreras
2021-05-12 18:06 ` Gregory Anders
2021-05-12 19:32 ` Felipe Contreras
2021-05-12 9:04 ` Ævar Arnfjörð Bjarmason
2021-05-12 13:18 ` Gregory Anders
2021-05-13 2:32 ` [PATCH v2] git-send-email: add option to specify sendmail command Gregory Anders
2021-05-13 3:58 ` Junio C Hamano
2021-05-13 13:31 ` Gregory Anders
2021-05-13 21:21 ` Junio C Hamano
2021-05-13 15:23 ` [PATCH v3] " Gregory Anders
2021-05-14 4:25 ` Junio C Hamano
2021-05-14 5:16 ` Junio C Hamano
2021-05-14 14:12 ` Gregory Anders
2021-05-14 15:15 ` [PATCH v4] " Gregory Anders
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YJvUMTAVKJqPZF2t@gpanders.com \
--to=greg@gpanders.com \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
/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.
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 NNTP newsgroup(s).