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

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

[Tanay Abhra]

[PATCH v4]: One style nit corrected, also added key to error messages.
	Diff with v3 at the bottom for easy review.

[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_const()`

 Documentation/technical/api-config.txt |   5 ++
 cache.h                                |  25 +++++++
 config.c                               | 118 +++++++++++++++++++++++++++++----
 t/t1308-config-set.sh                  |  21 ++++++
 t/t4055-diff-context.sh                |   2 +-
 test-config.c                          |  10 +++
 6 files changed, 167 insertions(+), 14 deletions(-)

-- 
1.9.0.GIT

-- >8 --
diff --git a/config.c b/config.c
index f2d1811..16e99bf 100644
--- a/config.c
+++ b/config.c
@@ -1251,9 +1251,10 @@ static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 		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");
+				die("unable to parse '%s' from command-line config", entry->key);
 			else
-				die("bad config file line %d in %s",
+				die("bad config variable '%s' at file line %d in %s",
+					entry->key,
 					kv_info->linenr,
 					kv_info->filename);
 		}
@@ -1566,9 +1567,12 @@ void git_die_config(const char *key)
 	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");
+		die("unable to parse '%s' from command-line config", key);
 	else
-		die("bad config file line %d in %s", kv_info->linenr, kv_info->filename);
+		die("bad config variable '%s' at file line %d in %s",
+			key,
+			kv_info->linenr,
+			kv_info->filename);
  }
 
 /*
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 3217b4d..f012dd6 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -126,7 +126,7 @@ test_expect_success 'find string value for a key' '
 
 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
+	grep "fatal: bad config variable '\''case.foo'\'' at file line 7 in .git/config" result
 '
 
 test_expect_success 'find integer if value is non parse-able' '
@@ -215,7 +215,7 @@ test_expect_success 'check line errors for malformed values' '
 		br
 	EOF
 	test_expect_code 128 git br 2>result &&
-	grep "fatal: bad config file line 2 in .git/config" result
+	grep "fatal: bad config variable '\''alias.br'\'' at file line 2 in .git/config" result
 '
 
 test_done
diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
index cd04543..741e080 100755
--- a/t/t4055-diff-context.sh
+++ b/t/t4055-diff-context.sh
@@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' '
 test_expect_success 'negative integer config parsing' '
 	git config diff.context -1 &&
 	test_must_fail git diff 2>output &&
-	test_i18ngrep "bad config file" output
+	test_i18ngrep "bad config variable" output
 '
 
 test_expect_success '-U0 is valid, so is diff.context=0' '
-- >8 --

[PATCH v4 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 v4 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 v4 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.

Add configuration variable in addition to line number and file name to the
error message that is printed when `git_config()` dies.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 cache.h                 | 24 ++++++++++++++++++++
 config.c                | 58 ++++++++++++++++++++++++++++++++++++++++++-------
 t/t4055-diff-context.sh |  2 +-
 3 files changed, 75 insertions(+), 9 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..1134741 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,45 @@ 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 '%s' from command-line config", entry->key);
+			else
+				die("bad config variable '%s' at file line %d in %s",
+					entry->key,
+					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 +1296,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 +1312,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 +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)
@@ -1322,6 +1360,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 +1482,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)
diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
index cd04543..741e080 100755
--- a/t/t4055-diff-context.sh
+++ b/t/t4055-diff-context.sh
@@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' '
 test_expect_success 'negative integer config parsing' '
 	git config diff.context -1 &&
 	test_must_fail git diff 2>output &&
-	test_i18ngrep "bad config file" output
+	test_i18ngrep "bad config variable" output
 '
 
 test_expect_success '-U0 is valid, so is diff.context=0' '
-- 
1.9.0.GIT

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

[Tanay Abhra]

> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
> index cd04543..741e080 100755
> --- a/t/t4055-diff-context.sh
> +++ b/t/t4055-diff-context.sh
> @@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' '
>  test_expect_success 'negative integer config parsing' '
>  	git config diff.context -1 &&
>  	test_must_fail git diff 2>output &&
> -	test_i18ngrep "bad config file" output
> +	test_i18ngrep "bad config variable" output
>  '
>
>  test_expect_success '-U0 is valid, so is diff.context=0' '
>

This was a minor fixup with only a changed word, so I didn't flip it and
fix after in the series as you said yesterday. Dunno if it's alright.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>> diff --git a/t/t4055-diff-context.sh b/t/t4055-diff-context.sh
>> index cd04543..741e080 100755
>> --- a/t/t4055-diff-context.sh
>> +++ b/t/t4055-diff-context.sh
>> @@ -79,7 +79,7 @@ test_expect_success 'non-integer config parsing' '
>>  test_expect_success 'negative integer config parsing' '
>>  	git config diff.context -1 &&
>>  	test_must_fail git diff 2>output &&
>> -	test_i18ngrep "bad config file" output
>> +	test_i18ngrep "bad config variable" output
>>  '
>>
>>  test_expect_success '-U0 is valid, so is diff.context=0' '
>>
>
> This was a minor fixup with only a changed word, so I didn't flip it and
> fix after in the series as you said yesterday. Dunno if it's alright.

You did right. It's not a new test, but an existing one that needs
update together with your code update.

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

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +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 '%s' from command-line config", entry->key);
> +			else
> +				die("bad config variable '%s' at file line %d in %s",
> +					entry->key,
> +					kv_info->linenr,
> +					kv_info->filename);
> +		}
> +	}
> +	return 0;
> +}

configset_iter unconditionnally returns 0 (or it dies). Since it is more
or less the equivalent of the old git_config(), I understand why we
never encounter the situation where git_config() would return -1 (syntax
error, weird permission issue => cannot happen when reading from
memory).

But then, do we really want this return value, and not just return void?

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

Here too, git_config now unconditionnally returns 0.

Most callers of git_config already ignore the return value. Actually,
there's only one exception in branch.c, but git still compiles with
this:

diff --git a/branch.c b/branch.c
index 660097b..92c3ff2 100644
--- a/branch.c
+++ b/branch.c
@@ -161,10 +161,7 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
        strbuf_addf(&name, "branch.%s.description", branch_name);
        cb.config_name = name.buf;
        cb.value = NULL;
-       if (git_config(read_branch_desc_cb, &cb) < 0) {
-               strbuf_release(&name);
-               return -1;
-       }
+       git_config(read_branch_desc_cb, &cb);
        if (cb.value)
                strbuf_addstr(buf, cb.value);
        strbuf_release(&name);
diff --git a/cache.h b/cache.h
index 40b4ef3..23bfc67 100644
--- a/cache.h
+++ b/cache.h
@@ -1266,7 +1266,7 @@ extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern int git_config(config_fn_t fn, void *);
+extern void git_config(config_fn_t fn, void *);
 extern int git_config_with_options(config_fn_t fn, void *,
                                   struct git_config_source *config_source,
                                   int respect_includes);
diff --git a/config.c b/config.c
index 0346681..3d033c9 100644
--- a/config.c
+++ b/config.c
@@ -1223,9 +1223,9 @@ int git_config_with_options(config_fn_t fn, void *data,
        return ret;
 }
 
-int git_config(config_fn_t fn, void *data)
+void git_config(config_fn_t fn, void *data)
 {
-       return git_config_with_options(fn, data, NULL, 1);
+       git_config_with_options(fn, data, NULL, 1);
 }
 
 static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)

I do not see any difference between the git_config() call in branch.c
and the others.

So, I think it's time to make it official that git_config() does not
return an error code, and make it return void. I would do that in a
patch before the git_config() -> git_config_raw() rewrite.

My preference would be to get the return value from
git_config_with_options and die() if it's negative, but I can also live
with a solution where the return value from git_config_with_options() is
ignored. It's the same discussion we already had about the call to
git_config() in git_config_check_init() actually, but I now think a
die() statement should be within git_config(), not after, so that every
callers benefit from it.

In any case, doing this in a separate patch means the commit message
(and possibly a comment next to the git_config() call) should explain
the situation clearly and justify the choice.

The current situation looks like someone tried to get good error
recovery, but the error code is lost in the way between
git_config_with_options() and the caller of git_config(), without a
clear justification of why an error code was once returned, nor a
justification of why it was later ignored.

So, in summary, my advice (but not the only option) would be: take my
patch above, add a die() statement and a comment, write a good commit
message and insert this before this patch.

>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>  {
>  	struct config_set_element k;
> @@ -1268,6 +1296,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 +1312,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;

I would spell this

	l_item = &cs->list.items[cs->list.nr];
	l_item->e = e;
	l_item->value_index = e->value_list.nr;
	cs->list.nr++;

to avoid having to wonder why the "- 1" is needed. But I'm OK with the
current code.

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

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

[Tanay Abhra]

On 7/29/2014 6:10 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
> configset_iter unconditionnally returns 0 (or it dies). Since it is more
> or less the equivalent of the old git_config(), I understand why we
> never encounter the situation where git_config() would return -1 (syntax
> error, weird permission issue => cannot happen when reading from
> memory).
> 
> But then, do we really want this return value, and not just return void?
>

Sounds sane to me.

>> +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);
>> +}
> 
> Here too, git_config now unconditionnally returns 0.
> 
> Most callers of git_config already ignore the return value. Actually,
> there's only one exception in branch.c, but git still compiles with
> this:
>

branch.c is in my git_config() rewrite patch so it should not be a problem
in the future even if it was the case.

> 
> So, I think it's time to make it official that git_config() does not
> return an error code, and make it return void. I would do that in a
> patch before the git_config() -> git_config_raw() rewrite.
> 
> My preference would be to get the return value from
> git_config_with_options and die() if it's negative, but I can also live

Doesn't git_config_with_options() only return positive values, we checked it
pretty intensively last time.

> with a solution where the return value from git_config_with_options() is
> ignored. It's the same discussion we already had about the call to
> git_config() in git_config_check_init() actually, but I now think a
> die() statement should be within git_config(), not after, so that every
> callers benefit from it.

The above patch works like that, doesn't it?

> 
> In any case, doing this in a separate patch means the commit message
> (and possibly a comment next to the git_config() call) should explain
> the situation clearly and justify the choice.
>

The choice being not to return a error code for git_config()?
I am pretty much confused by now.

> The current situation looks like someone tried to get good error
> recovery, but the error code is lost in the way between
> git_config_with_options() and the caller of git_config(), without a
> clear justification of why an error code was once returned, nor a
> justification of why it was later ignored.
> 
> So, in summary, my advice (but not the only option) would be: take my
> patch above, add a die() statement and a comment, write a good commit

Where can the die() statement be inserted? Again, I am confused.
Only thing which sounds reasonable to me is to rewrite existing git_config()
as void first. Other than that, it went over my head.

> message and insert this before this patch.
> 
>>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>>  {
>>  	struct config_set_element k;
>> @@ -1268,6 +1296,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 +1312,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;
> 
> I would spell this
> 
> 	l_item = &cs->list.items[cs->list.nr];
> 	l_item->e = e;
> 	l_item->value_index = e->value_list.nr;
> 	cs->list.nr++;
> 
> to avoid having to wonder why the "- 1" is needed. But I'm OK with the
> current code.
> 

Yup, you are right. Thanks.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/29/2014 6:10 PM, Matthieu Moy wrote:
>> So, I think it's time to make it official that git_config() does not
>> return an error code, and make it return void. I would do that in a
>> patch before the git_config() -> git_config_raw() rewrite.
>> 
>> My preference would be to get the return value from
>> git_config_with_options and die() if it's negative, but I can also live
>
> Doesn't git_config_with_options() only return positive values, we checked it
> pretty intensively last time.

In normal cases, yes.

But the value comes from lines like

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

and git_config_from_file returns either 0 or -1.

So, either we consider that git_config_from_file always returns 0, and
the "ret +=" part is dead code that should be removed as it only
confuses the reader, or we consider cases where git_config_from_file
returns -1, and we should do something with ret.

As we already discussed, "return -1" is possible in case of race
condition between access_or_die() and git_config_from_file(). Very, very
unlikely in practice, but may happen in theory. That's why I suggest to
die() in these cases: the user will never see it in practice, but it
guarantees that we won't try to proceed if such case happen.

My point is not to improve the behavior, but to improve the code, by
documenting properly where the error code is lost in the path from
git_parse_source() to the caller of git_config().

We wouldn't have such discussion if the code was clear. I spent quite
some time trying to understand why an error code could be returned by
e.g. git_config_early(), and I'd like future readers to avoid wasting
such time.

> Where can the die() statement be inserted? Again, I am confused.

I mean, changing the corresponding hunk to this:

--- a/config.c
+++ b/config.c
@@ -1223,9 +1223,21 @@ int git_config_with_options(config_fn_t fn, void *data,
        return ret;
 }
 
-int git_config(config_fn_t fn, void *data)
+void git_config(config_fn_t fn, void *data)
 {
-       return git_config_with_options(fn, data, NULL, 1);
+       if (git_config_with_options(fn, data, NULL, 1) < 0)
+               /*
+                * git_config_with_options() normally returns only
+                * positive values, as most errors are fatal, and
+                * non-fatal potential errors are guarded by "if"
+                * statements that are entered only when no error is
+                * possible.
+                *
+                * If we ever encounter a non-fatal error, it means
+                * something went really wrong and we should stop
+                * immediately.
+                */
+               die("Unknown error occured while reading the user's configuration");
 }
 
 static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)

>> with a solution where the return value from git_config_with_options() is
>> ignored. It's the same discussion we already had about the call to
>> git_config() in git_config_check_init() actually, but I now think a
>> die() statement should be within git_config(), not after, so that every
>> callers benefit from it.
>
> The above patch works like that, doesn't it?

Except, it ignores the return code silently.

If you chose not to use a die() here, then ignoring the return value
must be justified, or readers of the code will just assume a programming
error, and will be tempted to repair the code by not ignoring the return
value. If so, there is no point in counting errors in git_config_early()
anymore, and a cleanup patch should be applied, something like:

--- a/config.c
+++ b/config.c
@@ -1147,30 +1147,30 @@ int git_config_system(void)
 
 int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 {
-       int ret = 0, found = 0;
+       int found = 0;
        char *xdg_config = NULL;
        char *user_config = NULL;
 
        home_config_paths(&user_config, &xdg_config, "config");
 
        if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
-               ret += git_config_from_file(fn, git_etc_gitconfig(),
+               git_config_from_file(fn, git_etc_gitconfig(),
                                            data);
                found += 1;
        }
 
        if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
-               ret += git_config_from_file(fn, xdg_config, data);
+               git_config_from_file(fn, xdg_config, data);
                found += 1;
        }
 
        if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
-               ret += git_config_from_file(fn, user_config, data);
+               git_config_from_file(fn, user_config, data);
                found += 1;
        }
 
        if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
-               ret += git_config_from_file(fn, repo_config, data);
+               git_config_from_file(fn, repo_config, data);
                found += 1;
        }
 
@@ -1187,7 +1187,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
 
        free(xdg_config);
        free(user_config);
-       return ret == 0 ? found : ret;
+       return found;
 }
 
 int git_config_with_options(config_fn_t fn, void *data,

(untested)

My preference goes for the defensive one, using a proper die() statement
(or even an assert()).

>> In any case, doing this in a separate patch means the commit message
>> (and possibly a comment next to the git_config() call) should explain
>> the situation clearly and justify the choice.
>>
>
> The choice being not to return a error code for git_config()?
> I am pretty much confused by now.

The choice of which of the two patches above you'll prefer.

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

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

[Tanay Abhra]

On 7/29/2014 7:33 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 7/29/2014 6:10 PM, Matthieu Moy wrote:
>>> So, I think it's time to make it official that git_config() does not
>>> return an error code, and make it return void. I would do that in a
>>> patch before the git_config() -> git_config_raw() rewrite.
>>>
>>> My preference would be to get the return value from
>>> git_config_with_options and die() if it's negative, but I can also live
>>
>> Doesn't git_config_with_options() only return positive values, we checked it
>> pretty intensively last time.
> 
> In normal cases, yes.
> 
> But the value comes from lines like
> 
> 	if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> 		ret += git_config_from_file(fn, git_etc_gitconfig(),
> 					    data);
> 		found += 1;
> 	}
> 
> and git_config_from_file returns either 0 or -1.
> 
> So, either we consider that git_config_from_file always returns 0, and
> the "ret +=" part is dead code that should be removed as it only
> confuses the reader, or we consider cases where git_config_from_file
> returns -1, and we should do something with ret.
> 
> As we already discussed, "return -1" is possible in case of race
> condition between access_or_die() and git_config_from_file(). Very, very
> unlikely in practice, but may happen in theory.

This time I checked the entire blame history of the functions. You are right,
the return values are remnants of an earlier time.

The git_config() return value had no significance whatsoever for the majority
of the project existence.

git_config_with_options() return value is only checked for "git config --list"
, that also is redundant since doing something like this,

diff --git a/config.c b/config.c
index 058505c..63f1d30 100644
--- a/config.c
+++ b/config.c
@@ -1169,7 +1169,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)

        free(xdg_config);
        free(user_config);
-       return ret == 0 ? found : ret;
+       return found;
 }

still passes all the tests.

Also I tried every trick in the book, to make it return a negative value, but I failed.
So the only case left for a negative return value is what you said, race condition. I checked
about it and you are right, quoting from the "Linux programming interface",

"The time gap between a call to access() and a subsequent operation on a file means
that there is no guarantee that the information returned by access() will still be true
at the time of the later operation (no matter how brief the interval). This situation
could lead to security holes in some application designs."

"time-ofcheck, time-of-use race condition"


 That's why I suggest to
> die() in these cases: the user will never see it in practice, but it
> guarantees that we won't try to proceed if such case happen.
> 
> My point is not to improve the behavior, but to improve the code, by
> documenting properly where the error code is lost in the path from
> git_parse_source() to the caller of git_config().
> 
> We wouldn't have such discussion if the code was clear. I spent quite
> some time trying to understand why an error code could be returned by
> e.g. git_config_early(), and I'd like future readers to avoid wasting
> such time.
> 
>> Where can the die() statement be inserted? Again, I am confused.
> 
> I mean, changing the corresponding hunk to this:
> 
> --- a/config.c
> +++ b/config.c
> @@ -1223,9 +1223,21 @@ int git_config_with_options(config_fn_t fn, void *data,
>         return ret;
>  }
>  
> -int git_config(config_fn_t fn, void *data)
> +void git_config(config_fn_t fn, void *data)
>  {
> -       return git_config_with_options(fn, data, NULL, 1);
> +       if (git_config_with_options(fn, data, NULL, 1) < 0)
> +               /*
> +                * git_config_with_options() normally returns only
> +                * positive values, as most errors are fatal, and
> +                * non-fatal potential errors are guarded by "if"
> +                * statements that are entered only when no error is
> +                * possible.
> +                *
> +                * If we ever encounter a non-fatal error, it means
> +                * something went really wrong and we should stop
> +                * immediately.
> +                */
> +               die("Unknown error occured while reading the user's configuration");
>  }
>

Sounds reasonable, will apply in the next series.
Somewhat validation for git_config_with_options() return value is given in 1f2baa78c6,
quoted below for review.

"config: treat non-existent config files as empty

The git_config() function signals error by returning -1 in
two instances:

  1. An actual error occurs in opening a config file (parse
     errors cause an immediate die).

  2. Of the three possible config files, none was found.

However, this second case is often not an error at all; it
simply means that the user has no configuration (they are
outside a repo, and they have no ~/.gitconfig file). This
can lead to confusing errors, such as when the bash
completion calls "git config --list" outside of a repo. If
the user has a ~/.gitconfig, the command completes
succesfully; if they do not, it complains to stderr.

This patch allows callers of git_config to distinguish
between the two cases. Error is signaled by -1, and
otherwise the return value is the number of files parsed.
This means that the traditional "git_config(...) < 0" check
for error should work, but callers who want to know whether
we parsed any files or not can still do so."


>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
> 
>>> with a solution where the return value from git_config_with_options() is
>>> ignored. It's the same discussion we already had about the call to
>>> git_config() in git_config_check_init() actually, but I now think a
>>> die() statement should be within git_config(), not after, so that every
>>> callers benefit from it.
>>
>> The above patch works like that, doesn't it?
> 
> Except, it ignores the return code silently.
> 
> If you chose not to use a die() here, then ignoring the return value
> must be justified, or readers of the code will just assume a programming
> error, and will be tempted to repair the code by not ignoring the return
> value. If so, there is no point in counting errors in git_config_early()
> anymore, and a cleanup patch should be applied, something like:
> 
> --- a/config.c
> +++ b/config.c
> @@ -1147,30 +1147,30 @@ int git_config_system(void)
>  
>  int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  {
> -       int ret = 0, found = 0;
> +       int found = 0;
>         char *xdg_config = NULL;
>         char *user_config = NULL;
>  
>         home_config_paths(&user_config, &xdg_config, "config");
>  
>         if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {
> -               ret += git_config_from_file(fn, git_etc_gitconfig(),
> +               git_config_from_file(fn, git_etc_gitconfig(),
>                                             data);
>                 found += 1;
>         }
>  
>         if (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {
> -               ret += git_config_from_file(fn, xdg_config, data);
> +               git_config_from_file(fn, xdg_config, data);
>                 found += 1;
>         }
>  
>         if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {
> -               ret += git_config_from_file(fn, user_config, data);
> +               git_config_from_file(fn, user_config, data);
>                 found += 1;
>         }
>  
>         if (repo_config && !access_or_die(repo_config, R_OK, 0)) {
> -               ret += git_config_from_file(fn, repo_config, data);
> +               git_config_from_file(fn, repo_config, data);
>                 found += 1;
>         }
>  
> @@ -1187,7 +1187,7 @@ int git_config_early(config_fn_t fn, void *data, const char *repo_config)
>  
>         free(xdg_config);
>         free(user_config);
> -       return ret == 0 ? found : ret;
> +       return found;
>  }
>  
>  int git_config_with_options(config_fn_t fn, void *data,
> 
> (untested)
> 
> My preference goes for the defensive one, using a proper die() statement
> (or even an assert()).
> 

Yes, mine too goes with the defensive one. Thanks.

[PATCH v4 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 variable.

Add a test documenting that such errors cause a die printing the
the configuration variable, accurate line number and the 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..e2f9d0b 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 variable '\''alias.br'\'' at file line 2 in .git/config" result
+'
+
 test_done
-- 
1.9.0.GIT

[PATCH v4 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                               | 27 +++++++++++++++++++++++++--
 3 files changed, 31 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 1134741..16e99bf 100644
--- a/config.c
+++ b/config.c
@@ -1506,8 +1506,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)
@@ -1548,10 +1552,29 @@ 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 '%s' from command-line config", key);
+	else
+		die("bad config variable '%s' at file line %d in %s",
+			key,
+			kv_info->linenr,
+			kv_info->filename);
+ }
+
 /*
  * Find all the stuff for git_config_set() below.
  */
-- 
1.9.0.GIT

[PATCH v4 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 | 10 ++++++++++
 test-config.c         | 10 ++++++++++
 2 files changed, 20 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index e2f9d0b..f012dd6 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,16 @@ 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 &&
+	check_config expect_code 1 get_string case.ba "Value not found for \"case.ba\""
+'
+
+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 variable '\''case.foo'\'' at 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