Gregory Anders <greg@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
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.
