Re: [PATCH] git-send-email: add sendmailCommand option
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-05-12 09:28:35
On Tue, May 11 2021, Gregory Anders wrote:
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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...