{"thread":{"id":"37264","subject":"[PATCH v6 1/7] config.c: fix accuracy of line number in errors","startedAt":"2014-07-31T15:47:35Z","lastAt":"2014-08-01T09:22:57Z","messageCount":20,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":7},"messages":[{"id":"247055","messageId":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":null,"subject":"[PATCH v6 0/7] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:35Z","receivedAt":"2014-07-31T15:47:35Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[Patch v6]: Added _(....) to error messages.\n\tDiff between v6 and v4 at the bottom.\n\n[PATCH v5]: New patch added (3/7). git_config() now returns void.\n\n[PATCH v4]: One style nit corrected, also added key to error messages.\n\n[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 (7):\n  config.c: fix accuracy of line number in errors\n  add line number and file name info to `config_set`\n  change `git_config()` return value to void\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_const()`\n\n Documentation/technical/api-config.txt |   5 ++\n branch.c                               |   5 +-\n cache.h                                |  27 ++++++-\n config.c                               | 131 +++++++++++++++++++++++++++++----\n t/t1308-config-set.sh                  |  21 ++++++\n t/t4055-diff-context.sh                |   2 +-\n test-config.c                          |  10 +++\n 7 files changed, 181 insertions(+), 20 deletions(-)\n\n-- \n1.9.0.GIT\n\n-- 8< --\ndiff --git a/branch.c b/branch.c\nindex 46e8aa8..735767d 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)\n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n \tcb.config_name = name.buf;\n \tcb.value = NULL;\n-\tif (git_config(read_branch_desc_cb, &cb) < 0) {\n-\t\tstrbuf_release(&name);\n-\t\treturn -1;\n-\t}\n+\tgit_config(read_branch_desc_cb, &cb);\n \tif (cb.value)\n \t\tstrbuf_addstr(buf, cb.value);\n \tstrbuf_release(&name);\ndiff --git a/cache.h b/cache.h\nindex 8512225..243f618 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1295,7 +1295,7 @@ extern int git_config_from_buf(config_fn_t fn, const char *name,\n \t\t\t       const char *buf, size_t len, void *data);\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n-extern int git_config(config_fn_t fn, void *);\n+extern void git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n \t\t\t\t   struct git_config_source *config_source,\n \t\t\t\t   int respect_includes);\ndiff --git a/config.c b/config.c\nindex f9bf60a..15fcd91 100644\n--- a/config.c\n+++ b/config.c\n@@ -1229,12 +1229,24 @@ struct key_value_info {\n \tint linenr;\n };\n \n-static int git_config_raw(config_fn_t fn, void *data)\n+static void git_config_raw(config_fn_t fn, void *data)\n {\n-\treturn git_config_with_options(fn, data, NULL, 1);\n+\tif (git_config_with_options(fn, data, NULL, 1) < 0)\n+\t\t/*\n+\t\t * git_config_with_options() normally returns only\n+\t\t * positive values, as most errors are fatal, and\n+\t\t * non-fatal potential errors are guarded by \"if\"\n+\t\t * statements that are entered only when no error is\n+\t\t * possible.\n+\t\t *\n+\t\t * If we ever encounter a non-fatal error, it means\n+\t\t * something went really wrong and we should stop\n+\t\t * immediately.\n+\t\t */\n+\t\tdie(_(\"unknown error occured while reading the configuration files\"));\n }\n \n-static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n+static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n {\n \tint i, value_index;\n \tstruct string_list *values;\n@@ -1249,23 +1261,22 @@ static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\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 '%s' from command-line config\", entry->key);\n+\t\t\t\tdie(_(\"unable to parse '%s' from command-line config\"), entry->key);\n \t\t\telse\n-\t\t\t\tdie(\"bad config variable '%s' at file line %d in %s\",\n+\t\t\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n \t\t\t\t\tentry->key,\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+void git_config(config_fn_t fn, void *data)\n {\n \tgit_config_check_init();\n-\treturn configset_iter(&the_config_set, fn, data);\n+\tconfigset_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@@ -1565,9 +1576,9 @@ void git_die_config(const char *key)\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 '%s' from command-line config\", key);\n+\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n \telse\n-\t\tdie(\"bad config variable '%s' at file line %d in %s\",\n+\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n \t\t\tkey,\n \t\t\tkv_info->linenr,\n \t\t\tkv_info->filename);\n-- 8< --\n"},{"id":"247054","messageId":"1406821662-1570-2-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 1/7] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:36Z","receivedAt":"2014-07-31T15:47:36Z","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 a191328..ed5fc8e 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":"247058","messageId":"1406821662-1570-3-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 2/7] add line number and file name info to `config_set`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:37Z","receivedAt":"2014-07-31T15:47:37Z","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 ed5fc8e..4a15383 100644\n--- a/config.c\n+++ b/config.c\n@@ -1230,6 +1230,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@@ -1260,6 +1265,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@@ -1272,7 +1280,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@@ -1299,7 +1316,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":"247059","messageId":"1406821662-1570-4-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 3/7] change `git_config()` return value to void","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:38Z","receivedAt":"2014-07-31T15:47:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Currently `git_config()` returns an integer signifying an error code.\nDuring rewrites of the function most of the code was shifted to\n`git_config_with_options()`. `git_config_with_options()` normally\nreturns positive values if its `config_source` parameter is set as NULL,\nas most errors are fatal, and non-fatal potential errors are guarded\nby \"if\" statements that are entered only when no error is possible.\n\nStill a negative value can be returned in case of race condition between\n`access_or_die()` & `git_config_from_file()`. Also, all callers of\n`git_config()` ignore the return value except for one case in branch.c.\n\nChange `git_config()` return value to void and make it die if it receives\na negative value from `git_config_with_options()`.\n\nOriginal-patch-by: Matthieu Moy <Matthieu.Moy@imag.fr>\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n branch.c |  5 +----\n cache.h  |  2 +-\n config.c | 16 ++++++++++++++--\n 3 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 46e8aa8..735767d 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)\n \tstrbuf_addf(&name, \"branch.%s.description\", branch_name);\n \tcb.config_name = name.buf;\n \tcb.value = NULL;\n-\tif (git_config(read_branch_desc_cb, &cb) < 0) {\n-\t\tstrbuf_release(&name);\n-\t\treturn -1;\n-\t}\n+\tgit_config(read_branch_desc_cb, &cb);\n \tif (cb.value)\n \t\tstrbuf_addstr(buf, cb.value);\n \tstrbuf_release(&name);\ndiff --git a/cache.h b/cache.h\nindex 7292aef..c61919d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char *name,\n \t\t\t       const char *buf, size_t len, void *data);\n extern void git_config_push_parameter(const char *text);\n extern int git_config_from_parameters(config_fn_t fn, void *data);\n-extern int git_config(config_fn_t fn, void *);\n+extern void git_config(config_fn_t fn, void *);\n extern int git_config_with_options(config_fn_t fn, void *,\n \t\t\t\t   struct git_config_source *config_source,\n \t\t\t\t   int respect_includes);\ndiff --git a/config.c b/config.c\nindex 4a15383..ecbe14f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1235,9 +1235,21 @@ struct key_value_info {\n \tint linenr;\n };\n \n-int git_config(config_fn_t fn, void *data)\n+void git_config(config_fn_t fn, void *data)\n {\n-\treturn git_config_with_options(fn, data, NULL, 1);\n+\tif (git_config_with_options(fn, data, NULL, 1) < 0)\n+\t\t/*\n+\t\t * git_config_with_options() normally returns only\n+\t\t * positive values, as most errors are fatal, and\n+\t\t * non-fatal potential errors are guarded by \"if\"\n+\t\t * statements that are entered only when no error is\n+\t\t * possible.\n+\t\t *\n+\t\t * If we ever encounter a non-fatal error, it means\n+\t\t * something went really wrong and we should stop\n+\t\t * immediately.\n+\t\t */\n+\t\tdie(_(\"unknown error occured while reading the configuration files\"));\n }\n \n static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)\n-- \n1.9.0.GIT\n"},{"id":"247056","messageId":"1406821662-1570-5-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 4/7] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:39Z","receivedAt":"2014-07-31T15:47:39Z","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 t/t4055-diff-context.sh |  2 +-\n 3 files changed, 74 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c61919d..4d3c6bd 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 ecbe14f..6b4dd65 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@@ -1235,7 +1229,7 @@ struct key_value_info {\n \tint linenr;\n };\n \n-void git_config(config_fn_t fn, void *data)\n+static void git_config_raw(config_fn_t fn, void *data)\n {\n \tif (git_config_with_options(fn, data, NULL, 1) < 0)\n \t\t/*\n@@ -1252,6 +1246,39 @@ void git_config(config_fn_t fn, void *data)\n \t\tdie(_(\"unknown error occured while reading the configuration files\"));\n }\n \n+static void 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 '%s' from command-line config\"), entry->key);\n+\t\t\telse\n+\t\t\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n+\t\t\t\t\tentry->key,\n+\t\t\t\t\tkv_info->linenr,\n+\t\t\t\t\tkv_info->filename);\n+\t\t}\n+\t}\n+}\n+\n+static void git_config_check_init(void);\n+\n+void git_config(config_fn_t fn, void *data)\n+{\n+\tgit_config_check_init();\n+\tconfigset_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@@ -1278,6 +1305,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@@ -1293,6 +1321,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@@ -1316,6 +1350,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@@ -1332,6 +1369,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@@ -1450,7 +1491,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)\ndiff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh\nindex cd04543..741e080 100755\n--- a/t/t4055-diff-context.sh\n+++ b/t/t4055-diff-context.sh\n@@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' '\n test_expect_success 'negative integer config parsing' '\n \tgit config diff.context -1 &&\n \ttest_must_fail git diff 2>output &&\n-\ttest_i18ngrep \"bad config file\" output\n+\ttest_i18ngrep \"bad config variable\" output\n '\n \n test_expect_success '-U0 is valid, so is diff.context=0' '\n-- \n1.9.0.GIT\n"},{"id":"247057","messageId":"1406821662-1570-6-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 5/7] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:40Z","receivedAt":"2014-07-31T15:47:40Z","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..e2f9d0b 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 variable '\\''alias.br'\\'' at file line 2 in .git/config\" result\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"247060","messageId":"1406821662-1570-7-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:41Z","receivedAt":"2014-07-31T15:47:41Z","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---\n Documentation/technical/api-config.txt |  5 +++++\n cache.h                                |  1 +\n config.c                               | 27 +++++++++++++++++++++++++--\n 3 files changed, 31 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 4d3c6bd..243f618 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 6b4dd65..15fcd91 100644\n--- a/config.c\n+++ b/config.c\n@@ -1515,8 +1515,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@@ -1557,10 +1561,29 @@ 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 '%s' from command-line config\"), key);\n+\telse\n+\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n+\t\t\tkey,\n+\t\t\tkv_info->linenr,\n+\t\t\tkv_info->filename);\n+ }\n+\n /*\n  * Find all the stuff for git_config_set() below.\n  */\n-- \n1.9.0.GIT\n"},{"id":"247061","messageId":"1406821662-1570-8-git-send-email-tanayabh@gmail.com","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v6 7/7] add tests for `git_config_get_string_const()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T15:47:42Z","receivedAt":"2014-07-31T15:47:42Z","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 | 10 ++++++++++\n test-config.c         | 10 ++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex e2f9d0b..f012dd6 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -119,6 +119,16 @@ 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+\tcheck_config expect_code 1 get_string case.ba \"Value not found for \\\"case.ba\\\"\"\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 variable '\\''case.foo'\\'' at 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":"247062","messageId":"vpqr411h69h.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"1406821662-1570-7-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-31T15:55:54Z","receivedAt":"2014-07-31T15:55:54Z","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 '%s' from command-line config\"), key);\n> +\telse\n> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n> +\t\t\tkey,\n> +\t\t\tkv_info->linenr,\n> +\t\t\tkv_info->filename);\n> + }\n\nExtra whitespace before }.\n\nAlso, didn't we agree that it was a good thing to factor this\nif/then/else into a helper function?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247063","messageId":"vpqlhr9h67r.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"1406821662-1570-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v6 0/7] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-31T15:56:56Z","receivedAt":"2014-07-31T15:56:56Z","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 v6]: Added _(....) to error messages.\n> \tDiff between v6 and v4 at the bottom.\n\nExcept for the tiny style issue in PATCH 6, the series is now\n\nReviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247064","messageId":"53DA7281.8060403@gmail.com","threadId":"37264","inReplyTo":"vpqr411h69h.fsf@anie.imag.fr","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T16:44:49Z","receivedAt":"2014-07-31T16:44:49Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/31/2014 9:25 PM, Matthieu Moy wrote:\n> 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 '%s' from command-line config\"), key);\n>> +\telse\n>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>> +\t\t\tkey,\n>> +\t\t\tkv_info->linenr,\n>> +\t\t\tkv_info->filename);\n>> + }\n> \n> Extra whitespace before }.\n> \n> Also, didn't we agree that it was a good thing to factor this\n> if/then/else into a helper function?\n> \n\nI have been thinking about it, wouldn't it be better to give users\na function like,\n\ngit_config_die_exact(key, value);\n\nwhere user supplies key & value both and it would print the correct message based\non that. git_die_config() will be a wrapper over it, supplying the last value.\nIt will also replace the redundant code in git_config_raw() and help in the\nmulti value case. If the idea sounds okay, I will send a reroll.\n"},{"id":"247066","messageId":"vpqr411bhd5.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"53DA7281.8060403@gmail.com","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-31T16:52:38Z","receivedAt":"2014-07-31T16:52:38Z","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/31/2014 9:25 PM, Matthieu Moy wrote:\n>> 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 '%s' from command-line config\"), key);\n>>> +\telse\n>>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>>> +\t\t\tkey,\n>>> +\t\t\tkv_info->linenr,\n>>> +\t\t\tkv_info->filename);\n>>> + }\n>> \n>> Extra whitespace before }.\n>> \n>> Also, didn't we agree that it was a good thing to factor this\n>> if/then/else into a helper function?\n>> \n>\n> I have been thinking about it, wouldn't it be better to give users\n> a function like,\n>\n> git_config_die_exact(key, value);\n>\n> where user supplies key & value both and it would print the correct message based\n> on that.\n\nI suggested git_config_die_linenr(key, linenr), and I now realize it\nshould take the value too.\n\nYou're suggesting git_config_die_exact(key, value). Is it a typo that\nyou forgot the line number, or is it intentional? If intentional, I\ndon't think it solves your issue:\n\n[section]\n   key\n   key\n\nThere are two errors in this file, and you need to provide a line\nnumber. key and value alone do not allow you to know which line the\nerror is. You can use a convention like \"complain on the first value\nequal to the argument\", but I'm not sure that would always work. And\nthat seems backward to me to reconstruct the line number since the\nfunction can be called from places where the line number is already\nknown (while iterating over the string_list for example).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247069","messageId":"53DA7C23.3090603@gmail.com","threadId":"37264","inReplyTo":"vpqr411bhd5.fsf@anie.imag.fr","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T17:25:55Z","receivedAt":"2014-07-31T17:25:55Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/31/2014 10:22 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:\n>>> 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 '%s' from command-line config\"), key);\n>>>> +\telse\n>>>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>>>> +\t\t\tkey,\n>>>> +\t\t\tkv_info->linenr,\n>>>> +\t\t\tkv_info->filename);\n>>>> + }\n>>>\n>>> Extra whitespace before }.\n>>>\n>>> Also, didn't we agree that it was a good thing to factor this\n>>> if/then/else into a helper function?\n>>>\n>>\n>> I have been thinking about it, wouldn't it be better to give users\n>> a function like,\n>>\n>> git_config_die_exact(key, value);\n>>\n>> where user supplies key & value both and it would print the correct message based\n>> on that.\n> \n> I suggested git_config_die_linenr(key, linenr), and I now realize it\n> should take the value too.\n> \n> You're suggesting git_config_die_exact(key, value). Is it a typo that\n> you forgot the line number, or is it intentional? If intentional, I\n> don't think it solves your issue:\n> \n> [section]\n>    key\n>    key\n>\n> There are two errors in this file, and you need to provide a line\n> number. key and value alone do not allow you to know which line the\n> error is. You can use a convention like \"complain on the first value\n> equal to the argument\", but I'm not sure that would always work. And\n> that seems backward to me to reconstruct the line number since the\n> function can be called from places where the line number is already\n> known (while iterating over the string_list for example).\n\nStill the user would have to know that the linenr info is there.\nFirst hear my argument, then we can go either way.\n\nLet's first see the previous code behavior, git_config() would die on the\nfirst corrupt value, we wouldn't live to see the future value.\n\nfor example,\n\n[section]\n\tkey\t// error(old git_config() would die here)\n\tkey = good_value\n\n[section]\n\tkey\t//again error\n\nNow for the new behavior,\n\nsingle valued callers use git_config_get_value() which will directly\nsupply the last value, so we don't see the first error value.\nFor such cases, git_die_config(key) is enough.\n\nThe new git_config() works exactly as the old code, it would die\non the first case of erroneous value. Here, git_die_config_exact(key, value)\nwould be enough.\n\nThe last case is git_config_get_value_multi(), here we iterate over the keys,\nand then call git_die_config_exact(key, value) for the erroneous value.\n(pros and cons: abstracts the error message implementation from the user\nbut there is an extra call to git_config_get_value_multi(), but its cheap and\nwe are dying anyway)\n\n+\tif (load_config_refs) {\n+\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n+\t\tif (values) {\n+\t\t\tfor (i = 0; i < values->nr; i++) {\n+\t\t\t\tif (!values->items[i].string) {\n+\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n+\t\t\t\t\tgit_die_config_exact(\"notes.displayref\", values->items[i].string);\n+\t\t\t\t}\n+\t\t\t\telse\n+\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n+\t\t\t\t\t\t\t\t     values->items[i].string);\n+\t\t\t}\n+\t\t}\n+\t}\n\nWhat do you think?\n"},{"id":"247070","messageId":"vpqa97p8koq.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"53DA7C23.3090603@gmail.com","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-31T18:09:09Z","receivedAt":"2014-07-31T18:09:09Z","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/31/2014 10:22 PM, Matthieu Moy wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:\n>>>> 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 '%s' from command-line config\"), key);\n>>>>> +\telse\n>>>>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>>>>> +\t\t\tkey,\n>>>>> +\t\t\tkv_info->linenr,\n>>>>> +\t\t\tkv_info->filename);\n>>>>> + }\n>>>>\n>>>> Extra whitespace before }.\n>>>>\n>>>> Also, didn't we agree that it was a good thing to factor this\n>>>> if/then/else into a helper function?\n>>>>\n>>>\n>>> I have been thinking about it, wouldn't it be better to give users\n>>> a function like,\n>>>\n>>> git_config_die_exact(key, value);\n>>>\n>>> where user supplies key & value both and it would print the correct message based\n>>> on that.\n>> \n>> I suggested git_config_die_linenr(key, linenr), and I now realize it\n>> should take the value too.\n>> \n>> You're suggesting git_config_die_exact(key, value). Is it a typo that\n>> you forgot the line number, or is it intentional? If intentional, I\n>> don't think it solves your issue:\n>> \n>> [section]\n>>    key\n>>    key\n>>\n>> There are two errors in this file, and you need to provide a line\n>> number. key and value alone do not allow you to know which line the\n>> error is. You can use a convention like \"complain on the first value\n>> equal to the argument\", but I'm not sure that would always work. And\n>> that seems backward to me to reconstruct the line number since the\n>> function can be called from places where the line number is already\n>> known (while iterating over the string_list for example).\n>\n> Still the user would have to know that the linenr info is there.\n> First hear my argument, then we can go either way.\n>\n> Let's first see the previous code behavior, git_config() would die on the\n> first corrupt value, we wouldn't live to see the future value.\n>\n> for example,\n>\n> [section]\n> \tkey\t// error(old git_config() would die here)\n> \tkey = good_value\n>\n> [section]\n> \tkey\t//again error\n>\n> Now for the new behavior,\n>\n> single valued callers use git_config_get_value() which will directly\n> supply the last value, so we don't see the first error value.\n> For such cases, git_die_config(key) is enough.\n\nYes. And it is better than the old behavior which was dying on the\nerroneous value without giving a chance to the user to override the\nboggus value in a more specific config file (e.g. if your sysadmin\nmessed-up /etc/gitconfig).\n\nBut since git_die_config(key) is simpler to use for the caller, it's\nindependant from git_die_config_exact()'s parameters.\n\n> The new git_config() works exactly as the old code, it would die\n> on the first case of erroneous value. Here, git_die_config_exact(key, value)\n> would be enough.\n\nYes. But this callsite has the line number information, so it could use\nit.\n\n> The last case is git_config_get_value_multi(), here we iterate over the keys,\n> and then call git_die_config_exact(key, value) for the erroneous value.\n> (pros and cons: abstracts the error message implementation from the user\n> but there is an extra call to git_config_get_value_multi(), but its cheap and\n> we are dying anyway)\n\nThis is the part I find weird. You're calling git_die_config_exact() on\nthe first boggus value, and git_die_config_exact() will notify an error\nat the line of the last boggus value.\n\nI agree it works (if we give only one error message, it can be the first\nor the last, the user doesn't really care), but I find the\nimplementation backwards. You have the line number already, as you are\niterating over the string_list which contain it. So why forget the line\nnumber, and recompute one, possibly different, right after?\n\nSo, I see only cases where you already have the line number, hence no\nreason to recompute it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247071","messageId":"53DA8A54.6060208@gmail.com","threadId":"37264","inReplyTo":"vpqa97p8koq.fsf@anie.imag.fr","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-31T18:26:28Z","receivedAt":"2014-07-31T18:26:28Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/31/2014 11:39 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> On 7/31/2014 10:22 PM, Matthieu Moy wrote:\n>>> Tanay Abhra <tanayabh@gmail.com> writes:\n>>>\n>>>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:\n>>>>> 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 '%s' from command-line config\"), key);\n>>>>>> +\telse\n>>>>>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>>>>>> +\t\t\tkey,\n>>>>>> +\t\t\tkv_info->linenr,\n>>>>>> +\t\t\tkv_info->filename);\n>>>>>> + }\n>>>>>\n>>>>> Extra whitespace before }.\n>>>>>\n>>>>> Also, didn't we agree that it was a good thing to factor this\n>>>>> if/then/else into a helper function?\n>>>>>\n>>>>\n>>>> I have been thinking about it, wouldn't it be better to give users\n>>>> a function like,\n>>>>\n>>>> git_config_die_exact(key, value);\n>>>>\n>>>> where user supplies key & value both and it would print the correct message based\n>>>> on that.\n>>>\n>>> I suggested git_config_die_linenr(key, linenr), and I now realize it\n>>> should take the value too.\n>>>\n>>> You're suggesting git_config_die_exact(key, value). Is it a typo that\n>>> you forgot the line number, or is it intentional? If intentional, I\n>>> don't think it solves your issue:\n>>>\n>>> [section]\n>>>    key\n>>>    key\n>>>\n>>> There are two errors in this file, and you need to provide a line\n>>> number. key and value alone do not allow you to know which line the\n>>> error is. You can use a convention like \"complain on the first value\n>>> equal to the argument\", but I'm not sure that would always work. And\n>>> that seems backward to me to reconstruct the line number since the\n>>> function can be called from places where the line number is already\n>>> known (while iterating over the string_list for example).\n>>\n>> Still the user would have to know that the linenr info is there.\n>> First hear my argument, then we can go either way.\n>>\n>> Let's first see the previous code behavior, git_config() would die on the\n>> first corrupt value, we wouldn't live to see the future value.\n>>\n>> for example,\n>>\n>> [section]\n>> \tkey\t// error(old git_config() would die here)\n>> \tkey = good_value\n>>\n>> [section]\n>> \tkey\t//again error\n>>\n>> Now for the new behavior,\n>>\n>> single valued callers use git_config_get_value() which will directly\n>> supply the last value, so we don't see the first error value.\n>> For such cases, git_die_config(key) is enough.\n> \n> Yes. And it is better than the old behavior which was dying on the\n> erroneous value without giving a chance to the user to override the\n> boggus value in a more specific config file (e.g. if your sysadmin\n> messed-up /etc/gitconfig).\n> \n> But since git_die_config(key) is simpler to use for the caller, it's\n> independant from git_die_config_exact()'s parameters.\n> \n>> The new git_config() works exactly as the old code, it would die\n>> on the first case of erroneous value. Here, git_die_config_exact(key, value)\n>> would be enough.\n> \n> Yes. But this callsite has the line number information, so it could use\n> it.\n> \n>> The last case is git_config_get_value_multi(), here we iterate over the keys,\n>> and then call git_die_config_exact(key, value) for the erroneous value.\n>> (pros and cons: abstracts the error message implementation from the user\n>> but there is an extra call to git_config_get_value_multi(), but its cheap and\n>> we are dying anyway)\n> \n> This is the part I find weird. You're calling git_die_config_exact() on\n> the first boggus value, and git_die_config_exact() will notify an error\n> at the line of the last boggus value.\n>\n\nHmn, we may have some confusion here. I meant the implementation of\ngit_config_exact() to look like this,\n\nvoid git_die_config_exact(const char *key, const char *value)\n{\n\tint i;\n\tconst struct string_list *values;\n\tstruct key_value_info *kv_info;\n\tvalues = git_config_get_value_multi(key);\n\tfor (i = 0; i < values.nr; i++) {\n\t\tkv_info = values->items[i].util;\n\t\t/* A null check will be here also */\n\t\tif (!strcmp(value, values->items[i].string)) {\n\t\t\tif (!kv_info->linenr)\n\t\t\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n\t\t\telse\n\t\t\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n\t\t\t\t\tkey,\n\t\t\t\t\tkv_info->linenr,\n\t\t\t\t\tkv_info->filename);\n\t\t}\n\t}\n}\n\nThe above code would print the info on first bogus value.\nI am only rooting for it because the caller has to think very little to use it.\nIt's your call, I am open to both ideas.\n\n> I agree it works (if we give only one error message, it can be the first\n> or the last, the user doesn't really care), but I find the\n> implementation backwards. You have the line number already, as you are\n> iterating over the string_list which contain it. So why forget the line\n> number, and recompute one, possibly different, right after?\n> \n> So, I see only cases where you already have the line number, hence no\n> reason to recompute it.\n> \n"},{"id":"247072","messageId":"vpqoaw55q16.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"53DA8A54.6060208@gmail.com","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-31T18:41:57Z","receivedAt":"2014-07-31T18:41:57Z","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/31/2014 11:39 PM, Matthieu Moy wrote:\n>\n>> This is the part I find weird. You're calling git_die_config_exact() on\n>> the first boggus value, and git_die_config_exact() will notify an error\n>> at the line of the last boggus value.\n>\n> Hmn, we may have some confusion here. I meant the implementation of\n> git_config_exact() to look like this,\n>\n> void git_die_config_exact(const char *key, const char *value)\n> {\n> \tint i;\n> \tconst struct string_list *values;\n> \tstruct key_value_info *kv_info;\n> \tvalues = git_config_get_value_multi(key);\n> \tfor (i = 0; i < values.nr; i++) {\n> \t\tkv_info = values->items[i].util;\n> \t\t/* A null check will be here also */\n> \t\tif (!strcmp(value, values->items[i].string)) {\n> \t\t\tif (!kv_info->linenr)\n> \t\t\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n> \t\t\telse\n> \t\t\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n> \t\t\t\t\tkey,\n> \t\t\t\t\tkv_info->linenr,\n> \t\t\t\t\tkv_info->filename);\n> \t\t}\n> \t}\n> }\n>\n> The above code would print the info on first bogus value.\n\nOK, I got confused because git_die_config() errors out at the first\nerror. So, your version works, but I do not see any added value in this\nextra complexity.\n\nTo be cleary, my version would be\n\nNORETURN static /* not sure about the \"static\" */\nvoid git_die_config_linenr(const char *key,\n                           const char *filename, int linenr)\n{\n\tif (!linenr)\n\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n\telse\n\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n\t\t\tkey,\n\t\t\tlinenr,\n\t\t\tfilename);\n}\n\n(hmm, I actually do not need the value, it's not printed)\n\nNORETURN\nvoid 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\tgit_die_config_linenr(key, kv_info->linenr, kv_info->filename);\n}\n\n(we forgot the NORETURN in previous version BTW. It should be there in\nboth functions here and in the .h file)\n\nIn my version, git_die_config uses git_die_config_linenr, and there's no\nissue with first/last boggus value. Callers of git_die_config_linenr\nhave all the information to call it directly.\n\nThere are 3 code path that leads to the final die() calls, and having\nthis little helper allows making sure the messages are the same for\nevery callers, by construction (as opposed to cut-and-pasting).\n\nReally, I don't see any reason to do anything more complex.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247076","messageId":"xmqqtx5xw7n7.fsf@gitster.dls.corp.google.com","threadId":"37264","inReplyTo":"vpqoaw55q16.fsf@anie.imag.fr","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-31T21:17:00Z","receivedAt":"2014-07-31T21:17:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>\n>> On 7/31/2014 11:39 PM, Matthieu Moy wrote:\n>>\n>>> This is the part I find weird. You're calling git_die_config_exact() on\n>>> the first boggus value, and git_die_config_exact() will notify an error\n>>> at the line of the last boggus value.\n>>\n>> Hmn, we may have some confusion here. I meant the implementation of\n>> git_config_exact() to look like this,\n>>\n>> void git_die_config_exact(const char *key, const char *value)\n>> {\n>> \tint i;\n>> \tconst struct string_list *values;\n>> \tstruct key_value_info *kv_info;\n>> \tvalues = git_config_get_value_multi(key);\n>> \tfor (i = 0; i < values.nr; i++) {\n>> \t\tkv_info = values->items[i].util;\n>> \t\t/* A null check will be here also */\n>> \t\tif (!strcmp(value, values->items[i].string)) {\n>> \t\t\tif (!kv_info->linenr)\n>> \t\t\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n>> \t\t\telse\n>> \t\t\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>> \t\t\t\t\tkey,\n>> \t\t\t\t\tkv_info->linenr,\n>> \t\t\t\t\tkv_info->filename);\n>> \t\t}\n>> \t}\n>> }\n>>\n>> The above code would print the info on first bogus value.\n>\n> OK, I got confused because git_die_config() errors out at the first\n> error. So, your version works, but I do not see any added value in this\n> extra complexity.\n\nHmm, I am still confused ;-)\n\nCan there be more than one 'i' whose value->items[i].string is the\nsame as the given value?  IOW, if you have \"[user] name<EOL>\" in\nboth .git/config and ~/.gitconfig, don't we want to make sure that\nwe complain on the one in .git/config, which would have taken\nprecedence if it were spelled correctly?\n\nYour version below makes it very clear that you only complain on the\nlast one, no matter how many identical copies of the value for the\ngiven key are defined incorrectly the same way, which is easier to\nunderstand what is going on.\n\n> To be cleary, my version would be\n>\n> NORETURN static /* not sure about the \"static\" */\n> void git_die_config_linenr(const char *key,\n>                            const char *filename, int linenr)\n> {\n> \tif (!linenr)\n> \t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n> \telse\n> \t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n> \t\t\tkey,\n> \t\t\tlinenr,\n> \t\t\tfilename);\n> }\n>\n> (hmm, I actually do not need the value, it's not printed)\n>\n> NORETURN\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> \tgit_die_config_linenr(key, kv_info->linenr, kv_info->filename);\n> }\n>\n> (we forgot the NORETURN in previous version BTW. It should be there in\n> both functions here and in the .h file)\n>\n> In my version, git_die_config uses git_die_config_linenr, and there's no\n> issue with first/last boggus value. Callers of git_die_config_linenr\n> have all the information to call it directly.\n>\n> There are 3 code path that leads to the final die() calls, and having\n> this little helper allows making sure the messages are the same for\n> every callers, by construction (as opposed to cut-and-pasting).\n>\n> Really, I don't see any reason to do anything more complex.\n"},{"id":"247097","messageId":"vpqfvhg4nix.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"xmqqtx5xw7n7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-01T08:33:42Z","receivedAt":"2014-08-01T08:33:42Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Hmm, I am still confused ;-)\n>\n> Can there be more than one 'i' whose value->items[i].string is the\n> same as the given value?  IOW, if you have \"[user] name<EOL>\" in\n> both .git/config and ~/.gitconfig, don't we want to make sure that\n> we complain on the one in .git/config, which would have taken\n> precedence if it were spelled correctly?\n\nFor single-valued variables, yes. And for them, git_die_config(), as\ncurrently implemented does the right thing. My proposed change does not\nmodify the behavior, it just extracts error messages and an if/then/else\ninto a helper that can be used elsewhere.\n\nFor multi-valued variables, it's different. If you have a boggus value,\nfurther values will not override it. It will remain bogus.\n\nThe code consuming such multi-valued variable has to iterate over the\nstring_list, and raise an error on bogus values. Ideally, it could show\nall the errors, but it's also reasonable to die() at the first one. The\nsimplest behavior to implement is to die() at the first error.\n\n(currently, git _segfaults_ at the first NULL value for\nnotes.displayref, so it can't be much worse)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247098","messageId":"53DB582F.8040501@gmail.com","threadId":"37264","inReplyTo":"vpqoaw55q16.fsf@anie.imag.fr","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T09:04:47Z","receivedAt":"2014-08-01T09:04:47Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 8/1/2014 12:11 AM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> On 7/31/2014 11:39 PM, Matthieu Moy wrote:\n>>\n>>> This is the part I find weird. You're calling git_die_config_exact() on\n>>> the first boggus value, and git_die_config_exact() will notify an error\n>>> at the line of the last boggus value.\n>>\n>> Hmn, we may have some confusion here. I meant the implementation of\n>> git_config_exact() to look like this,\n>>\n>> void git_die_config_exact(const char *key, const char *value)\n>> {\n>> \tint i;\n>> \tconst struct string_list *values;\n>> \tstruct key_value_info *kv_info;\n>> \tvalues = git_config_get_value_multi(key);\n>> \tfor (i = 0; i < values.nr; i++) {\n>> \t\tkv_info = values->items[i].util;\n>> \t\t/* A null check will be here also */\n>> \t\tif (!strcmp(value, values->items[i].string)) {\n>> \t\t\tif (!kv_info->linenr)\n>> \t\t\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n>> \t\t\telse\n>> \t\t\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>> \t\t\t\t\tkey,\n>> \t\t\t\t\tkv_info->linenr,\n>> \t\t\t\t\tkv_info->filename);\n>> \t\t}\n>> \t}\n>> }\n>>\n>> The above code would print the info on first bogus value.\n> \n> OK, I got confused because git_die_config() errors out at the first\n> error. So, your version works, but I do not see any added value in this\n> extra complexity.\n> \n> To be cleary, my version would be\n> \n> NORETURN static /* not sure about the \"static\" */\n> void git_die_config_linenr(const char *key,\n>                            const char *filename, int linenr)\n> {\n> \tif (!linenr)\n> \t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n> \telse\n> \t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n> \t\t\tkey,\n> \t\t\tlinenr,\n> \t\t\tfilename);\n> }\n> \n> (hmm, I actually do not need the value, it's not printed)\n> \n> NORETURN\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> \tgit_die_config_linenr(key, kv_info->linenr, kv_info->filename);\n> }\n> \n> (we forgot the NORETURN in previous version BTW. It should be there in\n> both functions here and in the .h file)\n> \n> In my version, git_die_config uses git_die_config_linenr, and there's no\n> issue with first/last boggus value. Callers of git_die_config_linenr\n> have all the information to call it directly.\n> \n> There are 3 code path that leads to the final die() calls, and having\n> this little helper allows making sure the messages are the same for\n> every callers, by construction (as opposed to cut-and-pasting).\n> \n> Really, I don't see any reason to do anything more complex.\n>\n\nThanks, I am sending your version with the reroll.\nAlso, for clarity the multi value use case would look like,\n\n\tstruct key_value_info *kv_info;\n\tif (load_config_refs) {\n\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n\t\tif (values) {\n\t\t\tfor (i = 0; i < values->nr; i++) {\n\t\t\t\tif (!values->items[i].string) {\n\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n\t\t\t\t\tkv_info = values->items[i].util;\n\t\t\t\t\tgit_die_config_linenr(\"notes.displayref\",\n\t\t\t\t\t\t\t      kv_info->filename,\n\t\t\t\t\t\t\t      kv_info->linenr);\n\t\t\t\t}\n\t\t\t\telse\n\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n\t\t\t\t\t\t\t\t     values->items[i].string);\n\t\t\t}\n\t\t}\n\t}\n\nwith my function it would have looked like,\n\n\tif (load_config_refs) {\n\t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n\t\tif (values) {\n\t\t\tfor (i = 0; i < values->nr; i++) {\n\t\t\t\tif (!values->items[i].string) {\n\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n\t\t\t\t\tgit_die_config_exact(\"notes.displayref\", values->items[i].string);\n\t\t\t\t}\n\t\t\t\telse\n\t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n\t\t\t\t\t\t\t\t     values->items[i].string);\n\t\t\t}\n\t\t}\n\t}\n\nThanks.\n"},{"id":"247099","messageId":"vpqlhr836oe.fsf@anie.imag.fr","threadId":"37264","inReplyTo":"53DB582F.8040501@gmail.com","subject":"Re: [PATCH v6 6/7] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-01T09:22:57Z","receivedAt":"2014-08-01T09:22:57Z","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> Thanks, I am sending your version with the reroll.\n> Also, for clarity the multi value use case would look like,\n>\n> \tstruct key_value_info *kv_info;\n> \tif (load_config_refs) {\n> \t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n> \t\tif (values) {\n> \t\t\tfor (i = 0; i < values->nr; i++) {\n> \t\t\t\tif (!values->items[i].string) {\n> \t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n> \t\t\t\t\tkv_info = values->items[i].util;\n> \t\t\t\t\tgit_die_config_linenr(\"notes.displayref\",\n> \t\t\t\t\t\t\t      kv_info->filename,\n> \t\t\t\t\t\t\t      kv_info->linenr);\n> \t\t\t\t}\n> \t\t\t\telse\n> \t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n> \t\t\t\t\t\t\t\t     values->items[i].string);\n> \t\t\t}\n> \t\t}\n> \t}\n>\n> with my function it would have looked like,\n>\n> \tif (load_config_refs) {\n> \t\tvalues = git_config_get_value_multi(\"notes.displayref\");\n> \t\tif (values) {\n> \t\t\tfor (i = 0; i < values->nr; i++) {\n> \t\t\t\tif (!values->items[i].string) {\n> \t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n> \t\t\t\t\tgit_die_config_exact(\"notes.displayref\", values->items[i].string);\n> \t\t\t\t}\n> \t\t\t\telse\n> \t\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n> \t\t\t\t\t\t\t\t     values->items[i].string);\n> \t\t\t}\n> \t\t}\n> \t}\n\nI still think that checking for non-null values should be done in a\nhelper in config.c, and then the code would look like\n\n \tif (load_config_refs) {\n \t\tvalues = git_config_get_value_multi_nonbool(\"notes.displayref\");\n \t\tif (values)\n \t\t\tfor (i = 0; i < values->nr; i++)\n\t\t\t\tstring_list_add_refs_by_glob(&display_notes_refs,\n\t\t\t\t\t\t\t     values->items[i].string);\n \t}\n\nThe same helper could at least be used for \"branch.<remote>.merge\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}