{"thread":{"id":"36873","subject":"[PATCH v1] Git config cache & special querying api utilizing the cache","startedAt":"2014-06-09T12:49:05Z","lastAt":"2014-06-11T14:01:32Z","messageCount":11,"participants":["Tanay Abhra","Matthieu Moy","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243619","messageId":"1402318146-5062-1-git-send-email-tanayabh@gmail.com","threadId":"36873","inReplyTo":null,"subject":"[PATCH v1] Git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-09T12:49:05Z","receivedAt":"2014-06-09T12:49:05Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nI am taking this patch out of RFC.\n\n[PATCH V1]:Most of the invaluable suggestions by Eric Sunshine, Torsten Bogershausen and\n\tJeff King has been implemented[1]. Complete rewrite of config_cache*() family\n\tusing git_config() as hook as suggested by Jeff. Thanks for the review.\n\n[RFC V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen.\n\tAdded cache invalidation when config file is changed.[2]\n\tI am using git_config_set_multivar_in_file() as an update hook.\n\nThis is my first patch for this year's GSoC. My project is\n\"Git Config API improvements\". The link of my proposal is appended below [3].\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 the first time when any of the new query functions is\ncalled. It is invalidated by using git_config_set_multivar_in_file() as an\nupdate hook.\n\nWe add two new functions 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 using custom functions and it works\nfine.\n\n[1] http://marc.info/?t=140172066200006&r=1&w=2\n[2] http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html\n[3] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing\n\nCheers,\nTanay Abhra.\n\nTanay Abhra (1):\n  config: Add hashtable for config parsing & retrival\n\n Documentation/technical/api-config.txt |  18 +++++\n cache.h                                |   2 +\n config.c                               | 122 +++++++++++++++++++++++++++++++++\n 3 files changed, 142 insertions(+)\n\n-- \n1.9.0.GIT\n"},{"id":"243620","messageId":"1402318146-5062-2-git-send-email-tanayabh@gmail.com","threadId":"36873","inReplyTo":"1402318146-5062-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-09T12:49:06Z","receivedAt":"2014-06-09T12:49:06Z","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 Documentation/technical/api-config.txt |  18 +++++\n cache.h                                |   2 +\n config.c                               | 122 +++++++++++++++++++++++++++++++++\n 3 files changed, 142 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..5b6e376 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. for the same variable value in the repo config\n+will be preferred over value in user wide config).\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 \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..6f6b04e 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,120 @@ static struct config_source *cf;\n \n static int zlib_compression_seen;\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;\n+\n+static int config_cache_set_value(const char *key, const char *value);\n+\n+static int config_cache_entry_cmp_case(const struct config_cache_entry *e1,\n+\t\t\t\t const struct config_cache_entry *e2, const void *unused)\n+{\n+\treturn strcmp(e1->key, e2->key);\n+}\n+\n+static void config_cache_init(struct hashmap *config_cache)\n+{\n+\thashmap_init(config_cache, (hashmap_cmp_fn) config_cache_entry_cmp_case, 0);\n+}\n+\n+static int config_cache_callback(const char *key, const char *value, void *unused)\n+{\n+\tconfig_cache_set_value(key, value);\n+\treturn 0;\n+}\n+\n+static struct hashmap *get_config_cache(void)\n+{\n+\tstatic struct hashmap config_cache;\n+\tif (!hashmap_is_init) {\n+\t\tconfig_cache_init(&config_cache);\n+\t\thashmap_is_init = 1;\n+\t\tgit_config(config_cache_callback, NULL);\n+\t\treturn &config_cache;\n+\t}\n+\treturn &config_cache;\n+}\n+\n+static void config_cache_free(void)\n+{\n+\tstruct hashmap *config_cache;\n+\tstruct config_cache_entry *entry;\n+\tstruct hashmap_iter iter;\n+\tconfig_cache = get_config_cache();\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+\thashmap_is_init = 0;\n+}\n+\n+static struct config_cache_entry *config_cache_find_entry(const char *key)\n+{\n+\tstruct hashmap *config_cache;\n+\tstruct config_cache_entry k;\n+\tchar *normalized_key;\n+\tint baselength = 0, ret;\n+\tconfig_cache = get_config_cache();\n+\tret = git_config_parse_key(key, &normalized_key, &baselength);\n+\n+\tif (ret)\n+\t\treturn NULL;\n+\n+\thashmap_entry_init(&k, strhash(normalized_key));\n+\tk.key = (char*) key;\n+\treturn hashmap_get(config_cache, &k, NULL);\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 int config_cache_set_value(const char *key, const char *value)\n+{\n+\tstruct hashmap *config_cache;\n+\tstruct config_cache_entry *e;\n+\n+\tconfig_cache = get_config_cache();\n+\te = config_cache_find_entry(key);\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\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\tstring_list_append(e->value_list, value);\n+\t}\n+\treturn 0;\n+}\n+\n+extern const char *git_config_get_string(const char *key)\n+{\n+\tstruct string_list *values;\n+\tvalues = config_cache_get_value(key);\n+\tif (!values)\n+\t\treturn NULL;\n+\tassert(values->nr > 0);\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@@ -1700,6 +1816,12 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tlock = NULL;\n \tret = 0;\n \n+\t/* contents of config file has changed, so invalidate the\n+\t * config cache used by non-callback based query functions.\n+\t */\n+\tif (hashmap_is_init)\n+\t\tconfig_cache_free();\n+\n out_free:\n \tif (lock)\n \t\trollback_lock_file(lock);\n-- \n1.9.0.GIT\n"},{"id":"243625","messageId":"vpqmwdmtb2o.fsf@anie.imag.fr","threadId":"36873","inReplyTo":"1402318146-5062-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-09T14:24:47Z","receivedAt":"2014-06-09T14:24:47Z","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> +the highest priority(i.e. for the same variable value in the repo config\n                       ^\nmissing space.\n\n> +struct config_cache_entry {\n> +\tstruct hashmap_entry ent;\n> +\tchar *key;\n> +\tstruct string_list *value_list;\n> +};\n\nI guess this crossed Eric's remark about the fact that this is a\npointer.\n\n> +static int hashmap_is_init;\n\nI'd call it hashmap_initialized, but that's a matter of taste.\n\n> +static void config_cache_free(void)\n\nI didn't look closely enough to make sure there were no memory leak\nremaining, but did you check with valgrind --leak-check that it is the\ncase in practice?\n\n> +\t/* contents of config file has changed, so invalidate the\n> +\t * config cache used by non-callback based query functions.\n> +\t */\n\nStyle: Git usually writes multi-line comments\n\n/*\n * like\n * this\n */\n\n(not always applied, but documented in Documentation/CodingGuidelines)\n\n(no time for a more detailed review, sorry)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"243720","messageId":"CAPig+cTR5SdDF0QnKN8GFEKFkNEK_HoCDHj_vTnAbp37HK3kDA@mail.gmail.com","threadId":"36873","inReplyTo":"1402318146-5062-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T11:27:11Z","receivedAt":"2014-06-10T11:27:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 9, 2014 at 8:49 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/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n> index 230b3a0..5b6e376 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\nIt's strange to have a bulleted list for a single item. It can be\nstated more naturally as a full sentence, without the list.\n\nMore below.\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. for the same variable value in the repo config\n> +will be preferred over value in user wide config).\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> diff --git a/config.c b/config.c\n> index a30cb5c..6f6b04e 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,120 @@ static struct config_source *cf;\n>\n>  static int zlib_compression_seen;\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 and v2 reviews [1][2], and reiterated by\nMatthieu [3]: Why is 'value_list' a pointer to a structure rather than\njust a structure?\n\nThis is a genuine question. As a reviewer, I'm not sure how to\ninterpret your lack of response. I can't tell if you merely overlook\nor forget the question multiple times; if you don't understand the\ninquiry; or if you have a strong reason to prefer making this a\npointer. If you don't understand the question, it's okay to ask for\nclarification. If there is a strong reason for this to be a pointer,\nit should be explained. Without feedback from you, reviewers will\ncontinue asking the same question(s) upon each submission.\n\n> +};\n> +\n> +static int hashmap_is_init;\n> +\n> +static int config_cache_set_value(const char *key, const char *value);\n> +\n> +static int config_cache_entry_cmp_case(const struct config_cache_entry *e1,\n> +                                const struct config_cache_entry *e2, const void *unused)\n> +{\n> +       return strcmp(e1->key, e2->key);\n\nAs suggested by Peff [4], this comparison is now case-sensitive,\ninstead of case-insensitive as in the previous version. Rather than\nchanging the appended '_icase' to '_case', a more typical function\nname would be simply config_cache_entry_cmp().\n\n> +}\n> +\n> +static void config_cache_init(struct hashmap *config_cache)\n> +{\n> +       hashmap_init(config_cache, (hashmap_cmp_fn) config_cache_entry_cmp_case, 0);\n\nIn his review [4], Peff suggested giving config_cache_entry_cmp_case()\nthe correct hashmap_cmp_fn signature so that this cast can be dropped.\nIt's not clear whether you disagree with his advice or overlooked or\nforgot about it. If you disagree with his suggestion, it's okay to say\nso and explain why you feel the way you do, but without feedback from\nyou, he or another reviewer is going to continue suggesting the same\nchange.\n\n> +}\n> +\n> +static int config_cache_callback(const char *key, const char *value, void *unused)\n> +{\n> +       config_cache_set_value(key, value);\n> +       return 0;\n> +}\n> +\n> +static struct hashmap *get_config_cache(void)\n> +{\n> +       static struct hashmap config_cache;\n> +       if (!hashmap_is_init) {\n> +               config_cache_init(&config_cache);\n> +               hashmap_is_init = 1;\n> +               git_config(config_cache_callback, NULL);\n> +               return &config_cache;\n\nWhy do you 'return' here rather than just falling through to the\n'return' outside the conditional?\n\n> +       }\n> +       return &config_cache;\n> +}\n> +\n> +static void config_cache_free(void)\n> +{\n> +       struct hashmap *config_cache;\n> +       struct config_cache_entry *entry;\n> +       struct hashmap_iter iter;\n> +       config_cache = get_config_cache();\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> +       hashmap_is_init = 0;\n> +}\n> +\n> +static struct config_cache_entry *config_cache_find_entry(const char *key)\n> +{\n> +       struct hashmap *config_cache;\n> +       struct config_cache_entry k;\n> +       char *normalized_key;\n> +       int baselength = 0, ret;\n> +       config_cache = get_config_cache();\n> +       ret = git_config_parse_key(key, &normalized_key, &baselength);\n\nSince you neither care about nor ever reference 'baselength', you\nshould just pass NULL as the third argument.\n\n> +       if (ret)\n> +               return NULL;\n> +\n> +       hashmap_entry_init(&k, strhash(normalized_key));\n> +       k.key = (char*) key;\n\nThis looks fishy. You're hashing based upon 'normalized_key' but then\ncomparing against the unnormalized 'key'. Peff's suggestion [4] was to\nstore the normalized key in the hash, which means that you should use\n'normalized_key' for both hashing and comparing. (See additional\ncommentary about this issue below in config_cache_set_value().)\n\nStyle: (char *)key\n\n> +       return hashmap_get(config_cache, &k, NULL);\n\nYou're leaking 'normalized_key', which git_config_parse_key()\nallocated on your behalf.\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 int config_cache_set_value(const char *key, const char *value)\n> +{\n> +       struct hashmap *config_cache;\n> +       struct config_cache_entry *e;\n> +\n> +       config_cache = get_config_cache();\n> +       e = config_cache_find_entry(key);\n> +       if (!e) {\n> +               e = xmalloc(sizeof(*e));\n> +               hashmap_entry_init(e, strhash(key));\n\nThe hash computed to store the key should be the same as the hash to\nlook it up. In config_cache_find_entry(), you're correctly computing\nthe hash based upon the normalized key, but here you're doing so from\nthe unnormalized key.\n\n> +               e->key = xstrdup(key);\n\nLikewise. Normalized keys should be stored in the hash, not unnormalized.\n\nYou should be invoking git_config_parse_key() to normalize the key\nboth for hashing and storing.\n\nNote, also, that call git_config_parse_key() allocates the normalize\nkey on your behalf, so you do *not* want to xstrdup() it here.\n\n> +               e->value_list = xcalloc(sizeof(struct string_list), 1);\n> +               e->value_list->strdup_strings = 1;\n\nThis code is perhaps too intimate with the implementation of\nstring_list. It may work today (because it is safe to initialize all\nstring_list fields to 0 via xcalloc()), but a future change to the\nstring_list implementation may invalidate that assumption. The\ninitialization macros in string-list.h exist to preserve the\nabstraction, so you don't have to know the details of initialization.\nThe macros are not suitable for your use-case, but an initialization\nfunction, such as string_list_init(), would be suitable, and it would\nbe appropriate to add such a function in a preparatory patch (as\nalready suggested by [1]).\n\n> +               string_list_append(e->value_list, value);\n> +               hashmap_add(config_cache, e);\n> +       } else {\n> +               string_list_append(e->value_list, value);\n> +       }\n> +       return 0;\n> +}\n> +\n> +extern const char *git_config_get_string(const char *key)\n\nDrop the 'extern'. The header already declares it such.\n\n> +{\n> +       struct string_list *values;\n> +       values = config_cache_get_value(key);\n> +       if (!values)\n> +               return NULL;\n> +       assert(values->nr > 0);\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\nDrop the 'extern'. The header already declares it such.\n\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> @@ -1700,6 +1816,12 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>         lock = NULL;\n>         ret = 0;\n>\n> +       /* contents of config file has changed, so invalidate the\n> +        * config cache used by non-callback based query functions.\n> +        */\n> +       if (hashmap_is_init)\n> +               config_cache_free();\n> +\n>  out_free:\n>         if (lock)\n>                 rollback_lock_file(lock);\n> --\n> 1.9.0.GIT\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[2]: http://thread.gmane.org/gmane.comp.version-control.git/250566/focus=251058\n[3]: http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251079\n[4]: http://thread.gmane.org/gmane.comp.version-control.git/250566/focus=250618\n"},{"id":"243721","messageId":"CAPig+cSosjLsRekAb6+jqmL3Lcyjpp5Z7MPWXJB+j+tj_g7FGg@mail.gmail.com","threadId":"36873","inReplyTo":"1402318146-5062-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T11:45:45Z","receivedAt":"2014-06-10T11:45:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"One additional comment...\n\nOn Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> +static int config_cache_set_value(const char *key, const char *value)\n> +{\n> +       struct hashmap *config_cache;\n> +       struct config_cache_entry *e;\n> +\n> +       config_cache = get_config_cache();\n> +       e = config_cache_find_entry(key);\n> +       if (!e) {\n> +               e = xmalloc(sizeof(*e));\n> +               hashmap_entry_init(e, strhash(key));\n> +               e->key = xstrdup(key);\n> +               e->value_list = xcalloc(sizeof(struct string_list), 1);\n\nOrder of xcalloc() arguments is incorrect [1].\n\n[1]: http://git.661346.n2.nabble.com/PATCH-00-15-Rearrange-xcalloc-arguments-td7611675.html\n\n> +               e->value_list->strdup_strings = 1;\n> +               string_list_append(e->value_list, value);\n> +               hashmap_add(config_cache, e);\n> +       } else {\n> +               string_list_append(e->value_list, value);\n> +       }\n> +       return 0;\n> +}\n"},{"id":"243722","messageId":"CAPig+cSjAxVN8GLit-nZ9sUxmMvC1KcSFMJayJSarQEcqyYvjw@mail.gmail.com","threadId":"36873","inReplyTo":"vpqmwdmtb2o.fsf@anie.imag.fr","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T11:51:13Z","receivedAt":"2014-06-10T11:51:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 9, 2014 at 10:24 AM, Matthieu Moy\n<Matthieu.Moy@grenoble-inp.fr> wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>> +struct config_cache_entry {\n>> +     struct hashmap_entry ent;\n>> +     char *key;\n>> +     struct string_list *value_list;\n>> +};\n>\n> I guess this crossed Eric's remark about the fact that this is a\n> pointer.\n>\n>> +static void config_cache_free(void)\n>\n> I didn't look closely enough to make sure there were no memory leak\n> remaining, but did you check with valgrind --leak-check that it is the\n> case in practice?\n\nIt does leak the config_cache_entry::value_list member which was\nxcalloc()'d in config_cache_set_value().  (I didn't mention it in my\nreviews because I was expecting 'value_list' to be changed to a plain\nstructure rather than a pointer to a structure.)\n"},{"id":"243725","messageId":"5396FB7C.6000709@gmail.com","threadId":"36873","inReplyTo":"CAPig+cTR5SdDF0QnKN8GFEKFkNEK_HoCDHj_vTnAbp37HK3kDA@mail.gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-10T12:35:08Z","receivedAt":"2014-06-10T12:35:08Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nThanks for the review, Eric. I have replied to your comments below,\nI will try to reply early and more promptly now.\n\nOn 06/10/2014 04:27 AM, Eric Sunshine wrote:\n>> ---\n>> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n>> index 230b3a0..5b6e376 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> It's strange to have a bulleted list for a single item. It can be\n> stated more naturally as a full sentence, without the list.\n\nPoint Noted.\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. for the same variable value in the repo config\n>> +will be preferred over value in user wide config).\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>> diff --git a/config.c b/config.c\n>> index a30cb5c..6f6b04e 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,120 @@ static struct config_source *cf;\n>>\n>>  static int zlib_compression_seen;\n>>\n>> +struct config_cache_entry {\n>> +       struct hashmap_entry ent;\n>> +       char *key;\n>> +       struct string_list *value_list;\n> \n> Same question as in my v1 and v2 reviews [1][2], and reiterated by\n> Matthieu [3]: Why is 'value_list' a pointer to a structure rather than\n> just a structure?\n> \n\nSorry for the lack of response on this part. I thought choosing a pointer to a\nstructure or just the structure itself was a stylistic choice. Since most of the\nfunctions just pass the pointer to this struct, I thought it would be more natural to\nuse it. But I have changed my mind on this one. In the next iteration I will be using\na plane old struct.\n\n> \n>> +};\n>> +\n>> +static int hashmap_is_init;\n>> +\n>> +static int config_cache_set_value(const char *key, const char *value);\n>> +\n>> +static int config_cache_entry_cmp_case(const struct config_cache_entry *e1,\n>> +                                const struct config_cache_entry *e2, const void *unused)\n>> +{\n>> +       return strcmp(e1->key, e2->key);\n> \n> As suggested by Peff [4], this comparison is now case-sensitive,\n> instead of case-insensitive as in the previous version. Rather than\n> changing the appended '_icase' to '_case', a more typical function\n> name would be simply config_cache_entry_cmp().\n\nNoted.\n\n>> +}\n>> +\n>> +static void config_cache_init(struct hashmap *config_cache)\n>> +{\n>> +       hashmap_init(config_cache, (hashmap_cmp_fn) config_cache_entry_cmp_case, 0);\n> \n> In his review [4], Peff suggested giving config_cache_entry_cmp_case()\n> the correct hashmap_cmp_fn signature so that this cast can be dropped.\n> It's not clear whether you disagree with his advice or overlooked or\n> forgot about it. If you disagree with his suggestion, it's okay to say\n> so and explain why you feel the way you do, but without feedback from\n> you, he or another reviewer is going to continue suggesting the same\n> change.\n\nNow on this one, I checked the code thoroughly. Every single hashmap_init()\nincantation in git code has a hashmap_cmp_fn cast. I don't think that it is\nnecessary to do so. Is it?\n\n>> +}\n>> +\n>> +static int config_cache_callback(const char *key, const char *value, void *unused)\n>> +{\n>> +       config_cache_set_value(key, value);\n>> +       return 0;\n>> +}\n>> +\n>> +static struct hashmap *get_config_cache(void)\n>> +{\n>> +       static struct hashmap config_cache;\n>> +       if (!hashmap_is_init) {\n>> +               config_cache_init(&config_cache);\n>> +               hashmap_is_init = 1;\n>> +               git_config(config_cache_callback, NULL);\n>> +               return &config_cache;\n> \n> Why do you 'return' here rather than just falling through to the\n> 'return' outside the conditional?\n\nNoted.\n\n>> +static struct config_cache_entry *config_cache_find_entry(const char *key)\n>> +{\n>> +       struct hashmap *config_cache;\n>> +       struct config_cache_entry k;\n>> +       char *normalized_key;\n>> +       int baselength = 0, ret;\n>> +       config_cache = get_config_cache();\n>> +       ret = git_config_parse_key(key, &normalized_key, &baselength);\n> \n> Since you neither care about nor ever reference 'baselength', you\n> should just pass NULL as the third argument.\n> \n\nNoted.\n\n>> +       if (ret)\n>> +               return NULL;\n>> +\n>> +       hashmap_entry_init(&k, strhash(normalized_key));\n>> +       k.key = (char*) key;\n> \n> This looks fishy. You're hashing based upon 'normalized_key' but then\n> comparing against the unnormalized 'key'. Peff's suggestion [4] was to\n> store the normalized key in the hash, which means that you should use\n> 'normalized_key' for both hashing and comparing. (See additional\n> commentary about this issue below in config_cache_set_value().)\n\nOuch, this I had corrected in a future commit. But forgot to include in\nthis patch.\n\n> Style: (char *)key\n\nNoted. In function arguments the code uses (char*) key. I copied it from there.\n:)\n\n>> +       return hashmap_get(config_cache, &k, NULL);\n> \n> You're leaking 'normalized_key', which git_config_parse_key()\n> allocated on your behalf.\n>\nNoted. I will now check with valgrind before sending any future series.\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 int config_cache_set_value(const char *key, const char *value)\n>> +{\n>> +       struct hashmap *config_cache;\n>> +       struct config_cache_entry *e;\n>> +\n>> +       config_cache = get_config_cache();\n>> +       e = config_cache_find_entry(key);\n>> +       if (!e) {\n>> +               e = xmalloc(sizeof(*e));\n>> +               hashmap_entry_init(e, strhash(key));\n> \n> The hash computed to store the key should be the same as the hash to\n> look it up. In config_cache_find_entry(), you're correctly computing\n> the hash based upon the normalized key, but here you're doing so from\n> the unnormalized key.\n> \n>> +               e->key = xstrdup(key);\n> \n> Likewise. Normalized keys should be stored in the hash, not unnormalized.\n> \n> You should be invoking git_config_parse_key() to normalize the key\n> both for hashing and storing.\n> \n> Note, also, that call git_config_parse_key() allocates the normalize\n> key on your behalf, so you do *not* want to xstrdup() it here.\n\nconfig_cache_set_value() gets its values from git_config() as it the callback.\ngit_config() feeds the callback only normalized values by using functions like\nget_extended_base_var(), get_base_var() etc. Thus, I didn't use\ngit_config_parse_key(). Please correct me if I am wrong, but I tested this case\nthoroughly.\n\n>> +               e->value_list = xcalloc(sizeof(struct string_list), 1);\n>> +               e->value_list->strdup_strings = 1;\n> \n> This code is perhaps too intimate with the implementation of\n> string_list. It may work today (because it is safe to initialize all\n> string_list fields to 0 via xcalloc()), but a future change to the\n> string_list implementation may invalidate that assumption. The\n> initialization macros in string-list.h exist to preserve the\n> abstraction, so you don't have to know the details of initialization.\n> The macros are not suitable for your use-case, but an initialization\n> function, such as string_list_init(), would be suitable, and it would\n> be appropriate to add such a function in a preparatory patch (as\n> already suggested by [1]).\n\nNoted. As I am going to use a simple struct for the list, this won't be\na problem.\n\n>> +               string_list_append(e->value_list, value);\n>> +               hashmap_add(config_cache, e);\n>> +       } else {\n>> +               string_list_append(e->value_list, value);\n>> +       }\n>> +       return 0;\n>> +}\n>> +\n>> +extern const char *git_config_get_string(const char *key)\n> \n> Drop the 'extern'. The header already declares it such.\n>\n\nNoted.\n\n>> +{\n>> +       struct string_list *values;\n>> +       values = config_cache_get_value(key);\n>> +       if (!values)\n>> +               return NULL;\n>> +       assert(values->nr > 0);\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> Drop the 'extern'. The header already declares it such.\n> \n\nNoted.\n\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>> @@ -1700,6 +1816,12 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>>         lock = NULL;\n>>         ret = 0;\n>>\n>> +       /* contents of config file has changed, so invalidate the\n>> +        * config cache used by non-callback based query functions.\n>> +        */\n>> +       if (hashmap_is_init)\n>> +               config_cache_free();\n>> +\n>>  out_free:\n>>         if (lock)\n>>                 rollback_lock_file(lock);\n>> --\n>> 1.9.0.GIT\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> [2]: http://thread.gmane.org/gmane.comp.version-control.git/250566/focus=251058\n> [3]: http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251079\n> [4]: http://thread.gmane.org/gmane.comp.version-control.git/250566/focus=250618\n> \n"},{"id":"243726","messageId":"5396FBFB.1010101@gmail.com","threadId":"36873","inReplyTo":"CAPig+cSosjLsRekAb6+jqmL3Lcyjpp5Z7MPWXJB+j+tj_g7FGg@mail.gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-10T12:37:15Z","receivedAt":"2014-06-10T12:37:15Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/10/2014 04:45 AM, Eric Sunshine wrote:\n> One additional comment...\n> \n> On Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n>> +static int config_cache_set_value(const char *key, const char *value)\n>> +{\n>> +       struct hashmap *config_cache;\n>> +       struct config_cache_entry *e;\n>> +\n>> +       config_cache = get_config_cache();\n>> +       e = config_cache_find_entry(key);\n>> +       if (!e) {\n>> +               e = xmalloc(sizeof(*e));\n>> +               hashmap_entry_init(e, strhash(key));\n>> +               e->key = xstrdup(key);\n>> +               e->value_list = xcalloc(sizeof(struct string_list), 1);\n> \n> Order of xcalloc() arguments is incorrect [1].\n> \n\nNoted. Thanks for pointing it out.\n\n> [1]: http://git.661346.n2.nabble.com/PATCH-00-15-Rearrange-xcalloc-arguments-td7611675.html\n> \n>> +               e->value_list->strdup_strings = 1;\n>> +               string_list_append(e->value_list, value);\n>> +               hashmap_add(config_cache, e);\n>> +       } else {\n>> +               string_list_append(e->value_list, value);\n>> +       }\n>> +       return 0;\n>> +}\n"},{"id":"243779","messageId":"CAPig+cT0gLzMV7p9-ZavZn2t00Oh_-4oQWZuk8ptYWEtmCuYVA@mail.gmail.com","threadId":"36873","inReplyTo":"5396FB7C.6000709@gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T21:28:03Z","receivedAt":"2014-06-10T21:28:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 10, 2014 at 8:35 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> Thanks for the review, Eric. I have replied to your comments below,\n> I will try to reply early and more promptly now.\n\nThanks for responding. More below.\n\n> On 06/10/2014 04:27 AM, Eric Sunshine wrote:\n>>> ---\n>>> diff --git a/config.c b/config.c\n>>> index a30cb5c..6f6b04e 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,120 @@ static struct config_source *cf;\n>>>\n>>>  static int zlib_compression_seen;\n>>>\n>>> +struct config_cache_entry {\n>>> +       struct hashmap_entry ent;\n>>> +       char *key;\n>>> +       struct string_list *value_list;\n>>\n>> Same question as in my v1 and v2 reviews [1][2], and reiterated by\n>> Matthieu [3]: Why is 'value_list' a pointer to a structure rather than\n>> just a structure?\n>\n> Sorry for the lack of response on this part. I thought choosing a pointer to a\n> structure or just the structure itself was a stylistic choice.\n\nEach point or question raised in a review is meaningful to the\nreviewer and deserves some sort of response or consideration, even if\nyou don't agree with the reviewer's opinion. It's okay to have\nstylistic or other preferences (and to argue for them), but unless you\nvoice them, the reviewer is likely to conclude that his review effort\nis wasted.\n\nIn this case, it's not a stylistic consideration which prompted the\nquestion, but practical concern. (More on that below.)\n\n> Since most of the\n> functions just pass the pointer to this struct, I thought it would be more natural to\n> use it. But I have changed my mind on this one. In the next iteration I will be using\n> a plane old struct.\n\nThere are practical reasons for choosing a structure rather than a\npointer to a structure. Problems with a pointer include:\n\n- Expense of an extra (unnecessary) heap allocation. You allocate both\nconfig_cache_entry and string_list on the heap for each entry being\ninserted into the hash, rather than allocating only a\nconfig_cache_entry.\n\n- Potential additional heap fragmentation.\n\n- Increased possibility of leaking memory. The current implementation\nof config_cache_free() drives this point home: although it correctly\nclears the string_list, it leaks the heap-allocated string_list\nitself.\n\n- Potentially misleading readers of the code into thinking that there\nis an important reason for the string_list to be allocated separately\nfrom its containing object (for instance, \"is the string list shared?\"\nor \"does the string list need to exist beyond the lifetime of the\nconfig_cache_entry?\").\n\n>>> +}\n>>> +\n>>> +static void config_cache_init(struct hashmap *config_cache)\n>>> +{\n>>> +       hashmap_init(config_cache, (hashmap_cmp_fn) config_cache_entry_cmp_case, 0);\n>>\n>> In his review [4], Peff suggested giving config_cache_entry_cmp_case()\n>> the correct hashmap_cmp_fn signature so that this cast can be dropped.\n>> It's not clear whether you disagree with his advice or overlooked or\n>> forgot about it. If you disagree with his suggestion, it's okay to say\n>> so and explain why you feel the way you do, but without feedback from\n>> you, he or another reviewer is going to continue suggesting the same\n>> change.\n>\n> Now on this one, I checked the code thoroughly. Every single hashmap_init()\n> incantation in git code has a hashmap_cmp_fn cast.\n\nThat's a good argument for explaining to Peff why you did it this way,\nbut unless you voice this, he will likely feel that you simply ignored\nhis review comment.\n\n> I don't think that it is necessary to do so. Is it?\n\nThere are good reasons for avoiding the cast. Jonathan gives one such\nreason here [1]. In that case, he let it slide for the reason you\nmention: existing hashmap clients use the cast. (However, the\nimplication is that it would be nice for someone -- not necessarily\nyou -- to submit a patch series eliminating the casts.)\n\nWhether Peff will be swayed by the same argument, only he can answer.\n(If he doesn't answer, taking [1] into consideration, you'd probably\nbe on safe ground to continue using the cast.)\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243818/focus=243917\n(see his comments about hashmap_init()).\n\n>>> +       if (ret)\n>>> +               return NULL;\n>>> +\n>>> +       hashmap_entry_init(&k, strhash(normalized_key));\n>>> +       k.key = (char*) key;\n>>\n>> This looks fishy. You're hashing based upon 'normalized_key' but then\n>> comparing against the unnormalized 'key'. Peff's suggestion [4] was to\n>> store the normalized key in the hash, which means that you should use\n>> 'normalized_key' for both hashing and comparing. (See additional\n>> commentary about this issue below in config_cache_set_value().)\n>\n> Ouch, this I had corrected in a future commit. But forgot to include in\n> this patch.\n\n[Leaving unsnipped since it has relevance below.]\n\n>> Style: (char *)key\n>\n> Noted. In function arguments the code uses (char*) key. I copied it from there.\n> :)\n\nIndeed, there's an unfortunate lack of consistency: in fact, even\nconfig.c already has a mix of \"(x*)y\", \"(x *)y\", and \"(x *) y\", but I\nthink \"(x *)y\" is the one preferred and seems to be the most common.\n\n>>> +static int config_cache_set_value(const char *key, const char *value)\n>>> +{\n>>> +       struct hashmap *config_cache;\n>>> +       struct config_cache_entry *e;\n>>> +\n>>> +       config_cache = get_config_cache();\n>>> +       e = config_cache_find_entry(key);\n>>> +       if (!e) {\n>>> +               e = xmalloc(sizeof(*e));\n>>> +               hashmap_entry_init(e, strhash(key));\n>>\n>> The hash computed to store the key should be the same as the hash to\n>> look it up. In config_cache_find_entry(), you're correctly computing\n>> the hash based upon the normalized key, but here you're doing so from\n>> the unnormalized key.\n>>\n>>> +               e->key = xstrdup(key);\n>>\n>> Likewise. Normalized keys should be stored in the hash, not unnormalized.\n>>\n>> You should be invoking git_config_parse_key() to normalize the key\n>> both for hashing and storing.\n>>\n>> Note, also, that call git_config_parse_key() allocates the normalize\n>> key on your behalf, so you do *not* want to xstrdup() it here.\n>\n> config_cache_set_value() gets its values from git_config() as it the callback.\n> git_config() feeds the callback only normalized values by using functions like\n> get_extended_base_var(), get_base_var() etc. Thus, I didn't use\n> git_config_parse_key(). Please correct me if I am wrong, but I tested this case\n> thoroughly.\n\nYou're right. Documentation/api-config.txt even states that 'key' is\nalready normalized here. Thanks for pointing it out. So, the only\nthing in need of fixing is the fishy bit in config_cache_find_entry()\nwhere it is incorrectly searching for the unnormalized key.\n\n>>> +               e->value_list = xcalloc(sizeof(struct string_list), 1);\n>>> +               e->value_list->strdup_strings = 1;\n>>\n>> This code is perhaps too intimate with the implementation of\n>> string_list. It may work today (because it is safe to initialize all\n>> string_list fields to 0 via xcalloc()), but a future change to the\n>> string_list implementation may invalidate that assumption. The\n>> initialization macros in string-list.h exist to preserve the\n>> abstraction, so you don't have to know the details of initialization.\n>> The macros are not suitable for your use-case, but an initialization\n>> function, such as string_list_init(), would be suitable, and it would\n>> be appropriate to add such a function in a preparatory patch (as\n>> already suggested by [1]).\n>\n> Noted. As I am going to use a simple struct for the list, this won't be\n> a problem.\n\nEven after 'value_list' becomes a simple struct rather than a pointer\nto a struct, the string_list will still require initialization, and\nthe STRING_LIST_INIT_* macros still don't help your case, so it\nremains a problem. The suggested preparatory patch introducing a new\nstring_list_init() function would allow you to initialize your\nstring_list while preserving the abstraction.\n"},{"id":"243865","messageId":"CAPig+cQ13nATVpZFZhFLB0Bm9eBLQEyuZkao02ERNzkbGNLEGA@mail.gmail.com","threadId":"36873","inReplyTo":"1402318146-5062-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v1] config: Add hashtable for config parsing & retrival","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-10T23:30:23Z","receivedAt":"2014-06-10T23:30:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"One other minor point...\n\nOn Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> Subject: config: Add hashtable for config parsing & retrival\n\ns/retrival/retrieval/\n\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"},{"id":"243909","messageId":"vpqr42v35qb.fsf@anie.imag.fr","threadId":"36873","inReplyTo":"1402318146-5062-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v1] Git config cache & special querying api utilizing the cache","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-11T14:01:32Z","receivedAt":"2014-06-11T14:01:32Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> I have run the tests and debug the code using custom functions and it works\n> fine.\n\nI understand that you wrote the custom functions for you, but didn't\ninclude them in the patch, right?\n\nIf so, wouldn't it make sense to include these as unit-tests for your\ncode? See what Git already does with test-*.c, e.g.\n\n$ git grep test-hashmap\n.gitignore:/test-hashmap\nDocumentation/technical/api-hashmap.txt:See test-hashmap.c for an example using arbitrary-length strings as keys.\nMakefile:TEST_PROGRAMS_NEED_X += test-hashmap\nt/t0011-hashmap.sh:     echo \"$1\" | test-hashmap $3 > actual &&\nt/t0011-hashmap.sh:     cat in | test-hashmap > out &&\ntest-hashmap.c: * Usage: time echo \"perfhashmap method rounds\" | test-hashmap\n\nfor examples. The resulting binary would be a good candidate for\nvalgrind --leak-check=full.\n\nThe tests are not just for you, they are also:\n\n* For reviewers, to show an example of correct usage of the new API,\n\n* For people of the future, to avoid regressions.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}