{"thread":{"id":"37848","subject":"Re: [PATCH] config: add show_err flag to git_config_parse_key()","startedAt":"2014-10-30T18:18:06Z","lastAt":"2014-10-30T18:18:06Z","messageCount":1,"participants":["Tanay Abhra"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251228","messageId":"CAEc54XCcY68bqw9mp4wokOKJEz3Fr5+U7jOStfQ3QWcj7sgDqw@mail.gmail.com","threadId":"37848","inReplyTo":null,"subject":"Re: [PATCH] config: add show_err flag to git_config_parse_key()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-10-30T18:18:06Z","receivedAt":"2014-10-30T18:18:06Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":">From c87ddf6397964154932d49385ed1433b62631f30 Mon Sep 17 00:00:00 2001\nFrom: Tanay Abhra <tanayabh@gmail.com>\nDate: Thu, 30 Oct 2014 08:54:58 -0700\nSubject: [PATCH] config: add show_err flag to git_config_parse_key()\n\n`git_config_parse_key()` is used to sanitize the input key.\nSome callers of the function like `git_config_set_multivar_in_file()`\nget the pre-sanitized key directly from the user so it becomes\nnecessary to raise an error specifying what went wrong when the entered\nkey is defective.\n\nOther callers like `configset_find_element()` get their keys from\nthe git itself so a return value signifying error would be enough.\nThe error output shown to the user is useless and confusing in this\ncase so add a show_err flag to suppress errors in such cases.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n\n*Resend*\nI am behind a firewall and gmail web interface butchered my previous mail.\nThis is a resend, if the patch again corrupts I will send it tomorrow through\na proper internet connection.\n\nHi,\n\nYou were right, one of the functions was calling git_config_parse_key()\nwhich was leaking errors to the console. git_config_parse_key() was\nmeant for sanitizing user provided keys only but it was being used\ninternally in a place where only a return value would be enough.\n\nThanks for bringing this to our attention.\n\nCheers,\nTanay Abhra.\n\n builtin/config.c |  2 +-\n cache.h          |  2 +-\n config.c         | 19 ++++++++++++-------\n 3 files changed, 14 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 8cc2604..51635dc 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -200,7 +200,7 @@ static int get_value(const char *key_, const char *regex_)\n \t\t\tgoto free_strings;\n \t\t}\n \t} else {\n-\t\tif (git_config_parse_key(key_, &key, NULL)) {\n+\t\tif (git_config_parse_key(key_, &key, NULL, 1)) {\n \t\t\tret = CONFIG_INVALID_KEY;\n \t\t\tgoto free_strings;\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex 99ed096..8129590 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1362,7 +1362,7 @@ extern int git_config_string(const char **,\nconst char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set(const char *, const char *);\n-extern int git_config_parse_key(const char *, char **, int *);\n+extern int git_config_parse_key(const char *, char **, int *, int);\n extern int git_config_set_multivar(const char *, const char *, const\nchar *, int);\n extern int git_config_set_multivar_in_file(const char *, const char\n*, const char *, const char *, int);\n extern int git_config_rename_section(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex 15a2983..eb9058c 100644\n--- a/config.c\n+++ b/config.c\n@@ -1299,7 +1299,7 @@ static struct config_set_element\n*configset_find_element(struct config_set *cs,\n \t * `key` may come from the user, so normalize it before using it\n \t * for querying entries from the hashmap.\n \t */\n-\tret = git_config_parse_key(key, &normalized_key, NULL);\n+\tret = git_config_parse_key(key, &normalized_key, NULL, 0);\n\n \tif (ret)\n \t\treturn NULL;\n@@ -1832,8 +1832,9 @@ int git_config_set(const char *key, const char *value)\n  *             lowercase section and variable name\n  * baselen - pointer to int which will hold the length of the\n  *           section + subsection part, can be NULL\n+ * show_err - toggle whether the function raises an error on a defective key\n  */\n-int git_config_parse_key(const char *key, char **store_key, int *baselen_)\n+int git_config_parse_key(const char *key, char **store_key, int\n*baselen_, int show_err)\n {\n \tint i, dot, baselen;\n \tconst char *last_dot = strrchr(key, '.');\n@@ -1844,12 +1845,14 @@ int git_config_parse_key(const char *key, char\n**store_key, int *baselen_)\n \t */\n\n \tif (last_dot == NULL || last_dot == key) {\n-\t\terror(\"key does not contain a section: %s\", key);\n+\t\tif (show_err)\n+\t\t\terror(\"key does not contain a section: %s\", key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n\n \tif (!last_dot[1]) {\n-\t\terror(\"key does not contain variable name: %s\", key);\n+\t\tif (show_err)\n+\t\t\terror(\"key does not contain variable name: %s\", key);\n \t\treturn -CONFIG_NO_SECTION_OR_NAME;\n \t}\n\n@@ -1871,12 +1874,14 @@ int git_config_parse_key(const char *key, char\n**store_key, int *baselen_)\n \t\tif (!dot || i > baselen) {\n \t\t\tif (!iskeychar(c) ||\n \t\t\t    (i == baselen + 1 && !isalpha(c))) {\n-\t\t\t\terror(\"invalid key: %s\", key);\n+\t\t\t\tif (show_err)\n+\t\t\t\t\terror(\"invalid key: %s\", key);\n \t\t\t\tgoto out_free_ret_1;\n \t\t\t}\n \t\t\tc = tolower(c);\n \t\t} else if (c == '\\n') {\n-\t\t\terror(\"invalid key (newline): %s\", key);\n+\t\t\tif (show_err)\n+\t\t\t\terror(\"invalid key (newline): %s\", key);\n \t\t\tgoto out_free_ret_1;\n \t\t}\n \t\t(*store_key)[i] = c;\n@@ -1926,7 +1931,7 @@ int git_config_set_multivar_in_file(const char\n*config_filename,\n \tchar *filename_buf = NULL;\n\n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n-\tret = 0 - git_config_parse_key(key, &store.key, &store.baselen);\n+\tret = 0 - git_config_parse_key(key, &store.key, &store.baselen, 1);\n \tif (ret)\n \t\tgoto out_free;\n\n-- \n1.9.0.GIT\n"}]}