threads / discuss / 26243
Subject: git-config does not check validity of variable names
Hello,
I've noticed that git-config accepts variable names in the form "a=b" for its "get" operation. That means "git config a=b" does not write anything to its output and exists with status 1.
According to the man page only alphanumeric characters and - are allowed in variable names. Would it make sense to spit out an error message when the user supplies an invalid variable name like the above?
Libor
-- Libor Pechacek SUSE L3 Team, Prague
Jeff KingRe: git-config does not check validity of variable names
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
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
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
Jeff KingRe: [PATCH] Sanity-ckeck config variable names
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
Jeff KingRe: [PATCH] Sanity-ckeck config variable names
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
Re: [PATCH] Sanity-ckeck config variable names
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
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
Jeff KingRe: [PATCH v2] Sanity-ckeck config variable names
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
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
Junio C HamanoRe: [PATCH v3] Sanity-check config variable names
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
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
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
Junio C HamanoRe: [PATCH v4] Sanity-check config variable names
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
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] Documentation fixes in git-config
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
Jeff KingRe: [PATCH] Documentation fixes in git-config
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
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
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
Jeff KingRe: [PATCH v2] Documentation fixes in git-config
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
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
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
Jeff KingRe: [PATCH v3] Documentation fixes in git-config
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
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
Junio C HamanoRe: [PATCH v3] Documentation fixes in git-config
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
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