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

[Junio C Hamano <gitster@pobox.com>]

Jakub Narebski <jnareb@gmail.com> writes:

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

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

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