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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2010, 12:09 UTC
Message-ID: <7veijnp0jl.fsf@alter.siamese.dyndns.org>
In-Reply-To: <87d3z7dx2y.fsf@jondo.cante.net>

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

> The code is based on Perl library Test::Reporter

Thanks for the pointer. Mention that somewhere in the code or commit, perhaps?

> Why FQDN is passed to MTA?

That wasn't what I was asking---you know I was aware of the need when you read the message from me you are responding to.

>> Side note.  Apparently this seems to be a common issue. ...

> It might be good to convert all global variables to de facto UPPERCASE.
> What do you think? (a separate patch?).

Look at the surrounding code in git-send-email.perl; no all-caps variable except for the language defined globals like %ENV is used there. In Perl all-caps tend to give things magic meaning (think BEGIN, $ENV{}, @ARGV,...) and I don't think it is a good idea to name your own stuff in all-caps unless there is a good reason for it. The variable being in global scope is not a good enough reason.

> It's needed in the error message (see variable's scope):

A global is visible there, isn't it (see the other review message)?

Show 10 quoted lines

>>> @@ -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?
>
> The --smtp-debug was introduced in the current patch along with maildomain().

I don't think so, at least in the submitted form. Perhaps you originally wrote a single combined commit, and later split it into a few commits.

The patch under discussion adds --smtp-domain, and would apply on top of a separate patch that adds --smtp-debug. I already said that it is a good thing to suggest using the option in die() message, but that should be done _not_ in this patch, but in the previous one that added --smtp-debug.