{"thread":{"id":"37297","subject":"[PATCH v8 3/8] add line number and file name info to `config_set`","startedAt":"2014-08-06T14:53:04Z","lastAt":"2014-08-11T09:59:13Z","messageCount":22,"participants":["Tanay Abhra","Matthieu Moy","Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":8,"patchTotal":8},"messages":[{"id":"247321","messageId":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":null,"subject":"[PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:04Z","receivedAt":"2014-08-06T14:53:04Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[Patch v8]: git_die_config now allows custom error messages.\n\tnew tests are now not too reliant on specific strings. Diff\n\tbetween v7 & v8 is appended at the bottom. Thanks to Junio &\n\tMatthieu for their suggestions.\n\n[Patch v7]: style nit corrected. (1/8) is Matthieu's translation patch.\n\tgit_die_config_linenr() helper function added. Diff between v6\n\tand v7 appended for review.\n\n[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\twhich I think it is now. (added the line number and file name info just for\n\tcompleteness)\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\nMatthieu Moy (1):\n  config.c: mark error and warnings strings for translation\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  config: add `git_die_config()` to the config-set API\n  rewrite git_config() to use the config-set API\n  add a test for semantic errors in config files\n  add tests for `git_config_get_string_const()`\n\n Documentation/technical/api-config.txt |  13 +++\n branch.c                               |   5 +-\n cache.h                                |  34 +++++++-\n config.c                               | 152 +++++++++++++++++++++++++++------\n t/t1308-config-set.sh                  |  21 +++++\n t/t4055-diff-context.sh                |   2 +-\n test-config.c                          |  10 +++\n 7 files changed, 207 insertions(+), 30 deletions(-)\n\n-- \n1.9.0.GIT\n\n-- 8< --\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex d7d0cf9..0d8b99b 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -155,10 +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+`git_die_config(const char *key, const char *err, ...)`::\n \n-\tDies printing the line number and the file name of the highest\n-\tpriority value for the configuration variable `key`.\n+\tFirst prints the error message specified by the caller in `err` and then\n+\tdies printing the line number and the file name of the highest priority\n+\tvalue for the configuration variable `key`.\n \n `void git_die_config_linenr(const char *key, const char *filename, int linenr)`::\n \ndiff --git a/cache.h b/cache.h\nindex ff17889..2693a37 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1406,8 +1406,14 @@ 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-extern void git_die_config_linenr(const char *key, const char *filename, int linenr);\n+\n+struct key_value_info {\n+\tconst char *filename;\n+\tint linenr;\n+};\n+\n+extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\n+extern NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr);\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 cf9124f..427850a 100644\n--- a/config.c\n+++ b/config.c\n@@ -1224,11 +1224,6 @@ 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 static void git_config_raw(config_fn_t fn, void *data)\n {\n \tif (git_config_with_options(fn, data, NULL, 1) < 0)\n@@ -1326,8 +1321,8 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\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\tkv_info->filename = NULL;\n+\t\tkv_info->linenr = -1;\n \t}\n \tsi->util = kv_info;\n \n@@ -1513,7 +1508,7 @@ int git_config_get_string_const(const char *key, const char **dest)\n \tgit_config_check_init();\n \tret = git_configset_get_string_const(&the_config_set, key, dest);\n \tif (ret < 0)\n-\t\tgit_die_config(key);\n+\t\tgit_die_config(key, NULL);\n \treturn ret;\n }\n \n@@ -1559,27 +1554,32 @@ int git_config_get_pathname(const char *key, const char **dest)\n \tgit_config_check_init();\n \tret = git_configset_get_pathname(&the_config_set, key, dest);\n \tif (ret < 0)\n-\t\tgit_die_config(key);\n+\t\tgit_die_config(key, NULL);\n \treturn ret;\n }\n \n NORETURN\n void git_die_config_linenr(const char *key, const char *filename, int linenr)\n {\n-\tif (!linenr)\n+\tif (!filename)\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+\t\tdie(_(\"bad config variable '%s' in file '%s' at line %d\"),\n+\t\t    key, filename, linenr);\n }\n \n-NORETURN\n-void git_die_config(const char *key)\n+NORETURN __attribute__((format(printf, 2, 3)))\n+void git_die_config(const char *key, const char *err, ...)\n {\n \tconst struct string_list *values;\n \tstruct key_value_info *kv_info;\n+\n+\tif (err) {\n+\t\tva_list params;\n+\t\tva_start(params, err);\n+\t\tvreportf(\"error: \", err, params);\n+\t\tva_end(params);\n+\t}\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->filename, kv_info->linenr);\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex f012dd6..ecc757a 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -126,7 +126,7 @@ test_expect_success 'find string value for a key' '\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+\tgrep \"line 7.*.git/config\\|.git/config.*line 7\" result\n '\n \n test_expect_success 'find integer if value is non parse-able' '\n@@ -215,7 +215,7 @@ test_expect_success 'check line errors for malformed values' '\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+\tgrep \"line 2.*.git/config\\|.git/config.*line 2\" result\n '\n \n test_done\n-- 8< --\n"},{"id":"247320","messageId":"1407336792-16962-2-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 1/8] config.c: mark error and warnings strings for translation","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:05Z","receivedAt":"2014-08-06T14:53:05Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"From: Matthieu Moy <Matthieu.Moy@imag.fr>\n\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n config.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex a191328..34940fd 100644\n--- a/config.c\n+++ b/config.c\n@@ -457,9 +457,9 @@ static int git_parse_source(config_fn_t fn, void *data)\n \t\t\tbreak;\n \t}\n \tif (cf->die_on_error)\n-\t\tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n+\t\tdie(_(\"bad config file line %d in %s\"), cf->linenr, cf->name);\n \telse\n-\t\treturn error(\"bad config file line %d in %s\", cf->linenr, cf->name);\n+\t\treturn error(_(\"bad config file line %d in %s\"), cf->linenr, cf->name);\n }\n \n static int parse_unit_factor(const char *end, uintmax_t *val)\n@@ -575,9 +575,9 @@ static void die_bad_number(const char *name, const char *value)\n \t\tvalue = \"\";\n \n \tif (cf && cf->name)\n-\t\tdie(\"bad numeric config value '%s' for '%s' in %s: %s\",\n+\t\tdie(_(\"bad numeric config value '%s' for '%s' in %s: %s\"),\n \t\t    value, name, cf->name, reason);\n-\tdie(\"bad numeric config value '%s' for '%s': %s\", value, name, reason);\n+\tdie(_(\"bad numeric config value '%s' for '%s': %s\"), value, name, reason);\n }\n \n int git_config_int(const char *name, const char *value)\n@@ -662,7 +662,7 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \t\treturn config_error_nonbool(var);\n \t*dest = expand_user_path(value);\n \tif (!*dest)\n-\t\tdie(\"Failed to expand user dir in: '%s'\", value);\n+\t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n \treturn 0;\n }\n \n@@ -740,7 +740,7 @@ static int git_default_core_config(const char *var, const char *value)\n \t\tif (level == -1)\n \t\t\tlevel = Z_DEFAULT_COMPRESSION;\n \t\telse if (level < 0 || level > Z_BEST_COMPRESSION)\n-\t\t\tdie(\"bad zlib compression level %d\", level);\n+\t\t\tdie(_(\"bad zlib compression level %d\"), level);\n \t\tzlib_compression_level = level;\n \t\tzlib_compression_seen = 1;\n \t\treturn 0;\n@@ -751,7 +751,7 @@ static int git_default_core_config(const char *var, const char *value)\n \t\tif (level == -1)\n \t\t\tlevel = Z_DEFAULT_COMPRESSION;\n \t\telse if (level < 0 || level > Z_BEST_COMPRESSION)\n-\t\t\tdie(\"bad zlib compression level %d\", level);\n+\t\t\tdie(_(\"bad zlib compression level %d\"), level);\n \t\tcore_compression_level = level;\n \t\tcore_compression_seen = 1;\n \t\tif (!zlib_compression_seen)\n@@ -873,7 +873,7 @@ static int git_default_core_config(const char *var, const char *value)\n \t\telse if (!strcmp(value, \"link\"))\n \t\t\tobject_creation_mode = OBJECT_CREATION_USES_HARDLINKS;\n \t\telse\n-\t\t\tdie(\"Invalid mode for object creation: %s\", value);\n+\t\t\tdie(_(\"invalid mode for object creation: %s\"), value);\n \t\treturn 0;\n \t}\n \n@@ -1173,7 +1173,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \n \tswitch (git_config_from_parameters(fn, data)) {\n \tcase -1: /* error */\n-\t\tdie(\"unable to parse command-line config\");\n+\t\tdie(_(\"unable to parse command-line config\"));\n \t\tbreak;\n \tcase 0: /* found nothing */\n \t\tbreak;\n@@ -1514,7 +1514,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \tcase KEY_SEEN:\n \t\tif (matches(key, value)) {\n \t\t\tif (store.seen == 1 && store.multi_replace == 0) {\n-\t\t\t\twarning(\"%s has multiple values\", key);\n+\t\t\t\twarning(_(\"%s has multiple values\"), key);\n \t\t\t}\n \n \t\t\tALLOC_GROW(store.offset, store.seen + 1,\n-- \n1.9.0.GIT\n"},{"id":"247319","messageId":"1407336792-16962-3-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 2/8] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:06Z","receivedAt":"2014-08-06T14:53:06Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"From: Matthieu Moy <Matthieu.Moy@imag.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>\nSigned-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n---\n config.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 34940fd..bb4629e 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":"247318","messageId":"1407336792-16962-4-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 3/8] add line number and file name info to `config_set`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:07Z","receivedAt":"2014-08-06T14:53:07Z","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 cache.h  |  5 +++++\n config.c | 16 ++++++++++++++--\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7292aef..0b1bdfd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1383,6 +1383,11 @@ 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 \n+struct key_value_info {\n+\tconst char *filename;\n+\tint linenr;\n+};\n+\n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\n \ndiff --git a/config.c b/config.c\nindex bb4629e..e4d745e 100644\n--- a/config.c\n+++ b/config.c\n@@ -1260,6 +1260,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 +1275,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 = NULL;\n+\t\tkv_info->linenr = -1;\n+\t}\n+\tsi->util = kv_info;\n \n \treturn 0;\n }\n@@ -1299,7 +1311,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":"247324","messageId":"1407336792-16962-5-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 4/8] change `git_config()` return value to void","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:08Z","receivedAt":"2014-08-06T14:53:08Z","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 0b1bdfd..f11ce41 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 e4d745e..4cefa25 100644\n--- a/config.c\n+++ b/config.c\n@@ -1230,9 +1230,21 @@ int git_config_with_options(config_fn_t fn, void *data,\n \treturn ret;\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":"247325","messageId":"1407336792-16962-6-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 5/8] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:09Z","receivedAt":"2014-08-06T14:53:09Z","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`. A custom\nerror message is also printed before dying, specified by the caller, which can\nbe skipped if `err` argument is set to NULL.\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\tif (!strcmp(value, \"foo\"))\n\t\t\tgit_config_die(key, \"value: `%s` is illegal\", value);\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 | 13 ++++++++++++\n cache.h                                |  3 +++\n config.c                               | 39 ++++++++++++++++++++++++++++++++--\n 3 files changed, 53 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 21f280c..0d8b99b 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -155,6 +155,19 @@ 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+`git_die_config(const char *key, const char *err, ...)`::\n+\n+\tFirst prints the error message specified by the caller in `err` and then\n+\tdies printing the line number and the file name of the highest priority\n+\tvalue for the configuration variable `key`.\n+\n+`void git_die_config_linenr(const char *key, const char *filename, int linenr)`::\n+\n+\tHelper function which formats the die error message according to the\n+\tparameters entered. Used by `git_die_config()`. It can be used by callers\n+\thandling `git_config_get_value_multi()` to print the correct error message\n+\tfor the desired value.\n+\n See test-config.c for usage examples.\n \n Value Parsing Helpers\ndiff --git a/cache.h b/cache.h\nindex f11ce41..89a0d51 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1388,6 +1388,9 @@ struct key_value_info {\n \tint linenr;\n };\n \n+extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\n+extern NORETURN void git_die_config_linenr(const char *key, const char *filename, int linenr);\n+\n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\n \ndiff --git a/config.c b/config.c\nindex 4cefa25..5ae9ab0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1469,8 +1469,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, NULL);\n+\treturn ret;\n }\n \n int git_config_get_string(const char *key, char **dest)\n@@ -1511,8 +1515,39 @@ 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, NULL);\n+\treturn ret;\n+}\n+\n+NORETURN\n+void git_die_config_linenr(const char *key, const char *filename, int linenr)\n+{\n+\tif (!filename)\n+\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n+\telse\n+\t\tdie(_(\"bad config variable '%s' in file '%s' at line %d\"),\n+\t\t    key, filename, linenr);\n+}\n+\n+NORETURN __attribute__((format(printf, 2, 3)))\n+void git_die_config(const char *key, const char *err, ...)\n+{\n+\tconst struct string_list *values;\n+\tstruct key_value_info *kv_info;\n+\n+\tif (err) {\n+\t\tva_list params;\n+\t\tva_start(params, err);\n+\t\tvreportf(\"error: \", err, params);\n+\t\tva_end(params);\n+\t}\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->filename, kv_info->linenr);\n }\n \n /*\n-- \n1.9.0.GIT\n"},{"id":"247322","messageId":"1407336792-16962-7-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 6/8] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:10Z","receivedAt":"2014-08-06T14:53:10Z","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                | 51 +++++++++++++++++++++++++++++++++++++++++--------\n t/t4055-diff-context.sh |  2 +-\n 3 files changed, 68 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 89a0d51..2693a37 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 5ae9ab0..427850a 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@@ -1230,7 +1224,7 @@ int git_config_with_options(config_fn_t fn, void *data,\n \treturn ret;\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@@ -1247,6 +1241,33 @@ 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\tgit_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr);\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@@ -1273,6 +1294,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@@ -1288,6 +1310,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@@ -1311,6 +1339,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@@ -1327,6 +1358,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@@ -1445,7 +1480,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":"247326","messageId":"1407336792-16962-8-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 7/8] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:11Z","receivedAt":"2014-08-06T14:53:11Z","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..5ea0aa4 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 \"line 2.*.git/config\\|.git/config.*line 2\" result\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"247323","messageId":"1407336792-16962-9-git-send-email-tanayabh@gmail.com","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 8/8] add tests for `git_config_get_string_const()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T14:53:12Z","receivedAt":"2014-08-06T14:53:12Z","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 5ea0aa4..ecc757a 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 \"line 7.*.git/config\\|.git/config.*line 7\" 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":"247330","messageId":"vpqy4v1vdui.fsf@anie.imag.fr","threadId":"37297","inReplyTo":"1407336792-16962-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-06T15:26:29Z","receivedAt":"2014-08-06T15:26:29Z","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> --- a/cache.h\n> +++ b/cache.h\n> @@ -1406,8 +1406,14 @@ extern int git_config_get_bool(const char *key, int *dest);\n[...]\n> +struct key_value_info {\n> +\tconst char *filename;\n> +\tint linenr;\n> +};\n[...]\n> diff --git a/config.c b/config.c\n> index cf9124f..427850a 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1224,11 +1224,6 @@ 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\nWhy is this needed? Are you now using key_value_info outside config.c?\nOr is it a leftover from a previous experiment?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247331","messageId":"vpqmwbhvdkx.fsf@anie.imag.fr","threadId":"37297","inReplyTo":"1407336792-16962-9-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 8/8] add tests for `git_config_get_string_const()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-06T15:32:14Z","receivedAt":"2014-08-06T15:32:14Z","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 '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 \"line 7.*.git/config\\|.git/config.*line 7\" result\n> +'\n\nThis is still dependant on the locale (\"line\" is translated). You need\nto use test_i18ngrep instead of grep here (see its definition and\ncomment in t/test-lib.sh).\n\nI don't think you need these two alternatives OTOH.\n\nBTW, Junio, I don't understand your remark \"This test is too tight (the\nfull string)\" in the previous iteration. Can you elaborate?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247335","messageId":"53E24D5F.6010104@gmail.com","threadId":"37297","inReplyTo":"vpqmwbhvdkx.fsf@anie.imag.fr","subject":"Re: [PATCH v8 8/8] add tests for `git_config_get_string_const()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-06T15:44:31Z","receivedAt":"2014-08-06T15:44:31Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 8/6/2014 9:02 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\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 \"line 7.*.git/config\\|.git/config.*line 7\" result\n>> +'\n> \n> This is still dependant on the locale (\"line\" is translated). You need\n> to use test_i18ngrep instead of grep here (see its definition and\n> comment in t/test-lib.sh).\n>\n\nOh, and I was searching t/README for a hint. Maybe we should add a line there\nto hint future readers.\nThanks. :)\n\n> I don't think you need these two alternatives OTOH.\n> \n> BTW, Junio, I don't understand your remark \"This test is too tight (the\n> full string)\" in the previous iteration. Can you elaborate?\n>\n\nI think he meant we must search for the relevant snippets instead of the whole string.\n"},{"id":"247336","messageId":"53E24EA2.2030807@ramsay1.demon.co.uk","threadId":"37297","inReplyTo":"1407336792-16962-4-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 3/8] add line number and file name info to `config_set`","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-06T15:49:54Z","receivedAt":"2014-08-06T15:49:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 06/08/14 15:53, Tanay Abhra wrote:\n> Store file name and line number for each key-value pair in the cache\n> during parsing of the configuration files.\n> \n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n>  cache.h  |  5 +++++\n>  config.c | 16 ++++++++++++++--\n>  2 files changed, 19 insertions(+), 2 deletions(-)\n> \n> diff --git a/cache.h b/cache.h\n> index 7292aef..0b1bdfd 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1383,6 +1383,11 @@ 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>  \n> +struct key_value_info {\n> +\tconst char *filename;\n> +\tint linenr;\n> +};\n> +\n\nHmm, why was this moved here? As far as I can tell, it is\n(still) not needed outside of config.c. What am I missing?\n\nATB,\nRamsay Jones\n"},{"id":"247368","messageId":"xmqq4mxpmhyb.fsf@gitster.dls.corp.google.com","threadId":"37297","inReplyTo":"vpqmwbhvdkx.fsf@anie.imag.fr","subject":"Re: [PATCH v8 8/8] add tests for `git_config_get_string_const()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-06T21:22:36Z","receivedAt":"2014-08-06T21:22:36Z","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>> ...\n>> +\tgrep \"line 7.*.git/config\\|.git/config.*line 7\" result\n>> +'\n>\n> This is still dependant on the locale (\"line\" is translated). You need\n> to use test_i18ngrep instead of grep here (see its definition and\n> comment in t/test-lib.sh).\n>\n> I don't think you need these two alternatives OTOH.\n>\n> BTW, Junio, I don't understand your remark \"This test is too tight (the\n> full string)\" in the previous iteration. Can you elaborate?\n\nThe original test was trying to match the string fully, i.e.\n\n> +\tgrep \"fatal: bad config variable '\\''alias.br'\\'' at file line 2 in .git/config\" result\n\nAs I already was feeling funny about seeing the phrase \"file line\"\nin the message and expecting it to be corrected, I thought I should\nencourage a check that does not depend on minor phrasing changes, if\nit can be done without bending backwards.\n\nI do agree with you that using \"\\|\" in \"grep\" a pattern to trigger\nERE Alternation counts as \"bending backwards\" as that is a GNU\nextension and not portable.\n"},{"id":"247410","messageId":"vpq38d8pupy.fsf@anie.imag.fr","threadId":"37297","inReplyTo":"xmqq4mxpmhyb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 8/8] add tests for `git_config_get_string_const()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-07T08:30:49Z","receivedAt":"2014-08-07T08:30:49Z","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> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>>\n>>> ...\n>>> +\tgrep \"line 7.*.git/config\\|.git/config.*line 7\" result\n>>> +'\n>>\n>> This is still dependant on the locale (\"line\" is translated). You need\n>> to use test_i18ngrep instead of grep here (see its definition and\n>> comment in t/test-lib.sh).\n>>\n>> I don't think you need these two alternatives OTOH.\n>>\n>> BTW, Junio, I don't understand your remark \"This test is too tight (the\n>> full string)\" in the previous iteration. Can you elaborate?\n>\n> The original test was trying to match the string fully, i.e.\n>\n>> +\tgrep \"fatal: bad config variable '\\''alias.br'\\'' at file line 2 in .git/config\" result\n>\n> As I already was feeling funny about seeing the phrase \"file line\"\n> in the message and expecting it to be corrected, I thought I should\n> encourage a check that does not depend on minor phrasing changes, if\n> it can be done without bending backwards.\n\nOK.\n\nI personally prefer tight tests in this case, as the patch doing the\nrephrase \"sees\" what changed by running the tests, and documents the\nvisible change by changing the expected in the tests scripts. But no\nstrong opinion here, I'd be fine with e.g.\n\ntest_i18ngrep \"fatal: .* alias.br.*line 2 in .git/config\" result\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247451","messageId":"xmqq8un0ktqu.fsf@gitster.dls.corp.google.com","threadId":"37297","inReplyTo":"vpqy4v1vdui.fsf@anie.imag.fr","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-07T19:03:05Z","receivedAt":"2014-08-07T19:03:05Z","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>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1406,8 +1406,14 @@ extern int git_config_get_bool(const char *key, int *dest);\n> [...]\n>> +struct key_value_info {\n>> +\tconst char *filename;\n>> +\tint linenr;\n>> +};\n> [...]\n>> diff --git a/config.c b/config.c\n>> index cf9124f..427850a 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1224,11 +1224,6 @@ 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>\n> Why is this needed? Are you now using key_value_info outside config.c?\n> Or is it a leftover from a previous experiment?\n\nHas this been resolved in the new round?\n"},{"id":"247454","messageId":"vpq61i4nld7.fsf@anie.imag.fr","threadId":"37297","inReplyTo":"xmqq8un0ktqu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-07T19:35:48Z","receivedAt":"2014-08-07T19:35:48Z","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> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>>\n>>> --- a/cache.h\n>>> +++ b/cache.h\n>>> @@ -1406,8 +1406,14 @@ extern int git_config_get_bool(const char *key, int *dest);\n>> [...]\n>>> +struct key_value_info {\n>>> +\tconst char *filename;\n>>> +\tint linenr;\n>>> +};\n>> [...]\n>>> diff --git a/config.c b/config.c\n>>> index cf9124f..427850a 100644\n>>> --- a/config.c\n>>> +++ b/config.c\n>>> @@ -1224,11 +1224,6 @@ 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>>\n>> Why is this needed? Are you now using key_value_info outside config.c?\n>> Or is it a leftover from a previous experiment?\n>\n> Has this been resolved in the new round?\n\nTanay explained in another subthread why this was needed. For callers\niterating over the string_list who want to get the file/line info, they\nneed to be able to cast the void * pointer to struct key_value_info *.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247459","messageId":"xmqqoavwjb3i.fsf@gitster.dls.corp.google.com","threadId":"37297","inReplyTo":"vpq61i4nld7.fsf@anie.imag.fr","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-07T20:31:13Z","receivedAt":"2014-08-07T20:31:13Z","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>>> Why is this needed? Are you now using key_value_info outside config.c?\n>>> Or is it a leftover from a previous experiment?\n>>\n>> Has this been resolved in the new round?\n>\n> Tanay explained in another subthread why this was needed. For callers\n> iterating over the string_list who want to get the file/line info, they\n> need to be able to cast the void * pointer to struct key_value_info *.\n\nFor callers that want to see all the multi-values, it would be\npreferrable for the iterator to pass the filename and the linenumber\nto the callback function, instead of exposing its implementation\ndetail as a single string list and telling them to pick it apart,\nno?\n\nNot a very convincing argument, but OK for now in the sense that we\ncan fix it later if we wanted to before it gets too late.\n"},{"id":"247477","messageId":"53E4D986.6040708@gmail.com","threadId":"37297","inReplyTo":"xmqqoavwjb3i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-08T14:07:02Z","receivedAt":"2014-08-08T14:07:02Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 8/8/2014 2:01 AM, Junio C Hamano wrote:\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> \n>>>> Why is this needed? Are you now using key_value_info outside config.c?\n>>>> Or is it a leftover from a previous experiment?\n>>>\n>>> Has this been resolved in the new round?\n>>\n>> Tanay explained in another subthread why this was needed. For callers\n>> iterating over the string_list who want to get the file/line info, they\n>> need to be able to cast the void * pointer to struct key_value_info *.\n> \n> For callers that want to see all the multi-values, it would be\n> preferrable for the iterator to pass the filename and the linenumber\n> to the callback function, instead of exposing its implementation\n> detail as a single string list and telling them to pick it apart,\n> no?\n> \n> Not a very convincing argument, but OK for now in the sense that we\n> can fix it later if we wanted to before it gets too late.\n>\n\n(cc to Ramsay)\n\nThe discussion in both threads (v8 and v9), boils down to this,\nis the `key_value_info` struct really required to be declared public or should be\njust an implementation detail. I will give you the context,\n\nThe usage of the above mentioned struct is only required for\ngit_config_get_value_multi(). With the public struct, the code flow would look\nlike,\n\n\n-- 8< --\ndiff --git a/notes.c b/notes.c\nindex 5fe691d..b7ab115 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,\n \tfree(globs_copy);\n }\n\n-static int notes_display_config(const char *k, const char *v, void *cb)\n-{\n-\tint *load_refs = cb;\n-\n-\tif (*load_refs && !strcmp(k, \"notes.displayref\")) {\n-\t\tif (!v)\n-\t\t\tconfig_error_nonbool(k);\n-\t\tstring_list_add_refs_by_glob(&display_notes_refs, v);\n-\t}\n-\n-\treturn 0;\n-}\n-\n const char *default_notes_ref(void)\n {\n \tconst char *notes_ref = NULL;\n@@ -1041,7 +1028,9 @@ struct notes_tree **load_notes_trees(struct string_list *refs)\n void init_display_notes(struct display_notes_opt *opt)\n {\n \tchar *display_ref_env;\n-\tint load_config_refs = 0;\n+\tconst struct string_list *values;\n+\tstruct key_value_info *kv_info;\n+\tint load_config_refs = 0, i;\n \tdisplay_notes_refs.strdup_strings = 1;\n\n \tassert(!display_notes_trees);\n@@ -1058,7 +1047,21 @@ void init_display_notes(struct display_notes_opt *opt)\n \t\t\tload_config_refs = 1;\n \t}\n\n-\tgit_config(notes_display_config, &load_config_refs);\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\tkv_info = values->items[i].util;\n+\t\t\t\t\tconfig_error_nonbool(\"notes.displayref\");\n+\t\t\t\t\tgit_die_config_linenr(\"notes.displayref\", kv_info->filename, 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 \tif (opt) {\n \t\tstruct string_list_item *item;\n-- 8< --\n\nWe cannot use git_die_config() here because it is applicable to the last\nvalue for a given variable.\n\nAlternative solution to the problem can be a helper function like this,\n\ngit_die_config_index(key, value_index, err_msg, ...) which needs the value_index for a multi valued one,\n\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\tgit_die_config_linenr(\"notes.displayref\", i, \"no null values allowed for :'%s'\", \"notes.displayref\");\n+\t\t\t\telse ; /* do stuff */\n+\t\t\t}\n\nA callback iterator which supplies the linenr and filename to the callback\nfunction is not helpful, because there are many variable checks in a\ngit_config() call where multi valued and single valued both reside, so we cannot\nuse a callback iterator without adding more code cruft.\n\nWhat do you think, which way seems least obtrusive, or is there an another way out?\n"},{"id":"247478","messageId":"53E4DF6D.8070904@ramsay1.demon.co.uk","threadId":"37297","inReplyTo":"53E4D986.6040708@gmail.com","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-08T14:32:13Z","receivedAt":"2014-08-08T14:32:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 08/08/14 15:07, Tanay Abhra wrote:\n> On 8/8/2014 2:01 AM, Junio C Hamano wrote:\n>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>>\n>>>>> Why is this needed? Are you now using key_value_info outside config.c?\n>>>>> Or is it a leftover from a previous experiment?\n>>>>\n>>>> Has this been resolved in the new round?\n>>>\n>>> Tanay explained in another subthread why this was needed. For callers\n>>> iterating over the string_list who want to get the file/line info, they\n>>> need to be able to cast the void * pointer to struct key_value_info *.\n>>\n>> For callers that want to see all the multi-values, it would be\n>> preferrable for the iterator to pass the filename and the linenumber\n>> to the callback function, instead of exposing its implementation\n>> detail as a single string list and telling them to pick it apart,\n>> no?\n>>\n>> Not a very convincing argument, but OK for now in the sense that we\n>> can fix it later if we wanted to before it gets too late.\n>>\n> \n> (cc to Ramsay)\n> \n> The discussion in both threads (v8 and v9), boils down to this,\n> is the `key_value_info` struct really required to be declared public or should be\n> just an implementation detail. I will give you the context,\n\nNo, this is not the issue for me. The patch which introduces the\nstruct in cache.h does not make use of that struct in any interface.\nIt *is* an implementation detail of some code in config.c only.\n\nI do not know how that structure will be used in future patches. ;-)\n\nATB,\nRamsay Jones\n"},{"id":"247556","messageId":"xmqqppg88d8x.fsf@gitster.dls.corp.google.com","threadId":"37297","inReplyTo":"53E4DF6D.8070904@ramsay1.demon.co.uk","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-10T17:29:18Z","receivedAt":"2014-08-10T17:29:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> On 08/08/14 15:07, Tanay Abhra wrote:\n> ...\n>> (cc to Ramsay)\n>> \n>> The discussion in both threads (v8 and v9), boils down to this,\n>> is the `key_value_info` struct really required to be declared public or should be\n>> just an implementation detail. I will give you the context,\n>\n> No, this is not the issue for me. The patch which introduces the\n> struct in cache.h does not make use of that struct in any interface.\n> It *is* an implementation detail of some code in config.c only.\n>\n> I do not know how that structure will be used in future patches. ;-)\n\nIt is debatable if it is a good API that tells the users \"In the\ndata I return to you, there is a structure hanging there with these\ntwo fields. Feel free to peek into it if you need what is recorded\nin them\".  But the contract between the the endgame \"API\" and its\ncallers can include such a direct access, and then you can say that\nit is a part of the API, not just an implementation detail.\n\nI think you and Tanay are both right (and I am wrong ;-).\n\nYou are right in that the \"API\" is giving more than the callers\nconverted to it at this point in the series.\n\nTanay is right in that the way the struct will be used, which is\nillustrated by the example in the message you are responding to,\nshould be in the part of this series that gives the implementation\nof the API before presenting the converted users, as the series\ndeems it part of the API to let the users peek into the struct.\n\nIt may turn out that we have to abstract it further when we need a\nmore elaborate data structure kept in the kv-info in the future.  At\nthat point it will become undesirable to keep giving the callers\ndirect access to it, because direct struct access means that the\nparticular aspect of the implementaiton detail will be cast in stone\nand would not allow to be replaced with some other representation\nthat is more efficient for the implementation.\n\nBut as I said, what we see in this series can do for now.  The\nfuture can come later ;-)\n"},{"id":"247579","messageId":"53E893F1.9080408@ramsay1.demon.co.uk","threadId":"37297","inReplyTo":"xmqqppg88d8x.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 0/8] Rewrite `git_config()` using config-set API","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-08-11T09:59:13Z","receivedAt":"2014-08-11T09:59:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 10/08/14 18:29, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n> \n>> On 08/08/14 15:07, Tanay Abhra wrote:\n>> ...\n>>> (cc to Ramsay)\n>>>\n>>> The discussion in both threads (v8 and v9), boils down to this,\n>>> is the `key_value_info` struct really required to be declared public or should be\n>>> just an implementation detail. I will give you the context,\n>>\n>> No, this is not the issue for me. The patch which introduces the\n>> struct in cache.h does not make use of that struct in any interface.\n>> It *is* an implementation detail of some code in config.c only.\n>>\n>> I do not know how that structure will be used in future patches. ;-)\n> \n> It is debatable if it is a good API that tells the users \"In the\n> data I return to you, there is a structure hanging there with these\n> two fields. Feel free to peek into it if you need what is recorded\n> in them\".\n\nThere is no debate in my mind. ;-)\n\nIn this future patch, the movement of the structure out of config.c would\nbe plain for everyone to see, and (hopefully) debate the merits of such\nan 'interface'. Along with checking that it is properly documented. (which\npatch should the documentation be in?) Where should it be documented?\n \n>              But the contract between the the endgame \"API\" and its\n> callers can include such a direct access, and then you can say that\n> it is a part of the API, not just an implementation detail.\n\nSure. It's just a question of timing and allowing reviewers to see the\nactual change in one patch.\n\n> I think you and Tanay are both right (and I am wrong ;-).\n\n;-)\n\nI don't have *very* strong feelings about this, which is why I didn't\nrespond to the earlier replies by Tanay and Matthieu, but since I was\ncc-ed on this thread ... (It seemed to me that my comments had been\nmisunderstood).\n\nSo yes, I think this \"API\" is ugly and could be improved upon, but I\ndon't care sufficiently to make any further comment. :-D\n\nATB,\nRamsay Jones\n"}]}