{"thread":{"id":"37037","subject":"[PATCH v4 0/3] git config cache & special querying api utilizing the cache","startedAt":"2014-07-02T06:01:43Z","lastAt":"2014-07-07T17:02:19Z","messageCount":17,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":3},"messages":[{"id":"245281","messageId":"1404280905-26763-1-git-send-email-tanayabh@gmail.com","threadId":"37037","inReplyTo":null,"subject":"[PATCH v4 0/3] git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-02T06:01:43Z","receivedAt":"2014-07-02T06:01:43Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\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\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\nTanay Abhra (2):\n config-hash.c\n test-config\n\n .gitignore | 1 +\n Documentation/technical/api-config.txt | 144 ++++++++++++\n Makefile | 2 +\n cache.h | 41 ++++\n config-hash.c | 405 +++++++++++++++++++++++++++++++++\n config.c | 3 +\n t/t1308-config-hash.sh | 163 +++++++++++++\n test-config.c | 129 +++++++++++\n 8 files changed, 888 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":"245282","messageId":"1404280905-26763-2-git-send-email-tanayabh@gmail.com","threadId":"37037","inReplyTo":"1404280905-26763-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-02T06:01:44Z","receivedAt":"2014-07-02T06:01:44Z","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` construct that points to an ordered set of config files\nspecified by the user, each of which represents what was read and cached\nin core as hashmaps. Add two external functions `git_configset_get_value`\nand `git_configset_get_value_multi` for querying from the config sets,\nwhich follow `last one wins` semantic(i.e. if there are multiple matches\nfor the queried key in the files of the configset the value returned will\nthe last value in the last matching file of the configset) Add type\nspecific 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(). The reason for doing so is\ntwofold, one is to honour include directives, another is to guarantee O(1)\nlookups for usual config values, as values are queried for hundred of\ntimes during a run of a git program.\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 \n---\n Documentation/technical/api-config.txt | 144 ++++++++++++\n Makefile | 1 +\n cache.h | 41 ++++\n config-hash.c | 405 +++++++++++++++++++++++++++++++++\n config.c | 3 +\n 5 files changed, 594 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..2c02fee 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,75 @@ 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+`git_config_get_value` 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. The caller\n+ should not free or modify the value returned as it is owned by the cache.\n+\n+`git_config_get_value` returns 0 on success, or -1 for no value found.\n+\n+`git_config_get_value_multi` allocates and returns a `string_list`\n+containing all the values for the key passed as parameter, sorted in order\n+of increasing priority (Note: caller has to call `string_list_clear` on\n+the returned pointer and then free it).\n+\n+`git_config_get_value_multi` returns NULL for no value found.\n+\n+`git_config_clear` is provided for resetting and invalidating the cache.\n+\n+`git_config_iter` gives a way to iterate over the keys in cache. Existing\n+`git_config` callback function signature is used for iterating.\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+`git_config_get_int`::\n+Parse the retrieved value to an integer, including unit factors. Dies on\n+error; otherwise, allocates and copies the integer into the dest parameter.\n+Returns 0 on success, or 1 if no value was found.\n+\n+`git_config_get_ulong`::\n+Identical to `git_config_get_int` but for unsigned longs.\n+\n+`git_config_bool`::\n+Parse the retrieved value into a boolean value, respecting keywords like\n+\"true\" and \"false\". Integer values are converted into true/false values\n+(when they are non-zero or zero, respectively). Other values cause a die().\n+If parsing is successful, allocates and copies the result into the dest\n+parameter. Returns 0 on success, or 1 if no value is found.\n+\n+`git_config_get_bool_or_int`::\n+Same as `git_config_get_bool`, except that integers are copied as-is, and\n+an `is_bool` flag is unset.\n+\n+`git_config_get_maybe_bool`::\n+Same as `git_config_get_bool`, except that it returns -1 on error rather\n+than dying.\n+\n+`git_config_get_string`::\n+Allocates and copies the retrieved value string into the `dest` parameter;\n+if NULL string is given, prints an error message and returns -1. Returns 1,\n+if no value is found for the queried variable .\n+\n+`git_config_get_pathname`::\n+Similar to `git_config_get_string`, but expands `~` or `~user` into the\n+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 +203,81 @@ 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` points at an ordered set of config files, each of\n+which represents what was read and cached in-core from a file.\n+It can be used to construct an in-memory cache for config files that\n+the caller specifies. For example,\n+\n+---------------------------------------\n+struct config_set gm_config = CONFIG_SET_INIT;\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+/* 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+`git_configset_init`::\n+\n+Returns an allocated and initialized struct `config_set` pointer.\n+\n+`git_configset_add_file`::\n+\n+Parses the file and constructs an in core cache which is appended at the end of\n+the config_set, dies if there is an error in parsing the file.\n+\n+`git_configset_get_value`::\n+\n+Follows \"last one wins\" semantic, i.e., if there are multiple matches for the\n+queried key in the files of the configset the value returned will the last value\n+in the last matching file of the configset.\n+\n+It takes three parameters,\n+\n+- the `config_set` from which the value will be queried.\n+\n+- a key string in canonical flat form\n+\n+- a pointer to a string which will point to the retrieved value. The caller\n+ should not free or modify the value returned as it is owned by the cache.\n+\n+`git_configset_get_value` returns 0 on success, or -1 for no value found.\n+\n+`git_configset_get_value_multi`::\n+\n+`git_configset_get_value_multi` allocates and returns a `string_list`\n+containing all the values for the key passed as parameter, sorted in order\n+of increasing priority (Note: caller has to call `string_list_clear` on\n+the returned pointer and then free it).\n+\n+`git_configset_get_value_multi` returns NULL for no value found.\n+\n+'git_configset_iter'::\n+\n+It gives a way to iterate over the keys in config_set.\n+Existing `git_config` callback function signature is used for iterating.\n+\n+`git_configset_clear`::\n+It is used to clear the config_set after use. If the `config_set` was dynamically\n+allocated using `git_configset_init`, the caller has to free the `config_set`\n+after calling `git_configset_clear`.\n+\n+In addition to above the config_set API provides type specific functions in the\n+vein of `git_config_get_int` but with an extra parameter, `config_set`. They\n+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..005e945 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -7,6 +7,7 @@\n #include \"advice.h\"\n #include \"gettext.h\"\n #include \"convert.h\"\n+#include \"string-list.h\"\n \n #include SHA1_HEADER\n #ifndef git_SHA_CTX\n@@ -1325,6 +1326,46 @@ 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+#define CONFIG_SET_INIT { NULL, 0, 0 }\n+\n+struct config_set {\n+\tstruct config_set_item *item;\n+\tunsigned int nr, alloc;\n+};\n+\n+struct config_set_item {\n+\tstruct hashmap config_hash;\n+\tchar *filename;\n+};\n+\n+extern struct config_set *git_configset_init(void);\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..3e45d03\n--- /dev/null\n+++ b/config-hash.c\n@@ -0,0 +1,405 @@\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+static int the_config_set_initialised = 0;\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 *,\n+\t\t\t\t struct config_set_item *, struct config_set *);\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+struct config_set_cb {\n+\tstruct config_set *cs;\n+\tstruct config_set_item *ct;\n+};\n+\n+static int config_hash_callback(const char *key, const char *value, void *cb)\n+{\n+\tstruct config_set_cb *c = cb;\n+\tconfig_hash_add_value(key, value, c->ct, c->cs);\n+\treturn 0;\n+}\n+\n+static struct hashmap *add_config_hash(const char *filename, struct config_set *cs)\n+{\n+\tint ret = 0;\n+\tstruct config_set_item *ct;\n+\tstruct config_set_cb cb;\n+\tALLOC_GROW(cs->item, cs->nr + 1, cs->alloc);\n+\tct = &cs->item[cs->nr++];\n+\tct->filename = xstrdup(filename);\n+\tconfig_hash_init(&ct->config_hash);\n+\tcb.cs = cs;\n+\tcb.ct = ct;\n+\t/*\n+\t * When filename is \"default\", hashmap is constructed from the usual set of\n+\t * config files (i.e repo specific .git/config, user wide ~/.gitconfig and the\n+\t * global /etc/gitconfig), used in `the_config_set`\n+\t */\n+\tif (!strcmp(filename, \"default\"))\n+\t\tgit_config(config_hash_callback, &cb);\n+\telse\n+\t\tret = git_config_from_file(config_hash_callback, filename, &cb);\n+\tif (!ret)\n+\t\treturn &ct->config_hash;\n+\telse {\n+\t\tfree(ct->filename);\n+\t\thashmap_free(&ct->config_hash, 1);\n+\t\tcs->nr--;\n+\t\treturn NULL;\n+\t}\n+}\n+\n+static struct hashmap *get_config_hash(const char *filename, struct config_set *cs)\n+{\n+\tint i;\n+\tfor(i = 0; i < cs->nr; i++) {\n+\t\tif (!strcmp(cs->item[i].filename, filename))\n+\t\t\treturn &cs->item[i].config_hash;\n+\t}\n+\treturn add_config_hash(filename, cs);\n+}\n+\n+static struct config_hash_entry *config_hash_find_entry(const char *key, const char* filename,\n+\t\t\t\t\t\t\tstruct config_set *cs)\n+{\n+\tstruct hashmap *config_hash;\n+\tstruct config_hash_entry k;\n+\tstruct config_hash_entry *found_entry;\n+\tchar *normalized_key;\n+\tint ret;\n+\tconfig_hash = get_config_hash(filename, cs);\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 *config_hash_get_value(const char *key, const char* filename, \n+\t\t\t\t\t\t struct config_set *cs)\n+{\n+\tstruct config_hash_entry *e = config_hash_find_entry(key, filename, cs);\n+\treturn e ? &e->value_list : NULL;\n+}\n+\n+static int config_hash_add_value(const char *key, const char *value,\n+\t\t\t\t struct config_set_item *ct, struct config_set *cs)\n+{\n+\tstruct hashmap *config_hash = &ct->config_hash;\n+\tstruct config_hash_entry *e;\n+\te = config_hash_find_entry(key, ct->filename, cs);\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 normalised. 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+struct config_set *git_configset_init(void)\n+{\n+\tstruct config_set *cs = xcalloc(1, sizeof(*cs));\n+\treturn cs;\n+}\n+\n+int git_configset_add_file(struct config_set *cs, const char *filename)\n+{\n+\treturn get_config_hash(filename, cs) ? 0 : -1;\n+}\n+\n+int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n+{\n+\tint i;\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 the last value\n+\t * in the last matching file of the configset\n+\t */\n+\tassert(cs->nr > 0);\n+\tfor (i = cs->nr-1; i >= 0; i--) {\n+\t\tvalues = config_hash_get_value(key, cs->item[i].filename, cs);\n+\t\tif (values)\n+\t\t\tbreak;\n+\t}\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+\tint i, j;\n+\tstruct string_list *values = NULL;\n+\tstruct string_list *retval = xcalloc(1, sizeof(*retval));\n+\tfor (i = 0; i < cs->nr; i++) {\n+\t\tvalues = config_hash_get_value(key, cs->item[i].filename, cs);\n+\t\tif (values)\n+\t\t\tfor(j = 0; j < values->nr; j++)\n+\t\t\t\tstring_list_append_nodup(retval, values->items[j].string);\n+\t}\n+\tif (!retval->nr) {\n+\t\tfree(retval);\n+\t\treturn NULL;\n+\t}\n+\treturn retval;\n+}\n+\n+/*\n+ * Uses the existing git_config() callback function signature to iterate over\n+ * keys in the configset.\n+ */\n+void git_configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n+{\n+\tint i;\n+\tchar *value;\n+\tstruct string_list *strptr;\n+\tstruct config_hash_entry *entry;\n+\tstruct hashmap *config_hash;\n+\tstruct hashmap_iter iter;\n+\tfor (i = 0; i < cs->nr; i++) {\n+\t\tconfig_hash = &cs->item[i].config_hash;\n+\t\thashmap_iter_init(config_hash, &iter);\n+\t\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\t\tstrptr = &entry->value_list;\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tvalue = strptr->items[i].string;\n+\t\t\t\tfn(entry->key, value, data);\n+\t\t\t}\n+\t\t}\n+\t}\n+}\n+\n+void git_configset_clear(struct config_set *cs)\n+{\n+\tint i;\n+\tstruct hashmap *config_hash;\n+\tstruct config_hash_entry *entry;\n+\tstruct hashmap_iter iter;\n+\t/* No files in config_set so return */\n+\tif (!cs->nr)\n+\t\treturn;\n+\tfor (i = 0; i < cs->nr; i++) {\n+\t\tconfig_hash = &cs->item[i].config_hash;\n+\t\tfree(cs->item[i].filename);\n+\t\thashmap_iter_init(config_hash, &iter);\n+\t\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\t\tfree(entry->key);\n+\t\t\tstring_list_clear(&entry->value_list, 0);\n+\t\t}\n+\t\thashmap_free(config_hash, 1);\n+\t}\n+\tfree(cs->item);\n+\tcs->item = NULL;\n+\tcs->nr = cs->alloc = 0;\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+int git_config_get_value(const char *key, const char **value)\n+{\n+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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_initialised)\n+\t\treturn;\n+\tgit_configset_clear(&the_config_set);\n+\tthe_config_set_initialised = 0;\n+}\n+\n+void git_config_iter(config_fn_t fn, void *data)\n+{\n+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\n+\tgit_configset_iter(&the_config_set, fn, data);\n+}\n+\n+int git_config_get_string(const char *key, const char **dest)\n+{\n+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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+\tif (!the_config_set_initialised) {\n+\t\tgit_configset_add_file(&the_config_set, \"default\");\n+\t\tthe_config_set_initialised = 1;\n+\t}\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":"245283","messageId":"1404280905-26763-3-git-send-email-tanayabh@gmail.com","threadId":"37037","inReplyTo":"1404280905-26763-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH 2/2] test-config: Add tests for the config_set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-02T06:01:45Z","receivedAt":"2014-07-02T06:01:45Z","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 \n---\n .gitignore | 1 +\n Makefile | 1 +\n t/t1308-config-hash.sh | 163 +++++++++++++++++++++++++++++++++++++++++++++++++\n test-config.c | 129 ++++++++++++++++++++++++++++++++++++++\n 4 files changed, 294 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..6c059c5\n--- /dev/null\n+++ b/t/t1308-config-hash.sh\n@@ -0,0 +1,163 @@\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+cat > .git/config << EOF\n+[core]\n+\tpenguin = very blue\n+\tMovie = BadPhysics\n+\tUPPERCASE = true\n+\tMixedCase = true\n+\tmy =\n+\tfoo\n+\tbaz = sam\n+[Cores]\n+\tWhatEver = Second\n+[my \"Foo bAr\"]\n+\thi = hello\n+[core]\n+\tbaz = bat\n+\tbaz = hask\n+[lamb]\n+\tchop = 65\n+\thead = none\n+[goat]\n+\tlegs = 4\n+\thead = true\n+\tskin = false\n+\tnose = 1\n+\thorns\n+EOF\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+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+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+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 \"hello\" >expect &&\n+\ttest-config get_value \"my.Foo bAr.hi\">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+cat > expect << EOF\n+1\n+0\n+1\n+1\n+1\n+EOF\n+\n+test_expect_success 'find bool value for the entered key' '\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+cat > expect << EOF\n+sam\n+bat\n+hask\n+EOF\n+\n+test_expect_success 'find multiple values' '\n+\ttest-config get_value_multi \"core.baz\">actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat > config2 << EOF\n+[core]\n+\tbaz = lama\n+[my]\n+\tnew = silk\n+[core]\n+\tbaz = ball\n+EOF\n+\n+test_expect_success 'find value from a configset' '\n+\techo silk > expect &&\n+\ttest-config configset_get_value 2 config2 .git/config my.new >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 2 config2 .git/config core.baz >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat > except << EOF\n+sam\n+bat\n+hask\n+lama\n+ball\n+EOF\n+\n+test_expect_success 'find value_list for a key from a configset' '\n+\ttest-config configset_get_value 2 config2 .git/config core.baz >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..32c912a\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,129 @@\n+#include \"cache.h\"\n+#include \"hashmap.h\"\n+#include \"string-list.h\"\n+\n+/*\n+ * This program exposes the C API of the `config_set` 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 -> first makes a config-set of files entered as arguments\n+ *\t\t\t and then returns value with the highest priority for the\n+ *\t\t\t entered key.\n+ *\n+ * configset_get_value_multi -> first makes a config-set of files entered as\n+ * \t\t\t\targuments and then returns value_list for the entered\n+ *\t\t\t\tkey sorted in ascending order of priority.\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 = CONFIG_SET_INIT;\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\tno_of_files = git_config_int(\"unused\", argv[2]);\n+\t\tfor (i = 1; i <= no_of_files; i++) {\n+\t\t\tret = git_configset_add_file(&cs, argv[i + 2]);\n+\t\t\tif (ret)\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tif (!git_configset_get_value(&cs, argv[3 + no_of_files], &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\tno_of_files = git_config_int(\"unused\", argv[2]);\n+\t\tfor (i = 1; i <= no_of_files; i++) {\n+\t\t\tret = git_configset_add_file(&cs, argv[i + 2]);\n+\t\t\tif (ret)\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tstrptr = git_configset_get_value_multi(&cs, argv[3 + no_of_files]);\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":"245286","messageId":"vpqoax8m8bh.fsf@anie.imag.fr","threadId":"37037","inReplyTo":"1404280905-26763-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-02T09:14:10Z","receivedAt":"2014-07-02T09:14:10Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> Add a `config_set` construct that points to an ordered set of config files\n> specified by the user, each of which represents what was read and cached\n> in core as hashmaps. Add two external functions `git_configset_get_value`\n> and `git_configset_get_value_multi` for querying from the config sets,\n> which follow `last one wins` semantic(i.e. if there are multiple matches\n\nSpace before (\n\n> for the queried key in the files of the configset the value returned will\n> the last value in the last matching file of the configset) Add type\n\nwill _be_ ?\n\nDoes this remark also apply to git_configset_get_value_multi? My\nunderstanding was that \"last one wins\" applied only to\ngit_configset_get_value.\n\n> Add a default `config_set`, `the_config_set` to cache all key-value pairs\n\nI don't like the name \"the_config_set\". It's not the only one. Perhaps\nrepo_config_set? (not totally satisfactory as it does not contain only\nthe repo, but the repo+user+system)\n\nWhat do others think?\n\n> read from usual config files(repo specific .git/config, user wide\n\n(You always forget space before '('...)\n\n> ~/.gitconfig and the global /etc/gitconfig). `the_config_set` uses a\n> single hashmap populated using git_config(). The reason for doing so is\n> twofold, one is to honour include directives, another is to guarantee O(1)\n> lookups for usual config values, as values are queried for hundred of\n> times during a run of a git program.\n\nWhat is the reason to deal with `the_config_set` and other config sets\ndifferently? You're giving arguments to store `the_config_set` as a\nsingle hashmap, but what is the reason to store others as multiple\nhashmaps?\n\nAnd actually, I don't completely buy your arguments: having 3 or 4\nhashmaps (.git/config, ~/.gitconfig, ~/.config/git/config and\n/etc/gitconfig) would be a O(4) lookup, which is still O(1), and you\ncould deal with include directives by having \".git/config and included\nfiles\" in a hashmap, \"~/.gitconfig and included files\" in a second\nhashmap, ...\n\nMy guess is that the real argument is \"git_config already does what I\nwant and I'm too lazy to change it\". And I do consider lazyness as a\nquality for a computer-scientist ;-).\n\nI would personally find it much simpler to have a single hashmap. We'd\nlose the ability to invalidate the cache for only a single file, but I'm\nnot sure it's worth the code complexity.\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\nSpace after .\n\n> +`git_config_iter` gives a way to iterate over the keys in cache. Existing\n> +`git_config` callback function signature is used for iterating.\n\n\"Existing\" refers to the old state. It's OK in a commit message, but\nwon't make sense in the future, when your non-callback API and the old\ncallback API will live side by side.\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> +`git_config_get_int`::\n> +Parse the retrieved value to an integer, including unit factors. Dies on\n> +error; otherwise, allocates and copies the integer into the dest parameter.\n> +Returns 0 on success, or 1 if no value was found.\n\nIt seems strange to me to return 1 here, and -1 in git_config_get_value\nto mean the same thing.\n\n> +`git_config_bool`::\n\nIsn't it git_config_get_bool?\n\n> +/* config-hash.c */\n\nYou may point to the documentation in the comment too.\n\n> +#define CONFIG_SET_INIT { NULL, 0, 0 }\n> +\n> +struct config_set {\n> +\tstruct config_set_item *item;\n> +\tunsigned int nr, alloc;\n> +};\n> +\n> +struct config_set_item {\n> +\tstruct hashmap config_hash;\n> +\tchar *filename;\n\nPerhaps const char *?\n\n> +static struct hashmap *add_config_hash(const char *filename, struct config_set *cs)\n> +{\n> +\tint ret = 0;\n> +\tstruct config_set_item *ct;\n> +\tstruct config_set_cb cb;\n> +\tALLOC_GROW(cs->item, cs->nr + 1, cs->alloc);\n> +\tct = &cs->item[cs->nr++];\n> +\tct->filename = xstrdup(filename);\n> +\tconfig_hash_init(&ct->config_hash);\n> +\tcb.cs = cs;\n> +\tcb.ct = ct;\n> +\t/*\n> +\t * When filename is \"default\", hashmap is constructed from the usual set of\n> +\t * config files (i.e repo specific .git/config, user wide ~/.gitconfig and the\n> +\t * global /etc/gitconfig), used in `the_config_set`\n> +\t */\n> +\tif (!strcmp(filename, \"default\"))\n\nHow does one read a file actually called \"default\" with this API?\n\nMore generally, why do you need a special-case here? I think it would be\nmuch better to leave this part unaware of the default, and have a\nseparate function like create_repo_config_hash (or\ncreate_the_config_hash) that would call git_config(...). There actually\nisn't much shared code between the \"default\" case and the others in your\nfunction.\n\n> +static struct hashmap *get_config_hash(const char *filename, struct config_set *cs)\n> +{\n> +\tint i;\n> +\tfor(i = 0; i < cs->nr; i++) {\n> +\t\tif (!strcmp(cs->item[i].filename, filename))\n> +\t\t\treturn &cs->item[i].config_hash;\n> +\t}\n> +\treturn add_config_hash(filename, cs);\n> +}\n> +\n> +static struct config_hash_entry *config_hash_find_entry(const char *key, const char* filename,\n> +\t\t\t\t\t\t\tstruct config_set *cs)\n\nI don't get the logic here.\n\nEither the caller explicitly manages a config_set variable like\n\n config_set my_set = git_configset_init();\n git_configset_add_file(my_set, \"my-file\");\n git_configset_get_value(my_set, \"some.variable.name\");\n\nOr there's an implicit singleton mapping files to hashmaps to allow the\nuser to say\n\n git_configset_get_value(\"my-file\", \"some.variable.name\");\n\nwithout asking the user to explicitly manipulate config_set variables.\n\nIt seems to me that your solution is a bad compromise between both\nsolutions: you do require the user to manipulate config_set variables,\nbut you also require a filename to look for the right entry in the list.\n\nDid you miss the end of Junio's message:\n\n http://thread.gmane.org/gmane.comp.version-control.git/252567\n\n?\n\nIf the user explicitly asks for a single entry in the list, then why\ngroup them in a list? I guess the question is the same as above, and the\nmore I read the patch, the more I think a config_set should contain a\nsingle hashmap.\n\n> +static int config_hash_add_value(const char *key, const char *value,\n> +\t\t\t\t struct config_set_item *ct, struct config_set *cs)\n> +{\n> +\tstruct hashmap *config_hash = &ct->config_hash;\n> +\tstruct config_hash_entry *e;\n> +\te = config_hash_find_entry(key, ct->filename, cs);\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 normalised.\n\nWe usually speak en_US rather than en_UK => normalized.\n\n> +int git_config_get_value(const char *key, const char **value)\n> +{\n> +\tif (!the_config_set_initialised) {\n> +\t\tgit_configset_add_file(&the_config_set, \"default\");\n> +\t\tthe_config_set_initialised = 1;\n> +\t}\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> +\tif (!the_config_set_initialised) {\n> +\t\tgit_configset_add_file(&the_config_set, \"default\");\n> +\t\tthe_config_set_initialised = 1;\n> +\t}\n> +\treturn git_configset_get_value_multi(&the_config_set, key);\n> +}\n\nThese if statements could be refactored, and the_config_set_initialised\ncould be hidden to the caller. Just make a function get_the_config_set()\nthat initializes if needed and returns a pointer to the_config_set.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245287","messageId":"vpqzjgskt1v.fsf@anie.imag.fr","threadId":"37037","inReplyTo":"1404280905-26763-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH 2/2] test-config: Add tests for the config_set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-02T09:29:16Z","receivedAt":"2014-07-02T09:29:16Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> +test_expect_success 'clear default config' '\n> +\trm -f .git/config\n> +'\n> +\n> +cat > .git/config << EOF\n\nt/README says:\n\n - Put all code inside test_expect_success and other assertions.\n\n Even code that isn't a test per se, but merely some setup code\n should be inside a test assertion.\n\nEven these cat > would better be in a test_expect_success 'initialize\nconfig'.\n\n(Not applied everywhere in Git's code essentially because some tests\nwere written before the guideline was set and never updated).\n\n> +[core]\n> +\tpenguin = very blue\n> +\tMovie = BadPhysics\n> +\tUPPERCASE = true\n> +\tMixedCase = true\n> +\tmy =\n> +\tfoo\n> +\tbaz = sam\n> +[Cores]\n> +\tWhatEver = Second\n> +[my \"Foo bAr\"]\n> +\thi = hello\n\nTo really stress the \"case sensitive middle part\" case, you should also\nhave other sections like\n\n[my \"foo bar\"]\n\thi = lower-case\n[my \"FOO BAR\"]\n\thi = upper-case\n\nand check that you get the right value for my.*.hi\n\nSimilarly, I'd add a [CORE] and a [CoRe] section to check that their\ncontent is actually merged with [core].\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> +test_expect_success 'upper case key' '\n\nKeep the style consistent, if you separate tests with a single blank\nline, do it everywhere.\n\n> +cat > expect << EOF\n\nSee above, should be in test_expect_success.\n\nAlso, >expect, not > expect.\n\nThere are other instances.\n\n> +1\n> +0\n> +1\n> +1\n> +1\n> +EOF\n> +\n> +test_expect_success 'find bool value for the entered key' '\n> +\ttest-config get_bool goat.head >>actual &&\n\nThe first one should be a single >, or you should clear actual before\nthe test.\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 = CONFIG_SET_INIT;\n\n\n\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\tno_of_files = git_config_int(\"unused\", argv[2]);\n\nWhy ask the user to give a number of files on the command-line. With a\nsyntax like\n\ntest-config configset_get_value ...\n\nyou could just use argc to iterate over argv. Here, you trust the user\nto provide the right value, and most likely segfault otherwise (and this\nis not really documented). I know this is only test code, but why not do\nit right anyway ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245289","messageId":"53B3F403.6030501@gmail.com","threadId":"37037","inReplyTo":"vpqoax8m8bh.fsf@anie.imag.fr","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-02T11:58:59Z","receivedAt":"2014-07-02T11:58:59Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/2/2014 2:44 PM, Matthieu Moy wrote:\n> Tanay Abhra writes:\n> \n>> Add a `config_set` construct that points to an ordered set of config files\n>> specified by the user, each of which represents what was read and cached\n>> in core as hashmaps. Add two external functions `git_configset_get_value`\n>> and `git_configset_get_value_multi` for querying from the config sets,\n>> which follow `last one wins` semantic(i.e. if there are multiple matches\n> \n> Space before (\n> \n\nNoted. Sorry about that.\n\n>> for the queried key in the files of the configset the value returned will\n>> the last value in the last matching file of the configset) Add type\n\n> Does this remark also apply to git_configset_get_value_multi? My\n> understanding was that \"last one wins\" applied only to\n> git_configset_get_value.\n> \n\nMaybe a reworded sentence may work,\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.)\n\n>> Add a default `config_set`, `the_config_set` to cache all key-value pairs\n> \n> I don't like the name \"the_config_set\". It's not the only one. Perhaps\n> repo_config_set? (not totally satisfactory as it does not contain only\n> the repo, but the repo+user+system)\n> \n> What do others think?\n> \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(). The reason for doing so is\n>> twofold, one is to honour include directives, another is to guarantee O(1)\n>> lookups for usual config values, as values are queried for hundred of\n>> times during a run of a git program.\n> \n> What is the reason to deal with `the_config_set` and other config sets\n> differently? You're giving arguments to store `the_config_set` as a\n> single hashmap, but what is the reason to store others as multiple\n> hashmaps?\n> \n> And actually, I don't completely buy your arguments: having 3 or 4\n> hashmaps (.git/config, ~/.gitconfig, ~/.config/git/config and\n> /etc/gitconfig) would be a O(4) lookup, which is still O(1), and you\n> could deal with include directives by having \".git/config and included\n> files\" in a hashmap, \"~/.gitconfig and included files\" in a second\n> hashmap, ...\n> \n> My guess is that the real argument is \"git_config already does what I\n> want and I'm too lazy to change it\". And I do consider lazyness as a\n> quality for a computer-scientist ;-).\n> \n> I would personally find it much simpler to have a single hashmap. We'd\n> lose the ability to invalidate the cache for only a single file, but I'm\n> not sure it's worth the code complexity.\n> \n\nPoint noted. I can take the multiple hashmap route for `the_config_set`.\nStill, since it will be the most used config set in the code and for each\nquery it would have to look through n hashmaps hampering performance. I\ncan change it if you want but like you, I don't think it is worth the code\ncomplexity.\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>> +`git_config_iter` gives a way to iterate over the keys in cache. Existing\n>> +`git_config` callback function signature is used for iterating.\n> \n> \"Existing\" refers to the old state. It's OK in a commit message, but\n> won't make sense in the future, when your non-callback API and the old\n> callback API will live side by side.\n>\n\nNoted.\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>> +`git_config_get_int`::\n>> +Parse the retrieved value to an integer, including unit factors. Dies on\n>> +error; otherwise, allocates and copies the integer into the dest parameter.\n>> +Returns 0 on success, or 1 if no value was found.\n> \n> It seems strange to me to return 1 here, and -1 in git_config_get_value\n> to mean the same thing.\n> \n\nNoted. Some of the type specific function return -1 on wrong parsing, I will\nput return value 1 for no value found in all of the cases.\n\n>> +`git_config_bool`::\n>> +/* config-hash.c */\n\n>> +#define CONFIG_SET_INIT { NULL, 0, 0 }\n>> +\n>> +struct config_set {\n>> +\tstruct config_set_item *item;\n>> +\tunsigned int nr, alloc;\n>> +};\n>> +\n>> +struct config_set_item {\n>> +\tstruct hashmap config_hash;\n>> +\tchar *filename;\n>> +static struct hashmap *add_config_hash(const char *filename, struct config_set *cs)\n>> +{\n>> +\tint ret = 0;\n>> +\tstruct config_set_item *ct;\n>> +\tstruct config_set_cb cb;\n>> +\tALLOC_GROW(cs->item, cs->nr + 1, cs->alloc);\n>> +\tct = &cs->item[cs->nr++];\n>> +\tct->filename = xstrdup(filename);\n>> +\tconfig_hash_init(&ct->config_hash);\n>> +\tcb.cs = cs;\n>> +\tcb.ct = ct;\n>> +\t/*\n>> +\t * When filename is \"default\", hashmap is constructed from the usual set of\n>> +\t * config files (i.e repo specific .git/config, user wide ~/.gitconfig and the\n>> +\t * global /etc/gitconfig), used in `the_config_set`\n>> +\t */\n>> +\tif (!strcmp(filename, \"default\"))\n> \n> How does one read a file actually called \"default\" with this API?\n> \n> More generally, why do you need a special-case here? I think it would be\n> much better to leave this part unaware of the default, and have a\n> separate function like create_repo_config_hash (or\n> create_the_config_hash) that would call git_config(...). There actually\n> isn't much shared code between the \"default\" case and the others in your\n> function.\n>\n\nNoted, a separate function would be much better.\n\n>> +static struct hashmap *get_config_hash(const char *filename, struct config_set *cs)\n>> +{\n>> +\tint i;\n>> +\tfor(i = 0; i < cs->nr; i++) {\n>> +\t\tif (!strcmp(cs->item[i].filename, filename))\n>> +\t\t\treturn &cs->item[i].config_hash;\n>> +\t}\n>> +\treturn add_config_hash(filename, cs);\n>> +}\n>> +\n>> +static struct config_hash_entry *config_hash_find_entry(const char *key, const char* filename,\n>> +\t\t\t\t\t\t\tstruct config_set *cs)\n> \n> I don't get the logic here.\n> \n> Either the caller explicitly manages a config_set variable like\n> \n> config_set my_set = git_configset_init();\n> git_configset_add_file(my_set, \"my-file\");\n> git_configset_get_value(my_set, \"some.variable.name\");\n> \n> Or there's an implicit singleton mapping files to hashmaps to allow the\n> user to say\n> \n> git_configset_get_value(\"my-file\", \"some.variable.name\");\n> \n> without asking the user to explicitly manipulate config_set variables.\n> \n> It seems to me that your solution is a bad compromise between both\n> solutions: you do require the user to manipulate config_set variables,\n> but you also require a filename to look for the right entry in the list.\n> \n> Did you miss the end of Junio's message:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/252567\n> \n> ?\n>\n\nThis is an internal function which is used to iterate over every `config_set`\nitem which contains a hashmap and filename as an identifier.\nThe exposed API which I documented above doesn't require the user to pass the\nfilename when querying for a value.\n\nThe API works like this, suppose there are two files,\nA.config\tB.config\n[foo]\t\t[foo]\na = b\t\ta = d\na = c\t\ta = e\n\nconfig_set *my_set = git_configset_init();\ngit_configset_add_file(my_set, \"A.config\");\ngit_configset_add_file(my_set, \"B.config\");\ngit_configset_get_value(my_set, \"foo.a\");\n\nHere get_value calls config_hash_find_entry once for each configset_item\n\"A.config\" and \"B.config\" with key as \"foo.a\". which in turn returns\nconfig_hash_item which contains the key and the value list.\n\nWe get two string_lists containing values for the key foo.a,\n{b,c} and {d,e} corresponding to A.config & B.config respectively.\n\nget_value then returns the value with the highest priority which is 'e'\nafter going through both of the lists and picking the last one.\nThese steps are done by internal functions not by the user facing\nAPI ones.\n\n> If the user explicitly asks for a single entry in the list, then why\n> group them in a list? I guess the question is the same as above, and the\n> more I read the patch, the more I think a config_set should contain a\n> single hashmap.\n>\n\nDo you think so after reading the whole patch? It would be much easier\nto implement config_set using a single hashmap . But Junio had suggested\nthat it would be much better for config_set to be a collection of hashmaps\nand `the_config_set` to contain a single hashmap[1].\n\n[1]http://thread.gmane.org/gmane.comp.version-control.git/252329/focus=252460\n\n>> +static int config_hash_add_value(const char *key, const char *value,\n>> +\t\t\t\t struct config_set_item *ct, struct config_set *cs)\n>> +{\n>> +\tstruct hashmap *config_hash = &ct->config_hash;\n>> +\tstruct config_hash_entry *e;\n>> +\te = config_hash_find_entry(key, ct->filename, cs);\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 normalised.\n>> +int git_config_get_value(const char *key, const char **value)\n>> +{\n>> +\tif (!the_config_set_initialised) {\n>> +\t\tgit_configset_add_file(&the_config_set, \"default\");\n>> +\t\tthe_config_set_initialised = 1;\n>> +\t}\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>> +\tif (!the_config_set_initialised) {\n>> +\t\tgit_configset_add_file(&the_config_set, \"default\");\n>> +\t\tthe_config_set_initialised = 1;\n>> +\t}\n>> +\treturn git_configset_get_value_multi(&the_config_set, key);\n>> +}\n> \n> These if statements could be refactored, and the_config_set_initialised\n> could be hidden to the caller. Just make a function get_the_config_set()\n> that initializes if needed and returns a pointer to the_config_set.\n>\n\nNoted. Thanks for the quick review.\n\nTanay.\n"},{"id":"245290","messageId":"53B3F557.3080001@gmail.com","threadId":"37037","inReplyTo":"vpqzjgskt1v.fsf@anie.imag.fr","subject":"Re: [PATCH 2/2] test-config: Add tests for the config_set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-02T12:04:39Z","receivedAt":"2014-07-02T12:04:39Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/2/2014 2:59 PM, Matthieu Moy wrote:\n> Tanay Abhra writes:\n> \n>> +test_expect_success 'clear default config' '\n>> +\trm -f .git/config\n>> +'\n>> +\n>> +cat > .git/config << EOF\n> \n> t/README says:\n> \n> - Put all code inside test_expect_success and other assertions.\n> \n> Even code that isn't a test per se, but merely some setup code\n> should be inside a test assertion.\n> \n> Even these cat > would better be in a test_expect_success 'initialize\n> config'.\n> \n> (Not applied everywhere in Git's code essentially because some tests\n> were written before the guideline was set and never updated).\n\nSorry about that. I followed t1300-repo-config.sh which has these mistakes\nalso.\n\n>> +[core]\n>> +\tpenguin = very blue\n>> +\tMovie = BadPhysics\n>> +\tUPPERCASE = true\n>> +\tMixedCase = true\n>> +\tmy =\n>> +\tfoo\n>> +\tbaz = sam\n>> +[Cores]\n>> +\tWhatEver = Second\n>> +[my \"Foo bAr\"]\n>> +\thi = hello\n> \n> To really stress the \"case sensitive middle part\" case, you should also\n> have other sections like\n> \n> [my \"foo bar\"]\n> \thi = lower-case\n> [my \"FOO BAR\"]\n> \thi = upper-case\n> \n> and check that you get the right value for my.*.hi\n> \n> Similarly, I'd add a [CORE] and a [CoRe] section to check that their\n> content is actually merged with [core].\n>\n\nNoted.\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>> +test_expect_success 'upper case key' '\n> \n> Keep the style consistent, if you separate tests with a single blank\n> line, do it everywhere.\n> \n>> +cat > expect << EOF\n> \n> See above, should be in test_expect_success.\n> \n> Also, >expect, not > expect.\n> \n> There are other instances.\n>\n\nNoted. Again copied t1300-repo-config.sh style for cat.\n\n>> +1\n>> +0\n>> +1\n>> +1\n>> +1\n>> +EOF\n>> +\n>> +test_expect_success 'find bool value for the entered key' '\n>> +\ttest-config get_bool goat.head >>actual &&\n> \n> The first one should be a single >, or you should clear actual before\n> the test.\n> \n\nNoted.\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 = CONFIG_SET_INIT;\n> \n> \n> \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\tno_of_files = git_config_int(\"unused\", argv[2]);\n> \n> Why ask the user to give a number of files on the command-line. With a\n> syntax like\n> \n> test-config configset_get_value ...\n> \n> you could just use argc to iterate over argv. Here, you trust the user\n> to provide the right value, and most likely segfault otherwise (and this\n> is not really documented). I know this is only test code, but why not do\n> it right anyway ;-).\n> \n\nYup, your way is much better, thanks for the review.\nTanay.\n"},{"id":"245295","messageId":"vpqbnt7bzm0.fsf@anie.imag.fr","threadId":"37037","inReplyTo":"53B3F403.6030501@gmail.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-02T14:32:23Z","receivedAt":"2014-07-02T14:32:23Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n> On 7/2/2014 2:44 PM, Matthieu Moy wrote:\n>> Tanay Abhra writes:\n\n> Maybe a reworded sentence may work,\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.)\n\nOK.\n\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(). The reason for doing so is\n>>> twofold, one is to honour include directives, another is to guarantee O(1)\n>>> lookups for usual config values, as values are queried for hundred of\n>>> times during a run of a git program.\n>> \n>> What is the reason to deal with `the_config_set` and other config sets\n>> differently? You're giving arguments to store `the_config_set` as a\n>> single hashmap, but what is the reason to store others as multiple\n>> hashmaps?\n>> \n>> And actually, I don't completely buy your arguments: having 3 or 4\n>> hashmaps (.git/config, ~/.gitconfig, ~/.config/git/config and\n>> /etc/gitconfig) would be a O(4) lookup, which is still O(1), and you\n>> could deal with include directives by having \".git/config and included\n>> files\" in a hashmap, \"~/.gitconfig and included files\" in a second\n>> hashmap, ...\n>> \n>> My guess is that the real argument is \"git_config already does what I\n>> want and I'm too lazy to change it\". And I do consider lazyness as a\n>> quality for a computer-scientist ;-).\n>> \n>> I would personally find it much simpler to have a single hashmap. We'd\n>> lose the ability to invalidate the cache for only a single file, but I'm\n>> not sure it's worth the code complexity.\n>> \n>\n> Point noted. I can take the multiple hashmap route for `the_config_set`.\n> Still, since it will be the most used config set in the code and for each\n> query it would have to look through n hashmaps hampering performance. I\n> can change it if you want but like you, I don't think it is worth the code\n> complexity.\n\nThat's why my suggestion is to use a single hashmap everywhere.\n\nI don't have strong opinion either way, but whichever way you go, you\nshould justify the choice better in the commit message.\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>>> +`git_config_get_int`::\n>>> +Parse the retrieved value to an integer, including unit factors. Dies on\n>>> +error; otherwise, allocates and copies the integer into the dest parameter.\n>>> +Returns 0 on success, or 1 if no value was found.\n>> \n>> It seems strange to me to return 1 here, and -1 in git_config_get_value\n>> to mean the same thing.\n>> \n>\n> Noted. Some of the type specific function return -1 on wrong parsing, I will\n> put return value 1 for no value found in all of the cases.\n\nI'm not sure I fully get the existing convention. My understanding is\nthat when the extracted value is returned, -1 is used as a special value\nto mean \"no value\" (e.g. git_config_maybe_bool can return 1, 0 or -1),\nbut when the extracted value is written to a by-address parameter, then\nthe return value is 1 or 0.\n\n>>> +static struct hashmap *get_config_hash(const char *filename, struct config_set *cs)\n>>> +{\n>>> +\tint i;\n>>> +\tfor(i = 0; i < cs->nr; i++) {\n>>> +\t\tif (!strcmp(cs->item[i].filename, filename))\n>>> +\t\t\treturn &cs->item[i].config_hash;\n>>> +\t}\n>>> +\treturn add_config_hash(filename, cs);\n>>> +}\n>>> +\n>>> +static struct config_hash_entry *config_hash_find_entry(const char *key, const char* filename,\n>>> +\t\t\t\t\t\t\tstruct config_set *cs)\n>> \n>> I don't get the logic here.\n>> \n>> Either the caller explicitly manages a config_set variable like\n>> \n>> config_set my_set = git_configset_init();\n>> git_configset_add_file(my_set, \"my-file\");\n>> git_configset_get_value(my_set, \"some.variable.name\");\n>> \n>> Or there's an implicit singleton mapping files to hashmaps to allow the\n>> user to say\n>> \n>> git_configset_get_value(\"my-file\", \"some.variable.name\");\n>> \n>> without asking the user to explicitly manipulate config_set variables.\n>> \n>> It seems to me that your solution is a bad compromise between both\n>> solutions: you do require the user to manipulate config_set variables,\n>> but you also require a filename to look for the right entry in the list.\n>> \n>> Did you miss the end of Junio's message:\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/252567\n>> \n>> ?\n>>\n>\n> This is an internal function which is used to iterate over every `config_set`\n> item which contains a hashmap and filename as an identifier.\n> The exposed API which I documented above doesn't require the user to pass the\n> filename when querying for a value.\n>\n> The API works like this, suppose there are two files,\n> A.config\tB.config\n> [foo]\t\t[foo]\n> a = b\t\ta = d\n> a = c\t\ta = e\n>\n> config_set *my_set = git_configset_init();\n> git_configset_add_file(my_set, \"A.config\");\n> git_configset_add_file(my_set, \"B.config\");\n> git_configset_get_value(my_set, \"foo.a\");\n>\n> Here get_value calls config_hash_find_entry once for each configset_item\n> \"A.config\" and \"B.config\" with key as \"foo.a\". which in turn returns\n> config_hash_item which contains the key and the value list.\n\nOK, I understand what you did. But it still seem wrong to me. If you\ninline functions, you are doing something like\n\nfor (f in files) { // in git_configset_get_value\n for (g in files) { // in get_config_hash\n if (f == g) {\n take the hashmap for g.\n }\n }\n}\n\nThis is O(n^2) to iterate over an ordered list. OK, the list is small,\nbut still, why not iterate normally over the array?\n\nAlgorithmically speaking, you do not need the file names here, just\niterate over an array of hashmaps. Filenames do not harm in struct\nconfig_set, but they are just metadata, not usefull to do the search.\n\n> We get two string_lists containing values for the key foo.a,\n> {b,c} and {d,e} corresponding to A.config & B.config respectively.\n>\n> get_value then returns the value with the highest priority which is 'e'\n> after going through both of the lists and picking the last one.\n> These steps are done by internal functions not by the user facing\n> API ones.\n\nBut then, I have to wonder again what is the benefit of having multiple\nhashmaps in the config_set, since it has no user-visible benefit.\n\n>> If the user explicitly asks for a single entry in the list, then why\n>> group them in a list? I guess the question is the same as above, and the\n>> more I read the patch, the more I think a config_set should contain a\n>> single hashmap.\n>>\n>\n> Do you think so after reading the whole patch? It would be much easier\n> to implement config_set using a single hashmap . But Junio had suggested\n> that it would be much better for config_set to be a collection of hashmaps\n> and `the_config_set` to contain a single hashmap[1].\n>\n> [1]http://thread.gmane.org/gmane.comp.version-control.git/252329/focus=252460\n\nI do not read the message like you. Junio gives pros&cons of \"everything\nin a single hashmap\" Vs \"one hashmap per file\", and explicitely\nconcludes with\n\n| I don't know at this point, and thinking these things through\n| to arrive at a good design is part of the GSoC project after all, so\n| I'd rather not to think it through to the end myself ;-).\n\nI do not read \"there should be multiple hashmaps, except in the case of\nthe_configset which should be handled differently\".\n\nTo summarize:\n\nAdvantages of using a single hashmap per configset:\n\n1) Code is simpler\n\n2) Lookup is slightly more efficient\n\nAdvantages of using one hashmap per configset:\n\nA) Allows sharing hashmaps between configsets\n\nB) Allows selectively refreshing the config for a single file without\n touching the other files' hashmaps.\n\nC) Would allow more operations like \"tell me which file contains the\n config key foo.bar\", that could be useful in some situation.\n\nRight now, I do not think that your implementation uses any of A), B) or\nC), so you're doing something uselessly complex.\n\nIn the future, my feeling is that A) is essentially irrelevant (we're\ntalking about a few hundreds of entry for large config files, not\nsomething we want to fine-tune in memory), B) would raise tons of\ncorner-cases and would be hard to get right compared to the benefit. C)\ncould be interesting, but the most common case would be to use\nthe_configset, in which there's only one hashmap so it's not applicable\nwithout a big code change.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245311","messageId":"xmqqd2dnitm7.fsf@gitster.dls.corp.google.com","threadId":"37037","inReplyTo":"1404280905-26763-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T17:00:00Z","receivedAt":"2014-07-02T17:00:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n> index 230b3a0..2c02fee 100644\n> --- a/Documentation/technical/api-config.txt\n> +++ b/Documentation/technical/api-config.txt\n> @@ -77,6 +77,75 @@ 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> +`git_config_get_value` takes two parameters,\n> +\n> +- a key string in canonical flat form for which the corresponding value\n\nWhat is \"canonical flat form\"?\n\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. The caller\n> + should not free or modify the value returned as it is owned by the cache.\n> +\n> +`git_config_get_value` returns 0 on success, or -1 for no value found.\n> +\n> +`git_config_get_value_multi` allocates and returns a `string_list`\n> +containing all the values for the key passed as parameter, sorted in order\n> +of increasing priority (Note: caller has to call `string_list_clear` on\n> +the returned pointer and then free it).\n> +\n> +`git_config_get_value_multi` returns NULL for no value found.\n> +\n> +`git_config_clear` is provided for resetting and invalidating the cache.\n> +\n> +`git_config_iter` gives a way to iterate over the keys in cache. Existing\n> +`git_config` callback function signature is used for iterating.\n\nInstead of doing prose above and then enumeration below,\nconsistently using the enumeration-style would make the descriptions\nof API functions easier to find and read. Also show what parameters\neach function takes. E.g.\n\n\t`git_config_get_value(const char *key, const char **value)`::\n\t\tFind the highest-priority value for the\n\t\tconfiguration variable `key`, store the pointer to\n\t\tit in `value` and return 0. When the configuration\n\t\tvariable `key` is not found, return -1 without\n\t\ttouching `value`.\n \nA few random thoughts.\n\n - Are \"a value for the variable is found\" and \"no value for the\n variable is found\" the only possible outcome? Should somebody\n (not necessarily the calling code) be notified of an error---for\n example, if you opened a config file successfully but later found\n that the file could not be parsed due to a syntax error or an I/O\n error, is it possible the caller of this function to tell, or are\n these just some special cases of \"variable not found\"?\n\n - The same goes for the \"multi\" variant but it is a bit more grave,\n as a function that returns an \"int\" can later be updated to\n return different negative values to signal different kinds of\n errors, but a function that returns a pointer to string-list can\n only return one kind of NULL ;-)\n\n - For the purpose of \"git_config_get_value()\", what is the value\n returned for core.filemode when the configuration file says\n \"[core] filemode\\n\", i.e. when git_config() callback would get a\n NULL for value to signal a boolean true?\n\n - Is there a reason why you need a separate and new \"config_iter\"?\n What does it do differently from the good-old git_config() and\n how? It does not belong to \"Querying for Specific Variables\"\n anyway.\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> +`git_config_get_int`::\n> +Parse the retrieved value to an integer, including unit factors. Dies on\n> +error; otherwise, allocates and copies the integer into the dest parameter.\n> +Returns 0 on success, or 1 if no value was found.\n\n\"allocates and copies\"???? Is a caller forced to do something like\nthis?\n\n\tint *val;\n\n\tif (!git_config_get_int(\"int.one\", &val)) {\n \tuse(*val);\n free(val);\n\t}\n\nI'd expect it to be more like:\n\n\tint val;\n if (!git_config_get_int(\"int.one\", &val))\n \tuse(val);\n\nAlso, is there a reason why the \"not found\" signal is \"1\" (as\nopposed to \"-1\" for the lower-level get_value() API)?\n\n> +`git_config_get_ulong`::\n> +Identical to `git_config_get_int` but for unsigned longs.\n\nObviously this is not identical but merely \"Similar\" to.\n\n> +`git_config_bool`::\n\ngit_config_get_bool, perhaps?\n\n> +Custom Configsets\n> +-----------------\n> +\n> +A `config_set` points at an ordered set of config files, each of\n> +which represents what was read and cached in-core from a file.\n\nThis over-specifies the implementation. Judging from the list of\nAPI functions below, an implementation is possible without having to\nkeep track of what source files were fed in what order (i.e. it can\njust keep a single hash-table of read values and incrementally parse\nthe new contents given via configset-add-file into it, without even\nrecording the origins, and forget about the source files once it\nfinishes parsing).\n\nAs was pointed out during the previous review round, a stack of\n tuples cannot represent the configuration when\nconfig-includes are involved. Also we would eventually want to be\nable to also read from non-file sources (e.g. from a blob, or a\ncaller-supplied in-core strings).\n\nFor the purpose of reporting errors at the point of value being\nused, I have a suspicion that you would need to associate the\n pair with each individual value string you will keep\nafter configset_add_file() parses the file. The implementation of\nthe call-chain may look like:\n\n\n\tgit_configset_add_file(cs, filename):\n \tfile = open filename\n lineno = 0\n\t\twhile read line from file:\n\t git_configset_parse_line(cs, filename, lineno++, line)\n\t\tclose file\n\n\tgit_configset_parse_line(cs, filename, lineno, line):\n \t... this needs to implement a state machine\n ... that keeps track of what has been read halfway\n ... e.g. we may have seen \"[core] crlf =\\\\\\n\"\n \t... earlier, which is not a complete input yet,\n ... and now we may be looking at \"auto\" that lets\n ... us finally parse one item.\n\n\t if state in 'cs' and new 'line' gives us a complete input:\n\t\t\tdetermine key and value\n record for 'key' to cs.hash\n update state in 'cs'\n\nFor processing \"git -c section.variable=value\", we probably would\ncall git_configset_parse_line() on the_configset with filename\nset to \"command line\" or something.\n\nAnd then when the caller tries to actually use the value, e.g.\n\n git_configset_get_int(cs, key):\n\t\tlook up for 'key' from the cs.hash\n\t\tif value is successfully parsed as int:\n \treturn the parsed result\n\t\telse:\n \terror(\"not an int: '%s' (%s:%s)\", value, filename, lineno)\n\nperhaps?\n\n> +It can be used to construct an in-memory cache for config files that\n> +the caller specifies. For example,\n\nThis is almost good to help a reader decide if she might want to use\nit in her code, but we probably want to stress the fact that use of\na config_set is done for a namespace separate from the main\nconfiguration system, e.g. \".gitmodules\".\n\n> +---------------------------------------\n> +struct config_set gm_config = CONFIG_SET_INIT;\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> +/* 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> +`git_configset_init`::\n> +\n> +Returns an allocated and initialized struct `config_set` pointer.\n\n\"allocated\"??? Is a caller forced to do this, i.e. always have the\nconfig-set on heap?\n\n\tstruct config_set *config = git_configset_init();\n ... use it ...\n git_configset_clear(config);\n\nI'd expect it be more like this:\n\n\tstruct config_set config;\n\n\tgit_configset_init(&config);\n ... use it...\n git_configset_clear(&config);\n"},{"id":"245313","messageId":"vpqbnt7k7qk.fsf@anie.imag.fr","threadId":"37037","inReplyTo":"xmqqd2dnitm7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-02T17:09:39Z","receivedAt":"2014-07-02T17:09:39Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Tanay Abhra writes:\n>\n>> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n>> index 230b3a0..2c02fee 100644\n>> --- a/Documentation/technical/api-config.txt\n>> +++ b/Documentation/technical/api-config.txt\n>> @@ -77,6 +77,75 @@ 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>> +`git_config_get_value` takes two parameters,\n>> +\n>> +- a key string in canonical flat form for which the corresponding value\n>\n> What is \"canonical flat form\"?\n\nActually, it's defined above in the same file (was here before the\npatch):\n\n - the name of the parsed variable. This is in canonical \"flat\" form: the\n section, subsection, and variable segments will be separated by dots,\n and the section and variable segments will be all lowercase. E.g.,\n\nBut it might make sense to reword the doc, e.g. introduce a glossary\nsection at the beginning.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245312","messageId":"xmqq8uobit5y.fsf@gitster.dls.corp.google.com","threadId":"37037","inReplyTo":"vpqoax8m8bh.fsf@anie.imag.fr","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T17:09:45Z","receivedAt":"2014-07-02T17:09:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> I don't like the name \"the_config_set\". It's not the only one. Perhaps\n> repo_config_set? (not totally satisfactory as it does not contain only\n> the repo, but the repo+user+system)\n>\n> What do others think?\n\nI actually do like \"the_configset\", which goes nicely parallel to\n\"the_index\". Neither is the only one, but most of the time our code\noperates on the primary one and \"the_frotz\" refers to it (and\nconvenience wrappers that does not specify which set defaults to\nit).\n\n> What is the reason to deal with `the_config_set` and other config sets\n> differently? You're giving arguments to store `the_config_set` as a\n> single hashmap, but what is the reason to store others as multiple\n> hashmaps?\n\nConfusion, probably. \"the_configset\" should be just a singleton\ninstance used to store the values we use for the default config\nsystem, but its shape should be exactly the same as the other ones\nusers can use to read .gitmodules and friends with.\n\n> And actually, I don't completely buy your arguments: having 3 or 4\n> hashmaps (.git/config, ~/.gitconfig, ~/.config/git/config and\n> /etc/gitconfig) would be a O(4) lookup, which is still O(1), and you\n> could deal with include directives by having \".git/config and included\n> files\" in a hashmap, \"~/.gitconfig and included files\" in a second\n> hashmap, ...\n\nOK.\n\n> I would personally find it much simpler to have a single hashmap. We'd\n> lose the ability to invalidate the cache for only a single file, but I'm\n> not sure it's worth the code complexity.\n\nOK, too.\n"},{"id":"245381","messageId":"53B58D49.8010409@gmail.com","threadId":"37037","inReplyTo":"xmqqd2dnitm7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-03T17:05:13Z","receivedAt":"2014-07-03T17:05:13Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/2/2014 10:30 PM, Junio C Hamano wrote:\n> Tanay Abhra writes:\n> \n>> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\n>> index 230b3a0..2c02fee 100644\n>> --- a/Documentation/technical/api-config.txt\n>> +++ b/Documentation/technical/api-config.txt\n>> @@ -77,6 +77,75 @@ To read a specific file in git-config format, use\n>> +`git_config_get_value` returns 0 on success, or -1 for no value found.\n>> +\n>> +`git_config_get_value_multi` allocates and returns a `string_list`\n>> +containing all the values for the key passed as parameter, sorted in order\n>> +of increasing priority (Note: caller has to call `string_list_clear` on\n>> +the returned pointer and then free it).\n>> +\n>> +`git_config_get_value_multi` returns NULL for no value found.\n>> +\n>> +`git_config_clear` is provided for resetting and invalidating the cache.\n>> +\n>> +`git_config_iter` gives a way to iterate over the keys in cache. Existing\n>> +`git_config` callback function signature is used for iterating.\n> \n> Instead of doing prose above and then enumeration below,\n> consistently using the enumeration-style would make the descriptions\n> of API functions easier to find and read. Also show what parameters\n> each function takes. E.g.\n> \n\nNoted.\n\n> \n> A few random thoughts.\n> \n> - Are \"a value for the variable is found\" and \"no value for the\n> variable is found\" the only possible outcome? Should somebody\n> (not necessarily the calling code) be notified of an error---for\n> example, if you opened a config file successfully but later found\n> that the file could not be parsed due to a syntax error or an I/O\n> error, is it possible the caller of this function to tell, or are\n> these just some special cases of \"variable not found\"?\n\nThe syntactical errors when parsing the file are shown when it fails to\nconstruct the hashmap. Whenever a caller calls for a value for the first\ntime, the file is read and parsed and if it fails during parsing it dies\nraising a error specifying the lineno and filename.\n\nVariable not found means that the variable is not present in the file,\nnothing more. Note: the hashmap code uses git_config() parsing stack\nso whatever error it raises in normal git_config() invocation, it\nwould be raised here too.\n\n> - The same goes for the \"multi\" variant but it is a bit more grave,\n> as a function that returns an \"int\" can later be updated to\n> return different negative values to signal different kinds of\n> errors, but a function that returns a pointer to string-list can\n> only return one kind of NULL ;-)\n>\n\nNull pointer just means no variable found in the files. What other kind\nof errors may arise?\n\n> - For the purpose of \"git_config_get_value()\", what is the value\n> returned for core.filemode when the configuration file says\n> \"[core] filemode\\n\", i.e. when git_config() callback would get a\n> NULL for value to signal a boolean true?\n\nNULL in value pointer with 0 return value denoting variable found.\n\n> - Is there a reason why you need a separate and new \"config_iter\"?\n> What does it do differently from the good-old git_config() and\n> how? It does not belong to \"Querying for Specific Variables\"\n> anyway.\n>\n\nYou mentioned previously in the list for a analogue to git_config()\nwhich supports iterating over the keys.\nThe link is [1] I think, gmane is not working for me currently.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/252567\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>> +`git_config_get_int`::\n>> +Parse the retrieved value to an integer, including unit factors. Dies on\n>> +error; otherwise, allocates and copies the integer into the dest parameter.\n>> +Returns 0 on success, or 1 if no value was found.\n> \n> \"allocates and copies\"???? Is a caller forced to do something like\n> this?\n> \n> \tint *val;\n> \n> \tif (!git_config_get_int(\"int.one\", &val)) {\n> \tuse(*val);\n> free(val);\n> \t}\n> \n> I'd expect it to be more like:\n> \n> \tint val;\n> if (!git_config_get_int(\"int.one\", &val))\n> \tuse(val);\n>\n\nYup, you are right, my fault.\n\n> Also, is there a reason why the \"not found\" signal is \"1\" (as\n> opposed to \"-1\" for the lower-level get_value() API)?\n> \n\nMany of the type specific functions return -1 for wrongful parsing\nlike git_config_get_string and git_config_get_maybe_bool, that is\nwhy I changed the return value for such functions.\n\n>> +Custom Configsets\n>> +-----------------\n>> +\n>> +A `config_set` points at an ordered set of config files, each of\n>> +which represents what was read and cached in-core from a file.\n> \n> This over-specifies the implementation. Judging from the list of\n> API functions below, an implementation is possible without having to\n> keep track of what source files were fed in what order (i.e. it can\n> just keep a single hash-table of read values and incrementally parse\n> the new contents given via configset-add-file into it, without even\n> recording the origins, and forget about the source files once it\n> finishes parsing).\n> \n> As was pointed out during the previous review round, a stack of\n> tuples cannot represent the configuration when\n> config-includes are involved. Also we would eventually want to be\n> able to also read from non-file sources (e.g. from a blob, or a\n> caller-supplied in-core strings).\n> \n> For the purpose of reporting errors at the point of value being\n> used, I have a suspicion that you would need to associate the\n> pair with each individual value string you will keep\n> after configset_add_file() parses the file. The implementation of\n> the call-chain may look like:\n> \n> \n> \tgit_configset_add_file(cs, filename):\n> \tfile = open filename\n> lineno = 0\n> \t\twhile read line from file:\n> \t git_configset_parse_line(cs, filename, lineno++, line)\n> \t\tclose file\n> \n> \tgit_configset_parse_line(cs, filename, lineno, line):\n> \t... this needs to implement a state machine\n> ... that keeps track of what has been read halfway\n> ... e.g. we may have seen \"[core] crlf =\\\\\\n\"\n> \t... earlier, which is not a complete input yet,\n> ... and now we may be looking at \"auto\" that lets\n> ... us finally parse one item.\n> \n> \t if state in 'cs' and new 'line' gives us a complete input:\n> \t\t\tdetermine key and value\n> record for 'key' to cs.hash\n> update state in 'cs'\n> \n> For processing \"git -c section.variable=value\", we probably would\n> call git_configset_parse_line() on the_configset with filename\n> set to \"command line\" or something.\n> \n> And then when the caller tries to actually use the value, e.g.\n> \n> git_configset_get_int(cs, key):\n> \t\tlook up for 'key' from the cs.hash\n> \t\tif value is successfully parsed as int:\n> \treturn the parsed result\n> \t\telse:\n> \terror(\"not an int: '%s' (%s:%s)\", value, filename, lineno)\n> \n\nOkay, lets see what I can do with it.\n\n> \n>> +It can be used to construct an in-memory cache for config files that\n>> +the caller specifies. For example,\n> \n> This is almost good to help a reader decide if she might want to use\n> it in her code, but we probably want to stress the fact that use of\n> a config_set is done for a namespace separate from the main\n> configuration system, e.g. \".gitmodules\".\n> \n>> +---------------------------------------\n>> +struct config_set gm_config = CONFIG_SET_INIT;\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>> +/* 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>> +`git_configset_init`::\n>> +\n>> +Returns an allocated and initialized struct `config_set` pointer.\n> \n> \"allocated\"??? Is a caller forced to do this, i.e. always have the\n> config-set on heap?\n> \n> \tstruct config_set *config = git_configset_init();\n> ... use it ...\n> git_configset_clear(config);\n> \n> I'd expect it be more like this:\n> \n> \tstruct config_set config;\n> \n> \tgit_configset_init(&config);\n> ... use it...\n> git_configset_clear(&config);\n> \n\nYup, I prefer the second one.\nThanks for the review,\nTanay Abhra.\n"},{"id":"245401","messageId":"53B63461.2040300@gmail.com","threadId":"37037","inReplyTo":"vpqoax8m8bh.fsf@anie.imag.fr","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-04T04:58:09Z","receivedAt":"2014-07-04T04:58:09Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\nI have cooked up a single hashmap implementation. What are your\nthoughts about it? I must say it is much cleaner than the\nprevious attempt and it passes all the tests.\nI will send the revised patch tomorrow with the corrected\ndocumentation, till then please say so if you prefer this one\nor the multi hashmap one.\n\nCheers,\nTanay Abhra.\n\n-- >8 --\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..2a675f6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -7,6 +7,7 @@\n #include \"advice.h\"\n #include \"gettext.h\"\n #include \"convert.h\"\n+#include \"string-list.h\"\n\n #include SHA1_HEADER\n #ifndef git_SHA_CTX\n@@ -1325,6 +1326,41 @@ 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 struct config_set *git_configset_init(void);\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..e12bf57\n--- /dev/null\n+++ b/config-hash.c\n@@ -0,0 +1,325 @@\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+struct config_set *git_configset_init(void)\n+{\n+\tstruct config_set *cs = xcalloc(1, sizeof(*cs));\n+\treturn cs;\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 the last value\n+\t * in the last matching file of the configset\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+/*\n+ * Uses the existing git_config() callback function signature to iterate over\n+ * keys in the configset.\n+ */\n+void git_configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n+{\n+\tint i;\n+\tchar *value;\n+\tstruct string_list *strptr;\n+\tstruct config_hash_entry *entry;\n+\tstruct hashmap_iter iter;\n+\thashmap_iter_init(&cs->config_hash, &iter);\n+\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\tstrptr = &entry->value_list;\n+\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\tvalue = strptr->items[i].string;\n+\t\t\tfn(entry->key, value, data);\n+\t\t}\n+\t}\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+void git_config_iter(config_fn_t fn, void *data)\n+{\n+\tgit_config_check_init();\n+\tgit_configset_iter(&the_config_set, fn, data);\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":"245402","messageId":"vpq1tu1xz2g.fsf@anie.imag.fr","threadId":"37037","inReplyTo":"53B63461.2040300@gmail.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-04T09:17:43Z","receivedAt":"2014-07-04T09:17:43Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> Hi,\n>\n> I have cooked up a single hashmap implementation. What are your\n> thoughts about it?\n\nI had a quick look, and it looks good to me. I'll make a more detailed\nreview when you send the next series.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245403","messageId":"53B67322.1080408@gmail.com","threadId":"37037","inReplyTo":"vpq1tu1xz2g.fsf@anie.imag.fr","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-04T09:25:54Z","receivedAt":"2014-07-04T09:25:54Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/4/2014 2:47 PM, Matthieu Moy wrote:\n> Tanay Abhra writes:\n> \n>> Hi,\n>>\n>> I have cooked up a single hashmap implementation. What are your\n>> thoughts about it?\n> \n> I had a quick look, and it looks good to me. I'll make a more detailed\n> review when you send the next series.\n>\n\nOne more doubt, does for every value has any use other than\nraising semantic error in typespecific API functions.\n\nFor example, if we call git_config_get_int(foo.bar), we can show to the user\n\"value not a int at \". Other than that I cannot think of\nany other use of it. Currently `git_config_int` dies if value put for\nparsing is not an int.\n\nJunio and Karsten, both raised the point for saving , but I can't\nfind any use cases for it other than what I mentioned above.\n\nThanks.\nTanay.\n"},{"id":"245404","messageId":"vpqsimhtq69.fsf@anie.imag.fr","threadId":"37037","inReplyTo":"53B67322.1080408@gmail.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-04T09:43:26Z","receivedAt":"2014-07-04T09:43:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tanay Abhra writes:\n\n> One more doubt, does for every value has any use other than\n> raising semantic error in typespecific API functions.\n\nI don't see any other. My suggestion would be: ignore this for now, it's\nnot needed to get a new API that has at least as many features as the\nexisting one.\n\nIt's easy to add to the data-structure afterwards (although a bit less\neasy to add to the code that fills-in the data structure).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245474","messageId":"xmqqwqbpdrvo.fsf@gitster.dls.corp.google.com","threadId":"37037","inReplyTo":"53B67322.1080408@gmail.com","subject":"Re: [PATCH v4 1/2] add `config_set` API for caching config files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-07T17:02:19Z","receivedAt":"2014-07-07T17:02:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> On 7/4/2014 2:47 PM, Matthieu Moy wrote:\n>> Tanay Abhra writes:\n>> \n>>> Hi,\n>>>\n>>> I have cooked up a single hashmap implementation. What are your\n>>> thoughts about it?\n>> \n>> I had a quick look, and it looks good to me. I'll make a more detailed\n>> review when you send the next series.\n>>\n>\n> One more doubt, does for every value has any use other than\n> raising semantic error in typespecific API functions.\n>\n> For example, if we call git_config_get_int(foo.bar), we can show to the user\n> \"value not a int at \". Other than that I cannot think of\n> any other use of it. Currently `git_config_int` dies if value put for\n> parsing is not an int.\n>\n> Junio and Karsten, both raised the point for saving , but I can't\n> find any use cases for it other than what I mentioned above.\n\nYes, error reporting is what the pair needs to be kept for.\n"}]}