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

Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

quoted

The real problem was here.
...

quoted

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

quoted

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