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

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

[Jakub Narebski]

Add get_config([PREFIX]) method, taken from current gitweb, which
parses whole (or selected part) config file into hash (reading
"git config -z -l" output).  This means that we do not have to call
one git command per config variable... but it also means that
conversion to boolean, to integer, or to color must be done from
within Perl; you can use config_val_to_* functions for that.

NOTE: Currently config_val_to_color and config_val_to_colorbool
are lacking; error checking is more relaxed in config_val_to_bool().

One advantage of ->get_config() over ->config(VARIABLE) is that it can
deal correctly with "no value" variables: they are !defined(), but
they do exists().


Tests are included; while at it add some more tests for generic
->config*() methods.

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Cc-ed Petr "Pasky" Baudis, who is author of Git.pm, and Lea Wiemann,
who is author of Git.pm test t/t9700-perl-git.sh.  Lea is also
working on object interface to git in Perl (Git::Repo etc.); I hope
I am not repeating her work.


This is WIP (Work In Progress) as much as an RFC (Request For Comments)
patch, as there are a few things which are not finished or not cleaned
up:

 * there is no config_val_to_*() equivalent of ->get_colorbool() and
   ->get_color() methods to convert config values to ANSI color escape
   sequences.

 * config_val_to_bool() and config_val_to_int() does not error out
   on values which are not boolean or not integer, contrary to what
   usage of "git config --bool" and "git config --int" does in
   ->config_bool() and ->config_int() methods, respectively.

   This should be fairly easy to add by manually throwing Error...
   the minor trouble would be to follow what ->config_bool etc. does.

 * neither config_val_to_bool nor config_val_to_int are exported.

 * tests contain some cruft in 'set up test repository' stage, which
   was inspected manually that is correct (by examining Data::Dumper
   output of new ->get_config() method against tested config file),
   but for which actual tests were written.


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?

 * 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'})".

 * How config_val_to_bool etc. should be named? Perhaps just
   config_to_bool, like in gitweb?

 * Is "return wantarray ? %config : \%config;" DWIM-mery good style?
   I am _not_ a Perl hacker...

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

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

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

 * Should config_val_to_* be tested against ->config_* output?

 * Should we perltie hash?

As this is an RFC I have not checked if manpage (generated from
embedded POD documentation) renders correctly.

 perl/Git.pm         |  107 +++++++++++++++++++++++++++++++++++++++++++++++++++
 t/t9700-perl-git.sh |   10 ++++-
 t/t9700/test.pl     |   32 +++++++++++++++
 3 files changed, 148 insertions(+), 1 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 97e61ef..2f4a306 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -668,6 +668,113 @@ sub get_color {
 	return $color;
 }
 
+=item get_config ( [ PREFIX ] )
+
+Return hash (in list context) or hashref of the whole configuration,
+in the similar manner that C<config --list>, optionally limited
+to config entries which fully qualified key (variable) name begins
+with C<PREFIX> (usually name of section).
+
+The keys of returned hash are fully qualified value names (section,
+optional subsection, and variable name joined using '.'). If variable
+is set only once its value is used as hash value, if variable is set
+multiple times array reference of all values is used as hash value
+for given key.
+
+Please remember that section names and key names in config hash keys
+(in fully qualified config variable name) are normalized, which means
+that they are in lowercase.
+
+=cut
+
+sub get_config {
+	my ($self, $prefix) = _maybe_self(@_);
+
+	my @cmd = ('config');
+	unshift @cmd, $self if $self;
+	my ($fh, $ctx) = command_output_pipe(@cmd, '-z', '--list');
+
+	my %config;
+	local $/ = "\0";
+	$prefix = quotemeta($prefix) if defined($prefix);
+	while (my $keyval = <$fh>) {
+		chomp $keyval;
+		my ($key, $value) = split(/\n/, $keyval, 2);
+
+		if (!defined $prefix || $key =~ /^$prefix/o) {
+			# store multiple values for single key as anonymous array reference
+			# single values stored directly in the hash, not as [ <value> ]
+			if (!exists $config{$key}) {
+				$config{$key} = $value;
+			} elsif (!ref $config{$key}) {
+				$config{$key} = [ $config{$key}, $value ];
+			} else {
+				push @{$config{$key}}, $value;
+			}
+		}
+	}
+	my @ctx = ($fh, $ctx);
+	unshift @ctx, $self if $self;
+	command_close_pipe(@ctx);
+
+	return wantarray ? %config : \%config;
+}
+
+=item config_val_to_bool ( VALUE )
+
+Convert config value C<VALUE> to boolean; no value, number > 0, 'true'
+and 'yes' values are true, rest of values are treated as false (never
+as error, at least for now).
+
+This function is meant to be used on values in hash returned by
+C<get_config>.
+
+=cut
+
+sub config_val_to_bool {
+	my $val = shift;
+
+	# strip leading and trailing whitespace
+	$val =~ s/^\s+//;
+	$val =~ s/\s+$//;
+
+	return (!defined $val ||               # section.key
+	        ($val =~ /^\d+$/ && $val) ||   # section.key = 1
+	        ($val =~ /^(?:true|yes)$/i));  # section.key = true
+}
+
+=item config_val_to_int ( VALUE )
+
+Convert config value C<VALUE> to simple decimal number; an optional
+value suffix of 'k', 'm', or 'g' will cause the value to be multiplied
+by 1024, 1048576 (1024 x 1024), or 1073741824 (1024 x 1024 x 1024),
+respectively (unknown unit is treated as 1, at least for now).
+
+It does not throw error on argument which is not integer.
+
+This function is meant to be used on values in hash returned by
+C<get_config>.
+
+=cut
+
+sub config_val_to_int {
+	my $val = shift;
+
+	# strip leading and trailing whitespace
+	$val =~ s/^\s+//;
+	$val =~ s/\s+$//;
+
+	if (my ($num, $unit) = ($val =~ /^([0-9]*)([kmg])$/i)) {
+		$unit = lc($unit);
+		# unknown unit is treated as 1
+		return $num * ($unit eq 'g' ? 1073741824 :
+		               $unit eq 'm' ?    1048576 :
+		               $unit eq 'k' ?       1024 : 1);
+	}
+	return $val;
+}
+
+
 =item ident ( TYPE | IDENTSTR )
 
 =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 9706ee5..af6ac58 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -34,9 +34,17 @@ test_expect_success \
      git-config --add test.booltrue true &&
      git-config --add test.boolfalse no &&
      git-config --add test.boolother other &&
-     git-config --add test.int 2k
+     git-config --add test.int 2k &&
+     git-config --add teSt.duP val1 &&
+     git-config --add tesT.Dup val2 &&
+     git-config --add test.subsection.noDup val &&
+     git-config --add test.subSection.nodup val &&
+     git-config --add "test.sub # \\ \" '\'' section.key" val &&
+     echo "[test] noval" >> .git/config
      '
 
+test_debug 'cat .git/config'
+
 test_external_without_stderr \
     'Perl API' \
     perl ../t9700/test.pl
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 4d23125..4dd8bbf 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -11,6 +11,8 @@ use Cwd;
 use File::Basename;
 use File::Temp;
 
+use Data::Dumper;
+
 BEGIN { use_ok('Git') }
 
 # set up
@@ -30,11 +32,36 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
 is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
 ok($r->config_bool("test.booltrue"), "config_bool: true");
 ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
+ok($r->config_bool("test.noval"), "config_bool: true (noval)");
 our $ansi_green = "\x1b[32m";
 is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
 # Cannot test $r->get_colorbool("color.foo")) because we do not
 # control whether our STDOUT is a terminal.
 
+# testing get_config() and related functions/subroutines/methods
+is_deeply(scalar($r->get_config('color.')), {'color.test.slot1' => 'green'},
+          "get_config('color.')");
+my %config;
+ok(%config = $r->get_config(), "get_config(): list context");
+is($config{"test.string"}, "value",
+	"\%config scalar: string");
+is_deeply($config{"test.dupstring"}, ["value1", "value2"],
+	"\%config array: string");
+is($config{"test.nonexistent"}, undef,
+	"\%config scalar: nonexistent (undef)");
+ok(!exists($config{"test.nonexistent"}),
+	"\%config scalar: nonexistent (!exists)");
+is(Git::config_val_to_int($config{"test.int"}), 2048,
+	"config_val_to_int: integer");
+is(Git::config_val_to_int($config{"test.nonexistent"}), undef,
+	"config_val_to_int: nonexistent");
+ok( Git::config_val_to_bool($config{"test.booltrue"}),
+	"config_val_to_bool: true");
+ok(!Git::config_val_to_bool($config{"test.boolfalse"}),
+	"config_val_to_bool: false");
+ok( Git::config_val_to_bool($config{"test.noval"}),
+	"config_val_to_bool: true (noval)");
+
 # Failure cases for config:
 # Save and restore STDERR; we will probably extract this into a
 # "dies_ok" method and possibly move the STDERR handling to Git.pm.
@@ -43,6 +70,11 @@ eval { $r->config("test.dupstring") };
 ok($@, "config: duplicate entry in scalar context fails");
 eval { $r->config_bool("test.boolother") };
 ok($@, "config_bool: non-boolean values fail");
+TODO: {
+	$TODO = "config_val_to_bool returns false on non-bool values";
+	eval { Git::config_val_to_bool($config{"test.boolother"}) };
+	ok($@, "config_val_to_bool: non-boolean values fail");
+}
 open STDERR, ">&", $tmpstderr or die "cannot restore STDERR";
 
 # ident
-- 
1.5.6.1

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

[Lea Wiemann]

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

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

[Jakub Narebski]

Lea Wiemann wrote:
> 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).
 
>>  * 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.
 
>>  * 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...
 
>>  * 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;

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

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

[Lea Wiemann]

Jakub Narebski wrote:
> I'd rather have both functional (of sorts) and object interface.

I wouldn't, since it makes the code more complicated.  Having an
object-oriented interface is cleaner (in this case), and it doesn't make
it more complicated to use; hence I don't think there's any need for a
functional interface.

> Git::Repo / Git::Config could use methods / subroutines from Git.pm;

I don't think that's possible, since Git.pm has a pretty long-running
(and complicated) instantiation method, whereas Git::Repo has (and must
have) instantaneous instantiation (without system calls).  Also, Git.pm
is so strange (design-wise) that I'm not very happy with depending on it
as it is.  I'll write more about that later though.

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

[Petr Baudis]

On Mon, Jul 07, 2008 at 09:24:12PM +0200, Lea Wiemann wrote:
> Jakub Narebski wrote:
> > Git::Repo / Git::Config could use methods / subroutines from Git.pm;
> 
> I don't think that's possible, since Git.pm has a pretty long-running
> (and complicated) instantiation method, whereas Git::Repo has (and must
> have) instantaneous instantiation (without system calls).

There can be an alternative instantiation method that is faster.

> Also, Git.pm is so strange (design-wise) that I'm not very happy with
> depending on it as it is.  I'll write more about that later though.

I'm curious to hear about your reservations to Git.pm design when you
get to writing up something. But I think you should have _very_ good
reasons for introducing a completely separate alternative API and show
convincingly why the current one cannot be extended and build upon,
since going that way is bound to get quite messy. Please avoid the NIH
syndrome and don't reinvent the wheel needlessly. ;-)

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

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

[Petr Baudis]

  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

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

[Jakub Narebski]

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

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

[Petr Baudis]

On Thu, Jul 10, 2008 at 01:33:36AM +0200, Jakub Narebski wrote:
> On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:
> > On Thu, Jul 03, 2008 at 06:24:53PM +0200, Jakub Narebski wrote:
> > >  * 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.

Yes, this seems to be in agreement with what I suggested.

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

But what is the practical benefit? Does anyone use a config file long
enough that this makes any difference?

Or if you mean foreach use, it's trivial to

	foreach (grep { /^gitweb\./ } keys %$config)

or provide a method of Git::Config that will return this list, but it
does not seem appropriate to have specific Git::Config instances for
this. (Besides, if the script also needs to access a single variable
from a different section, it will need to re-parse the config again.)

So I think your approach would be good only if there are multiple
methods of Git::Config that would operate on the whole config and needed
a way to be restricted; is that the case?

> BTW. what should non-typecasting should be named? $c->get(<VAR>), 
> $c->value(<VAR>), $c->param(<VAR>), or something yet different?

I would prefer 'get' since it's the shortest and most clear, but 'value'
would be fine too, I suppose (and more in line with bool etc.).

> > 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?

Sorry, s/type really/variable really/. According to your original code,

	config_val_bool(undef)

would return true, while the undef could be from both non-existent and
unassigned variable. (This 'unassigned variables' case is really
annoying to handle.)

> > -- 
> > 				Petr "Pasky" Baudis
> > The last good thing written in C++ was the Pachelbel Canon. 
> >                                                  -- J. Olson 
> 
> Eh? Isn't it written C# rather?

Well, the quote is a bit dated and changing it would be too problematic.
;-)

-- 
				Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce

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

[Jakub Narebski]

On Sat, Jul 12, 2008, Petr Baudis wrote:
> On Thu, Jul 10, 2008 at 01:33:36AM +0200, Jakub Narebski wrote:
>> On Wed, Jul 09, 2008, Petr "Pasky" Baudis wrote:
>>> On Thu, Jul 03, 2008 at 06:24:53PM +0200, Jakub Narebski wrote:

>>>>  * 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.
> 
> But what is the practical benefit? Does anyone use a config file long
> enough that this makes any difference?

Probably not.

> Or if you mean foreach use, it's trivial to
> 
> 	foreach (grep { /^gitweb\./ } keys %$config)
> 
> or provide a method of Git::Config that will return this list, but it
> does not seem appropriate to have specific Git::Config instances for
> this. (Besides, if the script also needs to access a single variable
> from a different section, it will need to re-parse the config again.)
> 
> So I think your approach would be good only if there are multiple
> methods of Git::Config that would operate on the whole config and needed
> a way to be restricted; is that the case?

On the other hand if one uses (assuming now Git::Config object API)

  $conf = $repo->get_config('section');

one could access configuration variable values _without_ prefixing
them with subsection name, i.e.

  $conf->get('variable');

and not

  $conf->get('section.variable');


I'm not sure if it is much of an improvement.

>> BTW. what should non-typecasting should be named? $c->get(<VAR>), 
>> $c->value(<VAR>), $c->param(<VAR>), or something yet different?
> 
> I would prefer 'get' since it's the shortest and most clear, but 'value'
> would be fine too, I suppose (and more in line with bool etc.).

Well, that would have to be decided before other programs could use
this API.
 
We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and
(in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can
have $c->get(<VAR>), $c->get_bool(<VAR>) etc.

The former plays better with

  $r->get_config()->bool('gitweb.blame');

but this is nevertheless not recommended workflow; you can use

  $r->config_bool('gitweb.blame');

and it would be faster (unless some Perl/OO trickery with singletons
and the like).  Recommended workflow (code) is

  $c = $r->get_config();
  ...
  $c->get_bool('gitweb.blame');

or something like that.

>>> 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?
> 
> Sorry, s/type really/variable really/. According to your original code,
> 
> 	config_val_bool(undef)
> 
> would return true, while the undef could be from both non-existent and
> unassigned variable. (This 'unassigned variables' case is really
> annoying to handle.)

That is why you should check exists($config{<VAR>}) first.

-- 
Jakub Narebski
Poland

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

[Petr Baudis]

On Sat, Jul 12, 2008 at 02:35:48PM +0200, Jakub Narebski wrote:
> On Sat, Jul 12, 2008, Petr Baudis wrote:
> > Or if you mean foreach use, it's trivial to
> > 
> > 	foreach (grep { /^gitweb\./ } keys %$config)
> > 
> > or provide a method of Git::Config that will return this list, but it
> > does not seem appropriate to have specific Git::Config instances for
> > this. (Besides, if the script also needs to access a single variable
> > from a different section, it will need to re-parse the config again.)
> > 
> > So I think your approach would be good only if there are multiple
> > methods of Git::Config that would operate on the whole config and needed
> > a way to be restricted; is that the case?
> 
> On the other hand if one uses (assuming now Git::Config object API)
> 
>   $conf = $repo->get_config('section');
> 
> one could access configuration variable values _without_ prefixing
> them with subsection name, i.e.
> 
>   $conf->get('variable');
> 
> and not
> 
>   $conf->get('section.variable');
> 
> 
> I'm not sure if it is much of an improvement.

I would propose not to go with this feature yet and add it only when
justified by accompanied cleanups of real code using this.

> We can have $c->value(<VAR>), $c->bool(<VAR>), $c->int(<VAR>) and
> (in the future) $c->color(<VAR>) and $c->colorbool(<VAR>)... or we can
> have $c->get(<VAR>), $c->get_bool(<VAR>) etc.

I think I prefer the terser approach of value/bool/int now since it is
shorter and seems to stay pretty unambiguous. Besides, this way we can
easily make mutators by just adding second optional argument to these
methods.

> The former plays better with
> 
>   $r->get_config()->bool('gitweb.blame');
> 
> but this is nevertheless not recommended workflow; you can use
> 
>   $r->config_bool('gitweb.blame');
> 
> and it would be faster (unless some Perl/OO trickery with singletons
> and the like).
> 
>  Recommended workflow (code) is
> 
>   $c = $r->get_config();
>   ...
>   $c->get_bool('gitweb.blame');
> 
> or something like that.

I thought that the get_config() method would be reusing existing
Git::Config instances no-matter-what? (Otherwise, you can get into
rather tricky issues like one part of the code modifying the config and
the other not noticing the change, etc. And I can se no benefit. If you
want to reload the config, we might have method like reload(), but I
don't think that would be very useful except perhaps in some kind of
mod_perl'd gitweb scenario.)

> >>> 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?
> > 
> > Sorry, s/type really/variable really/. According to your original code,
> > 
> > 	config_val_bool(undef)
> > 
> > would return true, while the undef could be from both non-existent and
> > unassigned variable. (This 'unassigned variables' case is really
> > annoying to handle.)
> 
> That is why you should check exists($config{<VAR>}) first.

And that was my point when I talked about complicated code on the user
side.

But we're arguing about nothing, really, except the exact amount of
coolness of the Git::Config approach. :-)

-- 
				Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce

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

[Jakub Narebski]

On Sat, Jul 12, 2008, Petr Baudis wrote:
> On Sat, Jul 12, 2008 at 02:35:48PM +0200, Jakub Narebski wrote:
>> On Sat, Jul 12, 2008, Petr Baudis wrote:
 
>> On the other hand if one uses (assuming now Git::Config object API)
>> 
>>   $conf = $repo->get_config('section');
>> 
>> one could access configuration variable values _without_ prefixing
>> them with subsection name, i.e.
>> 
>>   $conf->get('variable');
>> 
>> and not
>> 
>>   $conf->get('section.variable');
>> 
>> 
>> I'm not sure if it is much of an improvement.
> 
> I would propose not to go with this feature yet and add it only when
> justified by accompanied cleanups of real code using this.

O.K.
 
>> The former plays better with
>> 
>>   $r->get_config()->bool('gitweb.blame');
>> 
>> but this is nevertheless not recommended workflow; you can use
>> 
>>   $r->config_bool('gitweb.blame');
>> 
>> and it would be faster (unless some Perl/OO trickery with singletons
>> and the like).
>> 
>>  Recommended workflow (code) is
>> 
>>   $c = $r->get_config();
>>   ...
>>   $c->get_bool('gitweb.blame');
>> 
>> or something like that.
> 
> I thought that the get_config() method would be reusing existing
> Git::Config instances no-matter-what? (Otherwise, you can get into
> rather tricky issues like one part of the code modifying the config and
> the other not noticing the change, etc. And I can se no benefit. If you
> want to reload the config, we might have method like reload(), but I
> don't think that would be very useful except perhaps in some kind of
> mod_perl'd gitweb scenario.)

Somehow I forgot that $repo can remember existing Git::Config, generate
it only once (and remember it) on demand but fully, and then serve it.
It probably is some OO "pattern" like SingletonFactory or sth...

It would probably play better with Git::Repo object interface, but can
I think work as well with Git->repository(...) instance.


As to changing config: I wasn't even thinking about that... and I don't
think that any command written in Perl which uses or can use either
Git or Git::Repo API needs writing config files.


> But we're arguing about nothing, really, except the exact amount of
> coolness of the Git::Config approach. :-)

True.  For me the fact that one can write $c->value('diff.renameLimit')
or $c->value('diff.renamelimit'); is teh winner ;-)

-- 
Jakub Narebski
Poland

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

[Petr Baudis]

On Sat, Jul 12, 2008 at 04:31:50PM +0200, Jakub Narebski wrote:
> As to changing config: I wasn't even thinking about that... and I don't
> think that any command written in Perl which uses or can use either
> Git or Git::Repo API needs writing config files.

  It'd be mainly for external users. E.g. the repo.or.cz duct tape would
make use of it.

				Petr "Pasky" Baudis