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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jul 3, 2008, 20:30 UTC
Message-ID: <486D36CB.3090400@gmail.com>
In-Reply-To: <200807031824.55958.jnareb@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