Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.'
From: Jakub Narebski <hidden>
Date: 2016-06-15 22:48:35
On Fri, 9 Apr 2010, Brian Gernhardt wrote:
Since the same condition is used twice and getting complex, let's move it to a new function.
Good idea. Note that the comments below are just nitpicking about Perl style.
quoted hunk ↗ jump to hunk
@@ -863,14 +863,19 @@ sub sanitize_address # This maildomain*() code is based on ideas in Perl library Test::Reporter # /usr/share/perl5/Test/Reporter/Mail/Util.pm ==> sub _maildomain () +sub valid_fqdn +{ + my $domain = $_[0]; + return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./; +}
A matter of style: in Perl it more usual to use
sub <name> {
...
}
style rather than
sub <name>
{
...
}
Unfortunately git-send-email.perl is a bit inconsistent in the style used;
23 subroutines use Perl style, 5 subroutines including previous one i.e.
sanitize_address use C-like style (one of).
Also, the usual way of unrolling @_; is to use either
my ($par1, $par2, ...) = @_;
or use
mu $par = shift;
The form $_[0] etc. is used very rarely. I think it is even against
Perl Best Practices (see http://www.perlcritic.org and Perl::Critic).
So in my opinion this fragment should be:
+sub valid_fqdn {
+ my $domain = shift;
+ return !($^O eq 'darwin' && $domain =~ /\.local$/) && $domain =~ /\./;
+}
quoted hunk ↗ jump to hunk
+ sub maildomain_net { my $maildomain; if (eval { require Net::Domain; 1 }) { my $domain = Net::Domain::domainname(); - $maildomain = $domain - unless $^O eq 'darwin' && $domain =~ /\.local$/; + $maildomain = $domain if valid_fqdn( $domain ); } return $maildomain;@@ -887,8 +892,7 @@ sub maildomain_mta my $domain = $smtp->domain; $smtp->quit; - $maildomain = $domain - unless $^O eq 'darwin' && $domain =~ /\.local$/; + $maildomain = $domain if valid_fqdn( $domain ); last if $maildomain; }
Style: usually there is no space around function arguments, so 'valid_fqdn($domain);'. -- Jakub Narebski Poland