{"thread":{"id":"37199","subject":"[PATCH 0/7] Rewrite `git_config()` using config-set API","startedAt":"2014-07-23T18:42:51Z","lastAt":"2014-07-25T18:55:45Z","messageCount":51,"participants":["Tanay Abhra","Matthieu Moy","Eric Sunshine","Junio C Hamano","John Keeping","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"246584","messageId":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":null,"subject":"[PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:51Z","receivedAt":"2014-07-23T18:42:51Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"This series builds on the top of 5def4132 (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\nTanay Abhra (7):\n\n Documentation/technical/api-config.txt | 5 ++\n cache.h | 1 +\n config.c | 93 +++++++++++++++++++++++++++++++---\n t/t1308-config-set.sh | 17 +++++++\n test-config.c | 10 ++++\n userdiff.c | 14 ++++-\n 6 files changed, 131 insertions(+), 9 deletions(-)\n\n-- \n1.9.0.GIT\n"},{"id":"246585","messageId":"1406140978-9472-2-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 1/7] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:52Z","receivedAt":"2014-07-23T18:42:52Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"If 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\nDiscovered-by: Tanay Abhra \nSigned-off-by: Matthieu Moy \n---\n config.c | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/config.c b/config.c\nindex 22971e9..6db8f97 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":"246586","messageId":"1406140978-9472-3-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 2/7] rewrite git_config() to use the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:53Z","receivedAt":"2014-07-23T18:42:53Z","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 \n---\n config.c | 29 +++++++++++++++++++++++++++--\n 1 file changed, 27 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 6db8f97..a7fb9a4 100644\n--- a/config.c\n+++ b/config.c\n@@ -1232,11 +1232,36 @@ 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+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;\n+\tstruct string_list *strptr;\n+\tstruct config_set_element *entry;\n+\tstruct hashmap_iter iter;\n+\thashmap_iter_init(&cs->config_hash, &iter);\n+\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\tstrptr = &entry->value_list;\n+\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\tif (fn(entry->key, strptr->items[i].string, data) < 0)\n+\t\t\t\tdie(\"bad config file line in (TODO: file/line info)\");\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@@ -1418,7 +1443,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":"246587","messageId":"1406140978-9472-4-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 3/7] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:54Z","receivedAt":"2014-07-23T18:42:54Z","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 \n---\n t/t1308-config-set.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 7fdf840..bd033df 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -197,4 +197,12 @@ 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+\tcp .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\techo \"[alias]\\n br\" >.git/config &&\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":"246588","messageId":"1406140978-9472-5-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 4/7] add line number and file name info to `config_set`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:55Z","receivedAt":"2014-07-23T18:42:55Z","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.\nUse the information to print line number and file name in errors raised\nby `git_config()` which now uses the configuration files caching layer\ninternally.\n\nSigned-off-by: Tanay Abhra \n---\n config.c | 32 ++++++++++++++++++++++++++++----\n 1 file changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex a7fb9a4..43a951f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1237,9 +1237,15 @@ static int git_config_raw(config_fn_t fn, void *data)\n \treturn git_config_with_options(fn, data, NULL, 1);\n }\n \n+struct key_value_info {\n+\tconst char *filename;\n+\tint linenr;\n+};\n+\n static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n {\n \tint i;\n+\tstruct key_value_info *kv_info;\n \tstruct string_list *strptr;\n \tstruct config_set_element *entry;\n \tstruct hashmap_iter iter;\n@@ -1247,8 +1253,15 @@ static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \twhile ((entry = hashmap_iter_next(&iter))) {\n \t\tstrptr = &entry->value_list;\n \t\tfor (i = 0; i < strptr->nr; i++) {\n-\t\t\tif (fn(entry->key, strptr->items[i].string, data) < 0)\n-\t\t\t\tdie(\"bad config file line in (TODO: file/line info)\");\n+\t\t\tif (fn(entry->key, strptr->items[i].string, data) < 0) {\n+\t\t\t\tkv_info = strptr->items[i].util;\n+\t\t\t\tif (!kv_info->linenr)\n+\t\t\t\t\tdie(\"unable to parse command-line config\");\n+\t\t\t\telse\n+\t\t\t\t\tdie(\"bad config file line %d in %s\",\n+\t\t\t\t\t\tkv_info->linenr,\n+\t\t\t\t\t\tkv_info->filename);\n+\t\t\t}\n \t\t}\n \t}\n \treturn 0;\n@@ -1287,6 +1300,8 @@ 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 key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n+\tstruct string_list_item *si;\n \te = configset_find_element(cs, key);\n \t/*\n \t * Since the keys are being fed by git_config*() callback mechanism, they\n@@ -1299,7 +1314,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@@ -1326,7 +1350,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":"246591","messageId":"1406140978-9472-6-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:56Z","receivedAt":"2014-07-23T18:42:56Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"t4018-diff-funcname.sh fails for the new `git_config()` which uses the\nconfiguration files caching layer internally.\nThe test introduced in commit d64d6cdc checks that whether `xfuncname` takes\nprecedence over `funcname` variable which was not guaranteed by config API\npreviously and worked only because values were parsed and fed in order. The\nnew `git_config()` only guarantees precedence order for variables with the\nsame name.\n\nAlso `funcname` variable is deprecated and not documented properly.\n`xfuncname` is mentioned in the docs and takes precedence over `funcname`.\nInstead of removing `funcname` variable, enforce `xfuncname` precedence over\n`funcname` when the variables have the same subsection. Remove dependency\nthat required values to be fed to userdiff_config() in parsing order for the\ntest to succeed.\n\nSigned-off-by: Tanay Abhra \n---\nNote: this the only test that failed for the new git_config() rewrite.\n\n userdiff.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex fad52d6..a51bc89 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -2,6 +2,7 @@\n #include \"userdiff.h\"\n #include \"cache.h\"\n #include \"attr.h\"\n+#include \"string-list.h\"\n \n static struct userdiff_driver *drivers;\n static int ndrivers;\n@@ -211,9 +212,12 @@ int userdiff_config(const char *k, const char *v)\n \tstruct userdiff_driver *drv;\n \tconst char *name, *type;\n \tint namelen;\n+\tchar *subsection = NULL;\n+\tstatic struct string_list xflag = STRING_LIST_INIT_DUP;\n \n \tif (parse_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n \t\treturn 0;\n+\tsubsection = xstrndup(name, namelen);\n \n \tdrv = userdiff_find_by_namelen(name, namelen);\n \tif (!drv) {\n@@ -224,10 +228,16 @@ int userdiff_config(const char *k, const char *v)\n \t\tdrv->binary = -1;\n \t}\n \n-\tif (!strcmp(type, \"funcname\"))\n+\tif (!strcmp(type, \"funcname\") && !unsorted_string_list_has_string(&xflag, subsection)) {\n+\t\tfree (subsection);\n \t\treturn parse_funcname(&drv->funcname, k, v, 0);\n-\tif (!strcmp(type, \"xfuncname\"))\n+\t}\n+\tif (!strcmp(type, \"xfuncname\")) {\n+\t\tstring_list_append(&xflag, subsection);\n+\t\tfree (subsection);\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n+\t}\n+\tfree(subsection);\n \tif (!strcmp(type, \"binary\"))\n \t\treturn parse_tristate(&drv->binary, k, v);\n \tif (!strcmp(type, \"command\"))\n-- \n1.9.0.GIT\n"},{"id":"246590","messageId":"1406140978-9472-7-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 6/7] config: add `git_die_config()` to the config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:57Z","receivedAt":"2014-07-23T18:42:57Z","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 \n---\n Documentation/technical/api-config.txt | 5 +++++\n cache.h | 1 +\n config.c | 24 ++++++++++++++++++++++--\n 3 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 8a86e45..14571e7 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -150,6 +150,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 2f63fd1..fc886c3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1380,6 +1380,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 43a951f..f0c9805 100644\n--- a/config.c\n+++ b/config.c\n@@ -1491,8 +1491,12 @@ const struct string_list *git_config_get_value_multi(const char *key)\n \n int git_config_get_string(const char *key, const 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@@ -1527,8 +1531,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 *strptr;\n+\tstruct key_value_info *kv_info;\n+\tstrptr = git_config_get_value_multi(key);\n+\tkv_info = strptr->items[strptr->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":"246589","messageId":"1406140978-9472-8-git-send-email-tanayabh@gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 7/7] Add tests for `git_config_get_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-23T18:42:58Z","receivedAt":"2014-07-23T18:42:58Z","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 an NULL\nvalue is retrieved for the given key.\n\nSigned-off-by: Tanay Abhra \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 bd033df..1cb453b 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..994741a 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(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":"246597","messageId":"vpqwqb3vp9u.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T19:38:53Z","receivedAt":"2014-07-23T19:38:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> This series builds on the top of 5def4132 (ta/config-set) in pu or\n> topic[1]\n\nNot exactly: 5def4132 has been replaced in pu, and it does not contain\ntests, hence PATCH 3 does not apply on top of 5def4132. The series\napplies to 0912a24, but does not compile, since you use strintern which\nis in master but not in 0912a24.\n\nOK, applied and compiled. Let the review begin.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246599","messageId":"vpq4my7vohx.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"1406140978-9472-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 2/7] rewrite git_config() to use the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T19:55:38Z","receivedAt":"2014-07-23T19:55:38Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> -int git_config(config_fn_t fn, void *data)\n> +static int git_config_raw(config_fn_t fn, void *data)\n\nAs you noticed already, this change breaks several tests. You are going\nto repair them later in the series, but your patch series produces a\nnon-bisectable history.\n\nThe history should pass tests at each commit. If needed, you can ensure\nthat with eg.\n\ngit rebase HEAD~7 --exec \"make && cd t/ && ./t1308-config-set.sh && ./t4018-diff-funcname.sh\" -i\n\n(or --exec 'make test', but that takes really long)\n\nSo, this patch should come later in the series (not hard, just a\nreordering with rebase -i).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246598","messageId":"vpq38drvohr.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"1406140978-9472-4-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 3/7] add a test for semantic errors in config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T19:55:44Z","receivedAt":"2014-07-23T19:55:44Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> +test_expect_success 'check line errors for malformed values' '\n> +\tcp .git/config .git/config.old &&\n> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n> +\techo \"[alias]\\n br\" >.git/config &&\n> +\ttest_expect_code 128 git br 2>result &&\n> +\tgrep \"fatal: bad config file line 2 in .git/config\" result\n> +'\n> +\n\nThe test fails at this point in history. I guess the problem will\ndisapear once you've put PATCH 2 at the right point in the series.\n\nAnother option is to mark the test as test_expect_failure when you\nintroduce it, and change it to test_expect_success when you fix it\n(probably not applicable here, but it's a trick I find elegant).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246600","messageId":"vpqwqb3u9s6.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"1406140978-9472-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 2/7] rewrite git_config() to use the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T19:58:49Z","receivedAt":"2014-07-23T19:58:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> +static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n> +{\n> +\tint i;\n> +\tstruct string_list *strptr;\n> +\tstruct config_set_element *entry;\n> +\tstruct hashmap_iter iter;\n> +\thashmap_iter_init(&cs->config_hash, &iter);\n> +\twhile ((entry = hashmap_iter_next(&iter))) {\n> +\t\tstrptr = &entry->value_list;\n> +\t\tfor (i = 0; i < strptr->nr; i++) {\n> +\t\t\tif (fn(entry->key, strptr->items[i].string, data) < 0)\n> +\t\t\t\tdie(\"bad config file line in (TODO: file/line info)\");\n\nOne more reason to reorder (but that will actually be slightly more than\n\"rebase -i\", you'll have a few conflicts to fix) is to avoid this TODO.\nPut the patch after the line number patch and you'll be able to provide\nthe right information directly.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246602","messageId":"vpqppgvu9gn.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"1406140978-9472-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T20:05:44Z","receivedAt":"2014-07-23T20:05:44Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> Also `funcname` variable is deprecated and not documented properly.\n\nI'd say \"purposely undocumented (see 45d9414fa5, Brandon Casey, Sep 18\n2008)\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246603","messageId":"vpqiomnu9bq.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"1406140978-9472-8-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 7/7] Add tests for `git_config_get_string()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T20:08:41Z","receivedAt":"2014-07-23T20:08:41Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> Add tests for `git_config_get_string()`, check whether it\n> dies printing the line number and the file name if an NULL\n\na NULL (no n).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246604","messageId":"CAPig+cRwtWMwDOxQEhLDy_eZbPuYRhiag4SwJ+A0OOu44un5MA@mail.gmail.com","threadId":"37199","inReplyTo":"1406140978-9472-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-07-23T21:04:52Z","receivedAt":"2014-07-23T21:04:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 23, 2014 at 2:42 PM, Tanay Abhra wrote:\n> t4018-diff-funcname.sh fails for the new `git_config()` which uses the\n> configuration files caching layer internally.\n> The test introduced in commit d64d6cdc checks that whether `xfuncname` takes\n\ns/that//\n\n> precedence over `funcname` variable which was not guaranteed by config API\n> previously and worked only because values were parsed and fed in order. The\n> new `git_config()` only guarantees precedence order for variables with the\n\ns/\\s+/ /\n\n> same name.\n>\n> Also `funcname` variable is deprecated and not documented properly.\n> `xfuncname` is mentioned in the docs and takes precedence over `funcname`.\n> Instead of removing `funcname` variable, enforce `xfuncname` precedence over\n> `funcname` when the variables have the same subsection. Remove dependency\n> that required values to be fed to userdiff_config() in parsing order for the\n> test to succeed.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> Note: this the only test that failed for the new git_config() rewrite.\n>\n> userdiff.c | 14 ++++++++++++--\n> 1 file changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/userdiff.c b/userdiff.c\n> index fad52d6..a51bc89 100644\n> --- a/userdiff.c\n> +++ b/userdiff.c\n> @@ -2,6 +2,7 @@\n> #include \"userdiff.h\"\n> #include \"cache.h\"\n> #include \"attr.h\"\n> +#include \"string-list.h\"\n>\n> static struct userdiff_driver *drivers;\n> static int ndrivers;\n> @@ -211,9 +212,12 @@ int userdiff_config(const char *k, const char *v)\n> struct userdiff_driver *drv;\n> const char *name, *type;\n> int namelen;\n> + char *subsection = NULL;\n> + static struct string_list xflag = STRING_LIST_INIT_DUP;\n>\n> if (parse_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n> return 0;\n> + subsection = xstrndup(name, namelen);\n>\n> drv = userdiff_find_by_namelen(name, namelen);\n> if (!drv) {\n> @@ -224,10 +228,16 @@ int userdiff_config(const char *k, const char *v)\n> drv->binary = -1;\n> }\n>\n> - if (!strcmp(type, \"funcname\"))\n> + if (!strcmp(type, \"funcname\") && !unsorted_string_list_has_string(&xflag, subsection)) {\n> + free (subsection);\n> return parse_funcname(&drv->funcname, k, v, 0);\n> - if (!strcmp(type, \"xfuncname\"))\n> + }\n> + if (!strcmp(type, \"xfuncname\")) {\n> + string_list_append(&xflag, subsection);\n> + free (subsection);\n> return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n> + }\n> + free(subsection);\n> if (!strcmp(type, \"binary\"))\n> return parse_tristate(&drv->binary, k, v);\n> if (!strcmp(type, \"command\"))\n> --\n> 1.9.0.GIT\n"},{"id":"246612","messageId":"xmqqiomnda1y.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"1406140978-9472-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T21:44:57Z","receivedAt":"2014-07-23T21:44:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> This series builds on the top of 5def4132 (ta/config-set) in pu or topic[1]\n> in the mailing list with name \"git config cache & special querying API utilizing\n> the cache\".\n>\n> This 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\nSounds promising.\n\nAre you done with the original series, or do you still want to fix\nthe const-ness issue with the string pointer before working on\nfollow-up topics like this one?\n"},{"id":"246613","messageId":"xmqqegxbd9u0.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"1406140978-9472-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 1/7] config.c: fix accuracy of line number in errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T21:49:43Z","receivedAt":"2014-07-23T21:49:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> If a callback returns a negative value to `git_config*()` family,\n> they call `die()` while printing the line number and the file name.\n> Currently the printed line number is off by one, thus printing the\n> wrong line number.\n>\n> Make `linenr` point to the line we just parsed during the call\n> to callback to get accurate line number in error messages.\n>\n> Discovered-by: Tanay Abhra \n> Signed-off-by: Matthieu Moy \n\nThanks.\n\nI am not sure what to read in these two lines. Was the fix done by\nyou or Matthieu?\n\n> ---\n> config.c | 12 +++++++++++-\n> 1 file changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/config.c b/config.c\n> index 22971e9..6db8f97 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"},{"id":"246614","messageId":"xmqq7g33d9fe.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"1406140978-9472-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 2/7] rewrite git_config() to use the config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T21:58:29Z","receivedAt":"2014-07-23T21:58:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> Of all the functions in `git_config*()` family, `git_config()` has the\n> most invocations in the whole code base. Each `git_config()` invocation\n> causes config file rereads which can be avoided using the config-set API.\n>\n> Use the config-set API to rewrite `git_config()` to use the config caching\n> layer to avoid config file rereads on each invocation during a git process\n> lifetime. First invocation constructs the cache, and after that for each\n> successive invocation, `git_config()` feeds values from the config cache\n> instead of rereading the configuration files.\n>\n> Signed-off-by: Tanay Abhra \n> ---\n> config.c | 29 +++++++++++++++++++++++++++--\n> 1 file changed, 27 insertions(+), 2 deletions(-)\n\nThis is the natural logical conclusion ;-)\n\n> diff --git a/config.c b/config.c\n> index 6db8f97..a7fb9a4 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1232,11 +1232,36 @@ 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> +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;\n> +\tstruct string_list *strptr;\n\nWe know string_list would hold strings, so naming the variable\nstrptr does not give us much information. As this is a list of\nvalues you would get by querying for a variable (or \"key\"), perhaps\nname it \"values\" or something?\n\n> +\tstruct config_set_element *entry;\n> +\tstruct hashmap_iter iter;\n\nHave a blank line between the local variable definitions (above) and\nthe first executable statement (below); it would make it easier to\nread, especially because out codebase do not have decl-after-stmt.\n\n> +\thashmap_iter_init(&cs->config_hash, &iter);\n> +\twhile ((entry = hashmap_iter_next(&iter))) {\n> +\t\tstrptr = &entry->value_list;\n> +\t\tfor (i = 0; i < strptr->nr; i++) {\n> +\t\t\tif (fn(entry->key, strptr->items[i].string, data) < 0)\n> +\t\t\t\tdie(\"bad config file line in (TODO: file/line info)\");\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\nPerhaps if you define this function at the right place in the file\nyou do not have to add an forward decl of git_config_check_init()?\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> @@ -1418,7 +1443,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"},{"id":"246616","messageId":"xmqq38drd8ue.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpq38drvohr.fsf@anie.imag.fr","subject":"Re: [PATCH 3/7] add a test for semantic errors in config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T22:11:05Z","receivedAt":"2014-07-23T22:11:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Tanay Abhra writes:\n>\n>> +test_expect_success 'check line errors for malformed values' '\n>> +\tcp .git/config .git/config.old &&\n\nShould this be \"mv\" not \"cp\"? You will be overwriting the file from\nscratch in the later part of this test.\n\n>> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n>> +\techo \"[alias]\\n br\" >.git/config &&\n\nIs the use of \\n portable?\n\n> Another option is to mark the test as test_expect_failure when you\n> introduce it, and change it to test_expect_success when you fix it\n> (probably not applicable here, but it's a trick I find elegant).\n\nYes, I agree that it is a good practice to document an existing\nbreakage in an early patch #1, and then make a fix and flip\nexpect-failure to expect-success in the patch #2.\n\nBreaking the code and documenting the breakage by expecting a\nfailure in one patch, and then later fixing the breakage and\nflipping the expectation in another patch, is a bit less nice,\nthough ;-)\n"},{"id":"246617","messageId":"xmqqy4vjbtnk.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"1406140978-9472-5-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 4/7] add line number and file name info to `config_set`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T22:24:31Z","receivedAt":"2014-07-23T22:24:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> @@ -1287,6 +1300,8 @@ 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 key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n> +\tstruct string_list_item *si;\n\nI have this suspicion that we may want to eventually build a custom\n\"config_value_list\" API that is very similar to what string_list\ndoes, with the only difference from string_list being that the util\nitem of config_value_item (i.e. a parallel to string_list_item)\nwould not be a \"void *\" that points at anything, but is \"struct\nkey_value_info\" embedded within, so that we do not have to waste a\npointer and fragmented allocation.\n\nI suspect such a config_value_list API must be built on top of a\nkind of generics which C does not allow, which would mean we would\nbe doing some preprocessor macro tricks (similar to the way how\ncommit-slab.h allows different kinds of payload) that lets us build\na templated \"string-list\" API, discarding the existing\n\"string-list.[ch]\" and replacing them with something like these two\nliners:\n\n declare_generic_string_list(string_list, void *); /* in string-list.h */\n define_generic_string_list(string_list, void *); /* in string-list.c */\n\nAnd at that point,\n\n declare_generic_string_list(config_value_list, struct key_value);\n define_generic_string_list(config_value_list, struct key_value);\n\nwould give us an API declaration and implementation that parallel\nthat of string-list, but with \"struct key_value\" in the util field\nof each item.\n\nBut that kind of clean-up can come much later after this series\nsettles, and what this patch has is fine for now.\n"},{"id":"246619","messageId":"xmqqtx67bt78.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"1406140978-9472-6-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T22:34:19Z","receivedAt":"2014-07-23T22:34:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> t4018-diff-funcname.sh fails for the new `git_config()` which uses the\n> configuration files caching layer internally.\n> The test introduced in commit d64d6cdc checks that whether `xfuncname` takes\n> precedence over `funcname` variable which was not guaranteed by config API\n> previously and worked only because values were parsed and fed in order.\n\nThe above is hard to understand. Do you mean that the old code and\ntest used whichever (between funcname and xfuncname) appeared last\nin the configuration file?\n\nFor example, if I had\n\n\tdiff.foo.xfuncname \"Beer$\"\n\nin my ~/.gitconfig and then for a particular project I wanted to use\na custom pattern in its .git/config but it was sufficient to define\nthe pattern without using extended regexp, I would be tempted to say\n\n\tdiff.foo.funcname \"Wine$\"\n\nWith the \"last one of xfuncname or funcname wins\" rule, I do not\nhave to worry about having xfuncname in ~/.gitconfig when I do so,\nbut with \"xfuncname is stronger than funcname\" rule, it suddenly\nforces me to go back and check if I have something stronger in my\nlower-precedence configuration file (i.e. ~/.gitconfig) when I am\ntrying to override with a higher-precedence configuration file\n(i.e. the project-specific .git/config).\n\nIf that is what this patch is trying to change, I am not sure if I\nagree that is a good change. They are two ways to spell the same\ninformation, and I would find it more natural if a later funcname\nspelled in BRE allowed me to override an earlier xfuncname spelled\nin ERE.\n"},{"id":"246632","messageId":"vpqlhrjqmyw.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"xmqqtx67bt78.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T06:39:51Z","receivedAt":"2014-07-24T06:39:51Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> in my ~/.gitconfig and then for a particular project I wanted to use\n> a custom pattern in its .git/config but it was sufficient to define\n> the pattern without using extended regexp, I would be tempted to say\n>\n> \tdiff.foo.funcname \"Wine$\"\n\nThe point is: to do that, you need to know that funcname exists, hence\nread archives of the doc before 2008 (it disapeared in\n45d9414fa5599b41578625961b53e18a9b9148c7) or read the code. And if I\nread correctly the commit message for the commit, funcname is not\nportable so you should use xfuncname anyway.\n\nThe real reason for this change is to let the code work with the new\ngit_config, but I think it's reasonable not to bother with the original\nbehavior for a feature deprecated so long ago.\n\nThat said, I think the commit message should be improved (recall the\ndrawbacks of funcname).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246633","messageId":"vpqfvhrqmsm.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"xmqq7g33d9fe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/7] rewrite git_config() to use the config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T06:43:37Z","receivedAt":"2014-07-24T06:43:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\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> Perhaps if you define this function at the right place in the file\n> you do not have to add an forward decl of git_config_check_init()?\n\nOTOH, having git_config right next to git_config_raw and configset_iter\nis a good thing, and keeping git_config_raw where git_config used to be\nseems logical.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246637","messageId":"53D10B37.3000403@gmail.com","threadId":"37199","inReplyTo":"xmqqegxbd9u0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/7] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T13:33:43Z","receivedAt":"2014-07-24T13:33:43Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/24/2014 3:19 AM, Junio C Hamano wrote:\n> Tanay Abhra writes:\n> \n>> If a callback returns a negative value to `git_config*()` family,\n>> they call `die()` while printing the line number and the file name.\n>> Currently the printed line number is off by one, thus printing the\n>> wrong line number.\n>>\n>> Make `linenr` point to the line we just parsed during the call\n>> to callback to get accurate line number in error messages.\n>>\n>> Discovered-by: Tanay Abhra \n>> Signed-off-by: Matthieu Moy \n> \n> Thanks.\n> \n> I am not sure what to read in these two lines. Was the fix done by\n> you or Matthieu?\n>\n\nI misunderstood the meaning of the message trailers. I will correct\nit in the next re roll.\n"},{"id":"246639","messageId":"53D11077.10907@gmail.com","threadId":"37199","inReplyTo":"xmqq38drd8ue.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/7] add a test for semantic errors in config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T13:56:07Z","receivedAt":"2014-07-24T13:56:07Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/24/2014 3:41 AM, Junio C Hamano wrote:\n> Matthieu Moy writes:\n> \n>> Tanay Abhra writes:\n>>\n>>> +test_expect_success 'check line errors for malformed values' '\n>>> +\tcp .git/config .git/config.old &&\n> \n> Should this be \"mv\" not \"cp\"? You will be overwriting the file from\n> scratch in the later part of this test.\n>\n\nNoted.\n\n>>> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n>>> +\techo \"[alias]\\n br\" >.git/config &&\n> \n> Is the use of \\n portable?\n> \n\nYes, you are right, will replace with printf in the next patch.\n"},{"id":"246640","messageId":"vpqha26kg2d.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"53D11077.10907@gmail.com","subject":"Re: [PATCH 3/7] add a test for semantic errors in config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T14:05:30Z","receivedAt":"2014-07-24T14:05:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n>>>> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n>>>> +\techo \"[alias]\\n br\" >.git/config &&\n>> \n>> Is the use of \\n portable?\n>> \n>\n> Yes, you are right, will replace with printf in the next patch.\n\n... or a cat >.git/config <<\\EOF, since this is the construct used\nelsewhere in the script.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246642","messageId":"53D12078.2070004@gmail.com","threadId":"37199","inReplyTo":"xmqqiomnda1y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T15:04:24Z","receivedAt":"2014-07-24T15:04:24Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"> \n> Are you done with the original series, or do you still want to fix\n> the const-ness issue with the string pointer before working on\n> follow-up topics like this one?\n>\n\nI am attaching the v12 with two new functions git_configset_get_string() &\ngit_configset_get_string_const().\n\nDiff between v11 and v12 is appended below for easy review.\n\n-- 8< --\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 8a86e45..815c1ee 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -138,13 +138,18 @@ as well as retrieval for the queried variable, including:\n \tSimilar to `git_config_get_bool`, except that it returns -1 on error\n \trather than dying.\n\n-`int git_config_get_string(const char *key, const char **dest)`::\n+`int git_config_get_string_const(const char *key, const char **dest)`::\n\n \tAllocates and copies the retrieved string into the `dest` parameter for\n \tthe configuration variable `key`; if NULL string is given, prints an\n \terror message and returns -1. When the configuration variable `key` is\n \tnot found, returns 1 without touching `dest`.\n\n+`int git_config_get_string(const char *key, char **dest)`::\n+\n+\tSimilar to `git_config_get_string_const`, except that retrieved value\n+\tcopied into the `dest` parameter is a mutable string.\n+\n `int git_config_get_pathname(const char *key, const char **dest)`::\n\n \tSimilar to `git_config_get_string`, but expands `~` or `~user` into\ndiff --git a/cache.h b/cache.h\nindex 2f63fd1..7292aef 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1361,7 +1361,8 @@ extern int git_configset_add_file(struct config_set *cs, const char *filename);\n extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n extern void git_configset_clear(struct config_set *cs);\n-extern int git_configset_get_string(struct config_set *cs, const char *key, const char **dest);\n+extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n+extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);\n extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n@@ -1373,7 +1374,8 @@ extern int git_config_get_value(const char *key, const char **value);\n extern const struct string_list *git_config_get_value_multi(const char *key);\n extern void git_config_clear(void);\n extern void git_config_iter(config_fn_t fn, void *data);\n-extern int git_config_get_string(const char *key, const char **dest);\n+extern int git_config_get_string_const(const char *key, const char **dest);\n+extern int git_config_get_string(const char *key, char **dest);\n extern int git_config_get_int(const char *key, int *dest);\n extern int git_config_get_ulong(const char *key, unsigned long *dest);\n extern int git_config_get_bool(const char *key, int *dest);\ndiff --git a/config.c b/config.c\nindex 22971e9..0d799e0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1332,7 +1332,7 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c\n \treturn e ? &e->value_list : NULL;\n }\n\n-int git_configset_get_string(struct config_set *cs, const char *key, const char **dest)\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@@ -1341,6 +1341,19 @@ int git_configset_get_string(struct config_set *cs, const char *key, const char\n \t\treturn 1;\n }\n\n+int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\t*dest = xstrdup(value);\n+\t\treturn 0;\n+\t}\n+\telse\n+\t\treturn 1;\n+}\n+\n int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n {\n \tconst char *value;\n@@ -1430,7 +1443,13 @@ const struct string_list *git_config_get_value_multi(const char *key)\n \treturn git_configset_get_value_multi(&the_config_set, key);\n }\n\n-int git_config_get_string(const char *key, const char **dest)\n+int git_config_get_string_const(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_string_const(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_string(const char *key, char **dest)\n {\n \tgit_config_check_init();\n \treturn git_configset_get_string(&the_config_set, key, dest);\n-- 8< --\n"},{"id":"246643","messageId":"53D120FE.8000809@gmail.com","threadId":"37199","inReplyTo":"xmqqiomnda1y.fsf@gitster.dls.corp.google.com","subject":"[PATCH v12 1/2] add `config_set` API for caching config-like files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T15:06:38Z","receivedAt":"2014-07-24T15:06:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Currently `git_config()` uses a callback mechanism and file rereads for\nconfig values. Due to this approach, it is not uncommon for the config\nfiles to be parsed several times during the run of a git program, with\ndifferent callbacks picking out different variables useful to themselves.\n\nAdd a `config_set`, that can be used to construct an in-memory cache for\nconfig-like files that the caller specifies (i.e., files like `.gitmodules`,\n`~/.gitconfig` etc.). Add two external functions `git_configset_get_value`\nand `git_configset_get_value_multi` for querying from the config sets.\n`git_configset_get_value` follows `last one wins` semantic (i.e. if there\nare multiple matches for the queried key in the files of the configset the\nvalue returned will be the last entry in `value_list`).\n`git_configset_get_value_multi` returns a list of values sorted in order of\nincreasing priority (i.e. last match will be at the end of the list). Add\ntype specific query functions like `git_configset_get_bool` and similar.\n\nAdd a default `config_set`, `the_config_set` to cache all key-value pairs\nread from usual config files (repo specific .git/config, user wide\n~/.gitconfig, XDG config and the global /etc/gitconfig). `the_config_set`\nis populated using `git_config()`.\n\nAdd two external functions `git_config_get_value` and\n`git_config_get_value_multi` for querying in a non-callback manner from\n`the_config_set`. Also, add type specific query functions that are\nimplemented as a thin wrapper around the `config_set` API.\n\nSigned-off-by: Matthieu Moy \nSigned-off-by: Tanay Abhra \n---\n Documentation/technical/api-config.txt | 142 +++++++++++++++++\n cache.h | 32 ++++\n config.c | 282 +++++++++++++++++++++++++++++++++\n setup.c | 9 ++\n 4 files changed, 465 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..815c1ee 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,86 @@ To read a specific file in git-config format, use\n `git_config_from_file`. This takes the same callback and data parameters\n as `git_config`.\n\n+Querying For Specific Variables\n+-------------------------------\n+\n+For programs wanting to query for specific variables in a non-callback\n+manner, the config API provides two functions `git_config_get_value`\n+and `git_config_get_value_multi`. They both read values from an internal\n+cache generated previously from reading the config files.\n+\n+`int git_config_get_value(const char *key, const char **value)`::\n+\n+\tFinds the highest-priority value for the configuration variable `key`,\n+\tstores the pointer to it in `value` and returns 0. When the\n+\tconfiguration variable `key` is not found, returns 1 without touching\n+\t`value`. The caller should not free or modify `value`, as it is owned\n+\tby the cache.\n+\n+`const struct string_list *git_config_get_value_multi(const char *key)`::\n+\n+\tFinds and returns the value list, sorted in order of increasing priority\n+\tfor the configuration variable `key`. When the configuration variable\n+\t`key` is not found, returns NULL. The caller should not free or modify\n+\tthe returned pointer, as it is owned by the cache.\n+\n+`void git_config_clear(void)`::\n+\n+\tResets and invalidates the config cache.\n+\n+The config API also provides type specific API functions which do conversion\n+as well as retrieval for the queried variable, including:\n+\n+`int git_config_get_int(const char *key, int *dest)`::\n+\n+\tFinds and parses the value to an integer for the configuration variable\n+\t`key`. Dies on error; otherwise, stores the value of the parsed integer in\n+\t`dest` and returns 0. When the configuration variable `key` is not found,\n+\treturns 1 without touching `dest`.\n+\n+`int git_config_get_ulong(const char *key, unsigned long *dest)`::\n+\n+\tSimilar to `git_config_get_int` but for unsigned longs.\n+\n+`int git_config_get_bool(const char *key, int *dest)`::\n+\n+\tFinds and parses the value into a boolean value, for the configuration\n+\tvariable `key` respecting keywords like \"true\" and \"false\". Integer\n+\tvalues are converted into true/false values (when they are non-zero or\n+\tzero, respectively). Other values cause a die(). If parsing is successful,\n+\tstores the value of the parsed result in `dest` and returns 0. When the\n+\tconfiguration variable `key` is not found, returns 1 without touching\n+\t`dest`.\n+\n+`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`::\n+\n+\tSimilar to `git_config_get_bool`, except that integers are copied as-is,\n+\tand `is_bool` flag is unset.\n+\n+`int git_config_get_maybe_bool(const char *key, int *dest)`::\n+\n+\tSimilar to `git_config_get_bool`, except that it returns -1 on error\n+\trather than dying.\n+\n+`int git_config_get_string_const(const char *key, const char **dest)`::\n+\n+\tAllocates and copies the retrieved string into the `dest` parameter for\n+\tthe configuration variable `key`; if NULL string is given, prints an\n+\terror message and returns -1. When the configuration variable `key` is\n+\tnot found, returns 1 without touching `dest`.\n+\n+`int git_config_get_string(const char *key, char **dest)`::\n+\n+\tSimilar to `git_config_get_string_const`, except that retrieved value\n+\tcopied into the `dest` parameter is a mutable string.\n+\n+`int git_config_get_pathname(const char *key, const char **dest)`::\n+\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+See test-config.c for usage examples.\n+\n Value Parsing Helpers\n ---------------------\n\n@@ -134,6 +214,68 @@ int read_file_with_include(const char *file, config_fn_t fn, void *data)\n `git_config` respects includes automatically. The lower-level\n `git_config_from_file` does not.\n\n+Custom Configsets\n+-----------------\n+\n+A `config_set` can be used to construct an in-memory cache for\n+config-like files that the caller specifies (i.e., files like `.gitmodules`,\n+`~/.gitconfig` etc.). For example,\n+\n+---------------------------------------\n+struct config_set gm_config;\n+git_configset_init(&gm_config);\n+int b;\n+/* we add config files to the config_set */\n+git_configset_add_file(&gm_config, \".gitmodules\");\n+git_configset_add_file(&gm_config, \".gitmodules_alt\");\n+\n+if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\", &b)) {\n+\t/* hack hack hack */\n+}\n+\n+/* when we are done with the configset */\n+git_configset_clear(&gm_config);\n+----------------------------------------\n+\n+Configset API provides functions for the above mentioned work flow, including:\n+\n+`void git_configset_init(struct config_set *cs)`::\n+\n+\tInitializes the config_set `cs`.\n+\n+`int git_configset_add_file(struct config_set *cs, const char *filename)`::\n+\n+\tParses the file and adds the variable-value pairs to the `config_set`,\n+\tdies if there is an error in parsing the file. Returns 0 on success, or\n+\t-1 if the file does not exist or is inaccessible. The user has to decide\n+\tif he wants to free the incomplete configset or continue using it when\n+\tthe function returns -1.\n+\n+`int git_configset_get_value(struct config_set *cs, const char *key, const char **value)`::\n+\n+\tFinds the highest-priority value for the configuration variable `key`\n+\tand config set `cs`, stores the pointer to it in `value` and returns 0.\n+\tWhen the configuration variable `key` is not found, returns 1 without\n+\ttouching `value`. The caller should not free or modify `value`, as it\n+\tis owned by the cache.\n+\n+`const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)`::\n+\n+\tFinds and returns the value list, sorted in order of increasing priority\n+\tfor the configuration variable `key` and config set `cs`. When the\n+\tconfiguration variable `key` is not found, returns NULL. The caller\n+\tshould not free or modify the returned pointer, as it is owned by the cache.\n+\n+`void git_configset_clear(struct config_set *cs)`::\n+\n+\tClears `config_set` structure, removes all saved variable-value pairs.\n+\n+In addition to above functions, the `config_set` API provides type specific\n+functions in the vein of `git_config_get_int` and family but with an extra\n+parameter, pointer to struct `config_set`.\n+They all behave similarly to the `git_config_get*()` family described in\n+\"Querying For Specific Variables\" above.\n+\n Writing Config Files\n --------------------\n\ndiff --git a/cache.h b/cache.h\nindex fcb511d..7292aef 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1351,6 +1351,38 @@ 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 {\n+\tstruct hashmap config_hash;\n+\tint hash_initialized;\n+};\n+\n+extern void git_configset_init(struct config_set *cs);\n+extern int git_configset_add_file(struct config_set *cs, const char *filename);\n+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n+extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n+extern void git_configset_clear(struct config_set *cs);\n+extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);\n+extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);\n+extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);\n+extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);\n+extern int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);\n+\n+extern int git_config_get_value(const char *key, const char **value);\n+extern const struct string_list *git_config_get_value_multi(const char *key);\n+extern void git_config_clear(void);\n+extern void git_config_iter(config_fn_t fn, void *data);\n+extern int git_config_get_string_const(const char *key, const char **dest);\n+extern int git_config_get_string(const char *key, char **dest);\n+extern int git_config_get_int(const char *key, int *dest);\n+extern int git_config_get_ulong(const char *key, unsigned long *dest);\n+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+\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 9767c4b..0d799e0 100644\n--- a/config.c\n+++ b/config.c\n@@ -9,6 +9,8 @@\n #include \"exec_cmd.h\"\n #include \"strbuf.h\"\n #include \"quote.h\"\n+#include \"hashmap.h\"\n+#include \"string-list.h\"\n\n struct config_source {\n \tstruct config_source *prev;\n@@ -33,10 +35,23 @@ 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\n+/*\n+ * Default config_set that contains key-value pairs from the usual set of config\n+ * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG\n+ * config file and the global /etc/gitconfig)\n+ */\n+static struct config_set the_config_set;\n+\n static int config_file_fgetc(struct config_source *conf)\n {\n \treturn fgetc(conf->u.file);\n@@ -1212,6 +1227,270 @@ int git_config(config_fn_t fn, void *data)\n \treturn git_config_with_options(fn, data, NULL, 1);\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+\tstruct config_set_element *found_entry;\n+\tchar *normalized_key;\n+\tint ret;\n+\t/*\n+\t * `key` may come from the user, so normalize it before using it\n+\t * for querying entries from the hashmap.\n+\t */\n+\tret = git_config_parse_key(key, &normalized_key, NULL);\n+\n+\tif (ret)\n+\t\treturn NULL;\n+\n+\thashmap_entry_init(&k, strhash(normalized_key));\n+\tk.key = normalized_key;\n+\tfound_entry = hashmap_get(&cs->config_hash, &k, NULL);\n+\tfree(normalized_key);\n+\treturn found_entry;\n+}\n+\n+static int configset_add_value(struct config_set *cs, const char *key, const char *value)\n+{\n+\tstruct config_set_element *e;\n+\te = configset_find_element(cs, key);\n+\t/*\n+\t * Since the keys are being fed by git_config*() callback mechanism, they\n+\t * are already normalized. So simply add them without any further munging.\n+\t */\n+\tif (!e) {\n+\t\te = xmalloc(sizeof(*e));\n+\t\thashmap_entry_init(e, strhash(key));\n+\t\te->key = xstrdup(key);\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+\n+\treturn 0;\n+}\n+\n+static int config_set_element_cmp(const struct config_set_element *e1,\n+\t\t\t\t const struct config_set_element *e2, const void *unused)\n+{\n+\treturn strcmp(e1->key, e2->key);\n+}\n+\n+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+}\n+\n+void git_configset_clear(struct config_set *cs)\n+{\n+\tstruct config_set_element *entry;\n+\tstruct hashmap_iter iter;\n+\tif (!cs->hash_initialized)\n+\t\treturn;\n+\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}\n+\thashmap_free(&cs->config_hash, 1);\n+\tcs->hash_initialized = 0;\n+}\n+\n+static int config_set_callback(const char *key, const char *value, void *cb)\n+{\n+\tstruct config_set *cs = cb;\n+\tconfigset_add_value(cs, key, value);\n+\treturn 0;\n+}\n+\n+int git_configset_add_file(struct config_set *cs, const char *filename)\n+{\n+\treturn git_config_from_file(config_set_callback, filename, cs);\n+}\n+\n+int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n+{\n+\tconst struct string_list *values = NULL;\n+\t/*\n+\t * Follows \"last one wins\" semantic, i.e., if there are multiple matches for the\n+\t * queried key in the files of the configset, the value returned will be the last\n+\t * value in the value list for that key.\n+\t */\n+\tvalues = git_configset_get_value_multi(cs, key);\n+\n+\tif (!values)\n+\t\treturn 1;\n+\tassert(values->nr > 0);\n+\t*value = values->items[values->nr - 1].string;\n+\treturn 0;\n+}\n+\n+const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n+{\n+\tstruct config_set_element *e = configset_find_element(cs, key);\n+\treturn e ? &e->value_list : NULL;\n+}\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+}\n+\n+int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(key);\n+\t\t*dest = xstrdup(value);\n+\t\treturn 0;\n+\t}\n+\telse\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_int(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_ulong(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_bool(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_bool(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_bool_or_int(struct config_set *cs, const char *key,\n+\t\t\t\tint *is_bool, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_bool_or_int(key, value, is_bool);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_maybe_bool(key, value);\n+\t\tif (*dest == -1)\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_pathname(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_pathname(dest, key, value);\n+\telse\n+\t\treturn 1;\n+}\n+\n+static void git_config_check_init(void)\n+{\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+}\n+\n+void git_config_clear(void)\n+{\n+\tif (!the_config_set.hash_initialized)\n+\t\treturn;\n+\tgit_configset_clear(&the_config_set);\n+}\n+\n+int git_config_get_value(const char *key, const char **value)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_value(&the_config_set, key, value);\n+}\n+\n+const struct string_list *git_config_get_value_multi(const char *key)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_value_multi(&the_config_set, key);\n+}\n+\n+int git_config_get_string_const(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_string_const(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_string(const char *key, char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_string(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_int(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_int(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_ulong(const char *key, unsigned long *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_ulong(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_bool(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_bool(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_bool_or_int(&the_config_set, key, is_bool, dest);\n+}\n+\n+int git_config_get_maybe_bool(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_maybe_bool(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_pathname(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_pathname(&the_config_set, key, dest);\n+}\n+\n /*\n * Find all the stuff for git_config_set() below.\n */\n@@ -1707,6 +1986,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tlock = NULL;\n \tret = 0;\n\n+\t/* Invalidate the config cache */\n+\tgit_config_clear();\n+\n out_free:\n \tif (lock)\n \t\trollback_lock_file(lock);\ndiff --git a/setup.c b/setup.c\nindex 0a22f8b..793369d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -625,6 +625,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n \tint one_filesystem = 1;\n\n \t/*\n+\t * We may have read an incomplete configuration before\n+\t * setting-up the git directory. If so, clear the cache so\n+\t * that the next queries to the configuration reload complete\n+\t * configuration (including the per-repo config file that we\n+\t * ignored previously).\n+\t */\n+\tgit_config_clear();\n+\n+\t/*\n \t * Let's assume that we are in a git repository.\n \t * If it turns out later that we are somewhere else, the value will be\n \t * updated accordingly.\n-- \n1.9.0.GIT\n"},{"id":"246644","messageId":"53D121B9.2020103@gmail.com","threadId":"37199","inReplyTo":"xmqqiomnda1y.fsf@gitster.dls.corp.google.com","subject":"[PATCH v12 2/2] test-config: add tests for the config_set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T15:09:45Z","receivedAt":"2014-07-24T15:09:45Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Expose the `config_set` C API as a set of simple commands in order to\nfacilitate testing. Add tests for the `config_set` API as well as for\n`git_config_get_*()` family for the usual config files.\n\nSigned-off-by: Matthieu Moy \nSigned-off-by: Tanay Abhra \n---\n .gitignore | 1 +\n Makefile | 1 +\n t/t1308-config-set.sh | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++\n test-config.c | 142 +++++++++++++++++++++++++++++++++++\n 4 files changed, 344 insertions(+)\n create mode 100755 t/t1308-config-set.sh\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 81e12c0..5bfb234 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -178,6 +178,7 @@\n /gitweb/static/gitweb.min.*\n /test-chmtime\n /test-ctype\n+/test-config\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/Makefile b/Makefile\nindex 2320de5..b7462e3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -551,6 +551,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n\n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nnew file mode 100755\nindex 0000000..7fdf840\n--- /dev/null\n+++ b/t/t1308-config-set.sh\n@@ -0,0 +1,200 @@\n+#!/bin/sh\n+\n+test_description='Test git config-set API in different settings'\n+\n+. ./test-lib.sh\n+\n+# 'check_config get_* section.key value' verifies that the entry for\n+# section.key is 'value'\n+check_config () {\n+\tif test \"$1\" = expect_code\n+\tthen\n+\t\texpect_code=\"$2\" && shift && shift\n+\telse\n+\t\texpect_code=0\n+\tfi &&\n+\top=$1 key=$2 && shift && shift &&\n+\tif test $# != 0\n+\tthen\n+\t\tprintf \"%s\\n\" \"$@\"\n+\tfi >expect &&\n+\ttest_expect_code $expect_code test-config \"$op\" \"$key\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'setup default config' '\n+\tcat >.git/config <<\\EOF\n+\t[case]\n+\t\tpenguin = very blue\n+\t\tMovie = BadPhysics\n+\t\tUPPERCASE = true\n+\t\tMixedCase = true\n+\t\tmy =\n+\t\tfoo\n+\t\tbaz = sam\n+\t[Cores]\n+\t\tWhatEver = Second\n+\t\tbaz = bar\n+\t[cores]\n+\t\tbaz = bat\n+\t[CORES]\n+\t\tbaz = ball\n+\t[my \"Foo bAr\"]\n+\t\thi = mixed-case\n+\t[my \"FOO BAR\"]\n+\t\thi = upper-case\n+\t[my \"foo bar\"]\n+\t\thi = lower-case\n+\t[case]\n+\t\tbaz = bat\n+\t\tbaz = hask\n+\t[lamb]\n+\t\tchop = 65\n+\t\thead = none\n+\t[goat]\n+\t\tlegs = 4\n+\t\thead = true\n+\t\tskin = false\n+\t\tnose = 1\n+\t\thorns\n+\tEOF\n+'\n+\n+test_expect_success 'get value for a simple key' '\n+\tcheck_config get_value case.penguin \"very blue\"\n+'\n+\n+test_expect_success 'get value for a key with value as an empty string' '\n+\tcheck_config get_value case.my \"\"\n+'\n+\n+test_expect_success 'get value for a key with value as NULL' '\n+\tcheck_config get_value case.foo \"(NULL)\"\n+'\n+\n+test_expect_success 'upper case key' '\n+\tcheck_config get_value case.UPPERCASE \"true\" &&\n+\tcheck_config get_value case.uppercase \"true\"\n+'\n+\n+test_expect_success 'mixed case key' '\n+\tcheck_config get_value case.MixedCase \"true\" &&\n+\tcheck_config get_value case.MIXEDCASE \"true\" &&\n+\tcheck_config get_value case.mixedcase \"true\"\n+'\n+\n+test_expect_success 'key and value with mixed case' '\n+\tcheck_config get_value case.Movie \"BadPhysics\"\n+'\n+\n+test_expect_success 'key with case sensitive subsection' '\n+\tcheck_config get_value \"my.Foo bAr.hi\" \"mixed-case\" &&\n+\tcheck_config get_value \"my.FOO BAR.hi\" \"upper-case\" &&\n+\tcheck_config get_value \"my.foo bar.hi\" \"lower-case\"\n+'\n+\n+test_expect_success 'key with case insensitive section header' '\n+\tcheck_config get_value cores.baz \"ball\" &&\n+\tcheck_config get_value Cores.baz \"ball\" &&\n+\tcheck_config get_value CORES.baz \"ball\" &&\n+\tcheck_config get_value coreS.baz \"ball\"\n+'\n+\n+test_expect_success 'key with case insensitive section header & variable' '\n+\tcheck_config get_value CORES.BAZ \"ball\" &&\n+\tcheck_config get_value cores.baz \"ball\" &&\n+\tcheck_config get_value cores.BaZ \"ball\" &&\n+\tcheck_config get_value cOreS.bAz \"ball\"\n+'\n+\n+test_expect_success 'find value with misspelled key' '\n+\tcheck_config expect_code 1 get_value \"my.fOo Bar.hi\" \"Value not found for \\\"my.fOo Bar.hi\\\"\"\n+'\n+\n+test_expect_success 'find value with the highest priority' '\n+\tcheck_config get_value case.baz \"hask\"\n+'\n+\n+test_expect_success 'find integer value for a key' '\n+\tcheck_config get_int lamb.chop 65\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+'\n+\n+test_expect_success 'find bool value for the entered key' '\n+\tcheck_config get_bool goat.head 1 &&\n+\tcheck_config get_bool goat.skin 0 &&\n+\tcheck_config get_bool goat.nose 1 &&\n+\tcheck_config get_bool goat.horns 1 &&\n+\tcheck_config get_bool goat.legs 1\n+'\n+\n+test_expect_success 'find multiple values' '\n+\tcheck_config get_value_multi case.baz sam bat hask\n+'\n+\n+test_expect_success 'find value from a configset' '\n+\tcat >config2 <<-\\EOF &&\n+\t[case]\n+\t\tbaz = lama\n+\t[my]\n+\t\tnew = silk\n+\t[case]\n+\t\tbaz = ball\n+\tEOF\n+\techo silk >expect &&\n+\ttest-config configset_get_value my.new config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value with highest priority from a configset' '\n+\techo hask >expect &&\n+\ttest-config configset_get_value case.baz config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value_list for a key from a configset' '\n+\tcat >except <<-\\EOF &&\n+\tsam\n+\tbat\n+\thask\n+\tlama\n+\tball\n+\tEOF\n+\ttest-config configset_get_value case.baz config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'proper error on non-existent files' '\n+\techo \"Error (-1) reading configuration file non-existent-file.\" >expect &&\n+\ttest_expect_code 2 test-config configset_get_value foo.bar non-existent-file 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '\n+\tchmod -r .git/config &&\n+\ttest_when_finished \"chmod +r .git/config\" &&\n+\techo \"Error (-1) reading configuration file .git/config.\" >expect &&\n+\ttest_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'proper error on error in default config files' '\n+\tcp .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\techo \"[\" >>.git/config &&\n+\techo \"fatal: bad config file line 35 in .git/config\" >expect &&\n+\ttest_expect_code 128 test-config get_value foo.bar 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'proper error on error in custom config files' '\n+\techo \"[\" >>syntax-error &&\n+\techo \"fatal: bad config file line 1 in syntax-error\" >expect &&\n+\ttest_expect_code 128 test-config configset_get_value foo.bar syntax-error 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..9dd1b22\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,142 @@\n+#include \"cache.h\"\n+#include \"string-list.h\"\n+\n+/*\n+ * This program exposes the C API of the configuration mechanism\n+ * as a set of simple commands in order to facilitate testing.\n+ *\n+ * Reads stdin and prints result of command to stdout:\n+ *\n+ * get_value -> prints the value with highest priority for the entered key\n+ *\n+ * get_value_multi -> prints all values for the entered key in increasing order\n+ *\t\t of priority\n+ *\n+ * get_int -> print integer value for the entered key or die\n+ *\n+ * get_bool -> print bool 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+ * configset_get_value_multi -> returns value_list for the entered key sorted in\n+ * \t\t\t\tascending order of priority from a config_set\n+ * \t\t\t\tconstructed from files entered as arguments.\n+ *\n+ * Examples:\n+ *\n+ * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n+ * \ttest-config get_value \"foo.bAr Baz.rock\"\n+ *\n+ */\n+\n+\n+int main(int argc, char **argv)\n+{\n+\tint i, val;\n+\tconst char *v;\n+\tconst struct string_list *strptr;\n+\tstruct config_set cs;\n+\tgit_configset_init(&cs);\n+\n+\tif (argc < 2) {\n+\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n+\t\tgoto exit1;\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n+\t\tif (!git_config_get_value(argv[2], &v)) {\n+\t\t\tif (!v)\n+\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\telse\n+\t\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 (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n+\t\tstrptr = git_config_get_value_multi(argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tif (!v)\n+\t\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\t}\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 (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n+\t\tif (!git_config_get_int(argv[2], &val)) {\n+\t\t\tprintf(\"%d\\n\", val);\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 (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n+\t\tif (!git_config_get_bool(argv[2], &val)) {\n+\t\t\tprintf(\"%d\\n\", val);\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+\t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n+\t\t\t\tfprintf(stderr, \"Error (%d) reading configuration file %s.\\n\", err, argv[i]);\n+\t\t\t\tgoto exit2;\n+\t\t\t}\n+\t\t}\n+\t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n+\t\t\tif (!v)\n+\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\telse\n+\t\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_multi\")) {\n+\t\tfor (i = 3; i < argc; i++) {\n+\t\t\tint err;\n+\t\t\tif ((err = git_configset_add_file(&cs, argv[i]))) {\n+\t\t\t\tfprintf(stderr, \"Error (%d) reading configuration file %s.\\n\", err, argv[i]);\n+\t\t\t\tgoto exit2;\n+\t\t\t}\n+\t\t}\n+\t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tif (!v)\n+\t\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\t}\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}\n+\n+\tdie(\"%s: Please check the syntax and the function name\", argv[0]);\n+\n+exit0:\n+\tgit_configset_clear(&cs);\n+\treturn 0;\n+\n+exit1:\n+\tgit_configset_clear(&cs);\n+\treturn 1;\n+\n+exit2:\n+\tgit_configset_clear(&cs);\n+\treturn 2;\n+}\n-- \n1.9.0.GIT\n"},{"id":"246645","messageId":"vpq1ttadauu.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"53D12078.2070004@gmail.com","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T15:39:53Z","receivedAt":"2014-07-24T15:39:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> I am attaching the v12 with two new functions git_configset_get_string() &\n> git_configset_get_string_const().\n\nDidn't you intend to change git_config_get_string to let it die in case\nof error instead of returning an error code?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246646","messageId":"53D12D56.2080505@gmail.com","threadId":"37199","inReplyTo":"vpq1ttadauu.fsf@anie.imag.fr","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T15:59:18Z","receivedAt":"2014-07-24T15:59:18Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/24/2014 9:09 PM, Matthieu Moy wrote:\n> Tanay Abhra writes:\n> \n>> I am attaching the v12 with two new functions git_configset_get_string() &\n>> git_configset_get_string_const().\n> \n> Didn't you intend to change git_config_get_string to let it die in case\n> of error instead of returning an error code?\n>\n\nYup, but it is the part of the new series not the old one.\nI sent this revised patch just for solving the constness\ndebate.\nAlso, I would need git_die_config() to print a proper\nerror message which comes in the 7 part series not this one.\n"},{"id":"246647","messageId":"vpqsilq7muy.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"53D12D56.2080505@gmail.com","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T16:17:09Z","receivedAt":"2014-07-24T16:17:09Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> On 7/24/2014 9:09 PM, Matthieu Moy wrote:\n>> Tanay Abhra writes:\n>> \n>>> I am attaching the v12 with two new functions git_configset_get_string() &\n>>> git_configset_get_string_const().\n>> \n>> Didn't you intend to change git_config_get_string to let it die in case\n>> of error instead of returning an error code?\n>>\n>\n> Yup, but it is the part of the new series not the old one.\n> I sent this revised patch just for solving the constness\n> debate.\n> Also, I would need git_die_config() to print a proper\n> error message which comes in the 7 part series not this one.\n\nAh, right, you don't have line numbers yet, so you can't die properly.\n\nSo, hopefully, this part can be considered done, and we can continue\nbuilding on top.\n\nAs a side effect, I guess Junio will apply this on top of master so the\nstring interning API will be available immediately at the tip of the\nbranch.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246650","messageId":"xmqqmwbyaevo.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpqha26kg2d.fsf@anie.imag.fr","subject":"Re: [PATCH 3/7] add a test for semantic errors in config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T16:41:15Z","receivedAt":"2014-07-24T16:41:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Tanay Abhra writes:\n>\n>>>>> +\ttest_when_finished \"mv .git/config.old .git/config\" &&\n>>>>> +\techo \"[alias]\\n br\" >.git/config &&\n>>> \n>>> Is the use of \\n portable?\n>>> \n>>\n>> Yes, you are right, will replace with printf in the next patch.\n>\n> ... or a cat >.git/config <<\\EOF, since this is the construct used\n> elsewhere in the script.\n\nThat sounds much better ;-) Thanks.\n"},{"id":"246651","messageId":"xmqqiommadl0.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpqlhrjqmyw.fsf@anie.imag.fr","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T17:09:15Z","receivedAt":"2014-07-24T17:09:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>> in my ~/.gitconfig and then for a particular project I wanted to use\n>> a custom pattern in its .git/config but it was sufficient to define\n>> the pattern without using extended regexp, I would be tempted to say\n>>\n>> \tdiff.foo.funcname \"Wine$\"\n>\n> The point is: to do that, you need to know that funcname exists, hence\n> read archives of the doc before 2008 (it disapeared in\n> 45d9414fa5599b41578625961b53e18a9b9148c7) or read the code. And if I\n> read correctly the commit message for the commit, funcname is not\n> portable so you should use xfuncname anyway.\n>\n> The real reason for this change is to let the code work with the new\n> git_config, but I think it's reasonable not to bother with the original\n> behavior for a feature deprecated so long ago.\n\nWe may be able to explain funcname vs xfuncname away, but the\nproblem is much deeper, and it is not just you and Tanay, but\nanybody who thought that it is a natural and logical conclusion\nto emulate git_config() by iterating over in-core hashtable X-<\n\nWe have assumed that it is OK for git_config() to call its callbacks\nwith any random order of keys as long as the values for the same key\nare given in the same order as the original \"read from file every\ntime\" implementation would have called. The assumption overlooked\nthe possibility that values given to two different keys can\nsemantically interact. $foo.funcname and $foo.xfuncname being two\ndifferent ways to define the same piece of information is merely one\nof the many ways such interactions can happen. For example, by\nassuming that orders of callbacks across keys does not matter, we\nwould be breaking scripts that expected to be able to deal with\nsomething like this:\n\n\t[character]\n \tname = Snoopy\n color = white black\n\n name = Hobbes\n color = yellow white black\n\nFor *our* own data, we can force them to be spelled this way instead:\n\n\t[character \"Snoopy\"]\n \tcolor = white black\n\t[character \"Hobbes\"]\n \tcolor = yellow white black\n\nbut that misses the point, as our config file format and its\nsemantics are not only about .git/config but the system is designed\nand promoted to be a way for third-party applications to store their\nown data as well.\n\nBecause we do not condone using libgit.a as a library to link into\nthird-party applications, git_config() at the C level may not have\nto read a random third-party data and call its callback in the\npredictable way, but this shows us that \"git config -l\" interface\nmust return the data in the same order as we have done always by\neither using the _raw interface to return things uncached, or\nteaching by git_config() to also be predicatable.\n\n> That said, I think the commit message should be improved (recall the\n> drawbacks of funcname).\n\nIn any case, I agree that the log message and the subject is wrong.\nIt is not that we want to give xfuncname precedence over funcname;\nsuch a behaviour is undesirable if both were in wide use. It is\npapering over the fallout that comes from the loss of ordering\ninformation, which is imposed by the data structure chosen to\nrepresent a config_set.\n\nI haven't formed a firm opinion whether preserving the order is\nnecessary at git_config() iteration level yet. If the only in-core\nuser that will be broken by not doing so is xfuncname vs funcname,\nit may not be too bad, but it will constrain us in the future, which\nis a lot bigger problem. We will be forbidden from correcting a UI\nmistake by using the approach we took to transtion \"funcname\" over\nto \"xfuncname\" (i.e. giving users \"funcname\" and allowing the\nplatform BRE parser that may or may not allow the users to write\nbackslashed ERE is spreading possible incompatibility among the\nparticipants of a project; explicitly stating that the value is ERE\nhas fewer compatibility issues), while still supporting \"funcname\"\nand have them interact in a sane way.\n"},{"id":"246655","messageId":"xmqq1ttaa9sa.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"53D10B37.3000403@gmail.com","subject":"Re: [PATCH 1/7] config.c: fix accuracy of line number in errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T18:31:17Z","receivedAt":"2014-07-24T18:31:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> On 7/24/2014 3:19 AM, Junio C Hamano wrote:\n>> Tanay Abhra writes:\n>> \n>>> If a callback returns a negative value to `git_config*()` family,\n>>> they call `die()` while printing the line number and the file name.\n>>> Currently the printed line number is off by one, thus printing the\n>>> wrong line number.\n>>>\n>>> Make `linenr` point to the line we just parsed during the call\n>>> to callback to get accurate line number in error messages.\n>>>\n>>> Discovered-by: Tanay Abhra \n>>> Signed-off-by: Matthieu Moy \n>> \n>> Thanks.\n>> \n>> I am not sure what to read in these two lines. Was the fix done by\n>> you or Matthieu?\n>>\n>\n> I misunderstood the meaning of the message trailers. I will correct\n> it in the next re roll.\n\nWell, that does not really answer my question, and it does not tell\nus if you now understand the meaning of trailers correctly, or\nreplaced an earlier misunderstanding with a different\nmisunderstanding ;-)\n\nMore importantly, it would not help me interpret what you mean when\nyou write a sequence like this:\n\n\tFrom: tanay\n S-o-b: matthieu\n S-o-b: tanay\n\nwhich does not quite make sense and leave the reader puzzled.\n\nIf the patch was authored by you and helped by matthieu, the first\nS-o-b would be lying. If the patch was written by matthieu and you\nare merely relaying, then the authorship From is lying. If the\npatch was written by you and matthieu is relaying, then the two\nS-o-bs are in incorrect order.\n"},{"id":"246656","messageId":"53D15188.2010209@gmail.com","threadId":"37199","inReplyTo":"xmqqiommadl0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T18:33:44Z","receivedAt":"2014-07-24T18:33:44Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n> \n> I haven't formed a firm opinion whether preserving the order is\n> necessary at git_config() iteration level yet. If the only in-core\n> user that will be broken by not doing so is xfuncname vs funcname,\n> it may not be too bad, but it will constrain us in the future, which\n> is a lot bigger problem.\n\nIf we give users a choice between two methods git_config_raw() &\ngit_config_cache(), it would give us a easy fallback if we want\nto do something like support xfuncname and funcname for performing\nthe same function differently.\n\nFor core the only test failing was xfuncname vs funcname,\nso the situation is not as bad as you think. One course of action\nwould be leave git_config() as it is, so that third party apps\nmay not be broken. Provide a function like git_config_cache(),\nthen rename all the git_config() calls in core to git_config_cache(),\nfallback to git_config() where it is not applicable (for example,\ngit config -l). I haven't thought a lot about it and I haven't\nheard Matthieu's opinion about it yet.\n\nAlso can you name any third party apps that use the git_config()\nsystem on which I can test the patches. I have seen gitk and gitgui\nusing the config files to save data.\n\nCheers,\nTanay Abhra.\n\n We will be forbidden from correcting a UI\n> mistake by using the approach we took to transtion \"funcname\" over\n> to \"xfuncname\" (i.e. giving users \"funcname\" and allowing the\n> platform BRE parser that may or may not allow the users to write\n> backslashed ERE is spreading possible incompatibility among the\n> participants of a project; explicitly stating that the value is ERE\n> has fewer compatibility issues), while still supporting \"funcname\"\n> and have them interact in a sane way.\n> \n"},{"id":"246657","messageId":"53D15307.4060601@gmail.com","threadId":"37199","inReplyTo":"xmqq1ttaa9sa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/7] config.c: fix accuracy of line number in errors","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T18:40:07Z","receivedAt":"2014-07-24T18:40:07Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/25/2014 12:01 AM, Junio C Hamano wrote:\n> Tanay Abhra writes:\n> \n>> On 7/24/2014 3:19 AM, Junio C Hamano wrote:\n>>> Tanay Abhra writes:\n>>>\n>>>> If a callback returns a negative value to `git_config*()` family,\n>>>> they call `die()` while printing the line number and the file name.\n>>>> Currently the printed line number is off by one, thus printing the\n>>>> wrong line number.\n>>>>\n>>>> Make `linenr` point to the line we just parsed during the call\n>>>> to callback to get accurate line number in error messages.\n>>>>\n>>>> Discovered-by: Tanay Abhra \n>>>> Signed-off-by: Matthieu Moy \n\nThis patch should have looked like,\n\nFrom: Matthieu Moy \n\n\n\nCommit-message-by: Tanay Abhra \n\n>>>\n>>> Thanks.\n>>>\n>>> I am not sure what to read in these two lines. Was the fix done by\n>>> you or Matthieu?\n>>>\n>>\n>> I misunderstood the meaning of the message trailers. I will correct\n>> it in the next re roll.\n> \n> Well, that does not really answer my question, and it does not tell\n> us if you now understand the meaning of trailers correctly, or\n> replaced an earlier misunderstanding with a different\n> misunderstanding ;-)\n> \n> More importantly, it would not help me interpret what you mean when\n> you write a sequence like this:\n> \n> \tFrom: tanay\n> S-o-b: matthieu\n> S-o-b: tanay\n>\n\nActually it was because that patch contained fixup patch from Matthieu\nsquashed into it, that is why there were two signoffs.\n\n> which does not quite make sense and leave the reader puzzled.\n> \n> If the patch was authored by you and helped by matthieu, the first\n> S-o-b would be lying. If the patch was written by matthieu and you\n> are merely relaying, then the authorship From is lying. If the\n> patch was written by you and matthieu is relaying, then the two\n> S-o-bs are in incorrect order.\n> \n"},{"id":"246658","messageId":"xmqqwqb28uqg.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"53D120FE.8000809@gmail.com","subject":"Re: [PATCH v12 1/2] add `config_set` API for caching config-like files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T18:41:43Z","receivedAt":"2014-07-24T18:41:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\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> +}\n> +\n> +int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n> +{\n> +\tconst char *value;\n> +\tif (!git_configset_get_value(cs, key, &value)) {\n> +\t\tif (!value)\n> +\t\t\treturn config_error_nonbool(key);\n> +\t\t*dest = xstrdup(value);\n> +\t\treturn 0;\n> +\t}\n> +\telse\n> +\t\treturn 1;\n> +}\n\nHmm, do we really need duplicate and an almost identical\nimplementations?\n\nI would have expected either one of these two (expecting the latter\nmore than the former from the discussion):\n\n 1. Because git_configset_get_string_const() returns a const string\n not to be touched by the caller, it does not even xstrdup(),\n while git_configset_get_string() does.\n\n 2. These behave identically, and the only reason we have two is\n because some callers want to receive the string in \"char *\"\n while others want to use \"const char *\".\n\nIf you were going route #1, then you would need duplicate but subtly\ndifferent implementations; get_string_const() variant would not be\ncalling git_config_string() because it does xstrdup() the value and\ngive the caller a new string the caller has to free.\n\nIf you were going route #2, the I would have expected there is only\none implementation, _get_string(), and _get_string_const() would\ncall it while casting the dest parameter it receives.\n"},{"id":"246660","messageId":"vpqegxa386m.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"53D15188.2010209@gmail.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T18:47:45Z","receivedAt":"2014-07-24T18:47:45Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> For core the only test failing was xfuncname vs funcname,\n\nBeing a little pessimistic: there may be other cases where the hashtable\nmagically gives the right order for existing tests, but that would fail\nfor untested use-cases.\n\nBut I can't think of any such case.\n\n> so the situation is not as bad as you think. One course of action\n> would be leave git_config() as it is, so that third party apps\n> may not be broken. Provide a function like git_config_cache(),\n> then rename all the git_config() calls in core to git_config_cache(),\n> fallback to git_config() where it is not applicable (for example,\n> git config -l).\n\nI think Junio's point about \"git config -l\" is correct: we should just\nkeep git_config_raw there.\n\nOTOH, renaming git_config to git_config_cache seems a lot of code churn,\nso I'd keep the name git_config. Perhaps git_config_raw is no longer a\ngood name and it could be called git_config_ordered.\n\nIf we keep one call to git_config_raw there, then maybe we can use it\nfor xfuncname/funcname too, and keep the behavior unchanged.\n\n> Also can you name any third party apps that use the git_config()\n> system on which I can test the patches.\n\nThere are probably tons of. I can think of git-multimail.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246661","messageId":"vpqiomm1tjv.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"53D15307.4060601@gmail.com","subject":"Re: [PATCH 1/7] config.c: fix accuracy of line number in errors","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-24T18:49:08Z","receivedAt":"2014-07-24T18:49:08Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> On 7/25/2014 12:01 AM, Junio C Hamano wrote:\n>> Tanay Abhra writes:\n>> \n>>> On 7/24/2014 3:19 AM, Junio C Hamano wrote:\n>>>> Tanay Abhra writes:\n>>>>\n>>>>> If a callback returns a negative value to `git_config*()` family,\n>>>>> they call `die()` while printing the line number and the file name.\n>>>>> Currently the printed line number is off by one, thus printing the\n>>>>> wrong line number.\n>>>>>\n>>>>> Make `linenr` point to the line we just parsed during the call\n>>>>> to callback to get accurate line number in error messages.\n>>>>>\n>>>>> Discovered-by: Tanay Abhra \n>>>>> Signed-off-by: Matthieu Moy \n>\n> This patch should have looked like,\n>\n> From: Matthieu Moy \n>\n> \n>\n> Commit-message-by: Tanay Abhra \n\nYes, the code itself was mine, and Tanay wrote the commit message.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246662","messageId":"20140724190403.GF26927@serenity.lan","threadId":"37199","inReplyTo":"vpqegxa386m.fsf@anie.imag.fr","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-07-24T19:04:03Z","receivedAt":"2014-07-24T19:04:03Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Jul 24, 2014 at 08:47:45PM +0200, Matthieu Moy wrote:\n> Tanay Abhra writes:\n> > Also can you name any third party apps that use the git_config()\n> > system on which I can test the patches.\n> \n> There are probably tons of. I can think of git-multimail.\n\nCGit [1] is probably the only one that links with libgit.a and uses the\ninternal C API.\n\n[1] http://git.zx2c4.com/cgit/\n"},{"id":"246663","messageId":"xmqqppgu8sxz.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpqegxa386m.fsf@anie.imag.fr","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T19:20:24Z","receivedAt":"2014-07-24T19:20:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Tanay Abhra writes:\n>\n>> For core the only test failing was xfuncname vs funcname,\n>\n> Being a little pessimistic: there may be other cases where the hashtable\n> magically gives the right order for existing tests, but that would fail\n> for untested use-cases.\n>\n> But I can't think of any such case.\n>\n>> so the situation is not as bad as you think. One course of action\n>> would be leave git_config() as it is, so that third party apps\n>> may not be broken. Provide a function like git_config_cache(),\n>> then rename all the git_config() calls in core to git_config_cache(),\n>> fallback to git_config() where it is not applicable (for example,\n>> git config -l).\n>\n> I think Junio's point about \"git config -l\" is correct: we should just\n> keep git_config_raw there.\n\nI have a slight preference of making git_config() do the right thing\nand take advantage of caching behaviour, to be honest. How much\nextra effort is necessary in your code to carry a piece of\ninformation, in addition to what lets you say \"here are the values\nassociated with that key in the order we read from the files\", in\norder to be able to say \"by the way, here is the order of key-value\npairs, if you want to show everything we read in the same order\"?\nIf it would be excessive, using _raw() could be an easy way to punt,\nbut because we know it is easy to decide to punt, I'd like to see us\nsee if a real solution is feasible first before punting.\n\n>> Also can you name any third party apps that use the git_config()\n>> system on which I can test the patches.\n>\n> There are probably tons of. I can think of git-multimail.\n\nThis question does not really matter.\n\nAmong the various points, I actually think the last one you omitted\nfrom your quote, closing door to one useful way to fix UI mistakes\nin the future, is the most serious one.\n"},{"id":"246664","messageId":"53D15EB4.1050303@gmail.com","threadId":"37199","inReplyTo":"xmqqppgu8sxz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-24T19:29:56Z","receivedAt":"2014-07-24T19:29:56Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/25/2014 12:50 AM, Junio C Hamano wrote:\n> Matthieu Moy writes:\n> \n>> Tanay Abhra writes:\n>>\n>>> For core the only test failing was xfuncname vs funcname,\n>>\n>> Being a little pessimistic: there may be other cases where the hashtable\n>> magically gives the right order for existing tests, but that would fail\n>> for untested use-cases.\n>>\n>> But I can't think of any such case.\n>>\n>>> so the situation is not as bad as you think. One course of action\n>>> would be leave git_config() as it is, so that third party apps\n>>> may not be broken. Provide a function like git_config_cache(),\n>>> then rename all the git_config() calls in core to git_config_cache(),\n>>> fallback to git_config() where it is not applicable (for example,\n>>> git config -l).\n>>\n>> I think Junio's point about \"git config -l\" is correct: we should just\n>> keep git_config_raw there.\n> \n> I have a slight preference of making git_config() do the right thing\n> and take advantage of caching behaviour, to be honest. How much\n> extra effort is necessary in your code to carry a piece of\n> information, in addition to what lets you say \"here are the values\n> associated with that key in the order we read from the files\", in\n> order to be able to say \"by the way, here is the order of key-value\n> pairs, if you want to show everything we read in the same order\"?\n> If it would be excessive, using _raw() could be an easy way to punt,\n> but because we know it is easy to decide to punt, I'd like to see us\n> see if a real solution is feasible first before punting.\n>\n\nI am thinking over it, lets see if there is a way before we take the\neasy route.\n\n>>> Also can you name any third party apps that use the git_config()\n>>> system on which I can test the patches.\n>>\n>> There are probably tons of. I can think of git-multimail.\n> \n> This question does not really matter.\n> \n> Among the various points, I actually think the last one you omitted\n> from your quote, closing door to one useful way to fix UI mistakes\n> in the future, is the most serious one.\n>\n\nIf we take the easy way out, fixing UI mistakes would be easier,\njust replace git_config_cache() with git_config_raw() for such cases.\n"},{"id":"246665","messageId":"xmqqlhri8rdb.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"53D15EB4.1050303@gmail.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T19:54:24Z","receivedAt":"2014-07-24T19:54:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> If we take the easy way out, fixing UI mistakes would be easier,\n> just replace git_config_cache() with git_config_raw() for such cases.\n\nI do not think that would fly well.\n\nThe kind of \"let's migrate funcname users to xfuncname while still\nsupporting the old uses\" change will be done in the callback\nfunctions like userdiff_config(). But it is very typical that\nmultiple config callback functions are cascaded (e.g. many\neventually end up calling git_default_core_config()); in order to a\nworkaround you suggest to help a callback in deep in a cascade chain\nwould require you to see which ones among all the callback functions\nwill eventually call the callback you are updating for migration and\nupdate all git_config() calls to git_config_raw().\n\nIf you fix it properly (assuming it is feasible; I haven't heard if\nyou even tried to see how much work it would involve), you do not\nneed to introduce \"git_config_cached()\" (or \"_raw()\" for that\nmatter), which is a huge plus. git_config() would instead do the\nright thing automatically, giving the same semantics except that it\ndoes not read the same file twice when it is known that the file has\nnot changed.\n\nI can understand \"If we take the easy way out, we do not have to do\nmuch\". But it would not make it any easier---it essentially closes\nthe door for \"oops, funcname is error-prone; let's give people\nxfuncname\" type of migration.\n"},{"id":"246666","messageId":"xmqqha268qxf.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpqsilq7muy.fsf@anie.imag.fr","subject":"Re: [PATCH 0/7] Rewrite `git_config()` using config-set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-24T20:03:56Z","receivedAt":"2014-07-24T20:03:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> As a side effect, I guess Junio will apply this on top of master so the\n> string interning API will be available immediately at the tip of the\n> branch.\n\nThanks for a heads-up. The two-patch series does not use the\ninterning, but does use string_list_init(), which is recent, and\nI will have to apply on a newer base than I have been applying its\nearlier iterations.\n"},{"id":"246668","messageId":"53D17919.4020702@ramsay1.demon.co.uk","threadId":"37199","inReplyTo":"xmqqlhri8rdb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-24T21:22:33Z","receivedAt":"2014-07-24T21:22:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 24/07/14 20:54, Junio C Hamano wrote:\n> Tanay Abhra writes:\n> \n>> If we take the easy way out, fixing UI mistakes would be easier,\n>> just replace git_config_cache() with git_config_raw() for such cases.\n> \n> I do not think that would fly well.\n> \n> The kind of \"let's migrate funcname users to xfuncname while still\n> supporting the old uses\" change will be done in the callback\n> functions like userdiff_config(). But it is very typical that\n> multiple config callback functions are cascaded (e.g. many\n> eventually end up calling git_default_core_config()); in order to a\n> workaround you suggest to help a callback in deep in a cascade chain\n> would require you to see which ones among all the callback functions\n> will eventually call the callback you are updating for migration and\n> update all git_config() calls to git_config_raw().\n> \n> If you fix it properly (assuming it is feasible; I haven't heard if\n> you even tried to see how much work it would involve), you do not\n> need to introduce \"git_config_cached()\" (or \"_raw()\" for that\n> matter), which is a huge plus. git_config() would instead do the\n> right thing automatically, giving the same semantics except that it\n> does not read the same file twice when it is known that the file has\n> not changed.\n> \n\nI haven't been following this conversation too closely, so if I have\ngrasped the wrong end of this stick, please accept my apologies! ;-)\n\nUsually if you need to iterate the values in a hash-table in the order\nof key insertion, you would simply link the hash-table entries into a\nlinked list. This assumes that the keys are distinct, or if not, that\nyou are using a 'multi-map' type of hash-table. Here, if memory serves\nme, you are doing the 'multi' bit yourself within the single hash-table\nentry for a given key; so its not quite so easy.\n\nHowever, I think it you could create a list of pairs in the config_set and use that to do\nthe iteration. A bit ugly, but it should work.\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"246676","messageId":"53D1CBFA.3020107@gmail.com","threadId":"37199","inReplyTo":"53D17919.4020702@ramsay1.demon.co.uk","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-25T03:16:10Z","receivedAt":"2014-07-25T03:16:10Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/25/2014 2:52 AM, Ramsay Jones wrote:\n> On 24/07/14 20:54, Junio C Hamano wrote:\n>> Tanay Abhra writes:\n>>\n>>> If we take the easy way out, fixing UI mistakes would be easier,\n>>> just replace git_config_cache() with git_config_raw() for such cases.\n>>\n>> I do not think that would fly well.\n>>\n>> The kind of \"let's migrate funcname users to xfuncname while still\n>> supporting the old uses\" change will be done in the callback\n>> functions like userdiff_config(). But it is very typical that\n>> multiple config callback functions are cascaded (e.g. many\n>> eventually end up calling git_default_core_config()); in order to a\n>> workaround you suggest to help a callback in deep in a cascade chain\n>> would require you to see which ones among all the callback functions\n>> will eventually call the callback you are updating for migration and\n>> update all git_config() calls to git_config_raw().\n>>\n>> If you fix it properly (assuming it is feasible; I haven't heard if\n>> you even tried to see how much work it would involve), you do not\n>> need to introduce \"git_config_cached()\" (or \"_raw()\" for that\n>> matter), which is a huge plus. git_config() would instead do the\n>> right thing automatically, giving the same semantics except that it\n>> does not read the same file twice when it is known that the file has\n>> not changed.\n>>\n> \n> I haven't been following this conversation too closely, so if I have\n> grasped the wrong end of this stick, please accept my apologies! ;-)\n> \n> Usually if you need to iterate the values in a hash-table in the order\n> of key insertion, you would simply link the hash-table entries into a\n> linked list. This assumes that the keys are distinct, or if not, that\n> you are using a 'multi-map' type of hash-table. Here, if memory serves\n> me, you are doing the 'multi' bit yourself within the single hash-table\n> entry for a given key; so its not quite so easy.\n> \n> However, I think it you could create a list of entry, string-list index> pairs in the config_set and use that to do\n> the iteration. A bit ugly, but it should work.\n>\n\nThanks for the advice, that is exactly what I am doing.\n\n> HTH\n> \n> ATB,\n> Ramsay Jones\n> \n> \n> \n"},{"id":"246682","messageId":"vpqlhriyll6.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"53D1CBFA.3020107@gmail.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-25T06:54:45Z","receivedAt":"2014-07-25T06:54:45Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> On 7/25/2014 2:52 AM, Ramsay Jones wrote:\n>> However, I think it you could create a list of > entry, string-list index> pairs in the config_set and use that to do\n>> the iteration. A bit ugly, but it should work.\n>\n> Thanks for the advice, that is exactly what I am doing.\n\nI'd just replace \"list\" with \"array\" and use\nDocumentation/technical/api-allocation-growing.txt.\n\nBut I can't think of a better way.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246718","messageId":"xmqqr41974cl.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpqlhriyll6.fsf@anie.imag.fr","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-25T17:09:14Z","receivedAt":"2014-07-25T17:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Tanay Abhra writes:\n>\n>> On 7/25/2014 2:52 AM, Ramsay Jones wrote:\n>>> However, I think it you could create a list of >> entry, string-list index> pairs in the config_set and use that to do\n>>> the iteration. A bit ugly, but it should work.\n>>\n>> Thanks for the advice, that is exactly what I am doing.\n>\n> I'd just replace \"list\" with \"array\" and use\n> Documentation/technical/api-allocation-growing.txt.\n>\n> But I can't think of a better way.\n\nPresumably this array will reflect the order the source file told us\nabout the keys and their values; I wonder if the \ninformation we already have can be used (or unified) with it?\n\nIs the \"when the last variable in a section disappears, make the\nsection header go away, with sensible handling of comments around\nit\" part of this GSoC project? If so, I suspect that the feature\nwould also want to know where things appear in the original file, so\nthis data structure may play a role there, too.\n"},{"id":"246721","messageId":"vpqlhrh9vtf.fsf@anie.imag.fr","threadId":"37199","inReplyTo":"xmqqr41974cl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-25T17:45:16Z","receivedAt":"2014-07-25T17:45:16Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> Tanay Abhra writes:\n>>\n>>> On 7/25/2014 2:52 AM, Ramsay Jones wrote:\n>>>> However, I think it you could create a list of >>> entry, string-list index> pairs in the config_set and use that to do\n>>>> the iteration. A bit ugly, but it should work.\n>>>\n>>> Thanks for the advice, that is exactly what I am doing.\n>>\n>> I'd just replace \"list\" with \"array\" and use\n>> Documentation/technical/api-allocation-growing.txt.\n>>\n>> But I can't think of a better way.\n>\n> Presumably this array will reflect the order the source file told us\n> about the keys and their values; I wonder if the \n> information we already have can be used (or unified) with it?\n\nI've thought about this too, and I think it would be really hard.\n\nFirst, there are several files in the picture (eg. /etc/gitconfig,\n~/.gitconfig and .git/config), and even included files hence it's not\neven a lexical order (file, line).\n\nThen, even if we had a way to order elements with (file, line),\niterating over the hashtable in this order wouldn't be easy (the naive\nway, \"get the smallest, get the second smallest, ...\" would be O(n^2)).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246736","messageId":"xmqq61il6zf2.fsf@gitster.dls.corp.google.com","threadId":"37199","inReplyTo":"vpqlhrh9vtf.fsf@anie.imag.fr","subject":"Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-25T18:55:45Z","receivedAt":"2014-07-25T18:55:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>>> I'd just replace \"list\" with \"array\" and use\n>>> Documentation/technical/api-allocation-growing.txt.\n>>>\n>>> But I can't think of a better way.\n>>\n>> Presumably this array will reflect the order the source file told us\n>> about the keys and their values; I wonder if the \n>> information we already have can be used (or unified) with it?\n>\n> I've thought about this too, and I think it would be really hard.\n\nYeah, I do not offhand think of a good solution, either, without\ngoing pointer-crazy and have bidirectional link everywhere X-<.\n"}]}