git-config does not check validity of variable names

Hello,

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?

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague

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

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.

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

-Peff

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

Sanity-ckeck config variable names when adding and retrieving them.

As a side effect code duplication between git_config_set_multivar and get_value
(in builtin/config.c) was removed and the common functionality was placed in
git_config_parse_key.

The regular expression patterns are left intact when using --get-regexp option.

Signed-off-by: Libor Pechacek <redacted>
Cc: Jeff King <redacted>
---
 builtin/config.c |    8 ++---
 cache.h          |    1 +
 config.c         |  106 ++++++++++++++++++++++++++++++++++--------------------
 3 files changed, 71 insertions(+), 44 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index ca4a0db..068ef76 100644
--- a/builtin/config.c
+++ b/builtin/config.c

@@ -153,7 +153,6 @@ static int show_config(const char *key_, const char *value_, void *cb)
 static int get_value(const char *key_, const char *regex_)
 {
 	int ret = -1;
-	char *tl;
 	char *global = NULL, *repo_config = NULL;
 	const char *system_wide = NULL, *local;
 

@@ -168,10 +167,6 @@ static int get_value(const char *key_, const char *regex_)
 	}
 
 	key = xstrdup(key_);
-	for (tl=key+strlen(key)-1; tl >= key && *tl != '.'; --tl)
-		*tl = tolower(*tl);
-	for (tl=key; *tl && *tl != '.'; ++tl)
-		*tl = tolower(*tl);
 
 	if (use_key_regexp) {
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));

@@ -179,6 +174,9 @@ static int get_value(const char *key_, const char *regex_)
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
 			goto free_strings;
 		}
+	} else {
+		if (git_config_parse_key(key_, &key, NULL))
+			goto free_strings;
 	}
 
 	if (regex_) {

diff --git a/cache.h b/cache.h
index d83d68c..1e32d63 100644
--- a/cache.h
+++ b/cache.h

@@ -997,6 +997,7 @@ extern int git_config_maybe_bool(const char *, const char *);
 extern int git_config_string(const char **, const char *, const char *);
 extern int git_config_pathname(const char **, const char *, const char *);
 extern int git_config_set(const char *, const char *);
+extern int git_config_parse_key(const char *, char **, int *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);

diff --git a/config.c b/config.c
index 625e051..205282f 100644
--- a/config.c
+++ b/config.c

@@ -1098,6 +1098,72 @@ int git_config_set(const char *key, const char *value)
 	return git_config_set_multivar(key, value, NULL, 0);
 }
 
+/* Auxiliary function to sanity-check and split the key into the section
+ * identifier and variable name.
+ *
+ * Returns 0 on success, 1 when there is an invalid character in the key and 2
+ * if there is no section name in the key.
+ *
+ * store_key - pointer to char* which will hold a copy of the key with
+ *             lowercase section and variable name, can be NULL
+ * baselen - pointer to int which will hold the length of the
+ *           section + subsection part, can be NULL
+ */
+int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+{
+	int i, dot, baselen;
+	const char *last_dot = strrchr(key, '.');
+
+	/*
+	 * Since "key" actually contains the section name and the real
+	 * key name separated by a dot, we have to know where the dot is.
+	 */
+
+	if (last_dot == NULL) {
+		error("key does not contain a section: %s", key);
+		return 2;
+	}
+
+	baselen = last_dot - key;
+	if (baselen_)
+		*baselen_ = baselen;
+
+	/*
+	 * Validate the key and while at it, lower case it for matching.
+	 */
+	if (store_key)
+		*store_key = xmalloc(strlen(key) + 1);
+
+	dot = 0;
+	for (i = 0; key[i]; i++) {
+		unsigned char c = key[i];
+		if (c == '.')
+			dot = 1;
+		/* Leave the extended basename untouched.. */
+		if (!dot || i > baselen) {
+			if (!iskeychar(c) || (i == baselen+1 && !isalpha(c))) {
+				error("invalid key: %s", key);
+				goto out_free_ret_1;
+			}
+			c = tolower(c);
+		} else if (c == '\n') {
+			error("invalid key (newline): %s", key);
+			goto out_free_ret_1;
+		}
+		if (store_key)
+			(*store_key)[i] = c;
+	}
+	if (store_key)
+		(*store_key)[i] = 0;
+
+	return 0;
+
+out_free_ret_1:
+	if (store_key)
+		free(*store_key);
+	return 1;
+}
+
 /*
  * If value==NULL, unset in (remove from) config,
  * if value_regex!=NULL, disregard key/value pairs where value does not match.

@@ -1124,59 +1190,21 @@ int git_config_set(const char *key, const char *value)
 int git_config_set_multivar(const char *key, const char *value,
 	const char *value_regex, int multi_replace)
 {
-	int i, dot;
 	int fd = -1, in_fd;
 	int ret;
 	char *config_filename;
 	struct lock_file *lock = NULL;
-	const char *last_dot = strrchr(key, '.');
 
 	if (config_exclusive_filename)
 		config_filename = xstrdup(config_exclusive_filename);
 	else
 		config_filename = git_pathdup("config");
 
-	/*
-	 * Since "key" actually contains the section name and the real
-	 * key name separated by a dot, we have to know where the dot is.
-	 */
-
-	if (last_dot == NULL) {
-		error("key does not contain a section: %s", key);
-		ret = 2;
+	if ((ret = git_config_parse_key(key, &store.key, &store.baselen)))
 		goto out_free;
-	}
-	store.baselen = last_dot - key;
 
 	store.multi_replace = multi_replace;
 
-	/*
-	 * Validate the key and while at it, lower case it for matching.
-	 */
-	store.key = xmalloc(strlen(key) + 1);
-	dot = 0;
-	for (i = 0; key[i]; i++) {
-		unsigned char c = key[i];
-		if (c == '.')
-			dot = 1;
-		/* Leave the extended basename untouched.. */
-		if (!dot || i > store.baselen) {
-			if (!iskeychar(c) || (i == store.baselen+1 && !isalpha(c))) {
-				error("invalid key: %s", key);
-				free(store.key);
-				ret = 1;
-				goto out_free;
-			}
-			c = tolower(c);
-		} else if (c == '\n') {
-			error("invalid key (newline): %s", key);
-			free(store.key);
-			ret = 1;
-			goto out_free;
-		}
-		store.key[i] = c;
-	}
-	store.key[i] = 0;
 
 	/*
 	 * The lock serves a purpose in addition to locking: the new

-- 
1.7.4.rc2.2.gb4f4f

Variable names must start with an alphabetic character, regexp config key
matching is case sensitive.

Signed-off-by: Libor Pechacek <redacted>
Cc: Jeff King <redacted>
---
 Documentation/config.txt     |   12 +++++++-----
 Documentation/git-config.txt |    4 +++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ff7c225..0f23bc7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -12,8 +12,9 @@ The configuration variables are used by both the git plumbing
 and the porcelains. The variables are divided into sections, wherein
 the fully qualified variable name of the variable itself is the last
 dot-separated segment and the section name is everything before the last
-dot. The variable names are case-insensitive and only alphanumeric
-characters are allowed. Some variables may appear multiple times.
+dot. The variable names are case-insensitive, only alphanumeric
+characters and '-' are allowed and must start with an alphabetic character.
+Some variables may appear multiple times.
 
 Syntax
 ~~~~~~

@@ -53,9 +54,10 @@ All the other lines (and the remainder of the line after the section
 header) are recognized as setting variables, in the form
 'name = value'.  If there is no equal sign on the line, the entire line
 is taken as 'name' and the variable is recognized as boolean "true".
-The variable names are case-insensitive and only alphanumeric
-characters and `-` are allowed.  There can be more than one value
-for a given variable; we say then that variable is multivalued.
+The variable names are case-insensitive, only alphanumeric
+characters and `-` are allowed and must start with an alphabetic character.
+There can be more than one value for a given variable; we say then that
+variable is multivalued.
 
 Leading and trailing whitespace in a variable value is discarded.
 Internal whitespace within a variable value is retained verbatim.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 543dd64..6966ed6 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -84,7 +84,9 @@ OPTIONS
 
 --get-regexp::
 	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Regular expression matching is case sensitive in all parts of the key,
+	therefore make sure your pattern matches lower case letters in section
+	and variable names.  Also outputs the key names.
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than

-- 
1.7.4.rc2.2.gb4f4f

On Wed, Jan 19, 2011 at 03:11:12PM +0100, Libor Pechacek wrote:

Sanity-ckeck config variable names when adding and retrieving them.

As a side effect code duplication between git_config_set_multivar and get_value
(in builtin/config.c) was removed and the common functionality was placed in
git_config_parse_key.

I think this is a good goal, but a few nits:

+/* Auxiliary function to sanity-check and split the key into the section
+ * identifier and variable name.
+ *
+ * Returns 0 on success, 1 when there is an invalid character in the key and 2
+ * if there is no section name in the key.

Please switch these to -1 and -2, as we generally use negative integers
to indicate errors in library-ish function. I know you were just copying
git_config_set_multivar's error codes, but it is designed to return
straight to exit(), which makes it an exception.

Other than that, the code looks OK to me. However, it does cause
t1300.85 to fail. The problem is that the test is using these bogus
names to check that "git -c" works. While it does technically work now
to say "git -c foo=bar config foo" (which your patch breaks), I don't
think that is a useful behavior in the real world, since no actual
config options exist without a section name. So yes, you can "git -c" a
non-sectioned variable, but why would you want to?

So I think it probably makes sense to squash this in:

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d0e5546..3e79c37 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh

@@ -876,11 +876,10 @@ test_expect_success 'check split_cmdline return' "
 	"
 
 test_expect_success 'git -c "key=value" support' '
-	test "z$(git -c name=value config name)" = zvalue &&
 	test "z$(git -c core.name=value config core.name)" = zvalue &&
-	test "z$(git -c CamelCase=value config camelcase)" = zvalue &&
-	test "z$(git -c flag config --bool flag)" = ztrue &&
-	test_must_fail git -c core.name=value config name
+	test "z$(git -c foo.CamelCase=value config foo.camelcase)" = zvalue &&
+	test "z$(git -c foo.flag config --bool foo.flag)" = ztrue &&
+	test_must_fail git -c name=value config core.name
 '
 
 test_done

and a note to the commit message like:

  This breaks a test in t1300 which used invalid section-less keys in
  the tests for "git -c". However, allowing such names there was
  useless, since there was no way to set them via config file, and no
  part of git actually tried to use section-less keys. This patch
  updates the test to use more realistic examples.

-Peff

On Thu, Jan 20, 2011 at 06:22:32PM -0500, Jeff King wrote:

Other than that, the code looks OK to me.

Actually, I take this back.

Doesn't this hunk:

Mean that regexp keys no longer get downcased properly? I.e.,

  git config Foo.value true
  git config --get-regexp 'foo.*'
  git config --get-regexp 'Foo.*'

used to work for both lookups, but now fails for the second one?

The problem is that your git_config_parse_key handles the downcasing for
the non-regexp case, but it is not called (for obvious reasons) in the
regexp case.

-Peff

On Wed, Jan 19, 2011 at 03:14:01PM +0100, Libor Pechacek wrote:

The intent of the change looks fine, but your sentence doesn't quite
parse to me (to be fair, the problem is in the one you are replacing,
but adding the third clause makes it even more confusing). How about:

  The variables names are case-insensitive, allow only alphanumeric
  characters and '-', and must start with an alphabetic character.

 --get-regexp::
 	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Regular expression matching is case sensitive in all parts of the key,
+	therefore make sure your pattern matches lower case letters in section
+	and variable names.  Also outputs the key names.

That is only true because of the breakage in your first patch. Without
your patch, both of these work:

  git config --get-regexp 'Foo.*'
  git config --get-regexp 'foo.*'

That being said, the downcasing is extremely naive for regexps, and you
should try to match the canonical name. The current downcasing behavior
should probably stay for historical reasons, but is not well thought-out
(it may even be accidental). Perhaps we should therefore explain it in
those terms:

  Regular expression matching is case-sensitive and done against a
  canonicalized version of the key in which section and variable names
  are lowercased, but subsection names are not. For historical reasons,
  some simple regular expressions are lower-cased before matching
  (everything before the first dot and after the last dot), which makes
  things like "Core.*' work.

I dunno. Maybe we should just declare "Core.*' to be broken, and anybody
who was relying on it is wrong.

-Peff

Variable names must start with an alphabetic character, regexp config key
matching has its limits.

Signed-off-by: Libor Pechacek <redacted>
Cc: Jeff King <redacted>
---
 Documentation/config.txt     |   12 +++++++-----
 Documentation/git-config.txt |    9 +++++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ff7c225..928ceda 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -12,8 +12,9 @@ The configuration variables are used by both the git plumbing
 and the porcelains. The variables are divided into sections, wherein
 the fully qualified variable name of the variable itself is the last
 dot-separated segment and the section name is everything before the last
-dot. The variable names are case-insensitive and only alphanumeric
-characters are allowed. Some variables may appear multiple times.
+dot. The variable names are case-insensitive, allow only alphanumeric
+characters and '-', and must start with an alphabetic character.  Some
+variables may appear multiple times.
 
 Syntax
 ~~~~~~

@@ -53,9 +54,10 @@ All the other lines (and the remainder of the line after the section
 header) are recognized as setting variables, in the form
 'name = value'.  If there is no equal sign on the line, the entire line
 is taken as 'name' and the variable is recognized as boolean "true".
-The variable names are case-insensitive and only alphanumeric
-characters and `-` are allowed.  There can be more than one value
-for a given variable; we say then that variable is multivalued.
+The variable names are case-insensitive, allow only alphanumeric characters
+and `-`, and must start with an alphabetic character.  There can be more
+than one value for a given variable; we say then that variable is
+multivalued.
 
 Leading and trailing whitespace in a variable value is discarded.
 Internal whitespace within a variable value is retained verbatim.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 543dd64..31f4658 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -83,8 +83,13 @@ OPTIONS
 	is not exactly one.
 
 --get-regexp::
-	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Like --get-all, but interprets the name as a regular expression and
+	writes out the key names.  Regular expression matching is currently
+	case-sensitive and done against a canonicalized version of the key
+	in which section and variable names are lowercased, but subsection
+	names are not.  Regular expressions are partially lower-cased
+	before matching (everything before the first dot and after the last
+	dot), which makes things like "Core.*' work.
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than

-- 
1.7.4.rc2.20.gdb1b81

On Fri, Jan 21, 2011 at 11:25:37AM +0100, Libor Pechacek wrote:

Variable names must start with an alphabetic character, regexp config key
matching has its limits.

I think this is fine, although:

 --get-regexp::
-	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Like --get-all, but interprets the name as a regular expression and
+	writes out the key names.  Regular expression matching is currently
+	case-sensitive and done against a canonicalized version of the key
+	in which section and variable names are lowercased, but subsection
+	names are not.  Regular expressions are partially lower-cased
+	before matching (everything before the first dot and after the last
+	dot), which makes things like "Core.*' work.

I am half-tempted to mark the lowercasing of the regex as deprecated (or
at least discouraged). It's such a hack, and I don't think we will ever
improve to make it work in the general case, as regexes are simply too
complex for us to handle all possible inputs.

Either way, though, this is definitely an improvement over what is
there, so:

Acked-by: Jeff King <redacted>

-Peff

On Fri 21-01-11 11:25:37, Jeff King wrote:

I am half-tempted to mark the lowercasing of the regex as deprecated (or
at least discouraged).

That's actually side effect of
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=2fa9a0fb31cbf01e8318a02c3e222d7fd3fd0a83
Don't see any intent to support "mixed-sensitivity" matching in it.

It's such a hack, and I don't think we will ever improve to make it work in
the general case, as regexes are simply too complex for us to handle all
possible inputs.

As far as I understand git uses host library implementation of regcomp and
regexec so we cannot fix that side.  Writing code to modify regexes is not
worth the effort.

FWIW I'm in favor of deprecating this functionality.

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague

On Thu 20-01-11 19:27:16, Jeff King wrote:

The intent of the change looks fine, but your sentence doesn't quite
parse to me (to be fair, the problem is in the one you are replacing,
but adding the third clause makes it even more confusing). How about:

  The variables names are case-insensitive, allow only alphanumeric
  characters and '-', and must start with an alphabetic character.

I like that very much.  The original sentence sounded a little bit artificial
to me, and after my amendments it felt like a collapsed list of items.  This is
far more natural, thanks for help.

quoted

 --get-regexp::
 	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Regular expression matching is case sensitive in all parts of the key,
+	therefore make sure your pattern matches lower case letters in section
+	and variable names.  Also outputs the key names.

That is only true because of the breakage in your first patch. Without
your patch, both of these work:

  git config --get-regexp 'Foo.*'
  git config --get-regexp 'foo.*'

That being said, the downcasing is extremely naive for regexps, and you
should try to match the canonical name. The current downcasing behavior
should probably stay for historical reasons, but is not well thought-out
(it may even be accidental). Perhaps we should therefore explain it in
those terms:

  Regular expression matching is case-sensitive and done against a
  canonicalized version of the key in which section and variable names
  are lowercased, but subsection names are not. For historical reasons,
  some simple regular expressions are lower-cased before matching
  (everything before the first dot and after the last dot), which makes
  things like "Core.*' work.

I dunno. Maybe we should just declare "Core.*' to be broken, and anybody
who was relying on it is wrong.

After thinking about it more, I realized that the goal is to have a "mixed
case-sensitivity" regexp matching.  The case sensitivity if of course
determined by the part of the key which is being processed, and not the pattern
itself.  That probably means changes in the regexp mathing sengine.

Declaring patterns like 'Core.*' invalid would be a step back in my opinion.

Therefore I've added the regexp lowercasing code back and documented the
limitation.  I believe it's fair to the users.

Updated patches will follow in a while.

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague

Variable names must start with an alphabetic character, regexp config key
matching has its limits.

Signed-off-by: Libor Pechacek <redacted>
Acked-by: Jeff King <redacted>
---

Hello Junio,

This patch has fallen through the cracks, therefore I re-send it.  Previous
discussion about this patch is at http://www.spinics.net/lists/git/msg149593.html.
The only change I've done since version 2 of this patch is replacing
apostrophes with backticks in the first hunk.

Be so kind as to apply the fix.

Libor

 Documentation/config.txt     |   12 +++++++-----
 Documentation/git-config.txt |    9 +++++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e55dae1..078313e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -12,8 +12,9 @@ The configuration variables are used by both the git plumbing
 and the porcelains. The variables are divided into sections, wherein
 the fully qualified variable name of the variable itself is the last
 dot-separated segment and the section name is everything before the last
-dot. The variable names are case-insensitive and only alphanumeric
-characters are allowed. Some variables may appear multiple times.
+dot. The variable names are case-insensitive, allow only alphanumeric
+characters and `-`, and must start with an alphabetic character.  Some
+variables may appear multiple times.
 
 Syntax
 ~~~~~~

@@ -54,9 +55,10 @@ All the other lines (and the remainder of the line after the section
 header) are recognized as setting variables, in the form
 'name = value'.  If there is no equal sign on the line, the entire line
 is taken as 'name' and the variable is recognized as boolean "true".
-The variable names are case-insensitive and only alphanumeric
-characters and `-` are allowed.  There can be more than one value
-for a given variable; we say then that variable is multivalued.
+The variable names are case-insensitive, allow only alphanumeric characters
+and `-`, and must start with an alphabetic character.  There can be more
+than one value for a given variable; we say then that variable is
+multivalued.
 
 Leading and trailing whitespace in a variable value is discarded.
 Internal whitespace within a variable value is retained verbatim.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index aa8303b..a54fee8 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -85,8 +85,13 @@ OPTIONS
 	is not exactly one.
 
 --get-regexp::
-	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Like --get-all, but interprets the name as a regular expression and
+	writes out the key names.  Regular expression matching is currently
+	case-sensitive and done against a canonicalized version of the key
+	in which section and variable names are lowercased, but subsection
+	names are not.  Regular expressions are partially lower-cased
+	before matching (everything before the first dot and after the last
+	dot), which makes things like "Core.*' work.
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than

-- 
1.7.9.2.324.g78dedf

On Thu, Mar 01, 2012 at 09:19:42AM +0100, Libor Pechacek wrote:

Variable names must start with an alphabetic character, regexp config key
matching has its limits.
[...]
This patch has fallen through the cracks, therefore I re-send it.  Previous
discussion about this patch is at http://www.spinics.net/lists/git/msg149593.html.
The only change I've done since version 2 of this patch is replacing
apostrophes with backticks in the first hunk.

Wow, it's been a while. :)

Generally it looks OK to me, but I have two comments:

Not an error you introduced, but should it be "...we say then that _the_
variable is multivalued".

I know I ack'ed this last time around, but reading it fresh, I think we
are probably better off to just not mention the down-casing at all. It's
just confusing, and people shouldn't depend on it. They should know that
they are comparing against the canonical name, and should use lowercase
in their regex. I.e., just cut out the last sentence from there.

-Peff

On Thu 01-03-12 04:08:28, Jeff King wrote:

On Thu, Mar 01, 2012 at 09:19:42AM +0100, Libor Pechacek wrote:

[...]

Generally it looks OK to me, but I have two comments:

quoted

 Syntax
 ~~~~~~

@@ -54,9 +55,10 @@ All the other lines (and the remainder of the line after the section
 header) are recognized as setting variables, in the form
 'name = value'.  If there is no equal sign on the line, the entire line
 is taken as 'name' and the variable is recognized as boolean "true".
-The variable names are case-insensitive and only alphanumeric
-characters and `-` are allowed.  There can be more than one value
-for a given variable; we say then that variable is multivalued.
+The variable names are case-insensitive, allow only alphanumeric characters
+and `-`, and must start with an alphabetic character.  There can be more
+than one value for a given variable; we say then that variable is
+multivalued.

Not an error you introduced, but should it be "...we say then that _the_
variable is multivalued".

Yes, even my language sense tells me the article is missing.  Fixed.

quoted

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index aa8303b..a54fee8 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -85,8 +85,13 @@ OPTIONS
 	is not exactly one.
 
 --get-regexp::
-	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Like --get-all, but interprets the name as a regular expression and
+	writes out the key names.  Regular expression matching is currently
+	case-sensitive and done against a canonicalized version of the key
+	in which section and variable names are lowercased, but subsection
+	names are not.  Regular expressions are partially lower-cased
+	before matching (everything before the first dot and after the last
+	dot), which makes things like "Core.*' work.

I know I ack'ed this last time around, but reading it fresh, I think we
are probably better off to just not mention the down-casing at all. It's
just confusing, and people shouldn't depend on it. They should know that
they are comparing against the canonical name, and should use lowercase
in their regex. I.e., just cut out the last sentence from there.

I agree, it's just a technical detail, which can be left out in sake of
readability.

Thanks for review, the final patch will follow.

Libor

Variable names must start with an alphabetic character, regexp config key
matching has its limits, sentence grammar.

Signed-off-by: Libor Pechacek <redacted>
Acked-by: Jeff King <redacted>
---

Junio,

Jeff had two more comments, which I've incorporated into the fix.  I think
we've reached the acme of patch polishing, and I'd call this patch final.
Please apply.


 Documentation/config.txt     |   12 +++++++-----
 Documentation/git-config.txt |    7 +++++--
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e55dae1..5367ba9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -12,8 +12,9 @@ The configuration variables are used by both the git plumbing
 and the porcelains. The variables are divided into sections, wherein
 the fully qualified variable name of the variable itself is the last
 dot-separated segment and the section name is everything before the last
-dot. The variable names are case-insensitive and only alphanumeric
-characters are allowed. Some variables may appear multiple times.
+dot. The variable names are case-insensitive, allow only alphanumeric
+characters and `-`, and must start with an alphabetic character.  Some
+variables may appear multiple times.
 
 Syntax
 ~~~~~~

@@ -54,9 +55,10 @@ All the other lines (and the remainder of the line after the section
 header) are recognized as setting variables, in the form
 'name = value'.  If there is no equal sign on the line, the entire line
 is taken as 'name' and the variable is recognized as boolean "true".
-The variable names are case-insensitive and only alphanumeric
-characters and `-` are allowed.  There can be more than one value
-for a given variable; we say then that variable is multivalued.
+The variable names are case-insensitive, allow only alphanumeric characters
+and `-`, and must start with an alphabetic character.  There can be more
+than one value for a given variable; we say then that the variable is
+multivalued.
 
 Leading and trailing whitespace in a variable value is discarded.
 Internal whitespace within a variable value is retained verbatim.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index aa8303b..81b0398 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -85,8 +85,11 @@ OPTIONS
 	is not exactly one.
 
 --get-regexp::
-	Like --get-all, but interprets the name as a regular expression.
-	Also outputs the key names.
+	Like --get-all, but interprets the name as a regular expression and
+	writes out the key names.  Regular expression matching is currently
+	case-sensitive and done against a canonicalized version of the key
+	in which section and variable names are lowercased, but subsection
+	names are not.
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than

-- 
1.7.9.2.324.g78dedf