On Sun, Apr 19, 2009 at 20:41, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Michael Witten <mfwitten@xxxxxxxxx> 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.
> 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;
}
> 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html