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.
next prev parent 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).