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

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

[Tanay Abhra]

Hi,

[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/

Tanay Abhra (2):
  config-hash.c
  test-config

 .gitignore                             |   1 +
 Documentation/technical/api-config.txt | 134 +++++++++++++++
 Makefile                               |   2 +
 cache.h                                |  34 ++++
 config-hash.c                          | 295 +++++++++++++++++++++++++++++++++
 config.c                               |   3 +
 t/t1308-config-hash.sh                 | 168 +++++++++++++++++++
 test-config.c                          | 125 ++++++++++++++
 8 files changed, 762 insertions(+)
 create mode 100644 config-hash.c
 create mode 100755 t/t1308-config-hash.sh
 create mode 100644 test-config.c

-- 
1.9.0.GIT

Diff between v6 and v5:

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index bdf86d0..65a6717 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -110,7 +110,7 @@ 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 pointer to the parsed integer in
+	`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`.
 
@@ -118,13 +118,13 @@ as well as retrieval for the queried variable, including:
 
 	Similar to `git_config_get_int` but for unsigned longs.
 
-`int git_config_get_int(const char *key, int *dest)`::
+`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 pointer to the parsed result in `dest` and returns 0. When the
+	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`.

@@ -236,7 +236,7 @@ Configset API provides functions for the above mentioned work flow, including:
 
 `void git_configset_init(struct config_set *cs)`::
 
-	Initializes the member variables of config_set `cs`.
+	Initializes the config_set `cs`.
 
 `int git_configset_add_file(struct config_set *cs, const char *filename)`::
 
diff --git a/config-hash.c b/config-hash.c
index 4c56bd9..7cf6a96 100644
--- a/config-hash.c
+++ b/config-hash.c
@@ -5,8 +5,8 @@
 
 /*
  * 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 and the
- * global /etc/gitconfig)
+ * 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;
 
@@ -24,9 +24,12 @@ static int config_hash_entry_cmp(const struct config_hash_entry *e1,
 	return strcmp(e1->key, e2->key);
 }
 
-static void config_hash_init(struct hashmap *config_hash)
+static void configset_init(struct config_set *cs)
 {
-	hashmap_init(config_hash, (hashmap_cmp_fn)config_hash_entry_cmp, 0);
+	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)
@@ -36,12 +39,10 @@ static int config_hash_callback(const char *key, const char *value, void *cb)
 	return 0;
 }
 
-static int add_configset_hash(const char *filename, struct config_set *cs)
+int git_configset_add_file(struct config_set *cs, const char *filename)
 {
 	int ret = 0;
-	if (!cs->hash_initialized)
-		config_hash_init(&cs->config_hash);
-	cs->hash_initialized = 1;
+	configset_init(cs);
 	ret = git_config_from_file(config_hash_callback, filename, cs);
 	if (!ret)
 		return 0;
@@ -59,6 +60,10 @@ static struct config_hash_entry *config_hash_find_entry(const char *key,
 	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)
@@ -81,7 +86,10 @@ static int config_hash_add_value(const char *key, const char *value, struct 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));
@@ -90,10 +98,6 @@ static int config_hash_add_value(const char *key, const char *value, struct hash
 		e->value_list.strdup_strings = 1;
 		hashmap_add(config_hash, e);
 	}
-	/*
-	 * Since the values are being fed by git_config*() callback mechanism, they
-	 * are already normalized. So simply add them without any further munging.
-	 */
 	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
 
 	return 0;
@@ -104,11 +108,6 @@ void git_configset_init(struct config_set *cs)
 	cs->hash_initialized = 0;
 }
 
-int git_configset_add_file(struct config_set *cs, const char *filename)
-{
-	return add_configset_hash(filename, cs);
-}
-
 int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
 {
 	struct string_list *values = NULL;
@@ -138,7 +137,7 @@ void git_configset_clear(struct config_set *cs)
 	if (!cs->hash_initialized)
 		return;
 
-	hashmap_iter_init( &cs->config_hash, &iter);
+	hashmap_iter_init(&cs->config_hash, &iter);
 	while ((entry = hashmap_iter_next(&iter))) {
 		free(entry->key);
 		string_list_clear(&entry->value_list, 0);
@@ -222,8 +221,7 @@ static int git_config_check_init(void)
 	int ret = 0;
 	if (the_config_set.hash_initialized)
 		return 0;
-	config_hash_init(&the_config_set.config_hash);
-	the_config_set.hash_initialized = 1;
+	configset_init(&the_config_set);
 	ret = git_config(config_hash_callback, &the_config_set);
 	if (ret >= 0)
 		return 0;
diff --git a/t/t1308-config-hash.sh b/t/t1308-config-hash.sh
index eba7102..ad99f8b 100755
--- a/t/t1308-config-hash.sh
+++ b/t/t1308-config-hash.sh
@@ -4,11 +4,15 @@ test_description='Test git config-hash API in different settings'
 
 . ./test-lib.sh
 
-test_expect_success 'clear default config' '
-	rm -f .git/config
-'
-
-test_expect_success 'initialize default config' '
+#'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 actual expected
+}
+
+test_expect_success 'setup default config' '
 	cat >.git/config << EOF
 	[core]
 		penguin = very blue
@@ -47,71 +51,48 @@ test_expect_success 'initialize default config' '
 '
 
 test_expect_success 'get value for a simple key' '
-	echo "very blue" >expect &&
-	test-config get_value core.penguin >actual &&
-	test_cmp expect actual
+	check core.penguin "very blue"
 '
 
 test_expect_success 'get value for a key with value as an empty string' '
-	echo "" >expect &&
-	test-config get_value core.my >actual &&
-	test_cmp expect actual
+	check core.my ""
 '
 
 test_expect_success 'get value for a key with value as NULL' '
-	echo "(NULL)" >expect &&
-	test-config get_value core.foo >actual &&
-	test_cmp expect actual
+	check core.foo "(NULL)"
 '
 
 test_expect_success 'upper case key' '
-	echo "true" >expect &&
-	test-config get_value core.UPPERCASE >actual &&
-	test_cmp expect actual
+	check core.UPPERCASE "true"
 '
 
 test_expect_success 'mixed case key' '
-	echo "true" >expect &&
-	test-config get_value core.MixedCase >actual &&
-	test_cmp expect actual
+	check core.MixedCase "true"
 '
 
 test_expect_success 'key and value with mixed case' '
-	echo "BadPhysics" >expect &&
-	test-config get_value core.Movie >actual &&
-	test_cmp expect actual
+	check core.Movie "BadPhysics"
 '
 
 test_expect_success 'key with case sensitive subsection' '
-	echo "mixed-case" >expect &&
-	echo "upper-case" >>expect &&
-	echo "lower-case" >>expect &&
-	test-config get_value "my.Foo bAr.hi" >actual &&
-	test-config get_value "my.FOO BAR.hi" >>actual &&
-	test-config get_value "my.foo bar.hi" >>actual &&
-	test_cmp expect actual
+	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' '
-	echo "ball" >expect &&
-	echo "ball" >>expect &&
-	echo "ball" >>expect &&
-	test-config get_value cores.baz >actual &&
-	test-config get_value Cores.baz >>actual &&
-	test-config get_value CORES.baz >>actual &&
-	test_cmp expect actual
+	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
+	check "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""
 '
 
 test_expect_success 'find value with the highest priority' '
-	echo hask >expect &&
-	test-config get_value "core.baz">actual &&
-	test_cmp expect actual
+	check core.baz "hask"
 '
 
 test_expect_success 'find integer value for a key' '
diff --git a/test-config.c b/test-config.c
index 45ccd0a..dc313c2 100644
--- a/test-config.c
+++ b/test-config.c
@@ -1,5 +1,4 @@
 #include "cache.h"
-#include "hashmap.h"
 #include "string-list.h"
 
 /*
@@ -34,14 +33,16 @@
 
 int main(int argc, char **argv)
 {
-	int i, no_of_files;
-	int ret = 0;
+	int i, val;
 	const char *v;
-	int val;
 	const struct string_list *strptr;
 	struct config_set cs;
 	git_configset_init(&cs);
-	if (argc == 3 && !strcmp(argv[1], "get_value")) {
+
+	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");
@@ -50,7 +51,7 @@ int main(int argc, char **argv)
 			return 0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return -1;
+			return 1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
 		strptr = git_config_get_value_multi(argv[2]);
@@ -65,7 +66,7 @@ int main(int argc, char **argv)
 			return 0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return -1;
+			return 1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_int")) {
 		if (!git_config_get_int(argv[2], &val)) {
@@ -73,7 +74,7 @@ int main(int argc, char **argv)
 			return 0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return -1;
+			return 1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_bool")) {
 		if (!git_config_get_bool(argv[2], &val)) {
@@ -81,13 +82,12 @@ int main(int argc, char **argv)
 			return 0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return -1;
+			return 1;
 		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
-			ret = git_configset_add_file(&cs, argv[i]);
-			if (ret)
-				return -1;
+			if (git_configset_add_file(&cs, argv[i]))
+				return 2;
 		}
 		if (!git_configset_get_value(&cs, argv[2], &v)) {
 			if (!v)
@@ -97,14 +97,12 @@ int main(int argc, char **argv)
 			return 0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return -1;
+			return 1;
 		}
-
 	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
 		for (i = 3; i < argc; i++) {
-			ret = git_configset_add_file(&cs, argv[i]);
-			if (ret)
-				return -1;
+			if (git_configset_add_file(&cs, argv[i]))
+				return 2;
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
 		if (strptr) {
@@ -118,7 +116,7 @@ int main(int argc, char **argv)
 			return 0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return -1;
+			return 1;
 		}
 	}

Re: [PATCH v6 0/3] git config cache & special querying api utilizing the cache

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>  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
> +	check "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""
>  '

This one is wrong: you did need the test_must_fail here.

(That's related to my other message about &&-chaining in check)

Other than my minor remarks, the patches now sounds good. Tanay: you
should be able to send a v7 very soon, and it should hopefully be ready
for pu.

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

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

[Tanay Abhra]

Hi,

[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/

Tanay Abhra (2):
  config-hash.c
  test-config

 .gitignore                             |   1 +
 Documentation/technical/api-config.txt | 134 +++++++++++++++
 Makefile                               |   2 +
 cache.h                                |  34 ++++
 config-hash.c                          | 295 +++++++++++++++++++++++++++++++++
 config.c                               |   3 +
 t/t1308-config-set.sh                  | 168 +++++++++++++++++++
 test-config.c                          | 125 ++++++++++++++
 8 files changed, 762 insertions(+)
 create mode 100644 config-hash.c
 create mode 100755 t/t1308-config-set.sh
 create mode 100644 test-config.c

-- 
1.9.0.GIT

[PATCH v7 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 +++++++++++++++
 Makefile                               |   1 +
 cache.h                                |  34 ++++
 config-hash.c                          | 295 +++++++++++++++++++++++++++++++++
 config.c                               |   3 +
 5 files changed, 467 insertions(+)
 create mode 100644 config-hash.c

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 230b3a0..65a6717 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/Makefile b/Makefile
index 07ea105..d503f78 100644
--- a/Makefile
+++ b/Makefile
@@ -777,6 +777,7 @@ LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += config.o
+LIB_OBJS += config-hash.o
 LIB_OBJS += connect.o
 LIB_OBJS += connected.o
 LIB_OBJS += convert.o
diff --git a/cache.h b/cache.h
index df65231..35bd71e 100644
--- a/cache.h
+++ b/cache.h
@@ -1325,6 +1325,40 @@ extern int parse_config_key(const char *var,
 			    const char **subsection, int *subsection_len,
 			    const char **key);
 
+/* config-hash.c */
+
+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-hash.c b/config-hash.c
new file mode 100644
index 0000000..7cf6a96
--- /dev/null
+++ b/config-hash.c
@@ -0,0 +1,295 @@
+#include "cache.h"
+#include "hashmap.h"
+#include "string-list.h"
+
+
+/*
+ * 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;
+
+struct config_hash_entry {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};
+
+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);
+}
diff --git a/config.c b/config.c
index a1aef1c..6cffec7 100644
--- a/config.c
+++ b/config.c
@@ -1708,6 +1708,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 v7 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 | 168 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test-config.c         | 125 +++++++++++++++++++++++++++++++++++++
 4 files changed, 295 insertions(+)
 create mode 100755 t/t1308-config-set.sh
 create mode 100644 test-config.c

diff --git a/.gitignore b/.gitignore
index 42294e5..eeb66e2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -176,6 +176,7 @@
 /gitweb/static/gitweb.js
 /gitweb/static/gitweb.min.*
 /test-chmtime
+/test-config
 /test-ctype
 /test-date
 /test-delta
diff --git a/Makefile b/Makefile
index d503f78..e875070 100644
--- a/Makefile
+++ b/Makefile
@@ -548,6 +548,7 @@ X =
 PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
 
 TEST_PROGRAMS_NEED_X += test-chmtime
+TEST_PROGRAMS_NEED_X += test-config
 TEST_PROGRAMS_NEED_X += test-ctype
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
new file mode 100755
index 0000000..27bce1e
--- /dev/null
+++ b/t/t1308-config-set.sh
@@ -0,0 +1,168 @@
+#!/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' '
+	test_must_fail check "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""
+'
+
+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 v7 1/2] add `config_set` API for caching config-like files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
> index 230b3a0..65a6717 100644
> --- a/Documentation/technical/api-config.txt
> +++ b/Documentation/technical/api-config.txt

> +`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

Missing space after ` => badly formatted in HTML.

I didn't find any other formatting error, but you may double-check with

cd Documentation/ && make technical/api-config.html && firefox technical/api-config.html

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

Re: [PATCH v7 2/2] test-config: Add tests for the config_set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +test_expect_success 'find value with misspelled key' '
> +	test_must_fail check "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""
> +'

Sorry, this is still not right. You're checking that either test-config
OR test_cmp fails. You want to check both.

Basically, you can't use the "check" helper here. Your v5 was right for
this test:

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
'


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

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

[Tanay Abhra]

On 7/9/2014 5:42 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
>> index 230b3a0..65a6717 100644
>> --- a/Documentation/technical/api-config.txt
>> +++ b/Documentation/technical/api-config.txt
> 
>> +`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
> 
> Missing space after ` => badly formatted in HTML.
> 
> I didn't find any other formatting error, but you may double-check with
> 
> cd Documentation/ && make technical/api-config.html && firefox technical/api-config.html
> 

Sorry for the wrong version number on the cover letter.
Thanks, for the review, I will check it ASAP.

Also, I tried implementing Junio's request about saving the line number and the file name for each
key-value pair. I had some success, but with some changes,

1. config-hash.c had to be shifted to config.c entirely.
2. string_list util pointer had to be used for each value.

I am appending the diff below, the only changes from version 7 is that, a new struct 'key_source'
has been added and `config_hash_add_value` has been altered a little.

For debugging `git_configset_get_value` prints the line number and file name for each value.
What are your views about it, too late for inclusion? too hackey or contrived?
I has started on a fresh page for it, but I soon saw that I was reimplementing what was already
available in git_config(), so I took this path.

One more thing, Karsten's string-intern API can be used for saving file names as they are repeated a
lot.

-- 8< --
diff --git a/config.c b/config.c
index a1aef1c..697ec1c 100644
--- a/config.c
+++ b/config.c
@@ -9,6 +9,8 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 #include "quote.h"
+#include "string-list.h"
+#include "hashmap.h"

 struct config_source {
 	struct config_source *prev;
@@ -33,10 +35,314 @@ 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;
+};
+
+struct key_source {
+	const char *filename;
+	int linenr;
+};
+
 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_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;
+	struct string_list_item *si;
+	struct key_source *ks = xmalloc(sizeof(*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);
+	}
+	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	ks->filename = xstrdup(cf->name);
+	ks->linenr = cf->linenr - 1;
+	si->util = ks;
+
+	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;
+	struct key_source *ks;
+	/*
+	 * 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;
+	ks = values->items[values->nr - 1].util;
+	printf("the value is in line %d of %s \n", ks->linenr, ks->filename);
+	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);
+}
+
 static int config_file_fgetc(struct config_source *conf)
 {
 	return fgetc(conf->u.file);
@@ -1708,6 +2014,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);
-- 8< --

Re: [PATCH v7 2/2] test-config: Add tests for the config_set API

[Tanay Abhra]

On 7/9/2014 5:43 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +test_expect_success 'find value with misspelled key' '
>> +	test_must_fail check "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""
>> +'
> 
> Sorry, this is still not right. You're checking that either test-config
> OR test_cmp fails. You want to check both.
> 
> Basically, you can't use the "check" helper here. Your v5 was right for
> this test:
> 
> 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
> '
> 
> 

Yup, I had thought so too, that was the reason I left the && chain in check()
in version 6. I will revert back to v5 for it.

Thanks.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Also, I tried implementing Junio's request about saving the line
> number and the file name for each
> key-value pair. I had some success, but with some changes,

My opinion on this:

* It's low priority. I think the order of priority should be (high to
  low)

  1) Finish and get the current series into pu (we're almost there, it's
     not time to back off and restart something new).

  2) Work on the other series that uses the new API. We don't need to
     change _all_ the uses, but we do need a few tens of them to
     validate the fact that the new API is nice and convenient to use.

  3) Get new actual features for the user (tidy up config files, give
     error messages based on numbers).

  You probably won't finish everything, so just think: if the GSoC ends
  between 1) and 2), how serious is it? if it ends between 2) and 3),
  how serious? If reverse the order, then the risk of having nothing
  finished and mergeable at the end is high. If it happens, the
  community may or may not take over and finish it ...

* Still, making sure that the (file, line) is doable later without too
  much change is good. So, if indeed, moving all code to another file is
  required, then it may make sense to do it now to avoid code move
  later.

> 1. config-hash.c had to be shifted to config.c entirely.

Why? I guess one reason is the use of struct cf (are there others?), but
moving just

config_hash_callback
config_hash_add_value
git_configset_add_file

to config.c should do it. Then, config.c could use config-hash.c.

But a cleaner way would be to allow the callback to receive the
config_file struct without accessing it as a global variable (currently,
we have no way to parse two config files in parallel for example).

In practice, it should be possible to pass a 4th pointer argument to the
callback, and keep compatibility with 3 arguments callback (having too
many arguments in not a problem will all ABI I know), but I'm don't
think this is allowed in theory.

On overall, I'm not convinced we should move the code. When the argument
"I need to merge these two things otherwise it doesn't compile" comes in
a discussion, it usually means there's an architecture issue
somewhere ;-).

> One more thing, Karsten's string-intern API can be used for saving
> file names as they are repeated a
> lot.

This, or storing the list of filenames in struct config_set (more or
less as you did in previous patch), and store pointers to the same
string in each config_hash_entry.

But strdup-ing all filenames seems a bit heavy to me. Even though we're
not talking about performance-critical part of Git, I don't like the
idea of wasting too much ;-).

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> My opinion on this:
>
> * It's low priority. I think the order of priority should be (high to
>   low)
>
>   1) Finish and get the current series into pu (we're almost there, it's
>      not time to back off and restart something new).
>
>   2) Work on the other series that uses the new API. We don't need to
>      change _all_ the uses, but we do need a few tens of them to
>      validate the fact that the new API is nice and convenient to use.
>
>   3) Get new actual features for the user (tidy up config files, give
>      error messages based on numbers).
>
>   You probably won't finish everything, so just think: if the GSoC ends
>   between 1) and 2), how serious is it? if it ends between 2) and 3),
>   how serious? If reverse the order, then the risk of having nothing
>   finished and mergeable at the end is high. If it happens, the
>   community may or may not take over and finish it ...
>
> * Still, making sure that the (file, line) is doable later without too
>   much change is good. So, if indeed, moving all code to another file is
>   required, then it may make sense to do it now to avoid code move
>   later.

Good thinking.  As long as the code is prepared, it is a good idea
to start small and bite off only we can chew at once, do things
incrementally.

>> 1. config-hash.c had to be shifted to config.c entirely.
>
> Why? I guess one reason is the use of struct cf (are there others?), but
> moving just
>
> config_hash_callback
> config_hash_add_value
> git_configset_add_file
>
> to config.c should do it. Then, config.c could use config-hash.c.

I am not sure why you guys needed a new file config-hash.c to begin
with, actually.  Besides, "hash" in its name is an implementation
detail (what it gives us is a way to grab values for configuration
variables from a config set) which we would rather not want to see.

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

[Tanay Abhra]

On 7/9/2014 7:49 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
> 
> My opinion on this:
> 
> * It's low priority. I think the order of priority should be (high to
>   low)
> 
>   1) Finish and get the current series into pu (we're almost there, it's
>      not time to back off and restart something new).

Noted. I will send the revised series tomorrow ASAP.

> 
>   2) Work on the other series that uses the new API. We don't need to
>      change _all_ the uses, but we do need a few tens of them to
>      validate the fact that the new API is nice and convenient to use.
> 

Okay, I have updated the series and will send the new one by friday.
Still, I am aiming to rewrite at least half of total calls next weeks
when (1) is in pu.

>   3) Get new actual features for the user (tidy up config files, give
>      error messages based on numbers).
> 
>   You probably won't finish everything, so just think: if the GSoC ends
>   between 1) and 2), how serious is it? if it ends between 2) and 3),
>   how serious? If reverse the order, then the risk of having nothing
>   finished and mergeable at the end is high. If it happens, the
>   community may or may not take over and finish it ...
>

Noted, still I want to add that even when GSoC finishes, I won't leave the
work unfinished. I had said that I wanted to continue being part of the Git
community and I mean that. :)

> * Still, making sure that the (file, line) is doable later without too
>   much change is good. So, if indeed, moving all code to another file is
>   required, then it may make sense to do it now to avoid code move
>   later.
>

Yes, the only problem I see is that the future readers of config.c might
read two versions of the git_config_type helpers and may get confused,
as they have similar names as git_config_*() & git_config_get_*().
That was the reason in the first place that I moved the code into a new file.

>> 1. config-hash.c had to be shifted to config.c entirely.
> 
> Why? I guess one reason is the use of struct cf (are there others?), but

Nope, just for using struct cf. All old API functions raise error by
accessing "cf" globally for the file name and line number.

> moving just
> 
> config_hash_callback
> config_hash_add_value
> git_configset_add_file
> 
> to config.c should do it. Then, config.c could use config-hash.c.
> 
> But a cleaner way would be to allow the callback to receive the
> config_file struct without accessing it as a global variable (currently,
> we have no way to parse two config files in parallel for example).
> 
> In practice, it should be possible to pass a 4th pointer argument to the
> callback, and keep compatibility with 3 arguments callback (having too
> many arguments in not a problem will all ABI I know), but I'm don't
> think this is allowed in theory.
> 
> On overall, I'm not convinced we should move the code. When the argument
> "I need to merge these two things otherwise it doesn't compile" comes in
> a discussion, it usually means there's an architecture issue
> somewhere ;-).
>

I have to decide on what to do next on moving the contents to config.c or not.
Seeing Junio's comments on the topic seems that he wasn't convinced in the
first place that we needed a new file. What should we do, as I am sending the
revised patch tomorrow? The move will be trivial, just cutting and pasting the
contents. Other approaches you mentioned are also doable but, after a certain
amount of retooling. I am open to any method you think would be best.

Cheers,
Tanay Abhra.

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

[Junio C Hamano]

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

>> * Still, making sure that the (file, line) is doable later without too
>>   much change is good. So, if indeed, moving all code to another file is
>>   required, then it may make sense to do it now to avoid code move
>>   later.
>
> Good thinking.  As long as the code is prepared, it is a good idea
> to start small and bite off only we can chew at once, do things
> incrementally.

After thinking about it a bit more, I think <file, line> support
needs to be done not as a mere client of the generic, uncached
git_config() API that we have had for forever, like the current
iteration, but needs to know a bit more about the state the caller
of the callback (namely, git_parse_source()), and we obviously do
not want to expose that machinery to anybody other than the
implementation of the config subsystem (to which the new facility
this GSoC project adds belongs to), so in that sense you have to
have your code in the same config.c file anyway.

It is somewhat dissapointing that this initial implementation was
done as a client of the traditional git_config(), by the way.  I
would have expected that it would be the other way around, in that
the traditional callers of git_config() would behefit automatically
by having the cache layer below it.

But we have to start from somewhere.  Perhaps the round after this
one can rename the exisiting implementation of git_config() to
something else internal to the caching layer and give the existing
callers a compatible interface that is backed by this new caching
layer in a transparent way.

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

[Matthieu Moy]

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

> After thinking about it a bit more, I think <file, line> support
> needs to be done not as a mere client of the generic, uncached
> git_config() API that we have had for forever, like the current
> iteration, but needs to know a bit more about the state the caller
> of the callback (namely, git_parse_source()), and we obviously do
> not want to expose that machinery to anybody other than the
> implementation of the config subsystem (to which the new facility
> this GSoC project adds belongs to), so in that sense you have to
> have your code in the same config.c file anyway.

I do not buy the argument. Why would callers of the callback-style API
not be allowed to give <file, line> errors?

To me, it is a weakness of the API that <file, line> information is not
available to callback functions, regardless of the config-cache.

> It is somewhat dissapointing that this initial implementation was
> done as a client of the traditional git_config(), by the way.  I
> would have expected that it would be the other way around, in that
> the traditional callers of git_config() would behefit automatically
> by having the cache layer below it.

I disagree, and I agree ;-).

I disagree that it is disapointing to use git_config(), and I think the
beauty of the current implementation is to allow this cache mechanism
without changing the parsing code.

I agree that the callers of git_config() could benefit from the cache
mechanism, i.e. use the in-memory pre-parsed config instead of
re-parsing the file each time.

> But we have to start from somewhere.  Perhaps the round after this
> one can rename the exisiting implementation of git_config() to
> something else internal to the caching layer and give the existing
> callers a compatible interface that is backed by this new caching
> layer in a transparent way.

In PATCH v4, there was a git_config_iter function that did exactly that.
I didn't notice it was gone for v5, but could be rather easily
resurected.

I suggest, on top of the current series:

PATCH 3 : (re)introduce git_config_iter, compatible with git_config
  (one variant with a configset argument, another working on the_config_set).

PATCH 4 : rename git_config to e.g. git_config_parse, and
  git_config_iter to git_config.

Then, check that tests still pass (PATCH 4 automatically uses the new
code essentially in every place where Git deals with config, so existing
tests will start to stress the code a lot more), and check with e.g.
"strace -e open git ..." that config files are now parsed only once
where they used to be parsed multiple times.

I'd do that as a separate series, to let the current one finally reach
pu.

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

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Noted, still I want to add that even when GSoC finishes, I won't leave the
> work unfinished. I had said that I wanted to continue being part of the Git
> community and I mean that. :)

This is a good thing, but you shouldn't rely on it for your GSoC. After
the GSoC finishes, you will have much less time for Git.

> I have to decide on what to do next on moving the contents to config.c or not.
> Seeing Junio's comments on the topic seems that he wasn't convinced in the
> first place that we needed a new file. What should we do, as I am sending the
> revised patch tomorrow? The move will be trivial, just cutting and pasting the
> contents. Other approaches you mentioned are also doable but, after a certain
> amount of retooling. I am open to any method you think would be best.

No strong opinion from me here. I like splitting stuff into relatively
small files, and to me it makes sense to keep the parsing code and the
caching code separate (although config-hash.c is no longer a good name,
config-set.c or config-cache.c would be better). But I can for sure live
with both in the same file.

I guess you'll have to make the decision if others don't give better
argument ;-).

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> After thinking about it a bit more, I think <file, line> support
>> needs to be done not as a mere client of the generic, uncached
>> git_config() API that we have had for forever, like the current
>> iteration, but needs to know a bit more about the state the caller
>> of the callback (namely, git_parse_source()), and we obviously do
>> not want to expose that machinery to anybody other than the
>> implementation of the config subsystem (to which the new facility
>> this GSoC project adds belongs to), so in that sense you have to
>> have your code in the same config.c file anyway.
>
> I do not buy the argument. Why would callers of the callback-style API
> not be allowed to give <file, line> errors?
>
> To me, it is a weakness of the API that <file, line> information is not
> available to callback functions, regardless of the config-cache.

I agree that it is good to allow scan-all-config-items-with-callback
callers to learn <file, line>, but that is irrelevant.  Perhaps you
misread what I envision as the endgame of this thing to be and that
is where our differences come from.  One thing I think would be good
to see in the endgame will be to give the benefit of the caching
layer to the callback callers without having them change a single
line of their code.

One possible sequence of changes to make it happen would go like
this, roughly in this order:

 - rename the current git_config() to git_config_raw(), and
   make it static to the config.c.

 - write a thin wrapper git_config() around git_config_raw() in
   config.c, until caching layer learns the iterator.

 - write caching layer to read from git_config_raw().

 - teach git_config_raw() feed its callback functions to learn the
   <file, line> information.  git_configset_get_<type> can then
   start using this in their error reporting.

 - implement iterator in the caching layer.

 - rewrite git_config() that was a thin wrapper around
   git_config_raw() by using the iterator over the cached values.

 - (optional) think about ways to give <file, line> information to
   the callback style callers.  Perhaps we need git_config_2().
   Perhaps we can rewrite all callers of git_config().  We do not
   have to decide it now.

Between git_parse_source() and git_config_raw() we would need to
pass the line-number information, but there is no reason for us to
make public (all of these will be implementation details of the
config system, including the config caching).

My answer to "why shouldn't the callbacks have <file, line>
information?" is "there is no reason why they shouldn't.  It is a
good addition in the long run".  But the optionality of the last
step in the above list makes it an irrelevant question.