{"thread":{"id":"36981","subject":"[PATCH v3 0/3] git config cache & special querying api utilizing the cache","startedAt":"2014-06-23T10:11:37Z","lastAt":"2014-06-27T17:13:28Z","messageCount":35,"participants":["Tanay Abhra","Matthieu Moy","Torsten Bögershausen","Ramsay Jones","Junio C Hamano","Eric Sunshine","Karsten Blees"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"244854","messageId":"1403518300-23053-1-git-send-email-tanayabh@gmail.com","threadId":"36981","inReplyTo":null,"subject":"[PATCH v3 0/3] git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:11:37Z","receivedAt":"2014-06-23T10:11:37Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\n[PATCH v3]: Added flag for NULL values that were causing segfaults in some cases.\n\tAdded test-config for usage examples.\n\tMinor changes and corrections. Refer to discussion thread[5] for more details.\n\tThanks to Matthieu, Jeff and Junio for their valuable suggestions.\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[5] http://thread.gmane.org/gmane.comp.version-control.git/251704/\n\nTanay Abhra (3):\n  string-list: Add string_list initializer helper functions\n  config: Add hashtable for config parsing & retrieval\n  test-config\n\n .gitignore                             |   1 +\n Documentation/technical/api-config.txt |  26 ++++++\n Makefile                               |   1 +\n cache.h                                |   2 +\n config.c                               | 144 +++++++++++++++++++++++++++++++++\n string-list.c                          |  18 +++++\n string-list.h                          |   3 +\n test-config.c                          |  93 +++++++++++++++++++++\n 8 files changed, 288 insertions(+)\n create mode 100644 test-config.c\n\n-- \n1.9.0.GIT\n"},{"id":"244856","messageId":"1403518300-23053-2-git-send-email-tanayabh@gmail.com","threadId":"36981","inReplyTo":"1403518300-23053-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 1/3] string-list: add string_list initialiser helper functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:11:38Z","receivedAt":"2014-06-23T10:11:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"The string-list API has STRING_LIST_INIT_* macros to be used\nto define variables with initialisers, but lacks functions\nto initialise an uninitialised piece of memory to be used as\na string-list at the run-time.\nIntroduce string_list_init_{dup,nodup}() functions for that.\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 dd5e294..c90d6c1 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, NULL }\n #define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }\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":"244855","messageId":"1403518300-23053-3-git-send-email-tanayabh@gmail.com","threadId":"36981","inReplyTo":"1403518300-23053-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:11:39Z","receivedAt":"2014-06-23T10:11:39Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Currently `git_config()` uses a callback mechanism and file rereads for\nconfig values. Due to this approach, it is not uncommon for the config\nfiles to be parsed several times during the run of a git program, with\ndifferent callbacks picking out different variables useful to themselves.\n\nAdd a 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. They support O(1) lookups once the hash table is constructed.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n Documentation/technical/api-config.txt |  26 ++++++\n cache.h                                |   2 +\n config.c                               | 144 +++++++++++++++++++++++++++++++++\n 3 files changed, 172 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..13f3f40 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,32 @@ 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 read values from an internal\n+cache generated previously from reading the config files.\n+\n+`git_config_get_string` takes two parameters,\n+\n+- a key string in canonical flat form for which the corresponding value\n+  with the highest priority (i.e. value in the repo config will be\n+  preferred over value in user wide config for the same variable) will\n+  be retrieved.\n+\n+- a pointer to a string which will point to the retrieved value.\n+\n+`git_config_get_string` returns 0 for success, or -1 for no value found.\n+\n+`git_config_get_string_multi` returns a `string_list` containing all the\n+values for the key passed as parameter, sorted in order of increasing\n+priority (Note: NULL values are flagged as 1, check `util` for each\n+'string_list_item` for flag value).\n+\n+See test-config.c for usage examples.\n+\n Value Parsing Helpers\n ---------------------\n \ndiff --git a/cache.h b/cache.h\nindex cbe1935..fec0a63 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1294,6 +1294,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 int git_config_get_string(const char *, 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 a1aef1c..6200f36 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,141 @@ 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_add_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_add_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, 1);\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 = 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_add_value(const char *key, const char *value)\n+{\n+\tstruct hashmap *config_cache;\n+\tstruct config_cache_entry *e;\n+\tstruct string_list_item *item;\n+\tint *boolean_null_flag;\n+\n+\tconfig_cache = get_config_cache();\n+\te = config_cache_find_entry(key);\n+\n+\tboolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));\n+\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\thashmap_add(config_cache, e);\n+\t}\n+\t/*\n+\t * If the variable had no value specified, the value will be NULL\n+\t * (typically this means it should be interpreted as boolean true).\n+\t * For such values, silently convert them to \"BOOLEAN_NULL\" to\n+\t * store in hashmap and flag that string_list_item as 1.\n+\t */\n+\tif (value == NULL) {\n+\t\tvalue = \"BOOLEAN_NULL\";\n+\t\t*boolean_null_flag = 1;\n+\t}\n+\titem = string_list_append(&e->value_list, value);\n+\titem->util = boolean_null_flag;\n+\n+\treturn 0;\n+}\n+\n+int git_config_get_string(const char *key, const char **value)\n+{\n+\tstruct string_list *values;\n+\tint *flag;\n+\tvalues = config_cache_get_value(key);\n+\tif (!values)\n+\t\treturn -1;\n+\tassert(values->nr > 0);\n+\t*value = values->items[values->nr - 1].string;\n+\tflag = values->items[values->nr - 1].util;\n+\tif (*flag)\n+\t\t*value = NULL;\n+\treturn 0;\n+}\n+\n+/* for NULL values, 'util' for each `string_list_item` is flagged as 1 */\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@@ -1708,6 +1845,13 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tlock = NULL;\n \tret = 0;\n \n+\t/*\n+\t * content 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":"244857","messageId":"1403518300-23053-4-git-send-email-tanayabh@gmail.com","threadId":"36981","inReplyTo":"1403518300-23053-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v3 3/3] test-config: add usage examples for non-callback query functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T10:11:40Z","receivedAt":"2014-06-23T10:11:40Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Add different usage examples for 'git_config_get_string' and\n`git_config_get_string_multi`. They will serve as documentation\non how to query for config values in a non-callback manner.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n .gitignore    |  1 +\n Makefile      |  1 +\n test-config.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 95 insertions(+)\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 42294e5..7677533 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -177,6 +177,7 @@\n /gitweb/static/gitweb.min.*\n /test-chmtime\n /test-ctype\n+/test-config\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/Makefile b/Makefile\nindex 07ea105..9544efb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..ff24cb8\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,93 @@\n+#include \"cache.h\"\n+#include \"hashmap.h\"\n+#include \"string-list.h\"\n+\n+/*\n+ * This program gives examples on how to use non-callback based query\n+ * functions like git_config_get_string & git_config_get_string_multi.\n+ *\n+ * Reads stdin and prints result of command to stdout:\n+ *\n+ * print_all -> prints all the key-value pairs contained in the hashmap\n+ *\t\talso checks if all entries in the hashmap matches with\n+ *\t\tthe content of config files\n+ *\n+ * get_value -> prints the value with highest priority for the entered key\n+ *\n+ * get_all_values -> prints all values for the given key in increasing order\n+ *\t\t     of priority\n+ * Examples:\n+ *\n+ * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n+ * \ttest-config get_value \"foo.bAr Baz.rock\"\n+ *\n+ */\n+\n+static const char *v;\n+static const struct string_list *strptr;\n+static int i;\n+static int *flag;\n+\n+static int config_cache_callback(const char *key, const char *value, void *unused)\n+{\n+\tstrptr = git_config_get_string_multi(key);\n+\tif (strptr) {\n+\t\tfor (i = 0; i < strptr->nr; i++)\n+\t\t{\n+\t\t\tv = strptr->items[i].string;\n+\t\t\tflag = strptr->items[i].util;\n+\t\t\t/* NULL values are flagged as 1 */\n+\t\t\tif (*flag == 1)\n+\t\t\t\tprintf(\"%s\\n\", key);\n+\t\t\telse if (*flag == 0)\n+\t\t\t\tprintf(\"%s=%s\\n\", key, v);\n+\t\t\t/* key-value pair printed so flag them as done */\n+\t\t\t*flag = -1;\n+\t\t}\n+\t\treturn 0;\n+\t} else {\n+\t\tprintf(\"%s\\n\", \"Config hashmap inconsistent\\n\");\n+\t\treturn -1;\n+\t}\n+}\n+\n+ int main(int argc, char **argv)\n+{\n+\tif (argc == 2 && !strcmp(argv[1], \"print_all\")) {\n+\t\tgit_config(config_cache_callback, NULL);\n+\t\treturn 0;\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n+\t\t/* enter key in canonical form enclosed in quotes */\n+\t\tif (!git_config_get_string(argv[2], &v)) {\n+\t\t\tprintf(\"%s\\n\", v);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"%s\\n\", \"Value not found for the entered key\\n\");\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_all_values\")) {\n+\t\t/* enter key in canonical form enclosed in quotes */\n+\t\tstrptr = git_config_get_string_multi(argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++)\n+\t\t\t{\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tflag = strptr->items[i].util;\n+\t\t\t\t/* prints NULL values as \"-\" */\n+\t\t\t\tif (*flag)\n+\t\t\t\t\tprintf(\"%s \", \"-\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s \", v);\n+\t\t\t}\n+\t\t\tprintf(\"\\n\");\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"%s\\n\", \"Value not found for the entered key\\n\");\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n+\tfprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n+\t\targv[1] ? argv[1] : \"(there was none)\");\n+\treturn 1;\n+}\n-- \n1.9.0.GIT\n"},{"id":"244867","messageId":"vpqtx7bn8ln.fsf@anie.imag.fr","threadId":"36981","inReplyTo":"1403518300-23053-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-23T11:55:32Z","receivedAt":"2014-06-23T11:55: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> +/* for NULL values, 'util' for each `string_list_item` is flagged as 1 */\n\nIt's a void *, so just saying that it is flagged as 1 does not say how\nit's encoded. How about \"... is an 'int *' pointing to the value 1\".\n\nAnd actually, you can save one malloc by encoding the value directly in\nthe util pointer itself like\n\n#define NULL_FLAG (void *)1\n\n\titem->util = NULL_FLAG;\n\nI find this a bit ugly, but I think Git already uses this in a few\nplaces (git grep 'void \\*) *1' for examples).\n\nOr you can use a pointer to a single static value:\n\nstatic const int boolean_null_flag = 1; // Or even without = 1.\n\nand then\n\n\titem->util = &boolean_null_flag;\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"244869","messageId":"53A81F45.70802@web.de","threadId":"36981","inReplyTo":"1403518300-23053-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 1/3] string-list: add string_list initialiser helper functions","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-06-23T12:36:21Z","receivedAt":"2014-06-23T12:36:21Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-06-23 12.11, Tanay Abhra wrote:\n> The string-list API has STRING_LIST_INIT_* macros to be used\n> to define variables with initialisers, but lacks functions\n> to initialise an uninitialised piece of memory to be used as\n> a string-list at the run-time.\n> Introduce string_list_init_{dup,nodup}() functions for that.\n> \n> Signed-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> \n> diff --git a/string-list.c b/string-list.c\n> index 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> +\nIf we look at the definition below:\nstruct string_list {\n\tstruct string_list_item *items;\n\tunsigned int nr, alloc;\n\tunsigned int strdup_strings:1;\n\tcompare_strings_fn cmp; /* NULL uses strcmp() */\nI think a simple memset() will be easier to read,\nand it will be more future proof:\nIn case elements are added, the will have 0 or NULL automatically:\n\nvoid string_list_init_nodup(struct string_list *list)\n{\n\tmemset (list, 0, sizeof(*list));\n}\n(But then I wonder if we need the function at all ?)\n\nOr does it make sense to have a common function similar to this,\nwhich covers both cases:\n\nvoid string_list_init(struct string_list *list, int strdup_strings)\n{\n\tmemset (list, 0, sizeof(*list));\n\tlist->strdup_strings = strdup_strings;\n}\n"},{"id":"244870","messageId":"53A82978.2010900@gmail.com","threadId":"36981","inReplyTo":"53A81F45.70802@web.de","subject":"Re: [PATCH v3 1/3] string-list: add string_list initialiser helper functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T13:19:52Z","receivedAt":"2014-06-23T13:19:52Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/23/2014 05:36 AM, Torsten Bögershausen wrote:\n> If we look at the definition below:\n> struct string_list {\n> \tstruct string_list_item *items;\n> \tunsigned int nr, alloc;\n> \tunsigned int strdup_strings:1;\n> \tcompare_strings_fn cmp; /* NULL uses strcmp() */\n> I think a simple memset() will be easier to read,\n> and it will be more future proof:\n> In case elements are added, the will have 0 or NULL automatically:\n\nYes, you are right. After sending the patch I saw that for string_list\ninitialization the codebase either uses xcalloc or memset and after that\nmarks the list as DUP or NODUP.\n\n> void string_list_init_nodup(struct string_list *list)\n> {\n> \tmemset (list, 0, sizeof(*list));\n> }\n> (But then I wonder if we need the function at all ?)\n> \n> Or does it make sense to have a common function similar to this,\n> which covers both cases:\n> \n> void string_list_init(struct string_list *list, int strdup_strings)\n> {\n> \tmemset (list, 0, sizeof(*list));\n> \tlist->strdup_strings = strdup_strings;\n> }\n> \n\nA common function would be much better as other API constructs as strbuf\nhave runtime init functions like the version you have shown above.\n\nThanks for the review.\n"},{"id":"244872","messageId":"53A84077.4010200@ramsay1.demon.co.uk","threadId":"36981","inReplyTo":"1403518300-23053-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-06-23T14:57:59Z","receivedAt":"2014-06-23T14:57:59Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 23/06/14 11:11, Tanay Abhra wrote:\n\n[snip]\n\n> diff --git a/config.c b/config.c\n> index a1aef1c..6200f36 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,141 @@ 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_add_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_add_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[I have not been following this series at all (sorry I haven't had\nthe time to spare), so take these comments with a very big pinch of\nsalt! ie just ignore me if it's already been discussed etc. ;-) ]\n\nThe 'git config' command can be used to read arbitrary files (so long\nas they conform to the config syntax). For example, see the --file and\n--blob options to git-config. At present, I think only scripted commands\nuse this facility (eg git-submodule). Noting the singleton config_cache,\nwhat happens when git-submodule becomes a C builtin, or indeed any other\nC builtin wants to take advantage of the new code when processing a non-\nstandard config file?\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, 1);\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 = 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_add_value(const char *key, const char *value)\n> +{\n> +\tstruct hashmap *config_cache;\n> +\tstruct config_cache_entry *e;\n> +\tstruct string_list_item *item;\n> +\tint *boolean_null_flag;\n> +\n> +\tconfig_cache = get_config_cache();\n> +\te = config_cache_find_entry(key);\n> +\n> +\tboolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));\n> +\n> +\tif (!e) {\n> +\t\te = xmalloc(sizeof(*e));\n> +\t\thashmap_entry_init(e, strhash(key));\n> +\t\te->key = xstrdup(key);\n\nconfig_cache_find_entry() searches for (and hashes the) normalized_key.\nShould you not be entering the normalized key here?\n\n> +\t\tstring_list_init_dup(&e->value_list);\n> +\t\thashmap_add(config_cache, e);\n> +\t}\n> +\t/*\n> +\t * If the variable had no value specified, the value will be NULL\n> +\t * (typically this means it should be interpreted as boolean true).\n> +\t * For such values, silently convert them to \"BOOLEAN_NULL\" to\n> +\t * store in hashmap and flag that string_list_item as 1.\n> +\t */\n> +\tif (value == NULL) {\n> +\t\tvalue = \"BOOLEAN_NULL\";\n> +\t\t*boolean_null_flag = 1;\n> +\t}\n> +\titem = string_list_append(&e->value_list, value);\n> +\titem->util = boolean_null_flag;\n> +\n> +\treturn 0;\n> +}\n> +\n\n[snip]\n\nATB,\nRamsay Jones\n"},{"id":"244874","messageId":"53A853E9.8060801@gmail.com","threadId":"36981","inReplyTo":"53A84077.4010200@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-23T16:20:57Z","receivedAt":"2014-06-23T16:20:57Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 06/23/2014 07:57 AM, Ramsay Jones wrote:\n> On 23/06/14 11:11, Tanay Abhra wrote:\n>> diff --git a/config.c b/config.c\n>> index a1aef1c..6200f36 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,141 @@ 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_add_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_add_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> [I have not been following this series at all (sorry I haven't had\n> the time to spare), so take these comments with a very big pinch of\n> salt! ie just ignore me if it's already been discussed etc. ;-) ]\n> \n> The 'git config' command can be used to read arbitrary files (so long\n> as they conform to the config syntax). For example, see the --file and\n> --blob options to git-config. At present, I think only scripted commands\n> use this facility (eg git-submodule). Noting the singleton config_cache,\n> what happens when git-submodule becomes a C builtin, or indeed any other\n> C builtin wants to take advantage of the new code when processing a non-\n> standard config file?\n> \n\nThis series was mainly to replace git_config() invocations around the codebase.\nThere are currently 111 git_config() invocations, each of which causes a file\nreread whenever called. git_config() only feeds values from the standard config\nfiles(i.e repo, user and global config).\n\nFor reading config values from specific files or blobs, there are three functions\ngit_config_with_options, git_config_from_file & git_config_from_blob which can be\neasily used inside a C builtin or anywhere in the code.\n\nThe bulk of git_config_api calls are only for git_config(). For example,\ngit_config_from_file() has three hits only in entire codebase,\ngit_config_with_options() has 5 hits, so I concentrated on generating a cache\nfor the usual config files only. For other files, the callers can fall back on older\nAPI functions like I had mentioned above.\n\nForgive me if I inferred your question incorrectly. More below.\n\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, 1);\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 = 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_add_value(const char *key, const char *value)\n>> +{\n>> +\tstruct hashmap *config_cache;\n>> +\tstruct config_cache_entry *e;\n>> +\tstruct string_list_item *item;\n>> +\tint *boolean_null_flag;\n>> +\n>> +\tconfig_cache = get_config_cache();\n>> +\te = config_cache_find_entry(key);\n>> +\n>> +\tboolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));\n>> +\n>> +\tif (!e) {\n>> +\t\te = xmalloc(sizeof(*e));\n>> +\t\thashmap_entry_init(e, strhash(key));\n>> +\t\te->key = xstrdup(key);\n> \n> config_cache_find_entry() searches for (and hashes the) normalized_key.\n> Should you not be entering the normalized key here?\n> \n\nconfig_cache_add_value() is fed key-values pairs through the git_config()\ncallback mechanism, which normalises the key beforehand, so no need for\nrenormalising.\n\nThanks for the review. :)\n\nCheers,\nTanay Abhra.\n"},{"id":"244902","messageId":"xmqqwqc79q1p.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"1403518300-23053-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T23:14:42Z","receivedAt":"2014-06-23T23:14:42Z","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> +static int hashmap_initialized;\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\nI find the arrangement of these two variables somewhat dubious at\nthe API design level.\n\nIf you are going to keep the singleton \"config_cache\" as a function\nscope static, shouldn't the corresponding guard also be in the same\nscope?\n\nIf you ever need to \"uninitialize\" to force re-read the file to the\nin-core cache, such an uninitializer will need access to not just\nthe \"is hashmap initialized?\" boolean (which you do by having it as\na file-scope global like this patch does) but also the thing that\nmay need to be uninitialized (i.e. the hashmap that may already be\npopulated), but a function scope static variable config_cache does\nnot allow access from other places, so you end up calling this\nfunction to initialize it if necessary only to get the pointer to\nthat structure in order to uninitialize it.\n\nSounds very twisted and ugly, doesn't it?\n"},{"id":"244904","messageId":"xmqqsimv9pjx.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"53A84077.4010200@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-23T23:25:22Z","receivedAt":"2014-06-23T23:25:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\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> [I have not been following this series at all (sorry I haven't had\n> the time to spare), so take these comments with a very big pinch of\n> salt! ie just ignore me if it's already been discussed etc. ;-) ]\n>\n> The 'git config' command can be used to read arbitrary files (so long\n> as they conform to the config syntax). For example, see the --file and\n> --blob options to git-config. At present, I think only scripted commands\n> use this facility (eg git-submodule). Noting the singleton config_cache,\n> what happens when git-submodule becomes a C builtin, or indeed any other\n> C builtin wants to take advantage of the new code when processing a non-\n> standard config file?\n\nYup, that is a very good point.  There needs an infrastructure to\ntie a set of files (i.e. the standard one being the chain of\nsystem-global /etc/gitconfig to repo-specific .git/config, and any\ncustom one that can be specified by the caller like submodule code)\nto a separate hashmap; a future built-in submodule code would use\ntwo hashmaps aka \"config-caches\", one to manage the usual\n\"configuration\" and the other to manage the contents of the\n.gitmodules file.\n\nWhen we are operating across repositories (think \"recursive\ncheckout\"), we somehow have to deal with multiple sets of\nconfiguration, one that applies to the top-level superproject and\nothers that apply to individual submodule repositories.\n\nEven there is currently one caller, the \"git config\" command\nimplementation, if one is designing the new API, the design\nshouldn't be tied to a singleton limitation, I would have to say, so\nthat future callers do not have to throw away everything done in\nthis series and start from scratch.\n"},{"id":"244911","messageId":"53A9276D.3020503@gmail.com","threadId":"36981","inReplyTo":"xmqqsimv9pjx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-24T07:23:25Z","receivedAt":"2014-06-24T07:23:25Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 6/24/2014 4:55 AM, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\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>> [I have not been following this series at all (sorry I haven't had\n>> the time to spare), so take these comments with a very big pinch of\n>> salt! ie just ignore me if it's already been discussed etc. ;-) ]\n>>\n>> The 'git config' command can be used to read arbitrary files (so long\n>> as they conform to the config syntax). For example, see the --file and\n>> --blob options to git-config. At present, I think only scripted commands\n>> use this facility (eg git-submodule). Noting the singleton config_cache,\n>> what happens when git-submodule becomes a C builtin, or indeed any other\n>> C builtin wants to take advantage of the new code when processing a non-\n>> standard config file?\n>\n> Yup, that is a very good point.  There needs an infrastructure to\n> tie a set of files (i.e. the standard one being the chain of\n> system-global /etc/gitconfig to repo-specific .git/config, and any\n> custom one that can be specified by the caller like submodule code)\n> to a separate hashmap; a future built-in submodule code would use\n> two hashmaps aka \"config-caches\", one to manage the usual\n> \"configuration\" and the other to manage the contents of the\n> .gitmodules file.\n>\n> When we are operating across repositories (think \"recursive\n> checkout\"), we somehow have to deal with multiple sets of\n> configuration, one that applies to the top-level superproject and\n> others that apply to individual submodule repositories.\n>\n> Even there is currently one caller, the \"git config\" command\n> implementation, if one is designing the new API, the design\n> shouldn't be tied to a singleton limitation, I would have to say, so\n> that future callers do not have to throw away everything done in\n> this series and start from scratch.\n>\n\nWhat changes should I implement in this series? Should I add new\nuser facing API adding to the singleton callers which are already in\nthis series.\n\n\"submodule lookup API\" was intoduced by Heiko Voigt in [1], which is in\n\"cooking\" now. His series already allows looking up local configuration\nby passing in the null_sha1 for gitmodule or commit sha1. What else can\nI add to my implementation, please suggest.\n\nThanks,\nTanay Abhra\n"},{"id":"244912","messageId":"53A927EA.4020106@gmail.com","threadId":"36981","inReplyTo":"xmqqsimv9pjx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-24T07:25:30Z","receivedAt":"2014-06-24T07:25:30Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 6/24/2014 4:55 AM, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\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>> [I have not been following this series at all (sorry I haven't had\n>> the time to spare), so take these comments with a very big pinch of\n>> salt! ie just ignore me if it's already been discussed etc. ;-) ]\n>>\n>> The 'git config' command can be used to read arbitrary files (so long\n>> as they conform to the config syntax). For example, see the --file and\n>> --blob options to git-config. At present, I think only scripted commands\n>> use this facility (eg git-submodule). Noting the singleton config_cache,\n>> what happens when git-submodule becomes a C builtin, or indeed any other\n>> C builtin wants to take advantage of the new code when processing a non-\n>> standard config file?\n>\n> Yup, that is a very good point.  There needs an infrastructure to\n> tie a set of files (i.e. the standard one being the chain of\n> system-global /etc/gitconfig to repo-specific .git/config, and any\n> custom one that can be specified by the caller like submodule code)\n> to a separate hashmap; a future built-in submodule code would use\n> two hashmaps aka \"config-caches\", one to manage the usual\n> \"configuration\" and the other to manage the contents of the\n> .gitmodules file.\n>\n> When we are operating across repositories (think \"recursive\n> checkout\"), we somehow have to deal with multiple sets of\n> configuration, one that applies to the top-level superproject and\n> others that apply to individual submodule repositories.\n>\n> Even there is currently one caller, the \"git config\" command\n> implementation, if one is designing the new API, the design\n> shouldn't be tied to a singleton limitation, I would have to say, so\n> that future callers do not have to throw away everything done in\n> this series and start from scratch.\n>\nSorry for the repost, missed the link.\n\nWhat changes should I implement in this series? Should I add new\nuser facing API adding to the singleton callers which are already in\nthis series.\n\n\"submodule lookup API\" was intoduced by Heiko Voigt in [1], which is in\n\"cooking\" now. His series already allows looking up local configuration\nby passing in the null_sha1 for gitmodule or commit sha1. What else can\nI add to my implementation, please suggest.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/250811\n\nThanks,\nTanay Abhra\n"},{"id":"244927","messageId":"53A969DD.2030201@gmail.com","threadId":"36981","inReplyTo":"vpqtx7bn8ln.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-24T12:06:53Z","receivedAt":"2014-06-24T12:06:53Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 6/23/2014 5:25 PM, Matthieu Moy wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>\n>> +/* for NULL values, 'util' for each `string_list_item` is flagged as 1 */\n>\n> It's a void *, so just saying that it is flagged as 1 does not say how\n> it's encoded. How about \"... is an 'int *' pointing to the value 1\".\n>\n> And actually, you can save one malloc by encoding the value directly in\n> the util pointer itself like\n>\n> #define NULL_FLAG (void *)1\n>\n> \titem->util = NULL_FLAG;\n>\n> I find this a bit ugly, but I think Git already uses this in a few\n> places (git grep 'void \\*) *1' for examples).\n>\n> Or you can use a pointer to a single static value:\n>\n> static const int boolean_null_flag = 1; // Or even without = 1.\n>\n> and then\n>\n> \titem->util = &boolean_null_flag;\n>\n\nThanks for the review. I will change the flag format to what you have\nsuggested. Instead of giving the users of the API the headache of\nthinking about the flag, let me give you an alternative,\n\n+const struct string_list *git_config_get_string_multi(const char *key)\n+{\n+\tint i, *flag;\n+\tstruct string_list *temp = config_cache_get_value(key);\n+\tif (!temp)\n+\t\treturn NULL;\n+\t/* create a NODUP string list which can have NULL values */\n+\tstruct string_list *l = xcalloc(1, sizeof(*l));\n+\tfor(i=0; i < temp->nr; i++) {\n+\t\tflag = temp->items[i].util;\n+\t\tif (*flag)\n+\t\t\tstring_list_append(l, NULL);\n+\t\telse\n+\t\t\tstring_list_append(l, temp->items[i].string);\n+\t}\n+\treturn l;\n+}\n\nNow the only headache for the user will be to free the string-list once\nhe is done with it. What do you think about this approach?\n\nAlso I have a doubt, when rewriting git_config() callers I saw a\ncurious case, when at the end of the callback they sometimes call\ngit_default_config().\n\nFor example in add.c,\n\nstatic int add_config(const char *var, const char *value, void *cb)\n{\n\tif (!strcmp(var, \"add.ignoreerrors\") ||\n\t    !strcmp(var, \"add.ignore-errors\")) {\n\t\tignore_add_errors = git_config_bool(var, value);\n\t\treturn 0;\n\t}\n\treturn git_default_config(var, value, cb);\n}\n\nWhat should I do about this, should I change the default_config() calls\nto git_config_get_string calls which I can easily do, if the values it \nis setting are not being set by any other callers.\n\nFor example in config.c,\n\nstatic int git_default_core_config(const char *var, const char *value)\n{\n\t/* This needs a better name */\n\tif (!strcmp(var, \"core.filemode\")) {\n\t\ttrust_executable_bit = git_config_bool(var, value);\n\t\treturn 0;\n\t}\n\tif (!strcmp(var, \"core.trustctime\")) {\n\t\ttrust_ctime = git_config_bool(var, value);\n\t\treturn 0;\n\t}\n\tif (!strcmp(var, \"core.statinfo\") ||\n\t    !strcmp(var, \"core.checkstat\")) {\n--[snip]\nvariables like \"core.filemode\" or \"core.statinfo\" have been requested \nhere only in the whole codebase, so I am safe to rewrite this function\nwith git_config_get_string, am I?\n\nIf not many callers (add_config...) behave like this. What should I\ndo with them?\n\nAlso, what are your views for the points Ramsay has raised.\n\nCheers,\nTanay Abhra.\n"},{"id":"244928","messageId":"53A96D2D.1020102@gmail.com","threadId":"36981","inReplyTo":"xmqqwqc79q1p.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-24T12:21:01Z","receivedAt":"2014-06-24T12:21:01Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"On 6/24/2014 4:44 AM, Junio C Hamano wrote:\n> Tanay Abhra <tanayabh@gmail.com> writes:\n>\n>> +static int hashmap_initialized;\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>\n> I find the arrangement of these two variables somewhat dubious at\n> the API design level.\n>\n> If you are going to keep the singleton \"config_cache\" as a function\n> scope static, shouldn't the corresponding guard also be in the same\n> scope?\n>\n> If you ever need to \"uninitialize\" to force re-read the file to the\n> in-core cache, such an uninitializer will need access to not just\n> the \"is hashmap initialized?\" boolean (which you do by having it as\n> a file-scope global like this patch does) but also the thing that\n> may need to be uninitialized (i.e. the hashmap that may already be\n> populated), but a function scope static variable config_cache does\n> not allow access from other places, so you end up calling this\n> function to initialize it if necessary only to get the pointer to\n> that structure in order to uninitialize it.\n>\n> Sounds very twisted and ugly, doesn't it?\n>\n\nHmn, You are right. but I couldn't find a way for that without making\n\"config_cache\" file-scope static.\n\nFor the config_cache_free(), would this change be enough?\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+\tif (!hashmap_initialized)\n+\t\treturn;\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, 1);\n+\t}\n+\thashmap_free(config_cache, 1);\n+\thashmap_initialized = 0;\n+}\n+\n\nThe idea was only initialize the hashmap only once,\nas it is fed by git_config(). I ended up following\nthis approach where the map would be lazily generated whenever\na function asked for the map. Is there another way out?\n"},{"id":"244934","messageId":"53A99A28.1090302@ramsay1.demon.co.uk","threadId":"36981","inReplyTo":"53A853E9.8060801@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-06-24T15:32:56Z","receivedAt":"2014-06-24T15:32:56Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 23/06/14 17:20, Tanay Abhra wrote:\n> On 06/23/2014 07:57 AM, Ramsay Jones wrote:\n>> On 23/06/14 11:11, Tanay Abhra wrote:\n[snip]\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>> [I have not been following this series at all (sorry I haven't had\n>> the time to spare), so take these comments with a very big pinch of\n>> salt! ie just ignore me if it's already been discussed etc. ;-) ]\n>>\n>> The 'git config' command can be used to read arbitrary files (so long\n>> as they conform to the config syntax). For example, see the --file and\n>> --blob options to git-config. At present, I think only scripted commands\n>> use this facility (eg git-submodule). Noting the singleton config_cache,\n>> what happens when git-submodule becomes a C builtin, or indeed any other\n>> C builtin wants to take advantage of the new code when processing a non-\n>> standard config file?\n>>\n> \n> This series was mainly to replace git_config() invocations around the codebase.\n> There are currently 111 git_config() invocations, each of which causes a file\n> reread whenever called. git_config() only feeds values from the standard config\n> files(i.e repo, user and global config).\n> \n> For reading config values from specific files or blobs, there are three functions\n> git_config_with_options, git_config_from_file & git_config_from_blob which can be\n> easily used inside a C builtin or anywhere in the code.\n> \n> The bulk of git_config_api calls are only for git_config(). For example,\n> git_config_from_file() has three hits only in entire codebase,\n> git_config_with_options() has 5 hits, so I concentrated on generating a cache\n> for the usual config files only. For other files, the callers can fall back on older\n> API functions like I had mentioned above.\n> \n> Forgive me if I inferred your question incorrectly. More below.\n\nHmm, maybe. The \"... take advantage of the new code\" refers to the\npossibility (or otherwise) of re-using your work to update these\n\"older API\" functions to the new API style. (also, see Junio's response).\n[In order to do this, I would have expected to see one hash table\nfor each file/blob, so the singleton object took me by surprise.]\n\nAn \"out of scope for this project\" is a perfectly acceptable\nresponse (*particularly* since it is very late in the day to be\nbringing this up!).\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 = 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_add_value(const char *key, const char *value)\n>>> +{\n>>> +\tstruct hashmap *config_cache;\n>>> +\tstruct config_cache_entry *e;\n>>> +\tstruct string_list_item *item;\n>>> +\tint *boolean_null_flag;\n>>> +\n>>> +\tconfig_cache = get_config_cache();\n>>> +\te = config_cache_find_entry(key);\n>>> +\n>>> +\tboolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));\n>>> +\n>>> +\tif (!e) {\n>>> +\t\te = xmalloc(sizeof(*e));\n>>> +\t\thashmap_entry_init(e, strhash(key));\n>>> +\t\te->key = xstrdup(key);\n>>\n>> config_cache_find_entry() searches for (and hashes the) normalized_key.\n>> Should you not be entering the normalized key here?\n>>\n> \n> config_cache_add_value() is fed key-values pairs through the git_config()\n> callback mechanism, which normalises the key beforehand, so no need for\n> renormalising.\n\nAh, yes, I forgot that the parsing code does a tolower() at various\nplaces while accumulating the key string. So the (potentially) non-\nnormalized keys come from the user via the new API functions and,\nrather than putting code to normalize the key in each of those,\njust do it once in config_cache_find_entry(). (Although, you could\npossibly do that in config_cache_get_value()).  OK.\n\nHmm, maybe add a short comment to that effect? dunno.\n\nATB,\nRamsay Jones\n"},{"id":"244935","messageId":"53A99FEB.5040808@ramsay1.demon.co.uk","threadId":"36981","inReplyTo":"xmqqsimv9pjx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-06-24T15:57:31Z","receivedAt":"2014-06-24T15:57:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 24/06/14 00:25, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\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>> [I have not been following this series at all (sorry I haven't had\n>> the time to spare), so take these comments with a very big pinch of\n>> salt! ie just ignore me if it's already been discussed etc. ;-) ]\n>>\n>> The 'git config' command can be used to read arbitrary files (so long\n>> as they conform to the config syntax). For example, see the --file and\n>> --blob options to git-config. At present, I think only scripted commands\n>> use this facility (eg git-submodule). Noting the singleton config_cache,\n>> what happens when git-submodule becomes a C builtin, or indeed any other\n>> C builtin wants to take advantage of the new code when processing a non-\n>> standard config file?\n> \n> Yup, that is a very good point.  There needs an infrastructure to\n> tie a set of files (i.e. the standard one being the chain of\n> system-global /etc/gitconfig to repo-specific .git/config, and any\n> custom one that can be specified by the caller like submodule code)\n> to a separate hashmap; a future built-in submodule code would use\n> two hashmaps aka \"config-caches\", one to manage the usual\n> \"configuration\" and the other to manage the contents of the\n> .gitmodules file.\n> \n\nI had expected to see one hash table per file/blob, with the three\nstandard config hash tables linked together to implement the scope/\npriority rules. (Well, these could be merged into one, as the current\ncode does, since that makes handling \"multi\" keys slightly easier).\n\nie when looking up a symbol in the .git/config, if not found then\nfollow the link to parent scope (~/.gitconfig) and search there ...\n\nOf course, other uses (I think) represent only a single scope, so they\nwould not have any parent scopes to link to.\n\n> When we are operating across repositories (think \"recursive\n> checkout\"), we somehow have to deal with multiple sets of\n> configuration, one that applies to the top-level superproject and\n> others that apply to individual submodule repositories.\n> \n\nHere, you could push the new .git/config onto a stack while linking\nto the parent (~/.gitconfig) scope. So, the lookup code starts from\nthe stack top, etc, ...\n\n> Even there is currently one caller, the \"git config\" command\n> implementation, if one is designing the new API, the design\n> shouldn't be tied to a singleton limitation, I would have to say, so\n> that future callers do not have to throw away everything done in\n> this series and start from scratch.\n> .\n\nI would like to stress that, despite the above, I have not given\nany great thought to this project. So please take my comments with\na big pinch of salt. I was just scrolling through the patch in my\nemail client and was surprised to see the singleton ...\n\nATB,\nRamsay Jones\n"},{"id":"244964","messageId":"CAPig+cTf+8H38SSxoLMSuU9JUSPiEuTCm=8wHFe5QSCK3j8ELw@mail.gmail.com","threadId":"36981","inReplyTo":"1403518300-23053-4-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 3/3] test-config: add usage examples for non-callback query functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-06-25T11:19:03Z","receivedAt":"2014-06-25T11:19:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jun 23, 2014 at 6:11 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n> Add different usage examples for 'git_config_get_string' and\n> `git_config_get_string_multi`. They will serve as documentation\n> on how to query for config values in a non-callback manner.\n\nThis is a good start, but it's not fully what Matthieu was suggesting\nwhen he said that you should prove to other developers, by way of\nreproducible tests, that your changes work. What he meant, was that\nyou should write a test-config program which exposes (as a runnable\ncommand) the new config C API you've added, and then write tests which\nexercise that API and implementation exhaustively.\n\nFor example, take a look at test-string-list.c and\nt/t0063-string-list.sh. The C program does no checking itself. It\nmerely exposes the C API via command-line arguments, such as \"split\",\n\"filter\", etc. The test script then employs that program to perform\nthe actual testing in a reproducible and (hopefully) exhaustive\nfashion. Because t/t0063-string-list.sh is part of the test suite, the\nstring-list tests are run regularly by many developers. It's not just\nsomething that someone might remember to run once in a while.\n\nContrary to your commit message and the comment in the program itself,\nthe purpose of test-config is not to serve as documentation or to\nprovide examples of usage. (Real documentation is better suited for\nthose purposes.) Instead, test-config should exist in support of a\nreal test script in t/ which is run regularly. The new script you add\nto t/ should exercise the exposed C API as exhaustively as possible.\nThis means checking each possible state: for instance, (1) when a key\nis absent, (2) when a value is boolean (NULL), (3) one non-boolean\n(non-NULL) value, (4) multiple values, etc. Moreover, it should check\nexpected success _and_ expected failure modes. Check not only that it\nreturns expected values, but that it fails when appropriate.\n\nMore below.\n\n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n>  .gitignore    |  1 +\n>  Makefile      |  1 +\n>  test-config.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  3 files changed, 95 insertions(+)\n>  create mode 100644 test-config.c\n>\n> diff --git a/.gitignore b/.gitignore\n> index 42294e5..7677533 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -177,6 +177,7 @@\n>  /gitweb/static/gitweb.min.*\n>  /test-chmtime\n>  /test-ctype\n> +/test-config\n>  /test-date\n>  /test-delta\n>  /test-dump-cache-tree\n> diff --git a/Makefile b/Makefile\n> index 07ea105..9544efb 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n>\n>  TEST_PROGRAMS_NEED_X += test-chmtime\n>  TEST_PROGRAMS_NEED_X += test-ctype\n> +TEST_PROGRAMS_NEED_X += test-config\n>  TEST_PROGRAMS_NEED_X += test-date\n>  TEST_PROGRAMS_NEED_X += test-delta\n>  TEST_PROGRAMS_NEED_X += test-dump-cache-tree\n> diff --git a/test-config.c b/test-config.c\n> new file mode 100644\n> index 0000000..ff24cb8\n> --- /dev/null\n> +++ b/test-config.c\n> @@ -0,0 +1,93 @@\n> +#include \"cache.h\"\n> +#include \"hashmap.h\"\n> +#include \"string-list.h\"\n> +\n> +/*\n> + * This program gives examples on how to use non-callback based query\n> + * functions like git_config_get_string & git_config_get_string_multi.\n\nSee above regarding the true purpose of test-config. A more accurate\ndescription might be something like this:\n\n    This program exposes the C API of the configuration mechanism\n    as a set of simple commands in order to facilitate testing.\n\n> + *\n> + * Reads stdin and prints result of command to stdout:\n> + *\n> + * print_all -> prints all the key-value pairs contained in the hashmap\n> + *             also checks if all entries in the hashmap matches with\n> + *             the content of config files\n\nSome comments:\n\nThis doesn't just print all values. It's doing validation on its own,\nso the name 'print_all' is misleading.\n\nThis isn't part of the C API which you are exposing, so such a command\nis somewhat out of place here. A script in t/ could perform similar\nfunctionality via the exposed API.\n\nHaving said that, it probably doesn't hurt to do such internal\nintegrity checking, however, a better command name is warranted.\nPerhaps 'check_integrity'. Moreover, a command which checks internal\nintegrity need not verbosely print all items. More useful, upon\nsuccess, would be to emit nothing at all or a simple \"ok\" or such. On\nfailure, emit enough information about detected problems to allow them\nto be tracked down and fixed.\n\n> + *\n> + * get_value -> prints the value with highest priority for the entered key\n> + *\n> + * get_all_values -> prints all values for the given key in increasing order\n> + *                  of priority\n\nSince you're exposing the C API, it might make sense to name these\nafter the C functions. For instance, \"get_string\" and\n\"get_string_multi\". (This can become important as the API is\nexpanded.)\n\n> + * Examples:\n> + *\n> + * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n> + *     test-config get_value \"foo.bAr Baz.rock\"\n> + *\n> + */\n> +\n> +static const char *v;\n> +static const struct string_list *strptr;\n> +static int i;\n> +static int *flag;\n\nThese variables are not shared between different functions in this\nfile, so they don't need to be global. Instead, you should declare\nthem as local variables where needed.\n\n> +static int config_cache_callback(const char *key, const char *value, void *unused)\n\nThis name doesn't convey much. Since this is actually implementing\nfunctionality of the 'show_all' command, perhaps show_all_cb() would\nbe better. Even better, given the comments above,\ncheck_integrity_cb().\n\n> +{\n> +       strptr = git_config_get_string_multi(key);\n> +       if (strptr) {\n> +               for (i = 0; i < strptr->nr; i++)\n> +               {\n\nStyle: { on same line as 'for'\n\n> +                       v = strptr->items[i].string;\n> +                       flag = strptr->items[i].util;\n> +                       /* NULL values are flagged as 1 */\n> +                       if (*flag == 1)\n> +                               printf(\"%s\\n\", key);\n> +                       else if (*flag == 0)\n> +                               printf(\"%s=%s\\n\", key, v);\n> +                       /* key-value pair printed so flag them as done */\n> +                       *flag = -1;\n\nCurious: It's not clear what the purpose of this -1 is. Is the\ncallback being invoked multiple times with the same key/value pair?\n\n> +               }\n> +               return 0;\n> +       } else {\n> +               printf(\"%s\\n\", \"Config hashmap inconsistent\\n\");\n\nSpelling out the actual inconsistency, rather than making a only\ngeneral statement, could help a programmer diagnose the problem.\n\n> +               return -1;\n> +       }\n> +}\n> +\n> + int main(int argc, char **argv)\n> +{\n> +       if (argc == 2 && !strcmp(argv[1], \"print_all\")) {\n> +               git_config(config_cache_callback, NULL);\n> +               return 0;\n> +       } else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n> +               /* enter key in canonical form enclosed in quotes */\n\nWhat does this comment mean? Is it telling us what the code is doing\nor is it an instruction to the person typing at the shell? The bit\nabout \"canonical form\" is something which you can document in the\ncomment block at the top of this file.\n\n> +               if (!git_config_get_string(argv[2], &v)) {\n> +                       printf(\"%s\\n\", v);\n\nWhat happens if this is a boolean (NULL) value? Behavior of '%s' on a\nNULL value is undefined. Better would be to emit some special,\nrecognizable token when NULL.\n\n> +                       return 0;\n> +               } else {\n> +                       printf(\"%s\\n\", \"Value not found for the entered key\\n\");\n\nAlthough the key was specified on the command-line, this diagnostic\ncan be more helpful by mentioning which key was not found.\n\n> +                       return -1;\n> +               }\n> +       } else if (argc == 3 && !strcmp(argv[1], \"get_all_values\")) {\n> +               /* enter key in canonical form enclosed in quotes */\n\nDitto regarding comment.\n\n> +               strptr = git_config_get_string_multi(argv[2]);\n> +               if (strptr) {\n> +                       for (i = 0; i < strptr->nr; i++)\n> +                       {\n\nStyle: { on same line as 'for'\n\n> +                               v = strptr->items[i].string;\n> +                               flag = strptr->items[i].util;\n> +                               /* prints NULL values as \"-\" */\n\nRather than \"-\", it might make more sense to choose a more unique and\nidentifiable representation which can be easily and reliably\nrecognized by the test script.\n\n> +                               if (*flag)\n> +                                       printf(\"%s \", \"-\");\n> +                               else\n> +                                       printf(\"%s \", v);\n> +                       }\n> +                       printf(\"\\n\");\n\nPrinting all items of a multi-part config value on a single line\ndelimited by spaces may not be the best output format. The values\nthemselves may contain whitespace. Taking into consideration that this\noutput should be easily consumable by a test script, it probably makes\nmore sees to print each value on its own line.\n\n> +                       return 0;\n> +               } else {\n> +                       printf(\"%s\\n\", \"Value not found for the entered key\\n\");\n\nTo be helpful, say which key was not found.\n\n> +                       return -1;\n> +               }\n> +       }\n> +\n> +       fprintf(stderr, \"%s: unknown function name: %s\\n\", argv[0],\n> +               argv[1] ? argv[1] : \"(there was none)\");\n\nThis is a bit of a lie. This diagnostic will be triggered, not only\nfor an unrecognized function, but also if the number of arguments to a\nfunction was incorrect.\n\n> +       return 1;\n> +}\n> --\n> 1.9.0.GIT\n"},{"id":"244975","messageId":"xmqq61joamcc.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"53A99FEB.5040808@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T18:13:55Z","receivedAt":"2014-06-25T18:13:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> On 24/06/14 00:25, Junio C Hamano wrote:\n> ...\n>> Yup, that is a very good point.  There needs an infrastructure to\n>> tie a set of files (i.e. the standard one being the chain of\n>> system-global /etc/gitconfig to repo-specific .git/config, and any\n>> custom one that can be specified by the caller like submodule code)\n>> to a separate hashmap; a future built-in submodule code would use\n>> two hashmaps aka \"config-caches\", one to manage the usual\n>> \"configuration\" and the other to manage the contents of the\n>> .gitmodules file.\n>> \n>\n> I had expected to see one hash table per file/blob, with the three\n> standard config hash tables linked together to implement the scope/\n> priority rules. (Well, these could be merged into one, as the current\n> code does, since that makes handling \"multi\" keys slightly easier).\n\nAgain, good point.  I think a rough outline of a design that take\nboth\n\n (1) we may have to read two or more separate sets of \"config like\n     things\" (e.g. the contents from the normal config system and\n     the contents from the .gitmodules file) and\n\n (2) we may have to read two or more files that make up a logically\n     single set of \"config-like things\" (e.g. the \"normal config\n     system\" reads from three separate files)\n\ninto account may look like this:\n\n * Each instance of in-core \"config-like things\" is expressed as a\n   struct \"config-set\".\n\n * A \"config-set\" points at an ordered set of struct \"config-file\",\n   each of which represents what was read and cached in-core from a\n   file.\n\n * When we know or notice that a single file on the filesystem was\n   modified, we do not have to invalidate the whole \"config-set\"\n   that depends on the file; the \"config-file\" that corresponds to\n   the file on the filesystem is invalidated instead.\n\n * The most generic API to read the values for a given key or\n   enumerate the keys in a set of \"config-like things\" takes\n   \"config-set\" as an argument, and reads from the ordered set of\n   \"config-file\" to keep the established illusion that we read them\n   all and accumulate, leading to \"the last one wins\" for single\n   valued variables.\n\n * Because reading from the normal config system happens everywhere\n   in the existing code, we will have one struct \"config-set\"\n   instance, called \"the_config_set\", and have a parallel API of the\n   most generic API above, that do not take the \"config-set\" as an\n   explicit argument.  They operate on the_config_set singleton\n   instance.\n\n\nThe implementation of the API function \"git-config-get-string\" may\nlook like this:\n\n\tint git_config_get_string(const char *key, const char **value)\n\t{\n        \treturn git_configset_get_string(&the_config_set, key, value);\n\t}\n\nwhich is the \"thin-wrapper\" for the more generic API to allow you\nread from an arbitrary config_set, which may even look like this:\n\n\t#define git_config_get_string(k, v) \\\n        \tgit_configset_get_string(&the_config_set, (k), (v))\n\n\nWhen the submodule script that uses \"git config -f .gitmodules\" is\nconverted into C, if the updated config API is ready, it may be able\nto do something like these in a single program:\n\n\tconst char *url;\n\tstruct config_set *gm_config;\n\n        /* read from $GIT_DIR/config */\n        url = git_config_get_string(\"remote.origin.url\");\n\n        /*\n         * allow us to read from .gitmodules; the parameters are\n         * list of files that make up the configset, perhaps.\n         */\n\tgm_config = git_configset_new(\".gitmodules\", NULL);\n\n\n        if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\")) {\n\t\t/* do a lot of stuff for the submodule */\n                ...\n\t}\n\n        /* when we are done with the configset */\n        git_configset_clear(gm_config);\n"},{"id":"244976","messageId":"xmqq1tucalzy.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"53A9276D.3020503@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T18:21:21Z","receivedAt":"2014-06-25T18:21:21Z","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> What changes should I implement in this series? Should I add new\n> user facing API adding to the singleton callers which are already in\n> this series.\n\nI think the underlying data structures that represent what the\nentire set of config data is needs to be refined.\n\nWe did this kind of conversion once, back when we made it possible\nto handle more than one instances of in-core cache.  You may want to\nstudy the series that implemented the transition to learn ideas from.\n\n - We used to have a single \"struct cache_entry **cache\", a single\n   pair of \"unsigned int cache_nr, cache_alloc\", etc. and the first\n   patch to introduce the feature was to define \"struct index_state\"\n   that holds all of these \"what the entire set of data that\n   represent the status of the in-core cache is\" variables.\n\n - We also used to have a set of functions that work on the\n   singleton instance of the in-core cache (read_cache(),\n   write_cache(), etc.).  We introduced a new set of API functions\n   to take an explicit pointer to the \"struct index_state\" we want\n   to work on, and made these old \"assume the singleton\" functions a\n   set of thin wrappers that pass &the_index to the corresponding\n   new API functions.\n"},{"id":"244978","messageId":"53AB2FAA.0@gmail.com","threadId":"36981","inReplyTo":"xmqq61joamcc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-25T20:23:06Z","receivedAt":"2014-06-25T20:23:06Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 25.06.2014 20:13, schrieb Junio C Hamano:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n> \n>> On 24/06/14 00:25, Junio C Hamano wrote:\n>> ...\n>>> Yup, that is a very good point.  There needs an infrastructure to\n>>> tie a set of files (i.e. the standard one being the chain of\n>>> system-global /etc/gitconfig to repo-specific .git/config, and any\n>>> custom one that can be specified by the caller like submodule code)\n>>> to a separate hashmap; a future built-in submodule code would use\n>>> two hashmaps aka \"config-caches\", one to manage the usual\n>>> \"configuration\" and the other to manage the contents of the\n>>> .gitmodules file.\n>>>\n>>\n>> I had expected to see one hash table per file/blob, with the three\n>> standard config hash tables linked together to implement the scope/\n>> priority rules. (Well, these could be merged into one, as the current\n>> code does, since that makes handling \"multi\" keys slightly easier).\n> \n> Again, good point.  I think a rough outline of a design that take\n> both\n> \n>  (1) we may have to read two or more separate sets of \"config like\n>      things\" (e.g. the contents from the normal config system and\n>      the contents from the .gitmodules file) and\n> \n>  (2) we may have to read two or more files that make up a logically\n>      single set of \"config-like things\" (e.g. the \"normal config\n>      system\" reads from three separate files)\n> \n> into account may look like this:\n> \n>  * Each instance of in-core \"config-like things\" is expressed as a\n>    struct \"config-set\".\n> \n>  * A \"config-set\" points at an ordered set of struct \"config-file\",\n>    each of which represents what was read and cached in-core from a\n>    file.\n\nIs this additional complexity really necessary?\n\nHow would you handle included files? Split up the including file in before / after parts? I.e.\n\n  repo-config-file[include-to-end]\n  included-file\n  repo-config-file[top-to-include]\n  user-config-file\n  ...\n\nLooking up a single-valued key would then be O(n) (where n is the number of sruct config_file's in the config_set) rather than O(1).\n\nLooking up a multi-valued key would involve joining values from all files, every time the value is looked up (dynamically allocating lists on the heap etc.).\n\nThe configuration is typically loaded once, followed by lots of lookups. So from a performance perspective, doing the merging at load time is sure better.\n\n> \n>  * When we know or notice that a single file on the filesystem was\n>    modified, we do not have to invalidate the whole \"config-set\"\n>    that depends on the file; the \"config-file\" that corresponds to\n>    the file on the filesystem is invalidated instead.\n> \n\nWhat's the use case for this? Do you expect e.g. 'git gc' to detect changed depth/window size at run time and adjust the algorithm accordingly? Or do you just intend to cache parsed config data (the latter could be done by recording all involved file names and stats in the config-set and reloading the whole thing if any of the files change)?\n"},{"id":"244979","messageId":"53AB303C.7010305@gmail.com","threadId":"36981","inReplyTo":"53A969DD.2030201@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-25T20:25:32Z","receivedAt":"2014-06-25T20:25:32Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 24.06.2014 14:06, schrieb Tanay Abhra:\n> On 6/23/2014 5:25 PM, Matthieu Moy wrote:\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>>\n>>> +/* for NULL values, 'util' for each `string_list_item` is flagged as 1 */\n>>\n>> It's a void *, so just saying that it is flagged as 1 does not say how\n>> it's encoded. How about \"... is an 'int *' pointing to the value 1\".\n>>\n>> And actually, you can save one malloc by encoding the value directly in\n>> the util pointer itself like\n>>\n>> #define NULL_FLAG (void *)1\n>>\n>>     item->util = NULL_FLAG;\n>>\n>> I find this a bit ugly, but I think Git already uses this in a few\n>> places (git grep 'void \\*) *1' for examples).\n>>\n>> Or you can use a pointer to a single static value:\n>>\n>> static const int boolean_null_flag = 1; // Or even without = 1.\n>>\n>> and then\n>>\n>>     item->util = &boolean_null_flag;\n>>\n> \n> Thanks for the review. I will change the flag format to what you have\n> suggested. Instead of giving the users of the API the headache of\n> thinking about the flag, let me give you an alternative,\n> \n> +const struct string_list *git_config_get_string_multi(const char *key)\n> +{\n> +    int i, *flag;\n> +    struct string_list *temp = config_cache_get_value(key);\n> +    if (!temp)\n> +        return NULL;\n> +    /* create a NODUP string list which can have NULL values */\n> +    struct string_list *l = xcalloc(1, sizeof(*l));\n> +    for(i=0; i < temp->nr; i++) {\n> +        flag = temp->items[i].util;\n> +        if (*flag)\n> +            string_list_append(l, NULL);\n> +        else\n> +            string_list_append(l, temp->items[i].string);\n> +    }\n> +    return l;\n> +}\n> \n\nReturning a list that is 'half-owned' by the caller (i.e. list is, values are not) is kindof strange. Especially weird is that the caller needs to string_list_clear() _and_ free() the list. Why don't you store NULL values in the string_list in the cache instead of doing this flag magic? I.e.\n\n  static int config_cache_add_value(const char *key, const char *value)\n  {\n  ...\n    if (value == NULL)\n      string_list_append_nodup(&e->value_list, NULL);\n    else\n      string_list_append(&e->value_list, value);\n\nor even\n\n    string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n"},{"id":"244981","messageId":"xmqqk38490d5.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"53AB2FAA.0@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T20:53:58Z","receivedAt":"2014-06-25T20:53:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees <karsten.blees@gmail.com> writes:\n\n> Am 25.06.2014 20:13, schrieb Junio C Hamano:\n>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n>>> I had expected to see one hash table per file/blob, with the three\n>>> standard config hash tables linked together to implement the scope/\n>>> priority rules. (Well, these could be merged into one, as the current\n>>> code does, since that makes handling \"multi\" keys slightly easier).\n>> \n>> Again, good point....\n>\n> Is this additional complexity really necessary?\n\nNothing is *really* necessary ;-) and it is possible that the best\nbalance may be at \"parse a single chain of files into a single\nhashtable for a config-set, and if anything changes, re-read\neverything from scratch\".\n\nThe point Ramsay raised about being able to share the pre-parsed\n$HOME/.gitconfig across multiple config-sets (one for the top-level\nsuperproject, and the others for submodules, when having to work\nacross module boundaries) triggered this thought experiment (aka \"I\nam not married to the approach\") to use one table per source.  If we\nwanted to take advantage of updating a single file and not having to\nre-read the whole thing, includes need to be handled a bit more\ncarefully than \"one config-file for one source\", as you noticed, and\na single source may have to be split into multiple pieces.\n\nAnd it is possible that the complexity necessary to do these\ncorrectly may make it not worth pursuing the approach.  Or it may\nnot.  I don't know at this point, and thinking these things through\nto arrive at a good design is part of the GSoC project after all, so\nI'd rather not to think it through to the end myself ;-).\n\n> What's the use case for this? Do you expect e.g. 'git gc' to\n> detect changed depth/window size at run time and adjust the\n> algorithm accordingly?\n\nI did write \"detect\" but I think a more realistic example is that we\ndo git-config-set internally and wish to see the effect inside the\nsame process (i.e. something like \"pull --set-upstream\" that sets\nconfiguration variable for later invocations and also perform the\noperation with the configuration in effect at the same time).\n"},{"id":"244989","messageId":"53AB42BE.9010308@gmail.com","threadId":"36981","inReplyTo":"1403518300-23053-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-25T21:44:30Z","receivedAt":"2014-06-25T21:44:30Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 23.06.2014 12:11, schrieb Tanay Abhra:\n\n[...]\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[I didn't follow all previous discussion, so feel free to ignore me if this has been discussed already]\n\nIs it really necessary to normalize keys on each lookup? The current git config code simply does a 'strcmp(\"lower-case-key\", var)' so normalization shouldn't be necessary for the majority of callers. If a caller uses a non-constant key (e.g. as passed to 'git config --get <name>'), it would be the caller's responsibility to normalize.\n\n[...]\n\n> +int git_config_get_string(const char *key, const char **value)\n\nI would have expected '..._get_string' to return a string. If the return value indicates whether something was found, it should probably be called '..._find_...'?\n\nIn the end, you want typed config functions that do type conversion and handle parse errors, at least for the common types bool/int/string... Thus the generic function that returns unparsed data should probably be called '..._value', not '..._string'?\n\nA typical pull-style config API will also let you specify default values, e.g.:\n\n  const char *git_config_get_string(const char *key, const char *default_value)\n  {\n    const char *value;\n    if (!git_config_find_value(key, &value))\n      return default_value;\n    if (!value)\n      config_error_nonbool();\n    return value;\n  }\n\n  int git_config_get_bool(const char *key, int default_value)\n  {\n    const char *value;\n    if (!git_config_find_value(key, &value))\n      return default_value;\n    return git_config_bool(key, value);\n  }\n"},{"id":"245013","messageId":"53ABDC68.4090701@gmail.com","threadId":"36981","inReplyTo":"CAPig+cTf+8H38SSxoLMSuU9JUSPiEuTCm=8wHFe5QSCK3j8ELw@mail.gmail.com","subject":"Re: [PATCH v3 3/3] test-config: add usage examples for non-callback query functions","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-06-26T08:40:08Z","receivedAt":"2014-06-26T08:40:08Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nI thought about adding a test*.sh file after sending the series.\nNo worries, I will rectify it in the next patch.\nAlso, I have read all your comments.\n\nThanks for the review.\n\nCheers,\nTanay Abhra.\n\nOn 6/25/2014 4:49 PM, Eric Sunshine wrote:\n> On Mon, Jun 23, 2014 at 6:11 AM, Tanay Abhra <tanayabh@gmail.com> wrote:\n>> Add different usage examples for 'git_config_get_string' and\n>> `git_config_get_string_multi`. They will serve as documentation\n>> on how to query for config values in a non-callback manner.\n> \n> This is a good start, but it's not fully what Matthieu was suggesting\n> when he said that you should prove to other developers, by way of\n> reproducible tests, that your changes work. What he meant, was that\n> you should write a test-config program which exposes (as a runnable\n> command) the new config C API you've added, and then write tests which\n> exercise that API and implementation exhaustively.\n> \n> For example, take a look at test-string-list.c and\n> t/t0063-string-list.sh. The C program does no checking itself. It\n> merely exposes the C API via command-line arguments, such as \"split\",\n> \"filter\", etc. The test script then employs that program to perform\n> the actual testing in a reproducible and (hopefully) exhaustive\n> fashion. Because t/t0063-string-list.sh is part of the test suite, the\n> string-list tests are run regularly by many developers. It's not just\n> something that someone might remember to run once in a while.\n> \n> Contrary to your commit message and the comment in the program itself,\n> the purpose of test-config is not to serve as documentation or to\n> provide examples of usage. (Real documentation is better suited for\n> those purposes.) Instead, test-config should exist in support of a\n> real test script in t/ which is run regularly. The new script you add\n> to t/ should exercise the exposed C API as exhaustively as possible.\n> This means checking each possible state: for instance, (1) when a key\n> is absent, (2) when a value is boolean (NULL), (3) one non-boolean\n> (non-NULL) value, (4) multiple values, etc. Moreover, it should check\n> expected success _and_ expected failure modes. Check not only that it\n> returns expected values, but that it fails when appropriate.\n> \n> More below.\n> \n"},{"id":"245022","messageId":"vpq8uojoddu.fsf@anie.imag.fr","threadId":"36981","inReplyTo":"53A99A28.1090302@ramsay1.demon.co.uk","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-26T16:15:57Z","receivedAt":"2014-06-26T16:15:57Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> Hmm, maybe. The \"... take advantage of the new code\" refers to the\n> possibility (or otherwise) of re-using your work to update these\n> \"older API\" functions to the new API style. (also, see Junio's response).\n\nI agree that, while caching the usual config files is the most\nimportant, allowing other users of the config code to use non-callback\nfunctions would be nice too.\n\nNot really for efficiency, but because I find it far more natural to ask\n\"what's the value of config key XYZ\" to the code than to ask \"can you\nparse all config keys in the file, let me know, and I'll tell you later\nwhich ones I'm interested in\".\n\n> [In order to do this, I would have expected to see one hash table\n> for each file/blob, so the singleton object took me by surprise.]\n\nThe singleton could be fine as long as it is optional. We can have\nan API looking like:\n\nint git_config_get_string(const char *key, const char **value) {\n\treturn git_config_get_string_from(the singleton config cache,\n                                          key, value);\n}\n\nint git_config_get_string_from(struct hashmap *map, const char *key, const char **value);\n\n(or take a structure representing \"a set of config files\" instead of\ndirectly the hashmap)\n\nNow, the good news is that such API can be written later, without\nbreaking the API. So I'd say it's fine to keep the code as-is for now,\nand make it more general later. No strong opinion, though.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245023","messageId":"vpqy4wjmyap.fsf@anie.imag.fr","threadId":"36981","inReplyTo":"53A96D2D.1020102@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-26T16:27:10Z","receivedAt":"2014-06-26T16:27:10Z","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> For the config_cache_free(), would this change be enough?\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> +\tif (!hashmap_initialized)\n> +\t\treturn;\n> +\tconfig_cache = get_config_cache();\n\nThat sounds better to me. And it justifies the different scopes: one can\nask whether the map is initialized from anywhere in the file, but can\nonly get the map after initialization, so it prevents mis-use of an\nuninitialized pointer.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245025","messageId":"vpqha37mxjq.fsf@anie.imag.fr","threadId":"36981","inReplyTo":"1403518300-23053-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-26T16:43:21Z","receivedAt":"2014-06-26T16:43:21Z","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> +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 read values from an internal\n> +cache generated previously from reading the config files.\n> +\n> +`git_config_get_string` takes two parameters,\n> +\n> +- a key string in canonical flat form for which the corresponding value\n> +  with the highest priority (i.e. value in the repo config will be\n> +  preferred over value in user wide config for the same variable) will\n> +  be retrieved.\n> +\n> +- a pointer to a string which will point to the retrieved value.\n> +\n> +`git_config_get_string` returns 0 for success, or -1 for no value found.\n> +\n> +`git_config_get_string_multi` returns a `string_list` containing all the\n> +values for the key passed as parameter, sorted in order of increasing\n> +priority (Note: NULL values are flagged as 1, check `util` for each\n> +'string_list_item` for flag value).\n\nI think you need to add something like:\n\n   Both functions return pointers to values owned by the config cache. The\n   caller need not free them, but should not modify them.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245029","messageId":"vpq1tubinbx.fsf@anie.imag.fr","threadId":"36981","inReplyTo":"xmqq61joamcc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-26T17:37:38Z","receivedAt":"2014-06-26T17:37:38Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> When the submodule script that uses \"git config -f .gitmodules\" is\n> converted into C, if the updated config API is ready, it may be able\n> to do something like these in a single program:\n>\n> \tconst char *url;\n> \tstruct config_set *gm_config;\n>\n>         /* read from $GIT_DIR/config */\n>         url = git_config_get_string(\"remote.origin.url\");\n>\n>         /*\n>          * allow us to read from .gitmodules; the parameters are\n>          * list of files that make up the configset, perhaps.\n>          */\n> \tgm_config = git_configset_new(\".gitmodules\", NULL);\n>\n>\n>         if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\")) {\n> \t\t/* do a lot of stuff for the submodule */\n>                 ...\n> \t}\n>\n>         /* when we are done with the configset */\n>         git_configset_clear(gm_config);\n\nIsn't that a bit overkill? Why not just let the caller manage a hashmap\ndirectly instead of a config_set? Your code could become\n\n  struct hashmap *gm_config;\n  config_cache_init(&gm_config);\n  config_cache_read_from_file(\".gitmodule\", gm_config);\n  /* possibly more calls to\n     config_cache_read_from_file(\"some-other-file\", ...). */\n  \nand then\n\n  if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\")) {\n     ...\n  \n\nPerhaps a bit more complicated to use, but much simpler to implement.\nSince there are very few callers, I'd say it's better to keep the\nimplementation simple.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245034","messageId":"xmqqpphv4hu6.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"vpq1tubinbx.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-26T19:00:01Z","receivedAt":"2014-06-26T19:00:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> When the submodule script that uses \"git config -f .gitmodules\" is\n>> converted into C, if the updated config API is ready, it may be able\n>> to do something like these in a single program:\n>>\n>> \tconst char *url;\n>> \tstruct config_set *gm_config;\n>>\n>>         /* read from $GIT_DIR/config */\n>>         url = git_config_get_string(\"remote.origin.url\");\n>>\n>>         /*\n>>          * allow us to read from .gitmodules; the parameters are\n>>          * list of files that make up the configset, perhaps.\n>>          */\n>> \tgm_config = git_configset_new(\".gitmodules\", NULL);\n>>\n>>\n>>         if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\")) {\n>> \t\t/* do a lot of stuff for the submodule */\n>>                 ...\n>> \t}\n>>\n>>         /* when we are done with the configset */\n>>         git_configset_clear(gm_config);\n>\n> Isn't that a bit overkill? Why not just let the caller manage a hashmap\n> directly instead of a config_set?\n\nBecause I had an experience under my belt of a painful refactoring\nof \"the_index\" which turned out to be not just a single array, I\nsimply suspect that the final data structure to represent a \"set of\nconfig-like things\" will not be just a single hashmap, hence I do\nprefer to have one layer of abstraction \"struct config_set\", which\nwould contain a hashmap and possibly more.  Doesn't \"is the hashmap\ninitialized\" bit belong there, for example?\n"},{"id":"245036","messageId":"53AC724E.3090106@gmail.com","threadId":"36981","inReplyTo":"xmqqpphv4hu6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-26T19:19:42Z","receivedAt":"2014-06-26T19:19:42Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 26.06.2014 21:00, schrieb Junio C Hamano:\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n> \n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> When the submodule script that uses \"git config -f .gitmodules\" is\n>>> converted into C, if the updated config API is ready, it may be able\n>>> to do something like these in a single program:\n>>>\n>>> \tconst char *url;\n>>> \tstruct config_set *gm_config;\n>>>\n>>>         /* read from $GIT_DIR/config */\n>>>         url = git_config_get_string(\"remote.origin.url\");\n>>>\n>>>         /*\n>>>          * allow us to read from .gitmodules; the parameters are\n>>>          * list of files that make up the configset, perhaps.\n>>>          */\n>>> \tgm_config = git_configset_new(\".gitmodules\", NULL);\n>>>\n>>>\n>>>         if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\")) {\n>>> \t\t/* do a lot of stuff for the submodule */\n>>>                 ...\n>>> \t}\n>>>\n>>>         /* when we are done with the configset */\n>>>         git_configset_clear(gm_config);\n>>\n>> Isn't that a bit overkill? Why not just let the caller manage a hashmap\n>> directly instead of a config_set?\n> \n> Because I had an experience under my belt of a painful refactoring\n> of \"the_index\" which turned out to be not just a single array, I\n> simply suspect that the final data structure to represent a \"set of\n> config-like things\" will not be just a single hashmap, hence I do\n> prefer to have one layer of abstraction \"struct config_set\", which\n> would contain a hashmap and possibly more.  Doesn't \"is the hashmap\n> initialized\" bit belong there, for example?\n\nWould an additional\n\n  int hashmap_is_initialized(constr struct hashmap *map)\n  {\n    return !!map->table;\n  }\n\nAPI help? (Note that hashmap_free() already does memset(0), so the usual notion of \"zero memory means unitialized\" applies).\n"},{"id":"245045","messageId":"xmqqzjgz2wqr.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"53AC724E.3090106@gmail.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-26T21:21:00Z","receivedAt":"2014-06-26T21:21:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees <karsten.blees@gmail.com> writes:\n\n>> Because I had an experience under my belt of a painful refactoring\n>> of \"the_index\" which turned out to be not just a single array, I\n>> simply suspect that the final data structure to represent a \"set of\n>> config-like things\" will not be just a single hashmap, hence I do\n>> prefer to have one layer of abstraction \"struct config_set\", which\n>> would contain a hashmap and possibly more.  Doesn't \"is the hashmap\n>> initialized\" bit belong there, for example?\n>\n> Would an additional\n>\n>   int hashmap_is_initialized(constr struct hashmap *map)\n>   {\n>     return !!map->table;\n>   }\n>\n> API help? (Note that hashmap_free() already does memset(0), so the\n> usual notion of \"zero memory means unitialized\" applies).\n\nIt may remove the need for the separate \"hashmap_initialized\" bit\nthat was implemented as a file-scope global in the patch.\n\nI however am not convinced that it will be the _only_ thing other\nthan the hashmap we would need to use to keep track of the in-core\n\"set of config-like things\", and usually a blanket statement \"these\nare the only thing we would ever need\" tends not to hold for long,\nso...\n"},{"id":"245057","messageId":"53AD2914.4030508@gmail.com","threadId":"36981","inReplyTo":"xmqqzjgz2wqr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-06-27T08:19:32Z","receivedAt":"2014-06-27T08:19:32Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 26.06.2014 23:21, schrieb Junio C Hamano:\n> Karsten Blees <karsten.blees@gmail.com> writes:\n> \n>>> Because I had an experience under my belt of a painful refactoring\n>>> of \"the_index\" which turned out to be not just a single array, I\n>>> simply suspect that the final data structure to represent a \"set of\n>>> config-like things\" will not be just a single hashmap, hence I do\n>>> prefer to have one layer of abstraction \"struct config_set\", which\n>>> would contain a hashmap and possibly more.  Doesn't \"is the hashmap\n>>> initialized\" bit belong there, for example?\n>>\n>> Would an additional\n>>\n>>   int hashmap_is_initialized(constr struct hashmap *map)\n>>   {\n>>     return !!map->table;\n>>   }\n>>\n>> API help? (Note that hashmap_free() already does memset(0), so the\n>> usual notion of \"zero memory means unitialized\" applies).\n> \n> It may remove the need for the separate \"hashmap_initialized\" bit\n> that was implemented as a file-scope global in the patch.\n> \n\nThat the variable is redundant was all I was trying to say, sorry for\nthe misunderstanding. But reading through api-hashmap.txt again, I\nfound that documentation of the hashmap members is pretty vague, so\nperhaps improving the docs would suffice (e.g. \"hashmap.table != NULL\nindicates the hashmap has been initialized.\").\n\n> I however am not convinced that it will be the _only_ thing other\n> than the hashmap we would need to use to keep track of the in-core\n> \"set of config-like things\", and usually a blanket statement \"these\n> are the only thing we would ever need\" tends not to hold for long,\n> so...\n>\n\nI think having an extra type for the configuration is a Good Thing\n(even if the only member is a hashmap). Although I wouldn't call it\n'config_set', as it conflicts with the verb 'set', resulting in\nweird APIs ('config_set_get_value()', 'config_set_set_value()').\nPerhaps 'config_map' or 'config_settings', or just 'config'?\n"},{"id":"245058","messageId":"vpqzjgyg3x5.fsf@anie.imag.fr","threadId":"36981","inReplyTo":"xmqqpphv4hu6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-06-27T08:19:50Z","receivedAt":"2014-06-27T08:19:50Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> When the submodule script that uses \"git config -f .gitmodules\" is\n>>> converted into C, if the updated config API is ready, it may be able\n>>> to do something like these in a single program:\n>>>\n>>> \tconst char *url;\n>>> \tstruct config_set *gm_config;\n>>>\n>>>         /* read from $GIT_DIR/config */\n>>>         url = git_config_get_string(\"remote.origin.url\");\n>>>\n>>>         /*\n>>>          * allow us to read from .gitmodules; the parameters are\n>>>          * list of files that make up the configset, perhaps.\n>>>          */\n>>> \tgm_config = git_configset_new(\".gitmodules\", NULL);\n>>>\n>>>\n>>>         if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\")) {\n>>> \t\t/* do a lot of stuff for the submodule */\n>>>                 ...\n>>> \t}\n>>>\n>>>         /* when we are done with the configset */\n>>>         git_configset_clear(gm_config);\n>>\n>> Isn't that a bit overkill? Why not just let the caller manage a hashmap\n>> directly instead of a config_set?\n>\n> Because I had an experience under my belt of a painful refactoring\n> of \"the_index\" which turned out to be not just a single array, I\n> simply suspect that the final data structure to represent a \"set of\n> config-like things\" will not be just a single hashmap, hence I do\n> prefer to have one layer of abstraction \"struct config_set\", which\n> would contain a hashmap and possibly more.\n\nOK, I guess I overinterpreted what you meant by \"struct config_set\". If\nit's a thin abstraction layer on top of the hashmap (i.e. essentially\ncontain the hashmap, and possibly a few more metadata), then it\ndefinitely makes sense.\n\n> Doesn't \"is the hashmap initialized\" bit belong there, for example?\n\nYes.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245079","messageId":"xmqqr42a2s3r.fsf@gitster.dls.corp.google.com","threadId":"36981","inReplyTo":"vpqzjgyg3x5.fsf@anie.imag.fr","subject":"Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-27T17:13:28Z","receivedAt":"2014-06-27T17:13:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>>> Isn't that a bit overkill? Why not just let the caller manage a hashmap\n>>> directly instead of a config_set?\n>>\n>> Because I had an experience under my belt of a painful refactoring\n>> of \"the_index\" which turned out to be not just a single array, I\n>> simply suspect that the final data structure to represent a \"set of\n>> config-like things\" will not be just a single hashmap, hence I do\n>> prefer to have one layer of abstraction \"struct config_set\", which\n>> would contain a hashmap and possibly more.\n>\n> OK, I guess I overinterpreted what you meant by \"struct config_set\". If\n> it's a thin abstraction layer on top of the hashmap (i.e. essentially\n> contain the hashmap, and possibly a few more metadata), then it\n> definitely makes sense.\n\nYup, and I do not strongly mind the initialization sequence of \"if\nyou want to overlay from a file, call *_from_file()\" you outlined;\nan initialiser that takes a list of file paths to read from was\nmerely an example and not meant to be the sole interface (it is\noverly rigid to be one).\n"}]}