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

[Jakub Narebski <jnareb@gmail.com>]

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

> ---
>  git-send-email.perl |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 70 insertions(+), 1 deletions(-)
> 
> 
> 
>     ================================
>     This is REVISION SET 4, reworked
>     ================================
 
Here you should describe differences from v3 (from previous version),
for easier review.
 
> +# This maildomain*() code is based on ideas in Perl library Test::Reporter
> +# /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain ()

Nice... although it might be better to use Test::Reporter::Mail::Util
as canonical module name (the package can be installed somewhere else,
depending on operating system / distribution, and if one uses
local::lib for local / per-user installation).

> +sub maildomain_net
> +{
> +	my $maildomain;
> +
> +	if (eval { require Net::Domain; 1 }) {
> +		eval "use Net::Domain";
> +		unless ($@) {
> +		    my $domain = Net::Domain::domainname();
> +		    $maildomain = $domain
> +			    unless $^O eq 'darwin' && $domain =~ /\.local$/;


Here should be a comment 'following what Test::Reporter does' or
something like that.

> +		}
> +	}
> +
> +	return $maildomain;
> +}

You still have duplicated 'require' ('use' is 'require' + 'import')
and check if it succeeded.  It should read:

+sub maildomain_net {
+	my $maildomain;
+
+	if (eval { require Net::Domain; 1 }) {
+		my $domain = Net::Domain::domainname();
+		$maildomain = $domain
+			unless $^O eq 'darwin' && $domain =~ /\.local$/;
+	}
+
+	return $maildomain;
+}

In the subroutine below you do not duplicate check for require.

Sidenote: alternate soultion would be to write (with one less level of
indent):

+sub maildomain_net {
+	my $maildomain;
+
+	eval { require Net::Domain; };
+	return if $@;
+
+	$maildomain = Net::Domain::domainname();
+		unless $^O eq 'darwin' && $domain =~ /\.local$/;
+
+	return $maildomain;
+}


> +
> +sub maildomain_mta
> +{

Use the same Perl convention that used elsewhere in git-send-email.perl
(this is usually used Perl style).

+sub maildomain_mta {

> +	my $maildomain;
> +
> +	if (eval { require Net::SMTP; 1 }) {
> +		for my $host (qw(mailhost localhost)) {
> +			my $smtp = Net::SMTP->new($host);
> +			if (defined $smtp) {
> +				my $domain = $smtp->domain;
> +				$smtp->quit;
> +
> +				$maildomain = $domain
> +					unless $^O eq 'darwin' && $domain =~ /\.local$/;
> +
> +				last if $maildomain;
> +			}
> +		}
> +	}
> +
> +	return $maildomain;
> +}
> +
> +sub maildomain
> +{
> +	return maildomain_net() || maildomain_mta() || $mail_domain_default;
> +}

Nice.

-- 
Jakub Narebski
Poland
ShadeHawk on #git