git-config does not check validity of variable names

git-config does not check validity of variable names

[Libor Pechacek]

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

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

[Jeff King]

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

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

[Libor Pechacek]

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

[PATCH] Sanity-ckeck config variable names

[Libor Pechacek]

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 <lpechacek@suse.cz>
Cc: Jeff King <peff@peff.net>
---
 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

Re: [PATCH] Sanity-ckeck config variable names

[Jeff King]

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

Re: [PATCH] Sanity-ckeck config variable names

[Jeff King]

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:

> @@ -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);

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

[PATCH] Documentation fixes in git-config

[Libor Pechacek]

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

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Cc: Jeff King <peff@peff.net>
---
 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

Re: [PATCH] Documentation fixes in git-config

[Jeff King]

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

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

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

Re: [PATCH] Documentation fixes in git-config

[Libor Pechacek]

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.

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

[PATCH v2] Documentation fixes in git-config

[Libor Pechacek]

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

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Cc: Jeff King <peff@peff.net>
---
 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

Re: [PATCH v2] Documentation fixes in git-config

[Jeff King]

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 <peff@peff.net>

-Peff

Re: [PATCH v2] Documentation fixes in git-config

[Libor Pechacek]

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

[PATCH v3] Documentation fixes in git-config

[Libor Pechacek]

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

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Acked-by: Jeff King <peff@peff.net>
---

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

Re: [PATCH v3] Documentation fixes in git-config

[Jeff King]

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:

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

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

-Peff

Re: [PATCH v3] Documentation fixes in git-config

[Libor Pechacek]

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

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

Re: [PATCH v3] Documentation fixes in git-config

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

It indeed is.

Thanks. Picked up the v4 patch.

[PATCH v4] Documentation fixes in git-config

[Libor Pechacek]

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

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Acked-by: Jeff King <peff@peff.net>
---

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

Re: [PATCH] Sanity-ckeck config variable names

[Libor Pechacek]

On Thu 20-01-11 18:22:32, Jeff King wrote:
> > + * 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.

Done.

> Other than that, the code looks OK to me. However, it does cause
> t1300.85 to fail.

Thanks for catching it, added your changes to the patch and tested.  I didn't
notice the test suite. :)

On Thu 20-01-11 19:06:29, Jeff King wrote:
> Doesn't this hunk:
> 
> > @@ -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);
> 
> 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?

After thinking about it I added the code back.  More on it in the
"documentation fix" thread.

Thanks for your kind guidance.

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague

[PATCH v2] Sanity-ckeck config variable names

[Libor Pechacek]

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.

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.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Cc: Jeff King <peff@peff.net>
---
 builtin/config.c       |   17 +++++--
 cache.h                |    1 +
 config.c               |  107 ++++++++++++++++++++++++++++++-----------------
 t/t1300-repo-config.sh |    7 +--
 4 files changed, 84 insertions(+), 48 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index ca4a0db..ba614e7 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,17 +167,25 @@ 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) {
+		char *tl;
+
+		/* TODO - this naive pattern lowercasing obviously does not
+		 * work for more complex patterns like `^[^.]*Foo.*bar' */
+		for (tl = key+strlen(key)-1; tl >= key && *tl != '.'; --tl)
+			*tl = tolower(*tl);
+		for (tl = key; *tl && *tl != '.'; ++tl)
+			*tl = tolower(*tl);
+
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			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..04a0c57 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,22 @@ 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;
+	ret = git_config_parse_key(key, &store.key, &store.baselen);
+	if (ret)
 		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
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
-- 
1.7.4.rc2.20.gdb1b81

Re: [PATCH v2] Sanity-ckeck config variable names

[Jeff King]

On Fri, Jan 21, 2011 at 11:23:39AM +0100, Libor Pechacek wrote:

> Sanity-ckeck config variable names when adding and retrieving them.  As a side

Typo: s/ckeck/check/

Other than that, this version looks good to me.

Acked-by: Jeff King <peff@peff.net>

-Peff

[PATCH v3] Sanity-check config variable names

[Libor Pechacek]

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

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.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Cc: Jeff King <peff@peff.net>
---

On Fri 21-01-11 11:23:19, Jeff King wrote:
> Typo: s/ckeck/check/
> 
> Other than that, this version looks good to me.

Fixed the typo and return values from get_value and git_config_set_multivar.
We have changed git_config_parse_key to return negative values on error, but
forgot to negate the numbers when returning them as exit codes.  That led to
process exit codes 255 and 254, which is, I think, unwanted.

> Acked-by: Jeff King <peff@peff.net>

Thanks.  Now I consider this patch truly final.

Libor

 builtin/config.c       |   18 ++++++--
 cache.h                |    1 +
 config.c               |  107 ++++++++++++++++++++++++++++++-----------------
 t/t1300-repo-config.sh |    7 +--
 4 files changed, 85 insertions(+), 48 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index ca4a0db..66c0586 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,17 +167,26 @@ 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) {
+		char *tl;
+
+		/* TODO - this naive pattern lowercasing obviously does not
+		 * work for more complex patterns like `^[^.]*Foo.*bar' */
+		for (tl = key+strlen(key)-1; tl >= key && *tl != '.'; --tl)
+			*tl = tolower(*tl);
+		for (tl = key; *tl && *tl != '.'; ++tl)
+			*tl = tolower(*tl);
+
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
 			goto free_strings;
 		}
+	} else {
+		ret = -git_config_parse_key(key_, &key, NULL);
+		if (ret)
+			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..c976544 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,22 @@ 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;
+	ret = -git_config_parse_key(key, &store.key, &store.baselen);
+	if (ret)
 		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
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
-- 
1.7.4.rc3.2.g27487

Re: [PATCH v3] Sanity-check config variable names

[Junio C Hamano]

Libor Pechacek <lpechacek@suse.cz> writes:

> Sanity-check 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.
>
> 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.
>
> Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
> Cc: Jeff King <peff@peff.net>
> ---
>
> On Fri 21-01-11 11:23:19, Jeff King wrote:
>> Typo: s/ckeck/check/
>> 
>> Other than that, this version looks good to me.
>
> Fixed the typo and return values from get_value and git_config_set_multivar.
> We have changed git_config_parse_key to return negative values on error, but
> forgot to negate the numbers when returning them as exit codes.

Earlier get_value() returned:

 -1: when it saw an uncompilable regexp (either in key or value);
  0: when a value was available (under --all) or unique (without --all);
  1: when the requested variable is missing; and
  2: when the requested variable is multivalued under --all.

As the new error condition you are adding is to detect and signal a broken
input, doesn't it fall into the same category as "broken regexp", which in
turn would mean that it should return the same -1?

Or to distinguish between "invalid character in the key" and "no section
name in the key", you might want to add -2 to the mix, but personally I
don't think it is worth it.  The advantage of being able to tell between
the two kinds of breakage feels much smaller than the cost of having to
worry about breaking existing callers that try to catch broken user input
by checking the return value from get_value() explicitly against -1, not
just any negative value.

Note that I am not suggesting to change the return value from
config-parse-key in the above; I am only talking about get_value().

>  	if (use_key_regexp) {
> +		char *tl;
> +
> +		/* TODO - this naive pattern lowercasing obviously does not
> +		 * work for more complex patterns like `^[^.]*Foo.*bar' */
> +		for (tl = key+strlen(key)-1; tl >= key && *tl != '.'; --tl)
> +			*tl = tolower(*tl);
> +		for (tl = key; *tl && *tl != '.'; ++tl)
> +			*tl = tolower(*tl);

When moving an existing code segment around like this, I would not mind to
see style fixes thrown in to the patch, as long as the moved code is small
enough.  Perhaps like this:

	/*
         * NEEDSWORK: this naive pattern lowercasing obviously does
	 * not work for more complex patterns like "^[^.]*Foo.*bar".
	 * Perhaps we should deprecate this altogether someday.
         */
	for (tl = key + strlen(key) - 1;
	     tl >= key && *tl != '.';
	     tl--)
		*tl = tolower(*tl);
	for (tl = key; *tl && *tl != '.'; tl++)
		*tl = tolower(*tl);

The style rules applied above are...

  - We want to see SP around binary operators;

  - However, we don't want to see a line that is too long;

  - When there is no reason to choose between (pre/post)-(in/de)crement,
    post-(in/de)crement is easier to read.

> diff --git a/config.c b/config.c
> index 625e051..c976544 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

	/*
         * Style. We write our multi-line comments
	 * like this.
         */

> +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);

Does it make sense for this function to be prepared to get called with
NULL in store_key like this (and in the remainder of your patch)?

The primary reason somebody calls this function is so that the caller can
then use the resulting lowercased form for matching, but "if store-key is
not NULL" feels as if this code is optimized for a special case caller
that only wants to validate without using the resulting lowercased key for
matching, which does not exist yet.

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

Don't you want to make sure that your sanity check triggers in new tests?

Re: [PATCH v3] Sanity-check config variable names

[Libor Pechacek]

On Thu 27-01-11 14:45:16, Junio C Hamano wrote:
> Libor Pechacek <lpechacek@suse.cz> writes:
> > Fixed the typo and return values from get_value and git_config_set_multivar.
> > We have changed git_config_parse_key to return negative values on error, but
> > forgot to negate the numbers when returning them as exit codes.
> 
> Earlier get_value() returned:
> 
>  -1: when it saw an uncompilable regexp (either in key or value);
>   0: when a value was available (under --all) or unique (without --all);
>   1: when the requested variable is missing; and
>   2: when the requested variable is multivalued under --all.

Fixed one part with the last change and broke the other one.  Thanks for
catching it.  The same return value for "invalid key" and "invalid regex" is OK
for me.

BTW is it OK to exit(-1);?  The return value of get_value() gets propagated to
the process exit status.  At the same time shell uses values >128 to indicate
that the process was terminated by a signal.

[...]
> When moving an existing code segment around like this, I would not mind to
> see style fixes thrown in to the patch, as long as the moved code is small
> enough.  Perhaps like this:

I've added the style fix into the patch.

[...]
> > +/* Auxiliary function to sanity-check and split the key into the section
> 
> 	/*
>        * Style. We write our multi-line comments
> 	 * like this.
>        */

Fixed.

> > +int git_config_parse_key(const char *key, char **store_key, int *baselen_)
[...]
> 
> Does it make sense for this function to be prepared to get called with
> NULL in store_key like this (and in the remainder of your patch)?

No, I wrote it unnecessarily generic.  Removed the excess code.  Thanks for
pointing it out.

[...]
> > +	test_must_fail git -c name=value config core.name
> >  '
> 
> Don't you want to make sure that your sanity check triggers in new tests?

Added a few tests after getting familiar with the test suite.

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague

[PATCH v4] Sanity-check config variable names

[Libor Pechacek]

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

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
as well as adding its own test.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Acked-by: Jeff King <peff@peff.net>
---
 builtin/config.c       |   22 ++++++++--
 cache.h                |    1 +
 config.c               |  104 ++++++++++++++++++++++++++++++------------------
 t/t1300-repo-config.sh |   18 ++++++--
 4 files changed, 97 insertions(+), 48 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index ca4a0db..dd17029 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,17 +167,30 @@ 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) {
+		char *tl;
+
+		/*
+		 * NEEDSWORK: this naive pattern lowercasing obviously does not
+		 * work for more complex patterns like "^[^.]*Foo.*bar".
+		 * Perhaps we should deprecate this altogether someday.
+		 */
+		for (tl = key + strlen(key) - 1;
+		     tl >= key && *tl != '.';
+		     tl--)
+			*tl = tolower(*tl);
+		for (tl = key; *tl && *tl != '.'; tl++)
+			*tl = tolower(*tl);
+
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			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..fde91f5 100644
--- a/config.c
+++ b/config.c
@@ -1099,6 +1099,69 @@ int git_config_set(const char *key, const char *value)
 }
 
 /*
+ * 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
+ * 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.
+	 */
+	*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;
+		}
+		(*store_key)[i] = c;
+	}
+	(*store_key)[i] = 0;
+
+	return 0;
+
+out_free_ret_1:
+	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.
  * if multi_replace==0, nothing, or only one matching key/value is replaced,
@@ -1124,59 +1187,22 @@ 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;
+	ret = -git_config_parse_key(key, &store.key, &store.baselen);
+	if (ret)
 		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
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d0e5546..c3d91d1 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -876,11 +876,21 @@ 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_expect_success 'key sanity-checking' '
+	test_must_fail git config foo=bar &&
+	test_must_fail git config foo=.bar &&
+	test_must_fail git config foo.ba=r &&
+	test_must_fail git config foo.1bar &&
+	test_must_fail git config foo."ba
+				z".bar &&
+	git config foo.bar true &&
+	git config foo."ba =z".bar false
 '
 
 test_done
-- 
1.7.4.rc3.10.gf91c9

Re: [PATCH v4] Sanity-check config variable names

[Junio C Hamano]

Libor Pechacek <lpechacek@suse.cz> writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index ca4a0db..dd17029 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -168,17 +167,30 @@ static int get_value(const char *key_, const char *regex_)
> ...
>  		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
>  		if (regcomp(key_regexp, key, REG_EXTENDED)) {
>  			fprintf(stderr, "Invalid key pattern: %s\n", key_);
>  			goto free_strings;

This is not a new issue introduced by this series, but isn't this goto
leaking "key" by jumping over free(key) later in the function but before
its target?

>  		}
> +	} else {
> +		if (git_config_parse_key(key_, &key, NULL))
> +			goto free_strings;

This seemingly has the same issue but is worse than that.  You allocate
and overwrite "key" in git_config_parse_key(), so by calling the function
after allocating key in the caller, you immediately leak it.  The new copy
allocated inside the callee is freed at its end upon error return, so
jumping over free(key) in the caller does not leak, though.

> @@ -1124,59 +1187,22 @@ int git_config_set(const char *key, const char *value)
> ...
> +	ret = -git_config_parse_key(key, &store.key, &store.baselen);
> +	if (ret)
>  		goto out_free;

This '-' is very easy to miss; I'd rather see it spelled out with an
explanation.

But looking at the bigger picture, don't you think that an internal
function like git_config_set() should return negative on error, and
we should make it the caller's responsibility to turn it to a value
suitable for feeding exit(3)?  It obviously is a separate topic.


Here is a minimum fix-up on top of your patch.
Thanks.

diff --git a/builtin/config.c b/builtin/config.c
index dd17029..6754da8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -166,8 +166,6 @@ static int get_value(const char *key_, const char *regex_)
 			system_wide = git_etc_gitconfig();
 	}
 
-	key = xstrdup(key_);
-
 	if (use_key_regexp) {
 		char *tl;
 
@@ -176,6 +174,8 @@ static int get_value(const char *key_, const char *regex_)
 		 * work for more complex patterns like "^[^.]*Foo.*bar".
 		 * Perhaps we should deprecate this altogether someday.
 		 */
+
+		key = xstrdup(key_);
 		for (tl = key + strlen(key) - 1;
 		     tl >= key && *tl != '.';
 		     tl--)
@@ -186,6 +186,7 @@ static int get_value(const char *key_, const char *regex_)
 		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
 		if (regcomp(key_regexp, key, REG_EXTENDED)) {
 			fprintf(stderr, "Invalid key pattern: %s\n", key_);
+			free(key);
 			goto free_strings;
 		}
 	} else {
diff --git a/config.c b/config.c
index fde91f5..f758734 100644
--- a/config.c
+++ b/config.c
@@ -1141,7 +1141,8 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 			dot = 1;
 		/* Leave the extended basename untouched.. */
 		if (!dot || i > baselen) {
-			if (!iskeychar(c) || (i == baselen+1 && !isalpha(c))) {
+			if (!iskeychar(c) ||
+			    (i == baselen + 1 && !isalpha(c))) {
 				error("invalid key: %s", key);
 				goto out_free_ret_1;
 			}
@@ -1197,7 +1198,8 @@ int git_config_set_multivar(const char *key, const char *value,
 	else
 		config_filename = git_pathdup("config");
 
-	ret = -git_config_parse_key(key, &store.key, &store.baselen);
+	/* parse-key returns negative; flip the sign to feed exit(3) */
+	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
 	if (ret)
 		goto out_free;
 

Re: [PATCH v4] Sanity-check config variable names

[Libor Pechacek]

On Thu 10-02-11 14:49:05, Junio C Hamano wrote:
> Libor Pechacek <lpechacek@suse.cz> writes:
> 
> > diff --git a/builtin/config.c b/builtin/config.c
> > index ca4a0db..dd17029 100644
> > --- a/builtin/config.c
> > +++ b/builtin/config.c
> > @@ -168,17 +167,30 @@ static int get_value(const char *key_, const char *regex_)
> > ...
> >  		key_regexp = (regex_t*)xmalloc(sizeof(regex_t));
> >  		if (regcomp(key_regexp, key, REG_EXTENDED)) {
> >  			fprintf(stderr, "Invalid key pattern: %s\n", key_);
> >  			goto free_strings;
> 
> This is not a new issue introduced by this series, but isn't this goto
> leaking "key" by jumping over free(key) later in the function but before
> its target?

Yes, sure and not only there.  A few lines below there is the compilation of
the value regex.  Freeing the memory allocated for key storage obviously needs
a little bit more work and I'd like to post the fix as a separate patch.

> >  		}
> > +	} else {
> > +		if (git_config_parse_key(key_, &key, NULL))
> > +			goto free_strings;
> 
> This seemingly has the same issue but is worse than that.  You allocate
> and overwrite "key" in git_config_parse_key(), so by calling the function
> after allocating key in the caller, you immediately leak it.  The new copy
> allocated inside the callee is freed at its end upon error return, so
> jumping over free(key) in the caller does not leak, though.

Negligence on my part, thanks for catching it.

> > @@ -1124,59 +1187,22 @@ int git_config_set(const char *key, const char *value)
> > ...
> > +	ret = -git_config_parse_key(key, &store.key, &store.baselen);
> > +	if (ret)
> >  		goto out_free;
> 
> This '-' is very easy to miss; I'd rather see it spelled out with an
> explanation.

Agree.  It would be yet better to get rid this weird transformation at all.

> But looking at the bigger picture, don't you think that an internal
> function like git_config_set() should return negative on error, and
> we should make it the caller's responsibility to turn it to a value
> suitable for feeding exit(3)?  It obviously is a separate topic.

Agree and not only that.  The exit codes are inconsistent between "set" and
"get" operation[1].  IMHO it's also questionable to feed exit(3) with negative
values.

Would you be in favor of getting the exit codes consistent and documented?

> Here is a minimum fix-up on top of your patch.

Squashed into my patch, with the exception of the memleak fix, which I'm going
to post separately.

> Thanks.

Thanks for the opportunity to contribute. :)

Libor

[1] $ ./git-config x.x x [; echo $?
    error: invalid pattern: [
    6
    $ ./git-config --get x.x [; echo $?
    Invalid pattern: [
    255

-- 
Libor Pechacek
SUSE L3 Team, Prague

[PATCH] Disallow empty section and variable names

[Libor Pechacek]

It is possible to break you repository config by creating an invalid config
option.  The config parser in turn chokes on it.

$ git init
Initialized empty Git repository in /tmp/gittest/.git/
$ git config .foo false
$ git config .foo
fatal: bad config file line 6 in .git/config

This patch makes git-config reject keys which start or end with a dot.  The fix
also revealed a typo in t5526-fetch-submodules, which is fixed by this patch as
well.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
---

Applies on top "Sanity-check config variable names".

 config.c                    |    7 ++++++-
 t/t5526-fetch-submodules.sh |    2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index c976544..81a0705 100644
--- a/config.c
+++ b/config.c
@@ -1119,7 +1119,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	 * key name separated by a dot, we have to know where the dot is.
 	 */
 
-	if (last_dot == NULL) {
+	if (last_dot == NULL || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
@@ -1156,6 +1156,11 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	if (store_key)
 		(*store_key)[i] = 0;
 
+	if (key[i-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		goto out_free_ret_1;
+	}
+
 	return 0;
 
 out_free_ret_1:
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 884a5e5..7106c6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 	(
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
+		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_cmp expect.out actual.out &&
-- 
1.7.4.rc3.3.g8b2bfe

[PATCH v2] Disallow empty section and variable names

[Libor Pechacek]

It is possible to break your repository config by creating an invalid key.  The
config parser in turn chokes on it.

$ git init
Initialized empty Git repository in /tmp/gittest/.git/
$ git config .foo false
$ git config .foo
fatal: bad config file line 6 in .git/config

This patch makes git-config reject keys which start or end with a dot, adds
tests for these cases and also fixes a typo in t5526-fetch-submodules, which
was exposed by the new check.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
---

Added tests for the cases checked, made git_config_parse_key consistently
return -2 when the key is invalid.  Applies on top "Sanity-check config
variable names".

 config.c                    |    8 +++++++-
 t/t1300-repo-config.sh      |    4 ++++
 t/t5526-fetch-submodules.sh |    2 +-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index fde91f5..e27a39f 100644
--- a/config.c
+++ b/config.c
@@ -1120,11 +1120,17 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	 * key name separated by a dot, we have to know where the dot is.
 	 */
 
-	if (last_dot == NULL) {
+	if (last_dot == NULL || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
+	i = strlen(key);
+	if (i && key[i-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		return -2;
+	}
+
 	baselen = last_dot - key;
 	if (baselen_)
 		*baselen_ = baselen;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c3d91d1..568d51d 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -889,6 +889,10 @@ test_expect_success 'key sanity-checking' '
 	test_must_fail git config foo.1bar &&
 	test_must_fail git config foo."ba
 				z".bar &&
+	test_must_fail git config . &&
+	test_must_fail git config .foo &&
+	test_must_fail git config foo. &&
+	test_must_fail git config .foo. &&
 	git config foo.bar true &&
 	git config foo."ba =z".bar false
 '
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 884a5e5..7106c6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 	(
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
+		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_cmp expect.out actual.out &&
-- 
1.7.4.rc3.11.g54760

Re: [PATCH v2] Disallow empty section and variable names

[Johannes Sixt]

Am 1/30/2011 21:34, schrieb Libor Pechacek:
> It is possible to break your repository config by creating an invalid key.  The
> config parser in turn chokes on it.
> 
> $ git init
> Initialized empty Git repository in /tmp/gittest/.git/
> $ git config .foo false
> $ git config .foo
> fatal: bad config file line 6 in .git/config

Just a nit: Your example is misleading because it "only" shows that if you
shove in junk, and ask for junk, you get breakage. However, the breakage
is much more serious, and you could have demonstrated it, if you had
written your example like this:

$ git config .foo false
$ git config user.email
fatal: bad config file line 6 in .git/config

> +	test_must_fail git config . &&
> +	test_must_fail git config .foo &&
> +	test_must_fail git config foo. &&
> +	test_must_fail git config .foo. &&

Not a nit: These tests only show that 'git config' cannot be asked for
junk, but they do not show that you cannot insert junk into the config
file anymore using 'git config'.

-- Hannes

Re: [PATCH v2] Disallow empty section and variable names

[Libor Pechacek]

On Mon 31-01-11 08:48:31, Johannes Sixt wrote:
> Am 1/30/2011 21:34, schrieb Libor Pechacek:
> > It is possible to break your repository config by creating an invalid key.  The
> > config parser in turn chokes on it.
> > 
> > $ git init
> > Initialized empty Git repository in /tmp/gittest/.git/
> > $ git config .foo false
> > $ git config .foo
> > fatal: bad config file line 6 in .git/config
> 
> Just a nit: Your example is misleading because it "only" shows that if you
> shove in junk, and ask for junk, you get breakage. However, the breakage
> is much more serious, and you could have demonstrated it, if you had
> written your example like this:
> 
> $ git config .foo false
> $ git config user.email
> fatal: bad config file line 6 in .git/config

Yes, that one is more persuasive.  Thanks for hint.

> > +	test_must_fail git config . &&
> > +	test_must_fail git config .foo &&
> > +	test_must_fail git config foo. &&
> > +	test_must_fail git config .foo. &&
> 
> Not a nit: These tests only show that 'git config' cannot be asked for
> junk, but they do not show that you cannot insert junk into the config
> file anymore using 'git config'.

This change builds on top of "Sanity-check config variable names" which makes
setting and getting values use the same key checking routine.  For the moment,
it does not matter if we test the "set" ot "get" path.

Yet, I accept the get/set operations can use different key checking routine in
the future (although I can't imagine the reson for it), and overall testing
the "set" path is in line with the idea of the patch.

Thanks for input, I'll send updated patch.

Libor
-- 
Libor Pechacek
SUSE L3 Team, Prague

Re: [PATCH v2] Disallow empty section and variable names

[Johannes Sixt]

Am 1/31/2011 10:17, schrieb Libor Pechacek:
> On Mon 31-01-11 08:48:31, Johannes Sixt wrote:
>> Am 1/30/2011 21:34, schrieb Libor Pechacek:
>>> +	test_must_fail git config . &&
>>> +	test_must_fail git config .foo &&
>>> +	test_must_fail git config foo. &&
>>> +	test_must_fail git config .foo. &&
>>
>> Not a nit: These tests only show that 'git config' cannot be asked for
>> junk, but they do not show that you cannot insert junk into the config
>> file anymore using 'git config'.
> 
> This change builds on top of "Sanity-check config variable names" which makes
> setting and getting values use the same key checking routine.  For the moment,
> it does not matter if we test the "set" ot "get" path.

The purpose of tests is not to check the implementation, but the
observable behavior. Therefore, you want to write a test case to make sure
that neither usage is broken by future changes.

-- Hannes

[PATCH v3] Disallow empty section and variable names

[Libor Pechacek]

It is possible to break your repository config by creating an invalid key.  The
config parser in turn chokes on it.

$ git init
Initialized empty Git repository in /tmp/gittest/.git/
$ git config .foo false
$ git config core.bare
fatal: bad config file line 6 in .git/config

This patch makes git-config reject keys which start or end with a dot, adds
tests for these cases and also fixes a typo in t5526-fetch-submodules, which
was exposed by the new check.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
Cc: Johannes Sixt <j.sixt@viscovery.net>
---

Incoporated feedback from Johannes, introduced keylen local variable to improve
readability of the code.  Applies on top "Sanity-check config variable names".

 config.c                    |   10 ++++++++--
 t/t1300-repo-config.sh      |    4 ++++
 t/t5526-fetch-submodules.sh |    2 +-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index fde91f5..5eb89a7 100644
--- a/config.c
+++ b/config.c
@@ -1113,6 +1113,7 @@ int git_config_set(const char *key, const char *value)
 int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 {
 	int i, dot, baselen;
+	int keylen = strlen(key);
 	const char *last_dot = strrchr(key, '.');
 
 	/*
@@ -1120,11 +1121,16 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	 * key name separated by a dot, we have to know where the dot is.
 	 */
 
-	if (last_dot == NULL) {
+	if (last_dot == NULL || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
+	if (keylen && key[keylen-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		return -2;
+	}
+
 	baselen = last_dot - key;
 	if (baselen_)
 		*baselen_ = baselen;
@@ -1132,7 +1138,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(strlen(key) + 1);
+	*store_key = xmalloc(keylen + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c3d91d1..53fb822 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -889,6 +889,10 @@ test_expect_success 'key sanity-checking' '
 	test_must_fail git config foo.1bar &&
 	test_must_fail git config foo."ba
 				z".bar &&
+	test_must_fail git config . false &&
+	test_must_fail git config .foo false &&
+	test_must_fail git config foo. false &&
+	test_must_fail git config .foo. false &&
 	git config foo.bar true &&
 	git config foo."ba =z".bar false
 '
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 884a5e5..7106c6c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
 	(
 		cd downstream &&
 		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
-		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
+		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&
 		git config --unset submodule.submodule.fetchRecurseSubmodules
 	) &&
 	test_cmp expect.out actual.out &&
-- 
1.7.4.rc3.11.g863f7

Re: [PATCH v3] Disallow empty section and variable names

[Jens Lehmann]

Am 31.01.2011 14:08, schrieb Libor Pechacek:
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 884a5e5..7106c6c 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -124,7 +124,7 @@ test_expect_success "--recurse-submodules overrides fetchRecurseSubmodules setti
>  	(
>  		cd downstream &&
>  		git fetch --recurse-submodules >../actual.out 2>../actual.err &&
> -		git config -f --unset .gitmodules submodule.submodule.fetchRecurseSubmodules true &&
> +		git config -f .gitmodules --unset submodule.submodule.fetchRecurseSubmodules true &&

This is not quite the right fix (notice the extra "true" value
at the end). IMHO this should be fixed separately, patch coming.

[PATCH v4] Disallow empty section and variable names

[Libor Pechacek]

It is possible to break your repository config by creating an invalid key.  The
config parser in turn chokes on it.

$ git init
Initialized empty Git repository in /tmp/gittest/.git/
$ git config .foo false
$ git config core.bare
fatal: bad config file line 6 in .git/config

This patch makes git-config reject keys which start or end with a dot and adds
tests for these cases.

Signed-off-by: Libor Pechacek <lpechacek@suse.cz>
---

Fix in t5526-fetch-submodules.sh was posted separately by Jens Lehmann.
Applies on top "Sanity-check config variable names".

 config.c               |   10 ++++++++--
 t/t1300-repo-config.sh |    4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index fde91f5..5eb89a7 100644
--- a/config.c
+++ b/config.c
@@ -1113,6 +1113,7 @@ int git_config_set(const char *key, const char *value)
 int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 {
 	int i, dot, baselen;
+	int keylen = strlen(key);
 	const char *last_dot = strrchr(key, '.');
 
 	/*
@@ -1120,11 +1121,16 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	 * key name separated by a dot, we have to know where the dot is.
 	 */
 
-	if (last_dot == NULL) {
+	if (last_dot == NULL || *key == '.') {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
+	if (keylen && key[keylen-1] == '.') {
+		error("key does not contain variable name: %s", key);
+		return -2;
+	}
+
 	baselen = last_dot - key;
 	if (baselen_)
 		*baselen_ = baselen;
@@ -1132,7 +1138,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(strlen(key) + 1);
+	*store_key = xmalloc(keylen + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c3d91d1..53fb822 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -889,6 +889,10 @@ test_expect_success 'key sanity-checking' '
 	test_must_fail git config foo.1bar &&
 	test_must_fail git config foo."ba
 				z".bar &&
+	test_must_fail git config . false &&
+	test_must_fail git config .foo false &&
+	test_must_fail git config foo. false &&
+	test_must_fail git config .foo. false &&
 	git config foo.bar true &&
 	git config foo."ba =z".bar false
 '
-- 
1.7.4.rc3.11.g863f7

Re: [PATCH v4] Disallow empty section and variable names

[Junio C Hamano]

Libor Pechacek <lpechacek@suse.cz> writes:

> diff --git a/config.c b/config.c
> index fde91f5..5eb89a7 100644
> --- a/config.c
> +++ b/config.c
> @@ -1113,6 +1113,7 @@ int git_config_set(const char *key, const char *value)
>  int git_config_parse_key(const char *key, char **store_key, int *baselen_)
>  {
>  	int i, dot, baselen;
> +	int keylen = strlen(key);
>  	const char *last_dot = strrchr(key, '.');
>  
>  	/*
> @@ -1120,11 +1121,16 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
>  	 * key name separated by a dot, we have to know where the dot is.
>  	 */
>  
> -	if (last_dot == NULL) {
> +	if (last_dot == NULL || *key == '.') {
>  		error("key does not contain a section: %s", key);
>  		return -2;
>  	}
>  
> +	if (keylen && key[keylen-1] == '.') {

Aren't these the same as saying

	if (!last_dot || last_dot == key) {
		...
        }
	if (!last_dot[1]) { /* we know last_dot is not NULL */
        	...
	}

Again, a minimum fix-up on top of your patch.
Thanks.


diff --git a/config.c b/config.c
index 75dd629..d5bb862 100644
--- a/config.c
+++ b/config.c
@@ -1113,7 +1113,6 @@ int git_config_set(const char *key, const char *value)
 int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 {
 	int i, dot, baselen;
-	int keylen = strlen(key);
 	const char *last_dot = strrchr(key, '.');
 
 	/*
@@ -1121,12 +1120,12 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	 * key name separated by a dot, we have to know where the dot is.
 	 */
 
-	if (last_dot == NULL || *key == '.') {
+	if (last_dot == NULL || last_dot == key) {
 		error("key does not contain a section: %s", key);
 		return -2;
 	}
 
-	if (keylen && key[keylen-1] == '.') {
+	if (!last_dot[1]) {
 		error("key does not contain variable name: %s", key);
 		return -2;
 	}
@@ -1138,7 +1137,7 @@ int git_config_parse_key(const char *key, char **store_key, int *baselen_)
 	/*
 	 * Validate the key and while at it, lower case it for matching.
 	 */
-	*store_key = xmalloc(keylen + 1);
+	*store_key = xmalloc(strlen(key) + 1);
 
 	dot = 0;
 	for (i = 0; key[i]; i++) {