{"thread":{"id":"36754","subject":"[RFC/PATCH 0/2] Git config cache & special querying api utilizing the cache","startedAt":"2014-05-26T17:33:19Z","lastAt":"2014-05-28T09:43:06Z","messageCount":7,"participants":["Tanay Abhra","Torsten Bögershausen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"242685","messageId":"1401125601-18249-1-git-send-email-tanayabh@gmail.com","threadId":"36754","inReplyTo":null,"subject":"[RFC/PATCH 0/2] Git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-05-26T17:33:19Z","receivedAt":"2014-05-26T17:33:19Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\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. Invalidity check: if a config file is written into, update the cache.\n   I am using git_config_set_multivar_in_file() as an update hook.\n\n2. 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 cache for config value querying\n  config: Add new query functions to the api\n\n Documentation/technical/api-config.txt |  19 ++++++\n cache.h                                |   2 +\n config.c                               | 105 +++++++++++++++++++++++++++++++++\n 3 files changed, 126 insertions(+)\n\n-- \n1.9.0.GIT\n"},{"id":"242686","messageId":"1401125601-18249-2-git-send-email-tanayabh@gmail.com","threadId":"36754","inReplyTo":"1401125601-18249-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH 1/2] config: Add cache for config value querying","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-05-26T17:33:20Z","receivedAt":"2014-05-26T17:33:20Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add an internal cache with the all variable value pairs read from the usual\nconfig files(repo specific .git/config, user wide ~/.gitconfig and the global\n/etc/gitconfig). Also, add two external functions `git_config_get_string` and\n`git_config_get_string_multi` for querying in an non callback manner from the\ncache.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n cache.h  |   2 ++\n config.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 107 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..f6515c2 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,102 @@ static struct config_source *cf;\n \n static int zlib_compression_seen;\n \n+struct hashmap config_cache;\n+\n+struct config_cache_node {\n+\tstruct hashmap_entry ent;\n+\tstruct strbuf key;\n+\tstruct string_list *value_list ;\n+};\n+\n+static int hashmap_init_v = 0;\n+\n+static int config_cache_node_cmp(const struct config_cache_node *e1,\n+\t\t\t   const struct config_cache_node *e2, const char *key)\n+{\n+\treturn strcmp(e1->key.buf, key ? key : e2->key.buf);\n+}\n+\n+static int config_cache_node_cmp_icase(const struct config_cache_node *e1,\n+\t\t\t\t const struct config_cache_node *e2, const char* key)\n+{\n+\treturn strcasecmp(e1->key.buf, key ? key : e2->key.buf);\n+}\n+\n+static void config_cache_init(const int icase)\n+{\n+\thashmap_init(&config_cache, (hashmap_cmp_fn) (icase ? config_cache_node_cmp_icase\n+\t\t\t: config_cache_node_cmp), 0);\n+}\n+\n+static void config_cache_free(void)\n+{\n+\thashmap_free(&config_cache, 1);\n+}\n+\n+static struct config_cache_node *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_node *e = config_cache_find_entry(key);\n+\tif (e == NULL)\n+\t\treturn NULL;\n+\telse\n+\t\treturn (e->value_list);\n+}\n+\n+\n+static void config_cache_set_value(const char *key, const char *value)\n+{\n+\tstruct config_cache_node *e;\n+\n+\tif (!value)\n+\t\treturn;\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\tstrbuf_init(&(e->key), 1024);\n+\t\tstrbuf_addstr(&(e->key),key);\n+\t\te->value_list = malloc(sizeof(struct string_list));\n+\t\te->value_list->strdup_strings = 1;\n+\t\te->value_list->nr = 0;\n+\t\te->value_list->alloc = 0;\n+\t\te->value_list->items = NULL;\n+\t\te->value_list->cmp = NULL;\n+\t\tstring_list_append(e->value_list, value);\n+\t\thashmap_add(&config_cache, e);\n+\t}\n+\telse {\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+extern const char *git_config_get_string(const char *name)\n+{\n+\tstruct string_list *values;\n+\tint num_values;\n+\tchar *result;\n+\tvalues = config_cache_get_value(name);\n+\tif (!values)\n+\t\treturn NULL;\n+\tnum_values = values->nr;\n+\tresult = values->items[num_values-1].string ;\n+\treturn result;\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 +431,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 +1234,12 @@ 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+\tint icase = 1;\n+\tif (!hashmap_init_v) {\n+\t\tconfig_cache_init(icase);\n+\t\thashmap_init_v = 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-- \n1.9.0.GIT\n"},{"id":"242687","messageId":"1401125601-18249-3-git-send-email-tanayabh@gmail.com","threadId":"36754","inReplyTo":"1401125601-18249-1-git-send-email-tanayabh@gmail.com","subject":"[RFC/PATCH 2/2] config: Add new query functions to the api","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-05-26T17:33:21Z","receivedAt":"2014-05-26T17:33:21Z","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 cache for querying in an non callback manner for a\nspecific variable.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n Documentation/technical/api-config.txt | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..33b5b90 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,25 @@ 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 variable as the key string for which the corresponding values will\n+  be retrieved and returned.\n+\n+They both read value 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 variable, sorted in order of increasing\n+priority.\n+\n Value Parsing Helpers\n ---------------------\n \n-- \n1.9.0.GIT\n"},{"id":"242695","messageId":"53839DEE.9090504@web.de","threadId":"36754","inReplyTo":"1401125601-18249-2-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH 1/2] config: Add cache for config value querying","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-05-26T20:02:54Z","receivedAt":"2014-05-26T20:02:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-05-26 19.33, Tanay Abhra wrote:\nI like the idea.\nPlease allow some minor comments (and read them as questions rather then answers)\n> Add an internal cache with the all variable value pairs read from the usual\n\"cache\": The word \"cache\" is in Git often used for \"index\" \n\"variable value\" can be written as \"key value\"\n\"usual\": I don't think we handle \"unusual\" config files,\n(so can we drop the word usual ?)\nI think the (important) first line can be written like this:\n\n>Add a hash table with the all key-value pairs read from the\nor\n>Add a hash table to cache all key-value pairs read from the\n\n> config files(repo specific .git/config, user wide ~/.gitconfig and the global\n> /etc/gitconfig). Also, add two external functions `git_config_get_string` and\nCan we drop \"Also\" ?\n> @@ -37,6 +39,102 @@ static struct config_source *cf;\n>  \n>  static int zlib_compression_seen;\n>  \n> +struct hashmap config_cache;\n> +\n> +struct config_cache_node {\n> +\tstruct hashmap_entry ent;\n> +\tstruct strbuf key;\nDo we need a whole strbuf for the key?\nOr could a \"char *key\" work as well? \n(and/or could it be \"const char *key\" ?\n> +\tstruct string_list *value_list ;\n\n\n\n> +static struct string_list *config_cache_get_value(const char *key)\n> +{\n> +\tstruct config_cache_node *e = config_cache_find_entry(key);\nwhy \"e\" ? Will \"node\" be easier to read ? Or entry ? \n\n\n> +static void config_cache_set_value(const char *key, const char *value)\n> +{\n> +\tstruct config_cache_node *e;\n> +\n> +\tif (!value)\n> +\t\treturn;\nHm, either NULL could mean \"unset==remove\" the value, (but we don't do that, do we?\n\nOr it could mean a programming or runtime error?, Should there be a warning ?\n\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\tstrbuf_init(&(e->key), 1024);\n> +\t\tstrbuf_addstr(&(e->key),key);\n> +\t\te->value_list = malloc(sizeof(struct string_list));\n> +\t\te->value_list->strdup_strings = 1;\n> +\t\te->value_list->nr = 0;\n> +\t\te->value_list->alloc = 0;\n> +\t\te->value_list->items = NULL;\n> +\t\te->value_list->cmp = NULL;\nWhen malloc() is replaced by xcalloc()  the x = NULL and y = 0 can go away,\nand the code is shorter and easier to read.\n\n\n> +extern const char *git_config_get_string(const char *name)\n> +{\n> +\tstruct string_list *values;\n> +\tint num_values;\n> +\tchar *result;\n> +\tvalues = config_cache_get_value(name);\n> +\tif (!values)\n> +\t\treturn NULL;\n> +\tnum_values = values->nr;\n> +\tresult = values->items[num_values-1].string ;\nWe could get rid of the variable  \"int num_values\" by simply writing\nresult = values->items[values->nr-1].string;\n"},{"id":"242744","messageId":"5384991E.10409@gmail.com","threadId":"36754","inReplyTo":"53839DEE.9090504@web.de","subject":"Re: [RFC/PATCH 1/2] config: Add cache for config value querying","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-05-27T13:54:38Z","receivedAt":"2014-05-27T13:54:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nOn 05/26/2014 01:02 PM, Torsten Bögershausen wrote:\n>> Add an internal cache with the all variable value pairs read from the usual\n> \"cache\": The word \"cache\" is in Git often used for \"index\" \nOkay, point noted. I thought about choosing between \"hashmap\" and \"cache\" and chose\nthe later.\n> \"variable value\" can be written as \"key value\"\n\nI  had used the term \"variable\" to be consistent with the documentation\n(api-config.txt). But I think \"key\" is much clearer.\n\n> \"usual\": I don't think we handle \"unusual\" config files,\n> (so can we drop the word usual ?)\n\nOkay, noted.\n\n> I think the (important) first line can be written like this:\n> \n>> Add a hash table with the all key-value pairs read from the\n> or\n>> Add a hash table to cache all key-value pairs read from the\n> \n>> config files(repo specific .git/config, user wide ~/.gitconfig and the global\n>> /etc/gitconfig). Also, add two external functions `git_config_get_string` and\n> Can we drop \"Also\" ?\n>> @@ -37,6 +39,102 @@ static struct config_source *cf;\n>>  \n>>  static int zlib_compression_seen;\n>>  \n>> +struct hashmap config_cache;\n>> +\n>> +struct config_cache_node {\n>> +\tstruct hashmap_entry ent;\n>> +\tstruct strbuf key;\n> Do we need a whole strbuf for the key?\n> Or could a \"char *key\" work as well? \n> (and/or could it be \"const char *key\" ?\n\nTo maintain consistency with config.c. config.c uses strbuf for both key and value\nthroughout. I found the reason by git-blaming config.c. Key length is flexible so it\nwould be better to use a api construct such as strbuf for it.\n\n>> +\tstruct string_list *value_list ;\n> \n> \n> \n>> +static struct string_list *config_cache_get_value(const char *key)\n>> +{\n>> +\tstruct config_cache_node *e = config_cache_find_entry(key);\n> why \"e\" ? Will \"node\" be easier to read ? Or entry ? \n\nNoted. Entry is much better.\n\n>> +static void config_cache_set_value(const char *key, const char *value)\n>> +{\n>> +\tstruct config_cache_node *e;\n>> +\n>> +\tif (!value)\n>> +\t\treturn;\n> Hm, either NULL could mean \"unset==remove\" the value, (but we don't do that, do we?\n> \n> Or it could mean a programming or runtime error?, Should there be a warning ?\n\nNope. It is just a check to not save blank values for a key in the hashmap. Removal\nfunctionality will come later. NULL==remove is implemented in\ngit_config_set_multivar_in_file(). We are not reading key value pairs from that, just\nfrom git_config().\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\tstrbuf_init(&(e->key), 1024);\n>> +\t\tstrbuf_addstr(&(e->key),key);\n>> +\t\te->value_list = malloc(sizeof(struct string_list));\n>> +\t\te->value_list->strdup_strings = 1;\n>> +\t\te->value_list->nr = 0;\n>> +\t\te->value_list->alloc = 0;\n>> +\t\te->value_list->items = NULL;\n>> +\t\te->value_list->cmp = NULL;\n> When malloc() is replaced by xcalloc()  the x = NULL and y = 0 can go away,\n> and the code is shorter and easier to read.\n\nMuch better, thanks.\n\n>> +extern const char *git_config_get_string(const char *name)\n>> +{\n>> +\tstruct string_list *values;\n>> +\tint num_values;\n>> +\tchar *result;\n>> +\tvalues = config_cache_get_value(name);\n>> +\tif (!values)\n>> +\t\treturn NULL;\n>> +\tnum_values = values->nr;\n>> +\tresult = values->items[num_values-1].string ;\n> We could get rid of the variable  \"int num_values\" by simply writing\n> result = values->items[values->nr-1].string;\n> \n\nNoted.\n\n\nCheers,\nTanay Abhra.\n"},{"id":"242852","messageId":"CAPig+cSvrJ0bjuHBaRpUYzi7tX+MEbhmCYv12oiTcexm3+Pd0g@mail.gmail.com","threadId":"36754","inReplyTo":"1401125601-18249-2-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH 1/2] config: Add cache for config value querying","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-05-28T09:31:11Z","receivedAt":"2014-05-28T09:31:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 26, 2014 at 1:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> Add an internal cache with the all variable value pairs read from the usual\n> config files(repo specific .git/config, user wide ~/.gitconfig and the global\n\ns/(/ (/\n\n> /etc/gitconfig). Also, add two external functions `git_config_get_string` and\n> `git_config_get_string_multi` for querying in an non callback manner from the\n\ns/an/a/\ns/non/non-/\n\nMore below.\n\n> cache.\n>\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n>  cache.h  |   2 ++\n>  config.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 107 insertions(+)\n>\n> diff --git a/cache.h b/cache.h\n> index 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\n> diff --git a/config.c b/config.c\n> index a30cb5c..f6515c2 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,102 @@ static struct config_source *cf;\n>\n>  static int zlib_compression_seen;\n>\n> +struct hashmap config_cache;\n\nThis should be 'static'.\n\n> +struct config_cache_node {\n> +       struct hashmap_entry ent;\n> +       struct strbuf key;\n\nUpon reading this, I had the same question as Torsten: Why isn't 'key'\na plain 'char *' allocated when the entry is created? Your answer:\n\n    To maintain consistency with config.c. config.c uses strbuf for\n    both key and value throughout. I found the reason by git-blaming\n    config.c. Key length is flexible so it would be better to use a\n    api construct such as strbuf for it.\n\ndoesn't make sense. The existing callback-based code re-uses the\nstrbuf for each key as it parses the file, however, you are just\nstoring each key once in the hashmap and never altering it, thus a\nstrbuf is overkill.\n\n> +       struct string_list *value_list ;\n\nWhy isn't this just a plain 'struct string_list'? Why the extra\nindirection of a pointer?\n\nDrop space before semicolon.\n\n> +};\n> +\n> +static int hashmap_init_v = 0;\n\nIt's not obvious what \"_v\" is meant to convey.\n\n> +static int config_cache_node_cmp(const struct config_cache_node *e1,\n> +                          const struct config_cache_node *e2, const char *key)\n> +{\n> +       return strcmp(e1->key.buf, key ? key : e2->key.buf);\n> +}\n> +\n> +static int config_cache_node_cmp_icase(const struct config_cache_node *e1,\n> +                                const struct config_cache_node *e2, const char* key)\n> +{\n> +       return strcasecmp(e1->key.buf, key ? key : e2->key.buf);\n> +}\n> +\n> +static void config_cache_init(const int icase)\n\n'const int' as a function argument is very unusual in this code base.\n(I found only a single instance of it in\nlog-tree.[ch]:parse_decorate_color_config.)\n\n> +{\n> +       hashmap_init(&config_cache, (hashmap_cmp_fn) (icase ? config_cache_node_cmp_icase\n> +                       : config_cache_node_cmp), 0);\n> +}\n> +\n> +static void config_cache_free(void)\n> +{\n> +       hashmap_free(&config_cache, 1);\n> +}\n\nDoesn't this leak the strbuf 'key' and string_list 'value_list'?\n\n> +static struct config_cache_node *config_cache_find_entry(const char *key)\n> +{\n> +       struct hashmap_entry k;\n> +       hashmap_entry_init(&k, strihash(key));\n\nHow does this unconditional use of case-insensitive strihash()\ninteract with the 'icase' argument of config_cache_init()?\n\n> +       return hashmap_get(&config_cache, &k, key);\n> +}\n> +\n> +static struct string_list *config_cache_get_value(const char *key)\n> +{\n> +       struct config_cache_node *e = config_cache_find_entry(key);\n> +       if (e == NULL)\n\nRephrase: if (!e)\n\n> +               return NULL;\n> +       else\n> +               return (e->value_list);\n\nUnnecessary parentheses.\n\n> +}\n\nA purely subject rewrite of the above:\n\n    return e ? e->value_list : NULL;\n\n> +static void config_cache_set_value(const char *key, const char *value)\n> +{\n> +       struct config_cache_node *e;\n> +\n> +       if (!value)\n> +               return;\n\nAgain, I had the same questions as Torsten. (\"Is this supposed to\ndelete the entry?\", \"Is it a programmer error?\", etc.) From a pure\nreading, it's not obvious why a NULL 'value' is handled this way. The\nintent might be clearer if the caller instead performs the check, and\ninvokes config_cache_set_value() only if 'value' is non-NULL.\n\n> +       e = config_cache_find_entry(key);\n> +       if (!e) {\n> +               e = malloc(sizeof(*e));\n> +               hashmap_entry_init(e, strihash(key));\n\nSame question as above regarding unconditional case-insensitive\nstrihash() versus the 'icase' argument of config_cache_init().\n\n> +               strbuf_init(&(e->key), 1024);\n\nWhy allocate such a large buffer for each key, especially when most\nkeys likely are quite short. Since the key is never altered after\nbeing assigned, this seems like considerable waste.\n\nAlso, unnecessary parentheses.\n\n> +               strbuf_addstr(&(e->key),key);\n\nUnnecessary parentheses. Add space after comma.\n\n> +               e->value_list = malloc(sizeof(struct string_list));\n> +               e->value_list->strdup_strings = 1;\n> +               e->value_list->nr = 0;\n> +               e->value_list->alloc = 0;\n> +               e->value_list->items = NULL;\n> +               e->value_list->cmp = NULL;\n\nThis string_list initialization code begs for a preparatory patch\nwhich adds a string_list_init() function (akin to strbuf_init()).\n\n> +               string_list_append(e->value_list, value);\n> +               hashmap_add(&config_cache, e);\n> +       }\n> +       else {\n\nCuddle the else with the braces: } else {\n\n> +               if (!unsorted_string_list_has_string((e->value_list), value))\n> +                       string_list_append((e->value_list), value);\n\nUnnecessary parentheses (on both lines).\n\nSince there is only an 'if' statement in this 'else' block, you could\nturn it into an 'else if', thus eliminating one level of indentation.\n\n> +       }\n> +}\n> +\n> +extern const char *git_config_get_string(const char *name)\n> +{\n> +       struct string_list *values;\n> +       int num_values;\n> +       char *result;\n> +       values = config_cache_get_value(name);\n> +       if (!values)\n> +               return NULL;\n> +       num_values = values->nr;\n> +       result = values->items[num_values-1].string ;\n\nAdd spaces around '-'. Drop space before semicolon.\n\n> +       return result;\n\nWhy do you need the 'result' variable if you are returning it\nimmediately after it's assigned?\n\n    return values->items[num_values - 1].string;\n\nOr, taking Torsten's recommendation into consideration to drop num_values:\n\n    return values->items[values->nr - 1].string;\n\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 +431,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 +1234,12 @@ 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> +       int icase = 1;\n> +       if (!hashmap_init_v) {\n> +               config_cache_init(icase);\n> +               hashmap_init_v = 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> --\n> 1.9.0.GIT\n"},{"id":"242853","messageId":"CAPig+cQZMNSZ-3C0zR3mvxp0n7C_L79wQex5L85Xr_w+8yvBZA@mail.gmail.com","threadId":"36754","inReplyTo":"1401125601-18249-3-git-send-email-tanayabh@gmail.com","subject":"Re: [RFC/PATCH 2/2] config: Add new query functions to the api","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-05-28T09:43:06Z","receivedAt":"2014-05-28T09:43:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 26, 2014 at 1:33 PM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> Subject: config: Add new query functions to the api\n\nThis sounds as if you are adding new functions to the header file.\nSaying \"... to the api docs\" would been clearer. Better:\n\n    config: document new query functions\n\n> Add explanations for `git_config_get_string_multi` and `git_config_get_string`\n> which utilize the config cache for querying in an non callback manner for a\n\ns/an/a/\ns/non/non-/\n\n> specific variable.\n>\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n\nOne might expect these functions to be documented in the same patch\nwhich introduces them. Since this patch is so small, it might make\nsense just to squash it into patch 1.\n\nMore below.\n\n> ---\n>  Documentation/technical/api-config.txt | 19 +++++++++++++++++++\n>  1 file changed, 19 insertions(+)\n>\n> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n> index 230b3a0..33b5b90 100644\n> --- a/Documentation/technical/api-config.txt\n> +++ b/Documentation/technical/api-config.txt\n> @@ -77,6 +77,25 @@ 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\nAll other section headers in this file capitalize each word.\n\n> +For programs wanting to query for specific variables in a non callback\n\ns/non-/\n\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 variable as the key string for which the corresponding values will\n> +  be retrieved and returned.\n> +\n> +They both read value from an internal cache generated previously from\n\nMinor observation: As this file is documenting the API, mentioning\nprivate implementation details such, as the \"internal cache\", may be\nfrowned upon (though probably doesn't matter in practice since this\n_is_ a technical document).\n\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 variable, sorted in order of increasing\n> +priority.\n> +\n>  Value Parsing Helpers\n>  ---------------------\n>\n> --\n> 1.9.0.GIT\n"}]}