Re: [PATCH 2/2] use new Git::config_path() for aliasesfile

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 30 Sep 2011, Junio C Hamano wrote:

> I think the addition of "config --path" support is a good idea, but the
> resulting code suffers from too many cut&paste cruft across the config*
> family of methods.
> 
> How about doing a bit of refactoring, perhaps something like this, on top
> as a separate patch?

This is a good idea, in my opinion.
 
> I tried to be careful to still forcing the "one value only" for config_bool
> and config_int, but extra sets of eyeballs would be needed.

We do have tests for that, have we?
 
[...]
> +Common subroutine to implement bulk of what the config* family of methods
> +do. This wraps command('config') so it is not so fast.

BTW. I wonder if it wouldn't be a good idea to restart work on Git::Config
with lazy/eager loading of the whole config, like git_get_project_config()
and git_parse_project_config() subroutines from gitweb do it - running 
"git config -z -l".  Though then --int/--bool/--path conversion would have
to be implemented in Perl...
  
>  =cut
>  
> -sub config {
> -	my ($self, $var) = _maybe_self(@_);
> -
> +sub _config_common {
> +	my ($self, $var, $opts) = _maybe_self(@_);
> +	
>  	try {
> -		my @cmd = ('config');
> +		my @cmd = ('config', $opts->{'kind'} ? @{$opts->{'kind'}} : ());

How it is supposed to work?

First, you probably want to check for "exists $opts->{'kind'}", or even
"defined $opts->{'kind'}".

Second, "@{$opts->{'kind'}}" assumes that $opts->{'kind'} is an array
reference (something we didn't check)... and it isn't.


BTW. why do you use hashref?  Do you plan for the future to pass more
options that 'kind'?

>  		unshift @cmd, $self if $self;
>  		if (wantarray) {
>  			return command(@cmd, '--get-all', $var);
> @@ -594,6 +590,21 @@ sub config {
>  			throw $E;
>  		}
>  	};
> +
> +}
> +
> +=item config ( VARIABLE )
> +
> +Retrieve the configuration C<VARIABLE> in the same manner as C<config>
> +does. In scalar context requires the variable to be set only one time
> +(exception is thrown otherwise), in array context returns allows the
> +variable to be set multiple times and returns all the values.
> +
> +=cut
> +
> +sub config {
> +	my ($self, $var) = _maybe_self(@_);
> +	return _config_common($self, $var, +{});

No need to pass an empty hash.  Perl has a feature called autovivification:
when an undefined variable is dereferenced, it gets silently upgraded to
an array or hash reference (depending of the type of the dereferencing).

It is un-Perlish to do so.

>  sub config_bool {
>  	my ($self, $var) = _maybe_self(@_);
[...]
> +	my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
> +	return (defined $val && $val eq 'true');
>  }
[...]

>  sub config_path {
>  	my ($self, $var) = _maybe_self(@_);
[...]
> +	return _config_common($self, $var, +{'kind' => '--path'});
>  }

Why the difference between {'kind' => '--bool'} and +{'kind' => '--path'}?

> -This currently wraps command('config') so it is not so fast.
> -

Shouldn't this be mentioned somewhat, even indirectly?

-- 
Jakub Narebski
Poland