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

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

[Tanay Abhra]

[PATCH V3]:All the suggestions in [3] applied. Built on top of [1].

[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 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/254286
[2]: http://thread.gmane.org/gmane.comp.version-control.git/254101
[3]: http://thread.gmane.org/gmane.comp.version-control.git/254211

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                                |  25 ++++++++
 config.c                               | 114 +++++++++++++++++++++++++++++----
 t/t1308-config-set.sh                  |  20 ++++++
 test-config.c                          |  10 +++
 5 files changed, 161 insertions(+), 13 deletions(-)

-- 
1.9.0.GIT

[PATCH v3 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 d3ad661..e5b7f10 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 v3 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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index e5b7f10..5499108 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;
 }
@@ -1301,7 +1318,7 @@ 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;
-- 
1.9.0.GIT

[PATCH v3 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  | 24 ++++++++++++++++++++++++
 config.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index 7292aef..93bdbab 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,32 @@ 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 5499108..2ce3318 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,12 @@ 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);
+
+	ALLOC_GROW(cs->list.items, cs->list.nr + 1, 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 +1340,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)
@@ -1322,6 +1359,10 @@ void git_configset_clear(struct config_set *cs)
 	}
 	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)
@@ -1440,7 +1481,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 v3 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 v3 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>
---
Note: git_config_get_string() calls git_config_get_string_const(),
so no need to check for whether to die or not, as it is done by
git_config_get_string_const().

 Documentation/technical/api-config.txt |  5 +++++
 cache.h                                |  1 +
 config.c                               | 24 ++++++++++++++++++++++--
 3 files changed, 28 insertions(+), 2 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 93bdbab..8512225 100644
--- a/cache.h
+++ b/cache.h
@@ -1406,6 +1406,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 2ce3318..136ee9c 100644
--- a/config.c
+++ b/config.c
@@ -1505,8 +1505,12 @@ 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)
@@ -1547,10 +1551,26 @@ 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);
+ }
+
 /*
  * Find all the stuff for git_config_set() below.
  */
-- 
1.9.0.GIT

[PATCH v3 6/6] add tests for `git_config_get_string_const()`

[Tanay Abhra]

Add tests for `git_config_get_string_const()`, 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 v3 5/6] config: add `git_die_config()` to the config-set API

[Ramsay Jones]

On 28/07/14 11:33, Tanay Abhra wrote:
> 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>
> ---
> Note: git_config_get_string() calls git_config_get_string_const(),
> so no need to check for whether to die or not, as it is done by
> git_config_get_string_const().
> 
>  Documentation/technical/api-config.txt |  5 +++++
>  cache.h                                |  1 +
>  config.c                               | 24 ++++++++++++++++++++++--
>  3 files changed, 28 insertions(+), 2 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 93bdbab..8512225 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1406,6 +1406,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 2ce3318..136ee9c 100644
> --- a/config.c
> +++ b/config.c
> @@ -1505,8 +1505,12 @@ 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)
> @@ -1547,10 +1551,26 @@ 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);

How about including the 'key' string in the error message?
Similar comment applies to an earlier patch in this series.

> + }
> +
>  /*
>   * Find all the stuff for git_config_set() below.
>   */
> 

ATB,
Ramsay Jones

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

[Tanay Abhra]

On 7/28/2014 4:25 PM, Ramsay Jones wrote:
> On 28/07/14 11:33, Tanay Abhra wrote:
>> 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>
>> ---
>> Note: git_config_get_string() calls git_config_get_string_const(),
>> so no need to check for whether to die or not, as it is done by
>> git_config_get_string_const().
>>
>>  Documentation/technical/api-config.txt |  5 +++++
>>  cache.h                                |  1 +
>>  config.c                               | 24 ++++++++++++++++++++++--
>>  3 files changed, 28 insertions(+), 2 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 93bdbab..8512225 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1406,6 +1406,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 2ce3318..136ee9c 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1505,8 +1505,12 @@ 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)
>> @@ -1547,10 +1551,26 @@ 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);
> 
> How about including the 'key' string in the error message?
> Similar comment applies to an earlier patch in this series.
>

git_config_get_string & pathname() are already raising a
config_error_nonbool() with the error message,
"Missing value for 'key'".

For other cases, I was just following the previous error message,
but it can be done for the new git_config().

Thanks.

>> + }
>> +
>>  /*
>>   * Find all the stuff for git_config_set() below.
>>   */
>>
> 
> ATB,
> Ramsay Jones
> 
> 
> 

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +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);

Missing space after ,.

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

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/28/2014 4:25 PM, Ramsay Jones wrote:
>> On 28/07/14 11:33, Tanay Abhra wrote:
>>> +	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);
>> 
>> How about including the 'key' string in the error message?
>> Similar comment applies to an earlier patch in this series.
>>
>
> git_config_get_string & pathname() are already raising a
> config_error_nonbool() with the error message,
> "Missing value for 'key'".
>
> For other cases, I was just following the previous error message,
> but it can be done for the new git_config().

It is good to follow the existing in this patch (better code, same
behavior), but I agree with Ramsay that including key in the error
message (for both branches of the if/then/else) in a further patch would
be good, and should be straightforward enough.

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

Re: [PATCH v3 4/6] add a test for semantic errors in config files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

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

This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have
eased review to group both patches, either

PATCH 1: introduce test_expect_failure test to demonstrate the failure
PATCH 2: fix the bug and change test_expect_failure to test_expect_success

Or putting both in the same patch.

I think the series is OK like this, my comment is just to be read as
"next time, here's how to do better".

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

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> [PATCH V3]:All the suggestions in [3] applied. Built on top of [1].

Except for the minor style fix in PATCH 5, the series looks OK to me.

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

Re: [PATCH v3 4/6] add a test for semantic errors in config files

[Tanay Abhra]

On 7/28/2014 4:52 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> +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
>> +'
> 
> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have
> eased review to group both patches, either
> 
> PATCH 1: introduce test_expect_failure test to demonstrate the failure

Didn't Junio comment that he wouldn't recommend inserting a test_expect_failure
for new tests and then flipping them after in the series.

> PATCH 2: fix the bug and change test_expect_failure to test_expect_success
> 
> Or putting both in the same patch.
>

Much better, thanks for the advice.

> I think the series is OK like this, my comment is just to be read as
> "next time, here's how to do better".
> 

Re: [PATCH v3 4/6] add a test for semantic errors in config files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/28/2014 4:52 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> +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
>>> +'
>> 
>> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have
>> eased review to group both patches, either
>> 
>> PATCH 1: introduce test_expect_failure test to demonstrate the failure
>
> Didn't Junio comment that he wouldn't recommend inserting a test_expect_failure
> for new tests and then flipping them after in the series.

No, he even said it was good practice:

http://thread.gmane.org/gmane.comp.version-control.git/254101/focus=254104

his point was to avoid breaking something and repairing in another patch
(which your initial series was doing because the test patch was coming
between "rewrite git_config() to use the config-set API" and "add line
number and file name info to `config_set`").

The situation is different when you have a pre-existing bug.

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