[PATCH v2 0/6] Rewrite `git_config()` using config-set API

[PATCH v2 0/6] Rewrite `git_config()` using config-set API

[Tanay Abhra]

[PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows
	correct parsing order. Reordered the patches. Removed xfuncname patch
	as it was unnecssary.

This series builds on the top of 1d2856f (ta/config-set) in pu or topic[1]
in the mailing list with name "git config cache & special querying API utilizing
the cache".

This series aims to do these three things,

* Use the config-set API to rewrite git_config().

* Solve any legacy bugs in the previous system while at it.

* To be feature complete compared to the previous git_config() implementation,
  which I think it is now. (added the line number and file name info just for
  completeness)

Also, I haven't yet checked the exact improvements but still as a teaser,
git status now only rereads the configuration files twice instead of four
times.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/253862
[2]: http://thread.gmane.org/gmane.comp.version-control.git/254101

Tanay Abhra (6):
  config.c: fix accuracy of line number in errors
  add line number and file name info to `config_set`
  rewrite git_config() to use the config-set API
  add a test for semantic errors in config files
  config: add `git_die_config()` to the config-set API
  Add tests for `git_config_get_string()`

 Documentation/technical/api-config.txt |   5 ++
 cache.h                                |  26 +++++++
 config.c                               | 135 ++++++++++++++++++++++++++++-----
 t/t1308-config-set.sh                  |  20 +++++
 test-config.c                          |  10 +++
 5 files changed, 175 insertions(+), 21 deletions(-)

-- 
1.9.0.GIT

[PATCH v2 1/6] config.c: fix accuracy of line number in errors

[Tanay Abhra]

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

If a callback returns a negative value to `git_config*()` family,
they call `die()` while printing the line number and the file name.
Currently the printed line number is off by one, thus printing the
wrong line number.

Make `linenr` point to the line we just parsed during the call
to callback to get accurate line number in error messages.

Commit-message-by: Tanay Abhra <tanayabh@gmail.com>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 config.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 0d799e0..06257d9 100644
--- a/config.c
+++ b/config.c
@@ -244,6 +244,7 @@ static int get_next_char(void)
 		cf->linenr++;
 	if (c == EOF) {
 		cf->eof = 1;
+		cf->linenr++;
 		c = '\n';
 	}
 	return c;
@@ -319,6 +320,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
 	int c;
 	char *value;
+	int ret;
 
 	/* Get the full name */
 	for (;;) {
@@ -341,7 +343,15 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 		if (!value)
 			return -1;
 	}
-	return fn(name->buf, value, data);
+	/*
+	 * We already consumed the \n, but we need linenr to point to
+	 * the line we just parsed during the call to fn to get
+	 * accurate line number in error messages.
+	 */
+	cf->linenr--;
+	ret = fn(name->buf, value, data);
+	cf->linenr++;
+	return ret;
 }
 
 static int get_extended_base_var(struct strbuf *name, int c)
-- 
1.9.0.GIT

[PATCH v2 2/6] add line number and file name info to `config_set`

[Tanay Abhra]

Store file name and line number for each key-value pair in the cache
during parsing of the configuration files.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 config.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 06257d9..110f9a5 100644
--- a/config.c
+++ b/config.c
@@ -1232,6 +1232,11 @@ int git_config_with_options(config_fn_t fn, void *data,
 	return ret;
 }
 
+struct key_value_info {
+	const char *filename;
+	int linenr;
+};
+
 int git_config(config_fn_t fn, void *data)
 {
 	return git_config_with_options(fn, data, NULL, 1);
@@ -1262,6 +1267,9 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
 static int configset_add_value(struct config_set *cs, const char *key, const char *value)
 {
 	struct config_set_element *e;
+	struct string_list_item *si;
+	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
+
 	e = configset_find_element(cs, key);
 	/*
 	 * Since the keys are being fed by git_config*() callback mechanism, they
@@ -1274,7 +1282,16 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 		string_list_init(&e->value_list, 1);
 		hashmap_add(&cs->config_hash, e);
 	}
-	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	if (cf) {
+		kv_info->filename = strintern(cf->name);
+		kv_info->linenr = cf->linenr;
+	} else {
+		/* for values read from `git_config_from_parameters()` */
+		kv_info->filename = strintern("parameter");
+		kv_info->linenr = 0;
+	}
+	si->util = kv_info;
 
 	return 0;
 }
-- 
1.9.0.GIT

[PATCH v2 3/6] rewrite git_config() to use the config-set API

[Tanay Abhra]

Of all the functions in `git_config*()` family, `git_config()` has the
most invocations in the whole code base. Each `git_config()` invocation
causes config file rereads which can be avoided using the config-set API.

Use the config-set API to rewrite `git_config()` to use the config caching
layer to avoid config file rereads on each invocation during a git process
lifetime. First invocation constructs the cache, and after that for each
successive invocation, `git_config()` feeds values from the config cache
instead of rereading the configuration files.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 cache.h  | 25 +++++++++++++++++++++++++
 config.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 76 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 7292aef..11ded5a 100644
--- a/cache.h
+++ b/cache.h
@@ -8,6 +8,7 @@
 #include "gettext.h"
 #include "convert.h"
 #include "trace.h"
+#include "string-list.h"
 
 #include SHA1_HEADER
 #ifndef git_SHA_CTX
@@ -1351,9 +1352,33 @@ extern int parse_config_key(const char *var,
 			    const char **subsection, int *subsection_len,
 			    const char **key);
 
+struct config_set_element {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};
+
+struct configset_list_item {
+	struct config_set_element *e;
+	int value_index;
+};
+
+/*
+ * the contents of the list are ordered according to their
+ * position in the config files and order of parsing the files.
+ * (i.e. key-value pair at the last position of .git/config will
+ * be at the last item of the list)
+ */
+
+struct configset_list {
+	struct configset_list_item *items;
+	unsigned int nr, alloc;
+};
+
 struct config_set {
 	struct hashmap config_hash;
 	int hash_initialized;
+	struct configset_list list;
 };
 
 extern void git_configset_init(struct config_set *cs);
diff --git a/config.c b/config.c
index 110f9a5..aa5c0ad 100644
--- a/config.c
+++ b/config.c
@@ -35,12 +35,6 @@ struct config_source {
 	long (*do_ftell)(struct config_source *c);
 };
 
-struct config_set_element {
-	struct hashmap_entry ent;
-	char *key;
-	struct string_list value_list;
-};
-
 static struct config_source *cf;
 
 static int zlib_compression_seen;
@@ -1237,11 +1231,44 @@ struct key_value_info {
 	int linenr;
 };
 
-int git_config(config_fn_t fn, void *data)
+static int git_config_raw(config_fn_t fn, void *data)
 {
 	return git_config_with_options(fn, data, NULL, 1);
 }
 
+static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
+{
+	int i, value_index;
+	struct string_list *values;
+	struct config_set_element *entry;
+	struct configset_list *list = &cs->list;
+	struct key_value_info *kv_info;
+
+	for (i = 0; i < list->nr; i++) {
+		entry = list->items[i].e;
+		value_index = list->items[i].value_index;
+		values = &entry->value_list;
+		if (fn(entry->key, values->items[value_index].string, data) < 0) {
+			kv_info = values->items[value_index].util;
+			if (!kv_info->linenr)
+				die("unable to parse command-line config");
+			else
+				die("bad config file line %d in %s",
+					kv_info->linenr,
+					kv_info->filename);
+		}
+	}
+	return 0;
+}
+
+static void git_config_check_init(void);
+
+int git_config(config_fn_t fn, void *data)
+{
+	git_config_check_init();
+	return configset_iter(&the_config_set, fn, data);
+}
+
 static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
 {
 	struct config_set_element k;
@@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 {
 	struct config_set_element *e;
 	struct string_list_item *si;
+	struct configset_list_item *l_item;
 	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
 
 	e = configset_find_element(cs, key);
@@ -1283,6 +1311,13 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 		hashmap_add(&cs->config_hash, e);
 	}
 	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+
+	if (cs->list.nr + 1 > cs->list.alloc)
+		ALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);
+	l_item = &cs->list.items[cs->list.nr++];
+	l_item->e = e;
+	l_item->value_index = e->value_list.nr - 1;
+
 	if (cf) {
 		kv_info->filename = strintern(cf->name);
 		kv_info->linenr = cf->linenr;
@@ -1306,6 +1341,9 @@ void git_configset_init(struct config_set *cs)
 {
 	hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_set_element_cmp, 0);
 	cs->hash_initialized = 1;
+	cs->list.nr = 0;
+	cs->list.alloc = 0;
+	cs->list.items = NULL;
 }
 
 void git_configset_clear(struct config_set *cs)
@@ -1318,10 +1356,14 @@ void git_configset_clear(struct config_set *cs)
 	hashmap_iter_init(&cs->config_hash, &iter);
 	while ((entry = hashmap_iter_next(&iter))) {
 		free(entry->key);
-		string_list_clear(&entry->value_list, 0);
+		string_list_clear(&entry->value_list, 1);
 	}
 	hashmap_free(&cs->config_hash, 1);
 	cs->hash_initialized = 0;
+	free(cs->list.items);
+	cs->list.nr = 0;
+	cs->list.alloc = 0;
+	cs->list.items = NULL;
 }
 
 static int config_set_callback(const char *key, const char *value, void *cb)
@@ -1448,7 +1490,7 @@ static void git_config_check_init(void)
 	if (the_config_set.hash_initialized)
 		return;
 	git_configset_init(&the_config_set);
-	git_config(config_set_callback, &the_config_set);
+	git_config_raw(config_set_callback, &the_config_set);
 }
 
 void git_config_clear(void)
-- 
1.9.0.GIT

[PATCH v2 4/6] add a test for semantic errors in config files

[Tanay Abhra]

Semantic errors (for example, for alias.* variables NULL values are
not allowed) in configuration files cause a die printing the line
number and file name of the offending value.

Add a test documenting that such errors cause a die printing the
accurate line number and file name.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 t/t1308-config-set.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7fdf840..35c6ee2 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -197,4 +197,15 @@ test_expect_success 'proper error on error in custom config files' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check line errors for malformed values' '
+	mv .git/config .git/config.old &&
+	test_when_finished "mv .git/config.old .git/config" &&
+	cat >.git/config <<-\EOF &&
+	[alias]
+		br
+	EOF
+	test_expect_code 128 git br 2>result &&
+	grep "fatal: bad config file line 2 in .git/config" result
+'
+
 test_done
-- 
1.9.0.GIT

[PATCH v2 5/6] config: add `git_die_config()` to the config-set API

[Tanay Abhra]

Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.

It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,

	if (!git_config_get_value(key, &value)) {
		/* NULL values not allowed */
		if (!value)
			git_config_die(key);
		else
			/* do work */
	}

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt |  5 ++++
 cache.h                                |  1 +
 config.c                               | 44 ++++++++++++++++++++++++++--------
 3 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 815c1ee..e7ec7cc 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:
 	Similar to `git_config_get_string`, but expands `~` or `~user` into
 	the user's home directory when found at the beginning of the path.
 
+`void git_die_config(const char *key)`::
+
+	Dies printing the line number and the file name of the highest
+	priority value for the configuration variable `key`.
+
 See test-config.c for usage examples.
 
 Value Parsing Helpers
diff --git a/cache.h b/cache.h
index 11ded5a..5c3dd88 100644
--- a/cache.h
+++ b/cache.h
@@ -1407,6 +1407,7 @@ 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 void git_die_config(const char *key);
 
 extern int committer_ident_sufficiently_given(void);
 extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index aa5c0ad..1e49ae7 100644
--- a/config.c
+++ b/config.c
@@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
 
 int git_configset_get_string_const(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 ret;
+	char *value;
+	ret = git_configset_get_string(cs, key, &value);
+	if (ret <= 0)
+		*dest = (const char*)value;
+	return ret;
 }
 
 int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
@@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
 			return config_error_nonbool(key);
 		*dest = xstrdup(value);
 		return 0;
-	}
-	else
+	} else
 		return 1;
 }
 
@@ -1514,14 +1514,22 @@ const struct string_list *git_config_get_value_multi(const char *key)
 
 int git_config_get_string_const(const char *key, const char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_string_const(&the_config_set, key, dest);
+	ret = git_configset_get_string_const(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
 }
 
 int git_config_get_string(const char *key, char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_string(&the_config_set, key, dest);
+	ret = git_configset_get_string(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
 }
 
 int git_config_get_int(const char *key, int *dest)
@@ -1556,8 +1564,24 @@ int git_config_get_maybe_bool(const char *key, int *dest)
 
 int git_config_get_pathname(const char *key, const char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_pathname(&the_config_set, key, dest);
+	ret = git_configset_get_pathname(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
+}
+
+void git_die_config(const char *key)
+{
+	const struct string_list *values;
+	struct key_value_info *kv_info;
+	values = git_config_get_value_multi(key);
+	kv_info = values->items[values->nr - 1].util;
+	if (!kv_info->linenr)
+		die("unable to parse command-line config");
+	else
+		die("bad config file line %d in %s",kv_info->linenr, kv_info->filename);
 }
 
 /*
-- 
1.9.0.GIT

[PATCH v2 6/6] Add tests for `git_config_get_string()`

[Tanay Abhra]

Add tests for `git_config_get_string()`, check whether it
dies printing the line number and the file name if a NULL
value is retrieved for the given key.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 t/t1308-config-set.sh |  9 +++++++++
 test-config.c         | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 35c6ee2..d7cdc6e 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,15 @@ test_expect_success 'find integer value for a key' '
 	check_config get_int lamb.chop 65
 '
 
+test_expect_success 'find string value for a key' '
+	check_config get_string case.baz hask
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+	test_expect_code 128 test-config get_string case.foo 2>result &&
+	grep "fatal: bad config file line 7 in .git/config" result
+'
+
 test_expect_success 'find integer if value is non parse-able' '
 	check_config expect_code 128 get_int lamb.head
 '
diff --git a/test-config.c b/test-config.c
index 9dd1b22..6a77552 100644
--- a/test-config.c
+++ b/test-config.c
@@ -16,6 +16,8 @@
  *
  * get_bool -> print bool value for the entered key or die
  *
+ * get_string -> print string 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.
  *
@@ -84,6 +86,14 @@ int main(int argc, char **argv)
 			printf("Value not found for \"%s\"\n", argv[2]);
 			goto exit1;
 		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_string")) {
+		if (!git_config_get_string_const(argv[2], &v)) {
+			printf("%s\n", v);
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
 			int err;
-- 
1.9.0.GIT

Re: [PATCH v2 3/6] rewrite git_config() to use the config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +struct config_set_element {
> +	struct hashmap_entry ent;
> +	char *key;
> +	struct string_list value_list;
> +};
> +
> +struct configset_list_item {
> +	struct config_set_element *e;
> +	int value_index;
> +};

I originally wondered why you had two levels of pointers, but
config_set_element is not new, just moved. It's OK.

> +/*
> + * the contents of the list are ordered according to their
> + * position in the config files and order of parsing the files.
> + * (i.e. key-value pair at the last position of .git/config will
> + * be at the last item of the list)
> + */
> +
> +struct configset_list {

I wouldn't put a blank line between comment and decl if the comment
applies to the decl.

> -int git_config(config_fn_t fn, void *data)
> +static int git_config_raw(config_fn_t fn, void *data)
>  {
>  	return git_config_with_options(fn, data, NULL, 1);
>  }

I would have done this and the new git_config() as a separate patch, but
I do not mind strongly.

>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>  {
>  	struct config_set_element k;
> @@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>  {
>  	struct config_set_element *e;
>  	struct string_list_item *si;
> +	struct configset_list_item *l_item;
>  	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
>  
>  	e = configset_find_element(cs, key);
> @@ -1283,6 +1311,13 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>  		hashmap_add(&cs->config_hash, e);
>  	}
>  	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
> +
> +	if (cs->list.nr + 1 > cs->list.alloc)

The "if" is already in ALLOC_GROW.

> +		ALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);

The 20 should be just 1 I guess. You're adding 1 element, and ALLOC_GROW
will take care of allocating more than 1 for you (see alloc_nr and
ALLOC_GROW's defs in cache.h).

> @@ -1318,10 +1356,14 @@ void git_configset_clear(struct config_set *cs)
>  	hashmap_iter_init(&cs->config_hash, &iter);
>  	while ((entry = hashmap_iter_next(&iter))) {
>  		free(entry->key);
> -		string_list_clear(&entry->value_list, 0);
> +		string_list_clear(&entry->value_list, 1);

Doesn't this change belong to PATCH 2/6 ?

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

Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> --- a/config.c
> +++ b/config.c
> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
>  
>  int git_configset_get_string_const(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 ret;
> +	char *value;
> +	ret = git_configset_get_string(cs, key, &value);
> +	if (ret <= 0)
> +		*dest = (const char*)value;
> +	return ret;
>  }

Isn't this a fixup meant for another series?

>  int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
> @@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
>  			return config_error_nonbool(key);
>  		*dest = xstrdup(value);
>  		return 0;
> -	}
> -	else
> +	} else
>  		return 1;

Useless churn.

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

Re: [PATCH v2 3/6] rewrite git_config() to use the config-set API

[Tanay Abhra]

On 7/25/2014 7:28 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +struct config_set_element {
>> +	struct hashmap_entry ent;
>> +	char *key;
>> +	struct string_list value_list;
>> +};
>> +
>> +struct configset_list_item {
>> +	struct config_set_element *e;
>> +	int value_index;
>> +};
> 
> I originally wondered why you had two levels of pointers, but
> config_set_element is not new, just moved. It's OK.
> 
>> +/*
>> + * the contents of the list are ordered according to their
>> + * position in the config files and order of parsing the files.
>> + * (i.e. key-value pair at the last position of .git/config will
>> + * be at the last item of the list)
>> + */
>> +
>> +struct configset_list {
> 
> I wouldn't put a blank line between comment and decl if the comment
> applies to the decl.
>

Noted.

>> -int git_config(config_fn_t fn, void *data)
>> +static int git_config_raw(config_fn_t fn, void *data)
>>  {
>>  	return git_config_with_options(fn, data, NULL, 1);
>>  }
> 
> I would have done this and the new git_config() as a separate patch, but
> I do not mind strongly.
> 
>>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>>  {
>>  	struct config_set_element k;
>> @@ -1268,6 +1295,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>>  {
>>  	struct config_set_element *e;
>>  	struct string_list_item *si;
>> +	struct configset_list_item *l_item;
>>  	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
>>  
>>  	e = configset_find_element(cs, key);
>> @@ -1283,6 +1311,13 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
>>  		hashmap_add(&cs->config_hash, e);
>>  	}
>>  	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
>> +
>> +	if (cs->list.nr + 1 > cs->list.alloc)
> 
> The "if" is already in ALLOC_GROW.
> 
>> +		ALLOC_GROW(cs->list.items, cs->list.nr + 20, cs->list.alloc);
> 
> The 20 should be just 1 I guess. You're adding 1 element, and ALLOC_GROW
> will take care of allocating more than 1 for you (see alloc_nr and
> ALLOC_GROW's defs in cache.h).
>

Oh, you are are right, I thought alloc grew by one, not by a factor.

>> @@ -1318,10 +1356,14 @@ void git_configset_clear(struct config_set *cs)
>>  	hashmap_iter_init(&cs->config_hash, &iter);
>>  	while ((entry = hashmap_iter_next(&iter))) {
>>  		free(entry->key);
>> -		string_list_clear(&entry->value_list, 0);
>> +		string_list_clear(&entry->value_list, 1);
> 
> Doesn't this change belong to PATCH 2/6 ?
> 

Yup, you are right again.
Thanks.

Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API

[Tanay Abhra]

On 7/25/2014 7:33 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> --- a/config.c
>> +++ b/config.c
>> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
>>  
>>  int git_configset_get_string_const(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 ret;
>> +	char *value;
>> +	ret = git_configset_get_string(cs, key, &value);
>> +	if (ret <= 0)
>> +		*dest = (const char*)value;
>> +	return ret;
>>  }
> 
> Isn't this a fixup meant for another series?
>

Though v12 is in pu, Junio commented that git_configset_get_string_const() &
git_configset_get_string() can be done more concisely, I was trying to do
that but I failed.

>>  int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
>> @@ -1418,8 +1419,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
>>  			return config_error_nonbool(key);
>>  		*dest = xstrdup(value);
>>  		return 0;
>> -	}
>> -	else
>> +	} else
>>  		return 1;
> 
> Useless churn.
> 

Re: [PATCH v2 5/6] config: add `git_die_config()` to the config-set API

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/25/2014 7:33 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> --- a/config.c
>>> +++ b/config.c
>>> @@ -1403,11 +1403,12 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
>>>  
>>>  int git_configset_get_string_const(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 ret;
>>> +	char *value;
>>> +	ret = git_configset_get_string(cs, key, &value);
>>> +	if (ret <= 0)
>>> +		*dest = (const char*)value;
>>> +	return ret;
>>>  }
>> 
>> Isn't this a fixup meant for another series?
>>
>
> Though v12 is in pu, Junio commented that git_configset_get_string_const() &
> git_configset_get_string() can be done more concisely, I was trying to do
> that but I failed.

My comment on that version was not about conciseness.  You had one
that called git_config_string() to let the callee do the nonbool
error handling and xstrdup() of the non-error return value, and the
other one that did exactly what a call to git_config_string() would
have done.  That is being redundant, not just failing to be concise.

I was actually hoping that we would see just

int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
{
	return git_configset_get_string_const(cs, key, (const char **)dest);
}

with the implementation of _const() variant be the one from v12.