git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Gregory Anders <greg@gpanders.com>
Cc: git@vger.kernel.org,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2] git-send-email: add option to specify sendmail command
Date: Thu, 13 May 2021 12:58:36 +0900	[thread overview]
Message-ID: <xmqq8s4jcmj7.fsf@gitster.g> (raw)
In-Reply-To: <20210513023212.72221-1-greg@gpanders.com> (Gregory Anders's message of "Wed, 12 May 2021 20:32:11 -0600")

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.

  reply	other threads:[~2021-05-13  3:58 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-12  3:30 [PATCH] git-send-email: add sendmailCommand option Gregory Anders
2021-05-12  4:19 ` Junio C Hamano
2021-05-12 13:03   ` Gregory Anders
2021-05-12  7:57 ` Felipe Contreras
2021-05-12 13:12   ` Gregory Anders
2021-05-12 17:21     ` Felipe Contreras
2021-05-12 18:06       ` Gregory Anders
2021-05-12 19:32         ` Felipe Contreras
2021-05-12  9:04 ` Ævar Arnfjörð Bjarmason
2021-05-12 13:18   ` Gregory Anders
2021-05-13  2:32 ` [PATCH v2] git-send-email: add option to specify sendmail command Gregory Anders
2021-05-13  3:58   ` Junio C Hamano [this message]
2021-05-13 13:31     ` Gregory Anders
2021-05-13 21:21       ` Junio C Hamano
2021-05-13 15:23   ` [PATCH v3] " Gregory Anders
2021-05-14  4:25     ` Junio C Hamano
2021-05-14  5:16       ` Junio C Hamano
2021-05-14 14:12       ` Gregory Anders
2021-05-14 15:15     ` [PATCH v4] " Gregory Anders

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq8s4jcmj7.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=greg@gpanders.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).