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

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

Michael J Gruber [off-list ref] writes:

quoted

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