From: Gregory Anders Date: Wed, 12 May 2021 13:18:04 GMT Subject: Re: [PATCH] git-send-email: add sendmailCommand option Message-ID: In-Reply-To: <87y2cks3lt.fsf@evledraar.gmail.com> On Wed, 12 May 2021 11:04 +0200, Ævar Arnfjörð Bjarmason wrote: > >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=
:: >> - 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. My mistake, this has been pointed out to me multiple times now. I'll remove it in the next revision and I'll be sure to avoid this in the future. > >> - $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? I mostly left this the way it is to minimize the diff, as this is the style the code was already written in. I agree that explicitly checking whether sendmail_command is undefined is probably clearer to the reader. > >> 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). > Shell interpolation in this case is considered a feature, not a bug, i.e. we want to provide users the ability to use arbitrary shell expressions (as they do in e.g. aliases) or pass arguments. I also modeled this after the implementation for --to-cmd and --cc-cmd, which also run their respective commands in the shell just like this. Also, this *did* cause the tests to fail since the tests write output to a path with a space in it. You'll notice that in the diff for the tests I had to wrap '$(pwd)/fake.sendmail' in additional quotes to resolve this. >> @@ -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... Sure, I can do that. Thanks for your feedback.