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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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