{"thread":{"id":"37271","subject":"[PATCH v7 0/8] Rewrite `git_config()` using config-set API","startedAt":"2014-08-01T17:05:48Z","lastAt":"2014-08-05T16:42:10Z","messageCount":19,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":7,"patchTotal":8},"messages":[{"id":"247107","messageId":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":null,"subject":"[PATCH v7 0/8] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:48Z","receivedAt":"2014-08-01T17:05:48Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[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 |  12 +++\n branch.c                               |   5 +-\n cache.h                                |  28 +++++-\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, 200 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 e7ec7cc..d6a2c39 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -160,6 +160,13 @@ as well as retrieval for the queried variable, including:\n \tDies printing the line number and the file name of the highest\n \tpriority value 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 243f618..ff17889 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1407,6 +1407,7 @@ 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 extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\ndiff --git a/config.c b/config.c\nindex 15fcd91..cf9124f 100644\n--- a/config.c\n+++ b/config.c\n@@ -461,9 +461,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@@ -579,9 +579,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@@ -666,7 +666,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@@ -744,7 +744,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@@ -755,7 +755,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@@ -877,7 +877,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@@ -1177,7 +1177,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@@ -1260,13 +1260,7 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\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\tgit_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr);\n \t\t}\n \t}\n }\n@@ -1569,20 +1563,27 @@ int git_config_get_pathname(const char *key, const char **dest)\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+\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+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-\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+\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n+}\n \n /*\n  * Find all the stuff for git_config_set() below.\n@@ -1617,7 +1618,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-- 8< --\n"},{"id":"247108","messageId":"1406912756-15517-2-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 1/8] config.c: mark error and warnings strings for translation","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:49Z","receivedAt":"2014-08-01T17:05:49Z","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":"247109","messageId":"1406912756-15517-3-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 2/8] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:50Z","receivedAt":"2014-08-01T17:05:50Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>\n\nIf a callback returns a negative value to `git_config*()` family,\nthey call `die()` while printing the line number and the file name.\nCurrently the printed line number is off by one, thus printing the\nwrong line number.\n\nMake `linenr` point to the line we just parsed during the call\nto callback to get accurate line number in error messages.\n\nCommit-message-by: Tanay Abhra <tanayabh@gmail.com>\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n config.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 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":"247110","messageId":"1406912756-15517-4-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 3/8] add line number and file name info to `config_set`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:51Z","receivedAt":"2014-08-01T17:05:51Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Store file name and line number for each key-value pair in the cache\nduring parsing of the configuration files.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n config.c | 21 +++++++++++++++++++--\n 1 file changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex bb4629e..e685b66 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":"247111","messageId":"1406912756-15517-5-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 4/8] change `git_config()` return value to void","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:52Z","receivedAt":"2014-08-01T17:05:52Z","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 e685b66..f923b1c 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":"247112","messageId":"1406912756-15517-6-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:53Z","receivedAt":"2014-08-01T17:05:53Z","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 | 12 ++++++++++++\n cache.h                                |  2 ++\n config.c                               | 34 ++++++++++++++++++++++++++++++++--\n 3 files changed, 46 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 815c1ee..d6a2c39 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -155,6 +155,18 @@ 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+`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 c61919d..9ec2f4e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1382,6 +1382,8 @@ 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 extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\ndiff --git a/config.c b/config.c\nindex f923b1c..ddbfe4d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1474,8 +1474,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@@ -1516,8 +1520,34 @@ 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+NORETURN\n+void git_die_config_linenr(const char *key, 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+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->filename, kv_info->linenr);\n }\n \n /*\n-- \n1.9.0.GIT\n"},{"id":"247113","messageId":"1406912756-15517-7-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 6/8] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:54Z","receivedAt":"2014-08-01T17:05:54Z","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 9ec2f4e..ff17889 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 ddbfe4d..cf9124f 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,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@@ -1278,6 +1299,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 +1315,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 +1344,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 +1363,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 +1485,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":"247114","messageId":"1406912756-15517-8-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 7/8] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:55Z","receivedAt":"2014-08-01T17:05:55Z","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":"247115","messageId":"1406912756-15517-9-git-send-email-tanayabh@gmail.com","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v7 8/8] add tests for `git_config_get_string_const()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-01T17:05:56Z","receivedAt":"2014-08-01T17:05:56Z","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":"247207","messageId":"vpq38dcs76x.fsf@anie.imag.fr","threadId":"37271","inReplyTo":"1406912756-15517-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v7 2/8] config.c: fix accuracy of line number in errors","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T13:41:42Z","receivedAt":"2014-08-04T13:41:42Z","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> From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>\n\nI usually use my @imag.fr, not @grenoble-inp.fr address as Git author\n(even though my mailer has @grenoble-inp.fr as From: field). Both\naddresses are equivalent so it's no big deal.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247208","messageId":"vpqtx5sqska.fsf@anie.imag.fr","threadId":"37271","inReplyTo":"1406912756-15517-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v7 0/8] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T13:43:01Z","receivedAt":"2014-08-04T13:43:01Z","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 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\nThis series is now\n\nReviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\n\nThanks,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247218","messageId":"xmqqoaw0ruwf.fsf@gitster.dls.corp.google.com","threadId":"37271","inReplyTo":"1406912756-15517-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T18:07:12Z","receivedAt":"2014-08-04T18:07:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> Add `git_die_config` that dies printing the line number and the file name\n> of the highest priority value for the configuration variable `key`.\n>\n> It has usage in non-callback based config value retrieval where we can\n> raise an error and die if there is a semantic error.\n> For example,\n>\n> \tif (!git_config_get_value(key, &value)) {\n> \t\t/* NULL values not allowed */\n> \t\tif (!value)\n> \t\t\tgit_config_die(key);\n> \t\telse\n> \t\t\t/* do work */\n> \t}\n\nIt feels a bit unnatural at the API level that this does not take\n'value'; I do understand that it is not a big deal in the error code\npath to locate again the value from the configuration using the key,\nbut still.\n\nIt feels even more unnatural that the caller cannot say _how_ it\nfinds the value offending by not taking any message.  For one\nparticular callchain, e.g. git_config_get_string() that eventually\ncalls git_config_string() which will show an error message via\nconfig_error_nonbool(), you may not want any extra message, but for\nnew callers that wants to make sure value falls within a supported\nrange, this forces it to write\n\n\tif (!git_config_get_int(key, &num)) {\n        \tif (!(0 < num && num < 4)) {\n\t\t\terror(\"'%s' must be between 1 and 3\");\n                        git_config_die(key);\n\t\t}\n\t\t/* otherwise work */\n\t}\n\nand then the error message would say something like:\n\n\terror: 'core.frotz' must be between 1 and 3\n\tfatal: bad config variable 'core.frotz' at file line 15 in .git/config\n\nwhich sounds somewhat backwards, at least to me.\n\n> +NORETURN\n> +void git_die_config_linenr(const char *key, const char *filename, int linenr)\n> +{\n> +\tif (!linenr)\n> +\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n\nDo we have existing code that says \"we signal that it is from the\ncommand line by setting linenr to zero\" already?  Otherwise I would\nhave thought filename == NULL would be a more sensible convention.\n\nOtherwise OK.\n\n> +\telse\n> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n\nAt least, quote the last '%s'.\n\n> +\t\t\tkey,\n> +\t\t\tlinenr,\n> +\t\t\tfilename);\n\nDon't waste vertical real-estate line this.\n\nPerhaps\n\n        die(_(\"bad config variable '%s' in file '%s' at line %d\"),\n            key, linenr, filename);\n"},{"id":"247219","messageId":"xmqqk36orul6.fsf@gitster.dls.corp.google.com","threadId":"37271","inReplyTo":"1406912756-15517-8-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v7 7/8] add a test for semantic errors in config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T18:13:57Z","receivedAt":"2014-08-04T18:13:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> +\tgrep \"fatal: bad config variable '\\''alias.br'\\'' at file line 2 in .git/config\" result\n\nThis test is too tight (the full string), and also needs to know\nabout i18n, I think.\n"},{"id":"247235","messageId":"53DFD729.8090307@gmail.com","threadId":"37271","inReplyTo":"xmqqoaw0ruwf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-04T18:55:37Z","receivedAt":"2014-08-04T18:55:37Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 8/4/2014 11:37 PM, Junio C Hamano wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> Add `git_die_config` that dies printing the line number and the file name\n>> of the highest priority value for the configuration variable `key`.\n>>\n>> It has usage in non-callback based config value retrieval where we can\n>> raise an error and die if there is a semantic error.\n>> For example,\n>>\n>> \tif (!git_config_get_value(key, &value)) {\n>> \t\t/* NULL values not allowed */\n>> \t\tif (!value)\n>> \t\t\tgit_config_die(key);\n>> \t\telse\n>> \t\t\t/* do work */\n>> \t}\n> \n> It feels a bit unnatural at the API level that this does not take\n> 'value'; I do understand that it is not a big deal in the error code\n> path to locate again the value from the configuration using the key,\n> but still.\n>\n\nBut, we don't have a use for \"value\" as it is not denoted in the error\nstring, that is why I left it out.\n\n> It feels even more unnatural that the caller cannot say _how_ it\n> finds the value offending by not taking any message.  For one\n> particular callchain, e.g. git_config_get_string() that eventually\n> calls git_config_string() which will show an error message via\n> config_error_nonbool(), you may not want any extra message, but for\n> new callers that wants to make sure value falls within a supported\n> range, this forces it to write\n> \n> \tif (!git_config_get_int(key, &num)) {\n>         \tif (!(0 < num && num < 4)) {\n> \t\t\terror(\"'%s' must be between 1 and 3\");\n>                         git_config_die(key);\n> \t\t}\n> \t\t/* otherwise work */\n> \t}\n> \n> and then the error message would say something like:\n> \n> \terror: 'core.frotz' must be between 1 and 3\n> \tfatal: bad config variable 'core.frotz' at file line 15 in .git/config\n> \n> which sounds somewhat backwards, at least to me.\n>\n\nI was aping the old git_config() system, it also does exactly what you described\nabove. for example, builtin/gc.c line 91,\n\n\t\tif (!strcmp(var, \"gc.pruneexpire\")) {\n\t\tif (value && strcmp(value, \"now\")) {\n\t\t\tunsigned long now = approxidate(\"now\");\n\t\t\tif (approxidate(value) >= now)\n\t\t\t\treturn error(_(\"Invalid %s: '%s'\"), var, value);\n\t\t}\n\nwould print,\n \terror: Invalid gc.pruneexpire: 'value'\n\tfatal: bad config variable 'gc.pruneexpire' at file line 15 in .git/config\n\nor imap-send.c line 1340,\n\n\tif (!strcmp(\"sslverify\", key))\n\t\tserver.ssl_verify = git_config_bool(key, val);\n\telse if (!strcmp(\"preformattedhtml\", key))\n\t\tserver.use_html = git_config_bool(key, val);\n\telse if (!val)\n\t\treturn config_error_nonbool(key);\nagain would cause a error & die, message combo as above. There are many examples like that.\nWe can easily take a custom error message but again I was just aping the old system.\n\n>> +NORETURN\n>> +void git_die_config_linenr(const char *key, const char *filename, int linenr)\n>> +{\n>> +\tif (!linenr)\n>> +\t\tdie(_(\"unable to parse '%s' from command-line config\"), key);\n> \n> Do we have existing code that says \"we signal that it is from the\n> command line by setting linenr to zero\" already?  Otherwise I would\n> have thought filename == NULL would be a more sensible convention.\n> \n> Otherwise OK.\n>\n\nNoted. Next reroll will have filename as the convention.\n\n>> +\telse\n>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n> \n> At least, quote the last '%s'.\n>\n\nNoted. Thanks.\n"},{"id":"247242","messageId":"vpqegwwauo2.fsf@anie.imag.fr","threadId":"37271","inReplyTo":"53DFD729.8090307@gmail.com","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-04T20:04:13Z","receivedAt":"2014-08-04T20:04:13Z","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> I was aping the old git_config() system, it also does exactly what you described\n> above. for example, builtin/gc.c line 91,\n>\n> \t\tif (!strcmp(var, \"gc.pruneexpire\")) {\n> \t\tif (value && strcmp(value, \"now\")) {\n> \t\t\tunsigned long now = approxidate(\"now\");\n> \t\t\tif (approxidate(value) >= now)\n> \t\t\t\treturn error(_(\"Invalid %s: '%s'\"), var, value);\n> \t\t}\n>\n> would print,\n>  \terror: Invalid gc.pruneexpire: 'value'\n> \tfatal: bad config variable 'gc.pruneexpire' at file line 15 in .git/config\n\nIt's good to do at least as well as the old system, but I agree with\nJunio that it's suboptimal.\n\nHaving a single API call to replace\n\n        error(\"'%s' must be between 1 and 3\");\n        git_config_die(key);\n\nwith stg like:\n\n        git_config_die(key, \"'%s' must be between 1 and 3\");\n\nin Junio's example would allow git_config_die to format the error\nmessage the way it likes (i.e. it can be the same as before when you\nintroduce it, and improve afterwards).\n\nI've never been disturbed by the quality of Git's error messages wrt\nconfig files (it's not a compiler!), so having good quality messages is\nnot a high priority IMHO, but having a good API that as a side effect\ncan produce good error messages is important. If changing the error\nmessages requires rewritting all callers later, then we've missed the\npoint doing the refactoring now.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247251","messageId":"xmqqiom8q8ob.fsf@gitster.dls.corp.google.com","threadId":"37271","inReplyTo":"53DFD729.8090307@gmail.com","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T20:52:36Z","receivedAt":"2014-08-04T20:52:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> On 8/4/2014 11:37 PM, Junio C Hamano wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> Add `git_die_config` that dies printing the line number and the file name\n>>> of the highest priority value for the configuration variable `key`.\n>>>\n>>> It has usage in non-callback based config value retrieval where we can\n>>> raise an error and die if there is a semantic error.\n>>> For example,\n>>>\n>>> \tif (!git_config_get_value(key, &value)) {\n>>> \t\t/* NULL values not allowed */\n>>> \t\tif (!value)\n>>> \t\t\tgit_config_die(key);\n>>> \t\telse\n>>> \t\t\t/* do work */\n>>> \t}\n>> \n>> It feels a bit unnatural at the API level that this does not take\n>> 'value'; I do understand that it is not a big deal in the error code\n>> path to locate again the value from the configuration using the key,\n>> but still.\n>>\n>\n> But, we don't have a use for \"value\" as it is not denoted in the error\n> string, that is why I left it out.\n\nThat is my point.  Why doesn't the error message talk about what\nvalue the caller found was offensive, and in what way?\n\n>>> +\telse\n>>> +\t\tdie(_(\"bad config variable '%s' at file line %d in %s\"),\n>> \n>> At least, quote the last '%s'.\n>>\n>\n> Noted. Thanks.\n\nActually, \"at file line\" sounded very strange, at least to me, hence\nthe suggested reword in the part you did not quote.\n"},{"id":"247274","messageId":"53E0F057.1040706@gmail.com","threadId":"37271","inReplyTo":"vpqegwwauo2.fsf@anie.imag.fr","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-08-05T14:55:19Z","receivedAt":"2014-08-05T14:55:19Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 8/5/2014 1:34 AM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> I was aping the old git_config() system, it also does exactly what you described\n>> above. for example, builtin/gc.c line 91,\n>>\n>> \t\tif (!strcmp(var, \"gc.pruneexpire\")) {\n>> \t\tif (value && strcmp(value, \"now\")) {\n>> \t\t\tunsigned long now = approxidate(\"now\");\n>> \t\t\tif (approxidate(value) >= now)\n>> \t\t\t\treturn error(_(\"Invalid %s: '%s'\"), var, value);\n>> \t\t}\n>>\n>> would print,\n>>  \terror: Invalid gc.pruneexpire: 'value'\n>> \tfatal: bad config variable 'gc.pruneexpire' at file line 15 in .git/config\n> \n> It's good to do at least as well as the old system, but I agree with\n> Junio that it's suboptimal.\n> \n> Having a single API call to replace\n> \n>         error(\"'%s' must be between 1 and 3\");\n>         git_config_die(key);\n> \n> with stg like:\n> \n>         git_config_die(key, \"'%s' must be between 1 and 3\");\n>\n\nMatthieu, I have finished the new version, but instead of flooding the mailing list with\na series again, I wanted to confirm if the new git_config_die() is alright.\n\n\tNORETURN __attribute__((format(printf, 2, 3)))\n\tvoid git_die_config(const char *key, const char *err, ...)\n\t{\n\t\tconst struct string_list *values;\n\t\tstruct key_value_info *kv_info;\n\n\t\tif (err) {\n\t\t\tva_list params;\n\t\t\tva_start(params, err);\n\t\t\tvreportf(\"error: \", err, params);\n\t\t\tva_end(params);\n\t\t}\n\t\tvalues = git_config_get_value_multi(key);\n\t\tkv_info = values->items[values->nr - 1].util;\n\t\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n\t}\n\nCurrently works like the old git_config() error reporting path. If err is set to \"NULL\",\nit would print no error message and just the die message. If given something like,\n\n\t git_config_die(key, \"value '%s' is not allowed\", value);\n\nit would print,\n\n\terror: value '3' is not allowed\n\tfatal: bad config variable 'core.frotz' at file line 15 in .git/config\n\nCheers,\nTanay Abhra.\n\n> in Junio's example would allow git_config_die to format the error\n> message the way it likes (i.e. it can be the same as before when you\n> introduce it, and improve afterwards).\n> \n> I've never been disturbed by the quality of Git's error messages wrt\n> config files (it's not a compiler!), so having good quality messages is\n> not a high priority IMHO, but having a good API that as a side effect\n> can produce good error messages is important. If changing the error\n> messages requires rewritting all callers later, then we've missed the\n> point doing the refactoring now.\n> \n"},{"id":"247275","messageId":"vpqiom72cj4.fsf@anie.imag.fr","threadId":"37271","inReplyTo":"53E0F057.1040706@gmail.com","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-08-05T15:15:27Z","receivedAt":"2014-08-05T15:15:27Z","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> Matthieu, I have finished the new version, but instead of flooding the mailing list with\n> a series again, I wanted to confirm if the new git_config_die() is alright.\n>\n> \tNORETURN __attribute__((format(printf, 2, 3)))\n> \tvoid git_die_config(const char *key, const char *err, ...)\n> \t{\n> \t\tconst struct string_list *values;\n> \t\tstruct key_value_info *kv_info;\n>\n> \t\tif (err) {\n> \t\t\tva_list params;\n> \t\t\tva_start(params, err);\n> \t\t\tvreportf(\"error: \", err, params);\n> \t\t\tva_end(params);\n> \t\t}\n> \t\tvalues = git_config_get_value_multi(key);\n> \t\tkv_info = values->items[values->nr - 1].util;\n> \t\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n> \t}\n>\n> Currently works like the old git_config() error reporting path. If err is set to \"NULL\",\n> it would print no error message and just the die message. If given something like,\n>\n> \t git_config_die(key, \"value '%s' is not allowed\", value);\n>\n> it would print,\n>\n> \terror: value '3' is not allowed\n> \tfatal: bad config variable 'core.frotz' at file line 15 in .git/config\n\nThat seems to be a good step forward.\n\nI think we would also want to improve the error message, but that\nshouldn't block your series from inclusion: we can do that later without\nAPI change.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"247278","messageId":"xmqqppgeq465.fsf@gitster.dls.corp.google.com","threadId":"37271","inReplyTo":"vpqiom72cj4.fsf@anie.imag.fr","subject":"Re: [PATCH v7 5/8] config: add `git_die_config()` to the config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-05T16:42:10Z","receivedAt":"2014-08-05T16:42:10Z","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>> Currently works like the old git_config() error reporting path. If err is set to \"NULL\",\n>> it would print no error message and just the die message. If given something like,\n>>\n>> \t git_config_die(key, \"value '%s' is not allowed\", value);\n>>\n>> it would print,\n>>\n>> \terror: value '3' is not allowed\n>> \tfatal: bad config variable 'core.frotz' at file line 15 in .git/config\n>\n> That seems to be a good step forward.\n>\n> I think we would also want to improve the error message, but that\n> shouldn't block your series from inclusion: we can do that later without\n> API change.\n\nYup, I agree with your assessment.\n\nThank you, both, for good polishing.\n"}]}