Re: [RFC/PATCH (WIP)] Git.pm: Add get_config() method and related subroutines

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 12, 2008, 12:35 UTC
Message-ID: <200807121435.49151.jnareb@gmail.com>
In-Reply-To: <20080712014708.GB10151@machine.or.cz>

On Sat, Jul 12, 2008, Petr Baudis wrote:

> On Thu, Jul 10, 2008 at 01:33:36AM +0200, Jakub Narebski wrote:
>> On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:
>>> On Thu, Jul 03, 2008 at 06:24:53PM +0200, Jakub Narebski wrote:

Show 11 quoted lines

>>>>  * What should ->get_config() have as an optional parameter:
>>>>    PREFIX (/^$prefix/o), or simply SECTION (/^(?:$section)\./o)?
>>> 
>>> Do we even _need_ a parameter like that? I don't understand what is
>>> this interface trying to address.
>> 
>> For example if one wants to access _all_ variables in gitweb.* section
>> (or in gitcvs.* section), and _only_ config variables in given section.
> 
> But what is the practical benefit? Does anyone use a config file long
> enough that this makes any difference?

Probably not.

Show 12 quoted lines

> Or if you mean foreach use, it's trivial to
> 
> 	foreach (grep { /^gitweb\./ } keys %$config)
> 
> or provide a method of Git::Config that will return this list, but it
> does not seem appropriate to have specific Git::Config instances for
> this. (Besides, if the script also needs to access a single variable
> from a different section, it will need to re-parse the config again.)
> 
> So I think your approach would be good only if there are multiple
> methods of Git::Config that would operate on the whole config and needed
> a way to be restricted; is that the case?

On the other hand if one uses (assuming now Git::Config object API)

  $conf = $repo->get_config('section');

one could access configuration variable values _without_ prefixing them with subsection name, i.e.

  $conf->get('variable');

and not

  $conf->get('section.variable');

I'm not sure if it is much of an improvement.

Show 5 quoted lines

>> BTW. what should non-typecasting should be named? $c->get(<VAR>), 
>> $c->value(<VAR>), $c->param(<VAR>), or something yet different?
> 
> I would prefer 'get' since it's the shortest and most clear, but 'value'
> would be fine too, I suppose (and more in line with bool etc.).

Well, that would have to be decided before other programs could use
this API.
 
We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and
(in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can
have $c->get(<VAR>), $c->get_bool(<VAR>) etc.

The former plays better with

  $r->get_config()->bool('gitweb.blame');

but this is nevertheless not recommended workflow; you can use

  $r->config_bool('gitweb.blame');

and it would be faster (unless some Perl/OO trickery with singletons and the like). Recommended workflow (code) is

  $c = $r->get_config();
  ...
  $c->get_bool('gitweb.blame');

or something like that.

Show 15 quoted lines

>>> Also, having accessors for special types lets you return undef when
>>> the type really isn't defined, instead of e.g. true with current
>>> config_val_bool, which is clearly bogus and requires complicated code
>>> on the user side. 
>>  
>> I don't follow you.  Didn't we agree on casting an error when variable
>> is not of given type?
> 
> Sorry, s/type really/variable really/. According to your original code,
> 
> 	config_val_bool(undef)
> 
> would return true, while the undef could be from both non-existent and
> unassigned variable. (This 'unassigned variables' case is really
> annoying to handle.)

That is why you should check exists($config{<VAR>}) first.

-- 
Jakub Narebski
Poland