From: Jakub Narebski <jnareb@gmail.com>
Date: Fri, 09 Apr 2010 16:31:48 GMT
Subject: Re: [PATCH v2 1/3] send-email: Don't use FQDNs without a '.'
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.
 
> @@ -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 =~ /\./;
+}


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