{"thread":{"id":"37107","subject":"[PATCH v8 0/3] git config cache & special querying api utilizing the cache","startedAt":"2014-07-11T03:34:13Z","lastAt":"2014-07-15T11:32:54Z","messageCount":14,"participants":["Tanay Abhra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":8,"patchTotal":3},"messages":[{"id":"245752","messageId":"1405049655-4265-1-git-send-email-tanayabh@gmail.com","threadId":"37107","inReplyTo":null,"subject":"[PATCH v8 0/3] git config cache & special querying api utilizing the cache","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-11T03:34:13Z","receivedAt":"2014-07-11T03:34:13Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Hi,\n\n[PATCH V8]: Moved the contents of config-set.c to config.c for future convenience. Reverted\n\ttest 'find value with misspelled key' to the one in v5. See [10] for the discussion.\n\n[PATCH V7]: Style nits and a broken && chain corrected in `t/t1308-config-set.sh`. See\n\t[9] for the nits.\n\n[PATCH V6]: Style nits and mistakes corrected. Diff between v6 and v5[8] is at the bottom.\n\t\tThanks to Matthieu, Ramsay and Ram for their suggestions.\n\n[PATCH V5]: `config_set` now uses a single hashmap. Corrected style nits raised in\n\t\t\tthe thread[7]. Thanks to Junio and Matthieu for their suggestions.\n\n[PATCH v4]: Introduced `config_set` construct which points to a ordered set of\n\tconfig-files cached as hashmaps. Added relevant API functions. For more\n\tdetails see the documentation. Rewrote the git_config_get* family to use\n\t`config_set` internally. Added tests for both config_set API and git_config_get\n\tfamily. Added type specific API functions which parses the found value and\n\tconverts it into a specific type.\n\tMost of the changes implemented are the result of discussion in [6].\n\tThanks to Eric, Ramsay, Junio, Matthieu & Karsten for their suggestions\n\tand review.\n\n[PATCH v3]: Added flag for NULL values that were causing segfaults in some cases.\n\tAdded test-config for usage examples.\n\tMinor changes and corrections. Refer to discussion thread[5] for more details.\n\tThanks to Matthieu, Jeff and Junio for their valuable suggestions.\n\n[PATCH v2]:Changed the string_list to a struct instead of pointer to a struct.\n\tAdded string-list initilization functions.\n\tMinor mistakes corrected acoording to review comments[4]. Thanks to\n\tEric and Matthieu for their review.\n\n[PATCH V1]:Most of the invaluable suggestions by Eric Sunshine, Torsten Bogershausen and\n\tJeff King has been implemented[1]. Complete rewrite of config_cache*() family\n\tusing git_config() as hook as suggested by Jeff. Thanks for the review.\n\n[RFC V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen.\n\tAdded cache invalidation when config file is changed.[2]\n\tI am using git_config_set_multivar_in_file() as an update hook.\n\nThis is patch is for this year's GSoC. My project is\n\"Git Config API improvements\". The link of my proposal is appended below [3].\n\nThe aim of this patch series is to generate a cache for querying values from\nthe config files in a non-callback manner as the current method reads and\nparses the config files every time a value is queried for.\n\nThe cache is generated from hooking the update_cache function to the current\nparsing and callback mechanism in config.c. It is implemented as an hashmap\nusing the hashmap-api with variables and its corresponding values list as\nits members. The values in the list are sorted in order of increasing priority.\nThe cache is initialised the first time when any of the new query functions is\ncalled. It is invalidated by using git_config_set_multivar_in_file() as an\nupdate hook.\n\nWe add two new functions to the config-api, git_config_get_string() and\ngit_config_get_string_multi() for querying in a non callback manner from\nthe cache.\n\n[1] http://marc.info/?t=140172066200006&r=1&w=2\n[2] http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html\n[3] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing\n[4] http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251369\n[5] http://thread.gmane.org/gmane.comp.version-control.git/251704/\n[6] http://thread.gmane.org/gmane.comp.version-control.git/252329/\n[7] http://marc.info/?t=140428115200001&r=1&w=2\n[8] http://article.gmane.org/gmane.comp.version-control.git/252942/\n[9] http://thread.gmane.org/gmane.comp.version-control.git/252959/\n[10] http://article.gmane.org/gmane.comp.version-control.git/253113\n\nTanay Abhra (2):\n config set\n test-config\n\n .gitignore | 1 +\n Documentation/technical/api-config.txt | 134 +++++++++++++++\n Makefile | 1 +\n cache.h | 31 ++++\n config.c | 296 +++++++++++++++++++++++++++++++++\n t/t1308-config-set.sh | 170 +++++++++++++++++++\n test-config.c | 125 ++++++++++++++\n 7 files changed, 758 insertions(+)\n create mode 100755 t/t1308-config-set.sh\n create mode 100644 test-config.c\n\n-- \n1.9.0.GIT\n"},{"id":"245753","messageId":"1405049655-4265-2-git-send-email-tanayabh@gmail.com","threadId":"37107","inReplyTo":"1405049655-4265-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-11T03:34:14Z","receivedAt":"2014-07-11T03:34:14Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Currently `git_config()` uses a callback mechanism and file rereads for\nconfig values. Due to this approach, it is not uncommon for the config\nfiles to be parsed several times during the run of a git program, with\ndifferent callbacks picking out different variables useful to themselves.\n\nAdd a `config_set`, that can be used to construct an in-memory cache for\nconfig-like files that the caller specifies (i.e., files like `.gitmodules`,\n`~/.gitconfig` etc.). Add two external functions `git_configset_get_value`\nand `git_configset_get_value_multi` for querying from the config sets.\n`git_configset_get_value` follows `last one wins` semantic (i.e. if there\nare multiple matches for the queried key in the files of the configset the\nvalue returned will be the last entry in `value_list`).\n`git_configset_get_value_multi` returns a list of values sorted in order of\nincreasing priority (i.e. last match will be at the end of the list). Add\ntype specific query functions like `git_configset_get_bool` and similar.\n\nAdd a default `config_set`, `the_config_set` to cache all key-value pairs\nread from usual config files (repo specific .git/config, user wide\n~/.gitconfig, XDG config and the global /etc/gitconfig). `the_config_set`\nis populated using `git_config()`.\n\nAdd two external functions `git_config_get_value` and \n`git_config_get_value_multi` for querying in a non-callback manner from\n`the_config_set`. Also, add type specific query functions that are\nimplemented as a thin wrapper around the `config_set` API.\n\nSigned-off-by: Tanay Abhra \n---\n Documentation/technical/api-config.txt | 134 +++++++++++++++\n cache.h | 31 ++++\n config.c | 296 +++++++++++++++++++++++++++++++++\n 3 files changed, 461 insertions(+)\n\ndiff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt\nindex 230b3a0..bb43830 100644\n--- a/Documentation/technical/api-config.txt\n+++ b/Documentation/technical/api-config.txt\n@@ -77,6 +77,81 @@ To read a specific file in git-config format, use\n `git_config_from_file`. This takes the same callback and data parameters\n as `git_config`.\n \n+Querying For Specific Variables\n+-------------------------------\n+\n+For programs wanting to query for specific variables in a non-callback\n+manner, the config API provides two functions `git_config_get_value`\n+and `git_config_get_value_multi`. They both read values from an internal\n+cache generated previously from reading the config files.\n+\n+`int git_config_get_value(const char *key, const char **value)`::\n+\n+\tFinds the highest-priority value for the configuration variable `key`,\n+\tstores the pointer to it in `value` and returns 0. When the\n+\tconfiguration variable `key` is not found, returns 1 without touching\n+\t`value`. The caller should not free or modify `value`, as it is owned\n+\tby the cache.\n+\n+`const struct string_list *git_config_get_value_multi(const char *key)`::\n+\n+\tFinds and returns the value list, sorted in order of increasing priority\n+\tfor the configuration variable `key`. When the configuration variable\n+\t`key` is not found, returns NULL. The caller should not free or modify\n+\tthe returned pointer, as it is owned by the cache.\n+\n+`void git_config_clear(void)`::\n+\n+\tResets and invalidates the config cache.\n+\n+The config API also provides type specific API functions which do conversion\n+as well as retrieval for the queried variable, including:\n+\n+`int git_config_get_int(const char *key, int *dest)`::\n+\n+\tFinds and parses the value to an integer for the configuration variable\n+\t`key`. Dies on error; otherwise, stores the value of the parsed integer in\n+\t`dest` and returns 0. When the configuration variable `key` is not found,\n+\treturns 1 without touching `dest`.\n+\n+`int git_config_get_ulong(const char *key, unsigned long *dest)`::\n+\n+\tSimilar to `git_config_get_int` but for unsigned longs.\n+\n+`int git_config_get_bool(const char *key, int *dest)`::\n+\n+\tFinds and parses the value into a boolean value, for the configuration\n+\tvariable `key` respecting keywords like \"true\" and \"false\". Integer\n+\tvalues are converted into true/false values (when they are non-zero or\n+\tzero, respectively). Other values cause a die(). If parsing is successful,\n+\tstores the value of the parsed result in `dest` and returns 0. When the\n+\tconfiguration variable `key` is not found, returns 1 without touching\n+\t`dest`.\n+\n+`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`::\n+\n+\tSimilar to `git_config_get_bool`, except that integers are copied as-is,\n+\tand `is_bool` flag is unset.\n+\n+`int git_config_get_maybe_bool(const char *key, int *dest)`::\n+\n+\tSimilar to `git_config_get_bool`, except that it returns -1 on error\n+\trather than dying.\n+\n+`int git_config_get_string(const char *key, const char **dest)`::\n+\n+\tAllocates and copies the retrieved string into the `dest` parameter for\n+\tthe configuration variable `key`; if NULL string is given, prints an\n+\terror message and returns -1. When the configuration variable `key` is\n+\tnot found, returns 1 without touching `dest`.\n+\n+`int git_config_get_pathname(const char *key, const char **dest)`::\n+\n+\tSimilar to `git_config_get_string`, but expands `~` or `~user` into\n+\tthe user's home directory when found at the beginning of the path.\n+\n+See test-config.c for usage examples.\n+\n Value Parsing Helpers\n ---------------------\n \n@@ -134,6 +209,65 @@ int read_file_with_include(const char *file, config_fn_t fn, void *data)\n `git_config` respects includes automatically. The lower-level\n `git_config_from_file` does not.\n \n+Custom Configsets\n+-----------------\n+\n+A `config_set` can be used to construct an in-memory cache for\n+config-like files that the caller specifies (i.e., files like `.gitmodules`,\n+`~/.gitconfig` etc.). For example,\n+\n+---------------------------------------\n+struct config_set gm_config;\n+git_configset_init(&gm_config);\n+int b;\n+/* we add config files to the config_set */\n+git_configset_add_file(&gm_config, \".gitmodules\");\n+git_configset_add_file(&gm_config, \".gitmodules_alt\");\n+\n+if (!git_configset_get_bool(gm_config, \"submodule.frotz.ignore\", &b)) {\n+\t/* hack hack hack */\n+}\n+\n+/* when we are done with the configset */\n+git_configset_clear(&gm_config);\n+----------------------------------------\n+\n+Configset API provides functions for the above mentioned work flow, including:\n+\n+`void git_configset_init(struct config_set *cs)`::\n+\n+\tInitializes the config_set `cs`.\n+\n+`int git_configset_add_file(struct config_set *cs, const char *filename)`::\n+\n+\tParses the file and adds the variable-value pairs to the `config_set`,\n+\tdies if there is an error in parsing the file.\n+\n+`int git_configset_get_value(struct config_set *cs, const char *key, const char **value)`::\n+\n+\tFinds the highest-priority value for the configuration variable `key`\n+\tand config set `cs`, stores the pointer to it in `value` and returns 0.\n+\tWhen the configuration variable `key` is not found, returns 1 without\n+\ttouching `value`. The caller should not free or modify `value`, as it\n+\tis owned by the cache.\n+\n+`const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)`::\n+\n+\tFinds and returns the value list, sorted in order of increasing priority\n+\tfor the configuration variable `key` and config set `cs`. When the\n+\tconfiguration variable `key` is not found, returns NULL. The caller\n+\tshould not free or modify the returned pointer, as it is owned by the cache.\n+\n+`void git_configset_clear(struct config_set *cs)`::\n+\n+\tClears `config_set` structure, removes all saved variable-value pairs.\n+\n+In addition to above functions, the `config_set` API provides type specific\n+functions in the vein of `git_config_get_int` and family but with an extra\n+parameter, pointer to struct `config_set`.\n+They all behave similarly to the `git_config_get*()` family described in\n+\"Querying For Specific Variables\" above.\n+\n Writing Config Files\n --------------------\n \ndiff --git a/cache.h b/cache.h\nindex df65231..75ec133 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1325,6 +1325,37 @@ 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+struct config_set {\n+\tstruct hashmap config_hash;\n+\tint hash_initialized;\n+};\n+\n+extern void git_configset_init(struct config_set *cs);\n+extern int git_configset_add_file(struct config_set *cs, const char *filename);\n+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);\n+extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n+extern void git_configset_clear(struct config_set *cs);\n+extern void git_configset_iter(struct config_set *cs, config_fn_t fn, void *data);\n+extern int git_configset_get_string(struct config_set *cs, const char *key, const char **dest);\n+extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);\n+extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);\n+extern int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);\n+extern int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);\n+\n+extern int git_config_get_value(const char *key, const char **value);\n+extern const struct string_list *git_config_get_value_multi(const char *key);\n+extern void git_config_clear(void);\n+extern void git_config_iter(config_fn_t fn, void *data);\n+extern int git_config_get_string(const char *key, const char **dest);\n+extern int git_config_get_int(const char *key, int *dest);\n+extern int git_config_get_ulong(const char *key, unsigned long *dest);\n+extern int git_config_get_bool(const char *key, int *dest);\n+extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);\n+extern int git_config_get_maybe_bool(const char *key, int *dest);\n+extern int git_config_get_pathname(const char *key, const char **dest);\n+\n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\n \ndiff --git a/config.c b/config.c\nindex ba882a1..aa58275 100644\n--- a/config.c\n+++ b/config.c\n@@ -9,6 +9,8 @@\n #include \"exec_cmd.h\"\n #include \"strbuf.h\"\n #include \"quote.h\"\n+#include \"hashmap.h\"\n+#include \"string-list.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -33,10 +35,23 @@ struct config_source {\n \tlong (*do_ftell)(struct config_source *c);\n };\n \n+struct config_hash_entry {\n+\tstruct hashmap_entry ent;\n+\tchar *key;\n+\tstruct string_list value_list;\n+};\n+\n static struct config_source *cf;\n \n static int zlib_compression_seen;\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, XDG\n+ * config file and the global /etc/gitconfig)\n+ */\n+static struct config_set the_config_set;\n+\n static int config_file_fgetc(struct config_source *conf)\n {\n \treturn fgetc(conf->u.file);\n@@ -1212,6 +1227,284 @@ int git_config(config_fn_t fn, void *data)\n \treturn git_config_with_options(fn, data, NULL, 1);\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 configset_init(struct config_set *cs)\n+{\n+\tif (!cs->hash_initialized) {\n+\t\thashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_hash_entry_cmp, 0);\n+\t\tcs->hash_initialized = 1;\n+\t}\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+int git_configset_add_file(struct config_set *cs, const char *filename)\n+{\n+\tint ret = 0;\n+\tconfigset_init(cs);\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+\t/*\n+\t * `key` may come from the user, so normalize it before using it\n+\t * for querying entries from the hashmap.\n+\t */\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+\t/*\n+\t * Since the keys 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+\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+\tstring_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n+\n+\treturn 0;\n+}\n+\n+void git_configset_init(struct config_set *cs)\n+{\n+\tcs->hash_initialized = 0;\n+}\n+\n+int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n+{\n+\tstruct string_list *values = NULL;\n+\t/*\n+\t * Follows \"last one wins\" semantic, i.e., if there are multiple matches for the\n+\t * queried key in the files of the configset, the value returned will be the last\n+\t * value in the value list for that key.\n+\t */\n+\tvalues = configset_get_list(key, cs);\n+\n+\tif (!values)\n+\t\treturn 1;\n+\tassert(values->nr > 0);\n+\t*value = values->items[values->nr - 1].string;\n+\treturn 0;\n+}\n+\n+const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n+{\n+\treturn configset_get_list(key, cs);\n+}\n+\n+void git_configset_clear(struct config_set *cs)\n+{\n+\tstruct config_hash_entry *entry;\n+\tstruct hashmap_iter iter;\n+\tif (!cs->hash_initialized)\n+\t\treturn;\n+\n+\thashmap_iter_init(&cs->config_hash, &iter);\n+\twhile ((entry = hashmap_iter_next(&iter))) {\n+\t\tfree(entry->key);\n+\t\tstring_list_clear(&entry->value_list, 0);\n+\t}\n+\thashmap_free(&cs->config_hash, 1);\n+}\n+\n+int git_configset_get_string(struct config_set *cs, const char *key, const char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value))\n+\t\treturn git_config_string(dest, key, value);\n+\telse\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_int(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_ulong(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_bool(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_bool(key, value);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_bool_or_int(struct config_set *cs, const char *key,\n+\t\t\t\tint *is_bool, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_bool_or_int(key, value, is_bool);\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value)) {\n+\t\t*dest = git_config_maybe_bool(key, value);\n+\t\tif (*dest == -1)\n+\t\t\treturn -1;\n+\t\treturn 0;\n+\t} else\n+\t\treturn 1;\n+}\n+\n+int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest)\n+{\n+\tconst char *value;\n+\tif (!git_configset_get_value(cs, key, &value))\n+\t\treturn git_config_pathname(dest, key, value);\n+\telse\n+\t\treturn 1;\n+}\n+\n+static int git_config_check_init(void)\n+{\n+\tint ret = 0;\n+\tif (the_config_set.hash_initialized)\n+\t\treturn 0;\n+\tconfigset_init(&the_config_set);\n+\tret = git_config(config_hash_callback, &the_config_set);\n+\tif (ret >= 0)\n+\t\treturn 0;\n+\telse {\n+\t\thashmap_free(&the_config_set.config_hash, 1);\n+\t\tthe_config_set.hash_initialized = 0;\n+\t\treturn -1;\n+\t}\n+}\n+\n+int git_config_get_value(const char *key, const char **value)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_value(&the_config_set, key, value);\n+}\n+\n+const struct string_list *git_config_get_value_multi(const char *key)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_value_multi(&the_config_set, key);\n+}\n+\n+void git_config_clear(void)\n+{\n+\tif (!the_config_set.hash_initialized)\n+\t\treturn;\n+\tgit_configset_clear(&the_config_set);\n+\tthe_config_set.hash_initialized = 0;\n+}\n+\n+int git_config_get_string(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_string(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_int(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_int(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_ulong(const char *key, unsigned long *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_ulong(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_bool(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_bool(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_bool_or_int(&the_config_set, key, is_bool, dest);\n+}\n+\n+int git_config_get_maybe_bool(const char *key, int *dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_maybe_bool(&the_config_set, key, dest);\n+}\n+\n+int git_config_get_pathname(const char *key, const char **dest)\n+{\n+\tgit_config_check_init();\n+\treturn git_configset_get_pathname(&the_config_set, key, dest);\n+}\n+\n /*\n * Find all the stuff for git_config_set() below.\n */\n@@ -1707,6 +2000,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":"245754","messageId":"1405049655-4265-3-git-send-email-tanayabh@gmail.com","threadId":"37107","inReplyTo":"1405049655-4265-1-git-send-email-tanayabh@gmail.com","subject":"[PATCH v8 2/2] test-config: add tests for the config_set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-11T03:34:15Z","receivedAt":"2014-07-11T03:34:15Z","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-set.sh | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++\n test-config.c | 125 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 297 insertions(+)\n create mode 100755 t/t1308-config-set.sh\n create mode 100644 test-config.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 42294e5..7677533 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -177,6 +177,7 @@\n /gitweb/static/gitweb.min.*\n /test-chmtime\n /test-ctype\n+/test-config\n /test-date\n /test-delta\n /test-dump-cache-tree\ndiff --git a/Makefile b/Makefile\nindex 07ea105..9544efb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))\n \n TEST_PROGRAMS_NEED_X += test-chmtime\n TEST_PROGRAMS_NEED_X += test-ctype\n+TEST_PROGRAMS_NEED_X += test-config\n TEST_PROGRAMS_NEED_X += test-date\n TEST_PROGRAMS_NEED_X += test-delta\n TEST_PROGRAMS_NEED_X += test-dump-cache-tree\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nnew file mode 100755\nindex 0000000..87a29f1\n--- /dev/null\n+++ b/t/t1308-config-set.sh\n@@ -0,0 +1,170 @@\n+#!/bin/sh\n+\n+test_description='Test git config-set API in different settings'\n+\n+. ./test-lib.sh\n+\n+# 'check section.key value' verifies that the entry for section.key is\n+# 'value'\n+check() {\n+\techo \"$2\" >expected &&\n+\ttest-config get_value \"$1\" >actual 2>&1 &&\n+\ttest_cmp expected actual\n+}\n+\n+test_expect_success 'setup default config' '\n+\tcat >.git/config << EOF\n+\t[core]\n+\t\tpenguin = very blue\n+\t\tMovie = BadPhysics\n+\t\tUPPERCASE = true\n+\t\tMixedCase = true\n+\t\tmy =\n+\t\tfoo\n+\t\tbaz = sam\n+\t[Cores]\n+\t\tWhatEver = Second\n+\t\tbaz = bar\n+\t[cores]\n+\t\tbaz = bat\n+\t[CORES]\n+\t\tbaz = ball\n+\t[my \"Foo bAr\"]\n+\t\thi = mixed-case\n+\t[my \"FOO BAR\"]\n+\t\thi = upper-case\n+\t[my \"foo bar\"]\n+\t\thi = lower-case\n+\t[core]\n+\t\tbaz = bat\n+\t\tbaz = hask\n+\t[lamb]\n+\t\tchop = 65\n+\t\thead = none\n+\t[goat]\n+\t\tlegs = 4\n+\t\thead = true\n+\t\tskin = false\n+\t\tnose = 1\n+\t\thorns\n+\tEOF\n+'\n+\n+test_expect_success 'get value for a simple key' '\n+\tcheck core.penguin \"very blue\"\n+'\n+\n+test_expect_success 'get value for a key with value as an empty string' '\n+\tcheck core.my \"\"\n+'\n+\n+test_expect_success 'get value for a key with value as NULL' '\n+\tcheck core.foo \"(NULL)\"\n+'\n+\n+test_expect_success 'upper case key' '\n+\tcheck core.UPPERCASE \"true\"\n+'\n+\n+test_expect_success 'mixed case key' '\n+\tcheck core.MixedCase \"true\"\n+'\n+\n+test_expect_success 'key and value with mixed case' '\n+\tcheck core.Movie \"BadPhysics\"\n+'\n+\n+test_expect_success 'key with case sensitive subsection' '\n+\tcheck \"my.Foo bAr.hi\" \"mixed-case\" &&\n+\tcheck \"my.FOO BAR.hi\" \"upper-case\" &&\n+\tcheck \"my.foo bar.hi\" \"lower-case\"\n+'\n+\n+test_expect_success 'key with case insensitive section header' '\n+\tcheck cores.baz \"ball\" &&\n+\tcheck Cores.baz \"ball\" &&\n+\tcheck CORES.baz \"ball\" &&\n+\tcheck coreS.baz \"ball\"\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+\tcheck core.baz \"hask\"\n+'\n+\n+test_expect_success 'find integer value for a key' '\n+\techo 65 >expect &&\n+\ttest-config get_int lamb.chop >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find integer if value is non parse-able' '\n+\techo 65 >expect &&\n+\ttest_must_fail test-config get_int lamb.head >actual &&\n+\ttest_must_fail test_cmp expect actual\n+'\n+\n+test_expect_success 'find bool value for the entered key' '\n+\tcat >expect <<-\\EOF &&\n+\t1\n+\t0\n+\t1\n+\t1\n+\t1\n+\tEOF\n+\ttest-config get_bool goat.head >actual &&\n+\ttest-config get_bool goat.skin >>actual &&\n+\ttest-config get_bool goat.nose >>actual &&\n+\ttest-config get_bool goat.horns >>actual &&\n+\ttest-config get_bool goat.legs >>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find multiple values' '\n+\tcat >expect <<-\\EOF &&\n+\tsam\n+\tbat\n+\thask\n+\tEOF\n+\ttest-config get_value_multi \"core.baz\">actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value from a configset' '\n+\tcat >config2 <<-\\EOF &&\n+\t[core]\n+\t\tbaz = lama\n+\t[my]\n+\t\tnew = silk\n+\t[core]\n+\t\tbaz = ball\n+\tEOF\n+\techo silk >expect &&\n+\ttest-config configset_get_value my.new config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value with highest priority from a configset' '\n+\techo hask > expect &&\n+\ttest-config configset_get_value core.baz config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'find value_list for a key from a configset' '\n+\tcat >except <<-\\EOF &&\n+\tsam\n+\tbat\n+\thask\n+\tlama\n+\tball\n+\tEOF\n+\ttest-config configset_get_value core.baz config2 .git/config >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/test-config.c b/test-config.c\nnew file mode 100644\nindex 0000000..dc313c2\n--- /dev/null\n+++ b/test-config.c\n@@ -0,0 +1,125 @@\n+#include \"cache.h\"\n+#include \"string-list.h\"\n+\n+/*\n+ * This program exposes the C API of the configuration mechanism\n+ * as a set of simple commands in order to facilitate testing.\n+ *\n+ * Reads stdin and prints result of command to stdout:\n+ *\n+ * get_value -> prints the value with highest priority for the entered key\n+ *\n+ * get_value_multi -> prints all values for the entered key in increasing order\n+ *\t\t of priority\n+ *\n+ * get_int -> print integer value for the entered key or die\n+ *\n+ * get_bool -> print bool value for the entered key or die\n+ *\n+ * configset_get_value -> returns value with the highest priority for the entered key\n+ * \t\t\tfrom a config_set constructed from files entered as arguments.\n+ *\n+ * configset_get_value_multi -> returns value_list for the entered key sorted in\n+ * \t\t\t\tascending order of priority from a config_set\n+ * \t\t\t\tconstructed from files entered as arguments.\n+ *\n+ * Examples:\n+ *\n+ * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n+ * \ttest-config get_value \"foo.bAr Baz.rock\"\n+ *\n+ */\n+\n+\n+int main(int argc, char **argv)\n+{\n+\tint i, val;\n+\tconst char *v;\n+\tconst struct string_list *strptr;\n+\tstruct config_set cs;\n+\tgit_configset_init(&cs);\n+\n+\tif (argc < 2) {\n+\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n+\t\treturn 1;\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_value\")) {\n+\t\tif (!git_config_get_value(argv[2], &v)) {\n+\t\t\tif (!v)\n+\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\telse\n+\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n+\t\tstrptr = git_config_get_value_multi(argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tif (!v)\n+\t\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\t}\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_int\")) {\n+\t\tif (!git_config_get_int(argv[2], &val)) {\n+\t\t\tprintf(\"%d\\n\", val);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t} else if (argc == 3 && !strcmp(argv[1], \"get_bool\")) {\n+\t\tif (!git_config_get_bool(argv[2], &val)) {\n+\t\t\tprintf(\"%d\\n\", val);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n+\t\tfor (i = 3; i < argc; i++) {\n+\t\t\tif (git_configset_add_file(&cs, argv[i]))\n+\t\t\t\treturn 2;\n+\t\t}\n+\t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n+\t\t\tif (!v)\n+\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\telse\n+\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n+\t\tfor (i = 3; i < argc; i++) {\n+\t\t\tif (git_configset_add_file(&cs, argv[i]))\n+\t\t\t\treturn 2;\n+\t\t}\n+\t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n+\t\tif (strptr) {\n+\t\t\tfor (i = 0; i < strptr->nr; i++) {\n+\t\t\t\tv = strptr->items[i].string;\n+\t\t\t\tif (!v)\n+\t\t\t\t\tprintf(\"(NULL)\\n\");\n+\t\t\t\telse\n+\t\t\t\t\tprintf(\"%s\\n\", v);\n+\t\t\t}\n+\t\t\treturn 0;\n+\t\t} else {\n+\t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n+\tfprintf(stderr, \"%s: Please check the syntax and the function name\\n\", argv[0]);\n+\treturn 1;\n+}\n-- \n1.9.0.GIT\n"},{"id":"245779","messageId":"vpqlhs02cz7.fsf@anie.imag.fr","threadId":"37107","inReplyTo":"1405049655-4265-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-11T14:21:00Z","receivedAt":"2014-07-11T14:21:00Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Hi,\n\nI had a closer look at error management (once more, sorry: I should have\ndone this earlier...), and it seems to me that:\n\n* Not all errors are managed properly\n\n* Most error cases are untested\n\nAmong the cases I can think of:\n\n* Syntax error when parsing the file\n\n* Non-existant file\n\n* Unreadable file (chmod -r)\n\nTanay Abhra writes:\n\n> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::\n> +\n> +\tParses the file and adds the variable-value pairs to the `config_set`,\n> +\tdies if there is an error in parsing the file.\n\nThe return value is undocumented.\n\nIf I read correctly, the only codepath from this to a syntax error sets\ndie_on_error, hence \"dies if there is an error in parsing the file\" is\ncorrect.\n\nStill, there are errors like \"unreadable file\" or \"no such file\" that do\nnot die (nor emit any error message, which is not very good for the\nuser), and lead to returning -1 here.\n\nI'm not sure this distinction is right (why die on syntax error and\ncontinue running on unreadable file?).\n\nIn any case, it should be documented and tested. I'll send a fixup patch\nwith a few more example tests (probably insufficient).\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> +\tconfigset_init(&the_config_set);\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\nWe have the same convention for errors here. But a more serious issue is\nthat the return value of this function is ignored most of the time.\n\nIt seems git_config should never return a negative value, as it calls\ngit_config_with_options -> git_config_early, which checks for file\nexistance and permission before calling git_config_from_file. Indeed,\nGit's tests still pass after this:\n\n--- a/config.c\n+++ b/config.c\n@@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,\n \n int git_config(config_fn_t fn, void *data)\n {\n- return git_config_with_options(fn, data, NULL, 1);\n+ int ret = git_config_with_options(fn, data, NULL, 1);\n+ if (ret < 0)\n+ die(\"Negative return value in git_config\");\n+ return ret;\n }\n\nStill, we can imagine cases like race condition between access_or_die()\nand git_config_from_file() in\n\n\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n\t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n\t\t\t\t\t data);\n\t\tfound += 1;\n\t}\n\nwhere the function would indeed return -1. In any case, either we\nconsider that git_config should never return -1, and we should die in\nthis case, or we consider that it may happen, and that the \"else\" branch\nof the if/else above is not dead code, and then we can't just ignore the\nreturn value.\n\nI think we should just do something like this:\n\ndiff --git a/config.c b/config.c\nindex 74adbbd..5c023e8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha\n return 1;\n }\n \n-static int git_config_check_init(void)\n+static void git_config_check_init(void)\n {\n int ret = 0;\n if (the_config_set.hash_initialized)\n@@ -1437,11 +1437,8 @@ static int git_config_check_init(void)\n ret = git_config(config_hash_callback, &the_config_set);\n if (ret >= 0)\n return 0;\n- else {\n- hashmap_free(&the_config_set.config_hash, 1);\n- the_config_set.hash_initialized = 0;\n- return -1;\n- }\n+ else\n+ die(\"Unknown error when parsing one of the configuration files.\");\n }\n \nIf not, a comment should explain what the \"else\" branch corresponds to,\nand why/when the return value can be safely ignored.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245781","messageId":"vpqwqbk0y8n.fsf@anie.imag.fr","threadId":"37107","inReplyTo":"1405049655-4265-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 2/2] test-config: add tests for the config_set API","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-11T14:24:40Z","receivedAt":"2014-07-11T14:24:40Z","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> diff --git a/test-config.c b/test-config.c\n> new file mode 100644\n> index 0000000..dc313c2\n> --- /dev/null\n> +++ b/test-config.c\n\n> +int main(int argc, char **argv)\n> +{\n> +\tint i, val;\n> +\tconst char *v;\n> +\tconst struct string_list *strptr;\n> +\tstruct config_set cs;\n> +\tgit_configset_init(&cs);\n\nThe configset is initialized, but never cleared.\n\nAs a result, valgrind --leak-check=full complains with \"definitely lost\"\nitems.\n\nI think it would make sense to apply something like this to get a\nvalgrind-clean test-config.c (I checked, it now passes without\nwarnings):\n\ndiff --git a/test-config.c b/test-config.c\nindex dc313c2..07b61ef 100644\n--- a/test-config.c\n+++ b/test-config.c\n@@ -41,17 +41,17 @@ int main(int argc, char **argv)\n \n \tif (argc < 2) {\n \t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n-\t\treturn 1;\n+\t\tgoto exit1;\n \t} else if (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\tgoto exit0;\n\n[...]\n \n \tfprintf(stderr, \"%s: Please check the syntax and the function name\\n\", argv[0]);\n+\tgoto exit1;\n+\n+exit0:\n+\tgit_configset_clear(&cs);\n+\treturn 0;\n+\n+exit1:\n+\tgit_configset_clear(&cs);\n \treturn 1;\n+\n+exit2:\n+\tgit_configset_clear(&cs);\n+\treturn 2;\n }\n\nI'll resend as a proper \"git am\"-able patch right after.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245783","messageId":"1405088833-5817-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"37107","inReplyTo":"vpqwqbk0y8n.fsf@anie.imag.fr","subject":"[fixup PATCH 1/2] Call configset_clear","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2014-07-11T14:27:12Z","receivedAt":"2014-07-11T14:27:12Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Signed-off-by: Matthieu Moy \n---\nConsider squashing this into PATCH 2/2.\n\n test-config.c | 42 +++++++++++++++++++++++++++---------------\n 1 file changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/test-config.c b/test-config.c\nindex dc313c2..07b61ef 100644\n--- a/test-config.c\n+++ b/test-config.c\n@@ -41,17 +41,17 @@ int main(int argc, char **argv)\n \n \tif (argc < 2) {\n \t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n-\t\treturn 1;\n+\t\tgoto exit1;\n \t} else if (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\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\treturn 1;\n+\t\t\tgoto exit1;\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@@ -63,46 +63,46 @@ int main(int argc, char **argv)\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\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\treturn 1;\n+\t\t\tgoto exit1;\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\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\treturn 1;\n+\t\t\tgoto exit1;\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\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\treturn 1;\n+\t\t\tgoto exit1;\n \t\t}\n \t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tif (git_configset_add_file(&cs, argv[i]))\n-\t\t\t\treturn 2;\n+\t\t\t\tgoto exit2;\n \t\t}\n \t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n \t\t\tif (!v)\n \t\t\t\tprintf(\"(NULL)\\n\");\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", v);\n-\t\t\treturn 0;\n+\t\t\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\treturn 1;\n+\t\t\tgoto exit1;\n \t\t}\n \t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n \t\t\tif (git_configset_add_file(&cs, argv[i]))\n-\t\t\t\treturn 2;\n+\t\t\t\tgoto exit2;\n \t\t}\n \t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n \t\tif (strptr) {\n@@ -113,13 +113,25 @@ int main(int argc, char **argv)\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\tgoto exit0;\n \t\t} else {\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n-\t\t\treturn 1;\n+\t\t\tgoto exit1;\n \t\t}\n \t}\n \n \tfprintf(stderr, \"%s: Please check the syntax and the function name\\n\", argv[0]);\n+\tgoto exit1;\n+\n+exit0:\n+\tgit_configset_clear(&cs);\n+\treturn 0;\n+\n+exit1:\n+\tgit_configset_clear(&cs);\n \treturn 1;\n+\n+exit2:\n+\tgit_configset_clear(&cs);\n+\treturn 2;\n }\n-- \n2.0.0.262.gdafc651\n"},{"id":"245782","messageId":"1405088833-5817-2-git-send-email-Matthieu.Moy@imag.fr","threadId":"37107","inReplyTo":"1405088833-5817-1-git-send-email-Matthieu.Moy@imag.fr","subject":"[PATCH 2/2] Better tests for error cases","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2014-07-11T14:27:13Z","receivedAt":"2014-07-11T14:27:13Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Signed-off-by: Matthieu Moy \n---\nConsider squashing this into PATCH 2/2\n\nProbably not sufficient.\n\n t/t1308-config-set.sh | 22 ++++++++++++++++++++++\n test-config.c | 8 ++++++--\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 87a29f1..f1e9e76 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -167,4 +167,26 @@ test_expect_success 'find value_list for a key from a configset' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'proper error on non-existant files' '\n+\techo \"Error reading configuration file non-existant-file.\" >expect &&\n+\ttest_must_fail test-config configset_get_value foo.bar non-existant-file 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'proper error on error in default config files' '\n+\tcp .git/config .git/config.old &&\n+\ttest_when_finished \"mv .git/config.old .git/config\" &&\n+\techo \"[\" >> .git/config &&\n+\techo \"fatal: bad config file line 35 in .git/config\" >expect &&\n+\ttest_must_fail test-config get_value foo.bar 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'proper error on error in custom config files' '\n+\techo \"[\" >> syntax-error &&\n+\techo \"fatal: bad config file line 1 in syntax-error\" >expect &&\n+\ttest_must_fail test-config configset_get_value foo.bar syntax-error 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/test-config.c b/test-config.c\nindex 07b61ef..49f8cd7 100644\n--- a/test-config.c\n+++ b/test-config.c\n@@ -86,8 +86,10 @@ int main(int argc, char **argv)\n \t\t}\n \t} else if (!strcmp(argv[1], \"configset_get_value\")) {\n \t\tfor (i = 3; i < argc; i++) {\n-\t\t\tif (git_configset_add_file(&cs, argv[i]))\n+\t\t\tif (git_configset_add_file(&cs, argv[i])) {\n+\t\t\t\tfprintf(stderr, \"Error reading configuration file %s.\\n\", argv[i]);\n \t\t\t\tgoto exit2;\n+\t\t\t}\n \t\t}\n \t\tif (!git_configset_get_value(&cs, argv[2], &v)) {\n \t\t\tif (!v)\n@@ -101,8 +103,10 @@ int main(int argc, char **argv)\n \t\t}\n \t} else if (!strcmp(argv[1], \"configset_get_value_multi\")) {\n \t\tfor (i = 3; i < argc; i++) {\n-\t\t\tif (git_configset_add_file(&cs, argv[i]))\n+\t\t\tif (git_configset_add_file(&cs, argv[i])) {\n+\t\t\t\tfprintf(stderr, \"Error reading configuration file %s.\\n\", argv[i]);\n \t\t\t\tgoto exit2;\n+\t\t\t}\n \t\t}\n \t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n \t\tif (strptr) {\n-- \n2.0.0.262.gdafc651\n"},{"id":"245792","messageId":"53C0116D.3010404@gmail.com","threadId":"37107","inReplyTo":"vpqlhs02cz7.fsf@anie.imag.fr","subject":"Re: [PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-11T16:31:41Z","receivedAt":"2014-07-11T16:31:41Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/11/2014 7:51 PM, Matthieu Moy wrote:\n> Hi,\n> \n> I had a closer look at error management (once more, sorry: I should have\n> done this earlier...), and it seems to me that:\n> \n> * Not all errors are managed properly\n> \n> * Most error cases are untested\n> \n> Among the cases I can think of:\n> \n> * Syntax error when parsing the file\n> \n> * Non-existant file\n> \n> * Unreadable file (chmod -r)\n>\n\nI had seen that there were checks for Syntax error or Non-existant files in\nt1300-repo-config, for example,\n\n# malformed configuration files\ntest_expect_success 'barf on syntax error' '\n\tcat >.git/config <<-\\EOF &&\n\t# broken section line\n\t[section]\n\tkey garbage\n\tEOF\n\ttest_must_fail git config --get section.key >actual 2>error &&\n\tgrep \" line 3 \" error\n'\n\ntest_expect_success 'barf on incomplete section header' '\n\tcat >.git/config <<-\\EOF &&\n\t# broken section line\n\t[section\n\tkey = value\n\tEOF\n\ttest_must_fail git config --get section.key >actual 2>error &&\n\tgrep \" line 2 \" error\n'\n\ntest_expect_success 'barf on incomplete string' '\n\tcat >.git/config <<-\\EOF &&\n\t# broken section line\n\t[section]\n\tkey = \"value string\n\tEOF\n\ttest_must_fail git config --get section.key >actual 2>error &&\n\tgrep \" line 3 \" error\n'\ntest_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '\n\ttest_must_fail git config --file non-existing-config -l\n'\n\nThey cover the same parsing code which I used for constructing the cache.\nStill, more tests will not harm anyone, I will add more testcases accordingly\nfor the corner cases you raised. :)\n\n> Tanay Abhra writes:\n> \n>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::\n>> +\n>> +\tParses the file and adds the variable-value pairs to the `config_set`,\n>> +\tdies if there is an error in parsing the file.\n> \n> The return value is undocumented.\n> \n> If I read correctly, the only codepath from this to a syntax error sets\n> die_on_error, hence \"dies if there is an error in parsing the file\" is\n> correct.\n> \n> Still, there are errors like \"unreadable file\" or \"no such file\" that do\n> not die (nor emit any error message, which is not very good for the\n> user), and lead to returning -1 here.\n> \n> I'm not sure this distinction is right (why die on syntax error and\n> continue running on unreadable file?).\n> \n> In any case, it should be documented and tested. I'll send a fixup patch\n> with a few more example tests (probably insufficient).\n>\n\nI am not sure of this myself, I will have to look into it.\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>> +\tconfigset_init(&the_config_set);\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> We have the same convention for errors here. But a more serious issue is\n> that the return value of this function is ignored most of the time.\n> \n> It seems git_config should never return a negative value, as it calls\n> git_config_with_options -> git_config_early, which checks for file\n> existance and permission before calling git_config_from_file. Indeed,\n> Git's tests still pass after this:\n> \n> --- a/config.c\n> +++ b/config.c\n> @@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,\n> \n> int git_config(config_fn_t fn, void *data)\n> {\n> - return git_config_with_options(fn, data, NULL, 1);\n> + int ret = git_config_with_options(fn, data, NULL, 1);\n> + if (ret < 0)\n> + die(\"Negative return value in git_config\");\n> + return ret;\n> }\n> \n> Still, we can imagine cases like race condition between access_or_die()\n> and git_config_from_file() in\n> \n> \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n> \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n> \t\t\t\t\t data);\n> \t\tfound += 1;\n> \t}\n> \n> where the function would indeed return -1. In any case, either we\n> consider that git_config should never return -1, and we should die in\n> this case, or we consider that it may happen, and that the \"else\" branch\n> of the if/else above is not dead code, and then we can't just ignore the\n> return value.\n> \n> I think we should just do something like this:\n> \n> diff --git a/config.c b/config.c\n> index 74adbbd..5c023e8 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha\n> return 1;\n> }\n> \n> -static int git_config_check_init(void)\n> +static void git_config_check_init(void)\n> {\n> int ret = 0;\n> if (the_config_set.hash_initialized)\n> @@ -1437,11 +1437,8 @@ static int git_config_check_init(void)\n> ret = git_config(config_hash_callback, &the_config_set);\n> if (ret >= 0)\n> return 0;\n> - else {\n> - hashmap_free(&the_config_set.config_hash, 1);\n> - the_config_set.hash_initialized = 0;\n> - return -1;\n> - }\n> + else\n> + die(\"Unknown error when parsing one of the configuration files.\");\n> }\n> \n> If not, a comment should explain what the \"else\" branch corresponds to,\n> and why/when the return value can be safely ignored.\n>\n\nYes, this is much better, I will make the necessary changes, thanks.\n"},{"id":"245796","messageId":"vpqwqbjvnkm.fsf@anie.imag.fr","threadId":"37107","inReplyTo":"53C0116D.3010404@gmail.com","subject":"Re: [PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-11T16:59:21Z","receivedAt":"2014-07-11T16:59:21Z","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> I had seen that there were checks for Syntax error or Non-existant files in\n> t1300-repo-config, for example,\n\nThe code raising the syntax error is there, and tested. But the way the\nerror code (eg. return -1 from git_config) is handled by your code is\nnot.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"245802","messageId":"xmqqfvi74xk5.fsf@gitster.dls.corp.google.com","threadId":"37107","inReplyTo":"1405049655-4265-2-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T17:25:46Z","receivedAt":"2014-07-11T17:25:46Z","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/config.c b/config.c\n> index ba882a1..aa58275 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -9,6 +9,8 @@\n> #include \"exec_cmd.h\"\n> #include \"strbuf.h\"\n> #include \"quote.h\"\n> +#include \"hashmap.h\"\n> +#include \"string-list.h\"\n> \n> struct config_source {\n> \tstruct config_source *prev;\n> @@ -33,10 +35,23 @@ struct config_source {\n> \tlong (*do_ftell)(struct config_source *c);\n> };\n> \n> +struct config_hash_entry {\n> +\tstruct hashmap_entry ent;\n> +\tchar *key;\n> +\tstruct string_list value_list;\n> +};\n> +\n> static struct config_source *cf;\n> \n> static int zlib_compression_seen;\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, XDG\n> + * config file and the global /etc/gitconfig)\n> + */\n> +static struct config_set the_config_set;\n> +\n> static int config_file_fgetc(struct config_source *conf)\n> {\n> \treturn fgetc(conf->u.file);\n> @@ -1212,6 +1227,284 @@ int git_config(config_fn_t fn, void *data)\n> \treturn git_config_with_options(fn, data, NULL, 1);\n> }\n> \n> +static int config_hash_add_value(const char *, const char *, struct hashmap *);\n\nThis is a funny forward declaration. If you define add-file and\nfriends that modify the config-set _after_ you define find-entry and\nfriends that are used to look-up, would you still need to do a\nforward declaration?\n\nIn any case, please give names to these first two parameters as what\nthey are for can be unclear; because they are of the same type,\nthere is one less clue than there usually are.\n\nThe function signature makes it sound as if this is not specific to\n\"config\"; what takes a hashmap, a key and a value and is called \"add\"\nis a function to add pair to a hashmap. Why doesn't it\ntake \"struct config_set\"? In other words, I would have expected\n\nstatic int config_set_add(struct config_set *, const char *key, const char *value)\n\ninstead. Not a complaint, but is puzzled why you chose not to do\nso.\n\nI suspect almost everywhere in this patch, you would want to do\ns/config_hash/config_set/. s/config_hash_entry/config_set_element/\nmight be a good idea, too. You have the concept of the \"config\nset\", and each element of that set is a \"config-set element\", not a\n\"config-hash entry\", isn't it?\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 configset_init(struct config_set *cs)\n> +{\n> +\tif (!cs->hash_initialized) {\n> +\t\thashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_hash_entry_cmp, 0);\n> +\t\tcs->hash_initialized = 1;\n> +\t}\n> +}\n\nHave uninitializer here, immediately after you defined the initializer.\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> +int git_configset_add_file(struct config_set *cs, const char *filename)\n> +{\n> +\tint ret = 0;\n> +\tconfigset_init(cs);\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\nDoes calling configset_clear() do too much for the purpose of this\nerror path? In other words, is it deliberate that you do not touch\nany string-list items you may have accumulated by calling the\ncallback?\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> +\t/*\n> +\t * `key` may come from the user, so normalize it before using it\n> +\t * for querying entries from the hashmap.\n> +\t */\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> +\t/*\n> +\t * Since the keys 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> +\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\nHmph, I thought you invented string_list_init() but perhaps it was\nsomebody else...\n\n> +\t\thashmap_add(config_hash, e);\n> +\t}\n> +\tstring_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);\n\nMental note: once \"string list\" is updated to make it a true\n\"string\" list for whatever reason, we would be broken by such a\nchange because we throw a NULL in there. Just something to watch\nout for in the future. Perhaps deserves a new comment in\nstring-list API to officially declare that such a use is supported\nand must continue to work? I dunno.\n\n> +\treturn 0;\n> +}\n> +\n> +void git_configset_init(struct config_set *cs)\n> +{\n> +\tcs->hash_initialized = 0;\n> +}\n\nThis is somewhat strange; as we are preparing cs for use, why not\ninitialize the config_hash member and flip hash_initialized to true\ninstead?\n\n> +int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n> +{\n> +\tstruct string_list *values = NULL;\n> +\t/*\n> +\t * Follows \"last one wins\" semantic, i.e., if there are multiple matches for the\n> +\t * queried key in the files of the configset, the value returned will be the last\n> +\t * value in the value list for that key.\n> +\t */\n> +\tvalues = configset_get_list(key, cs);\n> +\n> +\tif (!values)\n> +\t\treturn 1;\n> +\tassert(values->nr > 0);\n> +\t*value = values->items[values->nr - 1].string;\n> +\treturn 0;\n> +}\n> +\n> +const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n> +{\n> +\treturn configset_get_list(key, cs);\n> +}\n\nThis is OK, but I would have expected that configset_get_value(),\nknowing that its semantics is \"grab all and pick the last one\",\nwould call git_configset_get_value_multi() directly, which in turn\nwould mean there is no need for configset_get_list() intermediary.\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> +static int git_config_check_init(void)\n> +{\n> +\tint ret = 0;\n> +\tif (the_config_set.hash_initialized)\n> +\t\treturn 0;\n> +\tconfigset_init(&the_config_set);\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\nMove this deinitializer of the cached config system immediately\nafter git_config_check_init(), which is a lazy initializer.\n\nIt is somewhat unfortunate that git_config_get_* functions that are\nparallel to git_configset_get_* functions cannot be simple macros\nwith the fixed &the_config_set parameter due to lazy initialization,\nbut I think it is OK. This round mostly looks good to me, except\nfor the points I mentioned above.\n\nThanks.\n"},{"id":"245811","messageId":"xmqq8unz4v4c.fsf@gitster.dls.corp.google.com","threadId":"37107","inReplyTo":"1405049655-4265-3-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v8 2/2] test-config: add tests for the config_set API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-11T18:18:27Z","receivedAt":"2014-07-11T18:18:27Z","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/t/t1308-config-set.sh b/t/t1308-config-set.sh\n> new file mode 100755\n> index 0000000..87a29f1\n> --- /dev/null\n> +++ b/t/t1308-config-set.sh\n> @@ -0,0 +1,170 @@\n> +#!/bin/sh\n> +\n> +test_description='Test git config-set API in different settings'\n> +\n> +. ./test-lib.sh\n> +\n> +# 'check section.key value' verifies that the entry for section.key is\n> +# 'value'\n> +check() {\n\n(style) SP around both sides of ().\n\nMore importantly, will we ever have different kind of check in this\nscript, perhaps checking if all values for a multi-valued variables\nappear in the output, checking if get_bool works, etc. in the\nfuture? I would imagine the answer is yes, and in that case this\nshould be renamed to be a bit more specific (i.e. no \"too generic\"\nhelper called \"check\" would exist in the final result). Perhaps\ncall it \"check_single\", \"check_get_value\" or \"check_value\" (the last\none assumes that all your future checks are mostly about various\nforms of \"get\")?\n\n> +\techo \"$2\" >expected &&\n> +\ttest-config get_value \"$1\" >actual 2>&1 &&\n> +\ttest_cmp expected actual\n> +}\n> +\n> +test_expect_success 'setup default config' '\n> +\tcat >.git/config << EOF\n\n - No SP after redirection operator.\n\n - If you are not using variable substition in the here-doc, it is\n more friendly to quote the end-of-here-doc token to tell the\n reader that they do not have to worry about them.\n\n - There may be core.* variables that are crucial for correct\n operation of the version of Git being tested, so wiping and\n replacing .git/config wholesale is not a good idea. Appending\n your config items is sufficient for the purpose of these tests.\n\ni.e.\n\n\tcat >>.git/config <<\\EOF\n ...\n\tEOF\n\n> +\t[core]\n\nI'd feel safer if you did not abuse [core] like this. All you care\nabout is the config parsing, and you do not want future versions of\nGit introducing core.MixedCase to mean something meaningful that\naffects how \"git config\" and other commands work.\n\n> +\t\tpenguin = very blue\n> +\t\tMovie = BadPhysics\n> +\t\tUPPERCASE = true\n> +\t\tMixedCase = true\n> +\t\tmy =\n> ...\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'get value for a simple key' '\n> +\tcheck core.penguin \"very blue\"\n> +'\n> +\n> +test_expect_success 'get value for a key with value as an empty string' '\n> +\tcheck core.my \"\"\n> +'\n> +\n> +test_expect_success 'get value for a key with value as NULL' '\n> +\tcheck core.foo \"(NULL)\"\n> +'\n> +\n> +test_expect_success 'upper case key' '\n> +\tcheck core.UPPERCASE \"true\"\n> +'\n> +\n> +test_expect_success 'mixed case key' '\n> +\tcheck core.MixedCase \"true\"\n> +'\n\nYou would also need to see how\n\n\tcheck core.uppercase\n check core.MIXEDCASE\n\nbehave (after moving them out of \"core.\" hierarchy, of course), like\nthe following checks for case insensitivity of the first token. The\nfirst and the last token are both supposed to be case insensitive,\nno?\n\n> +test_expect_success 'key with case insensitive section header' '\n> +\tcheck cores.baz \"ball\" &&\n> +\tcheck Cores.baz \"ball\" &&\n> +\tcheck CORES.baz \"ball\" &&\n> +\tcheck coreS.baz \"ball\"\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\nIs test_must_fail suffice here? git_config_get_value() has two\nkinds of non-zero return values (one for \"error\", the other for \"not\nfound\"). Shouldn't test-config let the caller tell these two kinds\napart in some way? It would be reasonable to do so with its exit\nstatus, I would imagine, and in that case, test_expect_code may be\nmore appropriate here.\n\n> +\ttest_cmp expect actual\n\nAre you sure the expect and actual should match here?\n\nOh by the way, in \"check()\" helper shell function you spelled\nthe correct answer \"expected\" but here you use \"expect\" (like almost\nall the other existing tests). Perhaps s/expected/expect/ while we\nfix the helper function?\n\n> +'\n> +\n> +test_expect_success 'find value with the highest priority' '\n> +\tcheck core.baz \"hask\"\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\nPerhaps\n\n\tcheck_config () {\n\t\top=$1 key=$2 &&\n shift &&\n shift &&\n printf \"%s\\n\" \"$@\" >expect &&\n test-config \"$op\" \"$key\" >actual &&\n\t\ttest_cmp expect actual\n\t}\n\nand use it like so?\n\n\tcheck_config get_value core.mixedcase true\n check_config get_int lamb.chop 65\n check_config get_bool goat.head 1\n check_config get_value_multi core.baz sam bat hask\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\nDo not use test_must_fail on anything other than \"git\" command.\nWorse yet, you are not just interested in seeing expect and actual\ndiffer. When get_int finds something that is not an integer, you do\nnot just expect the output from the command to be any random string\nthat is not the correct answer. You expect it to be empty, no?\n\n\t>expect &&\n\ttest_expect_code 1 test-config get_int lamb.head >actual &&\n test_cmp expect actual\n\nor something (assuming that you chose to exit with 1 when you get an\nerror, but I didn't check).\n\nExtending the check_config helper a bit more, perhaps\n\n\tcheck_config () {\n\t\tcase \"$1\" in\n fail)\n\t\t\t>expect &&\n test_expect_code 1 test-config \"$2\" \"$3\" >actual\n \t;;\n\t\t*)\n \top=$1 key=$2 &&\n shift &&\n shift &&\n\t printf \"%s\\n\" \"$@\" >expect &&\n\t test-config \"$op\" \"$key\" >actual\n\t\t\t;;\n\t\tesac &&\n test_cmp expect actual\n\t}\n\nor something like that?\n\n> diff --git a/test-config.c b/test-config.c\n> new file mode 100644\n> index 0000000..dc313c2\n> --- /dev/null\n> +++ b/test-config.c\n> @@ -0,0 +1,125 @@\n> +#include \"cache.h\"\n> +#include \"string-list.h\"\n> +\n> +/*\n> + * This program exposes the C API of the configuration mechanism\n> + * as a set of simple commands in order to facilitate testing.\n> + *\n> + * Reads stdin and prints result of command to stdout:\n> + *\n> + * get_value -> prints the value with highest priority for the entered key\n> + *\n> + * get_value_multi -> prints all values for the entered key in increasing order\n> + *\t\t of priority\n> + *\n> + * get_int -> print integer value for the entered key or die\n> + *\n> + * get_bool -> print bool value for the entered key or die\n> + *\n> + * configset_get_value -> returns value with the highest priority for the entered key\n> + * \t\t\tfrom a config_set constructed from files entered as arguments.\n> + *\n> + * configset_get_value_multi -> returns value_list for the entered key sorted in\n> + * \t\t\t\tascending order of priority from a config_set\n> + * \t\t\t\tconstructed from files entered as arguments.\n> + *\n> + * Examples:\n> + *\n> + * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n> + * \ttest-config get_value \"foo.bAr Baz.rock\"\n> + *\n> + */\n> +\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tint i, val;\n> +\tconst char *v;\n> +\tconst struct string_list *strptr;\n> +\tstruct config_set cs;\n> +\tgit_configset_init(&cs);\n> +\n> +\tif (argc < 2) {\n> +\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n> +\t\treturn 1;\n> +\t} else if (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\nThis one is dubious. Is this for things like\n\n\t(in .git/config)\n\t[receive]\n \tfsckobjects\n\nand asking with\n\n\t$ git config receive.fsckobjects\n\nwhich I think gives an empty string? We may want to be consistent.\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\nSo \"not found\" is signalled with exit code 1. die() coming from\nother errors will give us something like 128, and you exit with 2\nwhen add-file fails (below), so the caller indeed can tell these\ncases apart.\n\nThe tests that use test_must_fail in your test scripts should be\nupdated to use test_expect_code then.\n\n> +\t\t}\n> +\t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n> +...\n> +\t}\n> +\n> +\tfprintf(stderr, \"%s: Please check the syntax and the function name\\n\", argv[0]);\n> +\treturn 1;\n\nThis is an error of different kind, no? Use a different exit code\nfor it. Instead of fprintf/return, using die() is fine here.\n\n> +}\n"},{"id":"246046","messageId":"53C50859.5070209@gmail.com","threadId":"37107","inReplyTo":"vpqlhs02cz7.fsf@anie.imag.fr","subject":"Re: [PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-15T10:54:17Z","receivedAt":"2014-07-15T10:54:17Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/11/2014 7:51 PM, Matthieu Moy wrote:\n>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::\n>> +\n>> +\tParses the file and adds the variable-value pairs to the `config_set`,\n>> +\tdies if there is an error in parsing the file.\n> \n> The return value is undocumented.\n> \n> If I read correctly, the only codepath from this to a syntax error sets\n> die_on_error, hence \"dies if there is an error in parsing the file\" is\n> correct.\n> \n> Still, there are errors like \"unreadable file\" or \"no such file\" that do\n> not die (nor emit any error message, which is not very good for the\n> user), and lead to returning -1 here.\n> \n> I'm not sure this distinction is right (why die on syntax error and\n> continue running on unreadable file?).\n> \n\nI checked the whole codebase and in all of the cases if they cannot read a file\nthey return -1 and continue running. So, I have updated the API to document the\nreturn value.\n\nI think if the file is unreadable. we must continue running as no harm has been\ndone yet, worse is parsing a file with wrong syntax which can cause reading\nwrong config values. So the decision to die on syntax error sounds alright\nto me.\n\nCheers,\nTanay Abhra.\n"},{"id":"246047","messageId":"loom.20140715T125714-160@post.gmane.org","threadId":"37107","inReplyTo":"xmqq8unz4v4c.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v8 2/2] test-config: add tests for the config_set API","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-15T11:10:26Z","receivedAt":"2014-07-15T11:10:26Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"Junio C Hamano pobox.com> writes:\n\n> \n> Tanay Abhra gmail.com> writes:\n> \n> > diff --git a/test-config.c b/test-config.c\n> > new file mode 100644\n> > index 0000000..dc313c2\n> > --- /dev/null\n> > +++ b/test-config.c\n> > -0,0 +1,125 \n> > +\n> > +\n> > +int main(int argc, char **argv)\n> > +{\n> > +\tint i, val;\n> > +\tconst char *v;\n> > +\tconst struct string_list *strptr;\n> > +\tstruct config_set cs;\n> > +\tgit_configset_init(&cs);\n> > +\n> > +\tif (argc < 2) {\n> > +\t\tfprintf(stderr, \"Please, provide a command name on the command-line\\n\");\n> > +\t\treturn 1;\n> > +\t} else if (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> \n> This one is dubious. Is this for things like\n> \n> \t(in .git/config)\n> \t[receive]\n> \tfsckobjects\n>\n\nYes, it was meant for the above case.\n\n> and asking with\n> \n> \t$ git config receive.fsckobjects\n> \n> which I think gives an empty string? We may want to be consistent.\n\n$ git config -l\nshows NULL values as foo.bar\nempty values as foo.bar=\nSo there is a difference between the two.\n$ git config receive.fsckobjects does covert the NULL value to a \"\"(empty\nstring).\n\nI had to diffrentiate between the two, so I took the path printf takes for\nNULL strings, it prints them as \"(NULL)\".\n"},{"id":"246049","messageId":"vpqiomy3li1.fsf@anie.imag.fr","threadId":"37107","inReplyTo":"53C50859.5070209@gmail.com","subject":"Re: [PATCH v8 1/2] add `config_set` API for caching config-like files","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-15T11:32:54Z","receivedAt":"2014-07-15T11:32:54Z","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> I checked the whole codebase and in all of the cases if they cannot read a file\n> they return -1 and continue running.\n\nMore precisely: in git_config_from_file, any fopen failure results in\n\"return -1\". But in git_config_early (one caller of\ngit_config_from_file()), the file is checked before access, e.g.:\n\n\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n\t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n\t\t\t\t\t data);\n\t\tfound += 1;\n\t}\n\nEssentially, if a config file does not exist, it's skipped (obviously\ndesirable), but if some really weird error occur (if \"err == ENOENT ||\nerr == ENOTDIR || ((flag & ACCESS_EACCES_OK) && err == EACCES\" is false,\nfrom access_eacces_ok() in wrapper.c), then the process dies.\n\n\"Permission denied\" errors are allowed for user-wide config, but not for\nothers. Read the log for commit 4698c8feb for more details.\n\nAnyway, this is the part of the code you're not touching.\n\n(I actually consider it as a bug that \"git config --file no-such-file\nfoo.bar\" and \"git config --file unreadable-file foo.bar\" fail without\nerror message, but your commit does not change this).\n\n> I think if the file is unreadable. we must continue running as no harm has been\n> done yet, worse is parsing a file with wrong syntax which can cause reading\n> wrong config values. So the decision to die on syntax error sounds alright\n> to me.\n\nIn the case of git_config_check_init(), I think it makes sense to die,\nbecause file accesses are protected with access_or_die(), so the return\nvalue can be negative only if something really went wrong.\n\nIf you chose not to die, then you should check the return value in the\ncallers of git_config_check_init().\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}