Re: [PATCH] git-send-email.perl - Fix 550 EHLO argument does not match calling host

Jari Aalto <jari.aalto@cante.net> writes:

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@provider.example) 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.

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.

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.

Looks like a funny indent here...

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.

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.

This part is a good addition, but it needs to be in the earlier patch, no?