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

From: Petr Baudis <pasky@suse.cz>
Date: Jul 9, 2008, 16:03 UTC
Message-ID: <20080709160303.GL10151@machine.or.cz>
In-Reply-To: <200807031824.55958.jnareb@gmail.com>

Hi!

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.

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

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

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

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

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

> 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

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