From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 18 Oct 2011 19:52:06 GMT
Subject: Re: [PATCH/RFC 3/2 (fixed)] Refactor Git::config_*
Message-ID: <7vty76f57d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201110181147.02397.jnareb@gmail.com>

Jakub Narebski <jnareb@gmail.com> writes:

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

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