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.
> - $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?
> 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.
> @@ -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...
