[PATCH] perl: add new module Git::Config for cached 'git config' access

[PATCH] perl: add new module Git::Config for cached 'git config' access

[Sam Vilain]

Add a new module, Git::Config, for a better Git configuration API.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 perl/Git/Config.pm   |  465 ++++++++++++++++++++++++++++++++++++++++++++++++++
 perl/Makefile.PL     |    5 +-
 t/t9700-perl-git.sh  |    8 +
 t/t9700/TestUtils.pm |   77 +++++++++
 t/t9700/config.t     |  127 ++++++++++++++
 5 files changed, 681 insertions(+), 1 deletions(-)
 create mode 100644 perl/Git/Config.pm
 create mode 100644 t/t9700/TestUtils.pm
 create mode 100644 t/t9700/config.t

diff --git a/perl/Git/Config.pm b/perl/Git/Config.pm
new file mode 100644
index 0000000..a0a6a41
--- /dev/null
+++ b/perl/Git/Config.pm
@@ -0,0 +1,465 @@
+
+package Git::Config;
+
+use 5.006;
+use strict;
+use warnings;
+
+use Git;
+use Error qw(:try);
+
+=head1 NAME
+
+Git::Config - caching interface to git-config state
+
+=head1 SYNOPSIS
+
+  use Git::Config;
+
+  my $conf = Git::Config->new( $git );
+
+  # return single config items
+  my $value = $conf->config( VARIABLE );
+
+  # return multi-config items
+  my @values = $conf->config( VARIABLE );
+
+  # manipulate type of slot, for special interpretation
+  $conf->type( VARIABLE => $type );
+
+  # change value
+  $conf->config( VARIABLE => $value );
+
+  # read or configure a global or system variable
+  $conf->global( VARIABLE => $value );
+  $conf->system( VARIABLE => $value );
+
+  # default is to autoflush 
+  $conf->autoflush( 0 );
+
+  # if autoflush is disabled, flush explicitly.
+  $conf->flush;
+
+  # update cache
+  $conf->read;
+
+=head1 DESCRIPTION
+
+This module provides a cached interface to the B<git config> command
+in Perl.
+
+=head1 CONSTRUCTION
+
+=head2 B<Git::Config-E<gt>new( I<$git> )>
+
+Creates a new B<Git::Config> object.  Does not read the configuration
+file yet.
+
+=cut
+
+sub new {
+	my $class = shift;
+	my $git = shift || Git->repository;
+	my $self = bless { git => $git }, $class;
+	while ( my ($item, $value) = splice @_, 0, 2 ) {
+		try {
+			$self->$item($value);
+		}
+		catch {
+			throw Error::Simple (
+				"Invalid constructor arg '$item'",
+			       );
+		};
+	}
+	return $self;
+}
+
+=head1 METHODS
+
+=head2 B<$conf-E<gt>config( C<item> )>
+
+Reads the value of a particular configuration item.
+When called in B<list context>, returns all values of multiple value
+items.
+When called in B<scalar context>, raises an error if the item is
+specified multiple times.
+
+=head2 B<$conf-E<gt>config( C<item> =E<gt> I<$value> )>
+
+Sets the value of an item.
+If an B<array reference> is specified, the entire list of items is
+replaced with the values specified.
+If a single item is passed but the item is specified multiple times in
+the configuration file, raises an error.
+Returns the old value.
+
+=cut
+
+sub config {
+	return $_[0]->_config("", @_[1..$#_]);
+}
+
+sub _config {
+	my $self = shift;
+	my $which = shift;
+	my $item = shift;
+	my $value_passed = @_;
+	my $value = shift;
+
+	my $type = $self->type( $item );
+
+	my $_which = $which ? "${which}_" : "";
+	my $_read = "read_${_which}state";
+	my $_state = "${_which}state";
+
+	if (!$self->{$_read}) {
+		$self->read($which);
+	}
+
+	my $state;
+	if (exists $self->{$_state}{$item}) {
+		$state = $self->{$_state}{$item};
+	}
+	else {
+		$state = $self->{$_read}{$item};
+	}
+
+	if ($value_passed) {
+		if (!ref $value and defined $value and ref $state) {
+			throw Error::Simple (
+				"'$item' is specified multiple times",
+			       );
+		}
+		if ( $type ) {
+			if (ref $value) {
+				$value = [ map { $self->freeze($type, $_) }
+						   @$value ];
+			}
+			else {
+				$value = $self->freeze($type, $value);
+			}
+		}
+		$self->{$_state}{$item} = $value;
+		$self->flush() if $self->autoflush;
+	}
+
+	if (defined wantarray) {
+		my @values = ref $state ? @$state :
+			defined $state ? ($state) : ();
+
+		if ( my $type = $self->type( $item ) ) {
+			@values = map { $self->thaw($type, $_) }
+				@values;
+		}
+
+		if ( @values > 1 and
+			     ( ($value_passed and not ref $value) or
+				       (not $value_passed and not wantarray) )
+			    ) {
+			throw Error::Simple (
+				"'$item' is specified multiple times",
+			       );
+		}
+
+		return (wantarray ? @values : $values[0]);
+	}
+}
+
+=head2 B<$conf-E<gt>read()>
+
+Reads the current state of the configuration file.
+
+=cut
+
+sub read {
+	my $self = shift;
+	my $which = shift;
+
+	my $git = $self->{git};
+
+	my ($fh, $c) = $git->command_output_pipe(
+		'config', ( $which ? ("--$which") : () ),
+		'--list',
+	       );
+	my $read_state = {};
+
+	while (<$fh>) {
+		my ($item, $value) = m{(.*?)=(.*)};
+		my $sl = \( $read_state->{$item} );
+		if (!defined $$sl) {
+			$$sl = $value;
+		}
+		elsif (!ref $$sl) {
+			$$sl = [ $$sl, $value ];
+		}
+		else {
+			push @{ $$sl }, $value;
+		}
+	}
+
+	$git->command_close_pipe($fh, $c);
+
+	if ( $which ) {
+		$which .= "_";
+	}
+	else {
+		$which = "";
+	};
+
+	$self->{"read_${which}state"} = $read_state;
+}
+
+=head2 type( VARIABLE => $type )
+
+Specifies which set of rules to use when returning to and from the
+config file.  C<$type> may be C<string>, C<boolean> or C<integer>.
+Globs such as C<*> are allowed which match everything except C<.>
+(full stop).
+
+=head2 type( VARIABLE )
+
+Returns the first matching defined type rule.
+
+=cut
+
+sub type {
+	my $self = shift;
+	my $item = shift;
+	my $got_type = @_;
+	my $type = shift;
+	$type ||= "string";
+
+	my $types = $self->{types} ||= [];
+	if ( $got_type ) {
+		$item =~ s{([\.(?])}{\\$1}g;
+		$item =~ s{\*}{[^\\.]*}g;
+		$item = qr{^$item$};
+		@$types = grep { $_->[0] ne $item }
+			@$types;
+		push @$types, [ $item, $type ];
+	}
+	else {
+	type:
+		for (@$types) {
+			if ($item =~ m{$_->[0]}) {
+				$type = $_->[1];
+				last type;
+			}
+		}
+
+		$type;
+	}
+}
+
+=head2 global( VARIABLE )
+
+=head2 system( VARIABLE )
+
+Return the value of the given variable in the global or system
+configuration (F<~/.gitconfig> and F</etc/gitconfig> on Unix).
+
+=head2 global( VARIABLE => $value )
+
+=head2 system( VARIABLE => $value )
+
+Set the value of the given variable in the global or system
+configuration.
+
+=cut
+
+sub global { return $_[0]->_config("global", @_[1..$#_]) }
+sub system { return $_[0]->_config("system", @_[1..$#_]) }
+
+=head2 autoflush
+
+Returns 0 or 1 depending on whether autoflush is enabled.  Defaults to
+1.
+
+=head2 autoflush ( $value )
+
+Set the value of autoflush.  If set to 1, flushes immediately.
+
+=cut
+
+sub autoflush {
+	my $self = shift;
+	if (@_) {
+		$self->{autoflush} = shift;
+	}
+	not (defined $self->{autoflush} and not $self->{autoflush});
+}
+
+=head2 flush
+
+Flushes all changed configuration values to the config file(s).
+
+=cut
+
+sub flush {
+	my $self = shift;
+
+	for my $which ("", "global", "system") {
+		my $st = $which . ($which ? "_" : "") . "state";;
+		my $read = "read_$st";
+		if (my $new = delete $self->{$st}) {
+			$self->_write($which, $new, $self->{$read});
+			$self->read($which);
+		}
+	}
+}
+
+sub _write {
+	my $self = shift;
+	my $which = shift;
+	my $state = shift;
+	my $read_state = shift;
+
+	my $git = $self->{git};
+
+	while (my ($item, $value) = each %$state) {
+		my $old_value = $read_state->{$item};
+		my @cmd = ($which ? ("--$which") : () );
+		my $type = $self->type($item);
+		if ($type ne "string") {
+			push @cmd, "--$type";
+		}
+		if (ref $value) {
+			$git->command_oneline (
+				"config", @cmd, "--replace-all",
+				 $item, $value->[0],
+			       );
+			for my $i (1..$#$value) {
+				$git->command_oneline (
+					"config", @cmd, "--add",
+					$item, $value->[$i],
+				       );
+			};
+		}
+		elsif (defined $value) {
+			$git->command_oneline (
+				"config", @cmd, $item, $value,
+			       );
+		}
+		elsif ($read_state->{$item}) {
+			$git->command_oneline (
+				"config", "--unset-all", @cmd,
+				$item, $value,
+			       );
+		}
+		else {
+			# nothing to do - already not in config
+		}
+	}
+}
+
+sub _dispatch {
+	no strict 'refs';
+	my $self = shift;
+	my $func = shift;
+	my $type = shift;
+	my $sym = __PACKAGE__."::${type}::$func";
+	defined &{$sym}
+		or throw Error::Simple "Bad type '$type' in $func";
+	&{$sym}(@_, $self);
+}
+
+sub freeze {
+	my $self = shift;
+	$self->_dispatch("freeze", @_);
+}
+
+sub thaw {
+	my $self = shift;
+	$self->_dispatch("thaw", @_);
+}
+
+{
+	package Git::Config::string;
+	sub freeze { shift }
+	sub thaw   { shift }
+}
+{
+	package Git::Config::integer;
+	our @mul = ("", "k", "M", "G");
+	sub freeze {
+		my $val = shift;
+		my $scale = 0;
+		while ( (my $num = int($val/1024))*1024 == $val ) {
+			$scale++;
+		 	$val = $num;
+			last if $scale == $#mul;
+		}
+		$val.$mul[$scale];
+	}
+	our $mul_re = qr/^(\d+)\s*${\( "(".join("|", @mul).")" )}$/i;
+	sub thaw {
+		my $val = shift;
+		$val =~ m{$mul_re}
+			or throw Error::Simple "Bad value for integer: '$val'";
+		my $num = $1;
+		if ($2) {
+			my $scale = 0;
+			do { $num = $num * 1024 }
+				until (lc($mul[++$scale]) eq lc($2));
+		}
+		$num;
+	}
+}
+{
+	package Git::Config::boolean;
+	our @true = qw(true yes 1);
+	our @false = qw(false no 0);
+	our $true_re = qr/^${\( "(".join("|", @true).")" )}$/i;
+	our $false_re = qr/^${\( "(".join("|", @false).")" )}$/i;
+	sub freeze {
+		my $val = shift;
+		if (!!$val) {
+			$true[0];
+		}
+		else {
+			$false[0];
+		}
+	}
+	sub thaw {
+		my $val = shift;
+		if ($val =~ m{$true_re}) {
+			1;
+		}
+		elsif ($val =~ m{$false_re}) {
+			0;
+		}
+		else {
+			throw Error::Simple "Bad value for boolean: '$val'";
+		}
+	}
+}
+
+1;
+
+__END__
+
+=head1 SEE ALSO
+
+L<Git>
+
+=head1 AUTHOR AND LICENSE
+
+Copyright 2009, Sam Vilain, L<sam@vilain.net>.  All Rights Reserved.
+This program is Free Software; you may use it under the terms of the
+Perl Artistic License 2.0 or later, or the GPL v2 or later.
+
+=cut
+
+# Local Variables:
+#   mode: cperl
+#   cperl-brace-offset: 0
+#   cperl-continued-brace-offset: 0
+#   cperl-indent-level: 8
+#   cperl-label-offset: -8
+#   cperl-merge-trailing-else: nil
+#   cperl-continued-statement-offset: 8
+#   cperl-indent-parens-as-block: t
+#   cperl-indent-wrt-brace: nil
+# End:
+#
+# vim: vim:tw=78:sts=0:noet
diff --git a/perl/Makefile.PL b/perl/Makefile.PL
index 320253e..80df0b0 100644
--- a/perl/Makefile.PL
+++ b/perl/Makefile.PL
@@ -8,7 +8,10 @@ instlibdir:
 MAKE_FRAG
 }
 
-my %pm = ('Git.pm' => '$(INST_LIBDIR)/Git.pm');
+my %pm = (map {( "$_.pm" => "\$(INST_LIBDIR)/$_.pm" )}
+	'Git',
+	'Git/Config',
+	);
 
 # We come with our own bundled Error.pm. It's not in the set of default
 # Perl modules so install it if it's not available on the system yet.
diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index b81d5df..2f53d00 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -37,8 +37,16 @@ test_expect_success \
      git config --add test.int 2k
      '
 
+export PERL5LIB="$TEST_DIRECTORY/t9700:$GITPERLLIB:$PERL5LIB"
+
 test_external_without_stderr \
     'Perl API' \
     perl "$TEST_DIRECTORY"/t9700/test.pl
 
+rm -rf .git
+
+test_external_without_stderr \
+    'config API' \
+    perl "$TEST_DIRECTORY"/t9700/config.t
+
 test_done
diff --git a/t/t9700/TestUtils.pm b/t/t9700/TestUtils.pm
new file mode 100644
index 0000000..72a386d
--- /dev/null
+++ b/t/t9700/TestUtils.pm
@@ -0,0 +1,77 @@
+#!/usr/bin/perl
+
+package TestUtils;
+
+use Carp;
+use base qw(Exporter);
+BEGIN {
+	our @EXPORT = qw(mk_tmp_repo in_empty_repo tmp_git random_port_pair);
+	$SIG{__WARN__} = sub {
+	    my $i = 0;
+	    while (my $caller = caller($i)) {
+		    if ($caller eq "Test::More") {
+			    return 0;
+		    }
+		    $i++;
+	    }
+	    my $culprit = caller;
+	    if ($culprit =~ m{Class::C3}) {
+		return 1;
+	    }
+	    else {
+		print STDERR "*** WARNING FROM $culprit FOLLOWS ***\n";
+	    }
+	    Carp::cluck(@_);
+	    print STDERR "*** END OF STACK DUMP ***\n";
+	    1
+	}
+	unless $ENV{NO_WARNING_TRACES};
+}
+
+use Cwd qw(fast_abs_path);
+use File::Temp qw(tempdir);
+
+sub mk_tmp_repo {
+	my $temp_dir = tempdir( "tmpXXXXX", CLEANUP => 1 );
+	system("cd $temp_dir; git-init >/dev/null 2>&1") == 0
+		or die "git-init failed; rc=$?";
+	fast_abs_path($temp_dir);
+}
+
+use Cwd;
+
+sub in_empty_repo {
+	my $coderef = shift;
+	my $old_wd = getcwd;
+	my $path = mk_tmp_repo();
+	chdir($path);
+	$coderef->();
+	chdir($old_wd);
+}
+
+use Git;
+sub tmp_git {
+	Git->repository(mk_tmp_repo);
+}
+
+# return an array ref of two unprivileged ports
+sub random_port_pair {
+	my $port = int(rand(2**16-1024-1)+1024);
+	[ $port, $port + 1 ];
+}
+
+1;
+
+# Local Variables:
+#   mode: cperl
+#   cperl-brace-offset: 0
+#   cperl-continued-brace-offset: 0
+#   cperl-indent-level: 8
+#   cperl-label-offset: -8
+#   cperl-merge-trailing-else: nil
+#   cperl-continued-statement-offset: 8
+#   cperl-indent-parens-as-block: t
+#   cperl-indent-wrt-brace: nil
+# End:
+#
+# vim: vim:tw=78:sts=0:noet
diff --git a/t/t9700/config.t b/t/t9700/config.t
new file mode 100644
index 0000000..395a5c9
--- /dev/null
+++ b/t/t9700/config.t
@@ -0,0 +1,127 @@
+#!/usr/bin/env perl -- -w
+
+use TestUtils;
+use Test::More no_plan;
+use strict;
+
+use Error qw(:try);
+
+use_ok("Git::Config");
+
+in_empty_repo sub {
+	my $git = Git->repository;
+	$git->command_oneline("config", "foo.bar", "baz");
+	$git->command_oneline("config", "list.value", "one");
+	$git->command_oneline("config", "--add", "list.value", "two");
+	$git->command_oneline("config", "foo.intval", "12g");
+	$git->command_oneline("config", "foo.false.val", "false");
+	$git->command_oneline("config", "foo.true.val", "yes");
+
+	my $conf = Git::Config->new();
+	ok($conf, "constructed a new Git::Config");
+	isa_ok($conf, "Git::Config", "Git::Config->new()");
+
+	is($conf->config("foo.bar"), "baz", "read single line");
+	$conf->config("foo.bar", "frop");
+	like($git->command_oneline("config", "foo.bar"), qr/frop/,
+		"->config() has immediate effect");
+	$conf->autoflush(0);
+	is_deeply(
+		[$conf->config("list.value")],
+		[qw(one two)],
+		"read multi-value item",
+	);
+
+	eval {
+		my $val = $conf->config("list.value");
+	};
+	like ($@, qr{multiple}i,
+	      "produced an error reading a list into a scalar");
+
+	eval {
+		$conf->config("list.value" => "single");
+	};
+	like($@, qr{multiple}i,
+	     "produced an error replacing a list with a scalar");
+
+	ok(eval { $conf->config("foo.bar", [ "baz", "frop"]); 1 },
+		"no error replacing a scalar with a list");
+
+	like($git->command_oneline("config", "foo.bar"), qr/frop/,
+		"->config() no immediate effect with autoflush = 0");
+
+	$conf->flush;
+
+	like($git->command("config", "--get-all", "foo.bar"),
+		qr/baz\s*frop/,
+		"->flush()");
+
+	$conf->config("x.y$$" => undef);
+	my $x = $conf->config("x.y$$");
+	ok(!defined $x, "unset items return undef in scalar context");
+	my @x = $conf->config("x.y$$");
+	ok(@x == 0, "unset items return empty list in list context");
+
+	$conf->config("foo.bar" => undef);
+	@x = $conf->config("foo.bar");
+	ok(@x == 0, "deleted items appear empty immediately");
+	$conf->flush;
+	ok(!eval {
+		$git->command("config", "--get-all", "foo.bar");
+		1;
+	}, "deleted items are removed from the config file");
+
+	$conf->type("foo.intval", "integer");
+	is($conf->type("foo.intval"), "integer",
+	   "->type()");
+	is($conf->config("foo.intval"), 12*1024*1024*1024,
+	   "integer thaw");
+	$conf->config("foo.intval", 1025*1024);
+	$conf->type("foo.intval", "string");
+	is($conf->config("foo.intval"), "1025k",
+	   "integer freeze");
+
+	$conf->type("foo.*.val", "boolean");
+	is($conf->type("foo.this.val"), "boolean",
+	   "wildcard types");
+	is($conf->config("foo.true.val"), 1,
+	   "boolean thaw - 'yes'");
+	is($conf->config("foo.false.val"), 0,
+	   "boolean thaw - 'false'");
+	my $unset = $conf->config("foo.bar.val");
+	is($unset, undef,
+	   "boolean thaw - not present");
+
+	$git->command_oneline("config", "foo.intval", "12g");
+	$git->command_oneline("config", "foo.falseval", "false");
+	$git->command_oneline("config", "foo.trueval", "on");
+
+	SKIP:{
+		if (eval {
+			$git->command(
+				"config", "--global",
+				"--get-all", "foo.bar"
+			       );
+			1;
+		}) {
+			skip "someone set foo.bar in global config", 1;
+		}
+		my @foo_bar = $conf->global("foo.bar");
+		is_deeply(\@foo_bar, [], "->global() reading only");
+	}
+};
+
+
+# Local Variables:
+#   mode: cperl
+#   cperl-brace-offset: 0
+#   cperl-continued-brace-offset: 0
+#   cperl-indent-level: 8
+#   cperl-label-offset: -8
+#   cperl-merge-trailing-else: nil
+#   cperl-continued-statement-offset: 8
+#   cperl-indent-parens-as-block: t
+#   cperl-indent-wrt-brace: nil
+# End:
+#
+# vim: vim:tw=78:sts=0:noet
-- 
1.6.0

[PATCH] perl: make Git.pm use new Git::Config module

[Sam Vilain]

Plug these two modules together.  The only minor API difference is that
in void context (ie, return value is not being used), the new Git::Config
function does not try to unpack the value.  So, the exist test - which was
testing an error condition - must be changed to actually try to retrieve
the values so that the exceptions can happen.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 perl/Git.pm     |  103 +++++++++++++++++++++++++++----------------------------
 t/t9700/test.pl |    4 +-
 2 files changed, 53 insertions(+), 54 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 7d7f2b1..3141b41 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -550,32 +550,38 @@ does. In scalar context requires the variable to be set only one time
 (exception is thrown otherwise), in array context returns allows the
 variable to be set multiple times and returns all the values.
 
-This currently wraps command('config') so it is not so fast.
+This delegates via L<Git::Config> for cached config file access.  To
+force a re-read of the configuration, call C<$git-E<gt>config->read>
 
-=cut
+=item config
 
-sub config {
-	my ($self, $var) = _maybe_self(@_);
+With no arguments, the C<config> method returns a L<Git::Config>
+object.  See L<Git::Config> for the API information.
 
-	try {
-		my @cmd = ('config');
-		unshift @cmd, $self if $self;
-		if (wantarray) {
-			return command(@cmd, '--get-all', $var);
-		} else {
-			return command_oneline(@cmd, '--get', $var);
-		}
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return;
-		} else {
-			throw $E;
-		}
+=item config ( VARIABLE => value )
+
+With two arguments, the configuration is updated with the passed
+value.
+
+=cut
+
+sub _config {
+	my ($self) = _maybe_self_new(@_);
+	$self->{config} ||= do {
+		require Git::Config;
+		Git::Config->new($self);
 	};
 }
 
+sub config {
+	(my $self, @_) = _maybe_self_new(@_);
+	if (@_) {
+		return $self->_config->config(@_);
+	}
+	else {
+		return $self->_config;
+	}
+}
 
 =item config_bool ( VARIABLE )
 
@@ -583,28 +589,21 @@ Retrieve the bool configuration C<VARIABLE>. The return value
 is usable as a boolean in perl (and C<undef> if it's not defined,
 of course).
 
-This currently wraps command('config') so it is not so fast.
+This delegates via L<Git::Config> for cached config file access.
+
+=item config_bool ( VARIABLE => value )
+
+Sets a boolean slot to the given value.  This always writes 'true' or
+'false' to the configuration file, regardless of the value passed.
 
 =cut
 
 sub config_bool {
-	my ($self, $var) = _maybe_self(@_);
+	(my ($self, $var), @_) = _maybe_self_new(@_);
 
-	try {
-		my @cmd = ('config', '--bool', '--get', $var);
-		unshift @cmd, $self if $self;
-		my $val = command_oneline(@cmd);
-		return undef unless defined $val;
-		return $val eq 'true';
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $conf = $self->_config;
+	$conf->type($var => "boolean");
+	$conf->config($var, @_);
 }
 
 =item config_int ( VARIABLE )
@@ -615,26 +614,22 @@ or 'g' in the config file will cause the value to be multiplied
 by 1024, 1048576 (1024^2), or 1073741824 (1024^3) prior to output.
 It would return C<undef> if configuration variable is not defined,
 
-This currently wraps command('config') so it is not so fast.
+This delegates via L<Git::Config> for cached config file access.
+
+=item config_int ( VARIABLE => value )
+
+Sets a integer slot to the given value.  This method will reduce the
+written value to the shortest way to express the given number; eg
+1024 is written as C<1k> and 16777216 will be written as C<16M>.
 
 =cut
 
 sub config_int {
-	my ($self, $var) = _maybe_self(@_);
+	(my ($self, $var), @_) = _maybe_self_new(@_);
 
-	try {
-		my @cmd = ('config', '--int', '--get', $var);
-		unshift @cmd, $self if $self;
-		return command_oneline(@cmd);
-	} catch Git::Error::Command with {
-		my $E = shift;
-		if ($E->value() == 1) {
-			# Key not found.
-			return undef;
-		} else {
-			throw $E;
-		}
-	};
+	my $conf = $self->_config;
+	$conf->type($var => "integer");
+	$conf->config($var, @_);
 }
 
 =item get_colorbool ( NAME )
@@ -1204,6 +1199,7 @@ either version 2, or (at your option) any later version.
 
 =cut
 
+our $_self;
 
 # Take raw method argument list and return ($obj, @args) in case
 # the method was called upon an instance and (undef, @args) if
@@ -1211,6 +1207,9 @@ either version 2, or (at your option) any later version.
 sub _maybe_self {
 	UNIVERSAL::isa($_[0], 'Git') ? @_ : (undef, @_);
 }
+sub _maybe_self_new {
+	UNIVERSAL::isa($_[0], 'Git') ? @_ : ($_self||=Git->repository, @_);
+}
 
 # Check if the command id is something reasonable.
 sub _check_valid_cmd {
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 697daf3..1b94633 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -35,9 +35,9 @@ is($r->get_color("color.test.slot1", "red"), $ansi_green, "get_color");
 # Save and restore STDERR; we will probably extract this into a
 # "dies_ok" method and possibly move the STDERR handling to Git.pm.
 open our $tmpstderr, ">&STDERR" or die "cannot save STDERR"; close STDERR;
-eval { $r->config("test.dupstring") };
+eval { my $x = $r->config("test.dupstring") };
 ok($@, "config: duplicate entry in scalar context fails");
-eval { $r->config_bool("test.boolother") };
+eval { my $x = $r->config_bool("test.boolother") };
 ok($@, "config_bool: non-boolean values fail");
 open STDERR, ">&", $tmpstderr or die "cannot restore STDERR";
 
-- 
1.6.0

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Frank Lichtenheld]

On Mon, Apr 06, 2009 at 11:46:15AM +1200, Sam Vilain wrote:
> +	my ($fh, $c) = $git->command_output_pipe(
> +		'config', ( $which ? ("--$which") : () ),
> +		'--list',
> +	       );
> +	my $read_state = {};
> +
> +	while (<$fh>) {
> +		my ($item, $value) = m{(.*?)=(.*)};
> +		my $sl = \( $read_state->{$item} );
> +		if (!defined $$sl) {
> +			$$sl = $value;
> +		}
> +		elsif (!ref $$sl) {
> +			$$sl = [ $$sl, $value ];
> +		}
> +		else {
> +			push @{ $$sl }, $value;
> +		}
> +	}

Any reason why you don't use --null here? The output of --list without --null
is not reliably parsable, since people can put newlines in values.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Sam Vilain]

On Mon, 2009-04-06 at 11:29 +0200, Frank Lichtenheld wrote:
> On Mon, Apr 06, 2009 at 11:46:15AM +1200, Sam Vilain wrote:
> > +	my ($fh, $c) = $git->command_output_pipe(
> > +		'config', ( $which ? ("--$which") : () ),
> > +		'--list',
> > +	       );
> Any reason why you don't use --null here? The output of --list without --null
> is not reliably parsable, since people can put newlines in values.

No particularly good reason :-)

Subject: [PATCH] perl: make Git::Config use --null

Use the form of 'git-config' designed for parsing by modules like
this for safety with values containing embedded line feeds.

Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>
---
 perl/Git/Config.pm |    6 ++++--
 t/t9700/config.t   |    4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/perl/Git/Config.pm b/perl/Git/Config.pm
index a0a6a41..a35d9f3 100644
--- a/perl/Git/Config.pm
+++ b/perl/Git/Config.pm
@@ -179,12 +179,14 @@ sub read {
 
 	my ($fh, $c) = $git->command_output_pipe(
 		'config', ( $which ? ("--$which") : () ),
-		'--list',
+		 '--null', '--list',
 	       );
 	my $read_state = {};
 
+	local($/)="\0";
 	while (<$fh>) {
-		my ($item, $value) = m{(.*?)=(.*)};
+		my ($item, $value) = m{(.*?)\n((?s:.*))\0}
+			or die "failed to parse it; \$_='$_'";
 		my $sl = \( $read_state->{$item} );
 		if (!defined $$sl) {
 			$$sl = $value;
diff --git a/t/t9700/config.t b/t/t9700/config.t
index 395a5c9..f0f7d2d 100644
--- a/t/t9700/config.t
+++ b/t/t9700/config.t
@@ -16,6 +16,7 @@ in_empty_repo sub {
 	$git->command_oneline("config", "foo.intval", "12g");
 	$git->command_oneline("config", "foo.false.val", "false");
 	$git->command_oneline("config", "foo.true.val", "yes");
+	$git->command_oneline("config", "multiline.val", "hello\nmultiline.val=world");
 
 	my $conf = Git::Config->new();
 	ok($conf, "constructed a new Git::Config");
@@ -92,6 +93,9 @@ in_empty_repo sub {
 	is($unset, undef,
 	   "boolean thaw - not present");
 
+	like($conf->config("multiline.val"), qr{\n},
+	     "parsing multi-line values");
+
 	$git->command_oneline("config", "foo.intval", "12g");
 	$git->command_oneline("config", "foo.falseval", "false");
 	$git->command_oneline("config", "foo.trueval", "on");
-- 
debian.1.5.6.1

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Jakub Narebski]

Sam Vilain <sam.vilain@catalyst.net.nz> writes:

> On Mon, 2009-04-06 at 11:29 +0200, Frank Lichtenheld wrote:
> > On Mon, Apr 06, 2009 at 11:46:15AM +1200, Sam Vilain wrote:

> > > +	my ($fh, $c) = $git->command_output_pipe(
> > > +		'config', ( $which ? ("--$which") : () ),
> > > +		'--list',
> > > +	       );
> >
> > Any reason why you don't use --null here? The output of --list
> > without --null is not reliably parsable, since people can put
> > newlines in values.
> 
> No particularly good reason :-)
> 
> Subject: [PATCH] perl: make Git::Config use --null
> 
> Use the form of 'git-config' designed for parsing by modules like
> this for safety with values containing embedded line feeds.
> 
> Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>

> diff --git a/perl/Git/Config.pm b/perl/Git/Config.pm
> index a0a6a41..a35d9f3 100644
> --- a/perl/Git/Config.pm
> +++ b/perl/Git/Config.pm
> @@ -179,12 +179,14 @@ sub read {
>  
>  	my ($fh, $c) = $git->command_output_pipe(
>  		'config', ( $which ? ("--$which") : () ),
> -		'--list',
> +		 '--null', '--list',
>  	       );
>  	my $read_state = {};
>  
> +	local($/)="\0";
>  	while (<$fh>) {
> -		my ($item, $value) = m{(.*?)=(.*)};
> +		my ($item, $value) = m{(.*?)\n((?s:.*))\0}
> +			or die "failed to parse it; \$_='$_'";
>  		my $sl = \( $read_state->{$item} );
>  		if (!defined $$sl) {
>  			$$sl = $value;

Errr... wouldn't it be better to simply use 

+		my ($item, $value) = split("\n", $_, 2)

here? Have you tested Git::Config with a "null" value, i.e. something
like

    [section]
        noval

in the config file (which evaluates to 'true' with '--bool' option)?
Because from what I remember from the discussion on the 
"git config --null --list" format the lack of "\n" is used to
distinguish between noval (which is equivalent to 'true'), and empty
value (which is equivalent to 'false')

    [boolean
        noval        # equivalent to 'true'
        empty1 =     # equivalent to 'false'
        empty2 = ""  # equivalent to 'false'

> diff --git a/t/t9700/config.t b/t/t9700/config.t
> index 395a5c9..f0f7d2d 100644
> --- a/t/t9700/config.t
> +++ b/t/t9700/config.t
> @@ -16,6 +16,7 @@ in_empty_repo sub {
>  	$git->command_oneline("config", "foo.intval", "12g");
>  	$git->command_oneline("config", "foo.false.val", "false");
>  	$git->command_oneline("config", "foo.true.val", "yes");
> +	$git->command_oneline("config", "multiline.val", "hello\nmultiline.val=world");
>  
>  	my $conf = Git::Config->new();
>  	ok($conf, "constructed a new Git::Config");

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Sam Vilain]

Jakub Narebski wrote:
>> -		my ($item, $value) = m{(.*?)=(.*)};
>> +		my ($item, $value) = m{(.*?)\n((?s:.*))\0}
>> +			or die "failed to parse it; \$_='$_'";
> 
> Errr... wouldn't it be better to simply use 
> 
> +		my ($item, $value) = split("\n", $_, 2)
> 
> here?

Yeah, I guess that's easier to read and possibly faster; both are
using the regexp engine and using COW strings though, so it's probably
not as bad as one might think.

> Have you tested Git::Config with a "null" value, i.e. something
> like
> 
>     [section]
>         noval
> 
> in the config file (which evaluates to 'true' with '--bool' option)?
> Because from what I remember from the discussion on the 
> "git config --null --list" format the lack of "\n" is used to
> distinguish between noval (which is equivalent to 'true'), and empty
> value (which is equivalent to 'false')
> 
>     [boolean]
>         noval        # equivalent to 'true'
>         empty1 =     # equivalent to 'false'
>         empty2 = ""  # equivalent to 'false'

That I didn't consider.  Below is a patch for this.  Any more gremlins?

Subject: perl: fix no value items in Git::Config

When interpreted as boolean, items in the configuration which do not
have an '=' are interpreted as true.  Parse for this situation, and
represent it with an object in the state hash which works a bit like
undef, but isn't.  Various internal tests that items were multiple
values with ref() must become stricter.  Reported by Jakub Narebski.
Sneak a couple of vim footer changes in too.

Signed-off-by: Sam Vilain <sam@vilain.net>
---
  pull 'perl-Config' from git://github.com/samv/git for a rebased
  version without the vim footer changes, and with cleaner whitespace.

 perl/Git/Config.pm |   36 +++++++++++++++++++++++++++---------
 t/t9700/config.t   |   22 +++++++++++++++++++++-
 2 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/perl/Git/Config.pm b/perl/Git/Config.pm
index a35d9f3..6b4d928 100644
--- a/perl/Git/Config.pm
+++ b/perl/Git/Config.pm
@@ -144,7 +144,7 @@ sub _config {
 	}
 
 	if (defined wantarray) {
-		my @values = ref $state ? @$state :
+		my @values = ref($state) eq "ARRAY" ? @$state :
 			defined $state ? ($state) : ();
 
 		if ( my $type = $self->type( $item ) ) {
@@ -171,6 +171,8 @@ Reads the current state of the configuration file.
 
 =cut
 
+our $NOVALUE = bless [__PACKAGE__."/NOVALUE"], "Git::Config::novalue";
+
 sub read {
 	my $self = shift;
 	my $which = shift;
@@ -185,13 +187,19 @@ sub read {
 
 	local($/)="\0";
 	while (<$fh>) {
-		my ($item, $value) = m{(.*?)\n((?s:.*))\0}
-			or die "failed to parse it; \$_='$_'";
+		my ($item, $value) = split "\n", $_, 2;
+		if (defined $value) {
+			chop($value);
+		} else {
+			chop($item);
+			$value = $NOVALUE;
+		}
+		my $exists = exists $read_state->{$item};
 		my $sl = \( $read_state->{$item} );
-		if (!defined $$sl) {
+		if (!$exists) {
 			$$sl = $value;
 		}
-		elsif (!ref $$sl) {
+		elsif (!ref $$sl or ref $$sl ne "ARRAY") {
 			$$sl = [ $$sl, $value ];
 		}
 		else {
@@ -325,7 +333,7 @@ sub _write {
 		if ($type ne "string") {
 			push @cmd, "--$type";
 		}
-		if (ref $value) {
+		if (ref $value eq "ARRAY") {
 			$git->command_oneline (
 				"config", @cmd, "--replace-all",
 				 $item, $value->[0],
@@ -378,7 +386,7 @@ sub thaw {
 {
 	package Git::Config::string;
 	sub freeze { shift }
-	sub thaw   { shift }
+	sub thaw   { (shift)."" }
 }
 {
 	package Git::Config::integer;
@@ -408,6 +416,15 @@ sub thaw {
 	}
 }
 {
+	package Git::Config::novalue;
+	sub as_string { "" }
+	sub as_num    {  0 }
+	use overload
+		'""' => \&as_string,
+		'0+' => \&as_num,
+		fallback => 1;
+}
+{
 	package Git::Config::boolean;
 	our @true = qw(true yes 1);
 	our @false = qw(false no 0);
@@ -424,7 +441,8 @@ sub thaw {
 	}
 	sub thaw {
 		my $val = shift;
-		if ($val =~ m{$true_re}) {
+		if (eval{$val->isa("Git::Config::novalue")}
+			    or $val =~ m{$true_re}) {
 			1;
 		}
 		elsif ($val =~ m{$false_re}) {
@@ -464,4 +482,4 @@ Perl Artistic License 2.0 or later, or the GPL v2 or later.
 #   cperl-indent-wrt-brace: nil
 # End:
 #
-# vim: vim:tw=78:sts=0:noet
+# vim: tw=78:sts=0:noet
diff --git a/t/t9700/config.t b/t/t9700/config.t
index f0f7d2d..9d7860f 100644
--- a/t/t9700/config.t
+++ b/t/t9700/config.t
@@ -17,6 +17,14 @@ in_empty_repo sub {
 	$git->command_oneline("config", "foo.false.val", "false");
 	$git->command_oneline("config", "foo.true.val", "yes");
 	$git->command_oneline("config", "multiline.val", "hello\nmultiline.val=world");
+	open(CONFIG, ">>.git/config") or die $!;
+	print CONFIG <<CONF;
+[boolean]
+   noval
+   empty1 =
+   empty2 = ""
+CONF
+	close CONFIG;
 
 	my $conf = Git::Config->new();
 	ok($conf, "constructed a new Git::Config");
@@ -100,6 +108,18 @@ in_empty_repo sub {
 	$git->command_oneline("config", "foo.falseval", "false");
 	$git->command_oneline("config", "foo.trueval", "on");
 
+	is($conf->config("boolean.noval"), "", "noval: string");
+	is($conf->config("boolean.empty1"), "", "empty1: string");
+	is($conf->config("boolean.empty2"), "", "empty2: string");
+
+	$conf->type("boolean.*" => "boolean");
+
+	ok($conf->config("boolean.noval"), "noval: boolean");
+	eval{my $x = $conf->config("boolean.empty1")};
+	ok($@, "empty1: boolean");
+	eval{my $x = $conf->config("boolean.empty2")};
+	ok($@, "empty2: boolean");
+
 	SKIP:{
 		if (eval {
 			$git->command(
@@ -128,4 +148,4 @@ in_empty_repo sub {
 #   cperl-indent-wrt-brace: nil
 # End:
 #
-# vim: vim:tw=78:sts=0:noet
+# vim: tw=78:sts=0:noet
-- 
1.6.0

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Errr... wouldn't it be better to simply use 
>
> +		my ($item, $value) = split("\n", $_, 2)
>
> here? Have you tested Git::Config with a "null" value, i.e. something
> like
>
>     [section]
>         noval
>
> in the config file (which evaluates to 'true' with '--bool' option)?
> Because from what I remember from the discussion on the 
> "git config --null --list" format the lack of "\n" is used to
> distinguish between noval (which is equivalent to 'true'), and empty
> value (which is equivalent to 'false')
>
>     [boolean
>         noval        # equivalent to 'true'
>         empty1 =     # equivalent to 'false'
>         empty2 = ""  # equivalent to 'false'

I do not mind if the _write method always wrote out

	[core]
        	autocrlf = true

for a variable that is true, but it should be able to read existing

	[core]
        	autocrlf

correctly.

Sam, I think you meant to make me squash the "Oops, for no good reason,
here is a fix-up" into the previous one, but for this case, I'd appreciate
a re-roll of the series, that includes a test to read from an existing
configuration file that contains such "presense of the name alone means
boolean true" variables.

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Petr Baudis]

On Mon, Apr 06, 2009 at 11:46:15AM +1200, Sam Vilain wrote:
> Add a new module, Git::Config, for a better Git configuration API.
> 
> Signed-off-by: Sam Vilain <sam.vilain@catalyst.net.nz>

I'm really sorry that I probably won't have time soon to properly review
this patch. :-(

> +			throw Error::Simple (
> +				"'$item' is specified multiple times",
> +			       );

So just one comment - in general people seem to be unhappy with this way
of exception handling, preferring die-eval to throw-catch. We might
seize the opportunity here and start using die in all new modules,
keeping Error::Simple only in legacy Git.pm (and its wrappers for the
Git::Config stuff).

-- 
				Petr "Pasky" Baudis
The average, healthy, well-adjusted adult gets up at seven-thirty
in the morning feeling just terrible. -- Jean Kerr

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Sam Vilain]

Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>   
>> Errr... wouldn't it be better to simply use 
>>
>> +		my ($item, $value) = split("\n", $_, 2)
>>
>> here? Have you tested Git::Config with a "null" value, i.e. something
>> like
>>
>>     [section]
>>         noval
>>
>> in the config file (which evaluates to 'true' with '--bool' option)?
>> Because from what I remember from the discussion on the 
>> "git config --null --list" format the lack of "\n" is used to
>> distinguish between noval (which is equivalent to 'true'), and empty
>> value (which is equivalent to 'false')
>>
>>     [boolean
>>         noval        # equivalent to 'true'
>>         empty1 =     # equivalent to 'false'
>>         empty2 = ""  # equivalent to 'false'
>>     
>
> I do not mind if the _write method always wrote out
>
> 	[core]
>         	autocrlf = true
>
> for a variable that is true, but it should be able to read existing
>
> 	[core]
>         	autocrlf
>
> correctly.
>   

Yep - that's what I thought was reasonable behaviour as well and what my 
submission does.

> Sam, I think you meant to make me squash the "Oops, for no good reason,
> here is a fix-up" into the previous one, but for this case, I'd appreciate
> a re-roll of the series, that includes a test to read from an existing
> configuration file that contains such "presense of the name alone means
> boolean true" variables.
>   

Sure, I rebased the series to have the fix-ups at the right places, but 
didn't think it was an interesting enough change to rate a full 
re-submission. The series at git://github.com/samv/git branch 
perl-Config has the minor change put into the place it was introduced. I 
put a little note to this effect after the --- line.

I'm not quite sure what you want squashed where, maybe just edit the 
below list to be how you'd like it,

pick d43238e perl: add new module Git::Config for cached 'git config' access
pick 5ea135d perl: make Git.pm use new Git::Config module
pick b2865bc perl: make Git::Config use --null
pick 28eecdc perl: fix no value items in Git::Config

:-)

Sam

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Jakub Narebski]

By the way, did you take a look how cached 'git config' access and
typecasting is done in gitweb?  See commit b201927 (gitweb: Read
repo config using 'git config -z -l') and following similar commits.

On Wed, 8 April 2009, Sam Vilain wrote:
> Jakub Narebski wrote:

>>> -		my ($item, $value) = m{(.*?)=(.*)};
>>> +		my ($item, $value) = m{(.*?)\n((?s:.*))\0}
>>> +			or die "failed to parse it; \$_='$_'";
>> 
>> Errr... wouldn't it be better to simply use 
>> 
>> +		my ($item, $value) = split("\n", $_, 2)
>> 
>> here?
> 
> Yeah, I guess that's easier to read and possibly faster; both are
> using the regexp engine and using COW strings though, so it's probably
> not as bad as one might think.

The version using 'split' has the advantage that for config variable
with no value (e.g. "[section] noval") it sets $item (why this variable
is called $item and not $var, $variable or $key, BTW.?) to fully 
qualified variable name (e.g. "section.noval"), and sets $value to 
undef, instead of failing like your original version using regexp.

And I also think that this version is easier to understand, and might be 
a bit faster as well; but it is more important to be easier to 
understand.

>> Have you tested Git::Config with a "null" value, i.e. something
>> like
>> 
>>     [section]
>>         noval
>> 
>> in the config file (which evaluates to 'true' with '--bool' option)?
>> Because from what I remember from the discussion on the 
>> "git config --null --list" format the lack of "\n" is used to
>> distinguish between noval (which is equivalent to 'true'), and empty
>> value (which is equivalent to 'false')
>> 
>>     [boolean]
>>         noval        # equivalent to 'true'
>>         empty1 =     # equivalent to 'false'
>>         empty2 = ""  # equivalent to 'false'
> 
> That I didn't consider.  Below is a patch for this.  Any more
> gremlins? 

I have nor examined your patch in detail; I'll try to do it soon,
but with git config file parsing there lies following traps.

1. In fully qualified variable name section name and variable name
   have to be compared case insensitive (or normalized, i.e.
   lowercased), while subsection part (if it exists) is case sensitive.

2. When coercing type to bool, you need to remember (and test) that
   there are values which are truish (no value, 'true', 'yes', non-zero
   integer usually 1), values which are falsish (empry, 'false', 'no',
   0); other values IIRC are truish too.

3. When coercing type to int, you need to remember about optional
   value suffixes: 'k', 'm' or 'g'.

4. I don't know if you remembered about 'colorbool' and 'color'; the
   latter would probably require some extra CPAN module for ANSI color
   escapes... or copying color codes from the C version.

> 
> Subject: perl: fix no value items in Git::Config
> 
> When interpreted as boolean, items in the configuration which do not
> have an '=' are interpreted as true.  Parse for this situation, and
> represent it with an object in the state hash which works a bit like
> undef, but isn't.

Why not represent it simply as an 'undef'? You can always distinguish 
between not defined and not existing by using 'exists'...

> Sneak a couple of vim footer changes in too.

Hmmm...

> 
> Signed-off-by: Sam Vilain <sam@vilain.net>

[...]
-- 
Jakub Narebski
Poland

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Sam Vilain]

Jakub Narebski wrote:
> By the way, did you take a look how cached 'git config' access and
> typecasting is done in gitweb?  See commit b201927 (gitweb: Read
> repo config using 'git config -z -l') and following similar commits.
>   

Right ... sure, looks fairly straightforward.  I guess gitweb could 
potentially use this tested module instead of including that code 
itself.  Also various parts of git-svn... anything really.

I actually wrote this code because I wanted something a bit nicer for 
writing the mirror-sync initial implementations.  And I wanted to have a 
bit of control over when values get committed, and save work for 
reading, so I wrote this.

>> Any more gremlins? 
>>     
> I have nor examined your patch in detail; I'll try to do it soon,
> but with git config file parsing there lies following traps.
>
> 1. In fully qualified variable name section name and variable name
>    have to be compared case insensitive (or normalized, i.e.
>    lowercased), while subsection part (if it exists) is case sensitive.
>   

I noticed that 'git config' hides this by normalising the case of what 
it outputs with 'git config --list'; do you think anything special is 
required in light of this?

> 2. When coercing type to bool, you need to remember (and test) that
>    there are values which are truish (no value, 'true', 'yes', non-zero
>    integer usually 1), values which are falsish (empry, 'false', 'no',
>    0); other values IIRC are truish too.
>   

Yep, see the Git::Config::boolean mini-package which has a list of 
those.  I think I used the documented legal values, which are 'true', 
'yes' and '1' for affirmative and 'false', 'no' and '0' for negative.  I 
guess I could make that include non-zero integers as well.

> 3. When coercing type to int, you need to remember about optional
>    value suffixes: 'k', 'm' or 'g'.
>   

Yep, covered on input and output :-).  See Git::Config::integer for the 
conversion functions.

> 4. I don't know if you remembered about 'colorbool' and 'color'; the
>    latter would probably require some extra CPAN module for ANSI color
>    escapes... or copying color codes from the C version.
>   

Yeah, I thought those could probably be done with a follow-up patch.  
It's just a matter of writing functions Git::Config::color::thaw and 
::freeze.

> Why not represent it simply as an 'undef'? You can always distinguish 
> between not defined and not existing by using 'exists'...
>   

I don't like 'undef' being a data value.  In this case I was already 
using setting a value to undef to tell the module to remove the key from 
the config file.  But in any case you should not need to care what form 
the values exist in the internal ->{read_state} hash, as you should 
always be retrieving them using the ->config method, which will marshall 
them into the type you want.  Note it's always the same object, just 
like Perl's &PL_undef via the C API.

>> Sneak a couple of vim footer changes in too.
>>     
>
> Hmmm...
>   

Guess someone noticed them.  Oh well, rebase time ...

Thanks for your input Jakub, I'll incorporate your suggestions.

Sam

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Junio C Hamano]

Sam Vilain <samv@catalyst.net.nz> writes:

> I'm not quite sure what you want squashed where, maybe just edit the
> below list to be how you'd like it,
>
> pick d43238e perl: add new module Git::Config for cached 'git config' access
> pick 5ea135d perl: make Git.pm use new Git::Config module
> pick b2865bc perl: make Git::Config use --null
> pick 28eecdc perl: fix no value items in Git::Config

For example, if Git::Config is a new thing that this series brings in, and
if the final decision made before the series is integrated into my tree is
that it should read from --null output, then that makes the third one an
"Oops, it was a thinko---I should have used --null from day one" fix-up
that shouldn't be in the resulting series as a separate commit, doesn't
it?  I think the same applies to the last "fix no value 'true' variables"
one.

Re: [PATCH] perl: add new module Git::Config for cached 'git config' access

[Jakub Narebski]

On Wed, 8 Apr 2009, Sam Vilain wrote:
> Jakub Narebski wrote:
>>
>> By the way, did you take a look how cached 'git config' access and
>> typecasting is done in gitweb?  See commit b201927 (gitweb: Read
>> repo config using 'git config -z -l') and following similar commits.
> 
> Right ... sure, looks fairly straightforward.  I guess gitweb could 
> potentially use this tested module instead of including that code 
> itself.  Also various parts of git-svn... anything really.

Well... first, gitweb use of config files is simplified to what, I guess,
you want, because it only considers _reading_ config, and doesn't worry
about writing and (what is I guess most difficult) rewriting config file.

Second, gitweb doesn't even use Git.pm (although "gitweb caching" project
by Lea Wiemann from GSoC 2008 introduced Git::Repo, the alternate OO
interface, and used it in caching gitweb).  This has the advantage of
being slightly easier to install... but we require git anyway, so it
is not much more diffucult requiring perl-Git / Git.pm.

> 
> I actually wrote this code because I wanted something a bit nicer for 
> writing the mirror-sync initial implementations.  And I wanted to have a 
> bit of control over when values get committed, and save work for 
> reading, so I wrote this.

Well, you could have written in C instead ;-)

>>> Any more gremlins? 
>>>     
>> I have nor examined your patch in detail; I'll try to do it soon,
>> but with git config file parsing there lies following traps.
>>
>> 1. In fully qualified variable name section name and variable name
>>    have to be compared case insensitive (or normalized, i.e.
>>    lowercased), while subsection part (if it exists) is case sensitive.
> 
> I noticed that 'git config' hides this by normalising the case of what 
> it outputs with 'git config --list'; do you think anything special is 
> required in light of this?

I'm not sure. I was thinking that get() method should normalize its
arguments before comparing... but I am not sure if it is necessary
(or even if it is a good idea).

>> 2. When coercing type to bool, you need to remember (and test) that
>>    there are values which are truish (no value, 'true', 'yes', non-zero
>>    integer usually 1), values which are falsish (empry, 'false', 'no',
>>    0); other values IIRC are truish too.
> 
> Yep, see the Git::Config::boolean mini-package which has a list of 
> those.  I think I used the documented legal values, which are 'true', 
> 'yes' and '1' for affirmative and 'false', 'no' and '0' for negative.  I 
> guess I could make that include non-zero integers as well.

They are, from what I understand, empty value, 'false', 'no' and '0' for
negative, all else is positive (which includes no value, 'true', 'yes'
and '1').  But you'd better check the C code yourself.

[...]
>> Why not represent it simply as an 'undef'? You can always distinguish 
>> between not defined and not existing by using 'exists'...
>>   
> 
> I don't like 'undef' being a data value.

Why not? It is IMHO the most natural way.

>                                           In this case I was already  
> using setting a value to undef to tell the module to remove the key from 
> the config file.

Why not use 'delete' to remove hash element, and 'exists' to check
whether it exists?


-- 
Jakub Narebski
Poland