Jakub Narebski <jnareb@xxxxxxxxx> 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'; ... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html