Re: [PATCH/RFC 3/2 (fixed)] Refactor Git::config_*

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2011, 19:52 UTC
Message-ID: <7vty76f57d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201110181147.02397.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

Show 17 quoted lines

> From: Junio C Hamano <gitster@pobox.com>
>
> There is, especially with addition of Git::config_path(), much code
> repetition in the Git::config_* family of subroutines.
>
> Refactor common parts of Git::config(), Git::config_bool(),
> Git::config_int() and Git::config_path() into _config_common()
> helper method, reducing code duplication.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Jakub Narebski wrote:
>>
>> I'll resend amended commit.
>
> Here it is.

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.

The real problem was here.

Show 16 quoted lines

> @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.
>  
>  sub config_bool {
>  	my ($self, $var) = _maybe_self(@_);
> -
> -	try {
> -		my @cmd = ('config', '--bool', '--get', $var);
> -		unshift @cmd, $self if $self;
> -		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?

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