threads / patch / 55668

patchgit-send-email: add sendmailCommand option

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

## tl;dr

19 messages between May 12, 2021 and May 14, 2021. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Gregory Anders· May 12, 2021, 03:30 UTC · lore

The sendemail.smtpServer option currently supports 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 I wish to pass arguments to my program, I 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.

This patch attempts to solve these problems by introducing a new configuration option sendemail.sendmailCommand as well as a command line option --sendmail-cmd. The value of this option is invoked with the standard sendmail options passed as arguments.

sendmailCommand has precedence over smtpServer. If both options are unspecified, the default is to search for 'sendmail' in /usr/sbin, /usr/lib, and $PATH. If not found, smtpServer is set to localhost. This mimics the current behavior when smtpServer is unspecified.

The option is passed to Perl's `exec()` function, which automatically determines whether or not to invoke a shell. If shell metacharacters are detected, then a shell is used; otherwise, the command is invoked directly. This means that e.g.

    [sendemail]
            sendmailCommand = f() { if [ "$(uname -s) = Darwin ]; then sendmail "$@"; else postfix "$@"; fi; };f
will be executed in a shell (as expected), while
    [sendemail]
            sendmailCommand = msmtp
will be executed directly.

This change deprecates the use of absolute paths in sendemail.smtpServer, although support is kept for backward compatibility. ---

Note that this patch is incompatible with (and supersedes) the patch discussed here:

    https://public-inbox.org/git/YJs2RceLliGHI5TX@gpanders.com/T/#t
 Documentation/git-send-email.txt |  39 ++++---
 git-send-email.perl              |  30 ++++--
 t/t9001-send-email.sh            | 169 ++++++++++++++++++-------------
 3 files changed, 140 insertions(+), 98 deletions(-)
Show changes to 3 files +140 −98

Documentation/git-send-email.txt, git-send-email.perl, t/t9001-send-email.sh

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 93708aefea..d9fe8cb7c0 100644
--- 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.
+
+--sendmail-cmd=<command>::
+	Specify a command to run to send the email. The command should
+	be compatible with `sendmail` as the arguments are passed
+	directly.  The command will be executed in the shell if
+	necessary.  Default is the value of `sendemail.sendmailCommand`.
+	If unspecified, and if --smtp-server is also unspecified,
+	git-send-email will search for `sendmail` in `/usr/sbin`,
+	`/usr/lib` and $PATH if such a program is available.
 
 --smtp-encryption=<encryption>::
 	Specify the encryption to use, either 'ssl' or 'tls'.  Any other
@@ -211,13 +221,14 @@ 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.  Prefer 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..cecca03ed1 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>  * 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,
 		    "smtp-server=s" => \$smtp_server,
 		    "smtp-server-option=s" => \@smtp_server_options,
 		    "smtp-server-port=s" => \$smtp_server_port,
@@ -1008,11 +1012,14 @@ sub expand_one_alias {
 	push @sendmail_paths, map {"$_/sendmail"} split /:/, $ENV{PATH};
 	foreach (@sendmail_paths) {
 		if (-x $_) {
-			$smtp_server = $_;
+			$sendmail_command ||= $_;
 			last;
 		}
 	}
-	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
+
+	if (!defined $sendmail_command) {
+		$smtp_server = 'localhost'; # could be 127.0.0.1, too... *shrug*
+	}
 }
 
 if ($compose && $compose > 0) {
@@ -1490,14 +1497,15 @@ sub send_message {
 
 	unshift (@sendmail_parameters, @smtp_server_options);
 
+	if (file_name_is_absolute($smtp_server)) {
+		# Preserved for backward compatibility
+		$sendmail_command ||= $smtp_server;
+	}
+
 	if ($dry_run) {
 		# We don't want to send the email.
-	} elsif (file_name_is_absolute($smtp_server)) {
-		my $pid = open my $sm, '|-';
-		defined $pid or die $!;
-		if (!$pid) {
-			exec($smtp_server, @sendmail_parameters) or die $!;
-		}
+	} elsif (defined $sendmail_command) {
+		open my $sm, '|-', "$sendmail_command @sendmail_parameters";
 		print $sm "$header\n$message";
 		close $sm or die $!;
 	} else {
@@ -1592,14 +1600,14 @@ 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_command) {
+			print "Sendmail: $sendmail_command ".join(' ',@sendmail_parameters)."\n";
+		} else {
 			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";
 		}
 		print $header, "\n";
 		if ($smtp) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b3035371..82a3efb987 100755
--- 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\"" \
 		$@ \
 		$patches >stdout &&
 		! grep "Send this email" stdout &&
@@ -94,7 +94,7 @@ test_expect_success $PREREQ 'No confirm with sendemail.confirm=never' '
 '
 
 test_expect_success $PREREQ 'Send patches' '
-	git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
+	git send-email --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --sendmail-cmd="\"$(pwd)/fake.sendmail\"" $patches 2>errors
 '
 
 test_expect_success $PREREQ 'setup expect' '
@@ -112,7 +112,7 @@ test_expect_success $PREREQ 'Verify commandline' '
 
 test_expect_success $PREREQ 'Send patches with --envelope-sender' '
 	clean_fake_sendmail &&
-	git send-email --envelope-sender="Patch Contributor <patch@example.com>" --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
+	git send-email --envelope-sender="Patch Contributor <patch@example.com>" --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --sendmail-cmd="\"$(pwd)/fake.sendmail\"" $patches 2>errors
 '
 
 test_expect_success $PREREQ 'setup expect' '
@@ -132,7 +132,7 @@ test_expect_success $PREREQ 'Verify commandline' '
 
 test_expect_success $PREREQ 'Send patches with --envelope-sender=auto' '
 	clean_fake_sendmail &&
-	git send-email --envelope-sender=auto --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server="$(pwd)/fake.sendmail" $patches 2>errors
+	git send-email --envelope-sender=auto --suppress-cc=sob --from="Example <nobody@example.com>" --to=nobody@example.com --sendmail-cmd="\"$(pwd)/fake.sendmail\"" $patches 2>errors
 '
 
 test_expect_success $PREREQ 'setup expect' '
@@ -178,7 +178,7 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '
 	EOF
 	clean_fake_sendmail &&
 	git send-email -1 --to=recipient@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" &&
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" &&
 	test_cmp expected-cc commandline1
 '
 
@@ -197,7 +197,7 @@ test_expect_success $PREREQ 'cc trailer with get_maintainer.pl output' '
 	clean_fake_sendmail &&
 	git send-email -1 --to=recipient@example.com \
 		--cc-cmd=./expected-cc-script.sh \
-		--smtp-server="$(pwd)/fake.sendmail" &&
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" &&
 	test_cmp expected-cc commandline1
 '
 
@@ -252,7 +252,7 @@ test_suppress_self () {
 		--to=nobody@example.com \
 		--cc-cmd=./cccmd-sed \
 		--suppress-cc=self \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		suppress-self-$3.patch &&
 
 	mv msgtxt1 msgtxt1-$3 &&
@@ -338,7 +338,7 @@ test_expect_success $PREREQ 'Prompting works' '
 	(echo "to@example.com" &&
 	 echo ""
 	) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches \
 		2>errors &&
 		grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
@@ -352,7 +352,7 @@ test_expect_success $PREREQ,AUTOIDENT 'implicit ident is allowed' '
 	sane_unset GIT_COMMITTER_NAME &&
 	sane_unset GIT_COMMITTER_EMAIL &&
 	GIT_SEND_EMAIL_NOTTY=1 git send-email \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--to=to@example.com \
 		$patches </dev/null 2>errors
 	)
@@ -366,7 +366,7 @@ test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email'
 	sane_unset GIT_COMMITTER_EMAIL &&
 	GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
 	test_must_fail git send-email \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--to=to@example.com \
 		$patches </dev/null 2>errors &&
 	test_i18ngrep "tell me who you are" errors
@@ -389,7 +389,7 @@ test_expect_success $PREREQ 'tocmd works' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to-cmd=./tocmd-sed \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		tocmd.patch \
 		&&
 	grep "^To: tocmd@example.com" msgtxt1
@@ -403,7 +403,7 @@ test_expect_success $PREREQ 'cccmd works' '
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
 		--cc-cmd=./cccmd-sed \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		cccmd.patch \
 		&&
 	grep "^	cccmd@example.com" msgtxt1
@@ -423,7 +423,7 @@ test_expect_success $PREREQ 'reject long lines' '
 	test_must_fail git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--transfer-encoding=8bit \
 		$patches longline.patch \
 		2>actual &&
@@ -443,7 +443,7 @@ test_expect_success $PREREQ 'Author From: in message body' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	sed "1,/^\$/d" <msgtxt1 >msgbody1 &&
 	grep "From: A <author@example.com>" msgbody1
@@ -454,7 +454,7 @@ test_expect_success $PREREQ 'Author From: not in message body' '
 	git send-email \
 		--from="A <author@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	sed "1,/^\$/d" <msgtxt1 >msgbody1 &&
 	! grep "From: A <author@example.com>" msgbody1
@@ -464,7 +464,7 @@ test_expect_success $PREREQ 'allow long lines with --no-validate' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--no-validate \
 		$patches longline.patch \
 		2>errors
@@ -475,7 +475,7 @@ test_expect_success $PREREQ 'short lines with auto encoding are 8bit' '
 	git send-email \
 		--from="A <author@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--transfer-encoding=auto \
 		$patches &&
 	grep "Content-Transfer-Encoding: 8bit" msgtxt1
@@ -486,7 +486,7 @@ test_expect_success $PREREQ 'long lines with auto encoding are quoted-printable'
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--transfer-encoding=auto \
 		--no-validate \
 		longline.patch &&
@@ -500,7 +500,7 @@ test_expect_success $PREREQ 'carriage returns with auto encoding are quoted-prin
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--transfer-encoding=auto \
 		--no-validate \
 		cr.patch &&
@@ -513,7 +513,7 @@ do
 		git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			--transfer-encoding=$enc \
 			--validate \
 			$patches longline.patch
@@ -533,7 +533,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
 	test_must_fail git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--validate \
 		longline.patch 2>actual &&
 	test_path_is_file my-hooks.ran &&
@@ -552,7 +552,7 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
 	test_must_fail git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		--validate \
 		longline.patch 2>actual &&
 	test_path_is_file my-hooks.ran &&
@@ -571,7 +571,7 @@ do
 		git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			--transfer-encoding=$enc \
 			$patches &&
 		grep "Content-Transfer-Encoding: $enc" msgtxt1
@@ -584,7 +584,7 @@ test_expect_success $PREREQ 'Invalid In-Reply-To' '
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
 		--in-reply-to=" " \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches \
 		2>errors &&
 	! grep "^In-Reply-To: < *>" msgtxt1
@@ -596,7 +596,7 @@ test_expect_success $PREREQ 'Valid In-Reply-To when prompting' '
 	 echo "To Example <to@example.com>" &&
 	 echo ""
 	) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches 2>errors &&
 	! grep "^In-Reply-To: < *>" msgtxt1
 '
@@ -609,7 +609,7 @@ test_expect_success $PREREQ 'In-Reply-To without --chain-reply-to' '
 		--to=nobody@example.com \
 		--no-chain-reply-to \
 		--in-reply-to="$(cat expect)" \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches $patches $patches \
 		2>errors &&
 	# The first message is a reply to --in-reply-to
@@ -631,7 +631,7 @@ test_expect_success $PREREQ 'In-Reply-To with --chain-reply-to' '
 		--to=nobody@example.com \
 		--chain-reply-to \
 		--in-reply-to="$(cat expect)" \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches $patches $patches \
 		2>errors &&
 	sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
@@ -658,7 +658,7 @@ test_expect_success $PREREQ '--compose works' '
 	--compose --subject foo \
 	--from="Example <nobody@example.com>" \
 	--to=nobody@example.com \
-	--smtp-server="$(pwd)/fake.sendmail" \
+	--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 	$patches \
 	2>errors
 '
@@ -992,7 +992,7 @@ test_confirm () {
 		git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$@ $patches >stdout &&
 	grep "Send this email" stdout
 }
@@ -1034,7 +1034,7 @@ test_expect_success $PREREQ 'confirm detects EOF (inform assumes y)' '
 		git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			outdir/*.patch </dev/null
 '
 
@@ -1046,7 +1046,7 @@ test_expect_success $PREREQ 'confirm detects EOF (auto causes failure)' '
 		test_must_fail git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			$patches </dev/null
 '
 
@@ -1058,7 +1058,7 @@ test_expect_success $PREREQ 'confirm does not loop forever' '
 		yes "bogus" | test_must_fail git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			$patches
 '
 
@@ -1069,7 +1069,7 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '
 	git send-email \
 	--from="Example <nobody@example.com>" \
 	--to=nobody@example.com \
-	--smtp-server="$(pwd)/fake.sendmail" \
+	--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 	outdir/*.patch &&
 	grep "^	" msgtxt1 |
 	grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"
@@ -1085,7 +1085,7 @@ test_expect_success $PREREQ '--compose adds MIME for utf8 body' '
 		--compose --subject foo \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^utf8 body" msgtxt1 &&
 	grep "^Content-Type: text/plain; charset=UTF-8" msgtxt1
@@ -1108,7 +1108,7 @@ test_expect_success $PREREQ '--compose respects user mime type' '
 		--compose --subject foo \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^utf8 body" msgtxt1 &&
 	grep "^Content-Type: text/plain; charset=iso-8859-1" msgtxt1 &&
@@ -1122,7 +1122,7 @@ test_expect_success $PREREQ '--compose adds MIME for utf8 subject' '
 		--compose --subject utf8-sübjëct \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^fake edit" msgtxt1 &&
 	grep "^Subject: =?UTF-8?q?utf8-s=C3=BCbj=C3=ABct?=" msgtxt1
@@ -1136,7 +1136,7 @@ test_expect_success $PREREQ 'utf8 author is correctly passed on' '
 	git format-patch --stdout -1 >funny_name.patch &&
 	git send-email --from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		funny_name.patch &&
 	grep "^From: Füñný Nâmé <odd_?=mail@example.com>" msgtxt1
 '
@@ -1149,7 +1149,7 @@ test_expect_success $PREREQ 'utf8 sender is not duplicated' '
 	git format-patch --stdout -1 >funny_name.patch &&
 	git send-email --from="Füñný Nâmé <odd_?=mail@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		funny_name.patch &&
 	grep "^From: " msgtxt1 >msgfrom &&
 	test_line_count = 1 msgfrom
@@ -1166,7 +1166,7 @@ test_expect_success $PREREQ 'sendemail.composeencoding works' '
 		--compose --subject foo \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^utf8 body" msgtxt1 &&
 	grep "^Content-Type: text/plain; charset=iso-8859-1" msgtxt1
@@ -1183,7 +1183,7 @@ test_expect_success $PREREQ '--compose-encoding works' '
 		--compose --subject foo \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^utf8 body" msgtxt1 &&
 	grep "^Content-Type: text/plain; charset=iso-8859-1" msgtxt1
@@ -1201,7 +1201,7 @@ test_expect_success $PREREQ '--compose-encoding overrides sendemail.composeencod
 		--compose --subject foo \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^utf8 body" msgtxt1 &&
 	grep "^Content-Type: text/plain; charset=iso-8859-2" msgtxt1
@@ -1215,7 +1215,7 @@ test_expect_success $PREREQ '--compose-encoding adds correct MIME for subject' '
 		--compose --subject utf8-sübjëct \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$patches &&
 	grep "^fake edit" msgtxt1 &&
 	grep "^Subject: =?iso-8859-2?q?utf8-s=C3=BCbj=C3=ABct?=" msgtxt1
@@ -1465,7 +1465,7 @@ test_expect_success $PREREQ 'ASCII subject is not RFC2047 quoted' '
 	clean_fake_sendmail &&
 	echo bogus |
 	git send-email --from=author@example.com --to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
 	grep "Subject" msgtxt1 >actual &&
@@ -1484,7 +1484,7 @@ test_expect_success $PREREQ 'asks about and fixes 8bit encodings' '
 	clean_fake_sendmail &&
 	echo |
 	git send-email --from=author@example.com --to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			email-using-8bit >stdout &&
 	grep "do not declare a Content-Transfer-Encoding" stdout &&
 	grep email-using-8bit stdout &&
@@ -1498,7 +1498,7 @@ test_expect_success $PREREQ 'sendemail.8bitEncoding works' '
 	git config sendemail.assume8bitEncoding UTF-8 &&
 	echo bogus |
 	git send-email --from=author@example.com --to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			email-using-8bit >stdout &&
 	egrep "Content|MIME" msgtxt1 >actual &&
 	test_cmp content-type-decl actual
@@ -1509,7 +1509,7 @@ test_expect_success $PREREQ '--8bit-encoding overrides sendemail.8bitEncoding' '
 	git config sendemail.assume8bitEncoding "bogus too" &&
 	echo bogus |
 	git send-email --from=author@example.com --to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
 	egrep "Content|MIME" msgtxt1 >actual &&
@@ -1538,7 +1538,7 @@ test_expect_success $PREREQ '--8bit-encoding also treats subject' '
 	clean_fake_sendmail &&
 	echo bogus |
 	git send-email --from=author@example.com --to=nobody@example.com \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			--8bit-encoding=UTF-8 \
 			email-using-8bit >stdout &&
 	grep "Subject" msgtxt1 >actual &&
@@ -1563,7 +1563,7 @@ test_expect_success $PREREQ '--transfer-encoding overrides sendemail.transferEnc
 	test_must_fail git -c sendemail.transferEncoding=8bit \
 		send-email \
 		--transfer-encoding=7bit \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-8bit \
 		2>errors >out &&
 	grep "cannot send message as 7bit" errors &&
@@ -1574,7 +1574,7 @@ test_expect_success $PREREQ 'sendemail.transferEncoding via config' '
 	clean_fake_sendmail &&
 	test_must_fail git -c sendemail.transferEncoding=7bit \
 		send-email \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-8bit \
 		2>errors >out &&
 	grep "cannot send message as 7bit" errors &&
@@ -1585,7 +1585,7 @@ test_expect_success $PREREQ 'sendemail.transferEncoding via cli' '
 	clean_fake_sendmail &&
 	test_must_fail git send-email \
 		--transfer-encoding=7bit \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-8bit \
 		2>errors >out &&
 	grep "cannot send message as 7bit" errors &&
@@ -1602,7 +1602,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=quoted-printab
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=quoted-printable \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-8bit \
 		2>errors >out &&
 	sed "1,/^$/d" msgtxt1 >actual &&
@@ -1619,7 +1619,7 @@ test_expect_success $PREREQ '8-bit and sendemail.transferencoding=base64' '
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=base64 \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-8bit \
 		2>errors >out &&
 	sed "1,/^$/d" msgtxt1 >actual &&
@@ -1645,7 +1645,7 @@ test_expect_success $PREREQ 'convert from quoted-printable to base64' '
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=base64 \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-qp \
 		2>errors >out &&
 	sed "1,/^$/d" msgtxt1 >actual &&
@@ -1675,7 +1675,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=quoted-printabl
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=quoted-printable \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-crlf \
 		2>errors >out &&
 	sed "1,/^$/d" msgtxt1 >actual &&
@@ -1692,7 +1692,7 @@ test_expect_success $PREREQ 'CRLF and sendemail.transferencoding=base64' '
 	clean_fake_sendmail &&
 	git send-email \
 		--transfer-encoding=base64 \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		email-using-crlf \
 		2>errors >out &&
 	sed "1,/^$/d" msgtxt1 >actual &&
@@ -1710,7 +1710,7 @@ test_expect_success $PREREQ 'refusing to send cover letter template' '
 	test_must_fail git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		outdir/0002-*.patch \
 		outdir/0000-*.patch \
 		outdir/0001-*.patch \
@@ -1727,7 +1727,7 @@ test_expect_success $PREREQ '--force sends cover letter template anyway' '
 		--force \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		outdir/0002-*.patch \
 		outdir/0000-*.patch \
 		outdir/0001-*.patch \
@@ -1750,7 +1750,7 @@ test_cover_addresses () {
 		--from="Example <nobody@example.com>" \
 		--no-to --no-cc \
 		"$@" \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		outdir/0000-*.patch \
 		outdir/0001-*.patch \
 		outdir/0002-*.patch \
@@ -1789,7 +1789,7 @@ test_expect_success $PREREQ 'escaped quotes in sendemail.aliasfiletype=mutt' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=sbd \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		outdir/0001-*.patch \
 		2>errors >out &&
 	grep "^!somebody@example\.org!$" commandline1 &&
@@ -1804,7 +1804,7 @@ test_expect_success $PREREQ 'sendemail.aliasfiletype=mailrc' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=sbd \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		outdir/0001-*.patch \
 		2>errors >out &&
 	grep "^!somebody@example\.org!$" commandline1
@@ -1818,7 +1818,7 @@ test_expect_success $PREREQ 'sendemail.aliasfile=~/.mailrc' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=sbd \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		outdir/0001-*.patch \
 		2>errors >out &&
 	grep "^!someone@example\.org!$" commandline1
@@ -1929,7 +1929,7 @@ test_sendmail_aliases () {
 		git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=alice --to=bcgrp \
-			--smtp-server="$(pwd)/fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 			outdir/0001-*.patch \
 			2>errors >out &&
 		for i in $expect
@@ -1995,7 +1995,7 @@ test_expect_success $PREREQ 'alias support in To header' '
 	git format-patch --stdout -1 --to=sbd >aliased.patch &&
 	git send-email \
 		--from="Example <nobody@example.com>" \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		aliased.patch \
 		2>errors >out &&
 	grep "^!someone@example\.org!$" commandline1
@@ -2009,7 +2009,7 @@ test_expect_success $PREREQ 'alias support in Cc header' '
 	git format-patch --stdout -1 --cc=sbd >aliased.patch &&
 	git send-email \
 		--from="Example <nobody@example.com>" \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		aliased.patch \
 		2>errors >out &&
 	grep "^!someone@example\.org!$" commandline1
@@ -2025,7 +2025,7 @@ test_expect_success $PREREQ 'tocmd works with aliases' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to-cmd=./tocmd-sed \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		tocmd.patch \
 		2>errors >out &&
 	grep "^!someone@example\.org!$" commandline1
@@ -2041,7 +2041,7 @@ test_expect_success $PREREQ 'cccmd works with aliases' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--cc-cmd=./cccmd-sed \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		cccmd.patch \
 		2>errors >out &&
 	grep "^!someone@example\.org!$" commandline1
@@ -2053,7 +2053,7 @@ do_xmailer_test () {
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=someone@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		$params \
 		0001-*.patch \
 		2>errors >out &&
@@ -2148,6 +2148,29 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
 	test_cmp expected-list actual-list
 '
 
+test_expect_success $PREREQ 'test using relative path with sendmailCommand' '
+	clean_fake_sendmail &&
+	PATH="$(pwd):$PATH" \
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--sendmail-cmd="fake.sendmail" \
+		HEAD~2 &&
+	test_path_is_file commandline1 &&
+	test_path_is_file commandline2
+'
+
+test_expect_success $PREREQ 'test using shell with sendmailCommand' '
+	clean_fake_sendmail &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--sendmail-cmd="[ 1 -eq 1 ] && \"$(pwd)/fake.sendmail\"" \
+		HEAD~2 &&
+	test_path_is_file commandline1 &&
+	test_path_is_file commandline2
+'
+
 test_expect_success $PREREQ 'invoke hook' '
 	mkdir -p .git/hooks &&
 
@@ -2174,7 +2197,7 @@ test_expect_success $PREREQ 'invoke hook' '
 		git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/../fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/../fake.sendmail\"" \
 			../0001-add-main.patch &&
 
 		# Verify error message when a patch is rejected by the hook
@@ -2182,7 +2205,7 @@ test_expect_success $PREREQ 'invoke hook' '
 		test_must_fail git send-email \
 			--from="Example <nobody@example.com>" \
 			--to=nobody@example.com \
-			--smtp-server="$(pwd)/../fake.sendmail" \
+			--sendmail-cmd="\"$(pwd)/../fake.sendmail\"" \
 			../another.patch 2>err &&
 		test_i18ngrep "rejected by sendemail-validate hook" err
 	)
@@ -2192,7 +2215,7 @@ test_expect_success $PREREQ 'test that send-email works outside a repo' '
 	nongit git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		"$(pwd)/0001-add-main.patch"
 '
 
@@ -2201,7 +2224,7 @@ test_expect_success $PREREQ 'test that sendmail config is rejected' '
 	test_must_fail git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		HEAD^ 2>err &&
 	test_i18ngrep "found configuration options for '"'"sendmail"'"'" err
 '
@@ -2211,7 +2234,7 @@ test_expect_success $PREREQ 'test that sendmail config rejection is specific' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		HEAD^
 '
 
@@ -2221,7 +2244,7 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
 	git send-email \
 		--from="Example <nobody@example.com>" \
 		--to=nobody@example.com \
-		--smtp-server="$(pwd)/fake.sendmail" \
+		--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
 		HEAD^
 '
 
-- 
2.31.1.576.g12e560b5fe
Junio C Hamano· May 12, 2021, 04:19 UTC · re: Gregory Anders · lore

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

Gregory Anders <greg@gpanders.com> writes:
> The sendemail.smtpServer option currently supports using a sendmail-like
> program to send emails by specifying an absolute file path. However,

That is not wrong per-se, but it is not limited to the configuration variable, but is a shared trait with --smtp-server command line option. It is easier on the readers to mention both.

Our problem description talks about the status quo in the present tense. No noiseword "currently " necessary. I.e. something along this line:

    The sendemail.smtpServer configuration variable (and the
    "--smtp-server" command line option of "git send-email" command)
    allows to name a command to run to send emails by specifying an
    absolute path name.  However,
Show 12 quoted lines
> 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 I wish
> to pass arguments to my program, I 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.

Up to here, nice explanation of the background and description of the problem being solved.

> This patch attempts to solve these problems by introducing a new
> configuration option sendemail.sendmailCommand as well as a command line
> option --sendmail-cmd. The value of this option is invoked with the
> standard sendmail options passed as arguments.

When presenting a potential solution, in the history of this project, we'd talk as if we are giving an order to the codebase to "be like so".

    Introduce a command line option '--sendmail-cmd' and a
    configuration variable sendemail.sendmailCommand that can be
    used to specify the command line (possibly including its command
    line options) to send pieces of e-mail.  This is invoked while
    honoring $PATH, so it does not have to be named with an absolute
    path to the command.
    Give it a higher precedence over --smtp-server (and
    sendemail.smtpServer), as the new interface is more flexible.
> sendmailCommand has precedence over smtpServer. If both options are
> unspecified, the default is to search for 'sendmail' in /usr/sbin,
> /usr/lib, and $PATH. If not found, smtpServer is set to localhost. This
> mimics the current behavior when smtpServer is unspecified.

I do not think "If both options are unspecified" and everything after it is needed.

> The option is passed to Perl's `exec()` function, which automatically
> determines whether or not to invoke a shell. If shell metacharacters are
> detected, then a shell is used; otherwise, the command is invoked
> directly.

I do not think this, and the two examples below (omitted), are relevant, either.

The "metacharacters make the command diverted to shell" is a mere optimization and not of interest to the end users. Even if sendemail.sendmailcommand is set to just a single word 'msmtp', which does not have any metacharacter, we _could_ spawn it via the shell and the observable end result would be the same as if the single word was directly executed without the shell.

> This change deprecates the use of absolute paths in 
> sendemail.smtpServer, although support is kept for backward
> compatibility.

I am on the fence about saying this. We may eventually want to deprecate, but until we start issuing a warning when the absolute-path form is used, I'd rather not to call it "deprecated" in either the proposed log message or in the documenation.

> ---
Missing sign-off.
Show 5 quoted lines
>
> Note that this patch is incompatible with (and supersedes) the patch
> discussed here: 
>
>     https://public-inbox.org/git/YJs2RceLliGHI5TX@gpanders.com/T/#t
Thanks---such a note is very valuable.
Show 23 quoted lines
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 93708aefea..d9fe8cb7c0 100644
> --- 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.

Is this a totally unwarranted rewrapping of an unrelated part of the same document, or was there some words or phrases in this description of the envelope-sender option that needed to be adjusted for the introduction of sendmail-cmd option?

Show 8 quoted lines
> +--sendmail-cmd=<command>::
> +	Specify a command to run to send the email. The command should
> +	be compatible with `sendmail` as the arguments are passed
> +	directly.  The command will be executed in the shell if
> +	necessary.  Default is the value of `sendemail.sendmailCommand`.
> +	If unspecified, and if --smtp-server is also unspecified,
> +	git-send-email will search for `sendmail` in `/usr/sbin`,
> +	`/usr/lib` and $PATH if such a program is available.
OK, but doesn't this also need to support '-i'?
Show 19 quoted lines
> @@ -211,13 +221,14 @@ 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.  Prefer using `--sendmail-cmd` instead.

Drop the last sentence, if we are not going to explain why. Or perhaps:

	... an absolute path to a program that behaves like
	`sendmail` (among other things, it must support the `-i`
	option).  As you only can specify the path to the program
	and cannot give any leading arguments to it, consider using
	`--sendmail-cmd` instead.
Show 8 quoted lines
> @@ -1490,14 +1497,15 @@ sub send_message {
>  
>  	unshift (@sendmail_parameters, @smtp_server_options);
>  
> +	if (file_name_is_absolute($smtp_server)) {
> +		# Preserved for backward compatibility
> +		$sendmail_command ||= $smtp_server;
> +	}
Hmph, I wonder if this makes the intent more clear.
	if (!defined $sendmail_command && file_name_is_absolute($smtp_server)) {
		$sendmail_command = $smtp_server;
	}

That is, if the user gave us the command in newer form, we do not even have to bother checking if the server is given as an absolute pathname.

Show 9 quoted lines
> @@ -1069,7 +1069,7 @@ test_expect_success $PREREQ 'utf8 Cc is rfc2047 encoded' '
>  	git send-email \
>  	--from="Example <nobody@example.com>" \
>  	--to=nobody@example.com \
> -	--smtp-server="$(pwd)/fake.sendmail" \
> +	--sendmail-cmd="\"$(pwd)/fake.sendmail\"" \
>  	outdir/*.patch &&
>  	grep "^	" msgtxt1 |
>  	grep "=?UTF-8?q?=C3=A0=C3=A9=C3=AC=C3=B6=C3=BA?= <utf8@example.com>"

You seem to have replaced every smtp-server="$(pwd)/ mechanically with sendmai-cmd=\"$(pwd)/, but please make sure that we have at least one test left that passes an absolute path to --smtp-server to ensure that the old mechanism keeps working. A bonus point for marking such a test that needs to be adjusted when the actual deprecation happens (i.e. we'd likely to detect the use of absolute path and throw a warning, so the test should notice the warning message).

Also you would want to tweak some of the --sendmail-cmd variants to use just the command name, with and without args, to ensure that (1) discovery on $PATH works, and (2) passing initial args works.

Thanks.
Gregory Anders· May 12, 2021, 13:03 UTC · re: Junio C Hamano · lore

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

On Wed, 12 May 2021 13:19 +0900, Junio C Hamano wrote:
>Is this a totally unwarranted rewrapping of an unrelated part of the
>same document, or was there some words or phrases in this
>description of the envelope-sender option that needed to be adjusted
>for the introduction of sendmail-cmd option?

Yes it's just a rewrapping that I did while adding my new paragraph. The other reviewers pointed this out as well. My mistake, I will remove this from the next patch revision.

Show 8 quoted lines
>> +--sendmail-cmd=<command>::
>> +	Specify a command to run to send the email. The command should
>> +	be compatible with `sendmail` as the arguments are passed
>> +	directly.  The command will be executed in the shell if
>> +	necessary.  Default is the value of `sendemail.sendmailCommand`.
>> +	If unspecified, and if --smtp-server is also unspecified,
>> +	git-send-email will search for `sendmail` in `/usr/sbin`,
>> +	`/usr/lib` and $PATH if such a program is available.
>OK, but doesn't this also need to support '-i'?

'The command should be compatible with `sendmail`' was meant to imply this, though I can make this more explicit.

Show 19 quoted lines
>> @@ -211,13 +221,14 @@ 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.  Prefer using `--sendmail-cmd` instead.
>Drop the last sentence, if we are not going to explain why.

I do think nudging users to use the "correct" option is valuable, so I will add some why text. Though I think adhering to the "--smtp-server should specify a host and --sendmail-cmd specifies a command" dichotomy is a good reason in and of itself.

Show 18 quoted lines
>> @@ -1490,14 +1497,15 @@ sub send_message {
>>
>>  	unshift (@sendmail_parameters, @smtp_server_options);
>>
>> +	if (file_name_is_absolute($smtp_server)) {
>> +		# Preserved for backward compatibility
>> +		$sendmail_command ||= $smtp_server;
>> +	}
>
>Hmph, I wonder if this makes the intent more clear.
>
>	if (!defined $sendmail_command && file_name_is_absolute($smtp_server)) {
>		$sendmail_command = $smtp_server;
>	}
>
>That is, if the user gave us the command in newer form, we do not
>even have to bother checking if the server is given as an absolute
>pathname.
Yes I think you're right, I'll make this change.
Show 8 quoted lines
>You seem to have replaced every smtp-server="$(pwd)/ mechanically
>with sendmai-cmd=\"$(pwd)/, but please make sure that we have at
>least one test left that passes an absolute path to --smtp-server to
>ensure that the old mechanism keeps working.  A bonus point for
>marking such a test that needs to be adjusted when the actual
>deprecation happens (i.e. we'd likely to detect the use of absolute
>path and throw a warning, so the test should notice the warning
>message).
Noted, I'll add a test for this case.
>Also you would want to tweak some of the --sendmail-cmd variants to
>use just the command name, with and without args, to ensure that (1)
>discovery on $PATH works, and (2) passing initial args works.
I did add two such tests:
Show changes to t/t9001-send-email.sh +23 −0
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b3035371..82a3efb987 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2148,6 +2148,29 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
         test_cmp expected-list actual-list
  '

+test_expect_success $PREREQ 'test using relative path with sendmailCommand' '
+       clean_fake_sendmail &&
+       PATH="$(pwd):$PATH" \
+       git send-email \
+               --from="Example <nobody@example.com>" \
+               --to=nobody@example.com \
+               --sendmail-cmd="fake.sendmail" \
+               HEAD~2 &&
+       test_path_is_file commandline1 &&
+       test_path_is_file commandline2
+'
+
+test_expect_success $PREREQ 'test using shell with sendmailCommand' '
+       clean_fake_sendmail &&
+       git send-email \
+               --from="Example <nobody@example.com>" \
+               --to=nobody@example.com \
+               --sendmail-cmd="[ 1 -eq 1 ] && \"$(pwd)/fake.sendmail\"" \
+               HEAD~2 &&
+       test_path_is_file commandline1 &&
+       test_path_is_file commandline2
+'
+
  test_expect_success $PREREQ 'invoke hook' '
         mkdir -p .git/hooks &&


Granted, the second test tests for some generic shell expression, not 
passing arguments, but I think if the former works the latter ought to 
as well.

Thanks for your feedback.
Felipe Contreras· May 12, 2021, 07:57 UTC · re: Gregory Anders · lore

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

Gregory Anders wrote:
Show 21 quoted lines
> --- 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.
Show 31 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?

Once again: "$command=s" -> \$command,
Show 8 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.

Cheers.
-- 
Felipe Contreras
Gregory Anders· May 12, 2021, 13:12 UTC · re: Felipe Contreras · lore

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

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
Felipe Contreras· May 12, 2021, 17:21 UTC · re: Gregory Anders · lore

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

Gregory Anders wrote:
Show 24 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 see. In that case that might make sense. I still prefer #1.
Show 27 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.

The sole purpose of software is that it's useful to users. Software that works today but not tomorrow is bad software.

The primary purpose of the testing framework is to ensure that doesn't happen; that git keeps working in the same way today than it did yesterday.

That's why it's more important that tests excercise the options people were using yesterday.

In addition we also want to be testing new functionality, but that's *in addition*.

Maybe at some point in the future more people will be using --sendmail-cmd, but right now that's not the case. Right now we need to be testing the option people are using *today*.

Cheers.
-- 
Felipe Contreras
Gregory Anders· May 12, 2021, 18:06 UTC · re: Felipe Contreras · lore

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

On Wed, 12 May 2021 12:21 -0500, Felipe Contreras wrote:
Show 16 quoted lines
>The sole purpose of software is that it's useful to users. Software 
>that works today but not tomorrow is bad software.
>
>The primary purpose of the testing framework is to ensure that doesn't
>happen; that git keeps working in the same way today than it did
>yesterday.
>
>That's why it's more important that tests excercise the options people
>were using yesterday.
>
>In addition we also want to be testing new functionality, but that's *in
>addition*.
>
>Maybe at some point in the future more people will be using
>--sendmail-cmd, but right now that's not the case. Right now we need to
>be testing the option people are using *today*.

I agree with this completely. Is this requirement satisfied with the addition of a test that checks the usage of an absolute path with --smtp-server (the behavior that people were using yesterday), while all of the existing tests are still converted to the new option? I have such a test written (along with a few others) in the forthcoming v2 patch:

Show changes to t/t9001-send-email.sh +49 −0
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 65b3035371..45bc3c0c4c 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2148,6 +2148,55 @@ test_expect_success $PREREQ 'leading and trailing 
whitespaces are removed' '
  	test_cmp expected-list actual-list
  '
  
+test_expect_success $PREREQ 'test using absolute path for --smtp-server' '
+	clean_fake_sendmail &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		HEAD^ &&
+	test_path_is_file commandline1
+'
+
+test_expect_success $PREREQ 'arguments not supported with --smtp-server' '
+	test_expect_code 127 git send-email \
+		--from="Example <nobody@example.com>" \
+		--to=nobody@example.com \
+		--smtp-server="$(pwd)/fake.sendmail -f nobody@example.com" \
+		HEAD^
+'
+
+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="[ 1 -eq 1 ] && \"$(pwd)/fake.sendmail\"" \
+		HEAD^ &&
+	test_path_is_file commandline1
+'
+
  test_expect_success $PREREQ 'invoke hook' '
         mkdir -p .git/hooks &&
Felipe Contreras· May 12, 2021, 19:32 UTC · re: Gregory Anders · lore

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

Gregory Anders wrote:
Show 23 quoted lines
> On Wed, 12 May 2021 12:21 -0500, Felipe Contreras wrote:
> >The sole purpose of software is that it's useful to users. Software 
> >that works today but not tomorrow is bad software.
> >
> >The primary purpose of the testing framework is to ensure that doesn't
> >happen; that git keeps working in the same way today than it did
> >yesterday.
> >
> >That's why it's more important that tests excercise the options people
> >were using yesterday.
> >
> >In addition we also want to be testing new functionality, but that's *in
> >addition*.
> >
> >Maybe at some point in the future more people will be using
> >--sendmail-cmd, but right now that's not the case. Right now we need to
> >be testing the option people are using *today*.
> 
> I agree with this completely. Is this requirement satisfied with the 
> addition of a test that checks the usage of an absolute path with 
> --smtp-server (the behavior that people were using yesterday), while all 
> of the existing tests are still converted to the new option? I have such 
> a test written (along with a few others) in the forthcoming v2 patch:

I still insist the current tests should not be converted. But ultimately it's Junio's call.

Cheers.
-- 
Felipe Contreras
Ævar Arnfjörð Bjarmason· May 12, 2021, 09:04 UTC · re: Gregory Anders · lore

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

On Tue, May 11 2021, Gregory Anders wrote:
Show 23 quoted lines
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 93708aefea..d9fe8cb7c0 100644
> --- 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.

Please don't include word-wrapping for unrelated changes in the main patch.

Show 6 quoted lines
> -	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
> +
> +	if (!defined $sendmail_command) {
> +		$smtp_server = 'localhost'; # could be 127.0.0.1, too... *shrug*
> +	}
>  }

This "let's not accept a 0" change seems unrelated & should be split into a prep cleanup / refactoring patch. On the one hand it's sensible, on the other nobody cares about having a command named "0" in their path (or a hostname), so I think it's fine to have the ||= Perl idiom leak out here.

But also, this just seems like confusing logic. Per your docs "your sendmailCommand has precedence over smtpServer.".

Why not make this "if not $sendmail_command" part of the top-level check here (the if this one is nested under), which is only done if $smtp_sever is not defined, if $sendmail_command is defined we don't care about $smtp_server later on, no?

Show 20 quoted lines
>  if ($compose && $compose > 0) {
> @@ -1490,14 +1497,15 @@ sub send_message {
>  
>  	unshift (@sendmail_parameters, @smtp_server_options);
>  
> +	if (file_name_is_absolute($smtp_server)) {
> +		# Preserved for backward compatibility
> +		$sendmail_command ||= $smtp_server;
> +	}
> +
>  	if ($dry_run) {
>  		# We don't want to send the email.
> -	} elsif (file_name_is_absolute($smtp_server)) {
> -		my $pid = open my $sm, '|-';
> -		defined $pid or die $!;
> -		if (!$pid) {
> -			exec($smtp_server, @sendmail_parameters) or die $!;
> -		}
> +	} elsif (defined $sendmail_command) {
> +		open my $sm, '|-', "$sendmail_command @sendmail_parameters";

Can we really not avoid moving from exec-as-list so Perl quotes everything, to doing our own interpolation here? It looks like the tests don't check arguments with whitespace (which should fail with this change).

>  		print $sm "$header\n$message";
>  		close $sm or die $!;
>  	} else {

I've just skimmed the previous thread, so forgive me if this was brought up.

I for one would be fine with just using --smtp-server and not adding an --sendmail-command, and doing this by simply doing an exec() on whatever the user specifies.

If it's an absolute path and an executable command, OK. If it's a command name we find in $PATH, OK, or other valid shell whatever. You can use $? to distinguish between a failed and nonexisting command.

If not exec() will return and we continue resolving it as a hostname/IP address/whatever. We'll have a conflict if someone has a command in their $PATH called gmail.com or whatever, but really. Who does that?

Maybe it's way too nasty. This design is also fine, just a suggestion.
Show 18 quoted lines
> @@ -1592,14 +1600,14 @@ 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_command) {
> +			print "Sendmail: $sendmail_command ".join(' ',@sendmail_parameters)."\n";
> +		} else {
>  			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";
>  		}
>  		print $header, "\n";
>  		if ($smtp) {

Minor nit: Let's just continue to use "if (!" here to keep the diff minimal or split up such refactoring into another change...

Gregory Anders· May 12, 2021, 13:18 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Wed, 12 May 2021 11:04 +0200, Ævar Arnfjörð Bjarmason wrote:
Show 29 quoted lines
>
>On Tue, May 11 2021, Gregory Anders wrote:
>
>> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
>> index 93708aefea..d9fe8cb7c0 100644
>> --- 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.
>
>Please don't include word-wrapping for unrelated changes in the main
>patch.

My mistake, this has been pointed out to me multiple times now. I'll remove it in the next revision and I'll be sure to avoid this in the future.

Show 21 quoted lines
>
>> -	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
>> +
>> +	if (!defined $sendmail_command) {
>> +		$smtp_server = 'localhost'; # could be 127.0.0.1, too... *shrug*
>> +	}
>>  }
>
>This "let's not accept a 0" change seems unrelated & should be split
>into a prep cleanup / refactoring patch. On the one hand it's sensible,
>on the other nobody cares about having a command named "0" in their path
>(or a hostname), so I think it's fine to have the ||= Perl idiom leak
>out here.
>
>But also, this just seems like confusing logic. Per your docs "your
>sendmailCommand has precedence over smtpServer.".
>
>Why not make this "if not $sendmail_command" part of the top-level check
>here (the if this one is nested under), which is only done if
>$smtp_sever is not defined, if $sendmail_command is defined we don't
>care about $smtp_server later on, no?

I mostly left this the way it is to minimize the diff, as this is the style the code was already written in. I agree that explicitly checking whether sendmail_command is undefined is probably clearer to the reader.

Show 27 quoted lines
>
>>  if ($compose && $compose > 0) {
>> @@ -1490,14 +1497,15 @@ sub send_message {
>>
>>  	unshift (@sendmail_parameters, @smtp_server_options);
>>
>> +	if (file_name_is_absolute($smtp_server)) {
>> +		# Preserved for backward compatibility
>> +		$sendmail_command ||= $smtp_server;
>> +	}
>> +
>>  	if ($dry_run) {
>>  		# We don't want to send the email.
>> -	} elsif (file_name_is_absolute($smtp_server)) {
>> -		my $pid = open my $sm, '|-';
>> -		defined $pid or die $!;
>> -		if (!$pid) {
>> -			exec($smtp_server, @sendmail_parameters) or die $!;
>> -		}
>> +	} elsif (defined $sendmail_command) {
>> +		open my $sm, '|-', "$sendmail_command @sendmail_parameters";
>
>Can we really not avoid moving from exec-as-list so Perl quotes
>everything, to doing our own interpolation here? It looks like the tests
>don't check arguments with whitespace (which should fail with this
>change).
>

Shell interpolation in this case is considered a feature, not a bug, i.e. we want to provide users the ability to use arbitrary shell expressions (as they do in e.g. aliases) or pass arguments. I also modeled this after the implementation for --to-cmd and --cc-cmd, which also run their respective commands in the shell just like this.

Also, this *did* cause the tests to fail since the tests write output to a path with a space in it. You'll notice that in the diff for the tests I had to wrap '$(pwd)/fake.sendmail' in additional quotes to resolve this.

Show 21 quoted lines
>> @@ -1592,14 +1600,14 @@ 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_command) {
>> +			print "Sendmail: $sendmail_command ".join(' ',@sendmail_parameters)."\n";
>> +		} else {
>>  			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";
>>  		}
>>  		print $header, "\n";
>>  		if ($smtp) {
>
>Minor nit: Let's just continue to use "if (!" here to keep the diff
>minimal or split up such refactoring into another change...
Sure, I can do that.
Thanks for your feedback.
Gregory Anders· May 13, 2021, 02:32 UTC · re: Gregory Anders · lore

[PATCH v2] git-send-email: add option to specify sendmail command

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>
---
 Documentation/git-send-email.txt | 26 +++++++++++++++++++-------
 git-send-email.perl              | 29 ++++++++++++++++++++++-------
 t/t9001-send-email.sh            | 31 +++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 14 deletions(-)
Show changes to 3 files +72 −14

Documentation/git-send-email.txt, git-send-email.perl, t/t9001-send-email.sh

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 93708aefea..f1e685a52c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -167,6 +167,15 @@ 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 compatible with `sendmail` (specifically, it should 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 +220,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..cbd9f89060 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,15 @@ 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 "$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 +1603,18 @@ 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";
+			if (!defined $sendmail_cmd) {
+				$sendmail_cmd = $smtp_server;
+			}
+
+			print "Sendmail: $sendmail_cmd ".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 &&
 
-- 
2.31.1
Junio C Hamano· May 13, 2021, 03:58 UTC · re: Gregory Anders · lore

Re: [PATCH v2] git-send-email: add option to specify sendmail command

Gregory Anders <greg@gpanders.com> writes:
Show 31 quoted lines
> 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>
> ---
Quite well explained.
Show 16 quoted lines
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 93708aefea..f1e685a52c 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -167,6 +167,15 @@ 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 compatible with `sendmail` (specifically, it should 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.
OK.
Show 24 quoted lines
>  --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 +220,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.
Two comments here:
 - The paragraph would probably not render well, unless you replace
   the blank "paragraph break" line before it with a line that
   consists of a sole '+', and dedent the paragraph body.
 - The way the "-i" option is mentioned is different from the one we saw
   earlier for `--sendmail-cmd` and might risk puzzling the users if
   the requirement is subtly different.
Show 17 quoted lines
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 175da07d94..cbd9f89060 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> ...
> @@ -1492,11 +1499,15 @@ 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 "$sendmail_cmd @sendmail_parameters" or die $!;

This looks problematic, as @sendmail_parameters is computed like this:

	my @sendmail_parameters = ('-i', @recipients);
	...
	$raw_from = extract_valid_address($raw_from);
	unshift (@sendmail_parameters,
			'-f', $raw_from) if(defined $envelope_sender);
	...
       	unshift (@sendmail_parameters, @smtp_server_options);

Notice that nothing quotes its elements for the shell, and it is natural if we think about the original use of this array---it is to be fed to the array form of exec($cmd, @args).

@recipients, and $raw_from come from extract_valid_address(), which gives 'add@ress' for "Human readable name <add@ress>", and it may be rare (but possible) to have a problematic characer in them. But the elements of @smtp_server_options can be anything, and because the values the end users already have in their configuration files are designed to be used in the original "exec ($smtp_server, @sendmail_parameters)" codepath, they would not be quoted for the shell, and they should not be treated differently in the new codepath.

In short, it is far from sufficient to just "$concatenate @variables" to form a single string. $sendmail_cmd should be left as-is (after all, we do want the shell to split it at $IFS whitespace into tokens), but each element of @sendmail_parameters should be protected from the shell (both word splitting and $interpolation rules). Perhaps something along the lines of this instead?

    exec ("sh", "-c", "$sendmail_cmd \"\$\@\"", "-", @sendmail_parameters);
> +			} else {
> +				exec ($smtp_server, @sendmail_parameters) or die $!;
> +			}
Other than that, looking good.
Thanks.
Gregory Anders· May 13, 2021, 13:31 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] git-send-email: add option to specify sendmail command

On Thu, 13 May 2021 12:58 +0900, Junio C Hamano wrote:
Show 8 quoted lines
>In short, it is far from sufficient to just "$concatenate @variables"
>to form a single string.  $sendmail_cmd should be left as-is (after
>all, we do want the shell to split it at $IFS whitespace into tokens),
>but each element of @sendmail_parameters should be protected from
>the shell (both word splitting and $interpolation rules).  Perhaps
>something along the lines of this instead?
>
>    exec ("sh", "-c", "$sendmail_cmd \"\$\@\"", "-", @sendmail_parameters);

Does this pose a problem for platforms such as Windows that don't have a 'sh' (not sure if there are any others)? Is git-send-email meant to support Windows?

Junio C Hamano· May 13, 2021, 21:21 UTC · re: Gregory Anders · lore

Re: [PATCH v2] git-send-email: add option to specify sendmail command

Gregory Anders <greg@gpanders.com> writes:
Show 13 quoted lines
> On Thu, 13 May 2021 12:58 +0900, Junio C Hamano wrote:
>>In short, it is far from sufficient to just "$concatenate @variables"
>>to form a single string.  $sendmail_cmd should be left as-is (after
>>all, we do want the shell to split it at $IFS whitespace into tokens),
>>but each element of @sendmail_parameters should be protected from
>>the shell (both word splitting and $interpolation rules).  Perhaps
>>something along the lines of this instead?
>>
>>    exec ("sh", "-c", "$sendmail_cmd \"\$\@\"", "-", @sendmail_parameters);
>
> Does this pose a problem for platforms such as Windows that don't have
> a 'sh' (not sure if there are any others)? Is git-send-email meant to 
> support Windows?

Seeing what run-command.c::prepare_shell_cmd() does under GIT_WINDOWS_NATIVE (or on other platforms), I doubt the construct would be a problem. Our Windows experts would certainly chime in if it is.

Thanks.
Gregory Anders· May 13, 2021, 15:23 UTC · re: Gregory Anders · lore

[PATCH v3] git-send-email: add option to specify sendmail command

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(-)
Show changes to 3 files +76 −14

Documentation/git-send-email.txt, git-send-email.perl, t/t9001-send-email.sh

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 &&
 
-- 
2.31.1
Junio C Hamano· May 14, 2021, 04:25 UTC · re: Gregory Anders · lore

Re: [PATCH v3] git-send-email: add option to specify sendmail command

Gregory Anders <greg@gpanders.com> writes:
Show 12 quoted lines
> 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.

Show 18 quoted lines
> 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.

Show 9 quoted lines
> +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?
Show 9 quoted lines
> +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· May 14, 2021, 05:16 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] git-send-email: add option to specify sendmail command

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.
Gregory Anders· May 14, 2021, 14:12 UTC · re: Junio C Hamano · lore

Re: [PATCH v3] git-send-email: add option to specify sendmail command

On Fri, 14 May 2021 13:25 +0900, Junio C Hamano wrote:
Show 7 quoted lines
>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.

Show 16 quoted lines
>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).

Show 30 quoted lines
>> +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
Gregory Anders· May 14, 2021, 15:15 UTC · re: Gregory Anders · lore

[PATCH v4] git-send-email: add option to specify sendmail command

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>
---
 Documentation/git-send-email.txt | 25 ++++++++++++++++-------
 git-send-email.perl              | 34 +++++++++++++++++++++++++-------
 t/t9001-send-email.sh            | 31 +++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+), 14 deletions(-)
Show changes to 3 files +76 −14

Documentation/git-send-email.txt, git-send-email.perl, t/t9001-send-email.sh

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..d771eb9297 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 &&
 
-- 
2.31.1.576.g1878d50f81

← back to recent threads