Re: git-config does not check validity of variable names

Hi Jeff,

On Tue 11-01-11 00:59:22, Jeff King wrote:

On Sat, Jan 08, 2011 at 03:46:44PM +0100, Libor Pechacek wrote:

quoted

I've noticed that git-config accepts variable names in the form "a=b" for its
"get" operation.  That means "git config a=b" does not write anything to its
output and exists with status 1.

According to the man page only alphanumeric characters and - are allowed in
variable names.  Would it make sense to spit out an error message when the user
supplies an invalid variable name like the above?

Probably. The current behavior isn't all that terrible, in that it
simply tries to look up the key, which of course doesn't exist (because
it cannot syntactically), and does signal an error (with the exit code).
So it is in some ways no worse than a typo like "git config
color.dif.branch". And we probably don't want to start writing to stderr
in such a case, as scripts assume they can call git config to find out
whether the variable is defined without having to redirect stderr.

I fully agree that there should be no extra output if the user searches for an
undefined variable.  My idea is to warn the user when the variable name is
clearly invalid.

That being said, I can see how the lack of a message could be confusing
for a user who mistakenly thinks "git config color.diff.branch=red"
should work. So I think a patch to make that better would get a
favorable response.

Note, though, that what you wrote above is not strictly true. The
manpage says variable names and section names must be alphanumeric. But
subsection names can contain any character except newline. So it is
valid syntactically to do:

  git config color.diff=red.branch

where the subsection contains the "=". Obviously this example is
nonsense, and in practice most such "a=b" forms will end up not being
syntactically valid (because the = will be part of the variable name,
not the subsection).

That was new for me, so I had to learn the concept.  Thanks for thorough
explanation.

But if you are going to write a patch, you need to
make sure not to accidentally disallow:

  git config 'diff.my custom diff driver.command'

There is already a check in git_config_set_multivar to prevent the user from
creating variables with invalid names.  That can probably be separated and
reused in the "get value" path.  I'll have a look.


In the course of reading the code I've spotted another trouble related to
regexps.  Variable and section names are case insensitive, while subsection
names are case sensitive.  How can regexp matching be made "partially case
sensitive"?  That's a challenge.

The current approach is to lowercase everything in the pattern from the
beginning until the first dot and from the last dot till the end.  That has its
limitations as regular expressions can be fairly complex.

Just to illustrate the situation:
$ git config --get-regexp '(CORE|USER)\.'
user.email lpechacek@suse.cz
core.repositoryformatversion 0
core.filemode true

$ git config --get-regexp '(COR.|USER)\.'
core.repositoryformatversion 0
core.filemode true

Currently have no idea how to fix that apart from fixing the documentation. :) 

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague