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

Jakub Narebski [off-list ref] writes:

quoted

Well, this breaks t9001 and I ended up spending an hour and half figuring
out why. Admittedly, I was doing something else on the side, so it is not
like the whole 90 minutes is the billable hour for reviewing this patch,
but as far as the Git project is concerned, I didn't have any other branch
checked out in my working tree, so that whole time was what ended up
costing.

I'm sorry about that.

No need to be sorry. After all you just re-sent a throw-away patch with
minor tweaks, but unfortunately a tricky Perl script sometimes needs line
by line inspection with fine toothed comb to avoid regression.

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

quoted

This is the reason why I do not particularly like a rewrite for the sake
of rewriting.

The goal was to reduce code duplication to _avoid_ errors.

That is exactly why I said I do not like a rewrite for such purpose.

Once the end result of smaller code is done correctly, it would help
avoiding future errors, but people tend to think unconsciously that the
change to reduce code duplication is much safer than adding new code.
Upon receiving such a patch, without knowing that it was not done with
enough attention to detail with fine toothed comb, it is me who ends up
spending nontrivial amount of time fixing the breakage. These "clean-ups"
are not cheap.

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.

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.

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

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