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

From: Michael Witten <mfwitten@gmail.com>
Date: Apr 20, 2009, 04:53 UTC
Message-ID: <49ec020a.050cc00a.2a50.ffffd0e3@mx.google.com>
In-Reply-To: <7viql0lzuw.fsf@gitster.siamese.dyndns.org>

On Sun, Apr 19, 2009 at 23:21, Junio C Hamano <gitster@pobox.com> wrote:

Show 40 quoted lines

> Michael Witten <mfwitten@gmail.com> writes:
>
>> ...
>>> I think a genuine improvement would be something like:
>>>
>>> 	if (!defined $smtp_server) {
>>> 		$smtp_server = 'localhost';
>>> 	}
>>
>> You don't care to search for a possible sendmail?
>
> That's something you already did before setting smtp_server
> unconditionally to localhost, right?  You do (in the above):
>
> 	if (user gave $smtp_server) {
> 		use it, notice and note if it is a command;
> 	} else {
> 		if (standard binary avaiable) {
> 			use it, note it is a command;
> 		}
> 		# otherwise it still is undef
> 	}
> 	if (!defined $smtp_server) {
> 		set it to localhost;
> 	}
>
> But I would probably write it this way:
>
> 	if (user didn't give us $smtp_server) {
> 		if (standard binary avaiable) {
> 			use it, note it is a command;
> 		} else {
> 			use localhost;
> 		}
> 	}
> 	if ($smtp_server looks like a command) {
> 		$smtp_server_is_a_command = true;
> 	}
>
>

I suppose it's hard to tell from the patch, but it's actually a combination of those two:

	if (user gave $smtp_server) {
		use it, notice and note if it is a command;
	} else { # use a default:
		if (standard binary avaiable) {
			use it, note it is a command;
		} else {
			use localhost;
			# automatically noted as a command
			# without doing anything (this would
			# cause warnings if $smtp_is_a_command
			# is used in places other than the bool
			# context, because it will be undef);
			# thus, my choice is a bad choice in
			# the long run, but I'm sticking to it.
		}
	}

The actual code:

	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' # 127.0.0.1 is not compatible with IPv6
			unless $smtp_server_is_a_command;
	}

(I'll remove the "127.0.0.1" comment).

There's a minimum of checking and assigning; it's beautiful. Plus, this organization fits in well with the server/port verification (if I recall).

P.S.

I also like:

	$smtp_server_is_a_command or ($smtp_server = 'localhost');

or, maybe:

	$smtp_server_is_a_command or $smtp_server = 'localhost;

However, I wasn't sure if that is acceptable to others; more importantly (:-D), I'm not sure that perl is smart enough to optimize away the unnecessary comparison of the values, so I figured that the modifier 'unless' is morally superior, because it probably has the advantage of fewer cycles than the 'or' form, and it has greater readability than the curly-braced conditional. I absolutely loathe curly braces around a body of one line:

	if ($you_loath_this) {
		print "Clap Your Hands!\n";
	}

If only the curly-braces weren't there. I don't know why, they just bug me terribly.