Re: [PATCH] git-send-email: add sendmailCommand option

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.

quoted

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

quoted

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

quoted

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