Re: [PATCH 2/3] cvsimport: fix the parsing of uppercase config options

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Nov 28, 2010, 19:30 UTC
Message-ID: <4CF2ADC9.8020504@drmicha.warpmail.net>
In-Reply-To: <7vwrnznueh.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 27.11.2010 07:38:

Show 20 quoted lines

> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> The current code leads to
>>
>> fatal: bad config value for 'cvsimport.r' in .git/config
>>
>> for a standard use case with cvsimport.r set:
>>
>> cvsimport sets internal variables by checking the config for each
>> possible command line option. The problem is that config items are case
>> insensitive, so config.r and config.R are the same. The ugly error is
>> due to that fact that cvsimport expects a bool for -R (and thus
>> config.R) but a remote name for -r (and thus config.r).
>>
>> Fix this by making cvsimport expect the config item "cvsimport.RR"
>> for the command line option "-R" etc.
> 
> I do not think this is "fixing" per-se.  Isn't it more like "We didn't
> have a way to use the configuration file to specify uppercase option; now
> we do thanks to this patch, and here is how"?

It is a fix because the the existing code base checks for "cvsimport.r" in order to set "opt_r" and for "cvsimport.R" in order to set "opt_R" (etc). That is, it sets "opt_R" from a config variable which should have nothing to do with it (cvsimport.r=cvsimport.R).

In the case when R expects a boolean you'll notice because "config get --bool" will die, but if both "x" and "X" expect the same type then you set defaults for "opt_x" and "opt_X" from the single "cvsimport.x".

> And the "here is how" workaround, while it may be a reasonable way out, is
> so obscure that it needs to be documented, no?  Ahh, that is what [3/3] is.

Yes, 3/3. Should I squash this with 2?

> The $ckey change from 1/3 needs to be done here, I think.

Yes, sorry, this was an "add -p" related oversight on my side. I'll resend.

Michael