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

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

Show 26 quoted lines

> 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;
>
> Nobody suggests to use 127.0.0.1 anymore with this change, so why not just
> get rid of that comment?

Fine with me.

Show 5 quoted lines

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

How embarassing. That's actually been fixed on my end since I sent that patch; for some reason, I forget to send the update; sorry for wasting your time.

>  (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 you misread the code (with the exception of the error on my part). The code could be read:

	if $smtp_server is already defined {
		determine whether it is a command;
	} else {
		find a suitable default for it;
	}

Show 5 quoted lines

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