{"thread":{"id":"37211","subject":"[PATCH v2 0/6] Rewrite `git_config()` using config-set API","startedAt":"2014-07-25T12:58:09Z","lastAt":"2014-07-25T17:29:02Z","messageCount":12,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"246691","messageId":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":null,"subject":"[PATCH v2 0/6] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:09Z","receivedAt":"2014-07-25T12:58:09Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"[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 1d2856f (ta/config-set) in pu or topic[1]\nin the mailing list with name \"git config cache & special querying API utilizing\nthe cache\".\n\nThis series aims to do these three things,\n\n* Use the config-set API to rewrite git_config().\n\n* Solve any legacy bugs in the previous system while at it.\n\n* To be feature complete compared to the previous git_config() implementation,\n  which I think it is now. (added the line number and file name info just for\n  completeness)\n\nAlso, I haven't yet checked the exact improvements but still as a teaser,\ngit status now only rereads the configuration files twice instead of four\ntimes.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/253862\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/254101\n\nTanay Abhra (6):\n  config.c: fix accuracy of line number in errors\n  add line number and file name info to `config_set`\n  rewrite git_config() to use the config-set API\n  add a test for semantic errors in config files\n  config: add `git_die_config()` to the config-set API\n  Add tests for `git_config_get_string()`\n\n Documentation/technical/api-config.txt |   5 ++\n cache.h                                |  26 +++++++\n config.c                               | 135 ++++++++++++++++++++++++++++-----\n t/t1308-config-set.sh                  |  20 +++++\n test-config.c                          |  10 +++\n 5 files changed, 175 insertions(+), 21 deletions(-)\n\n-- \n1.9.0.GIT\n"},{"id":"246692","messageId":"1406293095-15920-2-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 1/6] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:10Z","receivedAt":"2014-07-25T12:58:10Z","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 0d799e0..06257d9 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":"246693","messageId":"1406293095-15920-3-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 2/6] add line number and file name info to `config_set`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:11Z","receivedAt":"2014-07-25T12:58:11Z","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 | 19 ++++++++++++++++++-\n 1 file changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 06257d9..110f9a5 100644\n--- a/config.c\n+++ b/config.c\n@@ -1232,6 +1232,11 @@ int git_config_with_options(config_fn_t fn, void *data,\n \treturn ret;\n }\n \n+struct key_value_info {\n+\tconst char *filename;\n+\tint linenr;\n+};\n+\n int git_config(config_fn_t fn, void *data)\n {\n \treturn git_config_with_options(fn, data, NULL, 1);\n@@ -1262,6 +1267,9 @@ static struct config_set_element *configset_find_element(struct config_set *cs,\n static int configset_add_value(struct config_set *cs, const char *key, const char *value)\n {\n \tstruct config_set_element *e;\n+\tstruct string_list_item *si;\n+\tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n+\n \te = configset_find_element(cs, key);\n \t/*\n \t * Since the keys are being fed by git_config*() callback mechanism, they\n@@ -1274,7 +1282,16 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \t\tstring_list_init(&e->value_list, 1);\n \t\thashmap_add(&cs->config_hash, e);\n \t}\n-\tstring_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\tsi = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\tif (cf) {\n+\t\tkv_info->filename = strintern(cf->name);\n+\t\tkv_info->linenr = cf->linenr;\n+\t} else {\n+\t\t/* for values read from `git_config_from_parameters()` */\n+\t\tkv_info->filename = strintern(\"parameter\");\n+\t\tkv_info->linenr = 0;\n+\t}\n+\tsi->util = kv_info;\n \n \treturn 0;\n }\n-- \n1.9.0.GIT\n"},{"id":"246694","messageId":"1406293095-15920-4-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 3/6] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:12Z","receivedAt":"2014-07-25T12:58:12Z","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  | 25 +++++++++++++++++++++++++\n config.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 76 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7292aef..11ded5a 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,33 @@ 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+\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 110f9a5..aa5c0ad 100644\n--- a/config.c\n+++ b/config.c\n@@ -35,12 +35,6 @@ struct config_source {\n \tlong (*do_ftell)(struct config_source *c);\n };\n \n-struct config_set_element {\n-\tstruct hashmap_entry ent;\n-\tchar *key;\n-\tstruct string_list value_list;\n-};\n-\n static struct config_source *cf;\n \n static int zlib_compression_seen;\n@@ -1237,11 +1231,44 @@ struct key_value_info {\n \tint linenr;\n };\n \n-int git_config(config_fn_t fn, void *data)\n+static int git_config_raw(config_fn_t fn, void *data)\n {\n \treturn git_config_with_options(fn, data, NULL, 1);\n }\n \n+static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n+{\n+\tint i, value_index;\n+\tstruct string_list *values;\n+\tstruct config_set_element *entry;\n+\tstruct configset_list *list = &cs->list;\n+\tstruct key_value_info *kv_info;\n+\n+\tfor (i = 0; i < list->nr; i++) {\n+\t\tentry = list->items[i].e;\n+\t\tvalue_index = list->items[i].value_index;\n+\t\tvalues = &entry->value_list;\n+\t\tif (fn(entry->key, values->items[value_index].string, data) < 0) {\n+\t\t\tkv_info = values->items[value_index].util;\n+\t\t\tif (!kv_info->linenr)\n+\t\t\t\tdie(\"unable to parse command-line config\");\n+\t\t\telse\n+\t\t\t\tdie(\"bad config file line %d in %s\",\n+\t\t\t\t\tkv_info->linenr,\n+\t\t\t\t\tkv_info->filename);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static void git_config_check_init(void);\n+\n+int git_config(config_fn_t fn, void *data)\n+{\n+\tgit_config_check_init();\n+\treturn configset_iter(&the_config_set, fn, data);\n+}\n+\n static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)\n {\n \tstruct config_set_element k;\n@@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n {\n \tstruct config_set_element *e;\n \tstruct string_list_item *si;\n+\tstruct configset_list_item *l_item;\n \tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n \n \te = configset_find_element(cs, key);\n@@ -1283,6 +1311,13 @@ 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+\tif (cs->list.nr + 1 > cs->list.alloc)\n+\t\tALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);\n+\tl_item = &cs->list.items[cs->list.nr++];\n+\tl_item->e = e;\n+\tl_item->value_index = e->value_list.nr - 1;\n+\n \tif (cf) {\n \t\tkv_info->filename = strintern(cf->name);\n \t\tkv_info->linenr = cf->linenr;\n@@ -1306,6 +1341,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@@ -1318,10 +1356,14 @@ 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+\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@@ -1448,7 +1490,7 @@ static void git_config_check_init(void)\n \tif (the_config_set.hash_initialized)\n \t\treturn;\n \tgit_configset_init(&the_config_set);\n-\tgit_config(config_set_callback, &the_config_set);\n+\tgit_config_raw(config_set_callback, &the_config_set);\n }\n \n void git_config_clear(void)\n-- \n1.9.0.GIT\n"},{"id":"246695","messageId":"1406293095-15920-5-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 4/6] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:13Z","receivedAt":"2014-07-25T12:58:13Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Semantic errors (for example, for alias.* variables NULL values are\nnot allowed) in configuration files cause a die printing the line\nnumber and file name of the offending value.\n\nAdd a test documenting that such errors cause a die printing the\naccurate line number and file name.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n t/t1308-config-set.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 7fdf840..35c6ee2 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -197,4 +197,15 @@ test_expect_success 'proper error on error in custom config files' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check line errors for malformed values' '\n+\tmv .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\tcat >.git/config <<-\\EOF &&\n+\t[alias]\n+\t\tbr\n+\tEOF\n+\ttest_expect_code 128 git br 2>result &&\n+\tgrep \"fatal: bad config file line 2 in .git/config\" result\n+'\n+\n test_done\n-- \n1.9.0.GIT\n"},{"id":"246696","messageId":"1406293095-15920-6-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 5/6] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:14Z","receivedAt":"2014-07-25T12:58:14Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add `git_die_config` that dies printing the line number and the file name\nof the highest priority value for the configuration variable `key`.\n\nIt has usage in non-callback based config value retrieval where we can\nraise an error and die if there is a semantic error.\nFor example,\n\n\tif (!git_config_get_value(key, &value)) {\n\t\t/* NULL values not allowed */\n\t\tif (!value)\n\t\t\tgit_config_die(key);\n\t\telse\n\t\t\t/* do work */\n\t}\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n Documentation/technical/api-config.txt |  5 ++++\n cache.h                                |  1 +\n config.c                               | 44 ++++++++++++++++++++++++++--------\n 3 files changed, 40 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 815c1ee..e7ec7cc 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:\n \tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n \tthe user's home directory when found at the beginning of the path.\n \n+`void git_die_config(const char *key)`::\n+\n+\tDies printing the line number and the file name of the highest\n+\tpriority value for the configuration variable `key`.\n+\n See test-config.c for usage examples.\n \n Value Parsing Helpers\ndiff --git a/cache.h b/cache.h\nindex 11ded5a..5c3dd88 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1407,6 +1407,7 @@ extern int git_config_get_bool(const char *key, int *dest);\n extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n extern int git_config_get_maybe_bool(const char *key, int *dest);\n extern int git_config_get_pathname(const char *key, const char **dest);\n+extern void git_die_config(const char *key);\n \n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\ndiff --git a/config.c b/config.c\nindex aa5c0ad..1e49ae7 100644\n--- a/config.c\n+++ b/config.c\n@@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n \n int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n {\n-\tconst char *value;\n-\tif (!git_configset_get_value(cs, key, &value))\n-\t\treturn git_config_string(dest, key, value);\n-\telse\n-\t\treturn 1;\n+\tint ret;\n+\tchar *value;\n+\tret = git_configset_get_string(cs, key, &value);\n+\tif (ret <= 0)\n+\t\t*dest = (const char*)value;\n+\treturn ret;\n }\n \n int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n@@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest\n \t\t\treturn config_error_nonbool(key);\n \t\t*dest = xstrdup(value);\n \t\treturn 0;\n-\t}\n-\telse\n+\t} else\n \t\treturn 1;\n }\n \n@@ -1514,14 +1514,22 @@ 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 {\n+\tint ret;\n \tgit_config_check_init();\n-\treturn git_configset_get_string(&the_config_set, key, dest);\n+\tret = git_configset_get_string(&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_int(const char *key, int *dest)\n@@ -1556,8 +1564,24 @@ int git_config_get_maybe_bool(const char *key, int *dest)\n \n int git_config_get_pathname(const char *key, const char **dest)\n {\n+\tint ret;\n \tgit_config_check_init();\n-\treturn git_configset_get_pathname(&the_config_set, key, dest);\n+\tret = git_configset_get_pathname(&the_config_set, key, dest);\n+\tif (ret < 0)\n+\t\tgit_die_config(key);\n+\treturn ret;\n+}\n+\n+void git_die_config(const char *key)\n+{\n+\tconst struct string_list *values;\n+\tstruct key_value_info *kv_info;\n+\tvalues = git_config_get_value_multi(key);\n+\tkv_info = values->items[values->nr - 1].util;\n+\tif (!kv_info->linenr)\n+\t\tdie(\"unable to parse command-line config\");\n+\telse\n+\t\tdie(\"bad config file line %d in %s\",kv_info->linenr, kv_info->filename);\n }\n \n /*\n-- \n1.9.0.GIT\n"},{"id":"246697","messageId":"1406293095-15920-7-git-send-email-tanayabh@gmail.com","threadId":"37211","inReplyTo":"1406293095-15920-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 6/6] Add tests for `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T12:58:15Z","receivedAt":"2014-07-25T12:58:15Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add tests for `git_config_get_string()`, check whether it\ndies printing the line number and the file name if a NULL\nvalue is retrieved for the given key.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n t/t1308-config-set.sh |  9 +++++++++\n test-config.c         | 10 ++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 35c6ee2..d7cdc6e 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -119,6 +119,15 @@ test_expect_success 'find integer value for a key' '\n \tcheck_config get_int lamb.chop 65\n '\n \n+test_expect_success 'find string value for a key' '\n+\tcheck_config get_string case.baz hask\n+'\n+\n+test_expect_success 'check line error when NULL string is queried' '\n+\ttest_expect_code 128 test-config get_string case.foo 2>result &&\n+\tgrep \"fatal: bad config file line 7 in .git/config\" result\n+'\n+\n test_expect_success 'find integer if value is non parse-able' '\n \tcheck_config expect_code 128 get_int lamb.head\n '\ndiff --git a/test-config.c b/test-config.c\nindex 9dd1b22..6a77552 100644\n--- a/test-config.c\n+++ b/test-config.c\n@@ -16,6 +16,8 @@\n  *\n  * get_bool -> print bool value for the entered key or die\n  *\n+ * get_string -> print string value for the entered key or die\n+ *\n  * configset_get_value -> returns value with the highest priority for the entered key\n  * \t\t\tfrom a config_set constructed from files entered as arguments.\n  *\n@@ -84,6 +86,14 @@ int main(int argc, char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_string\")) {\n+\t\tif (!git_config_get_string_const(argv[2], &v)) {\n+\t\t\tprintf(\"%s\\n\", v);\n+\t\t\tgoto exit0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\tgoto exit1;\n+\t\t}\n \t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tint err;\n-- \n1.9.0.GIT\n"},{"id":"246702","messageId":"vpq38dppmjh.fsf@anie.imag.fr","threadId":"37211","inReplyTo":"1406293095-15920-4-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v2 3/6] rewrite git_config() to use the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-25T13:58:58Z","receivedAt":"2014-07-25T13:58:58Z","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> +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\nI originally wondered why you had two levels of pointers, but\nconfig_set_element is not new, just moved. It's OK.\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> +\n> +struct configset_list {\n\nI wouldn't put a blank line between comment and decl if the comment\napplies to the decl.\n\n> -int git_config(config_fn_t fn, void *data)\n> +static int git_config_raw(config_fn_t fn, void *data)\n>  {\n>  \treturn git_config_with_options(fn, data, NULL, 1);\n>  }\n\nI would have done this and the new git_config() as a separate patch, but\nI do not mind strongly.\n\n>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)\n>  {\n>  \tstruct config_set_element k;\n> @@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n>  {\n>  \tstruct config_set_element *e;\n>  \tstruct string_list_item *si;\n> +\tstruct configset_list_item *l_item;\n>  \tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n>  \n>  \te = configset_find_element(cs, key);\n> @@ -1283,6 +1311,13 @@ 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> +\tif (cs->list.nr + 1 > cs->list.alloc)\n\nThe \"if\" is already in ALLOC_GROW.\n\n> +\t\tALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);\n\nThe 20 should be just 1 I guess. You're adding 1 element, and ALLOC_GROW\nwill take care of allocating more than 1 for you (see alloc_nr and\nALLOC_GROW's defs in cache.h).\n\n> @@ -1318,10 +1356,14 @@ 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\nDoesn't this change belong to PATCH 2/6 ?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246703","messageId":"vpqvbqlo7s0.fsf@anie.imag.fr","threadId":"37211","inReplyTo":"1406293095-15920-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-25T14:03:11Z","receivedAt":"2014-07-25T14:03:11Z","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/config.c\n> +++ b/config.c\n> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n>  \n>  int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n>  {\n> -\tconst char *value;\n> -\tif (!git_configset_get_value(cs, key, &value))\n> -\t\treturn git_config_string(dest, key, value);\n> -\telse\n> -\t\treturn 1;\n> +\tint ret;\n> +\tchar *value;\n> +\tret = git_configset_get_string(cs, key, &value);\n> +\tif (ret <= 0)\n> +\t\t*dest = (const char*)value;\n> +\treturn ret;\n>  }\n\nIsn't this a fixup meant for another series?\n\n>  int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n> @@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest\n>  \t\t\treturn config_error_nonbool(key);\n>  \t\t*dest = xstrdup(value);\n>  \t\treturn 0;\n> -\t}\n> -\telse\n> +\t} else\n>  \t\treturn 1;\n\nUseless churn.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246704","messageId":"53D26499.4040104@gmail.com","threadId":"37211","inReplyTo":"vpq38dppmjh.fsf@anie.imag.fr","subject":"Re: [PATCH v2 3/6] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T14:07:21Z","receivedAt":"2014-07-25T14:07:21Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/25/2014 7:28 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\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> I originally wondered why you had two levels of pointers, but\n> config_set_element is not new, just moved. It's OK.\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>> +\n>> +struct configset_list {\n> \n> I wouldn't put a blank line between comment and decl if the comment\n> applies to the decl.\n>\n\nNoted.\n\n>> -int git_config(config_fn_t fn, void *data)\n>> +static int git_config_raw(config_fn_t fn, void *data)\n>>  {\n>>  \treturn git_config_with_options(fn, data, NULL, 1);\n>>  }\n> \n> I would have done this and the new git_config() as a separate patch, but\n> I do not mind strongly.\n> \n>>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)\n>>  {\n>>  \tstruct config_set_element k;\n>> @@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n>>  {\n>>  \tstruct config_set_element *e;\n>>  \tstruct string_list_item *si;\n>> +\tstruct configset_list_item *l_item;\n>>  \tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n>>  \n>>  \te = configset_find_element(cs, key);\n>> @@ -1283,6 +1311,13 @@ 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>> +\tif (cs->list.nr + 1 > cs->list.alloc)\n> \n> The \"if\" is already in ALLOC_GROW.\n> \n>> +\t\tALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);\n> \n> The 20 should be just 1 I guess. You're adding 1 element, and ALLOC_GROW\n> will take care of allocating more than 1 for you (see alloc_nr and\n> ALLOC_GROW's defs in cache.h).\n>\n\nOh, you are are right, I thought alloc grew by one, not by a factor.\n\n>> @@ -1318,10 +1356,14 @@ 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> \n> Doesn't this change belong to PATCH 2/6 ?\n> \n\nYup, you are right again.\nThanks.\n"},{"id":"246705","messageId":"53D265DA.8070702@gmail.com","threadId":"37211","inReplyTo":"vpqvbqlo7s0.fsf@anie.imag.fr","subject":"Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T14:12:42Z","receivedAt":"2014-07-25T14:12:42Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/25/2014 7:33 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n>>  \n>>  int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n>>  {\n>> -\tconst char *value;\n>> -\tif (!git_configset_get_value(cs, key, &value))\n>> -\t\treturn git_config_string(dest, key, value);\n>> -\telse\n>> -\t\treturn 1;\n>> +\tint ret;\n>> +\tchar *value;\n>> +\tret = git_configset_get_string(cs, key, &value);\n>> +\tif (ret <= 0)\n>> +\t\t*dest = (const char*)value;\n>> +\treturn ret;\n>>  }\n> \n> Isn't this a fixup meant for another series?\n>\n\nThough v12 is in pu, Junio commented that git_configset_get_string_const() &\ngit_configset_get_string() can be done more concisely, I was trying to do\nthat but I failed.\n\n>>  int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n>> @@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest\n>>  \t\t\treturn config_error_nonbool(key);\n>>  \t\t*dest = xstrdup(value);\n>>  \t\treturn 0;\n>> -\t}\n>> -\telse\n>> +\t} else\n>>  \t\treturn 1;\n> \n> Useless churn.\n> \n"},{"id":"246719","messageId":"xmqqmwbx73fl.fsf@gitster.dls.corp.google.com","threadId":"37211","inReplyTo":"53D265DA.8070702@gmail.com","subject":"Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-25T17:29:02Z","receivedAt":"2014-07-25T17:29:02Z","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 7/25/2014 7:33 PM, Matthieu Moy wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> --- a/config.c\n>>> +++ b/config.c\n>>> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n>>>  \n>>>  int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)\n>>>  {\n>>> -\tconst char *value;\n>>> -\tif (!git_configset_get_value(cs, key, &value))\n>>> -\t\treturn git_config_string(dest, key, value);\n>>> -\telse\n>>> -\t\treturn 1;\n>>> +\tint ret;\n>>> +\tchar *value;\n>>> +\tret = git_configset_get_string(cs, key, &value);\n>>> +\tif (ret <= 0)\n>>> +\t\t*dest = (const char*)value;\n>>> +\treturn ret;\n>>>  }\n>> \n>> Isn't this a fixup meant for another series?\n>>\n>\n> Though v12 is in pu, Junio commented that git_configset_get_string_const() &\n> git_configset_get_string() can be done more concisely, I was trying to do\n> that but I failed.\n\nMy comment on that version was not about conciseness.  You had one\nthat called git_config_string() to let the callee do the nonbool\nerror handling and xstrdup() of the non-error return value, and the\nother one that did exactly what a call to git_config_string() would\nhave done.  That is being redundant, not just failing to be concise.\n\nI was actually hoping that we would see just\n\nint git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n{\n\treturn git_configset_get_string_const(cs, key, (const char **)dest);\n}\n\nwith the implementation of _const() variant be the one from v12.\n"}]}