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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 9, 2008, 23:33 UTC
Message-ID: <200807100133.38163.jnareb@gmail.com>
In-Reply-To: <20080709160303.GL10151@machine.or.cz>

On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:

Show 9 quoted lines

> On Thu, Jul 03, 2008 at 06:24:53PM +0200, Jakub Narebski wrote:
> >
> > There are also a few things which I'd like some comments about:
> > 
> >  * Do config_val_to_bool and config_val_to_int should be exported
> >    by default?
> 
>   Yes with the current API, not with object API (see below). But if
> exported by default, there should be definitely a git_ prefix.

Object API for config is definitely good idea; some more reasons are given below.

Show 8 quoted lines

> >  * Should config_val_to_bool and config_val_to_int throw error or
> >    just return 'undef' on invalid values?  One can check if variable
> >    is defined using "exists($config_hash{'varname'})".
> 
>   I think that it's more reasonable to throw an error here (as long as
> you don't throw an error on undef argument). This particular case is
> clearly a misconfiguration by the user and you rarely need to handle
> this more gracefully, I believe.

If we follow git-config convention (and I guess we should), it would be value of appropriate type if variable value is of appropriate type, 'undef' (no output in the case of git-config) when variable does not exists, and throwing error (status and "fatal: ..." message on STDERR in the case of git-config) if variable is not of given type.

Show 10 quoted lines

> >  * How config_val_to_bool etc. should be named? Perhaps just
> >    config_to_bool, like in gitweb?
> 
>   What about Git::Config::bool()? ;-) (See below.)
> 
> >  * Is "return wantarray ? %config : \%config;" DWIM-mery good style?
> >    I am _not_ a Perl hacker...
> 
> I maybe wouldn't be as liberal myself, but I can't tell if it's a bad
> style either.

This won't matter, I think, in the case of object API (returning Git::Config instead of hash or hashref).

> >  * Should ->get_config() use ->command_output_pipe, or simpler
> >    ->command() method, reading whole config into array?
> 
> You have the code written already, I'd stick with it.

This is simple rewrite of code which is in gitweb, changing open '-|' into ->command_output_pipe, but we could just read the whole config into array using ->command(), which would be a bit simpler.

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

Show 6 quoted lines

> >  * Should we perltie hash?
> 
>   I agree with Lea that we should actually bless it. :-) Returning a
> Git::Config object provides a more flexible interface, and you can
> also do $repo->get_config()->bool('key') which is quite more elegant
> way than the val_ functions, I think.

Yes it is.

Also it does allow to use any capitalization of the section name and variable name (which are case insensitive), so you can write either

  $repo->get_config()->get('gitcvs.dbtablenameprefix');

but also

  $repo->get_config()->get('gitcvs.dbTableNamePrefix');

or even better, as below:

  $config = $repo->get_config();
  $config->get('gitcvs.dbTableNamePrefix');

BTW. what should non-typecasting should be named? $c->get(<VAR>), $c->value(<VAR>), $c->param(<VAR>), or something yet different?

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

By the way, bool values are processed a bit strangely. Value is true if it there is no value[*1*], if it is 'true' or 'yes' case insensitive, or of it is integer != 0. Value is false if it has empty value[*2*], if it is 'false' or 'no' case insensitive, and if it is integer of value '0'. All other values are invalid (and should cause throwing an error).

Both current config_val_to_bool() and config_val_to_int() implementation are too forbidding.

Show 7 quoted lines

> > As this is an RFC I have not checked if manpage (generated from
> > embedded POD documentation) renders correctly.
> 
>   By the way, unless you know already, you can do that trivially by
> issuing:
> 
> 	perldoc perl/Git.pm

Thanks, I didn't knew this.

> -- 
> 				Petr "Pasky" Baudis
> The last good thing written in C++ was the Pachelbel Canon. 
>                                                  -- J. Olson

Eh? Isn't it written C# rather?

-- 
Jakub Narebski
Poland