{"thread":{"id":"36814","subject":"[RFC/PATCH v2 0/2] Git config cache & special querying api utilizing the cache","startedAt":"2014-06-02T14:47:38Z","lastAt":"2014-06-09T08:17:47Z","messageCount":7,"participants":["Tanay Abhra","Torsten Bögershausen","Jeff King","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"243130","messageId":"1401720460-6525-1-git-send-email-tanayabh@gmail.com","threadId":"36814","inReplyTo":null,"subject":"[RFC/PATCH v2 0/2] Git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-02T14:47:38Z","receivedAt":"2014-06-02T14:47:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\n[V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen.\n      Added cache invalidation when config file is changed.\n      I am using git_config_set_multivar_in_file() as an update hook.\n\nThis is my first patch series for this year's GSoC. My project is\n\"Git Config API improvements\". The link of my proposal is appended below [1].\n\nThe aim of this patch series is to generate a cache for querying values from\nthe config files in a non-callback manner as the current method reads and\nparses the config files every time a value is queried for.\n\nThe cache is generated from hooking the update_cache function to the current\nparsing and callback mechanism in config.c. It is implemented as an hashmap\nusing the hashmap-api with variables and its corresponding values list as\nits members. The values in the list are sorted in order of increasing priority.\nThe cache is initialised in git_config_early() as it is the first time a `git_config`\nfunction is called during program startup. setup_git_directory_gently() calls\ngit_config_early() which in turn reads every config file (local, user and\nglobal config files).\n\nget_value() in config.c feeds variable and values into the callback function.\nUsing this function as a hook, we update the cache. Also, we add two new\nfunctions to the config-api git_config_get_string() and\ngit_config_get_string_multi() for querying in a non callback manner from\nthe cache.\n\nI have run the tests and debug the code and it works, but I have to add a\nfew things,\n\n1. Metadata about the variables and values. I have added only the file\n   from each variable value pair comes in an upcoming series.\n\nWhat else should I add or implement ;is my approach right? \n\n[1] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing\n\nCheers,\nTanay Abhra.\n\nTanay Abhra (2):\n  config: Add hashtable for config parsing & retrieval\n  config: Add new query functions to the api docs\n\n Documentation/technical/api-config.txt |  18 +++++\n cache.h                                |   2 +\n config.c                               | 118 +++++++++++++++++++++++++++++++++\n 3 files changed, 138 insertions(+)\n\n-- \n1.9.0.GIT\n"},{"id":"243131","messageId":"1401720460-6525-2-git-send-email-tanayabh@gmail.com","threadId":"36814","inReplyTo":"1401720460-6525-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH v2 1/2] config: Add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-02T14:47:39Z","receivedAt":"2014-06-02T14:47:39Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add a hash table to cache all key-value pairs read from config files\n(repo specific .git/config, user wide ~/.gitconfig and the global\n/etc/gitconfig). Add two external functions `git_config_get_string` and\n`git_config_get_string_multi` for querying in a non-callback manner from the\nhash table.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n cache.h  |   2 ++\n config.c | 118 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 120 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 107ac61..2038150 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1271,6 +1271,8 @@ extern int check_repository_format_version(const char *var, const char *value, v\n extern int git_env_bool(const char *, int);\n extern int git_config_system(void);\n extern int config_error_nonbool(const char *);\n+extern const char *git_config_get_string(const char *);\n+extern const struct string_list *git_config_get_string_multi(const char *);\n #if defined(__GNUC__) && ! defined(__clang__)\n #define config_error_nonbool(s) (config_error_nonbool(s), -1)\n #endif\ndiff --git a/config.c b/config.c\nindex a30cb5c..23c9403 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@@ -37,6 +39,112 @@ static struct config_source *cf;\n \n static int zlib_compression_seen;\n \n+static struct hashmap config_cache;\n+\n+struct config_cache_entry {\n+\tstruct hashmap_entry ent;\n+\tchar *key;\n+\tstruct string_list *value_list;\n+};\n+\n+static int hashmap_is_init = 0;\n+\n+static int config_cache_entry_cmp_icase(const struct config_cache_entry *e1,\n+\t\t\t\t const struct config_cache_entry *e2, const char* key)\n+{\n+\treturn strcasecmp(e1->key, key ? key : e2->key);\n+}\n+\n+static void config_cache_init(void)\n+{\n+\thashmap_init(&config_cache, (hashmap_cmp_fn)config_cache_entry_cmp_icase, 0);\n+}\n+\n+static void config_cache_free(void)\n+{\n+\tstruct config_cache_entry* entry;\n+\tstruct hashmap_iter iter;\n+\thashmap_iter_init(&config_cache, &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(&config_cache, 1);\n+}\n+\n+static struct config_cache_entry *config_cache_find_entry(const char *key)\n+{\n+\tstruct hashmap_entry k;\n+\thashmap_entry_init(&k, strihash(key));\n+\treturn hashmap_get(&config_cache, &k, key);\n+}\n+\n+static struct string_list *config_cache_get_value(const char *key)\n+{\n+\tstruct config_cache_entry *e = config_cache_find_entry(key);\n+\treturn e ? e->value_list : NULL;\n+}\n+\n+\n+static void config_cache_set_value(const char *key, const char *value)\n+{\n+\tstruct config_cache_entry *e;\n+\n+\te = config_cache_find_entry(key);\n+\tif (!e) {\n+\t\te = malloc(sizeof(*e));\n+\t\thashmap_entry_init(e, strihash(key));\n+\t\te->key=xstrdup(key);\n+\t\te->value_list = xcalloc(sizeof(struct string_list), 1);\n+\t\te->value_list->strdup_strings = 1;\n+\t\tstring_list_append(e->value_list, value);\n+\t\thashmap_add(&config_cache, e);\n+\t} else {\n+\t\tif (!unsorted_string_list_has_string(e->value_list, value))\n+\t\t\tstring_list_append(e->value_list, value);\n+\t}\n+}\n+\n+static void config_cache_remove_value(const char *key, const char *value)\n+{\n+\tstruct config_cache_entry *e;\n+\tint i;\n+\n+\te = config_cache_find_entry(key);\n+\tif(!e)\n+\t\treturn;\n+\t/* If value == NULL then remove all the entries for the key */\n+\tif(!value) {\n+\t\tstring_list_clear(e->value_list, 0);\n+\t\tfree(e->value_list);\n+\t\thashmap_remove(&config_cache, e, NULL);\n+\t\treturn;\n+\t}\n+\t/* All the occurances of \"value\" will be deleted */\n+\tfor (i = 0; i < e->value_list->nr; i++)\n+\t\tif (!strcmp(value, e->value_list->items[i].string))\n+\t\t\tunsorted_string_list_delete_item(e->value_list, i, 0);\n+\tif(e->value_list->nr == 0) {\n+\t\tstring_list_clear(e->value_list, 0);\n+\t\tfree(e->value_list);\n+\t\thashmap_remove(&config_cache, e, NULL);\n+\t}\n+}\n+\n+extern const char *git_config_get_string(const char *name)\n+{\n+\tstruct string_list *values;\n+\tvalues = config_cache_get_value(name);\n+\tif (!values)\n+\t\treturn NULL;\n+\treturn values->items[values->nr - 1].string;\n+}\n+\n+extern const struct string_list *git_config_get_string_multi(const char *key)\n+{\n+\treturn config_cache_get_value(key);\n+}\n+\n static int config_file_fgetc(struct config_source *conf)\n {\n \treturn fgetc(conf->u.file);\n@@ -333,6 +441,7 @@ 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+\tconfig_cache_set_value(name->buf, value);\n \treturn fn(name->buf, value, data);\n }\n \n@@ -1135,6 +1244,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n \tchar *xdg_config = NULL;\n \tchar *user_config = NULL;\n \n+\tif (!hashmap_is_init) {\n+\t\tconfig_cache_init();\n+\t\thashmap_is_init = 1;\n+\t}\n+\n \thome_config_paths(&user_config, &xdg_config, \"config\");\n \n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n@@ -1570,6 +1684,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\tif (!store_write_section(fd, key) ||\n \t\t    !store_write_pair(fd, key, value))\n \t\t\tgoto write_err_out;\n+\t\telse config_cache_set_value(key, value);\n \t} else {\n \t\tstruct stat st;\n \t\tchar *contents;\n@@ -1673,6 +1788,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t}\n \t\t\tif (!store_write_pair(fd, key, value))\n \t\t\t\tgoto write_err_out;\n+\t\t\telse config_cache_set_value(key, value);\n+\t\t} else {\n+\t\t\tconfig_cache_remove_value(key, NULL);\n \t\t}\n \n \t\t/* write the rest of the config */\n-- \n1.9.0.GIT\n"},{"id":"243132","messageId":"1401720460-6525-3-git-send-email-tanayabh@gmail.com","threadId":"36814","inReplyTo":"1401720460-6525-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH v2 2/2] config: Add new query functions to the api docs","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-02T14:47:40Z","receivedAt":"2014-06-02T14:47:40Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add explanations for `git_config_get_string_multi` and `git_config_get_string`\nwhich utilize the config hash table for querying in a non-callback manner for\na specific variable.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n Documentation/technical/api-config.txt | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..8f1a37e 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,24 @@ 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_string`\n+and `git_config_get_string_multi`. They both take a single parameter,\n+\n+- a key string in canonical flat form for which the corresponding values\n+  will be retrieved and returned.\n+\n+They both read values from an internal cache generated previously from\n+reading the config files. `git_config_get_string` returns the value with\n+the highest priority(i.e. value in the repo config will be preferred over\n+value in user wide config for the same variable).\n+\n+`git_config_get_string_multi` returns a `string_list` containing all the\n+values for that particular key, sorted in order of increasing priority.\n+\n Value Parsing Helpers\n ---------------------\n \n-- \n1.9.0.GIT\n"},{"id":"243148","messageId":"538CD1A2.4020400@web.de","threadId":"36814","inReplyTo":"1401720460-6525-2-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH v2 1/2] config: Add hashtable for config parsing & retrieval","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-06-02T19:33:54Z","receivedAt":"2014-06-02T19:33:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-06-02 16.47, Tanay Abhra wrote:\n\n[]\nPlease see 3 minor remarks inline.\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> @@ -37,6 +39,112 @@ static struct config_source *cf;\n>  \n>  static int zlib_compression_seen;\n>  \n> +static struct hashmap config_cache;\n> +\n> +struct config_cache_entry {\n> +\tstruct hashmap_entry ent;\n> +\tchar *key;\n> +\tstruct string_list *value_list;\n> +};\n> +\n> +static int hashmap_is_init = 0;\nwe don't need the \" = 0\", as all static data is initialized to 0 (or NULL) \n> +\n> +static int config_cache_entry_cmp_icase(const struct config_cache_entry *e1,\n> +\t\t\t\t const struct config_cache_entry *e2, const char* key)\nthe * should be aligned to the variable \"key\":\nconst char *key\n\n[]\n\n> +static void config_cache_set_value(const char *key, const char *value)\n> +{\n> +\tstruct config_cache_entry *e;\n> +\n> +\te = config_cache_find_entry(key);\n> +\tif (!e) {\n> +\t\te = malloc(sizeof(*e));\nI think we need xmalloc() here (from wrapper.c)\n"},{"id":"243182","messageId":"20140603075853.GA32249@sigill.intra.peff.net","threadId":"36814","inReplyTo":"1401720460-6525-2-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH v2 1/2] config: Add hashtable for config parsing & retrieval","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-03T07:58:53Z","receivedAt":"2014-06-03T07:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 02, 2014 at 07:47:39AM -0700, Tanay Abhra wrote:\n\n> +static int config_cache_entry_cmp_icase(const struct config_cache_entry *e1,\n> +\t\t\t\t const struct config_cache_entry *e2, const char* key)\n> +{\n> +\treturn strcasecmp(e1->key, key ? key : e2->key);\n> +}\n> +\n> +static void config_cache_init(void)\n> +{\n> +\thashmap_init(&config_cache, (hashmap_cmp_fn)config_cache_entry_cmp_icase, 0);\n> +}\n\nIt looks like this is meant to treat config keys like \"diff.renames\" as\ncase-insensitive. That's OK for two-part names, but longer names with a\nmiddle section, like \"remote.Foo.url\", should have their middle sections\nbe case-sensitive.\n\nAlso, please avoid casting function ptrs. Give your function the correct\nhashmap_cmp_fn signature, and cast from \"void *\" internally if you need\nto.\n\n> +static void config_cache_free(void)\n> +{\n> +\tstruct config_cache_entry* entry;\n> +\tstruct hashmap_iter iter;\n> +\thashmap_iter_init(&config_cache, &iter);\n> +\twhile (entry = hashmap_iter_next(&iter)) {\n\nPlease wrap assignment inside a loop condition with an extra (), like:\n\n  while ((entry = hashmap_iter_next(&iter)))\n\nThis suppresses gcc's \"did you mean ==\" warning. If you are not\ncompiling with \"-Wall -Werror\" for development, you probably should be.\n\n> +static struct config_cache_entry *config_cache_find_entry(const char *key)\n> +{\n> +\tstruct hashmap_entry k;\n> +\thashmap_entry_init(&k, strihash(key));\n> +\treturn hashmap_get(&config_cache, &k, key);\n> +}\n\nAs above, strihash isn't quite right here. It needs a custom function\nthat takes into account the case-sensitivity of the middle section of\nthe key name.\n\nAn alternative is to only feed already-normalized names into the hash\ncode, and then tell the hash to be case-sensitive.\n\n> +static void config_cache_set_value(const char *key, const char *value)\n> +{\n> +\tstruct config_cache_entry *e;\n> +\n> +\te = config_cache_find_entry(key);\n> +\tif (!e) {\n> +\t\te = malloc(sizeof(*e));\n> +\t\thashmap_entry_init(e, strihash(key));\n> +\t\te->key=xstrdup(key);\n\nStyle: please keep whitespace around \"=\".\n\n> +\t} else {\n> +\t\tif (!unsorted_string_list_has_string(e->value_list, value))\n> +\t\t\tstring_list_append(e->value_list, value);\n> +\t}\n\nI don't think we want to suppress duplicates here. If a caller reads the\nvalue with git_config_get_string(), then the duplicates do not matter\n(we show only the one with precedence). If they use the \"multi()\" form,\nthen they _should_ see each version. It is not up to the config code to\ndecide that duplicate values for the same key are not interesting; only\nthe caller knows that.\n\n> +static void config_cache_remove_value(const char *key, const char *value)\n> +{\n> +\tstruct config_cache_entry *e;\n> +\tint i;\n> +\n> +\te = config_cache_find_entry(key);\n> +\tif(!e)\n\nStyle: if (!e)\n\n> +\t/* If value == NULL then remove all the entries for the key */\n> +\tif(!value) {\n\nDitto (and elsewhere, but I'll stop noting each).\n\n> +extern const char *git_config_get_string(const char *name)\n> +{\n> +\tstruct string_list *values;\n> +\tvalues = config_cache_get_value(name);\n> +\tif (!values)\n> +\t\treturn NULL;\n> +\treturn values->items[values->nr - 1].string;\n> +}\n\nThe assumption here is that values->nr is never 0. I think that is true,\nbecause we do not create the cache entry until we get a value (and\nremove it if we drop the last value). However, it might be worth\ndocumenting that with an assertion.\n\n> @@ -333,6 +441,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n>               if (!value)\n>                       return -1;\n>       }\n> +     config_cache_set_value(name->buf, value);\n>       return fn(name->buf, value, data);\n>  }\n\nThe function get_value gets called for each entry from a parsed config\nfile. However, it does not get called for command-line config from \"git\n-c\". You'd need to update git_config_parse_parameter for that.\n\nHowever, I wonder if this is the right place to hook into the cache at\nall. The cache should be able to sit _atop_ the existing callback\nsystem. So you could easily implement it as a separate layer, and just\nlazily initialize it like:\n\n  static int config_cache_callback(const char *key, const char *val, void *cache)\n  {\n\tconfig_cache_set_value(cache, key, val);\n\treturn 0;\n  }\n\n  static struct hashmap *get_config_cache(void)\n  {\n\tstatic int initialized;\n\tstatic struct hashmap cache;\n\tif (!initialized) {\n\t\thash_map_init(&cache, \n\t\tgit_config(config_cache_callback, &cache);\n\t\tinitialized = 1;\n\t}\n  }\n\nand then teach config_cache_get_value and friends to access the cache\nthrough \"get_config_cache\".\n\n> @@ -1570,6 +1684,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>  \t\tif (!store_write_section(fd, key) ||\n>  \t\t    !store_write_pair(fd, key, value))\n>  \t\t\tgoto write_err_out;\n> +\t\telse config_cache_set_value(key, value);\n>  \t} else {\n>  \t\tstruct stat st;\n>  \t\tchar *contents;\n> @@ -1673,6 +1788,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>  \t\t\t}\n>  \t\t\tif (!store_write_pair(fd, key, value))\n>  \t\t\t\tgoto write_err_out;\n> +\t\t\telse config_cache_set_value(key, value);\n> +\t\t} else {\n> +\t\t\tconfig_cache_remove_value(key, NULL);\n>  \t\t}\n>  \n>  \t\t/* write the rest of the config */\n\nThese two seem suspect to me. How do we know that config_filename that\nwe are manipulating is even referenced by the config cache? When we\nremove an entry, how do we know we are getting the entry from that file,\nand not a similar entry in another file?\n\nStepping back, though, do we need to keep the cache up to date here with\nmicro-changes at all? If we have updated the config files on disk, then\nthe simplest thing would be to just invalidate the whole cache (and then\nlazily re-read it if we actually need it again). This should be a rare\ncase (I suspect that \"git init\" and \"git clone\" might want this, but\nthat's about it).\n\n-Peff\n"},{"id":"243189","messageId":"vpqegz6qa4a.fsf@anie.imag.fr","threadId":"36814","inReplyTo":"1401720460-6525-3-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH v2 2/2] config: Add new query functions to the api docs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-03T15:35:33Z","receivedAt":"2014-06-03T15:35:33Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> Add explanations for `git_config_get_string_multi` and `git_config_get_string`\n> which utilize the config hash table for querying in a non-callback manner for\n> a specific variable.\n\nI'd squash this into the previous patch: the reader appreciates having\nthe documentation when reviewing the code itself (so that one can check\nthat the documented behavior matches the implementation).\n\n> +the highest priority(i.e. value in the repo config will be preferred over\n\nMissing space before (.\n\nOther than that, this seems good.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"243608","messageId":"CAPig+cQxp2iAvFqu=C+hvU4mpVHd6rk5tURvi_jrasfBHHwm8g@mail.gmail.com","threadId":"36814","inReplyTo":"1401720460-6525-2-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH v2 1/2] config: Add hashtable for config parsing & retrieval","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-09T08:17:47Z","receivedAt":"2014-06-09T08:17:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In addition to the valuable review comments by Torsten and Peff, find\nmore below...\n\nOn Mon, Jun 2, 2014 at 10:47 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> Add a hash table to cache all key-value pairs read from config files\n> (repo specific .git/config, user wide ~/.gitconfig and the global\n> /etc/gitconfig). Add two external functions `git_config_get_string` and\n> `git_config_get_string_multi` for querying in a non-callback manner from the\n> hash table.\n>\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n> diff --git a/config.c b/config.c\n> index a30cb5c..23c9403 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>         struct config_source *prev;\n> @@ -37,6 +39,112 @@ static struct config_source *cf;\n>\n>  static int zlib_compression_seen;\n>\n> +static struct hashmap config_cache;\n> +\n> +struct config_cache_entry {\n> +       struct hashmap_entry ent;\n> +       char *key;\n> +       struct string_list *value_list;\n\nSame question as in my v1 review [1]: Why is this 'struct string_list\n*' rather than just 'struct string_list'?\n\n[1]: http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html#a7611860\n\n> +};\n> +\n> +static int hashmap_is_init = 0;\n> +\n> +static int config_cache_entry_cmp_icase(const struct config_cache_entry *e1,\n> +                                const struct config_cache_entry *e2, const char* key)\n> +{\n> +       return strcasecmp(e1->key, key ? key : e2->key);\n\nAre you planning on taking advantage of this extra complexity (the\n'key ? key : e2->key' stuff)? At present, the lone caller _always_\nprovides the 'key', so 'e2->key' is never used. If not needed,\ndropping it will simplify things for future readers of the code. (But,\nsee below for more commentary...)\n\n> +}\n> +\n> +static void config_cache_init(void)\n> +{\n> +       hashmap_init(&config_cache, (hashmap_cmp_fn)config_cache_entry_cmp_icase, 0);\n> +}\n> +\n> +static void config_cache_free(void)\n> +{\n> +       struct config_cache_entry* entry;\n\nStyle: struct config_cache_entry *entry;\n\n> +       struct hashmap_iter iter;\n> +       hashmap_iter_init(&config_cache, &iter);\n> +       while (entry = hashmap_iter_next(&iter)) {\n> +               free(entry->key);\n> +               string_list_clear(entry->value_list, 0);\n> +       }\n> +       hashmap_free(&config_cache, 1);\n> +}\n\nconfig_cache_free() is never called.\n\n> +static struct config_cache_entry *config_cache_find_entry(const char *key)\n> +{\n> +       struct hashmap_entry k;\n> +       hashmap_entry_init(&k, strihash(key));\n> +       return hashmap_get(&config_cache, &k, key);\n\nThis feels very dangerous. You've declared\nconfig_cache_entry_cmp_icase() as taking a 'struct config_cache_entry\n*' as its second argument, however, you are passing a 'struct\nhashmap_entry *'. While it may work correctly in this special case as\nyou've written it, where config_cache_entry_cmp_icase() never accesses\nany members of its second argument beyond 'ent', it is not\ntransparent. A future programmer may trust the type of the incoming\nsecond argument and change config_cache_entry_cmp_icase() to access\nelements beyond 'ent' without realizing that those elements aren't\nactually present.\n\nIt seems safer to code this as in the example section of\nDocumentation/api-hashmap.txt:\n\n    struct config_cache_entry k;\n    hashmap_entry_init(&k, strihash(key));\n    k.key = key; /* maybe cast-away const here */\n    return hashmap_get(&config_cache, &k, NULL);\n\nAnd modify config_cache_entry_cmp_icase() to always access its second\nargument and treat the third as unused.\n\n> +}\n> +\n> +static struct string_list *config_cache_get_value(const char *key)\n> +{\n> +       struct config_cache_entry *e = config_cache_find_entry(key);\n> +       return e ? e->value_list : NULL;\n> +}\n> +\n> +\n> +static void config_cache_set_value(const char *key, const char *value)\n> +{\n> +       struct config_cache_entry *e;\n> +\n> +       e = config_cache_find_entry(key);\n> +       if (!e) {\n> +               e = malloc(sizeof(*e));\n> +               hashmap_entry_init(e, strihash(key));\n> +               e->key=xstrdup(key);\n> +               e->value_list = xcalloc(sizeof(struct string_list), 1);\n> +               e->value_list->strdup_strings = 1;\n> +               string_list_append(e->value_list, value);\n> +               hashmap_add(&config_cache, e);\n> +       } else {\n> +               if (!unsorted_string_list_has_string(e->value_list, value))\n> +                       string_list_append(e->value_list, value);\n> +       }\n\nAlso mentioned in [1]: As there is only a single 'if' statement within\nthis 'else' clause, you could turn this into an 'else if' and drop one\nlevel of indentation. (Although, as Peff mentioned in his review, you\nprobably don't want to be folding out duplicate values anyhow, so this\n'if' would go away.)\n\n> +}\n> +\n> +static void config_cache_remove_value(const char *key, const char *value)\n> +{\n> +       struct config_cache_entry *e;\n> +       int i;\n> +\n> +       e = config_cache_find_entry(key);\n> +       if(!e)\n> +               return;\n> +       /* If value == NULL then remove all the entries for the key */\n> +       if(!value) {\n> +               string_list_clear(e->value_list, 0);\n> +               free(e->value_list);\n> +               hashmap_remove(&config_cache, e, NULL);\n\nIsn't this leaking e->key?\n\n> +               return;\n> +       }\n> +       /* All the occurances of \"value\" will be deleted */\n\ns/occurances/occurrences/\n\n> +       for (i = 0; i < e->value_list->nr; i++)\n> +               if (!strcmp(value, e->value_list->items[i].string))\n> +                       unsorted_string_list_delete_item(e->value_list, i, 0);\n\nThis loop is broken. It wants to delete all elements matching 'value',\nbut it will skip some. For instance, if 'value_list' contains items\n[\"foo\", \"foo\"], and you ask it to remove all \"foo\", it will return\n[\"foo\"]. This is because you increment 'i' even though the 'i'th\nelement has just been removed. To fix, either don't increment 'i' when\nan item is removed, or reverse the order of visitation (from back to\nfront).\n\n> +       if(e->value_list->nr == 0) {\n> +               string_list_clear(e->value_list, 0);\n> +               free(e->value_list);\n> +               hashmap_remove(&config_cache, e, NULL);\n\nIsn't this leaking e->key?\n\n> +       }\n> +}\n> +\n> +extern const char *git_config_get_string(const char *name)\n> +{\n> +       struct string_list *values;\n> +       values = config_cache_get_value(name);\n> +       if (!values)\n> +               return NULL;\n> +       return values->items[values->nr - 1].string;\n> +}\n> +\n> +extern const struct string_list *git_config_get_string_multi(const char *key)\n> +{\n> +       return config_cache_get_value(key);\n> +}\n> +\n>  static int config_file_fgetc(struct config_source *conf)\n>  {\n>         return fgetc(conf->u.file);\n> @@ -333,6 +441,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)\n>                 if (!value)\n>                         return -1;\n>         }\n> +       config_cache_set_value(name->buf, value);\n>         return fn(name->buf, value, data);\n>  }\n>\n> @@ -1135,6 +1244,11 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)\n>         char *xdg_config = NULL;\n>         char *user_config = NULL;\n>\n> +       if (!hashmap_is_init) {\n> +               config_cache_init();\n> +               hashmap_is_init = 1;\n> +       }\n> +\n>         home_config_paths(&user_config, &xdg_config, \"config\");\n>\n>         if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n> @@ -1570,6 +1684,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>                 if (!store_write_section(fd, key) ||\n>                     !store_write_pair(fd, key, value))\n>                         goto write_err_out;\n> +               else config_cache_set_value(key, value);\n>         } else {\n>                 struct stat st;\n>                 char *contents;\n> @@ -1673,6 +1788,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>                         }\n>                         if (!store_write_pair(fd, key, value))\n>                                 goto write_err_out;\n> +                       else config_cache_set_value(key, value);\n> +               } else {\n> +                       config_cache_remove_value(key, NULL);\n>                 }\n>\n>                 /* write the rest of the config */\n> --\n> 1.9.0.GIT\n>\n"}]}