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=<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.
- $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.
