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

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

[Gregory Anders]

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

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

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

[Junio C Hamano]

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,

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

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

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

> +--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'?

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

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

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

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

[Felipe Contreras]

Gregory Anders wrote:
> --- 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.

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

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

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

[Ævar Arnfjörð Bjarmason]

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.

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

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

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

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

[Gregory Anders]

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.

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

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

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

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

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.

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

[Gregory Anders]

On Wed, 12 May 2021 02:57 -0500, Felipe Contreras wrote:
>Gregory Anders wrote:
>I'm not against these kinds of changes but it took me one minute to
>figure out all you did was change the format.
>
>This belongs in a separate patch.

Yes this was pointed out by the other reviewers as well, I'll omit it in 
subsequent revisions.

>
>> +--sendmail-cmd=<command>::
>
>Oh no no no. Don't do shortcuts.
>
>If you think --sendmail-command is too long, then address that problem
>head on, don't try to hide it.
>
>I do think it's too long, which is why I suggested --command (especially
>since it's obvious which command we are talking about), but I wouldn't
>suggest --sdm-command, or something of that sort. We have to own our
>decisions.
>
>  1. --command
>  2. --sendmail
>  3. --sendmail-command
>
>We have to pick one. I suggest #1.
>
>To try to make #3 shorter is just shoving the problem under the rug.

The intention behind `--sendmail-cmd` was consistency with `--to-cmd` 
and `--cc-cmd`. Though both of those options also use the condensed 
'cmd' form in their configuration options as well, so I suppose I should 
also change this one to 'sendemail.sendmailcmd'.

I'm not opposed to just '--sendmail' and 'sendemail.sendmail' either. I 
personally believe --sendmail-cmd is the clearest, even if it's verbose, 
but I'll concede to whatever consensus we arrive at.

>
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -70,6 +70,7 @@ sub usage {
>>
>>    Sending:
>>      --envelope-sender       <str>  * Email envelope sender.
>> +    --sendmail-cmd          <str>  * Shell command to run to send email.
>>      --smtp-server       <str:int>  * Outgoing SMTP server to use. The port
>>                                       is optional. Default 'localhost'.
>>      --smtp-server-option    <str>  * Outgoing SMTP server option to use.
>> @@ -252,6 +253,7 @@ sub do_edit {
>>  my (@suppress_cc);
>>  my ($auto_8bit_encoding);
>>  my ($compose_encoding);
>> +my ($sendmail_command);
>>  # Variables with corresponding config settings & hardcoded defaults
>>  my ($debug_net_smtp) = 0;		# Net::SMTP, see send_message()
>>  my $thread = 1;
>> @@ -299,6 +301,7 @@ sub do_edit {
>>      "assume8bitencoding" => \$auto_8bit_encoding,
>>      "composeencoding" => \$compose_encoding,
>>      "transferencoding" => \$target_xfer_encoding,
>> +    "sendmailcommand" => \$sendmail_command,
>>  );
>>
>>  my %config_path_settings = (
>> @@ -432,6 +435,7 @@ sub read_config {
>>  		    "no-bcc" => \$no_bcc,
>>  		    "chain-reply-to!" => \$chain_reply_to,
>>  		    "no-chain-reply-to" => sub {$chain_reply_to = 0},
>> +		    "sendmail-cmd=s" => \$sendmail_command,
>
>Isn't it interesting that to make the code readable you picked
>$sendmail_command, but you don't want users to type so much, even if
>it's more readable?

See my note above.

>
>> --- a/t/t9001-send-email.sh
>> +++ b/t/t9001-send-email.sh
>> @@ -57,7 +57,7 @@ test_no_confirm () {
>>  		git send-email \
>>  		--from="Example <from@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

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

[Gregory Anders]

On Wed, 12 May 2021 11:04 +0200, Ævar Arnfjörð Bjarmason wrote:
>
>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.

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

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

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

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

[Felipe Contreras]

Gregory Anders wrote:
> >> +--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.

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

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

[Gregory Anders]

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:

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

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

[Felipe Contreras]

Gregory Anders wrote:
> 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

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

[Gregory Anders]

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

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

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

[Junio C Hamano]

Gregory Anders <greg@gpanders.com> writes:

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

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

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

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

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

[Gregory Anders]

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?

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

[Gregory Anders]

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

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

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

[Junio C Hamano]

Gregory Anders <greg@gpanders.com> writes:

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

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

[Junio C Hamano]

Gregory Anders <greg@gpanders.com> writes:

> Use a block scoped variable to print the sendmail invocation at the end 
> of the 'send_message' subroutine. Assigning directly to $sendmail_cmd 
> (as in the v2 patch) causes some bizarre problems; namely, it seems to 
> affect the value of $sendmail_cmd that is read at earlier points in the 
> same subroutine, which causes test invocations of the form
>
>     git send-email --smtp-server="$(pwd)/fake.sendmail"
>
> to fail. The value passed to --smtp-server was assigned to $sendmail_cmd 
> at the end of the 'send_message' subprocedure, but somehow this caused 
> the 'if (defined $sendmail_cmd)' condition earlier in the subproc to 
> evaluate to true.

Are you talking about the use of $sm that is local to the debug
output?  I think leaving $sendmail_cmd intact by using a separate
variable is the right choice.  Isn't the problem you observed a
consequence of send_message() getting called once for each message,
so assigning to $sendmail_cmd in the function for the first
invocation of the function would change its value for the second
invocation?

Also, if we have been using

	--smtp-server=$(pwd)/fake.sendmail

we cannot expect to use the same value like this:

	--sendmail-cmd=$(pwd)/fake.sendmail

because we deliberately add a space in the $(pwd) by choosing the
name of the test directory to be "trash directory.something".  We'd
need to do something like

	--sendmail-cmd='$(pwd)/fake.sendmail'

so that the shell sees '$(pwd)/fake.sendmail' literally and runs pwd
to find out what the path to the program is, I would think.

> diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
> index 65b3035371..583fbba410 100755
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -2148,6 +2148,37 @@ test_expect_success $PREREQ 'leading and trailing whitespaces are removed' '
>  	test_cmp expected-list actual-list
>  '
>  
> +test_expect_success $PREREQ 'test using command name with --sendmail-cmd' '
> +	clean_fake_sendmail &&
> +	PATH="$(pwd):$PATH" \
> +	git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--sendmail-cmd="fake.sendmail" \
> +		HEAD^ &&
> +	test_path_is_file commandline1
> +'

Nice demonstration of the "we no longer need an absolute pathname"
feature.

> +test_expect_success $PREREQ 'test using arguments with --sendmail-cmd' '
> +	clean_fake_sendmail &&
> +	git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--sendmail-cmd="\"$(pwd)/fake.sendmail\" -f nobody@example.com" \
> +		HEAD^ &&
> +	test_path_is_file commandline1
> +'

Hmph, if $(pwd) has a double quote character in it, this may not
work as expected, as the shell that is expanding the command line
arguments for "git send-email" would see $(pwd), expand it and our
program will see

    "/path/with/d"quote/git/t/trash directory.9001/fake.sendmail" -f nobody@e.c

as the value of --sendmail-cmd, which would not interpolate well,
no?

We want the shell that eats the command line of 'git send-email' to see

	--sendmail-cmd='$(pwd)/fake.sendmail'\" -f nobody@example.com"

and because this is inside a sq pair, it would become

	--sendmail-cmd='\''$(pwd)/fake.sendmail'\''\" -f nobody@example.com"

after we replace each sq with '\'', or something like that, perhaps?

> +test_expect_success $PREREQ 'test shell expression with --sendmail-cmd' '
> +	clean_fake_sendmail &&
> +	git send-email \
> +		--from="Example <nobody@example.com>" \
> +		--to=nobody@example.com \
> +		--sendmail-cmd="f() { \"$(pwd)/fake.sendmail\" \"\$@\"; };f" \
> +		HEAD^ &&
> +	test_path_is_file commandline1
> +'

Nice demonstration of how a bit of scripting can be used.

>  test_expect_success $PREREQ 'invoke hook' '
>  	mkdir -p .git/hooks &&

Thanks.

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

[Junio C Hamano]

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.

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

[Gregory Anders]

On Fri, 14 May 2021 13:25 +0900, Junio C Hamano wrote:
>Are you talking about the use of $sm that is local to the debug
>output?  I think leaving $sendmail_cmd intact by using a separate
>variable is the right choice.  Isn't the problem you observed a
>consequence of send_message() getting called once for each message,
>so assigning to $sendmail_cmd in the function for the first
>invocation of the function would change its value for the second
>invocation?

Yes that is right. That makes sense. I didn't realize the subprocess was 
called twice, though that is such an obvious explanation I don't know 
why I didn't think of it.

>Also, if we have been using
>
>	--smtp-server=$(pwd)/fake.sendmail
>
>we cannot expect to use the same value like this:
>
>	--sendmail-cmd=$(pwd)/fake.sendmail
>
>because we deliberately add a space in the $(pwd) by choosing the
>name of the test directory to be "trash directory.something".  We'd
>need to do something like
>
>	--sendmail-cmd='$(pwd)/fake.sendmail'
>
>so that the shell sees '$(pwd)/fake.sendmail' literally and runs pwd
>to find out what the path to the program is, I would think.

Indeed, in prior versions of this patch I had replaced the uses of 
`--smtp-server` in the test suite with `--sendmail-cmd` which included 
those extra quotes (I reverted back to using --smtp-server after 
feedback from other reviewers in lieu of simply adding new test cases 
for --sendmail-cmd).

>> +test_expect_success $PREREQ 'test using arguments with 
>> --sendmail-cmd' '
>> +	clean_fake_sendmail &&
>> +	git send-email \
>> +		--from="Example <nobody@example.com>" \
>> +		--to=nobody@example.com \
>> +		--sendmail-cmd="\"$(pwd)/fake.sendmail\" -f nobody@example.com" \
>> +		HEAD^ &&
>> +	test_path_is_file commandline1
>> +'
>
>Hmph, if $(pwd) has a double quote character in it, this may not
>work as expected, as the shell that is expanding the command line
>arguments for "git send-email" would see $(pwd), expand it and our
>program will see
>
>    "/path/with/d"quote/git/t/trash directory.9001/fake.sendmail" -f nobody@e.c
>
>as the value of --sendmail-cmd, which would not interpolate well,
>no?
>
>We want the shell that eats the command line of 'git send-email' to see
>
>	--sendmail-cmd='$(pwd)/fake.sendmail'\" -f nobody@example.com"
>
>and because this is inside a sq pair, it would become
>
>	--sendmail-cmd='\''$(pwd)/fake.sendmail'\''\" -f nobody@example.com"
>
>after we replace each sq with '\'', or something like that, perhaps?

I'll take a look at this (as well as your followup email) and send a new 
version.

Thanks,

Greg

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

[Gregory Anders]

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

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