Re: [PATCH] git-send-email: add sendmailCommand option
- From
Gregory Anders <greg@gpanders.com>
- Date
- May 12, 2021, 13:18 UTC
- Message-ID
- <YJvVjLTFsES2i8a0@gpanders.com>
- In-Reply-To
- <87y2cks3lt.fsf@evledraar.gmail.com>
On Wed, 12 May 2021 11:04 +0200, Ævar Arnfjörð Bjarmason wrote:
Show 29 quoted lines
> >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.
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.
Show 21 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?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.
Show 27 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).
>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.
Show 21 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...Sure, I can do that.
Thanks for your feedback.