Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.'

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 9, 2010, 16:31 UTC
Message-ID: <201004091831.49066.jnareb@gmail.com>
In-Reply-To: <1270827746-29229-1-git-send-email-brian@gernhardtsoftware.com>

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.

Show 9 quoted lines

> @@ -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 =~ /\./; +}

Show 23 quoted lines

> +
>  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