Re: [PATCH] git-send-email: add sendmailCommand option
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 12, 2021, 09:04 UTC
- Message-ID
- <87y2cks3lt.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210512033039.4022-1-greg@gpanders.com>
On Tue, May 11 2021, Gregory Anders wrote:
Show 23 quoted lines
> 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.
Show 6 quoted lines
> - $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?
Show 20 quoted lines
> 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.
Show 18 quoted lines
> @@ -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...