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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 8, 2009, 10:18 UTC
Message-ID: <200904081218.39984.jnareb@gmail.com>
In-Reply-To: <49DC3ADD.5000902@catalyst.net.nz>

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:

Show 13 quoted lines

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

Show 19 quoted lines

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

Show 7 quoted lines

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