| Message ID | 20210513152329.22578-1-greg@gpanders.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] git-send-email: add option to specify sendmail command | expand |
Gregory Anders <greg@gpanders.com> writes: > Use a block scoped variable to print the sendmail invocation at the end > of the 'send_message' subroutine. Assigning directly to $sendmail_cmd > (as in the v2 patch) causes some bizarre problems; namely, it seems to > affect the value of $sendmail_cmd that is read at earlier points in the > same subroutine, which causes test invocations of the form > > git send-email --smtp-server="$(pwd)/fake.sendmail" > > to fail. The value passed to --smtp-server was assigned to $sendmail_cmd > at the end of the 'send_message' subprocedure, but somehow this caused > the 'if (defined $sendmail_cmd)' condition earlier in the subproc to > evaluate to true. Are you talking about the use of $sm that is local to the debug output? I think leaving $sendmail_cmd intact by using a separate variable is the right choice. Isn't the problem you observed a consequence of send_message() getting called once for each message, so assigning to $sendmail_cmd in the function for the first invocation of the function would change its value for the second invocation? Also, if we have been using --smtp-server=$(pwd)/fake.sendmail we cannot expect to use the same value like this: --sendmail-cmd=$(pwd)/fake.sendmail because we deliberately add a space in the $(pwd) by choosing the name of the test directory to be "trash directory.something". We'd need to do something like --sendmail-cmd='$(pwd)/fake.sendmail' so that the shell sees '$(pwd)/fake.sendmail' literally and runs pwd to find out what the path to the program is, I would think. > diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh > index 65b3035371..583fbba410 100755 > --- a/t/t9001-send-email.sh > +++ b/t/t9001-send-email.sh > @@ -2148,6 +2148,37 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' > test_cmp expected-list actual-list > ' > > +test_expect_success $PREREQ 'test using command name with --sendmail-cmd' ' > + clean_fake_sendmail && > + PATH="$(pwd):$PATH" \ > + git send-email \ > + --from="Example <nobody@example.com>" \ > + --to=nobody@example.com \ > + --sendmail-cmd="fake.sendmail" \ > + HEAD^ && > + test_path_is_file commandline1 > +' Nice demonstration of the "we no longer need an absolute pathname" feature. > +test_expect_success $PREREQ 'test using arguments with --sendmail-cmd' ' > + clean_fake_sendmail && > + git send-email \ > + --from="Example <nobody@example.com>" \ > + --to=nobody@example.com \ > + --sendmail-cmd="\"$(pwd)/fake.sendmail\" -f nobody@example.com" \ > + HEAD^ && > + test_path_is_file commandline1 > +' Hmph, if $(pwd) has a double quote character in it, this may not work as expected, as the shell that is expanding the command line arguments for "git send-email" would see $(pwd), expand it and our program will see "/path/with/d"quote/git/t/trash directory.9001/fake.sendmail" -f nobody@e.c as the value of --sendmail-cmd, which would not interpolate well, no? We want the shell that eats the command line of 'git send-email' to see --sendmail-cmd='$(pwd)/fake.sendmail'\" -f nobody@example.com" and because this is inside a sq pair, it would become --sendmail-cmd='\''$(pwd)/fake.sendmail'\''\" -f nobody@example.com" after we replace each sq with '\'', or something like that, perhaps? > +test_expect_success $PREREQ 'test shell expression with --sendmail-cmd' ' > + clean_fake_sendmail && > + git send-email \ > + --from="Example <nobody@example.com>" \ > + --to=nobody@example.com \ > + --sendmail-cmd="f() { \"$(pwd)/fake.sendmail\" \"\$@\"; };f" \ > + HEAD^ && > + test_path_is_file commandline1 > +' Nice demonstration of how a bit of scripting can be used. > test_expect_success $PREREQ 'invoke hook' ' > mkdir -p .git/hooks && Thanks.
Junio C Hamano <gitster@pobox.com> writes: > We want the shell that eats the command line of 'git send-email' to see > > --sendmail-cmd='$(pwd)/fake.sendmail'\" -f nobody@example.com" Eh, sorry, but this is wrong. It would have to be something like --sendmail-cmd='"$(pwd)/fake.sendmail" -f nobody@example.com' The point is that the outer shell (i.e. the one that is eval'ing the body of the test_expect_success) should just see and treat the path to the sendmail-like program as "$(pwd)/fake.sendmail" as a literal string including the surrounding double quotes and pass it down to "git send-email", and the shell started by our "exec('sh','-c',...)" thing will see what $(pwd) expands to, appends /fake.sendmail to it, and treat the whole thing as a single "path to the program", that is followed by two args, i.e. '-f' and 'nobody@example.com'. And inside test_expect_success whose body is surrounded by a pair of sq, we'd express a sq as '\'', so it becomes --sendmail-cmd='\''"$(pwd)/fake.sendmail" -f nobody@example.com'\'' in the test script, I would think.
On Fri, 14 May 2021 13:25 +0900, Junio C Hamano wrote: >Are you talking about the use of $sm that is local to the debug >output? I think leaving $sendmail_cmd intact by using a separate >variable is the right choice. Isn't the problem you observed a >consequence of send_message() getting called once for each message, >so assigning to $sendmail_cmd in the function for the first >invocation of the function would change its value for the second >invocation? Yes that is right. That makes sense. I didn't realize the subprocess was called twice, though that is such an obvious explanation I don't know why I didn't think of it. >Also, if we have been using > > --smtp-server=$(pwd)/fake.sendmail > >we cannot expect to use the same value like this: > > --sendmail-cmd=$(pwd)/fake.sendmail > >because we deliberately add a space in the $(pwd) by choosing the >name of the test directory to be "trash directory.something". We'd >need to do something like > > --sendmail-cmd='$(pwd)/fake.sendmail' > >so that the shell sees '$(pwd)/fake.sendmail' literally and runs pwd >to find out what the path to the program is, I would think. Indeed, in prior versions of this patch I had replaced the uses of `--smtp-server` in the test suite with `--sendmail-cmd` which included those extra quotes (I reverted back to using --smtp-server after feedback from other reviewers in lieu of simply adding new test cases for --sendmail-cmd). >> +test_expect_success $PREREQ 'test using arguments with >> --sendmail-cmd' ' >> + clean_fake_sendmail && >> + git send-email \ >> + --from="Example <nobody@example.com>" \ >> + --to=nobody@example.com \ >> + --sendmail-cmd="\"$(pwd)/fake.sendmail\" -f nobody@example.com" \ >> + HEAD^ && >> + test_path_is_file commandline1 >> +' > >Hmph, if $(pwd) has a double quote character in it, this may not >work as expected, as the shell that is expanding the command line >arguments for "git send-email" would see $(pwd), expand it and our >program will see > > "/path/with/d"quote/git/t/trash directory.9001/fake.sendmail" -f nobody@e.c > >as the value of --sendmail-cmd, which would not interpolate well, >no? > >We want the shell that eats the command line of 'git send-email' to see > > --sendmail-cmd='$(pwd)/fake.sendmail'\" -f nobody@example.com" > >and because this is inside a sq pair, it would become > > --sendmail-cmd='\''$(pwd)/fake.sendmail'\''\" -f nobody@example.com" > >after we replace each sq with '\'', or something like that, perhaps? I'll take a look at this (as well as your followup email) and send a new version. Thanks, Greg
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 93708aefea..3db4eab4ba 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -167,6 +167,14 @@ Sending `sendemail.envelopeSender` configuration variable; if that is unspecified, choosing the envelope sender is left to your MTA. +--sendmail-cmd=<command>:: + Specify a command to run to send the email. The command should + be sendmail-like; specifically, it must support the `-i` option. + The command will be executed in the shell if necessary. Default + is the value of `sendemail.sendmailcmd`. If unspecified, and if + --smtp-server is also unspecified, git-send-email will search + for `sendmail` in `/usr/sbin`, `/usr/lib` and $PATH. + --smtp-encryption=<encryption>:: Specify the encryption to use, either 'ssl' or 'tls'. Any other value reverts to plain SMTP. Default is the value of @@ -211,13 +219,16 @@ a password is obtained using 'git-credential'. --smtp-server=<host>:: If set, specifies the outgoing SMTP server to use (e.g. - `smtp.example.com` or a raw IP address). Alternatively it can - specify a full pathname of a sendmail-like program instead; - the program must support the `-i` option. Default value can - be specified by the `sendemail.smtpServer` configuration - option; the built-in default is to search for `sendmail` in - `/usr/sbin`, `/usr/lib` and $PATH if such program is - available, falling back to `localhost` otherwise. + `smtp.example.com` or a raw IP address). If unspecified, and if + `--sendmail-cmd` is also unspecified, the default is to search + for `sendmail` in `/usr/sbin`, `/usr/lib` and $PATH if such a + program is available, falling back to `localhost` otherwise. ++ +For backward compatibility, this option can also specify a full pathname +of a sendmail-like program instead; the program must support the `-i` +option. This method does not support passing arguments or using plain +command names. For those use cases, consider using `--sendmail-cmd` +instead. --smtp-server-port=<port>:: Specifies a port different from the default port (SMTP diff --git a/git-send-email.perl b/git-send-email.perl index 175da07d94..dbb144aa11 100755 --- 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> * 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_cmd); # 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, + "sendmailcmd" => \$sendmail_cmd, ); 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_cmd, "smtp-server=s" => \$smtp_server, "smtp-server-option=s" => \@smtp_server_options, "smtp-server-port=s" => \$smtp_server_port, @@ -1003,16 +1007,19 @@ sub expand_one_alias { $reply_to = sanitize_address($reply_to); } -if (!defined $smtp_server) { +if (!defined $sendmail_cmd && !defined $smtp_server) { my @sendmail_paths = qw( /usr/sbin/sendmail /usr/lib/sendmail ); push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH}; foreach (@sendmail_paths) { if (-x $_) { - $smtp_server = $_; + $sendmail_cmd = $_; last; } } - $smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug* + + if (!defined $sendmail_cmd) { + $smtp_server = 'localhost'; # could be 127.0.0.1, too... *shrug* + } } if ($compose && $compose > 0) { @@ -1492,11 +1499,17 @@ sub send_message { if ($dry_run) { # We don't want to send the email. - } elsif (file_name_is_absolute($smtp_server)) { + } elsif (defined $sendmail_cmd || file_name_is_absolute($smtp_server)) { my $pid = open my $sm, '|-'; defined $pid or die $!; if (!$pid) { - exec($smtp_server, @sendmail_parameters) or die $!; + if (defined $sendmail_cmd) { + exec ("sh", "-c", "$sendmail_cmd \"\$@\"", "-", @sendmail_parameters) + or die $!; + } else { + exec ($smtp_server, @sendmail_parameters) + or die $!; + } } print $sm "$header\n$message"; close $sm or die $!; @@ -1592,14 +1605,21 @@ sub send_message { printf($dry_run ? __("Dry-Sent %s\n") : __("Sent %s\n"), $subject); } else { print($dry_run ? __("Dry-OK. Log says:\n") : __("OK. Log says:\n")); - if (!file_name_is_absolute($smtp_server)) { + if (!defined $sendmail_cmd && !file_name_is_absolute($smtp_server)) { print "Server: $smtp_server\n"; print "MAIL FROM:<$raw_from>\n"; foreach my $entry (@recipients) { print "RCPT TO:<$entry>\n"; } } else { - print "Sendmail: $smtp_server ".join(' ',@sendmail_parameters)."\n"; + my $sm; + if (defined $sendmail_cmd) { + $sm = $sendmail_cmd; + } else { + $sm = $smtp_server; + } + + print "Sendmail: $sm ".join(' ',@sendmail_parameters)."\n"; } print $header, "\n"; if ($smtp) { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index 65b3035371..583fbba410 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -2148,6 +2148,37 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' ' test_cmp expected-list actual-list ' +test_expect_success $PREREQ 'test using command name with --sendmail-cmd' ' + clean_fake_sendmail && + PATH="$(pwd):$PATH" \ + git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --sendmail-cmd="fake.sendmail" \ + HEAD^ && + test_path_is_file commandline1 +' + +test_expect_success $PREREQ 'test using arguments with --sendmail-cmd' ' + clean_fake_sendmail && + git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --sendmail-cmd="\"$(pwd)/fake.sendmail\" -f nobody@example.com" \ + HEAD^ && + test_path_is_file commandline1 +' + +test_expect_success $PREREQ 'test shell expression with --sendmail-cmd' ' + clean_fake_sendmail && + git send-email \ + --from="Example <nobody@example.com>" \ + --to=nobody@example.com \ + --sendmail-cmd="f() { \"$(pwd)/fake.sendmail\" \"\$@\"; };f" \ + HEAD^ && + test_path_is_file commandline1 +' + test_expect_success $PREREQ 'invoke hook' ' mkdir -p .git/hooks &&
The sendemail.smtpServer configuration option and --smtp-server command line option both support using a sendmail-like program to send emails by specifying an absolute file path. However, this is not ideal for the following reasons: 1. It overloads the meaning of smtpServer (now a program is being used for the server?) 2. It doesn't allow for non-absolute paths, arguments, or arbitrary scripting Requiring an absolute path is bad for portability, as the same program may be in different locations on different systems. If a user wishes to pass arguments to their program, they have to use the smtpServerOption option, which is cumbersome (as it must be repeated for each option) and doesn't adhere to normal git conventions. Introduce a new configuration option sendemail.sendmailCmd as well as a command line option --sendmail-cmd that can be used to specify a command (with or without arguments) or shell expression to run to send email. The name of this option is consistent with --to-cmd and --cc-cmd. This invocation honors the user's $PATH so that absolute paths are not necessary. Arbitrary shell expressions are also supported, allowing users to do basic scripting. Give this option a higher precedence over --smtp-server and sendemail.smtpServer, as the new interface is more flexible. For backward compatibility, continue to support absolute paths in --smtp-server and sendemail.smtpServer. Signed-off-by: Gregory Anders <greg@gpanders.com> --- Fix and reword the documentation to hopefully be more consistent between --sendmail-cmd and --smtp-server. Update the invocation of "sendmail_cmd" to ensure that parameters are not expanded by the shell (the parameters may contain any number of special characters that are not intended to be interpreted by the shell). Use a block scoped variable to print the sendmail invocation at the end of the 'send_message' subroutine. Assigning directly to $sendmail_cmd (as in the v2 patch) causes some bizarre problems; namely, it seems to affect the value of $sendmail_cmd that is read at earlier points in the same subroutine, which causes test invocations of the form git send-email --smtp-server="$(pwd)/fake.sendmail" to fail. The value passed to --smtp-server was assigned to $sendmail_cmd at the end of the 'send_message' subprocedure, but somehow this caused the 'if (defined $sendmail_cmd)' condition earlier in the subproc to evaluate to true. This would fail because $sendmail_cmd is expanded in the shell, and since $(pwd) contains a space the command was being split, resulting in a "command not found" error. I don't have enough Perl experience to explain what's happening in this case, but using a scoped variable resolves the issue. Documentation/git-send-email.txt | 25 ++++++++++++++++------- git-send-email.perl | 34 +++++++++++++++++++++++++------- t/t9001-send-email.sh | 31 +++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 14 deletions(-)