Re: [PATCH 3/3] git-send-email.perl - Fix 550 EHLO argument does not match calling host
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Mar 14, 2010, 19:53 UTC
- Message-ID
- <m3y6huk7f4.fsf@localhost.localdomain>
- In-Reply-To
- <87sk83aq76.fsf_-_@jondo.cante.net>
Jari Aalto <jari.aalto@cante.net> writes:
Show 9 quoted lines
> --- > 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).
Show 10 quoted lines
> +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.
Show 5 quoted lines
> + } > + } > + > + 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 {Show 24 quoted lines
> + 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