[PATCH] t9400: Use the repository config and nothing else.

[PATCH] t9400: Use the repository config and nothing else.

[Junio Hamano]

git-cvsserver has a bug in its configuration file output parser
that makes it choke if the configuration has these:

        [diff]
                color = auto
        [diff.color]
                whitespace = blue reverse

This needs to be fixed, but thanks to that bug, a separate bug
in t9400 test script was discovered.  The test discarded
GIT_CONFIG instead of pointing at the proper one to be used in
the exoprted repository.  This allowed user's .gitconfig and (if
exists) systemwide /etc/gitconfig to affect the outcome of the
test, which is a big no-no.

The patch fixes the problem in the test.  Fixing the
git-cvsserver's configuration parser is left as an exercise to
motivated volunteers ;-)

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * I think the fix to git-cvsserver should be straightforward.
   Instead of parsing two or three level names in a hierarchical
   hash around ll.187-192 for *all* config items, it should just
   parse gitcvs related ones only, as that are the only ones
   that the script cares about.

 t/t9400-git-cvsserver-server.sh |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh
index f137b30..b136a97 100755
--- a/t/t9400-git-cvsserver-server.sh
+++ b/t/t9400-git-cvsserver-server.sh
@@ -26,6 +26,7 @@ perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
 unset GIT_DIR GIT_CONFIG
 WORKDIR=$(pwd)
 SERVERDIR=$(pwd)/gitcvs.git
+git_config=$SERVERDIR/config
 CVSROOT=":fork:$SERVERDIR"
 CVSWORK=$(pwd)/cvswork
 CVS_SERVER=git-cvsserver
@@ -43,7 +44,7 @@ echo >empty &&
 # note that cvs doesn't accept absolute pathnames
 # as argument to co -d
 test_expect_success 'basic checkout' \
-  'cvs -Q co -d cvswork master &&
+  'GIT_CONFIG="$git_config" cvs -Q co -d cvswork master &&
    test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"'
 
 test_expect_success 'cvs update (create new file)' \
@@ -52,7 +53,7 @@ test_expect_success 'cvs update (create new file)' \
    git commit -q -m "Add testfile1" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
-   cvs -Q update &&
+   GIT_CONFIG="$git_config" cvs -Q update &&
    test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.1/" &&
    diff -q testfile1 ../testfile1'
 
@@ -63,7 +64,7 @@ test_expect_success 'cvs update (update existing file)' \
    git commit -q -m "Append to testfile1" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
-   cvs -Q update &&
+   GIT_CONFIG="$git_config" cvs -Q update &&
    test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.2/" &&
    diff -q testfile1 ../testfile1'
 
@@ -76,7 +77,7 @@ test_expect_failure "cvs update w/o -d doesn't create subdir (TODO)" \
    git commit -q -m "Single Subdirectory" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
-   cvs -Q update &&
+   GIT_CONFIG="$git_config" cvs -Q update &&
    test ! -d test'
 
 cd "$WORKDIR"
@@ -89,7 +90,7 @@ test_expect_success 'cvs update (subdirectories)' \
    git commit -q -m "deep sub directory structure" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
-   cvs -Q update -d &&
+   GIT_CONFIG="$git_config" cvs -Q update -d &&
    (for dir in A A/B A/B/C A/D E; do
       filename="file_in_$(echo $dir|sed -e "s#/# #g")" &&
       if test "$(echo $(grep -v ^D $dir/CVS/Entries|cut -d/ -f2,3,5))" = "$filename/1.1/" &&
@@ -107,7 +108,7 @@ test_expect_success 'cvs update (delete file)' \
    git commit -q -m "Remove testfile1" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
-   cvs -Q update &&
+   GIT_CONFIG="$git_config" cvs -Q update &&
    test -z "$(grep testfile1 CVS/Entries)" &&
    test ! -f testfile1'
 
@@ -118,7 +119,7 @@ test_expect_success 'cvs update (re-add deleted file)' \
    git commit -q -m "Re-Add testfile1" &&
    git push gitcvs.git >/dev/null &&
    cd cvswork &&
-   cvs -Q update &&
+   GIT_CONFIG="$git_config" cvs -Q update &&
    test "$(echo $(grep testfile1 CVS/Entries|cut -d/ -f2,3,5))" = "testfile1/1.4/" &&
    diff -q testfile1 ../testfile1'
 

Re: [PATCH] t9400: Use the repository config and nothing else.

[Frank Lichtenheld]

On Fri, May 11, 2007 at 04:35:18PM -0700, Junio Hamano wrote:
> This needs to be fixed, but thanks to that bug, a separate bug
> in t9400 test script was discovered.  The test discarded
> GIT_CONFIG instead of pointing at the proper one to be used in
> the exoprted repository.  This allowed user's .gitconfig and (if
> exists) systemwide /etc/gitconfig to affect the outcome of the
> test, which is a big no-no.

Shouldn't you also remove the "unset GIT_CONFIG" then?

> @@ -26,6 +26,7 @@ perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
>  unset GIT_DIR GIT_CONFIG
>  WORKDIR=$(pwd)
>  SERVERDIR=$(pwd)/gitcvs.git
> +git_config=$SERVERDIR/config
>  CVSROOT=":fork:$SERVERDIR"
>  CVSWORK=$(pwd)/cvswork

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

Re: [PATCH] t9400: Use the repository config and nothing else.

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> On Fri, May 11, 2007 at 04:35:18PM -0700, Junio Hamano wrote:
>> This needs to be fixed, but thanks to that bug, a separate bug
>> in t9400 test script was discovered.  The test discarded
>> GIT_CONFIG instead of pointing at the proper one to be used in
>> the exoprted repository.  This allowed user's .gitconfig and (if
>> exists) systemwide /etc/gitconfig to affect the outcome of the
>> test, which is a big no-no.
>
> Shouldn't you also remove the "unset GIT_CONFIG" then?

I didn't test the side of the test that works on the git side,
but I think you are right.

Re: [PATCH] t9400: Use the repository config and nothing else.

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> Frank Lichtenheld <frank@lichtenheld.de> writes:
>
>> On Fri, May 11, 2007 at 04:35:18PM -0700, Junio Hamano wrote:
>>> This needs to be fixed, but thanks to that bug, a separate bug
>>> in t9400 test script was discovered.  The test discarded
>>> GIT_CONFIG instead of pointing at the proper one to be used in
>>> the exoprted repository.  This allowed user's .gitconfig and (if
>>> exists) systemwide /etc/gitconfig to affect the outcome of the
>>> test, which is a big no-no.
>>
>> Shouldn't you also remove the "unset GIT_CONFIG" then?
>
> I didn't test the side of the test that works on the git side,
> but I think you are right.

Actually that is not sufficient, as unsetting means using the
value set in test-lib.sh suitable for usual single-repository
tests.

When you prepare gitcvs.enabled config in the cloned gitcvs.git
repository, you do not want to have GIT_CONFIG=.git/config in
the environment.  As you give GIT_DIR to these two commands, not
having GIT_CONFIG would make them do the right thing.

Re: [PATCH] t9400: Use the repository config and nothing else.

[Frank Lichtenheld]

On Sat, May 12, 2007 at 10:21:15AM -0700, Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
> When you prepare gitcvs.enabled config in the cloned gitcvs.git
> repository, you do not want to have GIT_CONFIG=.git/config in
> the environment.  As you give GIT_DIR to these two commands, not
> having GIT_CONFIG would make them do the right thing.

Yeah, which was the reason I unset it in the first place. But
if your concern is not to use other config files it should still
set GIT_CONFIG explicetly for these cases and leave it to the
default for all calls inside the non-bare repository, right?

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

[PATCH] cvsserver: Complete rewrite of the configuration parser

[Frank Lichtenheld]

Move the configuration parsing to a separate GITCVS::config
module. Simplifies using the configuration in the rest of
the code.

Restrict parsed configuration variables to
^gitcvs\.((ext|pserver)\.)?
since we don't use anything else anyway. This also
reduces the risk of getting confused with arbitrary
variables (especially arbitrary subsection names).

Also fixes a bug where the config parser got confused
if a section had a subsection and a variable with the
same name.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 git-cvsserver.perl |  187 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 158 insertions(+), 29 deletions(-)

 Maybe a bit overkill if one only wants to solve the problem Junio discovered
 but I believe it's still worthwile.

 Has a lot of overlap with perl/Git.pm though...

 Not extensively tested but it at least passes the test cases and creates a useful
 log which should take care of the two main code paths (get_gitcvs and
 get_gitcvs_bool).

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 3e7bf5b..e51ffd0 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -174,27 +174,17 @@ sub req_Root
        return 0;
     }
 
-    my @gitvars = `git-config -l`;
-    if ($?) {
-       print "E problems executing git-config on the server -- this is not a git repository or the PATH is not set correctly.\n";
-        print "E \n";
-        print "error 1 - problem executing git-config\n";
-       return 0;
-    }
-    foreach my $line ( @gitvars )
-    {
-        next unless ( $line =~ /^(.*?)\.(.*?)(?:\.(.*?))?=(.*)$/ );
-        unless ($3) {
-            $cfg->{$1}{$2} = $4;
-        } else {
-            $cfg->{$1}{$2}{$3} = $4;
-        }
+    $cfg = GITCVS::config->new();
+
+    unless ($cfg) {
+	print "E problems executing git-config on the server -- ".
+	    "this is not a git repository or the PATH is not set correctly.\n";
+	print "E \n";
+	print "error 1 - problem executing git-config\n";
+	return 0;
     }
 
-    unless ( ($cfg->{gitcvs}{$state->{method}}{enabled}
-	      and $cfg->{gitcvs}{$state->{method}}{enabled} =~ /^\s*(1|true|yes)\s*$/i)
-	     or ($cfg->{gitcvs}{enabled}
-	      and $cfg->{gitcvs}{enabled} =~ /^\s*(1|true|yes)\s*$/i) )
+    unless ( $cfg->get_gitcvs_bool($state->{method},'enabled') )
     {
         print "E GITCVS emulation needs to be enabled on this repo\n";
         print "E the repo config file needs a [gitcvs] section added, and the parameter 'enabled' set to 1\n";
@@ -203,7 +193,7 @@ sub req_Root
         return 0;
     }
 
-    my $logfile = $cfg->{gitcvs}{$state->{method}}{logfile} || $cfg->{gitcvs}{logfile};
+    my $logfile = $cfg->get_gitcvs($state->{method},'logfile');
     if ( $logfile )
     {
         $log->setfile($logfile);
@@ -1967,7 +1957,7 @@ sub kopts_from_path
 	# what attributes apply to this path.
 
 	# Until then, take the setting from the config file
-    unless ( defined ( $cfg->{gitcvs}{allbinary} ) and $cfg->{gitcvs}{allbinary} =~ /^\s*(1|true|yes)\s*$/i )
+    unless ( $cfg->get_gitcvs_bool('allbinary') )
     {
 		# Return "" to give no special treatment to any path
 		return "";
@@ -1978,6 +1968,147 @@ sub kopts_from_path
     }
 }
 
+package GITCVS::config;
+
+####
+#### Copyright 2007 Frank Lichtenheld <frank@lichtenheld.de>.
+####
+
+use strict;
+use warnings;
+
+=head1 NAME
+
+GITCVS::config -- interface to the git configuration files
+
+=head1 DESCRIPTION
+
+Parses the output of "git-config -l" once and then allows to access that
+information
+
+=head1 METHODS
+
+=cut
+
+=head2 new
+
+Creates a new object and retrieves the config information.
+If retrieving the configuration fails, returns undef.
+
+=cut
+sub new
+{
+    my $class = shift;
+
+    my $self = {};
+
+    bless $self, $class;
+
+    $self->update() or return;
+
+    return $self;
+}
+
+=head2 update
+
+Update the config information. Is called by new on creation.
+Currently limits itself to the variables actually used by
+git-cvsserver since the output of git-config -l is not actually
+completly maschine-parsable. Multi-valued variables are not
+supported, the last value found is used.
+
+=cut
+sub update
+{
+    my $self = shift;
+
+    my @gitvars = `git-config -l`;
+    return if $?;
+    foreach my $line ( @gitvars )
+    {
+	next unless ( $line =~ /^((gitcvs)\.(?:(ext|pserver)\.)?([\w-]+))=(.*)$/ );
+	$self->{cfg}{$1} = $5;
+    }
+
+    return $self;
+}
+
+=head2 get
+
+Retrieve a configuration value. Give the key as array.
+
+=cut
+sub get {
+    my $self = shift;
+    my @key = @_;
+
+    unless (($#key == 1)
+	    || ($#key == 2)) {
+	return;
+    }
+
+    $key[0] = lc $key[0];
+    $key[-1] = lc $key[-1];
+
+    my $key = join('.',@key);
+    if (exists $self->{cfg}{$key}) {
+	return $self->{cfg}{$key};
+    }
+    return;
+}
+
+=head2 get_bool
+
+Retrieve a configuration value. Give the key as array.
+Normalizes the value to either undef, 0, or 1.
+
+=cut
+sub get_bool {
+    my $self = shift;
+    my $value = $self->get(@_);
+
+    return unless defined($value);
+    return 1 if $value =~ /^\s*(1|true|yes)\s*$/i;
+    return 0;
+}
+
+=head2 get_gitcvs
+
+Like get(), but automatically assumes gitcvs as section.
+If given two paramters, tries with second one alone
+if the first query gave no result.
+
+=cut
+sub get_gitcvs {
+    my $self = shift;
+    my @key = @_;
+
+    my $value = $self->get('gitcvs',@key);
+    if (!defined($value) && ($#key == 1)) {
+	$value = $self->get('gitcvs',$key[1]);
+    }
+
+    return $value;
+}
+
+=head2 get_gitcvs_bool
+
+What get_bool is to get that
+is get_gitcvs_bool to get_gitcvs.
+
+=cut
+sub get_gitcvs_bool {
+    my $self = shift;
+    my @key = @_;
+
+    my $value = $self->get_bool('gitcvs',@key);
+    if (!defined($value) && ($#key == 1)) {
+	$value = $self->get_bool('gitcvs',$key[1]);
+    }
+
+    return $value;
+}
+
 package GITCVS::log;
 
 ####
@@ -2189,14 +2320,12 @@ sub new
 
     die "Git repo '$self->{git_path}' doesn't exist" unless ( -d $self->{git_path} );
 
-    $self->{dbdriver} = $cfg->{gitcvs}{$state->{method}}{dbdriver} ||
-        $cfg->{gitcvs}{dbdriver} || "SQLite";
-    $self->{dbname} = $cfg->{gitcvs}{$state->{method}}{dbname} ||
-        $cfg->{gitcvs}{dbname} || "%Ggitcvs.%m.sqlite";
-    $self->{dbuser} = $cfg->{gitcvs}{$state->{method}}{dbuser} ||
-        $cfg->{gitcvs}{dbuser} || "";
-    $self->{dbpass} = $cfg->{gitcvs}{$state->{method}}{dbpass} ||
-        $cfg->{gitcvs}{dbpass} || "";
+    $self->{dbdriver} = $cfg->get_gitcvs($state->{method},'dbdriver') ||
+	"SQLite";
+    $self->{dbname} = $cfg->get_gitcvs($state->{method},'dbname') ||
+	"%Ggitcvs.%m.sqlite";
+    $self->{dbuser} = $cfg->get_gitcvs($state->{method},'dbuser') || "";
+    $self->{dbpass} = $cfg->get_gitcvs($state->{method},'dbpass') || "";
     my %mapping = ( m => $module,
                     a => $state->{method},
                     u => getlogin || getpwuid($<) || $<,
-- 
1.5.1.4

Re: [PATCH] cvsserver: Complete rewrite of the configuration parser

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

>  Maybe a bit overkill if one only wants to solve the problem Junio discovered
>  but I believe it's still worthwile.
>
>  Has a lot of overlap with perl/Git.pm though...
>
>  Not extensively tested but it at least passes the test cases and creates a useful
>  log which should take care of the two main code paths (get_gitcvs and
>  get_gitcvs_bool).

I agree that the general direction should be to do something
like this in perl/Git.pm (Pasky CC'ed).  As there are some
things that current Git.pm config interface does not offer an
easy access to what you would want to do, we need to enumerate
what you need, decide if they are of general interest and design
what to put in Git.pm and what to implement in GITCVS::config as
a special-purpose feature.

perl/Git.pm currently gives us only this: 

 - grab all values for a named variable, in an array;

 - return canonicalized value for a named boolean variable;

GITCVS::config wants to read *everything* from config and
returns a 'config' instance you can:

 - enumerate keys (you do not have this, but it is easy to add);

 - retrieve a value for a key (either one- or two-level);

 - retrieve a canonicalized bool value for a key (either one- or
   two-level);

 - treat a request for "gitcvs.method.option" variable to fall
   back on "gitcvs.option" if the former is not given;

 - the same for boolean variant.

I think the best abstraction is to have the "read everything"
interface in perl/Git.pm side, make the current Git::config()
and Git::config_bool() interface to use it (without issuing
extra 'git config --get-all').  I am not sure it is common for
Git.pm users to want the behaviour of "section.method.option"
falling back to "section.option", but if it is common enough, it
probably is a good idea to have:

	sub config_fallback {
        	my ($self, $section, $specific, $var) = @_;
                my $cfg = $self->config();

                if (exists $cfg{"$section.$specific.$var"}) {
                	return $cfg{"$section.$specific.$var"};
		}
                if (exists $cfg{"$section.$var"}) {
                	return $cfg{"$section.$var"};
		}
                return undef;
	}

on perl/Git.pm side.

But all of this is post 1.5.2 material; we would want to have a
minimal fixup on 'master' before 1.5.2, independent of this
rewrite.

Re: [PATCH] cvsserver: Complete rewrite of the configuration parser

[Frank Lichtenheld]

On Sat, May 12, 2007 at 12:59:49PM -0700, Junio C Hamano wrote:
> But all of this is post 1.5.2 material; we would want to have a
> minimal fixup on 'master' before 1.5.2, independent of this
> rewrite.

Fair enough. So far I see three very minimal solutions, but I can't
decide which one is the least ugly:

(For all we can begin by limiting the used variables to
^gitcvs.((ext|pserver).)? )

1) Drop variables named gitcvs.ext and gitcvs.pserver manually
2) Use the complete variable name as key to the hash instead of
   using a hash of hashes of hashes
   { "diff.color => "auto",
     "diff.color.whitespace" => "blue reverse" }
3) Make the second level always a hash, instead of using a string
   directly, so that Junio's example would look like this
   { diff => { color => { value => "auto",
   			  whitespace => "blue reverse" } } }


Opinions?

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

Re: [PATCH] cvsserver: Complete rewrite of the configuration parser

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> On Sat, May 12, 2007 at 12:59:49PM -0700, Junio C Hamano wrote:
>> But all of this is post 1.5.2 material; we would want to have a
>> minimal fixup on 'master' before 1.5.2, independent of this
>> rewrite.
>
> Fair enough. So far I see three very minimal solutions, but I can't
> decide which one is the least ugly:
>
> (For all we can begin by limiting the used variables to
> ^gitcvs.((ext|pserver).)? )

That sounds sensible.  And ignore anything that do not match.

> 1) Drop variables named gitcvs.ext and gitcvs.pserver manually

I do not see any need for this; gitcvs.ext or gitcvs.pserver as
variables do not exist, at least right now.  The breakage was
purely that the old parser tried to parse things it does not
even know about (e.g. diff.color) without knowing the rules
there.

> 2) Use the complete variable name as key to the hash instead of
>    using a hash of hashes of hashes
>    { "diff.color => "auto",
>      "diff.color.whitespace" => "blue reverse" }

No need for this nor the next one either.  You understand only
gitcvs.<option> or gitcvs.<method>.<option>, and you know there
is no string that is common in <option> and <method>

> 3) Make the second level always a hash, instead of using a string
>    directly, so that Junio's example would look like this
>    { diff => { color => { value => "auto",
>    			  whitespace => "blue reverse" } } }
>
>
> Opinions?

[PATCH] cvsserver: Limit config parser to needed options

[Frank Lichtenheld]

Change the configuration parser so that it ignores
everything except for ^gitcvs.((ext|pserver).)?
This greatly reduces the risk of failing while
parsing some unknown and irrelevant config option.

The bug that triggered this change was that the
parsing doesn't handle sections that have a
subsection and a variable with the same name.

While this bug still remains, all remaining
causes can be attributed to user error, since
there are no defined variables gitcvs.ext and
gitcvs.pserver.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 git-cvsserver.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 3e7bf5b..a07c725 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -183,9 +183,9 @@ sub req_Root
     }
     foreach my $line ( @gitvars )
     {
-        next unless ( $line =~ /^(.*?)\.(.*?)(?:\.(.*?))?=(.*)$/ );
-        unless ($3) {
-            $cfg->{$1}{$2} = $4;
+        next unless ( $line =~ /^(gitcvs)\.(?:(ext|pserver)\.)?([\w-]+)=(.*)$/ );
+        unless ($2) {
+            $cfg->{$1}{$3} = $4;
         } else {
             $cfg->{$1}{$2}{$3} = $4;
         }
-- 
1.5.1.4

Re: [PATCH] cvsserver: Limit config parser to needed options

[Junio C Hamano]

Thanks.