Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*

[Jakub Narebski <jnareb@gmail.com>]

Junio C Hamano wrote:
> 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. 
 
>> 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...
 
> 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.
 
> 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