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

Frank Lichtenheld [off-list ref] 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.

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 [off-list ref]
www: http://www.djpig.de/