Re: [PATCH RFC3.5 02/12] send-email: No longer repeatedly test if $smtp_server is a command

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 20, 2009, 01:41 UTC
Message-ID: <7veivop0dx.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1240074128-16132-3-git-send-email-mfwitten@gmail.com>

Michael Witten <mfwitten@gmail.com> writes:

Show 21 quoted lines

> This is a minor change, but it's cleaner, and it sets up the
> $smtp_server initialization code for future improvements.
> ...
> -if (!defined $smtp_server) {
> +if (defined $smtp_server) {
> +
> +	$smtp_server_is_a_command = ($smtp_server =~ m{^/});
> +
> +} else { # use a default:
> +
>  	foreach (qw( /usr/sbin/sendmail /usr/lib/sendmail )) {
>  		if (-x $_) {
>  			$smtp_server = $_;
> +			$smtp_server_is_a_command = 1;
>  			last;
>  		}
>  	}
> -	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
> +
> +	$smtp_server = 'localhost'; # 127.0.0.1 is not compatible with IPv6
> +		unless $smtp_server_is_a_command;

Nobody suggests to use 127.0.0.1 anymore with this change, so why not just get rid of that comment?

Also the new statement looks wrong.

 (1) you have ';' after assignment before the statement modifier "unless";
     I do not think you meant it.  I generally *dis*like statement
     modifiers, but if you use it, at least please use it correctly.

 (2) earlier, when $smtp_server is defined (say, the name of your smtp
     host) but is not a command, we did not set smtp_server to
     'localhost', but kept the value given by the user.  Now you seem to
     kill the user's wish with this change.

I think a genuine improvement would be something like:

	if (!defined $smtp_server) {
        	$smtp_server = 'localhost';
	}

Of course if you are writing for a project that is "5.8.1 or later only", you could say:

	$smtp_server //= 'localhost';