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

[Lea Wiemann <lewiemann@gmail.com>]

Jakub Narebski wrote:
> Add get_config([PREFIX]) method [...]
>
> I hope I am not repeating [Lea's] work.

No, you're not.  (You could've checked my repo at
<http://repo.or.cz/w/git/gitweb-caching.git> ;-).)

FWIW, I don't think it'll make much of a difference for gitweb, since
the 'git config -l' output is cached anyway, but it's good someone's
extracting this.  Do you have any user for that function besides gitweb?

>  * Should config_val_to_bool and config_val_to_int throw error or
>    just return 'undef' on invalid values?

I suspect that if you have, say, command line tools, throwing an error
is better UI behavior than silently ignoring the entry.  And developers
can always catch errors if they want to.

>  * Is "return wantarray ? %config : \%config;" DWIM-mery good style?

Gitweb uses it as well, and it seems reasonable IMVHO.

>  * Should ->get_config() use ->command_output_pipe, or simpler
>    ->command() method, reading whole config into array?

Does it make a difference?  If you're worried about performance, config
files are so short that it won't matter; use the easier path.

>  * What should ->get_config() method be named? ->get_config()
>    or perhaps ->config_hash(), or ->config_hashref()?

Regarding the method naming, how about making this an object oriented
interface?  Bless the hash, and allow calls like
$config_hash->get('key').  I'm not sure how to name the constructor, but
if you can wait a week or so, you could maybe integrate this into the
Git::Repo interface (under Git/Config.pm), so you'd end up with ...

    Git::Repo->new(directory => 'repo.git')->config->get('key')

... where config() transparently parses the config file if it hasn't
been read already.  (The Git.pm API admittedly seems a little messy --
I'll post about that later -- so adding it to Git::Repo might be better
indeed.)

>  * What should ->get_config() have as an optional parameter:
>    PREFIX (/^$prefix/o), or simply SECTION (/^(?:$section)\./o)?

Off the top of my head, I don't see much need for a prefix parameter, so
I'd go for 'section'.

I haven't been able to answer all of the questions, but I hope this helps.

-- Lea