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