Re: [PATCH] git-send-email: add sendmailCommand option
- From
Gregory Anders <greg@gpanders.com>
- Date
- May 12, 2021, 13:12 UTC
- Message-ID
- <YJvUMTAVKJqPZF2t@gpanders.com>
- In-Reply-To
- <609b8a5a65826_6e0fc2084c@natae.notmuch>
On Wed, 12 May 2021 02:57 -0500, Felipe Contreras wrote:
Show 5 quoted lines
>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.
Show 20 quoted lines
> >> +--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.
Show 36 quoted lines
>
>> --- 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.
Show 19 quoted lines
>
>> --- 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