[PATCH v4 0/5] git_config callers rewritten with the new config cache API

[PATCH v4 0/5] git_config callers rewritten with the new config cache API

[Tanay Abhra]

[PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked.
	One thing to check is if the config variables I changed in the series
	are single valued or multi valued. Though I have tried to ascertain
	if the variable was single valued or not by reading the docs and code,
	mistakes may creep in.

[PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion.
	Also, new helpers introduced in v7 of the config-set API series have been used.

This series builds on the top of 4c715ebb in pu (ta/config-set).
See [1] for the documentation of the new API functions and a general description of the
new API.

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

Tanay Abhra (5):

 alias.c     | 25 ++++++-------------------
 branch.c    | 24 ++++--------------------
 imap-send.c | 61 +++++++++++++++++++++++++++----------------------------------
 notes.c     | 29 ++++++++++++++---------------
 pager.c     | 40 +++++++++++++---------------------------
 5 files changed, 64 insertions(+), 115 deletions(-)

-- 
1.9.0.GIT

[PATCH v4 1/5] pager.c: replace `git_config()` with `git_config_get_value()`

[Tanay Abhra]

Use `git_config_get_value()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 pager.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/pager.c b/pager.c
index 8b5cbc5..b7eb7e7 100644
--- a/pager.c
+++ b/pager.c
@@ -6,12 +6,6 @@
 #define DEFAULT_PAGER "less"
 #endif
 
-struct pager_config {
-	const char *cmd;
-	int want;
-	char *value;
-};
-
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -155,30 +149,22 @@ int decimal_width(int number)
 	return width;
 }
 
-static int pager_command_config(const char *var, const char *value, void *data)
+/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
+int check_pager_config(const char *cmd)
 {
-	struct pager_config *c = data;
-	if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
-		int b = git_config_maybe_bool(var, value);
+	int want = -1;
+	struct strbuf key = STRBUF_INIT;
+	const char *value = NULL;
+	strbuf_addf(&key, "pager.%s", cmd);
+	if (!git_config_get_value(key.buf, &value)) {
+		int b = git_config_maybe_bool(key.buf, value);
 		if (b >= 0)
-			c->want = b;
+			want = b;
 		else {
-			c->want = 1;
-			c->value = xstrdup(value);
+			want = 1;
+			pager_program = xstrdup(value);
 		}
 	}
-	return 0;
-}
-
-/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
-int check_pager_config(const char *cmd)
-{
-	struct pager_config c;
-	c.cmd = cmd;
-	c.want = -1;
-	c.value = NULL;
-	git_config(pager_command_config, &c);
-	if (c.value)
-		pager_program = c.value;
-	return c.want;
+	strbuf_release(&key);
+	return want;
 }
-- 
1.9.0.GIT

[PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

[Tanay Abhra]

Use `git_config_get_value_multi()` instead of `git_config()` to take
advantage of the config-set API which provides a cleaner control flow,
also previously 'string_list_add_refs_by_glob()' was called even when
the retrieved value was NULL, correct it while we are at it.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 notes.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/notes.c b/notes.c
index 5fe691d..abb0ce0 100644
--- a/notes.c
+++ b/notes.c
@@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	free(globs_copy);
 }
 
-static int notes_display_config(const char *k, const char *v, void *cb)
-{
-	int *load_refs = cb;
-
-	if (*load_refs && !strcmp(k, "notes.displayref")) {
-		if (!v)
-			config_error_nonbool(k);
-		string_list_add_refs_by_glob(&display_notes_refs, v);
-	}
-
-	return 0;
-}
-
 const char *default_notes_ref(void)
 {
 	const char *notes_ref = NULL;
@@ -1041,7 +1028,8 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 void init_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
-	int load_config_refs = 0;
+	const struct string_list *values;
+	int load_config_refs = 0, i;
 	display_notes_refs.strdup_strings = 1;
 
 	assert(!display_notes_trees);
@@ -1058,7 +1046,18 @@ void init_display_notes(struct display_notes_opt *opt)
 			load_config_refs = 1;
 	}
 
-	git_config(notes_display_config, &load_config_refs);
+	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");
+				else
+					string_list_add_refs_by_glob(&display_notes_refs,
+								     values->items[i].string);
+			}
+		}
+	}
 
 	if (opt) {
 		struct string_list_item *item;
-- 
1.9.0.GIT

[PATCH v4 3/5] imap-send.c: replace `git_config()` with `git_config_get_*()` family

[Tanay Abhra]

Use `git_config_get_*()` family instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 imap-send.c | 61 +++++++++++++++++++++++++++----------------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 524fbab..586bdd8 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,36 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
 
 static char *imap_folder;
 
-static int git_imap_config(const char *key, const char *val, void *cb)
+static void git_imap_config(void)
 {
-	if (!skip_prefix(key, "imap.", &key))
-		return 0;
+	const char *val = NULL;
+
+	git_config_get_bool("imap.sslverify", &server.ssl_verify);
+	git_config_get_bool("imap.preformattedhtml", &server.use_html);
+	git_config_get_string("imap.folder", &imap_folder);
 
-	/* check booleans first, and barf on others */
-	if (!strcmp("sslverify", key))
-		server.ssl_verify = git_config_bool(key, val);
-	else if (!strcmp("preformattedhtml", key))
-		server.use_html = git_config_bool(key, val);
-	else if (!val)
-		return config_error_nonbool(key);
-
-	if (!strcmp("folder", key)) {
-		imap_folder = xstrdup(val);
-	} else if (!strcmp("host", key)) {
-		if (starts_with(val, "imap:"))
-			val += 5;
-		else if (starts_with(val, "imaps:")) {
-			val += 6;
-			server.use_ssl = 1;
+	if (!git_config_get_value("imap.host", &val)) {
+		if (!val) {
+			config_error_nonbool("imap.host");
+			git_die_config("imap.host");
+		} else {
+			if (starts_with(val, "imap:"))
+				val += 5;
+			else if (starts_with(val, "imaps:")) {
+				val += 6;
+				server.use_ssl = 1;
+			}
+			if (starts_with(val, "//"))
+				val += 2;
+			server.host = xstrdup(val);
 		}
-		if (starts_with(val, "//"))
-			val += 2;
-		server.host = xstrdup(val);
-	} else if (!strcmp("user", key))
-		server.user = xstrdup(val);
-	else if (!strcmp("pass", key))
-		server.pass = xstrdup(val);
-	else if (!strcmp("port", key))
-		server.port = git_config_int(key, val);
-	else if (!strcmp("tunnel", key))
-		server.tunnel = xstrdup(val);
-	else if (!strcmp("authmethod", key))
-		server.auth_method = xstrdup(val);
+	}
 
-	return 0;
+	git_config_get_string("imap.user", &server.user);
+	git_config_get_string("imap.pass", &server.pass);
+	git_config_get_int("imap.port", &server.port);
+	git_config_get_string("imap.tunnel", &server.tunnel);
+	git_config_get_string("imap.authmethod", &server.auth_method);
 }
 
 int main(int argc, char **argv)
@@ -1383,7 +1376,7 @@ int main(int argc, char **argv)
 		usage(imap_send_usage);
 
 	setup_git_directory_gently(&nongit_ok);
-	git_config(git_imap_config, NULL);
+	git_imap_config();
 
 	if (!server.port)
 		server.port = server.use_ssl ? 993 : 143;
-- 
1.9.0.GIT

[PATCH v4 4/5] branch.c: replace `git_config()` with `git_config_get_string()

[Tanay Abhra]

Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 branch.c | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/branch.c b/branch.c
index 46e8aa8..df6b120 100644
--- a/branch.c
+++ b/branch.c
@@ -140,33 +140,17 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,
 	return 0;
 }
 
-struct branch_desc_cb {
-	const char *config_name;
-	const char *value;
-};
-
-static int read_branch_desc_cb(const char *var, const char *value, void *cb)
-{
-	struct branch_desc_cb *desc = cb;
-	if (strcmp(desc->config_name, var))
-		return 0;
-	free((char *)desc->value);
-	return git_config_string(&desc->value, var, value);
-}
-
 int read_branch_desc(struct strbuf *buf, const char *branch_name)
 {
-	struct branch_desc_cb cb;
+	char *v = NULL;
 	struct strbuf name = STRBUF_INIT;
 	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) {
+	if (git_config_get_string(name.buf, &v)) {
 		strbuf_release(&name);
 		return -1;
 	}
-	if (cb.value)
-		strbuf_addstr(buf, cb.value);
+	strbuf_addstr(buf, v);
+	free(v);
 	strbuf_release(&name);
 	return 0;
 }
-- 
1.9.0.GIT

[PATCH v4 5/5] alias.c: replace `git_config()` with `git_config_get_string()`

[Tanay Abhra]

Use `git_config_get_string()` instead of `git_config()` to take advantage of
the config-set API which provides a cleaner control flow.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 alias.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/alias.c b/alias.c
index 758c867..6aa164a 100644
--- a/alias.c
+++ b/alias.c
@@ -1,26 +1,13 @@
 #include "cache.h"
 
-static const char *alias_key;
-static char *alias_val;
-
-static int alias_lookup_cb(const char *k, const char *v, void *cb)
-{
-	const char *name;
-	if (skip_prefix(k, "alias.", &name) && !strcmp(name, alias_key)) {
-		if (!v)
-			return config_error_nonbool(k);
-		alias_val = xstrdup(v);
-		return 0;
-	}
-	return 0;
-}
-
 char *alias_lookup(const char *alias)
 {
-	alias_key = alias;
-	alias_val = NULL;
-	git_config(alias_lookup_cb, NULL);
-	return alias_val;
+	char *v = NULL;
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "alias.%s", alias);
+	git_config_get_string(key.buf, &v);
+	strbuf_release(&key);
+	return v;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
-- 
1.9.0.GIT

Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked.
> 	One thing to check is if the config variables I changed in the series
> 	are single valued or multi valued. Though I have tried to ascertain
> 	if the variable was single valued or not by reading the docs and code,
> 	mistakes may creep in.
>
> [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion.
> 	Also, new helpers introduced in v7 of the config-set API series have been used.
>
> This series builds on the top of 4c715ebb in pu (ta/config-set).

I think it semantically needs your other WIP series, that makes
git_config_get_* die in case of error. Otherwise, there's an unexpected
behavior change in case of error.

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

Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API

[Tanay Abhra]

On 7/30/2014 7:16 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> [PATCH v4]: Tiny style nits corrected. Patch 2/5 has been totally reworked.
>> 	One thing to check is if the config variables I changed in the series
>> 	are single valued or multi valued. Though I have tried to ascertain
>> 	if the variable was single valued or not by reading the docs and code,
>> 	mistakes may creep in.
>>
>> [PATCH v3]: Most of Eric's suggestions has been implemented. See [2] for discussion.
>> 	Also, new helpers introduced in v7 of the config-set API series have been used.
>>
>> This series builds on the top of 4c715ebb in pu (ta/config-set).
> 
> I think it semantically needs your other WIP series, that makes
> git_config_get_* die in case of error. Otherwise, there's an unexpected
> behavior change in case of error.
>

Yes you are right, there is a call to git_die_config() also in the series. I will add
the info in the next reroll. Also, any thoughts on what to do with git_default_config()?
We can,

1> make a new function git_load_default_config(), use it for the rewrites.

or

2> git_default_config() is rewritten to be loaded only once even if it is called
again and again during the process run, so that we use the same function in callbacks
and in the new API using rewrites.

Thanks.

Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> -	git_config(notes_display_config, &load_config_refs);
> +	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");
> +				else
> +					string_list_add_refs_by_glob(&display_notes_refs,
> +								     values->items[i].string);
> +			}
> +		}
> +	}

It seems to me that you're doing a lot here that should have been done
once in the config API:

* if (values) {
          for (i = 0; i < values->nr

  => We could avoid the "if" statement if git_config_get_value_multi was
  always returning a string_list, possibly empty (values->nr == 0
  instead of values == NULL).

  Not as obvious as it seems, because you normally return a pointer to
  the string_list that is already in the hashmap, so you can't just
  malloc() an empty one if you don't want to leak it.

  Another option would be to provide an iterator that would call a
  function on each value of the list, and do nothing when there's no
  list at all (back to the callback-style API, but you would iterate
  only through the values for the right key).

* if (!values->items[i].string)
          config_error_nonbool(

  => This check could be done once and for all in a function, say
  git_config_get_value_multi_nonbool, a trivial wrapper around
  git_config_get_value_multi like

const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)
{
	struct string_list l = git_configset_get_value_multi(cs, key);
        // possibly if(l) depending on the point above.
	for (i = 0; i < values->nr; i++) {
		if (!values->items[i].string)
			git_config_die(key);
	}
	return l;
}

const struct string_list *git_config_get_value_multi_nonbool(const char *key)
{
	git_config_check_init();
	return git_configset_get_value_multi_nonbool(&the_config_set, key);
}


  (totally untested)

  BTW, is it intentional that you call config_error_nonbool() without
  die-ing?

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

Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

[Tanay Abhra]

On 7/30/2014 7:43 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> -	git_config(notes_display_config, &load_config_refs);
>> +	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");
>> +				else
>> +					string_list_add_refs_by_glob(&display_notes_refs,
>> +								     values->items[i].string);
>> +			}
>> +		}
>> +	}
> 
> It seems to me that you're doing a lot here that should have been done
> once in the config API:
> 
> * if (values) {
>           for (i = 0; i < values->nr
> 
>   => We could avoid the "if" statement if git_config_get_value_multi was
>   always returning a string_list, possibly empty (values->nr == 0
>   instead of values == NULL).
>

or we can do something like,

	if (!git_config_get_value_multi("notes.displayref", &values)) {
		/* return 0 if there is a value_list for the key */

>   Not as obvious as it seems, because you normally return a pointer to
>   the string_list that is already in the hashmap, so you can't just
>   malloc() an empty one if you don't want to leak it.
> 
>   Another option would be to provide an iterator that would call a
>   function on each value of the list, and do nothing when there's no
>   list at all (back to the callback-style API, but you would iterate
>   only through the values for the right key).
>

This is also a good idea, but still we are back to the callback API,
and what we are gaining is fewer loop iterations than git_config().

Which way do you prefer, a reroll is easy but Junio might have been sick
of replacing the patches in pu by now. :)

> * if (!values->items[i].string)
>           config_error_nonbool(
> 
>   => This check could be done once and for all in a function, say
>   git_config_get_value_multi_nonbool, a trivial wrapper around
>   git_config_get_value_multi like
> 
> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)
> {
> 	struct string_list l = git_configset_get_value_multi(cs, key);
>         // possibly if(l) depending on the point above.
> 	for (i = 0; i < values->nr; i++) {
> 		if (!values->items[i].string)
> 			git_config_die(key);
> 	}
> 	return l;
> }
>

Not worth it, most the multi value calls do not die on a nonbool.

> const struct string_list *git_config_get_value_multi_nonbool(const char *key)
> {
> 	git_config_check_init();
> 	return git_configset_get_value_multi_nonbool(&the_config_set, key);
> }
> 
> 
>   (totally untested)
> 
>   BTW, is it intentional that you call config_error_nonbool() without
>   die-ing?
>

Yup, it's intentional, original code didn't die for empty values, and it seemed
logical to me to emulate that over to the rewrite.

Re: [PATCH v4 4/5] branch.c: replace `git_config()` with `git_config_get_string()

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>  int read_branch_desc(struct strbuf *buf, const char *branch_name)
>  {
> -	struct branch_desc_cb cb;
> +	char *v = NULL;
>  	struct strbuf name = STRBUF_INIT;
>  	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) {
> +	if (git_config_get_string(name.buf, &v)) {
>  		strbuf_release(&name);
>  		return -1;
>  	}
> -	if (cb.value)
> -		strbuf_addstr(buf, cb.value);
> +	strbuf_addstr(buf, v);
> +	free(v);
>  	strbuf_release(&name);
>  	return 0;
>  }

I think this is a behavior change. if (git_config(read_branch_desc_cb,
&cb) < 0) was never true in practice, so the "return -1" was essentially
dead code. You now return -1 when no value is found.

It probably doesn't matter, since all caller except
fmt-merge-msg.c:add_branch_desc() ignore the return value, and if I read
correctly, add_branch_desc does not need the test on the return value,
as the then branch of the if does nothing if no value is found anyway.

But here again, I have to wonder why the function does not just return
void.

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

Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/30/2014 7:43 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> -	git_config(notes_display_config, &load_config_refs);
>>> +	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");
>>> +				else
>>> +					string_list_add_refs_by_glob(&display_notes_refs,
>>> +								     values->items[i].string);
>>> +			}
>>> +		}
>>> +	}
>> 
>> It seems to me that you're doing a lot here that should have been done
>> once in the config API:
>> 
>> * if (values) {
>>           for (i = 0; i < values->nr
>> 
>>   => We could avoid the "if" statement if git_config_get_value_multi was
>>   always returning a string_list, possibly empty (values->nr == 0
>>   instead of values == NULL).
>>
>
> or we can do something like,
>
> 	if (!git_config_get_value_multi("notes.displayref", &values)) {
> 		/* return 0 if there is a value_list for the key */
>
>>   Not as obvious as it seems, because you normally return a pointer to
>>   the string_list that is already in the hashmap, so you can't just
>>   malloc() an empty one if you don't want to leak it.
>> 
>>   Another option would be to provide an iterator that would call a
>>   function on each value of the list, and do nothing when there's no
>>   list at all (back to the callback-style API, but you would iterate
>>   only through the values for the right key).
>>
>
> This is also a good idea, but still we are back to the callback API,
> and what we are gaining is fewer loop iterations than git_config().

Regardless of performance, the code would also be a bit shorter, since
the callback just gets the values for the right key, so it doesn't need
to re-test that the key is the right one.

Here, the callback would basically be the body of the for loop above.

> Which way do you prefer, a reroll is easy but Junio might have been sick
> of replacing the patches in pu by now. :)

No need to replace anything, you can add new helpers on top of the
existing.

Do it the way you feel is better, I'm just giving ideas.

>> * if (!values->items[i].string)
>>           config_error_nonbool(
>> 
>>   => This check could be done once and for all in a function, say
>>   git_config_get_value_multi_nonbool, a trivial wrapper around
>>   git_config_get_value_multi like
>> 
>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)
>> {
>> 	struct string_list l = git_configset_get_value_multi(cs, key);
>>         // possibly if(l) depending on the point above.
>> 	for (i = 0; i < values->nr; i++) {
>> 		if (!values->items[i].string)
>> 			git_config_die(key);
>> 	}
>> 	return l;
>> }
>>
>
> Not worth it, most the multi value calls do not die on a nonbool.

Can you cite some multi-value variables that can be nonbool? I can't
find many multi-valued variables, and I can't find any which would allow
bool and nonbool.

>> const struct string_list *git_config_get_value_multi_nonbool(const char *key)
>> {
>> 	git_config_check_init();
>> 	return git_configset_get_value_multi_nonbool(&the_config_set, key);
>> }
>> 
>> 
>>   (totally untested)
>> 
>>   BTW, is it intentional that you call config_error_nonbool() without
>>   die-ing?
>>
>
> Yup, it's intentional, original code didn't die for empty values, and it seemed
> logical to me to emulate that over to the rewrite.

The old code was doing

	if (*load_refs && !strcmp(k, "notes.displayref")) {
		if (!v)
			config_error_nonbool(k);
		string_list_add_refs_by_glob(&display_notes_refs, v);

It seems that the intent of the programmer was

	if (*load_refs && !strcmp(k, "notes.displayref")) {
		if (!v)
			return config_error_nonbool(k); // <---------------
		string_list_add_refs_by_glob(&display_notes_refs, v);

At least, that would explain why the code uses v even after testing that
it is a NULL pointer.

You're already fixing a bug in your patch by not using NULL values, but
then I don't see any reason to keep the old weird behavior (display an
error but do not die).

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

Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Yes you are right, there is a call to git_die_config() also in the series. I will add
> the info in the next reroll.

If unsure, rebase your branch locally on the commit on which it is meant
to apply, and check that it works for you.

> Also, any thoughts on what to do with git_default_config()? We can,
>
> 1> make a new function git_load_default_config(), use it for the rewrites.

That seems the most sensible option. It could be called it git.c before
the command-dependant part, so that any call to git loads this.

I didn't check if it was correct (e.g. do some command rely on the fact
that the default config is not loaded?)

> 2> git_default_config() is rewritten to be loaded only once even if it is called
> again and again during the process run, so that we use the same function in callbacks
> and in the new API using rewrites.

Seems like a workaround, and to me the point of your GSoC is to make the
code cleaner, hence avoid workaraounds ...

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

Re: [PATCH v4 0/5] git_config callers rewritten with the new config cache API

[Ramsay Jones]

On 30/07/14 17:45, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
>> Also, any thoughts on what to do with git_default_config()? We can,
>>
>> 1> make a new function git_load_default_config(), use it for the rewrites.
> 
> That seems the most sensible option. It could be called it git.c before
> the command-dependant part, so that any call to git loads this.
> 
> I didn't check if it was correct (e.g. do some command rely on the fact
> that the default config is not loaded?)
> 

Hmm, here be dragons ... :-P

I don't know that there has actually been any kind of policy
regarding the reading of config files/variables in git (or if
there is a different policy for plumbing vs porcelain), but it
has always seemed to be somewhat ad-hoc; each command decides
for itself what it wants to read.

However, with increased use of common code which _uses_ certain
config variables for correct operation, the 'choice' is much
harder to make (and may change after the fact!).

For example, about a year ago I submitted a couple of patches
which added a call to git_config(git_default_config, NULL) to
both 'git pack-refs' and 'git show-refs'. This was as a result
of the 'mh/ref-races' branch which introduced a 'stat_validity'
api for checking if the packed-refs file had changed on the
filesystem since last you looked. This re-used some of the same
code used to handle index updates that used config variables
like core.checkstat and core.trustctime. These config variables
can affect the correctness and/or the efficiency of the code on
some platforms (e.g. cygwin, mingw).

[Note: 'stat_validity' has since been re-used again (why not?)
in some shallow clone code, so similar comments may apply ...
I haven't looked.]

However, those patches were dropped, because it resulted in an
(unwanted) change in behaviour. In particular, 'git show-refs'
changed behaviour because it now 'listened' to core.abbrev!

I started to look at splitting the 'core config variables' into
two groups; essential variables that _all_ git commands should
read for correct/efficient/consistent behaviour and everything
else (mainly UI related variables).

However, something else came up ...

Just an FYI.

ATB,
Ramsay Jones

Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

[Matthieu Moy]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> On 7/30/2014 7:43 PM, Matthieu Moy wrote:
>>> * if (!values->items[i].string)
>>>           config_error_nonbool(
>>> 
>>>   => This check could be done once and for all in a function, say
>>>   git_config_get_value_multi_nonbool, a trivial wrapper around
>>>   git_config_get_value_multi like
>>> 
>>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)
>>> {
>>> 	struct string_list l = git_configset_get_value_multi(cs, key);
>>>         // possibly if(l) depending on the point above.
>>> 	for (i = 0; i < values->nr; i++) {
>>> 		if (!values->items[i].string)
>>> 			git_config_die(key);
>>> 	}
>>> 	return l;
>>> }
>>>
>>
>> Not worth it, most the multi value calls do not die on a nonbool.
>
> Can you cite some multi-value variables that can be nonbool? I can't
> find many multi-valued variables, and I can't find any which would allow
> bool and nonbool.

Thinking a bit more about it: we actually need more than your patch and
my code example above to give accurate error messages. Your code gives
no error message, and mine uses git_config_die(key); which gives the
line of the _last_ entry, but not necessarily the right line.

The right line number should be extracted from the info field of the
string_list. It's not completely trivial, hence I'd rather have a helper
doing it well in config.c than letting callers re-do the check and
possibly give wrong line in their error message as I did in my first
attempt.

I think you can introduce a helper git_config_die_linenr(key, linenr)
that displays the right error message. Then git_config_die becomes a
wrapper around it that does the lookup to find linenr from the hash.

You already have a duplicate piece of code in your other series:

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

That would be the content of git_config_die_linenr().

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

Re: [PATCH v4 2/5] notes.c: replace `git_config()` with `git_config_get_value_multi()`

[Tanay Abhra]

On 7/31/2014 5:08 PM, Matthieu Moy wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> On 7/30/2014 7:43 PM, Matthieu Moy wrote:
>>>> * if (!values->items[i].string)
>>>>           config_error_nonbool(
>>>>
>>>>   => This check could be done once and for all in a function, say
>>>>   git_config_get_value_multi_nonbool, a trivial wrapper around
>>>>   git_config_get_value_multi like
>>>>
>>>> const struct string_list *git_configset_get_value_multi_nonbool(struct config_set *cs, const char *key)
>>>> {
>>>> 	struct string_list l = git_configset_get_value_multi(cs, key);
>>>>         // possibly if(l) depending on the point above.
>>>> 	for (i = 0; i < values->nr; i++) {
>>>> 		if (!values->items[i].string)
>>>> 			git_config_die(key);
>>>> 	}
>>>> 	return l;
>>>> }
>>>>
>>>
>>> Not worth it, most the multi value calls do not die on a nonbool.
>>
>> Can you cite some multi-value variables that can be nonbool? I can't
>> find many multi-valued variables, and I can't find any which would allow
>> bool and nonbool.
> 
> Thinking a bit more about it: we actually need more than your patch and
> my code example above to give accurate error messages. Your code gives
> no error message, and mine uses git_config_die(key); which gives the
> line of the _last_ entry, but not necessarily the right line.
> 
> The right line number should be extracted from the info field of the
> string_list. It's not completely trivial, hence I'd rather have a helper
> doing it well in config.c than letting callers re-do the check and
> possibly give wrong line in their error message as I did in my first
> attempt.
> 
> I think you can introduce a helper git_config_die_linenr(key, linenr)
> that displays the right error message. Then git_config_die becomes a
> wrapper around it that does the lookup to find linenr from the hash.
> 
> You already have a duplicate piece of code in your other series:
> 
> 			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);
> 
> That would be the content of git_config_die_linenr().
> 

Thanks. I will add it in the next reroll.