Re: [PATCH 2/2] use new Git::config_path() for aliasesfile
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Sep 30, 2011, 22:00 UTC
- Message-ID
- <201110010000.13328.jnareb@gmail.com>
- In-Reply-To
- <7vsjndoldq.fsf@alter.siamese.dyndns.org>
On Fri, 30 Sep 2011, Junio C Hamano wrote:
Show 6 quoted lines
> 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...
Show 11 quoted lines
> =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'?
Show 22 quoted lines
> 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