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@xxxxxxxxxxx>" \
--to=nobody@xxxxxxxxxxx \
- --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
