Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*
Show 25 quoted lines
> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> The real problem was here.
>>> ...
>>>> - my $val = command_oneline(@cmd);
>>>> - return undef unless defined $val;
>>>> - return $val eq 'true';
>>>> ...
>>>> - };
>>>> + my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
>>>> + return (defined $val && $val eq 'true');
>>>> }
>>>
>>> Can you spot the difference?
>>
>> Damn.
>
> For people following from the sideline, the difference that bit us was
> that
>
> return $val eq 'true';
>
> yields "" (not undef) for false, but some callers care the distinction
> between undef kind of false and other kinds of false. It is not really the
> fault of the language per-se, but still... And Git::config* family of commands return 'undef' if key was not found.
So Git::config_bool() can return true, false or undef, and the last part
is important.
Show 14 quoted lines
>> What I have noticed is that there is slight difference between original
>> Git::config_path and the one after refactoring. In the version before
>> this patch the error catching part of config_path() looks like this:
>> ...
>> return undef;
>> ...
>> while after this patch (and in config()) it looks like this:
>> ...
>> return;
>> ...
>>
>> I am not sure which one is right, but I suspect the latter.
>
> This is Perl---"return;" returns undef, so there is no right or wrong.
Actually this is not true: "return;" returns undef in scalar context,
and empty list in list context. This means that result always evaluates
to false...
Show 9 quoted lines
> Having said that, I tend to prefer being explicit so that people not so
> familiar with the language do not have to waste time wondering about such
> differences. Especially where it matters, like this case where some
> callers may care about different kinds of falsehood.
>
> That is another reason I tend to hate the kind of "this makes it more
> Perl-ish" changes, as they tend to force readers to spend extra brain
> cells to see what is going on. I'd rather spell things more explicit,
> especially when the distinction matters.
...and that is why "return;" is more Perl-ish, and usually safer.
There might be exceptions where we want to return one-element list
containing single undef element in list context, but I guess they
are exceedingly rare.
Show 13 quoted lines
> I've already pushed out the fixed one as 6942a3d (libperl-git: refactor
> Git::config_*, 2011-10-18), with this bit:
>
> - ...
> - my $val = command_oneline(@cmd);
> - return undef unless defined $val;
> + # Do not rewrite this as return (defined $val && $val eq 'true')
> + # as some callers do care what kind of falsehood they receive.
> + if (!defined $val) {
> + return undef;
> + } else {
> return $val eq 'true';
> ...I like mine better... ;-))))
--
Jakub Narebski
Poland
Jakub Narebski <jnareb@gmail.com>