[PATCH v8 0/3] git config cache & special querying api utilizing the cache

[PATCH v8 0/3] git config cache & special querying api utilizing the cache

[Tanay Abhra]

Hi,

[PATCH V8]: Moved the contents of config-set.c to config.c for future convenience. Reverted
	test 'find value with misspelled key' to the one in v5. See [10] for the discussion.

[PATCH V7]: Style nits and a broken && chain corrected in `t/t1308-config-set.sh`. See
	[9] for the nits.

[PATCH V6]: Style nits and mistakes corrected. Diff between v6 and v5[8] is at the bottom.
		Thanks to Matthieu, Ramsay and Ram for their suggestions.

[PATCH V5]: `config_set` now uses a single hashmap. Corrected style nits raised in
			the thread[7]. Thanks to Junio and Matthieu for their suggestions.

[PATCH v4]: Introduced `config_set` construct which points to a ordered set of
	config-files cached as hashmaps. Added relevant API functions. For more
	details see the documentation. Rewrote the git_config_get* family to use
	`config_set` internally. Added tests for both config_set API and git_config_get
	family. Added type specific API functions which parses the found value and
	converts it into a specific type.
	Most of the changes implemented are the result of discussion in [6].
	Thanks to Eric, Ramsay, Junio, Matthieu & Karsten for their suggestions
	and review.

[PATCH v3]: Added flag for NULL values that were causing segfaults in some cases.
	Added test-config for usage examples.
	Minor changes and corrections. Refer to discussion thread[5] for more details.
	Thanks to Matthieu, Jeff and Junio for their valuable suggestions.

[PATCH v2]:Changed the string_list to a struct instead of pointer to a struct.
	Added string-list initilization functions.
	Minor mistakes corrected acoording to review comments[4]. Thanks to
	Eric and Matthieu for their review.

[PATCH V1]:Most of the invaluable suggestions by Eric Sunshine, Torsten Bogershausen and
	Jeff King has been implemented[1]. Complete rewrite of config_cache*() family
	using git_config() as hook as suggested by Jeff. Thanks for the review.

[RFC V2]: Improved according to the suggestions by Eric Sunshine and Torsten Bogershausen.
	Added cache invalidation when config file is changed.[2]
	I am using git_config_set_multivar_in_file() as an update hook.

This is patch is for this year's GSoC. My project is
"Git Config API improvements". The link of my proposal is appended below [3].

The aim of this patch series is to generate a cache for querying values from
the config files in a non-callback manner as the current method reads and
parses the config files every time a value is queried for.

The cache is generated from hooking the update_cache function to the current
parsing and callback mechanism in config.c. It is implemented as an hashmap
using the hashmap-api with variables and its corresponding values list as
its members. The values in the list are sorted in order of increasing priority.
The cache is initialised the first time when any of the new query functions is
called. It is invalidated by using git_config_set_multivar_in_file() as an
update hook.

We add two new functions to the config-api, git_config_get_string() and
git_config_get_string_multi() for querying in a non callback manner from
the cache.

[1] http://marc.info/?t=140172066200006&r=1&w=2
[2] http://git.661346.n2.nabble.com/RFC-PATCH-0-2-Git-config-cache-amp-special-querying-api-utilizing-the-cache-td7611691.html
[3] https://drive.google.com/file/d/0B4suZ-aHqDcnSUZJRXVTTnZUN1E/edit?usp=sharing
[4] http://thread.gmane.org/gmane.comp.version-control.git/251073/focus=251369
[5] http://thread.gmane.org/gmane.comp.version-control.git/251704/
[6] http://thread.gmane.org/gmane.comp.version-control.git/252329/
[7] http://marc.info/?t=140428115200001&r=1&w=2
[8] http://article.gmane.org/gmane.comp.version-control.git/252942/
[9] http://thread.gmane.org/gmane.comp.version-control.git/252959/
[10] http://article.gmane.org/gmane.comp.version-control.git/253113

Tanay Abhra (2):
  config set
  test-config

 .gitignore                             |   1 +
 Documentation/technical/api-config.txt | 134 +++++++++++++++
 Makefile                               |   1 +
 cache.h                                |  31 ++++
 config.c                               | 296 +++++++++++++++++++++++++++++++++
 t/t1308-config-set.sh                  | 170 +++++++++++++++++++
 test-config.c                          | 125 ++++++++++++++
 7 files changed, 758 insertions(+)
 create mode 100755 t/t1308-config-set.sh
 create mode 100644 test-config.c

-- 
1.9.0.GIT

[PATCH v8 1/2] add `config_set` API for caching config-like files

[Tanay Abhra]

Currently `git_config()` uses a callback mechanism and file rereads for
config values. Due to this approach, it is not uncommon for the config
files to be parsed several times during the run of a git program, with
different callbacks picking out different variables useful to themselves.

Add a `config_set`, that can be used to construct an in-memory cache for
config-like files that the caller specifies (i.e., files like `.gitmodules`,
`~/.gitconfig` etc.). Add two external functions `git_configset_get_value`
and `git_configset_get_value_multi` for querying from the config sets.
`git_configset_get_value` follows `last one wins` semantic (i.e. if there
are multiple matches for the queried key in the files of the configset the
value returned will be the last entry in `value_list`).
`git_configset_get_value_multi` returns a list of values sorted in order of
increasing priority (i.e. last match will be at the end of the list). Add
type specific query functions like `git_configset_get_bool` and similar.

Add a default `config_set`, `the_config_set` to cache all key-value pairs
read from usual config files (repo specific .git/config, user wide
~/.gitconfig, XDG config and the global /etc/gitconfig). `the_config_set`
is populated using `git_config()`.

Add two external functions `git_config_get_value` and 
`git_config_get_value_multi` for querying in a non-callback manner from
`the_config_set`. Also, add type specific query functions that are
implemented as a thin wrapper around the `config_set` API.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt | 134 +++++++++++++++
 cache.h                                |  31 ++++
 config.c                               | 296 +++++++++++++++++++++++++++++++++
 3 files changed, 461 insertions(+)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 230b3a0..bb43830 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -77,6 +77,81 @@ To read a specific file in git-config format, use
 `git_config_from_file`. This takes the same callback and data parameters
 as `git_config`.
 
+Querying For Specific Variables
+-------------------------------
+
+For programs wanting to query for specific variables in a non-callback
+manner, the config API provides two functions `git_config_get_value`
+and `git_config_get_value_multi`. They both read values from an internal
+cache generated previously from reading the config files.
+
+`int git_config_get_value(const char *key, const char **value)`::
+
+	Finds the highest-priority value for the configuration variable `key`,
+	stores the pointer to it in `value` and returns 0. When the
+	configuration variable `key` is not found, returns 1 without touching
+	`value`. The caller should not free or modify `value`, as it is owned
+	by the cache.
+
+`const struct string_list *git_config_get_value_multi(const char *key)`::
+
+	Finds and returns the value list, sorted in order of increasing priority
+	for the configuration variable `key`. When the configuration variable
+	`key` is not found, returns NULL. The caller should not free or modify
+	the returned pointer, as it is owned by the cache.
+
+`void git_config_clear(void)`::
+
+	Resets and invalidates the config cache.
+
+The config API also provides type specific API functions which do conversion
+as well as retrieval for the queried variable, including:
+
+`int git_config_get_int(const char *key, int *dest)`::
+
+	Finds and parses the value to an integer for the configuration variable
+	`key`. Dies on error; otherwise, stores the value of the parsed integer in
+	`dest` and returns 0. When the configuration variable `key` is not found,
+	returns 1 without touching `dest`.
+
+`int git_config_get_ulong(const char *key, unsigned long *dest)`::
+
+	Similar to `git_config_get_int` but for unsigned longs.
+
+`int git_config_get_bool(const char *key, int *dest)`::
+
+	Finds and parses the value into a boolean value, for the configuration
+	variable `key` respecting keywords like "true" and "false". Integer
+	values are converted into true/false values (when they are non-zero or
+	zero, respectively). Other values cause a die(). If parsing is successful,
+	stores the value of the parsed result in `dest` and returns 0. When the
+	configuration variable `key` is not found, returns 1 without touching
+	`dest`.
+
+`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`::
+
+	Similar to `git_config_get_bool`, except that integers are copied as-is,
+	and `is_bool` flag is unset.
+
+`int git_config_get_maybe_bool(const char *key, int *dest)`::
+
+	Similar to `git_config_get_bool`, except that it returns -1 on error
+	rather than dying.
+
+`int git_config_get_string(const char *key, const char **dest)`::
+
+	Allocates and copies the retrieved string into the `dest` parameter for
+	the configuration variable `key`; if NULL string is given, prints an
+	error message and returns -1. When the configuration variable `key` is
+	not found, returns 1 without touching `dest`.
+
+`int git_config_get_pathname(const char *key, const char **dest)`::
+
+	Similar to `git_config_get_string`, but expands `~` or `~user` into
+	the user's home directory when found at the beginning of the path.
+
+See test-config.c for usage examples.
+
 Value Parsing Helpers
 ---------------------
 
@@ -134,6 +209,65 @@ int read_file_with_include(const char *file, config_fn_t fn, void *data)
 `git_config` respects includes automatically. The lower-level
 `git_config_from_file` does not.
 
+Custom Configsets
+-----------------
+
+A `config_set` can be used to construct an in-memory cache for
+config-like files that the caller specifies (i.e., files like `.gitmodules`,
+`~/.gitconfig` etc.). For example,
+
+---------------------------------------
+struct config_set gm_config;
+git_configset_init(&gm_config);
+int b;
+/* we add config files to the config_set */
+git_configset_add_file(&gm_config, ".gitmodules");
+git_configset_add_file(&gm_config, ".gitmodules_alt");
+
+if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore", &b)) {
+	/* hack hack hack */
+}
+
+/* when we are done with the configset */
+git_configset_clear(&gm_config);
+----------------------------------------
+
+Configset API provides functions for the above mentioned work flow, including:
+
+`void git_configset_init(struct config_set *cs)`::
+
+	Initializes the config_set `cs`.
+
+`int git_configset_add_file(struct config_set *cs, const char *filename)`::
+
+	Parses the file and adds the variable-value pairs to the `config_set`,
+	dies if there is an error in parsing the file.
+
+`int git_configset_get_value(struct config_set *cs, const char *key, const char **value)`::
+
+	Finds the highest-priority value for the configuration variable `key`
+	and config set `cs`, stores the pointer to it in `value` and returns 0.
+	When the configuration variable `key` is not found, returns 1 without
+	touching `value`. The caller should not free or modify `value`, as it
+	is owned by the cache.
+
+`const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)`::
+
+	Finds and returns the value list, sorted in order of increasing priority
+	for the configuration variable `key` and config set `cs`. When the
+	configuration variable `key` is not found, returns NULL. The caller
+	should not free or modify the returned pointer, as it is owned by the cache.
+
+`void git_configset_clear(struct config_set *cs)`::
+
+	Clears `config_set` structure, removes all saved variable-value pairs.
+
+In addition to above functions, the `config_set` API provides type specific
+functions in the vein of `git_config_get_int` and family but with an extra
+parameter, pointer to struct `config_set`.
+They all behave similarly to the `git_config_get*()` family described in
+"Querying For Specific Variables" above.
+
 Writing Config Files
 --------------------
 
diff --git a/cache.h b/cache.h
index df65231..75ec133 100644
--- a/cache.h
+++ b/cache.h
@@ -1325,6 +1325,37 @@ extern int parse_config_key(const char *var,
 			    const char **subsection, int *subsection_len,
 			    const char **key);
 
+struct config_set {
+	struct hashmap config_hash;
+	int hash_initialized;
+};
+
+extern void git_configset_init(struct config_set *cs);
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);
+extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);
+extern void git_configset_clear(struct config_set *cs);
+extern void git_configset_iter(struct config_set *cs, config_fn_t fn, void *data);
+extern int git_configset_get_string(struct config_set *cs, const char *key, const char **dest);
+extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);
+extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);
+extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);
+extern int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);
+extern int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);
+extern int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);
+
+extern int git_config_get_value(const char *key, const char **value);
+extern const struct string_list *git_config_get_value_multi(const char *key);
+extern void git_config_clear(void);
+extern void git_config_iter(config_fn_t fn, void *data);
+extern int git_config_get_string(const char *key, const char **dest);
+extern int git_config_get_int(const char *key, int *dest);
+extern int git_config_get_ulong(const char *key, unsigned long *dest);
+extern int git_config_get_bool(const char *key, int *dest);
+extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
+extern int git_config_get_maybe_bool(const char *key, int *dest);
+extern int git_config_get_pathname(const char *key, const char **dest);
+
 extern int committer_ident_sufficiently_given(void);
 extern int author_ident_sufficiently_given(void);
 
diff --git a/config.c b/config.c
index ba882a1..aa58275 100644
--- a/config.c
+++ b/config.c
@@ -9,6 +9,8 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 #include "quote.h"
+#include "hashmap.h"
+#include "string-list.h"
 
 struct config_source {
 	struct config_source *prev;
@@ -33,10 +35,23 @@ struct config_source {
 	long (*do_ftell)(struct config_source *c);
 };
 
+struct config_hash_entry {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};
+
 static struct config_source *cf;
 
 static int zlib_compression_seen;
 
+/*
+ * Default config_set that contains key-value pairs from the usual set of config
+ * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG
+ * config file and the global /etc/gitconfig)
+ */
+static struct config_set the_config_set;
+
 static int config_file_fgetc(struct config_source *conf)
 {
 	return fgetc(conf->u.file);
@@ -1212,6 +1227,284 @@ int git_config(config_fn_t fn, void *data)
 	return git_config_with_options(fn, data, NULL, 1);
 }
 
+static int config_hash_add_value(const char *, const char *, struct hashmap *);
+
+static int config_hash_entry_cmp(const struct config_hash_entry *e1,
+				 const struct config_hash_entry *e2, const void *unused)
+{
+	return strcmp(e1->key, e2->key);
+}
+
+static void configset_init(struct config_set *cs)
+{
+	if (!cs->hash_initialized) {
+		hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_hash_entry_cmp, 0);
+		cs->hash_initialized = 1;
+	}
+}
+
+static int config_hash_callback(const char *key, const char *value, void *cb)
+{
+	struct config_set *cs = cb;
+	config_hash_add_value(key, value, &cs->config_hash);
+	return 0;
+}
+
+int git_configset_add_file(struct config_set *cs, const char *filename)
+{
+	int ret = 0;
+	configset_init(cs);
+	ret = git_config_from_file(config_hash_callback, filename, cs);
+	if (!ret)
+		return 0;
+	else {
+		hashmap_free(&cs->config_hash, 1);
+		cs->hash_initialized = 0;
+		return -1;
+	}
+}
+
+static struct config_hash_entry *config_hash_find_entry(const char *key,
+							struct hashmap *config_hash)
+{
+	struct config_hash_entry k;
+	struct config_hash_entry *found_entry;
+	char *normalized_key;
+	int ret;
+	/*
+	 * `key` may come from the user, so normalize it before using it
+	 * for querying entries from the hashmap.
+	 */
+	ret = git_config_parse_key(key, &normalized_key, NULL);
+
+	if (ret)
+		return NULL;
+
+	hashmap_entry_init(&k, strhash(normalized_key));
+	k.key = normalized_key;
+	found_entry = hashmap_get(config_hash, &k, NULL);
+	free(normalized_key);
+	return found_entry;
+}
+
+static struct string_list *configset_get_list(const char *key, struct config_set *cs)
+{
+	struct config_hash_entry *e = config_hash_find_entry(key, &cs->config_hash);
+	return e ? &e->value_list : NULL;
+}
+
+static int config_hash_add_value(const char *key, const char *value, struct hashmap *config_hash)
+{
+	struct config_hash_entry *e;
+	e = config_hash_find_entry(key, config_hash);
+	/*
+	 * Since the keys are being fed by git_config*() callback mechanism, they
+	 * are already normalized. So simply add them without any further munging.
+	 */
+	if (!e) {
+		e = xmalloc(sizeof(*e));
+		hashmap_entry_init(e, strhash(key));
+		e->key = xstrdup(key);
+		memset(&e->value_list, 0, sizeof(e->value_list));
+		e->value_list.strdup_strings = 1;
+		hashmap_add(config_hash, e);
+	}
+	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+
+	return 0;
+}
+
+void git_configset_init(struct config_set *cs)
+{
+	cs->hash_initialized = 0;
+}
+
+int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
+{
+	struct string_list *values = NULL;
+	/*
+	 * Follows "last one wins" semantic, i.e., if there are multiple matches for the
+	 * queried key in the files of the configset, the value returned will be the last
+	 * value in the value list for that key.
+	 */
+	values = configset_get_list(key, cs);
+
+	if (!values)
+		return 1;
+	assert(values->nr > 0);
+	*value = values->items[values->nr - 1].string;
+	return 0;
+}
+
+const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
+{
+	return configset_get_list(key, cs);
+}
+
+void git_configset_clear(struct config_set *cs)
+{
+	struct config_hash_entry *entry;
+	struct hashmap_iter iter;
+	if (!cs->hash_initialized)
+		return;
+
+	hashmap_iter_init(&cs->config_hash, &iter);
+	while ((entry = hashmap_iter_next(&iter))) {
+		free(entry->key);
+		string_list_clear(&entry->value_list, 0);
+	}
+	hashmap_free(&cs->config_hash, 1);
+}
+
+int git_configset_get_string(struct config_set *cs, const char *key, const char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value))
+		return git_config_string(dest, key, value);
+	else
+		return 1;
+}
+
+int git_configset_get_int(struct config_set *cs, const char *key, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_int(key, value);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_ulong(key, value);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_bool(struct config_set *cs, const char *key, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_bool(key, value);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_bool_or_int(struct config_set *cs, const char *key,
+				int *is_bool, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_bool_or_int(key, value, is_bool);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_maybe_bool(key, value);
+		if (*dest == -1)
+			return -1;
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value))
+		return git_config_pathname(dest, key, value);
+	else
+		return 1;
+}
+
+static int git_config_check_init(void)
+{
+	int ret = 0;
+	if (the_config_set.hash_initialized)
+		return 0;
+	configset_init(&the_config_set);
+	ret = git_config(config_hash_callback, &the_config_set);
+	if (ret >= 0)
+		return 0;
+	else {
+		hashmap_free(&the_config_set.config_hash, 1);
+		the_config_set.hash_initialized = 0;
+		return -1;
+	}
+}
+
+int git_config_get_value(const char *key, const char **value)
+{
+	git_config_check_init();
+	return git_configset_get_value(&the_config_set, key, value);
+}
+
+const struct string_list *git_config_get_value_multi(const char *key)
+{
+	git_config_check_init();
+	return git_configset_get_value_multi(&the_config_set, key);
+}
+
+void git_config_clear(void)
+{
+	if (!the_config_set.hash_initialized)
+		return;
+	git_configset_clear(&the_config_set);
+	the_config_set.hash_initialized = 0;
+}
+
+int git_config_get_string(const char *key, const char **dest)
+{
+	git_config_check_init();
+	return git_configset_get_string(&the_config_set, key, dest);
+}
+
+int git_config_get_int(const char *key, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_int(&the_config_set, key, dest);
+}
+
+int git_config_get_ulong(const char *key, unsigned long *dest)
+{
+	git_config_check_init();
+	return git_configset_get_ulong(&the_config_set, key, dest);
+}
+
+int git_config_get_bool(const char *key, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_bool(&the_config_set, key, dest);
+}
+
+int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_bool_or_int(&the_config_set, key, is_bool, dest);
+}
+
+int git_config_get_maybe_bool(const char *key, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_maybe_bool(&the_config_set, key, dest);
+}
+
+int git_config_get_pathname(const char *key, const char **dest)
+{
+	git_config_check_init();
+	return git_configset_get_pathname(&the_config_set, key, dest);
+}
+
 /*
  * Find all the stuff for git_config_set() below.
  */
@@ -1707,6 +2000,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	lock = NULL;
 	ret = 0;
 
+	/* Invalidate the config cache */
+	git_config_clear();
+
 out_free:
 	if (lock)
 		rollback_lock_file(lock);
-- 
1.9.0.GIT

[PATCH v8 2/2] test-config: add tests for the config_set API

[Tanay Abhra]

Expose the `config_set` C API as a set of simple commands in order to
facilitate testing. Add tests for the `config_set` API as well as for
`git_config_get_*()` family for the usual config files.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 .gitignore            |   1 +
 Makefile              |   1 +
 t/t1308-config-set.sh | 170 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test-config.c         | 125 +++++++++++++++++++++++++++++++++++++
 4 files changed, 297 insertions(+)
 create mode 100755 t/t1308-config-set.sh
 create mode 100644 test-config.c

diff --git a/.gitignore b/.gitignore
index 42294e5..7677533 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
 /gitweb/static/gitweb.min.*
 /test-chmtime
 /test-ctype
+/test-config
 /test-date
 /test-delta
 /test-dump-cache-tree
diff --git a/Makefile b/Makefile
index 07ea105..9544efb 100644
--- a/Makefile
+++ b/Makefile
@@ -549,6 +549,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_PROGRAMS_NEED_X += test-chmtime
 TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
new file mode 100755
index 0000000..87a29f1
--- /dev/null
+++ b/t/t1308-config-set.sh
@@ -0,0 +1,170 @@
+#!/bin/sh
+
+test_description='Test git config-set API in different settings'
+
+. ./test-lib.sh
+
+# 'check section.key value' verifies that the entry for section.key is
+# 'value'
+check() {
+	echo "$2" >expected &&
+	test-config get_value "$1" >actual 2>&1 &&
+	test_cmp expected actual
+}
+
+test_expect_success 'setup default config' '
+	cat >.git/config << EOF
+	[core]
+		penguin = very blue
+		Movie = BadPhysics
+		UPPERCASE = true
+		MixedCase = true
+		my =
+		foo
+		baz = sam
+	[Cores]
+		WhatEver = Second
+		baz = bar
+	[cores]
+		baz = bat
+	[CORES]
+		baz = ball
+	[my "Foo bAr"]
+		hi = mixed-case
+	[my "FOO BAR"]
+		hi = upper-case
+	[my "foo bar"]
+		hi = lower-case
+	[core]
+		baz = bat
+		baz = hask
+	[lamb]
+		chop = 65
+		head = none
+	[goat]
+		legs = 4
+		head = true
+		skin = false
+		nose = 1
+		horns
+	EOF
+'
+
+test_expect_success 'get value for a simple key' '
+	check core.penguin "very blue"
+'
+
+test_expect_success 'get value for a key with value as an empty string' '
+	check core.my ""
+'
+
+test_expect_success 'get value for a key with value as NULL' '
+	check core.foo "(NULL)"
+'
+
+test_expect_success 'upper case key' '
+	check core.UPPERCASE "true"
+'
+
+test_expect_success 'mixed case key' '
+	check core.MixedCase "true"
+'
+
+test_expect_success 'key and value with mixed case' '
+	check core.Movie "BadPhysics"
+'
+
+test_expect_success 'key with case sensitive subsection' '
+	check "my.Foo bAr.hi" "mixed-case" &&
+	check "my.FOO BAR.hi" "upper-case" &&
+	check "my.foo bar.hi" "lower-case"
+'
+
+test_expect_success 'key with case insensitive section header' '
+	check cores.baz "ball" &&
+	check Cores.baz "ball" &&
+	check CORES.baz "ball" &&
+	check coreS.baz "ball"
+'
+
+test_expect_success 'find value with misspelled key' '
+	echo "Value not found for \"my.fOo Bar.hi\"" >expect &&
+	test_must_fail test-config get_value "my.fOo Bar.hi" >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find value with the highest priority' '
+	check core.baz "hask"
+'
+
+test_expect_success 'find integer value for a key' '
+	echo 65 >expect &&
+	test-config get_int lamb.chop >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find integer if value is non parse-able' '
+	echo 65 >expect &&
+	test_must_fail test-config get_int lamb.head >actual &&
+	test_must_fail test_cmp expect actual
+'
+
+test_expect_success 'find bool value for the entered key' '
+	cat >expect <<-\EOF &&
+	1
+	0
+	1
+	1
+	1
+	EOF
+	test-config get_bool goat.head >actual &&
+	test-config get_bool goat.skin >>actual &&
+	test-config get_bool goat.nose >>actual &&
+	test-config get_bool goat.horns >>actual &&
+	test-config get_bool goat.legs >>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find multiple values' '
+	cat >expect <<-\EOF &&
+	sam
+	bat
+	hask
+	EOF
+	test-config get_value_multi "core.baz">actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find value from a configset' '
+	cat >config2 <<-\EOF &&
+	[core]
+		baz = lama
+	[my]
+		new = silk
+	[core]
+		baz = ball
+	EOF
+	echo silk >expect &&
+	test-config configset_get_value my.new config2 .git/config >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find value with highest priority from a configset' '
+	echo hask > expect &&
+	test-config configset_get_value core.baz config2 .git/config  >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find value_list for a key from a configset' '
+	cat >except <<-\EOF &&
+	sam
+	bat
+	hask
+	lama
+	ball
+	EOF
+	test-config configset_get_value core.baz config2 .git/config  >actual &&
+	test_cmp expect actual
+'
+
+test_done
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 0000000..dc313c2
--- /dev/null
+++ b/test-config.c
@@ -0,0 +1,125 @@
+#include "cache.h"
+#include "string-list.h"
+
+/*
+ * This program exposes the C API of the configuration mechanism
+ * as a set of simple commands in order to facilitate testing.
+ *
+ * Reads stdin and prints result of command to stdout:
+ *
+ * get_value -> prints the value with highest priority for the entered key
+ *
+ * get_value_multi -> prints all values for the entered key in increasing order
+ *		     of priority
+ *
+ * get_int -> print integer value for the entered key or die
+ *
+ * get_bool -> print bool value for the entered key or die
+ *
+ * configset_get_value -> returns value with the highest priority for the entered key
+ * 			from a config_set constructed from files entered as arguments.
+ *
+ * configset_get_value_multi -> returns value_list for the entered key sorted in
+ * 				ascending order of priority from a config_set
+ * 				constructed from files entered as arguments.
+ *
+ * Examples:
+ *
+ * To print the value with highest priority for key "foo.bAr Baz.rock":
+ * 	test-config get_value "foo.bAr Baz.rock"
+ *
+ */
+
+
+int main(int argc, char **argv)
+{
+	int i, val;
+	const char *v;
+	const struct string_list *strptr;
+	struct config_set cs;
+	git_configset_init(&cs);
+
+	if (argc < 2) {
+		fprintf(stderr, "Please, provide a command name on the command-line\n");
+		return 1;
+	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
+		if (!git_config_get_value(argv[2], &v)) {
+			if (!v)
+				printf("(NULL)\n");
+			else
+				printf("%s\n", v);
+			return 0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			return 1;
+		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
+		strptr = git_config_get_value_multi(argv[2]);
+		if (strptr) {
+			for (i = 0; i < strptr->nr; i++) {
+				v = strptr->items[i].string;
+				if (!v)
+					printf("(NULL)\n");
+				else
+					printf("%s\n", v);
+			}
+			return 0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			return 1;
+		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_int")) {
+		if (!git_config_get_int(argv[2], &val)) {
+			printf("%d\n", val);
+			return 0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			return 1;
+		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_bool")) {
+		if (!git_config_get_bool(argv[2], &val)) {
+			printf("%d\n", val);
+			return 0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			return 1;
+		}
+	} else if (!strcmp(argv[1], "configset_get_value")) {
+		for (i = 3; i < argc; i++) {
+			if (git_configset_add_file(&cs, argv[i]))
+				return 2;
+		}
+		if (!git_configset_get_value(&cs, argv[2], &v)) {
+			if (!v)
+				printf("(NULL)\n");
+			else
+				printf("%s\n", v);
+			return 0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			return 1;
+		}
+	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
+		for (i = 3; i < argc; i++) {
+			if (git_configset_add_file(&cs, argv[i]))
+				return 2;
+		}
+		strptr = git_configset_get_value_multi(&cs, argv[2]);
+		if (strptr) {
+			for (i = 0; i < strptr->nr; i++) {
+				v = strptr->items[i].string;
+				if (!v)
+					printf("(NULL)\n");
+				else
+					printf("%s\n", v);
+			}
+			return 0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			return 1;
+		}
+	}
+
+	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
+	return 1;
+}
-- 
1.9.0.GIT

Re: [PATCH v8 1/2] add `config_set` API for caching config-like files

[Matthieu Moy]

Hi,

I had a closer look at error management (once more, sorry: I should have
done this earlier...), and it seems to me that:

* Not all errors are managed properly

* Most error cases are untested

Among the cases I can think of:

* Syntax error when parsing the file

* Non-existant file

* Unreadable file (chmod -r)

Tanay Abhra <tanayabh@gmail.com> writes:

> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
> +
> +	Parses the file and adds the variable-value pairs to the `config_set`,
> +	dies if there is an error in parsing the file.

The return value is undocumented.

If I read correctly, the only codepath from this to a syntax error sets
die_on_error, hence "dies if there is an error in parsing the file" is
correct.

Still, there are errors like "unreadable file" or "no such file" that do
not die (nor emit any error message, which is not very good for the
user), and lead to returning -1 here.

I'm not sure this distinction is right (why die on syntax error and
continue running on unreadable file?).

In any case, it should be documented and tested. I'll send a fixup patch
with a few more example tests (probably insufficient).

> +static int git_config_check_init(void)
> +{
> +	int ret = 0;
> +	if (the_config_set.hash_initialized)
> +		return 0;
> +	configset_init(&the_config_set);
> +	ret = git_config(config_hash_callback, &the_config_set);
> +	if (ret >= 0)
> +		return 0;
> +	else {
> +		hashmap_free(&the_config_set.config_hash, 1);
> +		the_config_set.hash_initialized = 0;
> +		return -1;
> +	}
> +}

We have the same convention for errors here. But a more serious issue is
that the return value of this function is ignored most of the time.

It seems git_config should never return a negative value, as it calls
git_config_with_options -> git_config_early, which checks for file
existance and permission before calling git_config_from_file. Indeed,
Git's tests still pass after this:

--- a/config.c
+++ b/config.c
@@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,
 
 int git_config(config_fn_t fn, void *data)
 {
-       return git_config_with_options(fn, data, NULL, 1);
+       int ret = git_config_with_options(fn, data, NULL, 1);
+       if (ret < 0)
+               die("Negative return value in git_config");
+       return ret;
 }

Still, we can imagine cases like race condition between access_or_die()
and git_config_from_file() in

	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
		ret += git_config_from_file(fn, git_etc_gitconfig(),
					    data);
		found += 1;
	}

where the function would indeed return -1. In any case, either we
consider that git_config should never return -1, and we should die in
this case, or we consider that it may happen, and that the "else" branch
of the if/else above is not dead code, and then we can't just ignore the
return value.

I think we should just do something like this:

diff --git a/config.c b/config.c
index 74adbbd..5c023e8 100644
--- a/config.c
+++ b/config.c
@@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
                return 1;
 }
 
-static int git_config_check_init(void)
+static void git_config_check_init(void)
 {
        int ret = 0;
        if (the_config_set.hash_initialized)
@@ -1437,11 +1437,8 @@ static int git_config_check_init(void)
        ret = git_config(config_hash_callback, &the_config_set);
        if (ret >= 0)
                return 0;
-       else {
-               hashmap_free(&the_config_set.config_hash, 1);
-               the_config_set.hash_initialized = 0;
-               return -1;
-       }
+       else
+               die("Unknown error when parsing one of the configuration files.");
 }
 
If not, a comment should explain what the "else" branch corresponds to,
and why/when the return value can be safely ignored.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v8 2/2] test-config: add tests for the config_set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 0000000..dc313c2
> --- /dev/null
> +++ b/test-config.c

> +int main(int argc, char **argv)
> +{
> +	int i, val;
> +	const char *v;
> +	const struct string_list *strptr;
> +	struct config_set cs;
> +	git_configset_init(&cs);

The configset is initialized, but never cleared.

As a result, valgrind --leak-check=full complains with "definitely lost"
items.

I think it would make sense to apply something like this to get a
valgrind-clean test-config.c (I checked, it now passes without
warnings):

diff --git a/test-config.c b/test-config.c
index dc313c2..07b61ef 100644
--- a/test-config.c
+++ b/test-config.c
@@ -41,17 +41,17 @@ int main(int argc, char **argv)
 
 	if (argc < 2) {
 		fprintf(stderr, "Please, provide a command name on the command-line\n");
-		return 1;
+		goto exit1;
 	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
 		if (!git_config_get_value(argv[2], &v)) {
 			if (!v)
 				printf("(NULL)\n");
 			else
 				printf("%s\n", v);
-			return 0;
+			goto exit0;

[...]
 
 	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
+	goto exit1;
+
+exit0:
+	git_configset_clear(&cs);
+	return 0;
+
+exit1:
+	git_configset_clear(&cs);
 	return 1;
+
+exit2:
+	git_configset_clear(&cs);
+	return 2;
 }

I'll resend as a proper "git am"-able patch right after.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[fixup PATCH 1/2] Call configset_clear

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Consider squashing this into PATCH 2/2.

 test-config.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/test-config.c b/test-config.c
index dc313c2..07b61ef 100644
--- a/test-config.c
+++ b/test-config.c
@@ -41,17 +41,17 @@ int main(int argc, char **argv)
 
 	if (argc < 2) {
 		fprintf(stderr, "Please, provide a command name on the command-line\n");
-		return 1;
+		goto exit1;
 	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
 		if (!git_config_get_value(argv[2], &v)) {
 			if (!v)
 				printf("(NULL)\n");
 			else
 				printf("%s\n", v);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
 		strptr = git_config_get_value_multi(argv[2]);
@@ -63,46 +63,46 @@ int main(int argc, char **argv)
 				else
 					printf("%s\n", v);
 			}
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_int")) {
 		if (!git_config_get_int(argv[2], &val)) {
 			printf("%d\n", val);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_bool")) {
 		if (!git_config_get_bool(argv[2], &val)) {
 			printf("%d\n", val);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
 			if (git_configset_add_file(&cs, argv[i]))
-				return 2;
+				goto exit2;
 		}
 		if (!git_configset_get_value(&cs, argv[2], &v)) {
 			if (!v)
 				printf("(NULL)\n");
 			else
 				printf("%s\n", v);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
 		for (i = 3; i < argc; i++) {
 			if (git_configset_add_file(&cs, argv[i]))
-				return 2;
+				goto exit2;
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
 		if (strptr) {
@@ -113,13 +113,25 @@ int main(int argc, char **argv)
 				else
 					printf("%s\n", v);
 			}
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	}
 
 	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
+	goto exit1;
+
+exit0:
+	git_configset_clear(&cs);
+	return 0;
+
+exit1:
+	git_configset_clear(&cs);
 	return 1;
+
+exit2:
+	git_configset_clear(&cs);
+	return 2;
 }
-- 
2.0.0.262.gdafc651

[PATCH 2/2] Better tests for error cases

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Consider squashing this into PATCH 2/2

Probably not sufficient.

 t/t1308-config-set.sh | 22 ++++++++++++++++++++++
 test-config.c         |  8 ++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 87a29f1..f1e9e76 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -167,4 +167,26 @@ test_expect_success 'find value_list for a key from a configset' '
 	test_cmp expect actual
 '
 
+test_expect_success 'proper error on non-existant files' '
+	echo "Error reading configuration file non-existant-file." >expect &&
+	test_must_fail test-config configset_get_value foo.bar non-existant-file 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'proper error on error in default config files' '
+	cp .git/config .git/config.old &&
+	test_when_finished "mv .git/config.old .git/config" &&
+	echo "[" >> .git/config &&
+	echo "fatal: bad config file line 35 in .git/config" >expect &&
+	test_must_fail test-config get_value foo.bar 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'proper error on error in custom config files' '
+	echo "[" >> syntax-error &&
+	echo "fatal: bad config file line 1 in syntax-error" >expect &&
+	test_must_fail test-config configset_get_value foo.bar syntax-error 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/test-config.c b/test-config.c
index 07b61ef..49f8cd7 100644
--- a/test-config.c
+++ b/test-config.c
@@ -86,8 +86,10 @@ int main(int argc, char **argv)
 		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
-			if (git_configset_add_file(&cs, argv[i]))
+			if (git_configset_add_file(&cs, argv[i])) {
+				fprintf(stderr, "Error reading configuration file %s.\n", argv[i]);
 				goto exit2;
+			}
 		}
 		if (!git_configset_get_value(&cs, argv[2], &v)) {
 			if (!v)
@@ -101,8 +103,10 @@ int main(int argc, char **argv)
 		}
 	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
 		for (i = 3; i < argc; i++) {
-			if (git_configset_add_file(&cs, argv[i]))
+			if (git_configset_add_file(&cs, argv[i])) {
+				fprintf(stderr, "Error reading configuration file %s.\n", argv[i]);
 				goto exit2;
+			}
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
 		if (strptr) {
-- 
2.0.0.262.gdafc651

Re: [PATCH v8 1/2] add `config_set` API for caching config-like files

[Tanay Abhra]

On 7/11/2014 7:51 PM, Matthieu Moy wrote:
> Hi,
> 
> I had a closer look at error management (once more, sorry: I should have
> done this earlier...), and it seems to me that:
> 
> * Not all errors are managed properly
> 
> * Most error cases are untested
> 
> Among the cases I can think of:
> 
> * Syntax error when parsing the file
> 
> * Non-existant file
> 
> * Unreadable file (chmod -r)
>

I had seen that there were checks for Syntax error or Non-existant files in
t1300-repo-config, for example,

# malformed configuration files
test_expect_success 'barf on syntax error' '
	cat >.git/config <<-\EOF &&
	# broken section line
	[section]
	key garbage
	EOF
	test_must_fail git config --get section.key >actual 2>error &&
	grep " line 3 " error
'

test_expect_success 'barf on incomplete section header' '
	cat >.git/config <<-\EOF &&
	# broken section line
	[section
	key = value
	EOF
	test_must_fail git config --get section.key >actual 2>error &&
	grep " line 2 " error
'

test_expect_success 'barf on incomplete string' '
	cat >.git/config <<-\EOF &&
	# broken section line
	[section]
	key = "value string
	EOF
	test_must_fail git config --get section.key >actual 2>error &&
	grep " line 3 " error
'
test_expect_success 'alternative GIT_CONFIG (non-existing file should fail)' '
	test_must_fail git config --file non-existing-config -l
'

They cover the same parsing code which I used for constructing the cache.
Still, more tests will not harm anyone, I will add more testcases accordingly
for the corner cases you raised. :)

> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
>> +
>> +	Parses the file and adds the variable-value pairs to the `config_set`,
>> +	dies if there is an error in parsing the file.
> 
> The return value is undocumented.
> 
> If I read correctly, the only codepath from this to a syntax error sets
> die_on_error, hence "dies if there is an error in parsing the file" is
> correct.
> 
> Still, there are errors like "unreadable file" or "no such file" that do
> not die (nor emit any error message, which is not very good for the
> user), and lead to returning -1 here.
> 
> I'm not sure this distinction is right (why die on syntax error and
> continue running on unreadable file?).
> 
> In any case, it should be documented and tested. I'll send a fixup patch
> with a few more example tests (probably insufficient).
>

I am not sure of this myself, I will have to look into it.

>> +static int git_config_check_init(void)
>> +{
>> +	int ret = 0;
>> +	if (the_config_set.hash_initialized)
>> +		return 0;
>> +	configset_init(&the_config_set);
>> +	ret = git_config(config_hash_callback, &the_config_set);
>> +	if (ret >= 0)
>> +		return 0;
>> +	else {
>> +		hashmap_free(&the_config_set.config_hash, 1);
>> +		the_config_set.hash_initialized = 0;
>> +		return -1;
>> +	}
>> +}
> 
> We have the same convention for errors here. But a more serious issue is
> that the return value of this function is ignored most of the time.
> 
> It seems git_config should never return a negative value, as it calls
> git_config_with_options -> git_config_early, which checks for file
> existance and permission before calling git_config_from_file. Indeed,
> Git's tests still pass after this:
> 
> --- a/config.c
> +++ b/config.c
> @@ -1225,7 +1225,10 @@ int git_config_with_options(config_fn_t fn, void *data,
>  
>  int git_config(config_fn_t fn, void *data)
>  {
> -       return git_config_with_options(fn, data, NULL, 1);
> +       int ret = git_config_with_options(fn, data, NULL, 1);
> +       if (ret < 0)
> +               die("Negative return value in git_config");
> +       return ret;
>  }
> 
> Still, we can imagine cases like race condition between access_or_die()
> and git_config_from_file() in
> 
> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
> 					    data);
> 		found += 1;
> 	}
> 
> where the function would indeed return -1. In any case, either we
> consider that git_config should never return -1, and we should die in
> this case, or we consider that it may happen, and that the "else" branch
> of the if/else above is not dead code, and then we can't just ignore the
> return value.
> 
> I think we should just do something like this:
> 
> diff --git a/config.c b/config.c
> index 74adbbd..5c023e8 100644
> --- a/config.c
> +++ b/config.c
> @@ -1428,7 +1428,7 @@ int git_configset_get_pathname(struct config_set *cs, const char *key, const cha
>                 return 1;
>  }
>  
> -static int git_config_check_init(void)
> +static void git_config_check_init(void)
>  {
>         int ret = 0;
>         if (the_config_set.hash_initialized)
> @@ -1437,11 +1437,8 @@ static int git_config_check_init(void)
>         ret = git_config(config_hash_callback, &the_config_set);
>         if (ret >= 0)
>                 return 0;
> -       else {
> -               hashmap_free(&the_config_set.config_hash, 1);
> -               the_config_set.hash_initialized = 0;
> -               return -1;
> -       }
> +       else
> +               die("Unknown error when parsing one of the configuration files.");
>  }
>  
> If not, a comment should explain what the "else" branch corresponds to,
> and why/when the return value can be safely ignored.
>

Yes, this is much better, I will make the necessary changes, thanks.

Re: [PATCH v8 1/2] add `config_set` API for caching config-like files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> I had seen that there were checks for Syntax error or Non-existant files in
> t1300-repo-config, for example,

The code raising the syntax error is there, and tested. But the way the
error code (eg. return -1 from git_config) is handled by your code is
not.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v8 1/2] add `config_set` API for caching config-like files

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> diff --git a/config.c b/config.c
> index ba882a1..aa58275 100644
> --- a/config.c
> +++ b/config.c
> @@ -9,6 +9,8 @@
>  #include "exec_cmd.h"
>  #include "strbuf.h"
>  #include "quote.h"
> +#include "hashmap.h"
> +#include "string-list.h"
>  
>  struct config_source {
>  	struct config_source *prev;
> @@ -33,10 +35,23 @@ struct config_source {
>  	long (*do_ftell)(struct config_source *c);
>  };
>  
> +struct config_hash_entry {
> +	struct hashmap_entry ent;
> +	char *key;
> +	struct string_list value_list;
> +};
> +
>  static struct config_source *cf;
>  
>  static int zlib_compression_seen;
>  
> +/*
> + * Default config_set that contains key-value pairs from the usual set of config
> + * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG
> + * config file and the global /etc/gitconfig)
> + */
> +static struct config_set the_config_set;
> +
>  static int config_file_fgetc(struct config_source *conf)
>  {
>  	return fgetc(conf->u.file);
> @@ -1212,6 +1227,284 @@ int git_config(config_fn_t fn, void *data)
>  	return git_config_with_options(fn, data, NULL, 1);
>  }
>  
> +static int config_hash_add_value(const char *, const char *, struct hashmap *);

This is a funny forward declaration.  If you define add-file and
friends that modify the config-set _after_ you define find-entry and
friends that are used to look-up, would you still need to do a
forward declaration?

In any case, please give names to these first two parameters as what
they are for can be unclear; because they are of the same type,
there is one less clue than there usually are.

The function signature makes it sound as if this is not specific to
"config"; what takes a hashmap, a key and a value and is called "add"
is a function to add <key, value> pair to a hashmap.  Why doesn't it
take "struct config_set"?  In other words, I would have expected

static int config_set_add(struct config_set *, const char *key, const char *value)

instead.  Not a complaint, but is puzzled why you chose not to do
so.

I suspect almost everywhere in this patch, you would want to do
s/config_hash/config_set/.  s/config_hash_entry/config_set_element/
might be a good idea, too.  You have the concept of the "config
set", and each element of that set is a "config-set element", not a
"config-hash entry", isn't it?

> +static int config_hash_entry_cmp(const struct config_hash_entry *e1,
> +				 const struct config_hash_entry *e2, const void *unused)
> +{
> +	return strcmp(e1->key, e2->key);
> +}
> +
> +static void configset_init(struct config_set *cs)
> +{
> +	if (!cs->hash_initialized) {
> +		hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_hash_entry_cmp, 0);
> +		cs->hash_initialized = 1;
> +	}
> +}

Have uninitializer here, immediately after you defined the initializer.

> +static int config_hash_callback(const char *key, const char *value, void *cb)
> +{
> +	struct config_set *cs = cb;
> +	config_hash_add_value(key, value, &cs->config_hash);
> +	return 0;
> +}
> +
> +int git_configset_add_file(struct config_set *cs, const char *filename)
> +{
> +	int ret = 0;
> +	configset_init(cs);
> +	ret = git_config_from_file(config_hash_callback, filename, cs);
> +	if (!ret)
> +		return 0;
> +	else {
> +		hashmap_free(&cs->config_hash, 1);
> +		cs->hash_initialized = 0;
> +		return -1;

Does calling configset_clear() do too much for the purpose of this
error path?  In other words, is it deliberate that you do not touch
any string-list items you may have accumulated by calling the
callback?

> +static struct config_hash_entry *config_hash_find_entry(const char *key,
> +							struct hashmap *config_hash)
> +{
> +	struct config_hash_entry k;
> +	struct config_hash_entry *found_entry;
> +	char *normalized_key;
> +	int ret;
> +	/*
> +	 * `key` may come from the user, so normalize it before using it
> +	 * for querying entries from the hashmap.
> +	 */
> +	ret = git_config_parse_key(key, &normalized_key, NULL);
> +
> +	if (ret)
> +		return NULL;
> +
> +	hashmap_entry_init(&k, strhash(normalized_key));
> +	k.key = normalized_key;
> +	found_entry = hashmap_get(config_hash, &k, NULL);
> +	free(normalized_key);
> +	return found_entry;
> +}
> +
> +static struct string_list *configset_get_list(const char *key, struct config_set *cs)
> +{
> +	struct config_hash_entry *e = config_hash_find_entry(key, &cs->config_hash);
> +	return e ? &e->value_list : NULL;
> +}
> +
> +static int config_hash_add_value(const char *key, const char *value, struct hashmap *config_hash)
> +{
> +	struct config_hash_entry *e;
> +	e = config_hash_find_entry(key, config_hash);
> +	/*
> +	 * Since the keys are being fed by git_config*() callback mechanism, they
> +	 * are already normalized. So simply add them without any further munging.
> +	 */
> +	if (!e) {
> +		e = xmalloc(sizeof(*e));
> +		hashmap_entry_init(e, strhash(key));
> +		e->key = xstrdup(key);
> +		memset(&e->value_list, 0, sizeof(e->value_list));
> +		e->value_list.strdup_strings = 1;

Hmph, I thought you invented string_list_init() but perhaps it was
somebody else...

> +		hashmap_add(config_hash, e);
> +	}
> +	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);

Mental note: once "string list" is updated to make it a true
"string" list for whatever reason, we would be broken by such a
change because we throw a NULL in there.  Just something to watch
out for in the future.  Perhaps deserves a new comment in
string-list API to officially declare that such a use is supported
and must continue to work?  I dunno.

> +	return 0;
> +}
> +
> +void git_configset_init(struct config_set *cs)
> +{
> +	cs->hash_initialized = 0;
> +}

This is somewhat strange; as we are preparing cs for use, why not
initialize the config_hash member and flip hash_initialized to true
instead?

> +int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
> +{
> +	struct string_list *values = NULL;
> +	/*
> +	 * Follows "last one wins" semantic, i.e., if there are multiple matches for the
> +	 * queried key in the files of the configset, the value returned will be the last
> +	 * value in the value list for that key.
> +	 */
> +	values = configset_get_list(key, cs);
> +
> +	if (!values)
> +		return 1;
> +	assert(values->nr > 0);
> +	*value = values->items[values->nr - 1].string;
> +	return 0;
> +}
> +
> +const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
> +{
> +	return configset_get_list(key, cs);
> +}

This is OK, but I would have expected that configset_get_value(),
knowing that its semantics is "grab all and pick the last one",
would call git_configset_get_value_multi() directly, which in turn
would mean there is no need for configset_get_list() intermediary.

> +void git_configset_clear(struct config_set *cs)
> +{
> +	struct config_hash_entry *entry;
> +	struct hashmap_iter iter;
> +	if (!cs->hash_initialized)
> +		return;
> +
> +	hashmap_iter_init(&cs->config_hash, &iter);
> +	while ((entry = hashmap_iter_next(&iter))) {
> +		free(entry->key);
> +		string_list_clear(&entry->value_list, 0);
> +	}
> +	hashmap_free(&cs->config_hash, 1);
> +}
> +
> +int git_configset_get_string(struct config_set *cs, const char *key, const char **dest)
> +{
> +	const char *value;
> +	if (!git_configset_get_value(cs, key, &value))
> +		return git_config_string(dest, key, value);
> +	else
> +		return 1;
> +}
> +
> +int git_configset_get_int(struct config_set *cs, const char *key, int *dest)
> +{
> +	const char *value;
> +	if (!git_configset_get_value(cs, key, &value)) {
> +		*dest = git_config_int(key, value);
> +		return 0;
> +	} else
> +		return 1;
> +}
> + ...
> +static int git_config_check_init(void)
> +{
> +	int ret = 0;
> +	if (the_config_set.hash_initialized)
> +		return 0;
> +	configset_init(&the_config_set);
> +	ret = git_config(config_hash_callback, &the_config_set);
> +	if (ret >= 0)
> +		return 0;
> +	else {
> +		hashmap_free(&the_config_set.config_hash, 1);
> +		the_config_set.hash_initialized = 0;
> +		return -1;
> +	}
> +}

> +int git_config_get_value(const char *key, const char **value)
> +{
> +	git_config_check_init();
> +	return git_configset_get_value(&the_config_set, key, value);
> +}
> +
> +const struct string_list *git_config_get_value_multi(const char *key)
> +{
> +	git_config_check_init();
> +	return git_configset_get_value_multi(&the_config_set, key);
> +}

> +void git_config_clear(void)
> +{
> +	if (!the_config_set.hash_initialized)
> +		return;
> +	git_configset_clear(&the_config_set);
> +	the_config_set.hash_initialized = 0;
> +}

Move this deinitializer of the cached config system immediately
after git_config_check_init(), which is a lazy initializer.

It is somewhat unfortunate that git_config_get_* functions that are
parallel to git_configset_get_* functions cannot be simple macros
with the fixed &the_config_set parameter due to lazy initialization,
but I think it is OK.  This round mostly looks good to me, except
for the points I mentioned above.

Thanks.

Re: [PATCH v8 2/2] test-config: add tests for the config_set API

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
> new file mode 100755
> index 0000000..87a29f1
> --- /dev/null
> +++ b/t/t1308-config-set.sh
> @@ -0,0 +1,170 @@
> +#!/bin/sh
> +
> +test_description='Test git config-set API in different settings'
> +
> +. ./test-lib.sh
> +
> +# 'check section.key value' verifies that the entry for section.key is
> +# 'value'
> +check() {

(style) SP around both sides of ().

More importantly, will we ever have different kind of check in this
script, perhaps checking if all values for a multi-valued variables
appear in the output, checking if get_bool works, etc. in the
future?  I would imagine the answer is yes, and in that case this
should be renamed to be a bit more specific (i.e. no "too generic"
helper called "check" would exist in the final result).  Perhaps
call it "check_single", "check_get_value" or "check_value" (the last
one assumes that all your future checks are mostly about various
forms of "get")?

> +	echo "$2" >expected &&
> +	test-config get_value "$1" >actual 2>&1 &&
> +	test_cmp expected actual
> +}
> +
> +test_expect_success 'setup default config' '
> +	cat >.git/config << EOF

 - No SP after redirection operator.

 - If you are not using variable substition in the here-doc, it is
   more friendly to quote the end-of-here-doc token to tell the
   reader that they do not have to worry about them.

 - There may be core.* variables that are crucial for correct
   operation of the version of Git being tested, so wiping and
   replacing .git/config wholesale is not a good idea.  Appending
   your config items is sufficient for the purpose of these tests.

i.e.

	cat >>.git/config <<\EOF
        ...
	EOF

> +	[core]

I'd feel safer if you did not abuse [core] like this.  All you care
about is the config parsing, and you do not want future versions of
Git introducing core.MixedCase to mean something meaningful that
affects how "git config" and other commands work.

> +		penguin = very blue
> +		Movie = BadPhysics
> +		UPPERCASE = true
> +		MixedCase = true
> +		my =
> ...
> +	EOF
> +'
> +
> +test_expect_success 'get value for a simple key' '
> +	check core.penguin "very blue"
> +'
> +
> +test_expect_success 'get value for a key with value as an empty string' '
> +	check core.my ""
> +'
> +
> +test_expect_success 'get value for a key with value as NULL' '
> +	check core.foo "(NULL)"
> +'
> +
> +test_expect_success 'upper case key' '
> +	check core.UPPERCASE "true"
> +'
> +
> +test_expect_success 'mixed case key' '
> +	check core.MixedCase "true"
> +'

You would also need to see how

	check core.uppercase
        check core.MIXEDCASE

behave (after moving them out of "core." hierarchy, of course), like
the following checks for case insensitivity of the first token.  The
first and the last token are both supposed to be case insensitive,
no?

> +test_expect_success 'key with case insensitive section header' '
> +	check cores.baz "ball" &&
> +	check Cores.baz "ball" &&
> +	check CORES.baz "ball" &&
> +	check coreS.baz "ball"
> +'
> +
> +test_expect_success 'find value with misspelled key' '
> +	echo "Value not found for \"my.fOo Bar.hi\"" >expect &&
> +	test_must_fail test-config get_value "my.fOo Bar.hi" >actual &&

Is test_must_fail suffice here?  git_config_get_value() has two
kinds of non-zero return values (one for "error", the other for "not
found").  Shouldn't test-config let the caller tell these two kinds
apart in some way?  It would be reasonable to do so with its exit
status, I would imagine, and in that case, test_expect_code may be
more appropriate here.

> +	test_cmp expect actual

Are you sure the expect and actual should match here?

Oh by the way, in "check()" helper shell function you spelled
the correct answer "expected" but here you use "expect" (like almost
all the other existing tests).  Perhaps s/expected/expect/ while we
fix the helper function?

> +'
> +
> +test_expect_success 'find value with the highest priority' '
> +	check core.baz "hask"
> +'
> +
> +test_expect_success 'find integer value for a key' '
> +	echo 65 >expect &&
> +	test-config get_int lamb.chop >actual &&
> +	test_cmp expect actual
> +'

Perhaps

	check_config () {
		op=$1 key=$2 &&
                shift &&
                shift &&
                printf "%s\n" "$@" >expect &&
                test-config "$op" "$key" >actual &&
		test_cmp expect actual
	}

and use it like so?

	check_config get_value core.mixedcase true
        check_config get_int lamb.chop 65
        check_config get_bool goat.head 1
        check_config get_value_multi core.baz sam bat hask

> +test_expect_success 'find integer if value is non parse-able' '
> +	echo 65 >expect &&
> +	test_must_fail test-config get_int lamb.head >actual &&
> +	test_must_fail test_cmp expect actual

Do not use test_must_fail on anything other than "git" command.
Worse yet, you are not just interested in seeing expect and actual
differ.  When get_int finds something that is not an integer, you do
not just expect the output from the command to be any random string
that is not the correct answer.  You expect it to be empty, no?

	>expect &&
	test_expect_code 1 test-config get_int lamb.head >actual &&
        test_cmp expect actual

or something (assuming that you chose to exit with 1 when you get an
error, but I didn't check).

Extending the check_config helper a bit more, perhaps

	check_config () {
		case "$1" in
                fail)
			>expect &&
                        test_expect_code 1 test-config "$2" "$3" >actual
                	;;
		*)
                	op=$1 key=$2 &&
                        shift &&
                        shift &&
	                printf "%s\n" "$@" >expect &&
	                test-config "$op" "$key" >actual
			;;
		esac &&
                test_cmp expect actual
	}

or something like that?

> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 0000000..dc313c2
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,125 @@
> +#include "cache.h"
> +#include "string-list.h"
> +
> +/*
> + * This program exposes the C API of the configuration mechanism
> + * as a set of simple commands in order to facilitate testing.
> + *
> + * Reads stdin and prints result of command to stdout:
> + *
> + * get_value -> prints the value with highest priority for the entered key
> + *
> + * get_value_multi -> prints all values for the entered key in increasing order
> + *		     of priority
> + *
> + * get_int -> print integer value for the entered key or die
> + *
> + * get_bool -> print bool value for the entered key or die
> + *
> + * configset_get_value -> returns value with the highest priority for the entered key
> + * 			from a config_set constructed from files entered as arguments.
> + *
> + * configset_get_value_multi -> returns value_list for the entered key sorted in
> + * 				ascending order of priority from a config_set
> + * 				constructed from files entered as arguments.
> + *
> + * Examples:
> + *
> + * To print the value with highest priority for key "foo.bAr Baz.rock":
> + * 	test-config get_value "foo.bAr Baz.rock"
> + *
> + */
> +
> +
> +int main(int argc, char **argv)
> +{
> +	int i, val;
> +	const char *v;
> +	const struct string_list *strptr;
> +	struct config_set cs;
> +	git_configset_init(&cs);
> +
> +	if (argc < 2) {
> +		fprintf(stderr, "Please, provide a command name on the command-line\n");
> +		return 1;
> +	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
> +		if (!git_config_get_value(argv[2], &v)) {
> +			if (!v)
> +				printf("(NULL)\n");

This one is dubious.  Is this for things like

	(in .git/config)
	[receive]
        	fsckobjects

and asking with

	$ git config receive.fsckobjects

which I think gives an empty string?  We may want to be consistent.

> +			else
> +				printf("%s\n", v);
> +			return 0;
> +		} else {
> +			printf("Value not found for \"%s\"\n", argv[2]);
> +			return 1;

So "not found" is signalled with exit code 1.  die() coming from
other errors will give us something like 128, and you exit with 2
when add-file fails (below), so the caller indeed can tell these
cases apart.

The tests that use test_must_fail in your test scripts should be
updated to use test_expect_code then.

> +		}
> +	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
> +...
> +	}
> +
> +	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
> +	return 1;

This is an error of different kind, no?  Use a different exit code
for it.  Instead of fprintf/return, using die() is fine here.

> +}

Re: [PATCH v8 1/2] add `config_set` API for caching config-like files

[Tanay Abhra]

On 7/11/2014 7:51 PM, Matthieu Moy wrote:
>> +`int git_configset_add_file(struct config_set *cs, const char *filename)`::
>> +
>> +	Parses the file and adds the variable-value pairs to the `config_set`,
>> +	dies if there is an error in parsing the file.
> 
> The return value is undocumented.
> 
> If I read correctly, the only codepath from this to a syntax error sets
> die_on_error, hence "dies if there is an error in parsing the file" is
> correct.
> 
> Still, there are errors like "unreadable file" or "no such file" that do
> not die (nor emit any error message, which is not very good for the
> user), and lead to returning -1 here.
> 
> I'm not sure this distinction is right (why die on syntax error and
> continue running on unreadable file?).
> 

I checked the whole codebase and in all of the cases if they cannot read a file
they return -1 and continue running. So, I have updated the API to document the
return value.

I think if the file is unreadable. we must continue running as no harm has been
done yet, worse is parsing a file with wrong syntax which can cause reading
wrong config values. So the decision to die on syntax error sounds alright
to me.

Cheers,
Tanay Abhra.

Re: [PATCH v8 2/2] test-config: add tests for the config_set API

[Tanay Abhra]

Junio C Hamano <gitster <at> pobox.com> writes:

> 
> Tanay Abhra <tanayabh <at> gmail.com> writes:
> 
> > diff --git a/test-config.c b/test-config.c
> > new file mode 100644
> > index 0000000..dc313c2
> > --- /dev/null
> > +++ b/test-config.c
> >  <at>  <at>  -0,0 +1,125  <at>  <at> 
> > +
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	int i, val;
> > +	const char *v;
> > +	const struct string_list *strptr;
> > +	struct config_set cs;
> > +	git_configset_init(&cs);
> > +
> > +	if (argc < 2) {
> > +		fprintf(stderr, "Please, provide a command name on the command-line\n");
> > +		return 1;
> > +	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
> > +		if (!git_config_get_value(argv[2], &v)) {
> > +			if (!v)
> > +				printf("(NULL)\n");
> 
> This one is dubious.  Is this for things like
> 
> 	(in .git/config)
> 	[receive]
>         	fsckobjects
>

Yes, it was meant for the above case.

> and asking with
> 
> 	$ git config receive.fsckobjects
> 
> which I think gives an empty string?  We may want to be consistent.

$ git config -l
shows NULL values as foo.bar
empty values as      foo.bar=
So there is a difference between the two.
$ git config receive.fsckobjects does covert the NULL value to a ""(empty
string).

I had to diffrentiate between the two, so I took the path printf takes for
NULL strings, it prints them as "(NULL)".

Re: [PATCH v8 1/2] add `config_set` API for caching config-like files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> I checked the whole codebase and in all of the cases if they cannot read a file
> they return -1 and continue running.

More precisely: in git_config_from_file, any fopen failure results in
"return -1". But in git_config_early (one caller of
git_config_from_file()), the file is checked before access, e.g.:

	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
		ret += git_config_from_file(fn, git_etc_gitconfig(),
					    data);
		found += 1;
	}

Essentially, if a config file does not exist, it's skipped (obviously
desirable), but if some really weird error occur (if "err == ENOENT ||
err == ENOTDIR || ((flag & ACCESS_EACCES_OK) && err == EACCES" is false,
from access_eacces_ok() in wrapper.c), then the process dies.

"Permission denied" errors are allowed for user-wide config, but not for
others. Read the log for commit 4698c8feb for more details.

Anyway, this is the part of the code you're not touching.

(I actually consider it as a bug that "git config --file no-such-file
foo.bar" and "git config --file unreadable-file foo.bar" fail without
error message, but your commit does not change this).

> I think if the file is unreadable. we must continue running as no harm has been
> done yet, worse is parsing a file with wrong syntax which can cause reading
> wrong config values. So the decision to die on syntax error sounds alright
> to me.

In the case of git_config_check_init(), I think it makes sense to die,
because file accesses are protected with access_or_die(), so the return
value can be negative only if something really went wrong.

If you chose not to die, then you should check the return value in the
callers of git_config_check_init().

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/