From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Gregory Anders <greg@gpanders.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] git-send-email: add sendmailCommand option
Date: Wed, 12 May 2021 11:04:59 +0200 [thread overview]
Message-ID: <87y2cks3lt.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20210512033039.4022-1-greg@gpanders.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...
next prev parent reply other threads:[~2021-05-12 9:28 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 [this message]
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
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y2cks3lt.fsf@evledraar.gmail.com \
--to=avarab@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.
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 NNTP newsgroup(s).