{"thread":{"id":"37065","subject":"[PATCH v5 0/3] git config cache & special querying api utilizing the cache","startedAt":"2014-07-06T07:19:20Z","lastAt":"2014-07-07T07:11:37Z","messageCount":9,"participants":["Tanay Abhra","Ramsay Jones","Matthieu Moy","Ramkumar Ramachandra"],"isPatch":true,"patchVersion":5,"patchTotal":3},"messages":[{"id":"245436","messageId":"1404631162-18556-1-git-send-email-tanayabh@gmail.com","threadId":"37065","inReplyTo":null,"subject":"[PATCH v5 0/3] git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-06T07:19:20Z","receivedAt":"2014-07-06T07:19:20Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\n[PATCH V5]: `config_set` now uses a single hashmap. Corrected style nits raised in\n\t\t\tthe thread[7]. Thanks to Junio and Matthieu for their suggestions.\n\n[PATCH v4]: Introduced `config_set` construct which points to a ordered set of\n\tconfig-files cached as hashmaps. Added relevant API functions. For more\n\tdetails see the documentation. Rewrote the git_config_get* family to use\n\t`config_set` internally. Added tests for both config_set API and git_config_get\n\tfamily. Added type specific API functions which parses the found value and\n\tconverts it into a specific type.\n\tMost of the changes implemented are the result of discussion in [6].\n\tThanks to Eric, Ramsay, Junio, Matthieu & Karsten for their suggestions\n\tand review.\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 patch is 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[6] http://thread.gmane.org/gmane.comp.version-control.git/252329/\n[7] http://marc.info/?t=140428115200001&r=1&w=2\n\n\nTanay Abhra (2):\n  config-hash.c\n  test-config\n\n .gitignore                             |   1 +\n Documentation/technical/api-config.txt | 134 +++++++++++++++\n Makefile                               |   2 +\n cache.h                                |  34 ++++\n config-hash.c                          | 297 +++++++++++++++++++++++++++++++++\n config.c                               |   3 +\n t/t1308-config-hash.sh                 | 187 +++++++++++++++++++++\n test-config.c                          | 127 ++++++++++++++\n 8 files changed, 785 insertions(+)\n create mode 100644 config-hash.c\n create mode 100755 t/t1308-config-hash.sh\n create mode 100644 test-config.c\n\n-- \n1.9.0.GIT\n"},{"id":"245437","messageId":"1404631162-18556-2-git-send-email-tanayabh@gmail.com","threadId":"37065","inReplyTo":"1404631162-18556-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v5 1/2] add `config_set` API for caching config-like files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-06T07:19:21Z","receivedAt":"2014-07-06T07:19:21Z","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 `config_set`, that can be used to construct an in-memory cache for\nconfig-like files that the caller specifies (i.e., files like `.gitmodules`,\n`~/.gitconfig` etc.). Add two external functions `git_configset_get_value`\nand `git_configset_get_value_multi` for querying from the config sets.\n`git_configset_get_value` follows `last one wins` semantic (i.e. if there\nare multiple matches for the queried key in the files of the configset the\nvalue returned will be the last entry in `value_list`).\n`git_configset_get_value_multi` returns a list of values sorted in order of\nincreasing priority (i.e. last match will be at the end of the list). Add\ntype specific query functions like `git_configset_get_bool` and similar.\n\nAdd a default `config_set`, `the_config_set` to cache all key-value pairs\nread from usual config files (repo specific .git/config, user wide\n~/.gitconfig and the global /etc/gitconfig). `the_config_set` uses a\nsingle hashmap populated using `git_config()`.\n\nAdd two external functions `git_config_get_value` and \n`git_config_get_value_multi` for querying in a non-callback manner from\n`the_config_set`. Also, add type specific query functions that are\nimplemented as a thin wrapper around the `config_set` API.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n Documentation/technical/api-config.txt | 134 +++++++++++++++\n Makefile                               |   1 +\n cache.h                                |  34 ++++\n config-hash.c                          | 297 +++++++++++++++++++++++++++++++++\n config.c                               |   3 +\n 5 files changed, 469 insertions(+)\n create mode 100644 config-hash.c\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..bdf86d0 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,81 @@ 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_value`\n+and `git_config_get_value_multi`. They both read values from an internal\n+cache generated previously from reading the config files.\n+\n+`int git_config_get_value(const char *key, const char **value)`::\n+\n+\tFinds the highest-priority value for the configuration variable `key`,\n+\tstores the pointer to it in `value` and returns 0. When the\n+\tconfiguration variable `key` is not found, returns 1 without touching\n+\t`value`. The caller should not free or modify `value`, as it is owned\n+\tby the cache.\n+\n+`const struct string_list *git_config_get_value_multi(const char *key)`::\n+\n+\tFinds and returns the value list, sorted in order of increasing priority\n+\tfor the configuration variable `key`. When the configuration variable\n+\t`key` is not found, returns NULL. The caller should not free or modify\n+\tthe returned pointer, as it is owned by the cache.\n+\n+`void git_config_clear(void)`::\n+\n+\tResets and invalidate the config cache. \n+\n+The config API also provides type specific API functions which do conversion\n+as well as retrieval for the queried variable, including:\n+\n+`int git_config_get_int(const char *key, int *dest)`::\n+\n+\tFinds and parses the value to an integer for the configuration variable\n+\t`key`. Dies on error; otherwise, stores pointer to the parsed integer in\n+\t`dest` and returns 0. When the configuration variable `key` is not found,\n+\treturns 1 without touching `dest`.\n+\n+`int git_config_get_ulong(const char *key, unsigned long *dest)`::\n+\n+\tSimilar to `git_config_get_int` but for unsigned longs.\n+\n+`int git_config_get_int(const char *key, int *dest)`::\n+\n+\tFinds and parses the value into a boolean value, for the configuration\n+\tvariable `key`respecting keywords like \"true\" and \"false\". Integer\n+\tvalues are converted into true/false values (when they are non-zero or\n+\tzero, respectively). Other values cause a die(). If parsing is successful,\n+\tstores the pointer to the parsed result in `dest` and returns 0. When the\n+\tconfiguration variable `key` is not found, returns 1 without touching\n+\t`dest`.\n+\n+`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`::\n+\n+\tSimilar to `git_config_get_bool`, except that integers are copied as-is,\n+\tand `is_bool` flag is unset.\n+\n+`int git_config_get_maybe_bool(const char *key, int *dest)`::\n+\n+\tSimilar to `git_config_get_bool`, except that it returns -1 on error\n+\trather than dying.\n+\n+`int git_config_get_string(const char *key, const char **dest)`::\n+\n+\tAllocates and copies the retrieved string into the `dest` parameter for\n+\tthe configuration variable `key`; if NULL string is given, prints an\n+\terror message and returns -1. When the configuration variable `key` is\n+\tnot found, returns 1 without touching `dest`.\n+\n+`int git_config_get_pathname(const char *key, const char **dest)`::\n+\n+\tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n+\tthe user's home directory when found at the beginning of the path.\n+\n+See test-config.c for usage examples.\n+\n Value Parsing Helpers\n ---------------------\n \n@@ -134,6 +209,65 @@ int read_file_with_include(const char *file, config_fn_t fn, void *data)\n `git_config` respects includes automatically. The lower-level\n `git_config_from_file` does not.\n \n+Custom Configsets\n+-----------------\n+\n+A `config_set` can be used to construct an in-memory cache for config-like\n+files that the caller specifies (i.e., files like `.gitmodules`,\n+`~/.gitconfig` etc.). For example,\n+\n+---------------------------------------\n+struct config_set gm_config;\n+git_configset_init(&gm_config);\n+int b;\n+/* we add config files to the config_set */\n+git_configset_add_file(&gm_config, \".gitmodules\");\n+git_configset_add_file(&gm_config, \".gitmodules_alt\");\n+\n+if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\", &b)) {\n+\t/* hack hack hack */\n+}\n+\n+/* when we are done with the configset */\n+git_configset_clear(&gm_config);\n+----------------------------------------\n+\n+Configset API provides functions for the above mentioned work flow, including:\n+\n+`void git_configset_init(struct config_set *cs)`::\n+\n+\tInitializes the member variables of config_set `cs`.\n+\n+`int git_configset_add_file(struct config_set *cs, const char *filename)`::\n+\n+\tParses the file and adds the variable-value pairs to the `config_set`,\n+\tdies if there is an error in parsing the file.\n+\n+`int git_configset_get_value(struct config_set *cs, const char *key, const char **value)`::\n+\n+\tFinds the highest-priority value for the configuration variable `key`\n+\tand config set `cs`, stores the pointer to it in `value` and returns 0.\n+\tWhen the configuration variable `key` is not found, returns 1 without\n+\ttouching `value`. The caller should not free or modify `value`, as it\n+\tis owned by the cache.\n+\n+`const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)`::\n+\n+\tFinds and returns the value list, sorted in order of increasing priority\n+\tfor the configuration variable `key` and config set `cs`. When the\n+\tconfiguration variable `key` is not found, returns NULL. The caller\n+\tshould not free or modify the returned pointer, as it is owned by the cache.\n+\n+`void git_configset_clear(struct config_set *cs)`::\n+\n+\tClears `config_set` structure, removes all saved variable-value pairs.\n+\n+In addition to above functions, the `config_set` API provides type specific\n+functions in the vein of `git_config_get_int` and family but with an extra\n+parameter, pointer to struct `config_set`.\n+They all behave similarly to the `git_config_get*()` family described in\n+\"Querying For Specific Variables\" above.\n+\n Writing Config Files\n --------------------\n \ndiff --git a/Makefile b/Makefile\nindex 07ea105..d503f78 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -777,6 +777,7 @@ LIB_OBJS += commit.o\n LIB_OBJS += compat/obstack.o\n LIB_OBJS += compat/terminal.o\n LIB_OBJS += config.o\n+LIB_OBJS += config-hash.o\n LIB_OBJS += connect.o\n LIB_OBJS += connected.o\n LIB_OBJS += convert.o\ndiff --git a/cache.h b/cache.h\nindex df65231..35bd71e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1325,6 +1325,40 @@ extern int parse_config_key(const char *var,\n \t\t\t    const char **subsection, int *subsection_len,\n \t\t\t    const char **key);\n \n+/* config-hash.c */\n+\n+struct config_set {\n+\tstruct hashmap config_hash;\n+\tint hash_initialized;\n+};\n+\n+extern void git_configset_init(struct config_set *cs);\n+extern int git_configset_add_file(struct config_set *cs, const char *filename);\n+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n+extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n+extern void git_configset_clear(struct config_set *cs);\n+extern void git_configset_iter(struct config_set *cs, config_fn_t fn, void *data);\n+extern int git_configset_get_string(struct config_set *cs, const char *key, const char **dest);\n+extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);\n+extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);\n+extern int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);\n+\n+extern int git_config_get_value(const char *key, const char **value);\n+extern const struct string_list *git_config_get_value_multi(const char *key);\n+extern void git_config_clear(void);\n+extern void git_config_iter(config_fn_t fn, void *data);\n+extern int git_config_get_string(const char *key, const char **dest);\n+extern int git_config_get_int(const char *key, int *dest);\n+extern int git_config_get_ulong(const char *key, unsigned long *dest);\n+extern int git_config_get_bool(const char *key, int *dest);\n+extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n+extern int git_config_get_maybe_bool(const char *key, int *dest);\n+extern int git_config_get_pathname(const char *key, const char **dest);\n+\n+\n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\n \ndiff --git a/config-hash.c b/config-hash.c\nnew file mode 100644\nindex 0000000..4c56bd9\n--- /dev/null\n+++ b/config-hash.c\n@@ -0,0 +1,297 @@\n+#include \"cache.h\"\n+#include \"hashmap.h\"\n+#include \"string-list.h\"\n+\n+\n+/*\n+ * Default config_set that contains key-value pairs from the usual set of config\n+ * config files (i.e repo specific .git/config, user wide ~/.gitconfig and the\n+ * global /etc/gitconfig)\n+ */\n+static struct config_set the_config_set;\n+\n+struct config_hash_entry {\n+\tstruct hashmap_entry ent;\n+\tchar *key;\n+\tstruct string_list value_list;\n+};\n+\n+static int config_hash_add_value(const char *, const char *, struct hashmap *);\n+\n+static int config_hash_entry_cmp(const struct config_hash_entry *e1,\n+\t\t\t\t const struct config_hash_entry *e2, const void *unused)\n+{\n+\treturn strcmp(e1->key, e2->key);\n+}\n+\n+static void config_hash_init(struct hashmap *config_hash)\n+{\n+\thashmap_init(config_hash, (hashmap_cmp_fn)config_hash_entry_cmp, 0);\n+}\n+\n+static int config_hash_callback(const char *key, const char *value, void *cb)\n+{\n+\tstruct config_set *cs = cb;\n+\tconfig_hash_add_value(key, value, &cs->config_hash);\n+\treturn 0;\n+}\n+\n+static int add_configset_hash(const char *filename, struct config_set *cs)\n+{\n+\tint ret = 0;\n+\tif (!cs->hash_initialized)\n+\t\tconfig_hash_init(&cs->config_hash);\n+\tcs->hash_initialized = 1;\n+\tret = git_config_from_file(config_hash_callback, filename, cs);\n+\tif (!ret)\n+\t\treturn 0;\n+\telse {\n+\t\thashmap_free(&cs->config_hash, 1);\n+\t\tcs->hash_initialized = 0;\n+\t\treturn -1;\n+\t}\n+}\n+\n+static struct config_hash_entry *config_hash_find_entry(const char *key,\n+\t\t\t\t\t\t\tstruct hashmap *config_hash)\n+{\n+\tstruct config_hash_entry k;\n+\tstruct config_hash_entry *found_entry;\n+\tchar *normalized_key;\n+\tint ret;\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_hash, &k, NULL);\n+\tfree(normalized_key);\n+\treturn found_entry;\n+}\n+\n+static struct string_list *configset_get_list(const char *key, struct config_set *cs)\n+{\n+\tstruct config_hash_entry *e = config_hash_find_entry(key, &cs->config_hash);\n+\treturn e ? &e->value_list : NULL;\n+}\n+\n+static int config_hash_add_value(const char *key, const char *value, struct hashmap *config_hash)\n+{\n+\tstruct config_hash_entry *e;\n+\te = config_hash_find_entry(key, config_hash);\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\tmemset(&e->value_list, 0, sizeof(e->value_list));\n+\t\te->value_list.strdup_strings = 1;\n+\t\thashmap_add(config_hash, e);\n+\t}\n+\t/*\n+\t * Since the values are being fed by git_config*() callback mechanism, they\n+\t * are already normalized. So simply add them without any further munging.\n+\t */\n+\tstring_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\n+\treturn 0;\n+}\n+\n+void git_configset_init(struct config_set *cs)\n+{\n+\tcs->hash_initialized = 0;\n+}\n+\n+int git_configset_add_file(struct config_set *cs, const char *filename)\n+{\n+\treturn add_configset_hash(filename, cs);\n+}\n+\n+int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n+{\n+\tstruct string_list *values = NULL;\n+\t/*\n+\t * Follows \"last one wins\" semantic, i.e., if there are multiple matches for the\n+\t * queried key in the files of the configset, the value returned will be the last\n+\t * value in the value list for that key.\n+\t */\n+\tvalues = configset_get_list(key, cs);\n+\n+\tif (!values)\n+\t\treturn 1;\n+\tassert(values->nr > 0);\n+\t*value = values->items[values->nr - 1].string;\n+\treturn 0;\n+}\n+\n+const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n+{\n+\treturn configset_get_list(key, cs);\n+}\n+\n+void git_configset_clear(struct config_set *cs)\n+{\n+\tstruct config_hash_entry *entry;\n+\tstruct hashmap_iter iter;\n+\tif (!cs->hash_initialized)\n+\t\treturn;\n+\n+\thashmap_iter_init( &cs->config_hash, &iter);\n+\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\tfree(entry->key);\n+\t\tstring_list_clear(&entry->value_list, 0);\n+\t}\n+\thashmap_free(&cs->config_hash, 1);\n+}\n+\n+int git_configset_get_string(struct config_set *cs, const char *key, const char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value))\n+\t\treturn git_config_string(dest, key, value);\n+\telse\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_int(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_ulong(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_bool(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_bool(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_bool_or_int(struct config_set *cs, const char *key,\n+\t\t\t\tint *is_bool, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_bool_or_int(key, value, is_bool);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_maybe_bool(key, value);\n+\t\tif (*dest == -1)\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value))\n+\t\treturn git_config_pathname(dest, key, value);\n+\telse\n+\t\treturn 1;\n+}\n+\n+static int git_config_check_init(void)\n+{\n+\tint ret = 0;\n+\tif (the_config_set.hash_initialized)\n+\t\treturn 0;\n+\tconfig_hash_init(&the_config_set.config_hash);\n+\tthe_config_set.hash_initialized = 1;\n+\tret = git_config(config_hash_callback, &the_config_set);\n+\tif (ret >= 0)\n+\t\treturn 0;\n+\telse {\n+\t\thashmap_free(&the_config_set.config_hash, 1);\n+\t\tthe_config_set.hash_initialized = 0;\n+\t\treturn -1;\n+\t}\n+}\n+\n+int git_config_get_value(const char *key, const char **value)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_value(&the_config_set, key, value);\n+}\n+\n+const struct string_list *git_config_get_value_multi(const char *key)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_value_multi(&the_config_set, key);\n+}\n+\n+void git_config_clear(void)\n+{\n+\tif (!the_config_set.hash_initialized)\n+\t\treturn;\n+\tgit_configset_clear(&the_config_set);\n+\tthe_config_set.hash_initialized = 0;\n+}\n+\n+int git_config_get_string(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_string(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_int(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_int(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_ulong(const char *key, unsigned long *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_ulong(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_bool(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_bool(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_bool_or_int(&the_config_set, key, is_bool, dest);\n+}\n+\n+int git_config_get_maybe_bool(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_maybe_bool(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_pathname(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_pathname(&the_config_set, key, dest);\n+}\ndiff --git a/config.c b/config.c\nindex a1aef1c..6cffec7 100644\n--- a/config.c\n+++ b/config.c\n@@ -1708,6 +1708,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tlock = NULL;\n \tret = 0;\n \n+\t/* Invalidate the config cache */\n+\tgit_config_clear();\n+\n out_free:\n \tif (lock)\n \t\trollback_lock_file(lock);\n-- \n1.9.0.GIT\n"},{"id":"245438","messageId":"1404631162-18556-3-git-send-email-tanayabh@gmail.com","threadId":"37065","inReplyTo":"1404631162-18556-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v5 2/2] test-config: Add tests for the config_set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-06T07:19:22Z","receivedAt":"2014-07-06T07:19:22Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Expose the `config_set` C API as a set of simple commands in order to\nfacilitate testing. Add tests for the `config_set` API as well as for\n`git_config_get_*()` family for the usual config files.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n .gitignore             |   1 +\n Makefile               |   1 +\n t/t1308-config-hash.sh | 187 +++++++++++++++++++++++++++++++++++++++++++++++++\n test-config.c          | 127 +++++++++++++++++++++++++++++++++\n 4 files changed, 316 insertions(+)\n create mode 100755 t/t1308-config-hash.sh\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 42294e5..eeb66e2 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -176,6 +176,7 @@\n /gitweb/static/gitweb.js\n /gitweb/static/gitweb.min.*\n /test-chmtime\n+/test-config\n /test-ctype\n /test-date\n /test-delta\ndiff --git a/Makefile b/Makefile\nindex d503f78..e875070 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -548,6 +548,7 @@ X =\n PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-ctype\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\ndiff --git a/t/t1308-config-hash.sh b/t/t1308-config-hash.sh\nnew file mode 100755\nindex 0000000..eba7102\n--- /dev/null\n+++ b/t/t1308-config-hash.sh\n@@ -0,0 +1,187 @@\n+#!/bin/sh\n+\n+test_description='Test git config-hash API in different settings'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'clear default config' '\n+\trm -f .git/config\n+'\n+\n+test_expect_success 'initialize default config' '\n+\tcat >.git/config << EOF\n+\t[core]\n+\t\tpenguin = very blue\n+\t\tMovie = BadPhysics\n+\t\tUPPERCASE = true\n+\t\tMixedCase = true\n+\t\tmy =\n+\t\tfoo\n+\t\tbaz = sam\n+\t[Cores]\n+\t\tWhatEver = Second\n+\t\tbaz = bar\n+\t[cores]\n+\t\tbaz = bat\n+\t[CORES]\n+\t\tbaz = ball\n+\t[my \"Foo bAr\"]\n+\t\thi = mixed-case\n+\t[my \"FOO BAR\"]\n+\t\thi = upper-case\n+\t[my \"foo bar\"]\n+\t\thi = lower-case\n+\t[core]\n+\t\tbaz = bat\n+\t\tbaz = hask\n+\t[lamb]\n+\t\tchop = 65\n+\t\thead = none\n+\t[goat]\n+\t\tlegs = 4\n+\t\thead = true\n+\t\tskin = false\n+\t\tnose = 1\n+\t\thorns\n+\tEOF\n+'\n+\n+test_expect_success 'get value for a simple key' '\n+\techo \"very blue\" >expect &&\n+\ttest-config get_value core.penguin >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get value for a key with value as an empty string' '\n+\techo \"\" >expect &&\n+\ttest-config get_value core.my >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'get value for a key with value as NULL' '\n+\techo \"(NULL)\" >expect &&\n+\ttest-config get_value core.foo >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'upper case key' '\n+\techo \"true\" >expect &&\n+\ttest-config get_value core.UPPERCASE >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'mixed case key' '\n+\techo \"true\" >expect &&\n+\ttest-config get_value core.MixedCase >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'key and value with mixed case' '\n+\techo \"BadPhysics\" >expect &&\n+\ttest-config get_value core.Movie >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'key with case sensitive subsection' '\n+\techo \"mixed-case\" >expect &&\n+\techo \"upper-case\" >>expect &&\n+\techo \"lower-case\" >>expect &&\n+\ttest-config get_value \"my.Foo bAr.hi\" >actual &&\n+\ttest-config get_value \"my.FOO BAR.hi\" >>actual &&\n+\ttest-config get_value \"my.foo bar.hi\" >>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'key with case insensitive section header' '\n+\techo \"ball\" >expect &&\n+\techo \"ball\" >>expect &&\n+\techo \"ball\" >>expect &&\n+\ttest-config get_value cores.baz >actual &&\n+\ttest-config get_value Cores.baz >>actual &&\n+\ttest-config get_value CORES.baz >>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value with misspelled key' '\n+\techo \"Value not found for \\\"my.fOo Bar.hi\\\"\" >expect &&\n+\ttest_must_fail test-config get_value \"my.fOo Bar.hi\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value with the highest priority' '\n+\techo hask >expect &&\n+\ttest-config get_value \"core.baz\">actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find integer value for a key' '\n+\techo 65 >expect &&\n+\ttest-config get_int lamb.chop >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find integer if value is non parse-able' '\n+\techo 65 >expect &&\n+\ttest_must_fail test-config get_int lamb.head >actual &&\n+\ttest_must_fail test_cmp expect actual\n+'\n+\n+test_expect_success 'find bool value for the entered key' '\n+\tcat >expect <<-\\EOF &&\n+\t1\n+\t0\n+\t1\n+\t1\n+\t1\n+\tEOF\n+\ttest-config get_bool goat.head >actual &&\n+\ttest-config get_bool goat.skin >>actual &&\n+\ttest-config get_bool goat.nose >>actual &&\n+\ttest-config get_bool goat.horns >>actual &&\n+\ttest-config get_bool goat.legs >>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find multiple values' '\n+\tcat >expect <<-\\EOF &&\n+\tsam\n+\tbat\n+\thask\n+\tEOF\n+\ttest-config get_value_multi \"core.baz\">actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value from a configset' '\n+\tcat >config2 <<-\\EOF &&\n+\t[core]\n+\t\tbaz = lama\n+\t[my]\n+\t\tnew = silk\n+\t[core]\n+\t\tbaz = ball\n+\tEOF\n+\techo silk >expect &&\n+\ttest-config configset_get_value my.new config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value with highest priority from a configset' '\n+\techo hask > expect &&\n+\ttest-config configset_get_value core.baz config2 .git/config  >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value_list for a key from a configset' '\n+\tcat >except <<-\\EOF &&\n+\tsam\n+\tbat\n+\thask\n+\tlama\n+\tball\n+\tEOF\n+\ttest-config configset_get_value core.baz config2 .git/config  >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..45ccd0a\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,127 @@\n+#include \"cache.h\"\n+#include \"hashmap.h\"\n+#include \"string-list.h\"\n+\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+ * Reads stdin and prints result of command to stdout:\n+ *\n+ * get_value -> prints the value with highest priority for the entered key\n+ *\n+ * get_value_multi -> prints all values for the entered key in increasing order\n+ *\t\t     of priority\n+ *\n+ * get_int -> print integer value for the entered key or die\n+ *\n+ * get_bool -> print bool value for the entered key or die\n+ *\n+ * configset_get_value -> returns value with the highest priority for the entered key\n+ * \t\t\tfrom a config_set constructed from files entered as arguments.\n+ *\n+ * configset_get_value_multi -> returns value_list for the entered key sorted in\n+ * \t\t\t\tascending order of priority from a config_set \n+ * \t\t\t\tconstructed from files entered as arguments.\n+ *\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+\n+int main(int argc, char **argv)\n+{\n+\tint i, no_of_files;\n+\tint ret = 0;\n+\tconst char *v;\n+\tint val;\n+\tconst struct string_list *strptr;\n+\tstruct config_set cs;\n+\tgit_configset_init(&cs);\n+\tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n+\t\tif (!git_config_get_value(argv[2], &v)) {\n+\t\t\tif (!v)\n+\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\telse\n+\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n+\t\tstrptr = git_config_get_value_multi(argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tif (!v)\n+\t\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\t}\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n+\t\tif (!git_config_get_int(argv[2], &val)) {\n+\t\t\tprintf(\"%d\\n\", val);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n+\t\tif (!git_config_get_bool(argv[2], &val)) {\n+\t\t\tprintf(\"%d\\n\", val);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n+\t\tfor (i = 3; i < argc; i++) {\n+\t\t\tret = git_configset_add_file(&cs, argv[i]);\n+\t\t\tif (ret)\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n+\t\t\tif (!v)\n+\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\telse\n+\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n+\t\tfor (i = 3; i < argc; i++) {\n+\t\t\tret = git_configset_add_file(&cs, argv[i]);\n+\t\t\tif (ret)\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tif (!v)\n+\t\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\t}\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n+\tfprintf(stderr, \"%s: Please check the syntax and the function name\\n\", argv[0]);\n+\treturn 1;\n+}\n-- \n1.9.0.GIT\n"},{"id":"245440","messageId":"53B92FE7.6010707@ramsay1.demon.co.uk","threadId":"37065","inReplyTo":"1404631162-18556-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v5 1/2] add `config_set` API for caching config-like files","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-06T11:15:51Z","receivedAt":"2014-07-06T11:15:51Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 06/07/14 08:19, Tanay Abhra wrote:\n> Currently `git_config()` uses a callback mechanism and file rereads for\n> config values. Due to this approach, it is not uncommon for the config\n> files to be parsed several times during the run of a git program, with\n> different callbacks picking out different variables useful to themselves.\n> \n> Add a `config_set`, that can be used to construct an in-memory cache for\n> config-like files that the caller specifies (i.e., files like `.gitmodules`,\n> `~/.gitconfig` etc.). Add two external functions `git_configset_get_value`\n> and `git_configset_get_value_multi` for querying from the config sets.\n> `git_configset_get_value` follows `last one wins` semantic (i.e. if there\n> are multiple matches for the queried key in the files of the configset the\n> value returned will be the last entry in `value_list`).\n> `git_configset_get_value_multi` returns a list of values sorted in order of\n> increasing priority (i.e. last match will be at the end of the list). Add\n> type specific query functions like `git_configset_get_bool` and similar.\n> \n> Add a default `config_set`, `the_config_set` to cache all key-value pairs\n> read from usual config files (repo specific .git/config, user wide\n> ~/.gitconfig and the global /etc/gitconfig). `the_config_set` uses a\n> single hashmap populated using `git_config()`.\n> \n> Add two external functions `git_config_get_value` and \n> `git_config_get_value_multi` for querying in a non-callback manner from\n> `the_config_set`. Also, add type specific query functions that are\n> implemented as a thin wrapper around the `config_set` API.\n> \n> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>\n> ---\n>  Documentation/technical/api-config.txt | 134 +++++++++++++++\n>  Makefile                               |   1 +\n>  cache.h                                |  34 ++++\n>  config-hash.c                          | 297 +++++++++++++++++++++++++++++++++\n>  config.c                               |   3 +\n>  5 files changed, 469 insertions(+)\n>  create mode 100644 config-hash.c\n> \n> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n> index 230b3a0..bdf86d0 100644\n> --- a/Documentation/technical/api-config.txt\n> +++ b/Documentation/technical/api-config.txt\n> @@ -77,6 +77,81 @@ 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_value`\n> +and `git_config_get_value_multi`. They both read values from an internal\n> +cache generated previously from reading the config files.\n> +\n> +`int git_config_get_value(const char *key, const char **value)`::\n> +\n> +\tFinds the highest-priority value for the configuration variable `key`,\n> +\tstores the pointer to it in `value` and returns 0. When the\n> +\tconfiguration variable `key` is not found, returns 1 without touching\n> +\t`value`. The caller should not free or modify `value`, as it is owned\n> +\tby the cache.\n> +\n> +`const struct string_list *git_config_get_value_multi(const char *key)`::\n> +\n> +\tFinds and returns the value list, sorted in order of increasing priority\n> +\tfor the configuration variable `key`. When the configuration variable\n> +\t`key` is not found, returns NULL. The caller should not free or modify\n> +\tthe returned pointer, as it is owned by the cache.\n> +\n> +`void git_config_clear(void)`::\n> +\n> +\tResets and invalidate the config cache. \n> +\n> +The config API also provides type specific API functions which do conversion\n> +as well as retrieval for the queried variable, including:\n> +\n> +`int git_config_get_int(const char *key, int *dest)`::\n> +\n> +\tFinds and parses the value to an integer for the configuration variable\n> +\t`key`. Dies on error; otherwise, stores pointer to the parsed integer in\n\n... stores the value of the parsed integer in `dest` ...\n\n> +\t`dest` and returns 0. When the configuration variable `key` is not found,\n> +\treturns 1 without touching `dest`.\n> +\n> +`int git_config_get_ulong(const char *key, unsigned long *dest)`::\n> +\n> +\tSimilar to `git_config_get_int` but for unsigned longs.\n> +\n> +`int git_config_get_int(const char *key, int *dest)`::\n-----------------------^^^\n\ngit_config_get_bool\n\n> +\n> +\tFinds and parses the value into a boolean value, for the configuration\n> +\tvariable `key`respecting keywords like \"true\" and \"false\". Integer\n> +\tvalues are converted into true/false values (when they are non-zero or\n> +\tzero, respectively). Other values cause a die(). If parsing is successful,\n> +\tstores the pointer to the parsed result in `dest` and returns 0. When the\n\n... stores the value of the parsed result in `dest` ...\n\n> +\tconfiguration variable `key` is not found, returns 1 without touching\n> +\t`dest`.\n> +\n> +`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`::\n> +\n> +\tSimilar to `git_config_get_bool`, except that integers are copied as-is,\n> +\tand `is_bool` flag is unset.\n> +\n> +`int git_config_get_maybe_bool(const char *key, int *dest)`::\n> +\n> +\tSimilar to `git_config_get_bool`, except that it returns -1 on error\n> +\trather than dying.\n> +\n> +`int git_config_get_string(const char *key, const char **dest)`::\n> +\n> +\tAllocates and copies the retrieved string into the `dest` parameter for\n> +\tthe configuration variable `key`; if NULL string is given, prints an\n> +\terror message and returns -1. When the configuration variable `key` is\n> +\tnot found, returns 1 without touching `dest`.\n> +\n> +`int git_config_get_pathname(const char *key, const char **dest)`::\n> +\n> +\tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n> +\tthe user's home directory when found at the beginning of the path.\n> +\n> +See test-config.c for usage examples.\n> +\n\nSorry, I didn't have time to do more than squint at the code, but at first\nglance it looks good.\n\nATB,\nRamsay Jones\n"},{"id":"245447","messageId":"vpqa98m8jq2.fsf@anie.imag.fr","threadId":"37065","inReplyTo":"1404631162-18556-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v5 1/2] add `config_set` API for caching config-like files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-06T17:45:09Z","receivedAt":"2014-07-06T17:45:09Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> Add a default `config_set`, `the_config_set` to cache all key-value pairs\n> read from usual config files (repo specific .git/config, user wide\n> ~/.gitconfig and the global /etc/gitconfig). `the_config_set` uses a\n> single hashmap populated using `git_config()`.\n\n\"uses a single hashmap\" is no longer relevant. Any config_set use a\nsingle hashmap.\n\n(The \"populated using git_config()\" part is still relevant OTOH)\n\n> +`void git_configset_init(struct config_set *cs)`::\n> +\n> +\tInitializes the member variables of config_set `cs`.\n\nI'd say just \"initializes the config_set `cs`\".\n\n> +/*\n> + * Default config_set that contains key-value pairs from the usual set of config\n> + * config files (i.e repo specific .git/config, user wide ~/.gitconfig and the\n> + * global /etc/gitconfig)\n\nTo be technically correct, \"user wide ~/.gitconfig, XDG config file and\nthe global ...\".\n\n> +static int add_configset_hash(const char *filename, struct config_set *cs)\n> +{\n> +\tint ret = 0;\n> +\tif (!cs->hash_initialized)\n> +\t\tconfig_hash_init(&cs->config_hash);\n> +\tcs->hash_initialized = 1;\n\nThat would seem more natural to me to have a function config_set_init\ninstead of config_hash_init, that would set hash_initialized.\n\nconfig_hash_init is currently a one-liner, so there's no risk of making\nit too complex.\n\n> +static int config_hash_add_value(const char *key, const char *value, struct hashmap *config_hash)\n> +{\n> +\tstruct config_hash_entry *e;\n> +\te = config_hash_find_entry(key, config_hash);\n> +\n> +\tif (!e) {\n> +\t\te = xmalloc(sizeof(*e));\n> +\t\thashmap_entry_init(e, strhash(key));\n\nNitpick: you're hashing the same string twice. Once for\nconfig_hash_find_entry, and another here. It would be slightly faster to\nallow config_hash_find_entry to return the hashcode (as a by-address\nparameter).\n\nBut you may want to keep the code as it is for simplicity.\n\nIt took me some time to understand why you normalize in\nconfig_hash_find_entry and not here. My understanding is that\nconfig_hash_find_entry is used to query the hashmap from the user API\n(so you need normalization), but this particular codepath receives\nnormalized keys (the comment right below states that for values).\n\nIt's probably worth a comment here and/or in config_hash_find_entry.\n\n> +\t}\n> +\t/*\n> +\t * Since the values are being fed by git_config*() callback mechanism, they\n> +\t * are already normalized. So simply add them without any further munging.\n> +\t */\n> +\tstring_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n> +\n> +\treturn 0;\n> +}\n\n[...]\n\n> +int git_configset_add_file(struct config_set *cs, const char *filename)\n> +{\n> +\treturn add_configset_hash(filename, cs);\n> +}\n\nWhy have two functions doing the same, with arguments in different\norders?\n\nI guess it's just historical, and you can keep only one.\n\n> +void git_configset_clear(struct config_set *cs)\n> +{\n> +\tstruct config_hash_entry *entry;\n> +\tstruct hashmap_iter iter;\n> +\tif (!cs->hash_initialized)\n> +\t\treturn;\n> +\n> +\thashmap_iter_init( &cs->config_hash, &iter);\n\nNo space after (.\n\n> +int git_config_get_string(const char *key, const char **dest)\n> +{\n> +\tgit_config_check_init();\n> +\treturn git_configset_get_string(&the_config_set, key, dest);\n> +}\n> +\n> +int git_config_get_int(const char *key, int *dest)\n> +{\n> +\tgit_config_check_init();\n> +\treturn git_configset_get_int(&the_config_set, key, dest);\n> +}\n\nReading this list, I initially thought it might be worth factoring this\nin a macro. But the list isn't that long, and contains several\nspecial-cases (3 parameters instead of 2). So, don't bother.\n\nOK, we're getting close. v6 should be really good :-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245448","messageId":"vpqzjgm49s8.fsf@anie.imag.fr","threadId":"37065","inReplyTo":"1404631162-18556-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v5 2/2] test-config: Add tests for the config_set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-06T18:33:27Z","receivedAt":"2014-07-06T18:33:27Z","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> +test_expect_success 'get value for a simple key' '\n> +\techo \"very blue\" >expect &&\n> +\ttest-config get_value core.penguin >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nAll these tests would greatly benefit from a helper like\n\ntest_expect_config () {\n\techo \"1\" >expect &&\n\ttest-config get_value \"$2\" >actual &&\n\ttest_cmp expect actual\n}\n\nThen, all the 3-liners below would become 1-liners.\n\nShould not block inclusion, but may be worth considering.\n\n> +test_expect_success 'get value for a key with value as an empty string' '\n> +\techo \"\" >expect &&\n> +\ttest-config get_value core.my >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'get value for a key with value as NULL' '\n> +\techo \"(NULL)\" >expect &&\n> +\ttest-config get_value core.foo >actual &&\n> +\ttest_cmp expect actual\n> +'\n[...]\n\n> +test_expect_success 'key with case sensitive subsection' '\n> +\techo \"mixed-case\" >expect &&\n> +\techo \"upper-case\" >>expect &&\n> +\techo \"lower-case\" >>expect &&\n> +\ttest-config get_value \"my.Foo bAr.hi\" >actual &&\n> +\ttest-config get_value \"my.FOO BAR.hi\" >>actual &&\n> +\ttest-config get_value \"my.foo bar.hi\" >>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis would become a 3-liner with my helper.\n\n> +test_expect_success 'key with case insensitive section header' '\n> +\techo \"ball\" >expect &&\n> +\techo \"ball\" >>expect &&\n> +\techo \"ball\" >>expect &&\n> +\ttest-config get_value cores.baz >actual &&\n> +\ttest-config get_value Cores.baz >>actual &&\n> +\ttest-config get_value CORES.baz >>actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI think you miss a simple case: get_value with a case that doesn't exist\nin the config file, like \"get_value coreS.baz\".\n\n> +test_expect_success 'find value with the highest priority' '\n> +\techo hask >expect &&\n> +\ttest-config get_value \"core.baz\">actual &&\n\nSpace before >.\n\n> diff --git a/test-config.c b/test-config.c\n\nNo time for a real review of this file, but from a quick look, it seems\nOK.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245449","messageId":"vpqk37q2rc8.fsf@anie.imag.fr","threadId":"37065","inReplyTo":"1404631162-18556-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v5 2/2] test-config: Add tests for the config_set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-06T19:57:11Z","receivedAt":"2014-07-06T19:57:11Z","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> diff --git a/test-config.c b/test-config.c\n> new file mode 100644\n> index 0000000..45ccd0a\n> --- /dev/null\n> +++ b/test-config.c\n> @@ -0,0 +1,127 @@\n> +#include \"cache.h\"\n> +#include \"hashmap.h\"\n\nUseless include, you're not using the hashmap directly.\n\n> +int main(int argc, char **argv)\n> +{\n> +\tint i, no_of_files;\n\nUnused no_of_files.\n\nWith\n\nCFLAGS += -Wdeclaration-after-statement -Wall -Werror\n\nin config.mak, my git.git refuses to compile with your patch. You should\nhave similar options for hacking on git.git.\n\n> +\tif (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n\nYou should do something like\n\nif (argc < 2) {\n\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n\treturn 1;\n}\n\nbefore this. Otherwise, some argv[1] below are invalid on mis-use. No\nneed for thorough checks since it's just a test program, but better\navoid undefined behavior and segfaults anyway...\n\n> +\t\tif (!git_config_get_value(argv[2], &v)) {\n> +\t\t\tif (!v)\n> +\t\t\t\tprintf(\"(NULL)\\n\");\n> +\t\t\telse\n> +\t\t\t\tprintf(\"%s\\n\", v);\n> +\t\t\treturn 0;\n> +\t\t} else {\n> +\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n> +\t\t\treturn -1;\n\nAvoid returning negative values from main. Your shell's $? won't see -1,\nbut most likely 255 or so, but I think it even depends on the OS.\n\nYou don't seem to use main's return for anything but error, so 0 =\neverything OK; 1 = some error is the common convention.\n\n> +\t\t} else {\n> +\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n\nWhy a blank line before this \"else if\" and not before other \"else if\"s?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245451","messageId":"CALkWK0n9MWux3tA02n-hDvPjXt3Q1RCVd+x8tJV9ABjndaGQCQ@mail.gmail.com","threadId":"37065","inReplyTo":"1404631162-18556-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v5 2/2] test-config: Add tests for the config_set API","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2014-07-06T23:24:26Z","receivedAt":"2014-07-06T23:24:26Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"A couple of quick nits.\n\nTanay Abhra wrote:\n> +test_expect_success 'clear default config' '\n> +       rm -f .git/config\n> +'\n\nUnnecessary; a fresh temporary directory is created for each test run.\n\n> +test_expect_success 'initialize default config' '\n\nYou might want to mark this as \"setup\".\n"},{"id":"245465","messageId":"vpqvbr91w46.fsf@anie.imag.fr","threadId":"37065","inReplyTo":"CALkWK0n9MWux3tA02n-hDvPjXt3Q1RCVd+x8tJV9ABjndaGQCQ@mail.gmail.com","subject":"Re: [PATCH v5 2/2] test-config: Add tests for the config_set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-07T07:11:37Z","receivedAt":"2014-07-07T07:11:37Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> A couple of quick nits.\n>\n> Tanay Abhra wrote:\n>> +test_expect_success 'clear default config' '\n>> +       rm -f .git/config\n>> +'\n>\n> Unnecessary; a fresh temporary directory is created for each test run.\n\nHmm, fresh, but not empty.\n\nAnyway, the next test does a cat > on this file, so it will erase its\ncontent, so the \"rm -f\" is actually not needed.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}