RE: [PATCH] git-send-email: add sendmailCommand option
From: Felipe Contreras <hidden>
Date: 2021-05-12 07:57:23
Gregory Anders wrote:
quoted hunk ↗ jump to hunk
--- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt@@ -159,13 +159,23 @@ Sending ~~~~~~~ --envelope-sender=<address>:: - Specify the envelope sender used to send the emails. - This is useful if your default address is not the address that is - subscribed to a list. In order to use the 'From' address, set the - value to "auto". If you use the sendmail binary, you must have - suitable privileges for the -f parameter. Default is the value of the - `sendemail.envelopeSender` configuration variable; if that is - unspecified, choosing the envelope sender is left to your MTA. + Specify the envelope sender used to send the emails. This is + useful if your default address is not the address that is + subscribed to a list. In order to use the 'From' address, set + the value to "auto". If you use the sendmail binary, you must + have suitable privileges for the -f parameter. Default is the + value of the `sendemail.envelopeSender` configuration variable; + if that is unspecified, choosing the envelope sender is left to + your MTA.
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.
+--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.
quoted hunk ↗ jump to hunk
--- 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? Once again: "$command=s" -> \$command,
quoted hunk ↗ jump to hunk
--- 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. Cheers. -- Felipe Contreras