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

[Jakub Narebski <jnareb@gmail.com>]

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