On Sun, Apr 19, 2009 at 23:21, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Michael Witten <mfwitten@xxxxxxxxx> 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.
--
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