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, 04:21 UTC
Message-ID: <7viql0lzuw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <b4087cc50904191937x55f94dc3sb6fbda27f380b105@mail.gmail.com>

Michael Witten <mfwitten@gmail.com> writes:

Show 32 quoted lines

> On Sun, Apr 19, 2009 at 20:41, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael Witten <mfwitten@gmail.com> writes:
>>
>>> 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;
> ...
>> 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;
	}