Jari Aalto <jari.aalto@xxxxxxxxx> 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)?
>>> @@ -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.
--
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