[PATCH v6 0/7] Rewrite `git_config()` using config-set API

[PATCH v6 0/7] Rewrite `git_config()` using config-set API

[Tanay Abhra]

[Patch v6]: Added _(....) to error messages.
	Diff between v6 and v4 at the bottom.

[PATCH v5]: New patch added (3/7). git_config() now returns void.

[PATCH v4]: One style nit corrected, also added key to error messages.

[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 (7):
  config.c: fix accuracy of line number in errors
  add line number and file name info to `config_set`
  change `git_config()` return value to void
  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 ++
 branch.c                               |   5 +-
 cache.h                                |  27 ++++++-
 config.c                               | 131 +++++++++++++++++++++++++++++----
 t/t1308-config-set.sh                  |  21 ++++++
 t/t4055-diff-context.sh                |   2 +-
 test-config.c                          |  10 +++
 7 files changed, 181 insertions(+), 20 deletions(-)

-- 
1.9.0.GIT

-- 8< --
diff --git a/branch.c b/branch.c
index 46e8aa8..735767d 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 8512225..243f618 100644
--- a/cache.h
+++ b/cache.h
@@ -1295,7 +1295,7 @@ extern int git_config_from_buf(config_fn_t fn, const char *name,
 			       const char *buf, size_t len, void *data);
 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 f9bf60a..15fcd91 100644
--- a/config.c
+++ b/config.c
@@ -1229,12 +1229,24 @@ struct key_value_info {
 	int linenr;
 };
 
-static int git_config_raw(config_fn_t fn, void *data)
+static void git_config_raw(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 configuration files"));
 }
 
-static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
+static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 {
 	int i, value_index;
 	struct string_list *values;
@@ -1249,23 +1261,22 @@ 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 '%s' from command-line config", entry->key);
+				die(_("unable to parse '%s' from command-line config"), entry->key);
 			else
-				die("bad config variable '%s' at file line %d in %s",
+				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)
+void git_config(config_fn_t fn, void *data)
 {
 	git_config_check_init();
-	return configset_iter(&the_config_set, fn, data);
+	configset_iter(&the_config_set, fn, data);
 }
 
 static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
@@ -1565,9 +1576,9 @@ 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 '%s' from command-line config", key);
+		die(_("unable to parse '%s' from command-line config"), key);
 	else
-		die("bad config variable '%s' at file line %d in %s",
+		die(_("bad config variable '%s' at file line %d in %s"),
 			key,
 			kv_info->linenr,
 			kv_info->filename);
-- 8< --

[PATCH v6 1/7] 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 a191328..ed5fc8e 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 v6 2/7] 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 ed5fc8e..4a15383 100644
--- a/config.c
+++ b/config.c
@@ -1230,6 +1230,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);
@@ -1260,6 +1265,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
@@ -1272,7 +1280,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;
 }
@@ -1299,7 +1316,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 v6 3/7] change `git_config()` return value to void

[Tanay Abhra]

Currently `git_config()` returns an integer signifying an error code.
During rewrites of the function most of the code was shifted to
`git_config_with_options()`. `git_config_with_options()` normally
returns positive values if its `config_source` parameter is set as NULL,
as most errors are fatal, and non-fatal potential errors are guarded
by "if" statements that are entered only when no error is possible.

Still a negative value can be returned in case of race condition between
`access_or_die()` & `git_config_from_file()`. Also, all callers of
`git_config()` ignore the return value except for one case in branch.c.

Change `git_config()` return value to void and make it die if it receives
a negative value from `git_config_with_options()`.

Original-patch-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 branch.c |  5 +----
 cache.h  |  2 +-
 config.c | 16 ++++++++++++++--
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/branch.c b/branch.c
index 46e8aa8..735767d 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 7292aef..c61919d 100644
--- a/cache.h
+++ b/cache.h
@@ -1294,7 +1294,7 @@ extern int git_config_from_buf(config_fn_t fn, const char *name,
 			       const char *buf, size_t len, void *data);
 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 4a15383..ecbe14f 100644
--- a/config.c
+++ b/config.c
@@ -1235,9 +1235,21 @@ struct key_value_info {
 	int linenr;
 };
 
-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 configuration files"));
 }
 
 static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
-- 
1.9.0.GIT

[PATCH v6 4/7] 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 ++++++++++++++++++++++++++++++++++++++++++-------
 t/t4055-diff-context.sh |  2 +-
 3 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index c61919d..4d3c6bd 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 ecbe14f..6b4dd65 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;
@@ -1235,7 +1229,7 @@ struct key_value_info {
 	int linenr;
 };
 
-void git_config(config_fn_t fn, void *data)
+static void git_config_raw(config_fn_t fn, void *data)
 {
 	if (git_config_with_options(fn, data, NULL, 1) < 0)
 		/*
@@ -1252,6 +1246,39 @@ void git_config(config_fn_t fn, void *data)
 		die(_("unknown error occured while reading the configuration files"));
 }
 
+static void 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);
+		}
+	}
+}
+
+static void git_config_check_init(void);
+
+void git_config(config_fn_t fn, void *data)
+{
+	git_config_check_init();
+	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;
@@ -1278,6 +1305,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);
@@ -1293,6 +1321,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;
@@ -1316,6 +1350,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)
@@ -1332,6 +1369,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)
@@ -1450,7 +1491,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

[PATCH v6 5/7] 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..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 v6 6/7] 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 4d3c6bd..243f618 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 6b4dd65..15fcd91 100644
--- a/config.c
+++ b/config.c
@@ -1515,8 +1515,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)
@@ -1557,10 +1561,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 v6 7/7] 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

Re: [PATCH v6 6/7] 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 '%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);
> + }

Extra whitespace before }.

Also, didn't we agree that it was a good thing to factor this
if/then/else into a helper function?

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

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> [Patch v6]: Added _(....) to error messages.
> 	Diff between v6 and v4 at the bottom.

Except for the tiny style issue in PATCH 6, the series is now

Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>

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

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

[Tanay Abhra]

On 7/31/2014 9:25 PM, Matthieu Moy wrote:
> 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 '%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);
>> + }
> 
> Extra whitespace before }.
> 
> Also, didn't we agree that it was a good thing to factor this
> if/then/else into a helper function?
> 

I have been thinking about it, wouldn't it be better to give users
a function like,

git_config_die_exact(key, value);

where user supplies key & value both and it would print the correct message based
on that. git_die_config() will be a wrapper over it, supplying the last value.
It will also replace the redundant code in git_config_raw() and help in the
multi value case. If the idea sounds okay, I will send a reroll.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/31/2014 9:25 PM, Matthieu Moy wrote:
>> 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 '%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);
>>> + }
>> 
>> Extra whitespace before }.
>> 
>> Also, didn't we agree that it was a good thing to factor this
>> if/then/else into a helper function?
>> 
>
> I have been thinking about it, wouldn't it be better to give users
> a function like,
>
> git_config_die_exact(key, value);
>
> where user supplies key & value both and it would print the correct message based
> on that.

I suggested git_config_die_linenr(key, linenr), and I now realize it
should take the value too.

You're suggesting git_config_die_exact(key, value). Is it a typo that
you forgot the line number, or is it intentional? If intentional, I
don't think it solves your issue:

[section]
   key
   key

There are two errors in this file, and you need to provide a line
number. key and value alone do not allow you to know which line the
error is. You can use a convention like "complain on the first value
equal to the argument", but I'm not sure that would always work. And
that seems backward to me to reconstruct the line number since the
function can be called from places where the line number is already
known (while iterating over the string_list for example).

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

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

[Tanay Abhra]

On 7/31/2014 10:22 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:
>>> 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 '%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);
>>>> + }
>>>
>>> Extra whitespace before }.
>>>
>>> Also, didn't we agree that it was a good thing to factor this
>>> if/then/else into a helper function?
>>>
>>
>> I have been thinking about it, wouldn't it be better to give users
>> a function like,
>>
>> git_config_die_exact(key, value);
>>
>> where user supplies key & value both and it would print the correct message based
>> on that.
> 
> I suggested git_config_die_linenr(key, linenr), and I now realize it
> should take the value too.
> 
> You're suggesting git_config_die_exact(key, value). Is it a typo that
> you forgot the line number, or is it intentional? If intentional, I
> don't think it solves your issue:
> 
> [section]
>    key
>    key
>
> There are two errors in this file, and you need to provide a line
> number. key and value alone do not allow you to know which line the
> error is. You can use a convention like "complain on the first value
> equal to the argument", but I'm not sure that would always work. And
> that seems backward to me to reconstruct the line number since the
> function can be called from places where the line number is already
> known (while iterating over the string_list for example).

Still the user would have to know that the linenr info is there.
First hear my argument, then we can go either way.

Let's first see the previous code behavior, git_config() would die on the
first corrupt value, we wouldn't live to see the future value.

for example,

[section]
	key	// error(old git_config() would die here)
	key = good_value

[section]
	key	//again error

Now for the new behavior,

single valued callers use git_config_get_value() which will directly
supply the last value, so we don't see the first error value.
For such cases, git_die_config(key) is enough.

The new git_config() works exactly as the old code, it would die
on the first case of erroneous value. Here, git_die_config_exact(key, value)
would be enough.

The last case is git_config_get_value_multi(), here we iterate over the keys,
and then call git_die_config_exact(key, value) for the erroneous value.
(pros and cons: abstracts the error message implementation from the user
but there is an extra call to git_config_get_value_multi(), but its cheap and
we are dying anyway)

+	if (load_config_refs) {
+		values = git_config_get_value_multi("notes.displayref");
+		if (values) {
+			for (i = 0; i < values->nr; i++) {
+				if (!values->items[i].string) {
+					config_error_nonbool("notes.displayref");
+					git_die_config_exact("notes.displayref", values->items[i].string);
+				}
+				else
+					string_list_add_refs_by_glob(&display_notes_refs,
+								     values->items[i].string);
+			}
+		}
+	}

What do you think?

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/31/2014 10:22 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:
>>>> 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 '%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);
>>>>> + }
>>>>
>>>> Extra whitespace before }.
>>>>
>>>> Also, didn't we agree that it was a good thing to factor this
>>>> if/then/else into a helper function?
>>>>
>>>
>>> I have been thinking about it, wouldn't it be better to give users
>>> a function like,
>>>
>>> git_config_die_exact(key, value);
>>>
>>> where user supplies key & value both and it would print the correct message based
>>> on that.
>> 
>> I suggested git_config_die_linenr(key, linenr), and I now realize it
>> should take the value too.
>> 
>> You're suggesting git_config_die_exact(key, value). Is it a typo that
>> you forgot the line number, or is it intentional? If intentional, I
>> don't think it solves your issue:
>> 
>> [section]
>>    key
>>    key
>>
>> There are two errors in this file, and you need to provide a line
>> number. key and value alone do not allow you to know which line the
>> error is. You can use a convention like "complain on the first value
>> equal to the argument", but I'm not sure that would always work. And
>> that seems backward to me to reconstruct the line number since the
>> function can be called from places where the line number is already
>> known (while iterating over the string_list for example).
>
> Still the user would have to know that the linenr info is there.
> First hear my argument, then we can go either way.
>
> Let's first see the previous code behavior, git_config() would die on the
> first corrupt value, we wouldn't live to see the future value.
>
> for example,
>
> [section]
> 	key	// error(old git_config() would die here)
> 	key = good_value
>
> [section]
> 	key	//again error
>
> Now for the new behavior,
>
> single valued callers use git_config_get_value() which will directly
> supply the last value, so we don't see the first error value.
> For such cases, git_die_config(key) is enough.

Yes. And it is better than the old behavior which was dying on the
erroneous value without giving a chance to the user to override the
boggus value in a more specific config file (e.g. if your sysadmin
messed-up /etc/gitconfig).

But since git_die_config(key) is simpler to use for the caller, it's
independant from git_die_config_exact()'s parameters.

> The new git_config() works exactly as the old code, it would die
> on the first case of erroneous value. Here, git_die_config_exact(key, value)
> would be enough.

Yes. But this callsite has the line number information, so it could use
it.

> The last case is git_config_get_value_multi(), here we iterate over the keys,
> and then call git_die_config_exact(key, value) for the erroneous value.
> (pros and cons: abstracts the error message implementation from the user
> but there is an extra call to git_config_get_value_multi(), but its cheap and
> we are dying anyway)

This is the part I find weird. You're calling git_die_config_exact() on
the first boggus value, and git_die_config_exact() will notify an error
at the line of the last boggus value.

I agree it works (if we give only one error message, it can be the first
or the last, the user doesn't really care), but I find the
implementation backwards. You have the line number already, as you are
iterating over the string_list which contain it. So why forget the line
number, and recompute one, possibly different, right after?

So, I see only cases where you already have the line number, hence no
reason to recompute it.

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

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

[Tanay Abhra]

On 7/31/2014 11:39 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 7/31/2014 10:22 PM, Matthieu Moy wrote:
>>> Tanay Abhra <tanayabh@gmail.com> writes:
>>>
>>>> On 7/31/2014 9:25 PM, Matthieu Moy wrote:
>>>>> 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 '%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);
>>>>>> + }
>>>>>
>>>>> Extra whitespace before }.
>>>>>
>>>>> Also, didn't we agree that it was a good thing to factor this
>>>>> if/then/else into a helper function?
>>>>>
>>>>
>>>> I have been thinking about it, wouldn't it be better to give users
>>>> a function like,
>>>>
>>>> git_config_die_exact(key, value);
>>>>
>>>> where user supplies key & value both and it would print the correct message based
>>>> on that.
>>>
>>> I suggested git_config_die_linenr(key, linenr), and I now realize it
>>> should take the value too.
>>>
>>> You're suggesting git_config_die_exact(key, value). Is it a typo that
>>> you forgot the line number, or is it intentional? If intentional, I
>>> don't think it solves your issue:
>>>
>>> [section]
>>>    key
>>>    key
>>>
>>> There are two errors in this file, and you need to provide a line
>>> number. key and value alone do not allow you to know which line the
>>> error is. You can use a convention like "complain on the first value
>>> equal to the argument", but I'm not sure that would always work. And
>>> that seems backward to me to reconstruct the line number since the
>>> function can be called from places where the line number is already
>>> known (while iterating over the string_list for example).
>>
>> Still the user would have to know that the linenr info is there.
>> First hear my argument, then we can go either way.
>>
>> Let's first see the previous code behavior, git_config() would die on the
>> first corrupt value, we wouldn't live to see the future value.
>>
>> for example,
>>
>> [section]
>> 	key	// error(old git_config() would die here)
>> 	key = good_value
>>
>> [section]
>> 	key	//again error
>>
>> Now for the new behavior,
>>
>> single valued callers use git_config_get_value() which will directly
>> supply the last value, so we don't see the first error value.
>> For such cases, git_die_config(key) is enough.
> 
> Yes. And it is better than the old behavior which was dying on the
> erroneous value without giving a chance to the user to override the
> boggus value in a more specific config file (e.g. if your sysadmin
> messed-up /etc/gitconfig).
> 
> But since git_die_config(key) is simpler to use for the caller, it's
> independant from git_die_config_exact()'s parameters.
> 
>> The new git_config() works exactly as the old code, it would die
>> on the first case of erroneous value. Here, git_die_config_exact(key, value)
>> would be enough.
> 
> Yes. But this callsite has the line number information, so it could use
> it.
> 
>> The last case is git_config_get_value_multi(), here we iterate over the keys,
>> and then call git_die_config_exact(key, value) for the erroneous value.
>> (pros and cons: abstracts the error message implementation from the user
>> but there is an extra call to git_config_get_value_multi(), but its cheap and
>> we are dying anyway)
> 
> This is the part I find weird. You're calling git_die_config_exact() on
> the first boggus value, and git_die_config_exact() will notify an error
> at the line of the last boggus value.
>

Hmn, we may have some confusion here. I meant the implementation of
git_config_exact() to look like this,

void git_die_config_exact(const char *key, const char *value)
{
	int i;
	const struct string_list *values;
	struct key_value_info *kv_info;
	values = git_config_get_value_multi(key);
	for (i = 0; i < values.nr; i++) {
		kv_info = values->items[i].util;
		/* A null check will be here also */
		if (!strcmp(value, values->items[i].string)) {
			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);
		}
	}
}

The above code would print the info on first bogus value.
I am only rooting for it because the caller has to think very little to use it.
It's your call, I am open to both ideas.

> I agree it works (if we give only one error message, it can be the first
> or the last, the user doesn't really care), but I find the
> implementation backwards. You have the line number already, as you are
> iterating over the string_list which contain it. So why forget the line
> number, and recompute one, possibly different, right after?
> 
> So, I see only cases where you already have the line number, hence no
> reason to recompute it.
> 

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/31/2014 11:39 PM, Matthieu Moy wrote:
>
>> This is the part I find weird. You're calling git_die_config_exact() on
>> the first boggus value, and git_die_config_exact() will notify an error
>> at the line of the last boggus value.
>
> Hmn, we may have some confusion here. I meant the implementation of
> git_config_exact() to look like this,
>
> void git_die_config_exact(const char *key, const char *value)
> {
> 	int i;
> 	const struct string_list *values;
> 	struct key_value_info *kv_info;
> 	values = git_config_get_value_multi(key);
> 	for (i = 0; i < values.nr; i++) {
> 		kv_info = values->items[i].util;
> 		/* A null check will be here also */
> 		if (!strcmp(value, values->items[i].string)) {
> 			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);
> 		}
> 	}
> }
>
> The above code would print the info on first bogus value.

OK, I got confused because git_die_config() errors out at the first
error. So, your version works, but I do not see any added value in this
extra complexity.

To be cleary, my version would be

NORETURN static /* not sure about the "static" */
void git_die_config_linenr(const char *key,
                           const char *filename, int linenr)
{
	if (!linenr)
		die(_("unable to parse '%s' from command-line config"), key);
	else
		die(_("bad config variable '%s' at file line %d in %s"),
			key,
			linenr,
			filename);
}

(hmm, I actually do not need the value, it's not printed)

NORETURN
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;
	git_die_config_linenr(key, kv_info->linenr, kv_info->filename);
}

(we forgot the NORETURN in previous version BTW. It should be there in
both functions here and in the .h file)

In my version, git_die_config uses git_die_config_linenr, and there's no
issue with first/last boggus value. Callers of git_die_config_linenr
have all the information to call it directly.

There are 3 code path that leads to the final die() calls, and having
this little helper allows making sure the messages are the same for
every callers, by construction (as opposed to cut-and-pasting).

Really, I don't see any reason to do anything more complex.

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

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

[Junio C Hamano]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> On 7/31/2014 11:39 PM, Matthieu Moy wrote:
>>
>>> This is the part I find weird. You're calling git_die_config_exact() on
>>> the first boggus value, and git_die_config_exact() will notify an error
>>> at the line of the last boggus value.
>>
>> Hmn, we may have some confusion here. I meant the implementation of
>> git_config_exact() to look like this,
>>
>> void git_die_config_exact(const char *key, const char *value)
>> {
>> 	int i;
>> 	const struct string_list *values;
>> 	struct key_value_info *kv_info;
>> 	values = git_config_get_value_multi(key);
>> 	for (i = 0; i < values.nr; i++) {
>> 		kv_info = values->items[i].util;
>> 		/* A null check will be here also */
>> 		if (!strcmp(value, values->items[i].string)) {
>> 			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);
>> 		}
>> 	}
>> }
>>
>> The above code would print the info on first bogus value.
>
> OK, I got confused because git_die_config() errors out at the first
> error. So, your version works, but I do not see any added value in this
> extra complexity.

Hmm, I am still confused ;-)

Can there be more than one 'i' whose value->items[i].string is the
same as the given value?  IOW, if you have "[user] name<EOL>" in
both .git/config and ~/.gitconfig, don't we want to make sure that
we complain on the one in .git/config, which would have taken
precedence if it were spelled correctly?

Your version below makes it very clear that you only complain on the
last one, no matter how many identical copies of the value for the
given key are defined incorrectly the same way, which is easier to
understand what is going on.

> To be cleary, my version would be
>
> NORETURN static /* not sure about the "static" */
> void git_die_config_linenr(const char *key,
>                            const char *filename, int linenr)
> {
> 	if (!linenr)
> 		die(_("unable to parse '%s' from command-line config"), key);
> 	else
> 		die(_("bad config variable '%s' at file line %d in %s"),
> 			key,
> 			linenr,
> 			filename);
> }
>
> (hmm, I actually do not need the value, it's not printed)
>
> NORETURN
> 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;
> 	git_die_config_linenr(key, kv_info->linenr, kv_info->filename);
> }
>
> (we forgot the NORETURN in previous version BTW. It should be there in
> both functions here and in the .h file)
>
> In my version, git_die_config uses git_die_config_linenr, and there's no
> issue with first/last boggus value. Callers of git_die_config_linenr
> have all the information to call it directly.
>
> There are 3 code path that leads to the final die() calls, and having
> this little helper allows making sure the messages are the same for
> every callers, by construction (as opposed to cut-and-pasting).
>
> Really, I don't see any reason to do anything more complex.

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

[Matthieu Moy]

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

> Hmm, I am still confused ;-)
>
> Can there be more than one 'i' whose value->items[i].string is the
> same as the given value?  IOW, if you have "[user] name<EOL>" in
> both .git/config and ~/.gitconfig, don't we want to make sure that
> we complain on the one in .git/config, which would have taken
> precedence if it were spelled correctly?

For single-valued variables, yes. And for them, git_die_config(), as
currently implemented does the right thing. My proposed change does not
modify the behavior, it just extracts error messages and an if/then/else
into a helper that can be used elsewhere.

For multi-valued variables, it's different. If you have a boggus value,
further values will not override it. It will remain bogus.

The code consuming such multi-valued variable has to iterate over the
string_list, and raise an error on bogus values. Ideally, it could show
all the errors, but it's also reasonable to die() at the first one. The
simplest behavior to implement is to die() at the first error.

(currently, git _segfaults_ at the first NULL value for
notes.displayref, so it can't be much worse)

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

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

[Tanay Abhra]

On 8/1/2014 12:11 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 7/31/2014 11:39 PM, Matthieu Moy wrote:
>>
>>> This is the part I find weird. You're calling git_die_config_exact() on
>>> the first boggus value, and git_die_config_exact() will notify an error
>>> at the line of the last boggus value.
>>
>> Hmn, we may have some confusion here. I meant the implementation of
>> git_config_exact() to look like this,
>>
>> void git_die_config_exact(const char *key, const char *value)
>> {
>> 	int i;
>> 	const struct string_list *values;
>> 	struct key_value_info *kv_info;
>> 	values = git_config_get_value_multi(key);
>> 	for (i = 0; i < values.nr; i++) {
>> 		kv_info = values->items[i].util;
>> 		/* A null check will be here also */
>> 		if (!strcmp(value, values->items[i].string)) {
>> 			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);
>> 		}
>> 	}
>> }
>>
>> The above code would print the info on first bogus value.
> 
> OK, I got confused because git_die_config() errors out at the first
> error. So, your version works, but I do not see any added value in this
> extra complexity.
> 
> To be cleary, my version would be
> 
> NORETURN static /* not sure about the "static" */
> void git_die_config_linenr(const char *key,
>                            const char *filename, int linenr)
> {
> 	if (!linenr)
> 		die(_("unable to parse '%s' from command-line config"), key);
> 	else
> 		die(_("bad config variable '%s' at file line %d in %s"),
> 			key,
> 			linenr,
> 			filename);
> }
> 
> (hmm, I actually do not need the value, it's not printed)
> 
> NORETURN
> 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;
> 	git_die_config_linenr(key, kv_info->linenr, kv_info->filename);
> }
> 
> (we forgot the NORETURN in previous version BTW. It should be there in
> both functions here and in the .h file)
> 
> In my version, git_die_config uses git_die_config_linenr, and there's no
> issue with first/last boggus value. Callers of git_die_config_linenr
> have all the information to call it directly.
> 
> There are 3 code path that leads to the final die() calls, and having
> this little helper allows making sure the messages are the same for
> every callers, by construction (as opposed to cut-and-pasting).
> 
> Really, I don't see any reason to do anything more complex.
>

Thanks, I am sending your version with the reroll.
Also, for clarity the multi value use case would look like,

	struct key_value_info *kv_info;
	if (load_config_refs) {
		values = git_config_get_value_multi("notes.displayref");
		if (values) {
			for (i = 0; i < values->nr; i++) {
				if (!values->items[i].string) {
					config_error_nonbool("notes.displayref");
					kv_info = values->items[i].util;
					git_die_config_linenr("notes.displayref",
							      kv_info->filename,
							      kv_info->linenr);
				}
				else
					string_list_add_refs_by_glob(&display_notes_refs,
								     values->items[i].string);
			}
		}
	}

with my function it would have looked like,

	if (load_config_refs) {
		values = git_config_get_value_multi("notes.displayref");
		if (values) {
			for (i = 0; i < values->nr; i++) {
				if (!values->items[i].string) {
					config_error_nonbool("notes.displayref");
					git_die_config_exact("notes.displayref", values->items[i].string);
				}
				else
					string_list_add_refs_by_glob(&display_notes_refs,
								     values->items[i].string);
			}
		}
	}

Thanks.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Thanks, I am sending your version with the reroll.
> Also, for clarity the multi value use case would look like,
>
> 	struct key_value_info *kv_info;
> 	if (load_config_refs) {
> 		values = git_config_get_value_multi("notes.displayref");
> 		if (values) {
> 			for (i = 0; i < values->nr; i++) {
> 				if (!values->items[i].string) {
> 					config_error_nonbool("notes.displayref");
> 					kv_info = values->items[i].util;
> 					git_die_config_linenr("notes.displayref",
> 							      kv_info->filename,
> 							      kv_info->linenr);
> 				}
> 				else
> 					string_list_add_refs_by_glob(&display_notes_refs,
> 								     values->items[i].string);
> 			}
> 		}
> 	}
>
> with my function it would have looked like,
>
> 	if (load_config_refs) {
> 		values = git_config_get_value_multi("notes.displayref");
> 		if (values) {
> 			for (i = 0; i < values->nr; i++) {
> 				if (!values->items[i].string) {
> 					config_error_nonbool("notes.displayref");
> 					git_die_config_exact("notes.displayref", values->items[i].string);
> 				}
> 				else
> 					string_list_add_refs_by_glob(&display_notes_refs,
> 								     values->items[i].string);
> 			}
> 		}
> 	}

I still think that checking for non-null values should be done in a
helper in config.c, and then the code would look like

 	if (load_config_refs) {
 		values = git_config_get_value_multi_nonbool("notes.displayref");
 		if (values)
 			for (i = 0; i < values->nr; i++)
				string_list_add_refs_by_glob(&display_notes_refs,
							     values->items[i].string);
 	}

The same helper could at least be used for "branch.<remote>.merge".

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