{"thread":{"id":"37224","subject":"[PATCH v3 0/6] Rewrite `git_config()` using config-set API","startedAt":"2014-07-28T10:33:49Z","lastAt":"2014-07-28T11:38:41Z","messageCount":15,"participants":["Tanay Abhra","Ramsay Jones","Matthieu Moy"],"isPatch":true,"patchVersion":3,"patchTotal":6},"messages":[{"id":"246769","messageId":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":null,"subject":"[PATCH v3 0/6] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:49Z","receivedAt":"2014-07-28T10:33:49Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[PATCH V3]:All the suggestions in [3] applied. Built on top of [1].\n\n[PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows\n\tcorrect parsing order. Reordered the patches. Removed xfuncname patch\n\tas it was unnecssary.\n\nThis series builds on the top of topic[1] in the mailing list with name\n\"git config cache & special querying API utilizing the cache\".\n\nThis series aims to do these three things,\n\n* Use the config-set API to rewrite git_config().\n\n* Solve any legacy bugs in the previous system while at it.\n\n* To be feature complete compared to the previous git_config() implementation,\n  which I think it is now. (added the line number and file name info just for\n  completeness)\n\nAlso, I haven't yet checked the exact improvements but still as a teaser,\ngit status now only rereads the configuration files twice instead of four\ntimes.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/254286\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/254101\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/254211\n\nTanay Abhra (6):\n  config.c: fix accuracy of line number in errors\n  add line number and file name info to `config_set`\n  rewrite git_config() to use the config-set API\n  add a test for semantic errors in config files\n  config: add `git_die_config()` to the config-set API\n  add tests for `git_config_get_string()`\n\n Documentation/technical/api-config.txt |   5 ++\n cache.h                                |  25 ++++++++\n config.c                               | 114 +++++++++++++++++++++++++++++----\n t/t1308-config-set.sh                  |  20 ++++++\n test-config.c                          |  10 +++\n 5 files changed, 161 insertions(+), 13 deletions(-)\n\n-- \n1.9.0.GIT\n"},{"id":"246770","messageId":"1406543635-19281-2-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 1/6] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:50Z","receivedAt":"2014-07-28T10:33:50Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>\n\nIf a callback returns a negative value to `git_config*()` family,\nthey call `die()` while printing the line number and the file name.\nCurrently the printed line number is off by one, thus printing the\nwrong line number.\n\nMake `linenr` point to the line we just parsed during the call\nto callback to get accurate line number in error messages.\n\nCommit-message-by: Tanay Abhra <tanayabh@gmail.com>\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n config.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex d3ad661..e5b7f10 100644\n--- a/config.c\n+++ b/config.c\n@@ -244,6 +244,7 @@ static int get_next_char(void)\n \t\tcf->linenr++;\n \tif (c == EOF) {\n \t\tcf->eof = 1;\n+\t\tcf->linenr++;\n \t\tc = '\\n';\n \t}\n \treturn c;\n@@ -319,6 +320,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n {\n \tint c;\n \tchar *value;\n+\tint ret;\n \n \t/* Get the full name */\n \tfor (;;) {\n@@ -341,7 +343,15 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n \t\tif (!value)\n \t\t\treturn -1;\n \t}\n-\treturn fn(name->buf, value, data);\n+\t/*\n+\t * We already consumed the \\n, but we need linenr to point to\n+\t * the line we just parsed during the call to fn to get\n+\t * accurate line number in error messages.\n+\t */\n+\tcf->linenr--;\n+\tret = fn(name->buf, value, data);\n+\tcf->linenr++;\n+\treturn ret;\n }\n \n static int get_extended_base_var(struct strbuf *name, int c)\n-- \n1.9.0.GIT\n"},{"id":"246771","messageId":"1406543635-19281-3-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 2/6] add line number and file name info to `config_set`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:51Z","receivedAt":"2014-07-28T10:33:51Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Store file name and line number for each key-value pair in the cache\nduring parsing of the configuration files.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n config.c | 21 +++++++++++++++++++--\n 1 file changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex e5b7f10..5499108 100644\n--- a/config.c\n+++ b/config.c\n@@ -1232,6 +1232,11 @@ int git_config_with_options(config_fn_t fn, void *data,\n \treturn ret;\n }\n \n+struct key_value_info {\n+\tconst char *filename;\n+\tint linenr;\n+};\n+\n int git_config(config_fn_t fn, void *data)\n {\n \treturn git_config_with_options(fn, data, NULL, 1);\n@@ -1262,6 +1267,9 @@ static struct config_set_element *configset_find_element(struct config_set *cs,\n static int configset_add_value(struct config_set *cs, const char *key, const char *value)\n {\n \tstruct config_set_element *e;\n+\tstruct string_list_item *si;\n+\tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n+\n \te = configset_find_element(cs, key);\n \t/*\n \t * Since the keys are being fed by git_config*() callback mechanism, they\n@@ -1274,7 +1282,16 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \t\tstring_list_init(&e->value_list, 1);\n \t\thashmap_add(&cs->config_hash, e);\n \t}\n-\tstring_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\tsi = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\tif (cf) {\n+\t\tkv_info->filename = strintern(cf->name);\n+\t\tkv_info->linenr = cf->linenr;\n+\t} else {\n+\t\t/* for values read from `git_config_from_parameters()` */\n+\t\tkv_info->filename = strintern(\"parameter\");\n+\t\tkv_info->linenr = 0;\n+\t}\n+\tsi->util = kv_info;\n \n \treturn 0;\n }\n@@ -1301,7 +1318,7 @@ void git_configset_clear(struct config_set *cs)\n \thashmap_iter_init(&cs->config_hash, &iter);\n \twhile ((entry = hashmap_iter_next(&iter))) {\n \t\tfree(entry->key);\n-\t\tstring_list_clear(&entry->value_list, 0);\n+\t\tstring_list_clear(&entry->value_list, 1);\n \t}\n \thashmap_free(&cs->config_hash, 1);\n \tcs->hash_initialized = 0;\n-- \n1.9.0.GIT\n"},{"id":"246772","messageId":"1406543635-19281-4-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 3/6] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:52Z","receivedAt":"2014-07-28T10:33:52Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Of all the functions in `git_config*()` family, `git_config()` has the\nmost invocations in the whole code base. Each `git_config()` invocation\ncauses config file rereads which can be avoided using the config-set API.\n\nUse the config-set API to rewrite `git_config()` to use the config caching\nlayer to avoid config file rereads on each invocation during a git process\nlifetime. First invocation constructs the cache, and after that for each\nsuccessive invocation, `git_config()` feeds values from the config cache\ninstead of rereading the configuration files.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n cache.h  | 24 ++++++++++++++++++++++++\n config.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++--------\n 2 files changed, 73 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7292aef..93bdbab 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -8,6 +8,7 @@\n #include \"gettext.h\"\n #include \"convert.h\"\n #include \"trace.h\"\n+#include \"string-list.h\"\n \n #include SHA1_HEADER\n #ifndef git_SHA_CTX\n@@ -1351,9 +1352,32 @@ extern int parse_config_key(const char *var,\n \t\t\t    const char **subsection, int *subsection_len,\n \t\t\t    const char **key);\n \n+struct config_set_element {\n+\tstruct hashmap_entry ent;\n+\tchar *key;\n+\tstruct string_list value_list;\n+};\n+\n+struct configset_list_item {\n+\tstruct config_set_element *e;\n+\tint value_index;\n+};\n+\n+/*\n+ * the contents of the list are ordered according to their\n+ * position in the config files and order of parsing the files.\n+ * (i.e. key-value pair at the last position of .git/config will\n+ * be at the last item of the list)\n+ */\n+struct configset_list {\n+\tstruct configset_list_item *items;\n+\tunsigned int nr, alloc;\n+};\n+\n struct config_set {\n \tstruct hashmap config_hash;\n \tint hash_initialized;\n+\tstruct configset_list list;\n };\n \n extern void git_configset_init(struct config_set *cs);\ndiff --git a/config.c b/config.c\nindex 5499108..2ce3318 100644\n--- a/config.c\n+++ b/config.c\n@@ -35,12 +35,6 @@ struct config_source {\n \tlong (*do_ftell)(struct config_source *c);\n };\n \n-struct config_set_element {\n-\tstruct hashmap_entry ent;\n-\tchar *key;\n-\tstruct string_list value_list;\n-};\n-\n static struct config_source *cf;\n \n static int zlib_compression_seen;\n@@ -1237,11 +1231,44 @@ struct key_value_info {\n \tint linenr;\n };\n \n-int git_config(config_fn_t fn, void *data)\n+static int git_config_raw(config_fn_t fn, void *data)\n {\n \treturn git_config_with_options(fn, data, NULL, 1);\n }\n \n+static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n+{\n+\tint i, value_index;\n+\tstruct string_list *values;\n+\tstruct config_set_element *entry;\n+\tstruct configset_list *list = &cs->list;\n+\tstruct key_value_info *kv_info;\n+\n+\tfor (i = 0; i < list->nr; i++) {\n+\t\tentry = list->items[i].e;\n+\t\tvalue_index = list->items[i].value_index;\n+\t\tvalues = &entry->value_list;\n+\t\tif (fn(entry->key, values->items[value_index].string, data) < 0) {\n+\t\t\tkv_info = values->items[value_index].util;\n+\t\t\tif (!kv_info->linenr)\n+\t\t\t\tdie(\"unable to parse command-line config\");\n+\t\t\telse\n+\t\t\t\tdie(\"bad config file line %d in %s\",\n+\t\t\t\t\tkv_info->linenr,\n+\t\t\t\t\tkv_info->filename);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static void git_config_check_init(void);\n+\n+int git_config(config_fn_t fn, void *data)\n+{\n+\tgit_config_check_init();\n+\treturn configset_iter(&the_config_set, fn, data);\n+}\n+\n static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)\n {\n \tstruct config_set_element k;\n@@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n {\n \tstruct config_set_element *e;\n \tstruct string_list_item *si;\n+\tstruct configset_list_item *l_item;\n \tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n \n \te = configset_find_element(cs, key);\n@@ -1283,6 +1311,12 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \t\thashmap_add(&cs->config_hash, e);\n \t}\n \tsi = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\n+\tALLOC_GROW(cs->list.items, cs->list.nr + 1, cs->list.alloc);\n+\tl_item = &cs->list.items[cs->list.nr++];\n+\tl_item->e = e;\n+\tl_item->value_index = e->value_list.nr - 1;\n+\n \tif (cf) {\n \t\tkv_info->filename = strintern(cf->name);\n \t\tkv_info->linenr = cf->linenr;\n@@ -1306,6 +1340,9 @@ void git_configset_init(struct config_set *cs)\n {\n \thashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_set_element_cmp, 0);\n \tcs->hash_initialized = 1;\n+\tcs->list.nr = 0;\n+\tcs->list.alloc = 0;\n+\tcs->list.items = NULL;\n }\n \n void git_configset_clear(struct config_set *cs)\n@@ -1322,6 +1359,10 @@ void git_configset_clear(struct config_set *cs)\n \t}\n \thashmap_free(&cs->config_hash, 1);\n \tcs->hash_initialized = 0;\n+\tfree(cs->list.items);\n+\tcs->list.nr = 0;\n+\tcs->list.alloc = 0;\n+\tcs->list.items = NULL;\n }\n \n static int config_set_callback(const char *key, const char *value, void *cb)\n@@ -1440,7 +1481,7 @@ static void git_config_check_init(void)\n \tif (the_config_set.hash_initialized)\n \t\treturn;\n \tgit_configset_init(&the_config_set);\n-\tgit_config(config_set_callback, &the_config_set);\n+\tgit_config_raw(config_set_callback, &the_config_set);\n }\n \n void git_config_clear(void)\n-- \n1.9.0.GIT\n"},{"id":"246773","messageId":"1406543635-19281-5-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 4/6] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:53Z","receivedAt":"2014-07-28T10:33:53Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Semantic errors (for example, for alias.* variables NULL values are\nnot allowed) in configuration files cause a die printing the line\nnumber and file name of the offending value.\n\nAdd a test documenting that such errors cause a die printing the\naccurate line number and file name.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n t/t1308-config-set.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 7fdf840..35c6ee2 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -197,4 +197,15 @@ test_expect_success 'proper error on error in custom config files' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check line errors for malformed values' '\n+\tmv .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[alias]\n+\t\tbr\n+\tEOF\n+\ttest_expect_code 128 git br 2>result &&\n+\tgrep \"fatal: bad config file line 2 in .git/config\" result\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"246774","messageId":"1406543635-19281-6-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 5/6] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:54Z","receivedAt":"2014-07-28T10:33:54Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add `git_die_config` that dies printing the line number and the file name\nof the highest priority value for the configuration variable `key`.\n\nIt has usage in non-callback based config value retrieval where we can\nraise an error and die if there is a semantic error.\nFor example,\n\n\tif (!git_config_get_value(key, &value)) {\n\t\t/* NULL values not allowed */\n\t\tif (!value)\n\t\t\tgit_config_die(key);\n\t\telse\n\t\t\t/* do work */\n\t}\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\nNote: git_config_get_string() calls git_config_get_string_const(),\nso no need to check for whether to die or not, as it is done by\ngit_config_get_string_const().\n\n Documentation/technical/api-config.txt |  5 +++++\n cache.h                                |  1 +\n config.c                               | 24 ++++++++++++++++++++++--\n 3 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 815c1ee..e7ec7cc 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:\n \tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n \tthe user's home directory when found at the beginning of the path.\n \n+`void git_die_config(const char *key)`::\n+\n+\tDies printing the line number and the file name of the highest\n+\tpriority value for the configuration variable `key`.\n+\n See test-config.c for usage examples.\n \n Value Parsing Helpers\ndiff --git a/cache.h b/cache.h\nindex 93bdbab..8512225 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1406,6 +1406,7 @@ extern int git_config_get_bool(const char *key, int *dest);\n extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n extern int git_config_get_maybe_bool(const char *key, int *dest);\n extern int git_config_get_pathname(const char *key, const char **dest);\n+extern void git_die_config(const char *key);\n \n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\ndiff --git a/config.c b/config.c\nindex 2ce3318..136ee9c 100644\n--- a/config.c\n+++ b/config.c\n@@ -1505,8 +1505,12 @@ const struct string_list *git_config_get_value_multi(const char *key)\n \n int git_config_get_string_const(const char *key, const char **dest)\n {\n+\tint ret;\n \tgit_config_check_init();\n-\treturn git_configset_get_string_const(&the_config_set, key, dest);\n+\tret = git_configset_get_string_const(&the_config_set, key, dest);\n+\tif (ret < 0)\n+\t\tgit_die_config(key);\n+\treturn ret;\n }\n \n int git_config_get_string(const char *key, char **dest)\n@@ -1547,10 +1551,26 @@ int git_config_get_maybe_bool(const char *key, int *dest)\n \n int git_config_get_pathname(const char *key, const char **dest)\n {\n+\tint ret;\n \tgit_config_check_init();\n-\treturn git_configset_get_pathname(&the_config_set, key, dest);\n+\tret = git_configset_get_pathname(&the_config_set, key, dest);\n+\tif (ret < 0)\n+\t\tgit_die_config(key);\n+\treturn ret;\n }\n \n+void git_die_config(const char *key)\n+{\n+\tconst struct string_list *values;\n+\tstruct key_value_info *kv_info;\n+\tvalues = git_config_get_value_multi(key);\n+\tkv_info = values->items[values->nr - 1].util;\n+\tif (!kv_info->linenr)\n+\t\tdie(\"unable to parse command-line config\");\n+\telse\n+\t\tdie(\"bad config file line %d in %s\",kv_info->linenr, kv_info->filename);\n+ }\n+\n /*\n  * Find all the stuff for git_config_set() below.\n  */\n-- \n1.9.0.GIT\n"},{"id":"246775","messageId":"1406543635-19281-7-git-send-email-tanayabh@gmail.com","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 6/6] add tests for `git_config_get_string_const()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T10:33:55Z","receivedAt":"2014-07-28T10:33:55Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add tests for `git_config_get_string_const()`, check whether it\ndies printing the line number and the file name if a NULL\nvalue is retrieved for the given key.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n t/t1308-config-set.sh |  9 +++++++++\n test-config.c         | 10 ++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 35c6ee2..d7cdc6e 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -119,6 +119,15 @@ test_expect_success 'find integer value for a key' '\n \tcheck_config get_int lamb.chop 65\n '\n \n+test_expect_success 'find string value for a key' '\n+\tcheck_config get_string case.baz hask\n+'\n+\n+test_expect_success 'check line error when NULL string is queried' '\n+\ttest_expect_code 128 test-config get_string case.foo 2>result &&\n+\tgrep \"fatal: bad config file line 7 in .git/config\" result\n+'\n+\n test_expect_success 'find integer if value is non parse-able' '\n \tcheck_config expect_code 128 get_int lamb.head\n '\ndiff --git a/test-config.c b/test-config.c\nindex 9dd1b22..6a77552 100644\n--- a/test-config.c\n+++ b/test-config.c\n@@ -16,6 +16,8 @@\n  *\n  * get_bool -> print bool value for the entered key or die\n  *\n+ * get_string -> print string value for the entered key or die\n+ *\n  * configset_get_value -> returns value with the highest priority for the entered key\n  * \t\t\tfrom a config_set constructed from files entered as arguments.\n  *\n@@ -84,6 +86,14 @@ int main(int argc, char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n+\t\tif (!git_config_get_string_const(argv[2], &v)) {\n+\t\t\tprintf(\"%s\\n\", v);\n+\t\t\tgoto exit0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\tgoto exit1;\n+\t\t}\n \t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n-- \n1.9.0.GIT\n"},{"id":"246782","messageId":"53D62C1A.5000203@ramsay1.demon.co.uk","threadId":"37224","inReplyTo":"1406543635-19281-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-28T10:55:22Z","receivedAt":"2014-07-28T10:55:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 28/07/14 11:33, Tanay Abhra wrote:\n> Add `git_die_config` that dies printing the line number and the file name\n> of the highest priority value for the configuration variable `key`.\n> \n> It has usage in non-callback based config value retrieval where we can\n> raise an error and die if there is a semantic error.\n> For example,\n> \n> \tif (!git_config_get_value(key, &value)) {\n> \t\t/* NULL values not allowed */\n> \t\tif (!value)\n> \t\t\tgit_config_die(key);\n> \t\telse\n> \t\t\t/* do work */\n> \t}\n> \n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n> Note: git_config_get_string() calls git_config_get_string_const(),\n> so no need to check for whether to die or not, as it is done by\n> git_config_get_string_const().\n> \n>  Documentation/technical/api-config.txt |  5 +++++\n>  cache.h                                |  1 +\n>  config.c                               | 24 ++++++++++++++++++++++--\n>  3 files changed, 28 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n> index 815c1ee..e7ec7cc 100644\n> --- a/Documentation/technical/api-config.txt\n> +++ b/Documentation/technical/api-config.txt\n> @@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:\n>  \tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n>  \tthe user's home directory when found at the beginning of the path.\n>  \n> +`void git_die_config(const char *key)`::\n> +\n> +\tDies printing the line number and the file name of the highest\n> +\tpriority value for the configuration variable `key`.\n> +\n>  See test-config.c for usage examples.\n>  \n>  Value Parsing Helpers\n> diff --git a/cache.h b/cache.h\n> index 93bdbab..8512225 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1406,6 +1406,7 @@ extern int git_config_get_bool(const char *key, int *dest);\n>  extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n>  extern int git_config_get_maybe_bool(const char *key, int *dest);\n>  extern int git_config_get_pathname(const char *key, const char **dest);\n> +extern void git_die_config(const char *key);\n>  \n>  extern int committer_ident_sufficiently_given(void);\n>  extern int author_ident_sufficiently_given(void);\n> diff --git a/config.c b/config.c\n> index 2ce3318..136ee9c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1505,8 +1505,12 @@ const struct string_list *git_config_get_value_multi(const char *key)\n>  \n>  int git_config_get_string_const(const char *key, const char **dest)\n>  {\n> +\tint ret;\n>  \tgit_config_check_init();\n> -\treturn git_configset_get_string_const(&the_config_set, key, dest);\n> +\tret = git_configset_get_string_const(&the_config_set, key, dest);\n> +\tif (ret < 0)\n> +\t\tgit_die_config(key);\n> +\treturn ret;\n>  }\n>  \n>  int git_config_get_string(const char *key, char **dest)\n> @@ -1547,10 +1551,26 @@ int git_config_get_maybe_bool(const char *key, int *dest)\n>  \n>  int git_config_get_pathname(const char *key, const char **dest)\n>  {\n> +\tint ret;\n>  \tgit_config_check_init();\n> -\treturn git_configset_get_pathname(&the_config_set, key, dest);\n> +\tret = git_configset_get_pathname(&the_config_set, key, dest);\n> +\tif (ret < 0)\n> +\t\tgit_die_config(key);\n> +\treturn ret;\n>  }\n>  \n> +void git_die_config(const char *key)\n> +{\n> +\tconst struct string_list *values;\n> +\tstruct key_value_info *kv_info;\n> +\tvalues = git_config_get_value_multi(key);\n> +\tkv_info = values->items[values->nr - 1].util;\n> +\tif (!kv_info->linenr)\n> +\t\tdie(\"unable to parse command-line config\");\n> +\telse\n> +\t\tdie(\"bad config file line %d in %s\",kv_info->linenr, kv_info->filename);\n\nHow about including the 'key' string in the error message?\nSimilar comment applies to an earlier patch in this series.\n\n> + }\n> +\n>  /*\n>   * Find all the stuff for git_config_set() below.\n>   */\n> \n\nATB,\nRamsay Jones\n"},{"id":"246783","messageId":"53D62DBD.9@gmail.com","threadId":"37224","inReplyTo":"53D62C1A.5000203@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T11:02:21Z","receivedAt":"2014-07-28T11:02:21Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 7/28/2014 4:25 PM, Ramsay Jones wrote:\n> On 28/07/14 11:33, Tanay Abhra wrote:\n>> Add `git_die_config` that dies printing the line number and the file name\n>> of the highest priority value for the configuration variable `key`.\n>>\n>> It has usage in non-callback based config value retrieval where we can\n>> raise an error and die if there is a semantic error.\n>> For example,\n>>\n>> \tif (!git_config_get_value(key, &value)) {\n>> \t\t/* NULL values not allowed */\n>> \t\tif (!value)\n>> \t\t\tgit_config_die(key);\n>> \t\telse\n>> \t\t\t/* do work */\n>> \t}\n>>\n>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n>> ---\n>> Note: git_config_get_string() calls git_config_get_string_const(),\n>> so no need to check for whether to die or not, as it is done by\n>> git_config_get_string_const().\n>>\n>>  Documentation/technical/api-config.txt |  5 +++++\n>>  cache.h                                |  1 +\n>>  config.c                               | 24 ++++++++++++++++++++++--\n>>  3 files changed, 28 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n>> index 815c1ee..e7ec7cc 100644\n>> --- a/Documentation/technical/api-config.txt\n>> +++ b/Documentation/technical/api-config.txt\n>> @@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:\n>>  \tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n>>  \tthe user's home directory when found at the beginning of the path.\n>>  \n>> +`void git_die_config(const char *key)`::\n>> +\n>> +\tDies printing the line number and the file name of the highest\n>> +\tpriority value for the configuration variable `key`.\n>> +\n>>  See test-config.c for usage examples.\n>>  \n>>  Value Parsing Helpers\n>> diff --git a/cache.h b/cache.h\n>> index 93bdbab..8512225 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1406,6 +1406,7 @@ extern int git_config_get_bool(const char *key, int *dest);\n>>  extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n>>  extern int git_config_get_maybe_bool(const char *key, int *dest);\n>>  extern int git_config_get_pathname(const char *key, const char **dest);\n>> +extern void git_die_config(const char *key);\n>>  \n>>  extern int committer_ident_sufficiently_given(void);\n>>  extern int author_ident_sufficiently_given(void);\n>> diff --git a/config.c b/config.c\n>> index 2ce3318..136ee9c 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1505,8 +1505,12 @@ const struct string_list *git_config_get_value_multi(const char *key)\n>>  \n>>  int git_config_get_string_const(const char *key, const char **dest)\n>>  {\n>> +\tint ret;\n>>  \tgit_config_check_init();\n>> -\treturn git_configset_get_string_const(&the_config_set, key, dest);\n>> +\tret = git_configset_get_string_const(&the_config_set, key, dest);\n>> +\tif (ret < 0)\n>> +\t\tgit_die_config(key);\n>> +\treturn ret;\n>>  }\n>>  \n>>  int git_config_get_string(const char *key, char **dest)\n>> @@ -1547,10 +1551,26 @@ int git_config_get_maybe_bool(const char *key, int *dest)\n>>  \n>>  int git_config_get_pathname(const char *key, const char **dest)\n>>  {\n>> +\tint ret;\n>>  \tgit_config_check_init();\n>> -\treturn git_configset_get_pathname(&the_config_set, key, dest);\n>> +\tret = git_configset_get_pathname(&the_config_set, key, dest);\n>> +\tif (ret < 0)\n>> +\t\tgit_die_config(key);\n>> +\treturn ret;\n>>  }\n>>  \n>> +void git_die_config(const char *key)\n>> +{\n>> +\tconst struct string_list *values;\n>> +\tstruct key_value_info *kv_info;\n>> +\tvalues = git_config_get_value_multi(key);\n>> +\tkv_info = values->items[values->nr - 1].util;\n>> +\tif (!kv_info->linenr)\n>> +\t\tdie(\"unable to parse command-line config\");\n>> +\telse\n>> +\t\tdie(\"bad config file line %d in %s\",kv_info->linenr, kv_info->filename);\n> \n> How about including the 'key' string in the error message?\n> Similar comment applies to an earlier patch in this series.\n>\n\ngit_config_get_string & pathname() are already raising a\nconfig_error_nonbool() with the error message,\n\"Missing value for 'key'\".\n\nFor other cases, I was just following the previous error message,\nbut it can be done for the new git_config().\n\nThanks.\n\n>> + }\n>> +\n>>  /*\n>>   * Find all the stuff for git_config_set() below.\n>>   */\n>>\n> \n> ATB,\n> Ramsay Jones\n> \n> \n> \n"},{"id":"246786","messageId":"vpq8undvipr.fsf@anie.imag.fr","threadId":"37224","inReplyTo":"1406543635-19281-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-28T11:14:08Z","receivedAt":"2014-07-28T11:14:08Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> +void git_die_config(const char *key)\n> +{\n> +\tconst struct string_list *values;\n> +\tstruct key_value_info *kv_info;\n> +\tvalues = git_config_get_value_multi(key);\n> +\tkv_info = values->items[values->nr - 1].util;\n> +\tif (!kv_info->linenr)\n> +\t\tdie(\"unable to parse command-line config\");\n> +\telse\n> +\t\tdie(\"bad config file line %d in %s\",kv_info->linenr, kv_info->filename);\n\nMissing space after ,.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246787","messageId":"vpqvbqhu41s.fsf@anie.imag.fr","threadId":"37224","inReplyTo":"53D62DBD.9@gmail.com","subject":"Re: [PATCH v3 5/6] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-28T11:16:15Z","receivedAt":"2014-07-28T11:16:15Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> On 7/28/2014 4:25 PM, Ramsay Jones wrote:\n>> On 28/07/14 11:33, Tanay Abhra wrote:\n>>> +\tif (!kv_info->linenr)\n>>> +\t\tdie(\"unable to parse command-line config\");\n>>> +\telse\n>>> +\t\tdie(\"bad config file line %d in %s\",kv_info->linenr, kv_info->filename);\n>> \n>> How about including the 'key' string in the error message?\n>> Similar comment applies to an earlier patch in this series.\n>>\n>\n> git_config_get_string & pathname() are already raising a\n> config_error_nonbool() with the error message,\n> \"Missing value for 'key'\".\n>\n> For other cases, I was just following the previous error message,\n> but it can be done for the new git_config().\n\nIt is good to follow the existing in this patch (better code, same\nbehavior), but I agree with Ramsay that including key in the error\nmessage (for both branches of the if/then/else) in a further patch would\nbe good, and should be straightforward enough.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246788","messageId":"vpqmwbtu3rh.fsf@anie.imag.fr","threadId":"37224","inReplyTo":"1406543635-19281-5-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 4/6] add a test for semantic errors in config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-28T11:22:26Z","receivedAt":"2014-07-28T11:22:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> +test_expect_success 'check line errors for malformed values' '\n> +\tmv .git/config .git/config.old &&\n> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[alias]\n> +\t\tbr\n> +\tEOF\n> +\ttest_expect_code 128 git br 2>result &&\n> +\tgrep \"fatal: bad config file line 2 in .git/config\" result\n> +'\n\nThis is PATCH 4, and it tests a bug fixed in PATCH 1. It would have\neased review to group both patches, either\n\nPATCH 1: introduce test_expect_failure test to demonstrate the failure\nPATCH 2: fix the bug and change test_expect_failure to test_expect_success\n\nOr putting both in the same patch.\n\nI think the series is OK like this, my comment is just to be read as\n\"next time, here's how to do better\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246789","messageId":"vpqha21u3oq.fsf@anie.imag.fr","threadId":"37224","inReplyTo":"1406543635-19281-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 0/6] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-28T11:24:05Z","receivedAt":"2014-07-28T11:24:05Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> [PATCH V3]:All the suggestions in [3] applied. Built on top of [1].\n\nExcept for the minor style fix in PATCH 5, the series looks OK to me.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246790","messageId":"53D63389.5090205@gmail.com","threadId":"37224","inReplyTo":"vpqmwbtu3rh.fsf@anie.imag.fr","subject":"Re: [PATCH v3 4/6] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-28T11:27:05Z","receivedAt":"2014-07-28T11:27:05Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/28/2014 4:52 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> +test_expect_success 'check line errors for malformed values' '\n>> +\tmv .git/config .git/config.old &&\n>> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n>> +\tcat >.git/config <<-\\EOF &&\n>> +\t[alias]\n>> +\t\tbr\n>> +\tEOF\n>> +\ttest_expect_code 128 git br 2>result &&\n>> +\tgrep \"fatal: bad config file line 2 in .git/config\" result\n>> +'\n> \n> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have\n> eased review to group both patches, either\n> \n> PATCH 1: introduce test_expect_failure test to demonstrate the failure\n\nDidn't Junio comment that he wouldn't recommend inserting a test_expect_failure\nfor new tests and then flipping them after in the series.\n\n> PATCH 2: fix the bug and change test_expect_failure to test_expect_success\n> \n> Or putting both in the same patch.\n>\n\nMuch better, thanks for the advice.\n\n> I think the series is OK like this, my comment is just to be read as\n> \"next time, here's how to do better\".\n> \n"},{"id":"246792","messageId":"vpqr415r9vi.fsf@anie.imag.fr","threadId":"37224","inReplyTo":"53D63389.5090205@gmail.com","subject":"Re: [PATCH v3 4/6] add a test for semantic errors in config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-28T11:38:41Z","receivedAt":"2014-07-28T11:38:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> On 7/28/2014 4:52 PM, Matthieu Moy wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> +test_expect_success 'check line errors for malformed values' '\n>>> +\tmv .git/config .git/config.old &&\n>>> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n>>> +\tcat >.git/config <<-\\EOF &&\n>>> +\t[alias]\n>>> +\t\tbr\n>>> +\tEOF\n>>> +\ttest_expect_code 128 git br 2>result &&\n>>> +\tgrep \"fatal: bad config file line 2 in .git/config\" result\n>>> +'\n>> \n>> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have\n>> eased review to group both patches, either\n>> \n>> PATCH 1: introduce test_expect_failure test to demonstrate the failure\n>\n> Didn't Junio comment that he wouldn't recommend inserting a test_expect_failure\n> for new tests and then flipping them after in the series.\n\nNo, he even said it was good practice:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/254101/focus=254104\n\nhis point was to avoid breaking something and repairing in another patch\n(which your initial series was doing because the test patch was coming\nbetween \"rewrite git_config() to use the config-set API\" and \"add line\nnumber and file name info to `config_set`\").\n\nThe situation is different when you have a pre-existing bug.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}