On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:
> 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.
> > * 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.
> > * 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.
> > * 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.
> > * 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.
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
