Jari Aalto <jari.aalto@xxxxxxxxx> writes:
>> - You are trying to improve the chance that $smtp_server likes the name
>> your side identifies as; what does it have to do with your local
>> "mailhost" or "localhost" listening to the SMTP port? These local MTA
>> may be configured for local-only delivery after all.
>
> There is now explicit option to set the name with option --mail-domain.
> Hope this address the issue.
Not quite. Imagine if you were a maintainer of a system who received a
patch that looks like adding a piece of code that computes random rubbish.
sub a_function () {
compute some value that looked not so useful nor reliable;
return it to be used;
}
When you ask the contributor why the code computes and uses it, the
contributor sends an updated patch that lets the user override the logic
and specify the exact value to be used. That is:
sub a_function () {
+ if (the user gave a value)
+ return that value;
compute some value that looked not so useful nor reliable;
return it to be used;
}
Does that updated patch answer the question you asked?
Configurability does not alleviate the puzzlement about the logic. A
better explanation would be to describe how the computation produces
reliable and correct value for the most sane installations; then the user
configurability becomes only "just in case" way to give them the last
resort.
If you don't have a good explanation, the patch should instead be like
this:
sub a_function () {
- compute some value that looked not so useful nor reliable;
- return it to be used;
+ if (the user gave a value)
+ return that value;
+ die "please configure me";
}
So please explain why asking your local MTA (either mailhost or localhost)
how it is configured to identify to other MTA is a good way to obtain the
HELO domain you should give $smtp_server. Your answer could be "I expect
that most people use $smtp_server set to 'mailhost' or 'localhost' and
have the MTA configured to talk with the outside world." (which by the way
I do not think is true for most people). Or it could be "Xsmtp MTA that
is used by many people to send out mails through their ISP's mailserver
uses the same trick to solve this issue".
Side note. Apparently this seems to be a common issue. For example,
msmtp has this configurable via command line and configuration option:
domain argument
Use this command to set the argument of the SMTP EHLO (or LMTP
LHLO) command. The default is localhost (stupid, but working).
Possible choices are the domain part of your mail address
(provider.example for joe@xxxxxxxxxxxxxxxx) or the fully quali‐
fied domain name of your host (if available).
Interestingly, one of the suggestions above is to derive it from the From
address.
In any case, I want to hear your justification.
> + --smtp-domain <str> * The domain name sent to HELO/EHLO handshake
I think this addition makes sense. I suspect that we would also want to
have sendemail.smtpdomain configuration variable. They of course need to
be documented.
> @@ -131,6 +132,7 @@ my $have_email_valid = eval { require Email::Valid; 1 };
> my $have_mail_address = eval { require Mail::Address; 1 };
> my $smtp;
> my $auth;
> +my $MAIL_DOMAIN; # See maildomain()
Why uppercase? The lifetime rule for this looks exactly the same as
existing $smtp to me, so it would be more sensible to spell it in
lowercase, $mail_domain.
> +sub maildomain
> +{
> + return $MAIL_DOMAIN if $MAIL_DOMAIN;
> +
> + my $maildomain;
Looks like a funny indent here...
> + eval "use Net::SMTP";
> +
> + unless ( $@ ) {
> + for my $host ( qw(mailhost localhost) ) {
Please drop extra SP immediately after '(' and before ')'; they are
distracting.
But as I already said, I tend to think the logic implemented by this part
is of dubious validity.
> @@ -917,6 +962,8 @@ X-Mailer: git-send-email $gitversion
> }
> }
>
> + my $maildomain;
> +
> if ($dry_run) {
> # We don't want to send the email.
> } elsif ($smtp_server =~ m#^/#) {
> @@ -936,13 +983,18 @@ X-Mailer: git-send-email $gitversion
> ...
> + $maildomain = maildomain() || "localhost.localdomain";
> + $smtp ||= Net::SMTP::SSL->new($smtp_server,
> + Hello => $maildomain,
> + Port => $smtp_server_port);
Why not use the same structure as how lifetime is defined for $smtp
variable? IOW,
$mail_domain || = maildomain();
$smtp ||= ...
without an extra local $maildomain variable? If you do so, then
- You can lose the $maildomain variable local to this function;
- "return what the user configured" does not have to be in the maildomain
sub;
- the maildomain sub can return "localhost.localdomain" as a fallback
default; and
- various callsites of maildomain sub do not have to repeat the fallback
default like your patch does.
> @@ -962,9 +1014,10 @@ X-Mailer: git-send-email $gitversion
> }
>
> if (!$smtp) {
> - die "Unable to initialize SMTP properly. Check config. ",
> + die "Unable to initialize SMTP properly. Check config and use --smtp-debug. ",
This part is a good addition, but it needs to be in the earlier patch, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html