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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 3, 2008, 23:45 UTC
Message-ID: <200807040145.14724.jnareb@gmail.com>
In-Reply-To: <486D36CB.3090400@gmail.com>

Lea Wiemann wrote:

Show 7 quoted lines

> Jakub Narebski wrote:
>>
>> Add get_config([PREFIX]) method [...]
>
> 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?

If I remember correctly git-cvsserver used to have Perl parser for
_simplified_ config format, but now it uses `git config -l -z` or
`git config -l`.  Other git commands written in Perl which uses a lot
of configuration option could use it, like git-svn or git-send-email,
although for them shaving a little bit of execution time is not that
important (git-svn from what I understand still calls git-config
for each config variable).

Show 6 quoted lines

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

Actually for ->config_bool(<VARIABLE>) throwing error was the _only
way_ to distinguish between *non-existent* config variable (which we
can test using exists($config{<VARIABLE>}) when using ->get_config(),
and for which ->config_bool(<VARIABLE>) returned 'undef'), and
*non-bolean* value (for which config_val_to_bool($config{<VARIABLE>})
can return 'undef', and for which ->config_bool(<VARIABLE>) threw
error).
 
So we don't _need_ to _throw_ an error; we can detect error condition
in other way.

>>  * Is "return wantarray ? %config : \%config;" DWIM-mery good style?
> 
> Gitweb uses it as well, and it seems reasonable IMVHO.

Errr... if I remember correctly, the code in gitweb is by yours truly
:-), and as I have stated I am *not* a Perl hacker.

Show 5 quoted lines

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

I think using ->command() would be easier...

Show 6 quoted lines

>>  * 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?  [...] if you can wait a week or so, you could maybe
> integrate this into the Git::Repo interface [...]

I'd rather have both functional (of sorts) and object interface. Git::Repo / Git::Config could use methods / subroutines from Git.pm;

Show 5 quoted lines

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

O.K. (that is by the way how it is done in gitweb).

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

Thanks a lot!

-- 
Jakub Narebski
Poland