Re: [PATCH v2] add a flag to supress errors in git_config_parse_key()

[Jeff King]

On Mon, Feb 16, 2015 at 01:28:07PM +0530, Tanay Abhra wrote:

 I went through Junio's config guideline patch series
 and the whole thread of underscore bug report and I also think
 that pager.*.command is the right path to go.
 
 If you want to relax the syntactic requirement (such as add '_' to
 the current set of allowed chacters), I can work upon it but most of the
 comments point that moving towards pager.*.command would be better.

No, as silly as I find the _ restriction, it is not worth doing. One,
it would not cover all cases (it is one common case, so it makes the
problem more rare but does not eliminate it). And two, there are other
parsers of git's config format. Technically we do not need to care about
them and they can follow our lead, but we do not need to make things
harder on them than is necessary.

   if (last_dot == NULL || last_dot == key) {
 - error(key does not contain a section: %s, key);
 + if (!flags)
 + error(key does not contain a section: %s, key);

The intent of the flag variable is that you would check:

  if (!(flags  CONFIG_ERROR_QUIET))

here. I know that there are no other flags yet, so the two are
equivalent. But when somebody adds a new flag later, you would not want
them to have to tweak each of these sites.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] add a flag to supress errors in git_config_parse_key()

[Tanay Abhra]

`git_config_parse_key()` is used to sanitize the input key.
Some callers of the function like `git_config_set_multivar_in_file()`
get the pre-sanitized key directly from the user so it becomes
necessary to raise an error specifying what went wrong when the entered
key is syntactically malformed.

Other callers like `configset_find_element()` get their keys from
the git itself so a return value signifying error would be enough.
The error output shown to the user is useless and confusing in this
case so add a flag to suppress errors in such cases.

Helped-by: Junio C Hamano gits...@pobox.com
Helped-by: Jeff King p...@peff.net
Signed-off-by: Tanay Abhra tanay...@gmail.com
---
Hi Jeff,

I went through Junio's config guideline patch series
and the whole thread of underscore bug report and I also think
that pager.*.command is the right path to go.

If you want to relax the syntactic requirement (such as add '_' to
the current set of allowed chacters), I can work upon it but most of the
comments point that moving towards pager.*.command would be better.

p.s: I hope that I got the unsigned flag suggestion by Junio correctly.

-Tanay

 builtin/config.c |  2 +-
 cache.h  |  4 +++-
 config.c | 20 +---
 t/t7006-pager.sh |  9 +
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index d32c532..326d3d3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)
goto free_strings;
}
} else {
-   if (git_config_parse_key(key_, key, NULL)) {
+   if (git_config_parse_key(key_, key, NULL, 0)) {
ret = CONFIG_INVALID_KEY;
goto free_strings;
}
diff --git a/cache.h b/cache.h
index f704af5..9073ee2 100644
--- a/cache.h
+++ b/cache.h
@@ -1329,6 +1329,8 @@ extern int update_server_info(int);

 #define CONFIG_REGEX_NONE ((void *)1)

+#define CONFIG_ERROR_QUIET 0x0001
+
 struct git_config_source {
unsigned int use_stdin:1;
const char *file;
@@ -1358,7 +1360,7 @@ 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_in_file(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_parse_key(const char *, char **, int *, unsigned int);
 extern int git_config_set_multivar(const char *, const char *, const char *, 
int);
 extern int git_config_set_multivar_in_file(const char *, const char *, const 
char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index e5e64dc..7e23bb9 100644
--- a/config.c
+++ b/config.c
@@ -1309,7 +1309,7 @@ static struct config_set_element 
*configset_find_element(struct config_set *cs,
 * `key` may come from the user, so normalize it before using it
 * for querying entries from the hashmap.
 */
-   ret = git_config_parse_key(key, normalized_key, NULL);
+   ret = git_config_parse_key(key, normalized_key, NULL, 
CONFIG_ERROR_QUIET);

if (ret)
return NULL;
@@ -1842,8 +1842,10 @@ int git_config_set(const char *key, const char *value)
  * lowercase section and variable name
  * baselen - pointer to int which will hold the length of the
  *   section + subsection part, can be NULL
+ * flags - toggle whether the function raises an error on a syntactically
+ * malformed key
  */
-int git_config_parse_key(const char *key, char **store_key, int *baselen_)
+int git_config_parse_key(const char *key, char **store_key, int *baselen_, 
unsigned int flags)
 {
int i, dot, baselen;
const char *last_dot = strrchr(key, '.');
@@ -1854,12 +1856,14 @@ int git_config_parse_key(const char *key, char 
**store_key, int *baselen_)
 */

if (last_dot == NULL || last_dot == key) {
-   error(key does not contain a section: %s, key);
+   if (!flags)
+   error(key does not contain a section: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}

if (!last_dot[1]) {
-   error(key does not contain variable name: %s, key);
+   if (!flags)
+   error(key does not contain variable name: %s, key);
return -CONFIG_NO_SECTION_OR_NAME;
}

@@ -1881,12 +1885,14 @@ int git_config_parse_key(const char *key, char 
**store_key, int *baselen_)
if (!dot || i  baselen) {
if (!iskeychar(c) ||
(i == baselen + 1  !isalpha(c))) {
-   error(invalid key: %s, key);
+   if