{"thread":{"id":"36921","subject":"[PATCH v2 0/2] Git config cache & special querying api utilizing the cache","startedAt":"2014-06-16T08:27:10Z","lastAt":"2014-06-17T22:10:26Z","messageCount":11,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"244238","messageId":"1402907232-24629-1-git-send-email-tanayabh@gmail.com","threadId":"36921","inReplyTo":null,"subject":"[PATCH v2 0/2] Git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T08:27:10Z","receivedAt":"2014-06-16T08:27:10Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\n[PATCH v2]:Changed the string_list to a struct instead of pointer to a struct.\n\tAdded string-list initilization functions.\n\tMinor mistakes corrected acoording to review comments[4]. Thanks to\n\tEric and Matthieu for their review.\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\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[4] http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251369\n\nCheers,\nTanay Abhra.\n\n\nTanay Abhra (2):\n  string-list: Add string_list initializer helper functions\n  config: Add hashtable for config parsing & retrieval\n\n Documentation/technical/api-config.txt |  17 +++++\n cache.h                                |   2 +\n config.c                               | 123 +++++++++++++++++++++++++++++++++\n string-list.c                          |  18 +++++\n string-list.h                          |   3 +\n 5 files changed, 163 insertions(+)\n\n-- \n1.9.0.GIT\n"},{"id":"244239","messageId":"1402907232-24629-2-git-send-email-tanayabh@gmail.com","threadId":"36921","inReplyTo":"1402907232-24629-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 1/2] string-list: Add string_list initializer helper functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T08:27:11Z","receivedAt":"2014-06-16T08:27:11Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"When a compound construct like a string_list within another\nstruct is used, the default initializer macros are useless.\nFor such cases add helper functions for string_list\ninitialization for both DUP and NODUP modes.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n string-list.c | 18 ++++++++++++++++++\n string-list.h |  3 +++\n 2 files changed, 21 insertions(+)\n\ndiff --git a/string-list.c b/string-list.c\nindex aabb25e..8c3a4eb 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,6 +1,24 @@\n #include \"cache.h\"\n #include \"string-list.h\"\n \n+void string_list_init_nodup(struct string_list *list)\n+{\n+\tlist->items = NULL;\n+\tlist->nr = 0;\n+\tlist->alloc = 0;\n+\tlist->strdup_strings = 0;\n+\tlist->cmp = NULL;\n+}\n+\n+void string_list_init_dup(struct string_list *list)\n+{\n+\tlist->items = NULL;\n+\tlist->nr = 0;\n+\tlist->alloc = 0;\n+\tlist->strdup_strings = 1;\n+\tlist->cmp = NULL;\n+}\n+\n /* if there is no exact match, point to the index where the entry could be\n  * inserted */\n static int get_entry_index(const struct string_list *list, const char *string,\ndiff --git a/string-list.h b/string-list.h\nindex de6769c..8ae3376 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -18,6 +18,9 @@ struct string_list {\n #define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0 }\n #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1 }\n \n+void string_list_init_nodup(struct string_list *list);\n+void string_list_init_dup(struct string_list *list);\n+\n void print_string_list(const struct string_list *p, const char *text);\n void string_list_clear(struct string_list *list, int free_util);\n \n-- \n1.9.0.GIT\n"},{"id":"244240","messageId":"1402907232-24629-3-git-send-email-tanayabh@gmail.com","threadId":"36921","inReplyTo":"1402907232-24629-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v2 2/2] config: Add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T08:27:12Z","receivedAt":"2014-06-16T08:27:12Z","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 |  17 +++++\n cache.h                                |   2 +\n config.c                               | 123 +++++++++++++++++++++++++++++++++\n 3 files changed, 142 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..23e8ae3 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,23 @@ 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+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 \ndiff --git a/cache.h b/cache.h\nindex 1e4b4f0..7343692 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1295,6 +1295,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__)\n #define config_error_nonbool(s) (config_error_nonbool(s), const_error())\n #endif\ndiff --git a/config.c b/config.c\nindex 3c3e314..2fd3656 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_initialized;\n+\n+static int config_cache_set_value(const char *key, const char *value);\n+\n+static int config_cache_entry_cmp(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, 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_initialized) {\n+\t\tconfig_cache_init(&config_cache);\n+\t\thashmap_initialized = 1;\n+\t\tgit_config(config_cache_callback, NULL);\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_initialized = 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+\tstruct config_cache_entry *found_entry;\n+\tchar *normalized_key;\n+\tint ret;\n+\tconfig_cache = get_config_cache();\n+\tret = git_config_parse_key(key, &normalized_key, NULL);\n+\n+\tif (ret)\n+\t\treturn NULL;\n+\n+\thashmap_entry_init(&k, strhash(normalized_key));\n+\tk.key = (char *)normalized_key;\n+\tfound_entry = hashmap_get(config_cache, &k, NULL);\n+\tfree(normalized_key);\n+\treturn found_entry;\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+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\tstring_list_init_dup(&e->value_list);\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+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+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@@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tlock = NULL;\n \tret = 0;\n \n+\t/*\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_initialized)\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":"244268","messageId":"vpqy4ww230l.fsf@anie.imag.fr","threadId":"36921","inReplyTo":"1402907232-24629-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-16T17:11:22Z","receivedAt":"2014-06-16T17:11:22Z","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 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\nThis describes rather well _what_ your patch does, but the most\nimportant part of a commit message is to justify _why_ the change is\ngood, and why the way you implemented it is good.\n\nThink of it as an way to convince reviewers to accept your patch.\n\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n>  Documentation/technical/api-config.txt |  17 +++++\n>  cache.h                                |   2 +\n>  config.c                               | 123 +++++++++++++++++++++++++++++++++\n>  3 files changed, 142 insertions(+)\n\nI'm still concerned about the fact that there is no test. At this point,\ngit_config_get_string_multi and git_config_get_string are basically dead\ncode.\n\nI'm sure you did experiments to test these functions, run them through\nvalgrind, ... Now, you need to let other people reproduce these\nexperiments. \"other people\" can be reviewers, now, or anyone looking for\nbugs or regressions in the future.\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_initialized) {\n> +\t\tconfig_cache_init(&config_cache);\n> +\t\thashmap_initialized = 1;\n> +\t\tgit_config(config_cache_callback, NULL);\n> +\t}\n> +\treturn &config_cache;\n> +}\n\nGood, just enough code for the job :-).\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> +\tstruct config_cache_entry *found_entry;\n> +\tchar *normalized_key;\n> +\tint ret;\n> +\tconfig_cache = get_config_cache();\n> +\tret = git_config_parse_key(key, &normalized_key, NULL);\n> +\n> +\tif (ret)\n> +\t\treturn NULL;\n> +\n> +\thashmap_entry_init(&k, strhash(normalized_key));\n> +\tk.key = (char *)normalized_key;\n\nNo need to cast.\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\tstring_list_init_dup(&e->value_list);\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\nI find the function name a bit confusing, as it does not \"set\" in the\nsense \"override any previous value\". Wouldn't this be better named\nconfig_cache_add_value? Or perhaps a comment would help.\n\n> @@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>  \tlock = NULL;\n>  \tret = 0;\n>  \n> +\t/*\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\nSpaces after stars:\n\n/*\n * Contents of config file has changed, so invalidate the\n * config cache used by non-callback based query functions.\n */\n\n(I think the \"s\" of \"contents\" should be dropped too).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"244269","messageId":"539F293D.9000602@gmail.com","threadId":"36921","inReplyTo":"vpqy4ww230l.fsf@anie.imag.fr","subject":"Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-16T17:28:29Z","receivedAt":"2014-06-16T17:28:29Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/16/2014 10:11 AM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\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> This describes rather well _what_ your patch does, but the most\n> important part of a commit message is to justify _why_ the change is\n> good, and why the way you implemented it is good.\n> \n> Think of it as an way to convince reviewers to accept your patch.\n\nOkay, but isn't the content of the cover letter is doing that for now.\nShould I put some part of it in here?\n\n>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n>> ---\n>>  Documentation/technical/api-config.txt |  17 +++++\n>>  cache.h                                |   2 +\n>>  config.c                               | 123 +++++++++++++++++++++++++++++++++\n>>  3 files changed, 142 insertions(+)\n> \n> I'm still concerned about the fact that there is no test. At this point,\n> git_config_get_string_multi and git_config_get_string are basically dead\n> code.\n> \n> I'm sure you did experiments to test these functions, run them through\n> valgrind, ... Now, you need to let other people reproduce these\n> experiments. \"other people\" can be reviewers, now, or anyone looking for\n> bugs or regressions in the future.\n>\n\nYeah, I have run the experiments. I will add a test file for it. I should have\nappended it to this series only, my fault. :) A stray observation, Git has very less\nunit tests, compared to the comprehensive test directory for commands.\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>> +\tstruct config_cache_entry *found_entry;\n>> +\tchar *normalized_key;\n>> +\tint ret;\n>> +\tconfig_cache = get_config_cache();\n>> +\tret = git_config_parse_key(key, &normalized_key, NULL);\n>> +\n>> +\tif (ret)\n>> +\t\treturn NULL;\n>> +\n>> +\thashmap_entry_init(&k, strhash(normalized_key));\n>> +\tk.key = (char *)normalized_key;\n> \n> No need to cast.\n> \n\nNoted.\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\tstring_list_init_dup(&e->value_list);\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> I find the function name a bit confusing, as it does not \"set\" in the\n> sense \"override any previous value\". Wouldn't this be better named\n> config_cache_add_value? Or perhaps a comment would help.\n>\n\nNoted.\n\n>> @@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,\n>>  \tlock = NULL;\n>>  \tret = 0;\n>>  \n>> +\t/*\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> \n> Spaces after stars:\n> \n> /*\n>  * Contents of config file has changed, so invalidate the\n>  * config cache used by non-callback based query functions.\n>  */\n> \n> (I think the \"s\" of \"contents\" should be dropped too).\n> \n\nNoted. Thanks for the review.\n"},{"id":"244271","messageId":"vpqlhswzriw.fsf@anie.imag.fr","threadId":"36921","inReplyTo":"539F293D.9000602@gmail.com","subject":"Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-16T17:35:35Z","receivedAt":"2014-06-16T17:35:35Z","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> On 06/16/2014 10:11 AM, Matthieu Moy wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\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>> This describes rather well _what_ your patch does, but the most\n>> important part of a commit message is to justify _why_ the change is\n>> good, and why the way you implemented it is good.\n>> \n>> Think of it as an way to convince reviewers to accept your patch.\n>\n> Okay, but isn't the content of the cover letter is doing that for now.\n\nThe cover letter won't be part of the Git history, while the commit\nmessages are.\n\nImagine someone finding your functions in the code and wondering \"wtf\nis this code doing here?\". \"git blame\" will point this person to your\ncommit message, but digging the mail archives is one big extra step\n(that essentially no one will make).\n\n> Yeah, I have run the experiments. I will add a test file for it. I should have\n> appended it to this series only, my fault. :) A stray observation, Git has very less\n> unit tests, compared to the comprehensive test directory for commands.\n\nYes. But in most cases, code written in a commit is directly reachable\nfrom the command-line UI, and can be tested this way.\n\n(I do believe that Git would benefit from more unit-testing, but that's\nanother topic).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"244362","messageId":"xmqqsin4o406.fsf@gitster.dls.corp.google.com","threadId":"36921","inReplyTo":"1402907232-24629-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T22:59:05Z","receivedAt":"2014-06-16T22:59:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> When a compound construct like a string_list within another\n> struct is used, the default initializer macros are useless.\n> For such cases add helper functions for string_list\n> initialization for both DUP and NODUP modes.\n>\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n\n\nSorry, but I do not understand the above \"useless\".  Do you mean to\nsay that xyzzy below cannot be initialized that way?\n\n git.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/git.c b/git.c\nindex d261575..17714d1 100644\n--- a/git.c\n+++ b/git.c\n@@ -595,11 +595,24 @@ static int run_argv(int *argcp, const char ***argv)\n }\n \n \n+#include \"string-list.h\"\n+\n int main(int argc, char **av)\n {\n \tconst char **argv = (const char **) av;\n \tconst char *cmd;\n \n+\tstruct compound {\n+\t\tint frotz;\n+\t\tstruct string_list nitfol;\n+\t} xyzzy = {\n+\t\t314,\n+\t\tSTRING_LIST_INIT_DUP,\n+\t};\n+\tprintf(\"dup-strings is set to %s\\n\",\n+\t       xyzzy.nitfol.strdup_strings ? \"true\" : \"false\");\n+\treturn 0;\n+\n \tstartup_info = &git_startup_info;\n \n \tcmd = git_extract_argv0_path(argv[0]);\n"},{"id":"244376","messageId":"20140617053436.GB29957@sigill.intra.peff.net","threadId":"36921","inReplyTo":"1402907232-24629-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-17T05:34:36Z","receivedAt":"2014-06-17T05:34:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 16, 2014 at 01:27:12AM -0700, Tanay Abhra wrote:\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\nThis gets the normalized key, which is good; you should be able to just\nhash/lookup values without further munging as long as they, too, are\nnormalized (you may want to note that in the documentaiton for\nconfig_cache_find_entry and friends).\n\nThe value may be a string value, or NULL if it is a true boolean. That\nvalue gets passed to...\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\tstring_list_init_dup(&e->value_list);\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...this function, which then passes it to string_list_append. Which is\ngoing to call xstrdup() on it, which will then segfault.\n\nYou need some mechanism to store entries that are NULL. It may be enough\nto silently convert them into the string \"true\" inside the cached\nstorage. But there may be callers who treat NULL specially (e.g., a\ntri-state true/false/auto that treats a bare boolean as \"auto\"); you'd\nneed to check.\n\nThe other alternative is to use something besides string_list that can\nhandle a NULL.  You may also need to give some thought to how such NULLs\nwould be handled by git_config_get_string() (since a NULL there also\nmeans \"not found\").\n\n-Peff\n"},{"id":"244378","messageId":"20140617054645.GD29957@sigill.intra.peff.net","threadId":"36921","inReplyTo":"20140617053436.GB29957@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-17T05:46:45Z","receivedAt":"2014-06-17T05:46:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 17, 2014 at 01:34:36AM -0400, Jeff King wrote:\n\n> You need some mechanism to store entries that are NULL. It may be enough\n> to silently convert them into the string \"true\" inside the cached\n> storage. But there may be callers who treat NULL specially (e.g., a\n> tri-state true/false/auto that treats a bare boolean as \"auto\"); you'd\n> need to check.\n> \n> The other alternative is to use something besides string_list that can\n> handle a NULL.  You may also need to give some thought to how such NULLs\n> would be handled by git_config_get_string() (since a NULL there also\n> means \"not found\").\n\nAfter reading your other patch and thinking about config_error_nonbool,\nI don't think it would be right to silently convert this to \"true\".\n\nE.g., imagine config like:\n\n  [alias]\n  foo\n\nIf we interpret alias.foo as a boolean, it means \"true\". But since\nalias.* variables are not booleans, the current alias code will produce\nan error (with config_error_nonbool). Silently converting it to true in\nthe config layer would mean the alias code sees only \"true\", and rather\nthan generating an error, it would run the command \"true\".\n\n-Peff\n"},{"id":"244470","messageId":"53A09192.2030008@gmail.com","threadId":"36921","inReplyTo":"xmqqsin4o406.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-17T19:05:54Z","receivedAt":"2014-06-17T19:05:54Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/16/2014 03:59 PM, Junio C Hamano wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> When a compound construct like a string_list within another\n>> struct is used, the default initializer macros are useless.\n>> For such cases add helper functions for string_list\n>> initialization for both DUP and NODUP modes.\n>>\n>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n>> ---\n> \n> \n> Sorry, but I do not understand the above \"useless\".  Do you mean to\n> say that xyzzy below cannot be initialized that way?\n> \n>  git.c | 13 +++++++++++++\n>  1 file changed, 13 insertions(+)\n> \n> diff --git a/git.c b/git.c\n> index d261575..17714d1 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -595,11 +595,24 @@ static int run_argv(int *argcp, const char ***argv)\n>  }\n>  \n>  \n> +#include \"string-list.h\"\n> +\n>  int main(int argc, char **av)\n>  {\n>  \tconst char **argv = (const char **) av;\n>  \tconst char *cmd;\n>  \n> +\tstruct compound {\n> +\t\tint frotz;\n> +\t\tstruct string_list nitfol;\n> +\t} xyzzy = {\n> +\t\t314,\n> +\t\tSTRING_LIST_INIT_DUP,\n> +\t};\n> +\tprintf(\"dup-strings is set to %s\\n\",\n> +\t       xyzzy.nitfol.strdup_strings ? \"true\" : \"false\");\n> +\treturn 0;\n> +\n>  \tstartup_info = &git_startup_info;\n>  \n>  \tcmd = git_extract_argv0_path(argv[0]);\n> \n\nI was actually explaining for cases like below,\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 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\tstring_list_init_dup(&e->value_list);\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\nHere even if we use an initialization list as you have shown above, I would have to\ncheck contents of 'struct hashmap_entry', thus totally breaking the encapsulation\nthat the string_list macro was providing. There may not be default values for\n'struct hashmap_entry' as it may be using internal init function.\n\nAlso, I have to dynamically allocate the config_cache_entry struct, thus the\ninitialization list such as above cannot be used. Two previous reviewers of the\npatch suggested I put a preparatory patch with string_list_init functions because\nthe default macros will be useless in my case.\n\nIs there another way out?\n\nCheers,\nTanay abhra.\n"},{"id":"244492","messageId":"xmqqr42njigd.fsf@gitster.dls.corp.google.com","threadId":"36921","inReplyTo":"53A09192.2030008@gmail.com","subject":"Re: [PATCH v2 1/2] string-list: Add string_list initializer helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-17T22:10:26Z","receivedAt":"2014-06-17T22:10:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> On 06/16/2014 03:59 PM, Junio C Hamano wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> When a compound construct like a string_list within another\n>>> struct is used, the default initializer macros are useless.\n>>> For such cases add helper functions for string_list\n>>> initialization for both DUP and NODUP modes.\n>>>\n>>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n>>> ---\n>> \n>> \n>> Sorry, but I do not understand the above \"useless\".  Do you mean to\n>> say that xyzzy below cannot be initialized that way?\n>>  ...\n> I was actually explaining for cases like below,\n> ...\n> +\t\tstring_list_init_dup(&e->value_list);\n\nIf that is what you wanted to refer to, I would have to say\n\"useless\" is placing a stress on a wrong place.  (I do not see\nanything wrong with your new code; it was just the way it was\nexplained in the proposed log message was misleading).\n\nStructure initialisers are not something you can assign to a\nvariable anyway, and calling them \"useless\" is like complaining how\nunwieldty hammers are on screws.  \"Hammers are useless on screws\"\nmay not be technically wrong per-se, but the readers won't be helped\nby hearing it very much.\n\nInstead, you would want to explain what your new invention, a\nscrewdriver, is and how it is intended to be used.\n\nWe of course have precedences for this kind of thing.  STRBUF_INIT\nis for definition-time initialisation and strbuf_init() is to\ninitialise an uninitialised piece of memory to be used as a strbuf.\n\nI tend to think it was a long-time misdesign of string-list API to\nhave the STRING_LIST_INIT* definition-time initialisers without\nhaving runtime string_list_init*() initialisers, and it is a good\nidea to add them to complete the API.\n\nIf I were writing the log message for this, I would just say:\n\n\tThe string-list API has STRING_LIST_INIT_* macros to be used\n\tto define variables with initialisers, but lacks functions\n\tto initialise an uninitialised piece of memory to be used as\n\ta string-list at the run-time.\n\n        Introduce string_list_init_{dup,nodup}() functions for that.\n\nor something.\n"}]}