[PATCH v3 0/6] git_config callers rewritten with the new config cache API

[PATCH v3 0/6] git_config callers rewritten with the new config cache API

[Tanay Abhra]

[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.
	See [1] for the documentation of the new functions.

This series builds on the top of 5def4132 in pu or topic[1] in the mailing list
with name "git config cache & special querying API utilizing the cache".

All patches pass every test, but there is a catch, there is slight behaviour
change in most of them where originally the callback returns
config_error_nonbool() when it sees a NULL value for a key causing a die
specified in git_parse_source in config.c.

The die also prints the file name and the line number as,

	"die("bad config file line %d in %s", cf->linenr, cf->name);"

We lose the fine grained error checking when switching to this method.
Still, I will try to correct this anomaly in my next series.

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

Tanay Abhra (6):

 alias.c       | 27 +++++++--------------------
 branch.c      | 24 ++++--------------------
 imap-send.c   | 41 +++++++++++++++--------------------------
 notes-utils.c | 33 ++++++++++++++++-----------------
 notes.c       | 21 +++++++--------------
 pager.c       | 40 +++++++++++++---------------------------
 6 files changed, 62 insertions(+), 124 deletions(-)

-- 
1.9.0.GIT

[PATCH v3 1/6] 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.
The function now raises an error instead of dying when a NULL value is found.

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

diff --git a/alias.c b/alias.c
index 758c867..a453bd8 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)
+char *alias_lookup(const char* alias)
 {
-	alias_key = alias;
-	alias_val = NULL;
-	git_config(alias_lookup_cb, NULL);
-	return alias_val;
+	const 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 (char*)v;
 }
 
 #define SPLIT_CMDLINE_BAD_ENDING 1
-- 
1.9.0.GIT

Re: [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> --- 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)
> +char *alias_lookup(const char* alias)

Style: keep the * stuck to alias.

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

Re: [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`

[Ramsay Jones]

On 21/07/14 12:12, Tanay Abhra wrote:
> Use `git_config_get_string()` instead of `git_config()` to take advantage of
> the config-set API which provides a cleaner control flow.
> The function now raises an error instead of dying when a NULL value is found.
> 
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  alias.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/alias.c b/alias.c
> index 758c867..a453bd8 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)
> +char *alias_lookup(const char* alias)

No, this is not C++. :-D

>  {
> -	alias_key = alias;
> -	alias_val = NULL;
> -	git_config(alias_lookup_cb, NULL);
> -	return alias_val;
> +	const 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 (char*)v;
>  }
>  
>  #define SPLIT_CMDLINE_BAD_ENDING 1
> 

Re: [PATCH v3 1/6] alias.c: replace `git_config()` with `git_config_get_string()`

[Samuel Bronson]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> On 21/07/14 12:12, Tanay Abhra wrote:
>> -char *alias_lookup(const char *alias)
>> +char *alias_lookup(const char* alias)
>
> No, this is not C++. :-D

Why would C++ make a difference?  Shouldn't you *never* do that?

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature to help me spread!

[PATCH v3 2/6] 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..827307f 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;
+	const 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((char*)v);
 	strbuf_release(&name);
 	return 0;
 }
-- 
1.9.0.GIT

Re: [PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> 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..827307f 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;
> +	const 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((char*)v);

In this cast, I smell an API mistake to insist an extra constness to
the output parameter of git_config_get_string() in [3/4] of the
previous series.  Unlike the underlying git_config_get_value(),
which lets the caller peek into the internal cached copy, the caller
of git_config_get_string() is given its own copy, and I do not
offhand see a good reason to forbid the caller from modifying it.

>  	strbuf_release(&name);
>  	return 0;
>  }

Re: [PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`

[Matthieu Moy]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> -	if (cb.value)
>> -		strbuf_addstr(buf, cb.value);
>> +	strbuf_addstr(buf, v);
>> +	free((char*)v);
>
> In this cast, I smell an API mistake to insist an extra constness to
> the output parameter of git_config_get_string() in [3/4] of the
> previous series.  Unlike the underlying git_config_get_value(),
> which lets the caller peek into the internal cached copy, the caller
> of git_config_get_string() is given its own copy, and I do not
> offhand see a good reason to forbid the caller from modifying it.

Indeed. My suggestion to change the declaration of char * variables
actually seem like a workaround for an API mistake after reading this.

(well, actually, the declaration could be modified if we think it brings
any new safety, but that should be another topic)

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

Re: [PATCH v3 2/6] branch.c: replace `git_config()` with `git_config_get_string()`

[Tanay Abhra]

On 7/21/2014 11:29 PM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> 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..827307f 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;
>> +	const 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((char*)v);
> 
> In this cast, I smell an API mistake to insist an extra constness to
> the output parameter of git_config_get_string() in [3/4] of the
> previous series.  Unlike the underlying git_config_get_value(),
> which lets the caller peek into the internal cached copy, the caller
> of git_config_get_string() is given its own copy, and I do not
> offhand see a good reason to forbid the caller from modifying it.
>

I modeled git_config_get_string() on the previously existing API function
git_config_string() with the signature,
int git_config_string(const char **dest, const char *var, const char *value).

But after writing this series I do think there isn't a good reason to
keep the constness in the new function also since the dest is given
its own copy.


>>  	strbuf_release(&name);
>>  	return 0;
>>  }

[PATCH v3 3/6] 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.
The function now raises an error instead of dying in cases where a NULL value is
not allowed.

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

diff --git a/imap-send.c b/imap-send.c
index 524fbab..b7ec98a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1326,43 +1326,35 @@ 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;
-
-	/* 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;
+	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", (const char**)&imap_folder);
+
+	if (!git_config_get_value("imap.host", &val)) {
+		if(!val)
+			config_error_nonbool("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", (const char**)&server.user);
+	git_config_get_string("imap.pass", (const char**)&server.pass);
+	git_config_get_string("imap.port", (const char**)&server.port);
+	git_config_get_string("imap.tunnel", (const char**)&server.tunnel);
+	git_config_get_string("imap.authmethod", (const char**)&server.auth_method);
 }
 
 int main(int argc, char **argv)
@@ -1383,7 +1375,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;
-- 

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

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> +	git_config_get_string("imap.folder", (const char**)&imap_folder);

The same "why (const char **)--is that an API mistake?" question
applies here and other calls to git_config_get_string() in this
patch.

[PATCH v3 4/6] notes.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, 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 | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/notes.c b/notes.c
index 5fe691d..20c20f5 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,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs)
 void init_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
+	const char *value = NULL;
 	int load_config_refs = 0;
 	display_notes_refs.strdup_strings = 1;
 
@@ -1058,7 +1046,12 @@ 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 && !git_config_get_value("notes.displayref", &value)) {
+		if (!value)
+			config_error_nonbool("notes.displayref");
+		else
+			string_list_add_refs_by_glob(&display_notes_refs, value);
+	}
 
 	if (opt) {
 		struct string_list_item *item;
-- 
1.9.0.GIT

[PATCH v3 5/6] 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 v3 6/6] notes-util.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.
The function now raises an error instead of dying when a NULL value is found
for key "notes.rewritemode".

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 notes-utils.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index b64dc1b..ffa2b70 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -69,22 +69,24 @@ static combine_notes_fn parse_combine_notes_fn(const char *v)
 		return NULL;
 }
 
-static int notes_rewrite_config(const char *k, const char *v, void *cb)
+static void notes_rewrite_config(struct notes_rewrite_cfg *c)
 {
-	struct notes_rewrite_cfg *c = cb;
-	if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
-		c->enabled = git_config_bool(k, v);
-		return 0;
-	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
+	const char *v;
+	struct strbuf key = STRBUF_INIT;
+	strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
+	git_config_get_bool(key.buf, &c->enabled);
+	strbuf_release(&key);
+
+	if (!c->mode_from_env && !git_config_get_value("notes.rewritemode", &v)) {
 		if (!v)
-			return config_error_nonbool(k);
-		c->combine = parse_combine_notes_fn(v);
-		if (!c->combine) {
-			error(_("Bad notes.rewriteMode value: '%s'"), v);
-			return 1;
+			config_error_nonbool("notes.rewritemode");
+		else {
+			c->combine = parse_combine_notes_fn(v);
+			if (!c->combine)
+				error(_("Bad notes.rewriteMode value: '%s'"), v);
 		}
-		return 0;
-	} else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
+	}
+	if (!c->refs_from_env && !git_config_get_value("notes.rewriteref", &v)) {
 		/* note that a refs/ prefix is implied in the
 		 * underlying for_each_glob_ref */
 		if (starts_with(v, "refs/notes/"))
@@ -92,10 +94,7 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)
 		else
 			warning(_("Refusing to rewrite notes in %s"
 				" (outside of refs/notes/)"), v);
-		return 0;
 	}
-
-	return 0;
 }
 
 
@@ -124,7 +123,7 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 		c->refs_from_env = 1;
 		string_list_add_refs_from_colon_sep(c->refs, rewrite_refs_env);
 	}
-	git_config(notes_rewrite_config, c);
+	notes_rewrite_config(c);
 	if (!c->enabled || !c->refs->nr) {
 		string_list_clear(c->refs, 0);
 		free(c->refs);
-- 
1.9.0.GIT

Re: [PATCH v3 6/6] notes-util.c: replace `git_config()` with `git_config_get_value()`

[Jeff King]

On Mon, Jul 21, 2014 at 04:12:25AM -0700, Tanay Abhra wrote:

> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>  {
> -	struct notes_rewrite_cfg *c = cb;
> -	if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
> -		c->enabled = git_config_bool(k, v);
> -		return 0;
> -	} else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
> +	const char *v;
> +	struct strbuf key = STRBUF_INIT;
> +	strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
> +	git_config_get_bool(key.buf, &c->enabled);
> +	strbuf_release(&key);

I wonder if it is worth teaching the accessors to form such strings
themselves, like:

  void git_config_get_bool(int *out, const char *fmt, ...);

so you could do:

  git_config_get_bool(&c->enabled, "notes.rewrite.%s", c->cmd);

The "normal" cases where we do not need any run-time modification could
be used as-is (I swapped the parameter order above, but you would not
have to do so). But I guess that would require us doing extra work in
the common "normal" case to print the string into a buffer, even though
it does not have any expansions (or to do a strchr, I guess, to look for
"%").  It's probably not worth it considering how few config keys have
computed values like this.

-Peff

[PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[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>
---
Consider this as a proof of concept as the others callers have to be rewritten
as well.
I think that it is not so buggy as it passes all the tests.
After the first six patches in the series which you have already seen there are
five or four left which can rewritten without touching git_default_config().

Thus, this rewrite will serve as the base for rewriting other git_config()
callers which pass control to git_default_config() at the end of the function.
Also there are more than thirty direct callers to git_default_config()
(i.e git_config(git_default_config, NULL)), so this rewrite solves them
in one sweep.

Slight behaviour change, config_error_nonbool() has been replaced with
die("Missing value for '%s'", var);.
The original code also alerted the file name and the line number which we lose here.

Cheers,
Tanay Abhra.

 advice.c |  18 ++--
 advice.h |   2 +-
 cache.h  |   2 +-
 config.c | 287 ++++++++++++++++++++-------------------------------------------
 ident.c  |  15 ++--
 5 files changed, 104 insertions(+), 220 deletions(-)

diff --git a/advice.c b/advice.c
index 9b42033..92d89a9 100644
--- a/advice.c
+++ b/advice.c
@@ -59,22 +59,16 @@ void advise(const char *advice, ...)
 	strbuf_release(&buf);
 }

-int git_default_advice_config(const char *var, const char *value)
+void git_default_advice_config(void)
 {
-	const char *k;
+	struct strbuf var = STRBUF_INIT;
 	int i;
-
-	if (!skip_prefix(var, "advice.", &k))
-		return 0;
-
 	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		if (strcmp(k, advice_config[i].name))
-			continue;
-		*advice_config[i].preference = git_config_bool(var, value);
-		return 0;
+		strbuf_addf(&var, "advice.%s", advice_config[i].name);
+		git_config_get_bool(var.buf, advice_config[i].preference);
+		strbuf_reset(&var);
 	}
-
-	return 0;
+	strbuf_release(&var);
 }

 int error_resolve_conflict(const char *me)
diff --git a/advice.h b/advice.h
index 5ecc6c1..5bfe46c 100644
--- a/advice.h
+++ b/advice.h
@@ -19,7 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;

-int git_default_advice_config(const char *var, const char *value);
+void git_default_advice_config(void);
 __attribute__((format (printf, 1, 2)))
 void advise(const char *advice, ...);
 int error_resolve_conflict(const char *me);
diff --git a/cache.h b/cache.h
index e53651c..e667d92 100644
--- a/cache.h
+++ b/cache.h
@@ -1061,7 +1061,7 @@ extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
 extern const char *git_pager(int stdout_is_tty);
-extern int git_ident_config(const char *, const char *, void *);
+extern void git_ident_config(void);

 struct ident_split {
 	const char *name_begin;
diff --git a/config.c b/config.c
index fe9f399..72196a9 100644
--- a/config.c
+++ b/config.c
@@ -666,88 +666,47 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 	return 0;
 }

-static int git_default_core_config(const char *var, const char *value)
+static void git_default_core_config(void)
 {
+	const char *value = NULL;
 	/* This needs a better name */
-	if (!strcmp(var, "core.filemode")) {
-		trust_executable_bit = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "core.trustctime")) {
-		trust_ctime = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "core.checkstat")) {
+	git_config_get_bool("core.filemode", &trust_executable_bit);
+	git_config_get_bool("core.trustctime", &trust_ctime);
+
+	if (!git_config_get_value("core.checkstat", &value)) {
 		if (!strcasecmp(value, "default"))
 			check_stat = 1;
 		else if (!strcasecmp(value, "minimal"))
 			check_stat = 0;
 	}

-	if (!strcmp(var, "core.quotepath")) {
-		quote_path_fully = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.symlinks")) {
-		has_symlinks = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.ignorecase")) {
-		ignore_case = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.attributesfile"))
-		return git_config_pathname(&git_attributes_file, var, value);
-
-	if (!strcmp(var, "core.bare")) {
-		is_bare_repository_cfg = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.ignorestat")) {
-		assume_unchanged = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.prefersymlinkrefs")) {
-		prefer_symlink_refs = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.logallrefupdates")) {
-		log_all_ref_updates = git_config_bool(var, value);
-		return 0;
-	}
+	git_config_get_bool("core.quotepath", &quote_path_fully);
+	git_config_get_bool("core.symlinks", &has_symlinks);
+	git_config_get_bool("core.ignorecase", &ignore_case);
+	git_config_get_pathname("core.attributesfile", &git_attributes_file);
+	git_config_get_bool("core.bare", &is_bare_repository_cfg);
+	git_config_get_bool("core.ignorestat", &assume_unchanged);
+	git_config_get_bool("core.prefersymlinkrefs", &prefer_symlink_refs);
+	git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
+	git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);

-	if (!strcmp(var, "core.warnambiguousrefs")) {
-		warn_ambiguous_refs = git_config_bool(var, value);
-		return 0;
+	int abbrev;
+	if (!git_config_get_int("core.abbrev", &abbrev)) {
+		if (abbrev >= minimum_abbrev && abbrev <= 40)
+			default_abbrev = abbrev;
 	}

-	if (!strcmp(var, "core.abbrev")) {
-		int abbrev = git_config_int(var, value);
-		if (abbrev < minimum_abbrev || abbrev > 40)
-			return -1;
-		default_abbrev = abbrev;
-		return 0;
-	}
-
-	if (!strcmp(var, "core.loosecompression")) {
-		int level = git_config_int(var, value);
+	int level;
+	if (!git_config_get_int("core.loosecompression", &level)) {
 		if (level == -1)
 			level = Z_DEFAULT_COMPRESSION;
 		else if (level < 0 || level > Z_BEST_COMPRESSION)
 			die("bad zlib compression level %d", level);
 		zlib_compression_level = level;
 		zlib_compression_seen = 1;
-		return 0;
 	}

-	if (!strcmp(var, "core.compression")) {
-		int level = git_config_int(var, value);
+	if (!git_config_get_int("core.compression", &level)) {
 		if (level == -1)
 			level = Z_DEFAULT_COMPRESSION;
 		else if (level < 0 || level > Z_BEST_COMPRESSION)
@@ -756,57 +715,39 @@ static int git_default_core_config(const char *var, const char *value)
 		core_compression_seen = 1;
 		if (!zlib_compression_seen)
 			zlib_compression_level = level;
-		return 0;
 	}

-	if (!strcmp(var, "core.packedgitwindowsize")) {
+	if (!git_config_get_ulong("core.packedgitwindowsize", (long unsigned int*)&packed_git_window_size)) {
 		int pgsz_x2 = getpagesize() * 2;
-		packed_git_window_size = git_config_ulong(var, value);

 		/* This value must be multiple of (pagesize * 2) */
 		packed_git_window_size /= pgsz_x2;
 		if (packed_git_window_size < 1)
 			packed_git_window_size = 1;
 		packed_git_window_size *= pgsz_x2;
-		return 0;
-	}
-
-	if (!strcmp(var, "core.bigfilethreshold")) {
-		big_file_threshold = git_config_ulong(var, value);
-		return 0;
 	}

-	if (!strcmp(var, "core.packedgitlimit")) {
-		packed_git_limit = git_config_ulong(var, value);
-		return 0;
-	}
+	git_config_get_ulong("core.bigfilethreshold", &big_file_threshold);
+	git_config_get_ulong("core.packedgitlimit", (long unsigned int*)&packed_git_limit);
+	git_config_get_ulong("core.deltabasecachelimit", (long unsigned int*)&delta_base_cache_limit);

-	if (!strcmp(var, "core.deltabasecachelimit")) {
-		delta_base_cache_limit = git_config_ulong(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.autocrlf")) {
+	if (!git_config_get_value("core.autocrlf", &value)) {
 		if (value && !strcasecmp(value, "input")) {
 			if (core_eol == EOL_CRLF)
-				return error("core.autocrlf=input conflicts with core.eol=crlf");
+				die("core.autocrlf=input conflicts with core.eol=crlf");
 			auto_crlf = AUTO_CRLF_INPUT;
-			return 0;
-		}
-		auto_crlf = git_config_bool(var, value);
-		return 0;
+		} else
+			auto_crlf = git_config_bool("core.autocrlf", value);
 	}

-	if (!strcmp(var, "core.safecrlf")) {
-		if (value && !strcasecmp(value, "warn")) {
+	if (!git_config_get_value("core.safecrlf", &value)) {
+		if (value && !strcasecmp(value, "warn"))
 			safe_crlf = SAFE_CRLF_WARN;
-			return 0;
-		}
-		safe_crlf = git_config_bool(var, value);
-		return 0;
+		else
+			safe_crlf = git_config_bool("core.safecrlf", value);
 	}

-	if (!strcmp(var, "core.eol")) {
+	if (!git_config_get_value("core.eol", &value)) {
 		if (value && !strcasecmp(value, "lf"))
 			core_eol = EOL_LF;
 		else if (value && !strcasecmp(value, "crlf"))
@@ -816,108 +757,74 @@ static int git_default_core_config(const char *var, const char *value)
 		else
 			core_eol = EOL_UNSET;
 		if (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)
-			return error("core.autocrlf=input conflicts with core.eol=crlf");
-		return 0;
+			die("core.autocrlf=input conflicts with core.eol=crlf");
 	}

-	if (!strcmp(var, "core.notesref")) {
-		notes_ref_name = xstrdup(value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.pager"))
-		return git_config_string(&pager_program, var, value);
-
-	if (!strcmp(var, "core.editor"))
-		return git_config_string(&editor_program, var, value);
+	git_config_get_string("core.notesref", (const char**)&notes_ref_name);
+	git_config_get_string("core.pager", &pager_program);
+	git_config_get_string("core.editor", &editor_program);

-	if (!strcmp(var, "core.commentchar")) {
-		const char *comment;
-		int ret = git_config_string(&comment, var, value);
-		if (ret)
-			return ret;
-		else if (!strcasecmp(comment, "auto"))
+	const char *comment;
+	if (!git_config_get_string("core.commentchar", &comment)) {
+		if (!strcasecmp(comment, "auto"))
 			auto_comment_line_char = 1;
 		else if (comment[0] && !comment[1]) {
 			comment_line_char = comment[0];
 			auto_comment_line_char = 0;
 		} else
-			return error("core.commentChar should only be one character");
-		return 0;
+			die("core.commentChar should only be one character");
 	}

-	if (!strcmp(var, "core.askpass"))
-		return git_config_string(&askpass_program, var, value);
+	git_config_get_string("core.askpass", &askpass_program);

-	if (!strcmp(var, "core.excludesfile"))
-		return git_config_pathname(&excludes_file, var, value);
+	git_config_get_pathname("core.excludesfile", &excludes_file);

-	if (!strcmp(var, "core.whitespace")) {
+	if (!git_config_get_value("core.whitespace", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
-		whitespace_rule_cfg = parse_whitespace_rule(value);
-		return 0;
+			config_error_nonbool("core.whitespace");
+		else
+			whitespace_rule_cfg = parse_whitespace_rule(value);
 	}

-	if (!strcmp(var, "core.fsyncobjectfiles")) {
-		fsync_object_files = git_config_bool(var, value);
-		return 0;
-	}
+	git_config_get_bool("core.fsyncobjectfiles", &fsync_object_files);
+	git_config_get_bool("core.preloadindex", &core_preload_index);

-	if (!strcmp(var, "core.preloadindex")) {
-		core_preload_index = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.createobject")) {
+	if (!git_config_get_value("core.createobject", &value)) {
 		if (!strcmp(value, "rename"))
 			object_creation_mode = OBJECT_CREATION_USES_RENAMES;
 		else if (!strcmp(value, "link"))
 			object_creation_mode = OBJECT_CREATION_USES_HARDLINKS;
 		else
 			die("Invalid mode for object creation: %s", value);
-		return 0;
 	}

-	if (!strcmp(var, "core.sparsecheckout")) {
-		core_apply_sparse_checkout = git_config_bool(var, value);
-		return 0;
-	}
-
-	if (!strcmp(var, "core.precomposeunicode")) {
-		precomposed_unicode = git_config_bool(var, value);
-		return 0;
-	}
+	git_config_get_bool("core.sparsecheckout", &core_apply_sparse_checkout);
+	git_config_get_bool("core.precomposeunicode", &precomposed_unicode);

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_i18n_config(const char *var, const char *value)
+static void git_default_i18n_config(void)
 {
-	if (!strcmp(var, "i18n.commitencoding"))
-		return git_config_string(&git_commit_encoding, var, value);
-
-	if (!strcmp(var, "i18n.logoutputencoding"))
-		return git_config_string(&git_log_output_encoding, var, value);
+	git_config_get_string("i18n.commitencoding", &git_commit_encoding);
+	git_config_get_string("i18n.logoutputencoding", &git_log_output_encoding);

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_branch_config(const char *var, const char *value)
+static void git_default_branch_config(void)
 {
-	if (!strcmp(var, "branch.autosetupmerge")) {
-		if (value && !strcasecmp(value, "always")) {
+	const char *value = NULL;
+	if (!git_config_get_value("branch.autosetupmerge", &value)) {
+		if (value && !strcasecmp(value, "always"))
 			git_branch_track = BRANCH_TRACK_ALWAYS;
-			return 0;
-		}
-		git_branch_track = git_config_bool(var, value);
-		return 0;
+		else
+			git_branch_track = git_config_bool("branch.autosetupmerge", value);
 	}
-	if (!strcmp(var, "branch.autosetuprebase")) {
+
+	if (!git_config_get_value("branch.autosetuprebase", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			die("Missing value for 'branch.autosetuprebase'");
 		else if (!strcmp(value, "never"))
 			autorebase = AUTOREBASE_NEVER;
 		else if (!strcmp(value, "local"))
@@ -927,19 +834,18 @@ static int git_default_branch_config(const char *var, const char *value)
 		else if (!strcmp(value, "always"))
 			autorebase = AUTOREBASE_ALWAYS;
 		else
-			return error("Malformed value for %s", var);
-		return 0;
+			die("Malformed value for branch.autosetuprebase");
 	}

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_push_config(const char *var, const char *value)
+static void git_default_push_config(void)
 {
-	if (!strcmp(var, "push.default")) {
+	const char *value  = NULL;
+	if (!git_config_get_value("push.default", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			die("Missing value for 'push.default'");
 		else if (!strcmp(value, "nothing"))
 			push_default = PUSH_DEFAULT_NOTHING;
 		else if (!strcmp(value, "matching"))
@@ -953,60 +859,47 @@ static int git_default_push_config(const char *var, const char *value)
 		else if (!strcmp(value, "current"))
 			push_default = PUSH_DEFAULT_CURRENT;
 		else {
-			error("Malformed value for %s: %s", var, value);
-			return error("Must be one of nothing, matching, simple, "
+			error("Malformed value for %s: %s", "push.default", value);
+			die("Must be one of nothing, matching, simple, "
 				     "upstream or current.");
 		}
-		return 0;
 	}

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

-static int git_default_mailmap_config(const char *var, const char *value)
+static void git_default_mailmap_config(void)
 {
-	if (!strcmp(var, "mailmap.file"))
-		return git_config_pathname(&git_mailmap_file, var, value);
-	if (!strcmp(var, "mailmap.blob"))
-		return git_config_string(&git_mailmap_blob, var, value);
+	git_config_get_pathname("mailmap.file", &git_mailmap_file);
+	git_config_get_string("mailmap.blob", &git_mailmap_blob);

 	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
 }

 int git_default_config(const char *var, const char *value, void *dummy)
 {
-	if (starts_with(var, "core."))
-		return git_default_core_config(var, value);
+	const char *v = NULL;

-	if (starts_with(var, "user."))
-		return git_ident_config(var, value, dummy);
+	git_default_core_config();

-	if (starts_with(var, "i18n."))
-		return git_default_i18n_config(var, value);
+	git_ident_config();

-	if (starts_with(var, "branch."))
-		return git_default_branch_config(var, value);
+	git_default_i18n_config();

-	if (starts_with(var, "push."))
-		return git_default_push_config(var, value);
+	git_default_branch_config();

-	if (starts_with(var, "mailmap."))
-		return git_default_mailmap_config(var, value);
+	git_default_push_config();

-	if (starts_with(var, "advice."))
-		return git_default_advice_config(var, value);
+	git_default_mailmap_config();

-	if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) {
-		pager_use_color = git_config_bool(var,value);
-		return 0;
-	}
+	git_default_advice_config();

-	if (!strcmp(var, "pack.packsizelimit")) {
-		pack_size_limit_cfg = git_config_ulong(var, value);
-		return 0;
-	}
+	if (!git_config_get_value("pager.color", &v))
+		pager_use_color = git_config_bool("pager.color",v);
+	else if (!git_config_get_value("color.pager", &v))
+		pager_use_color = git_config_bool("color.pager",v);
+
+	git_config_get_ulong("pack.packsizelimit", &pack_size_limit_cfg);
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/ident.c b/ident.c
index 1d9b6e7..da889cf 100644
--- a/ident.c
+++ b/ident.c
@@ -392,29 +392,26 @@ int author_ident_sufficiently_given(void)
 	return ident_is_sufficient(author_ident_explicitly_given);
 }

-int git_ident_config(const char *var, const char *value, void *data)
+void git_ident_config(void)
 {
-	if (!strcmp(var, "user.name")) {
+	const char *value = NULL;
+	if (!git_config_get_value("user.name", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			die("Missing value for 'user.name'");
 		strbuf_reset(&git_default_name);
 		strbuf_addstr(&git_default_name, value);
 		committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		author_ident_explicitly_given |= IDENT_NAME_GIVEN;
-		return 0;
 	}

-	if (!strcmp(var, "user.email")) {
+	if (!git_config_get_value("user.email", &value)) {
 		if (!value)
-			return config_error_nonbool(var);
+			die("Missing value for 'user.email'");
 		strbuf_reset(&git_default_email);
 		strbuf_addstr(&git_default_email, value);
 		committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
-		return 0;
 	}
-
-	return 0;
 }

 static int buf_cmp(const char *a_begin, const char *a_end,
-- 
1.9.0.GIT

Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Consider this as a proof of concept as the others callers have to be rewritten
> as well.
> I think that it is not so buggy as it passes all the tests.

Before and after your patch, git_default_config() is called once per
config key. Before the patch, it made sense, but after the patch, it
doesn't anymore, as it loads the whole config file in one call.

I think it's OK to have this as an intermediate patch, but it should be
clear to reviewers.

At the end of your series, git_default_config should become an
argumentless function.

Actually, I'm wondering whether it makes sense to keep
git_default_config as-is, and introduce a new function like
git_load_default_config(void), initially empty. Then, move and change
code from git_default_config(...) to git_load_default_config(), and
finally remove git_default_config(...) once it's empty.

You have several decl-after-statements in your patch. You should have
something like this in config.mak to catch them:

CFLAGS += -Wdeclaration-after-statement -Wall -Werror

There are several (long unsigned int*) casts that seem useless to me.
Useless casts distract the reader, and prevent usefull warnings from the
compiler.

Actually, I'm wondering whether these casts are safe (they cast from
size_t * to ulong *). On Windows 64 bits for example, sizeof(size_t) ==
8 and sizeof(unsigned long) == 4 if google informed me correctly. On a
big-endian machine, this would be totally broken.

It is probably safer to add a new git_config_get_size_t analog to
git_config_get_ulong.

> +	if > +	git_config_get_string("core.notesref", (const char**)&notes_ref_name);

This cast is needed only because notes_ref_name is declared as
non-const, but a better fix would be to make the variable const, and
remove the cast.

The following patch solves these issues (modulo the question above on
cast safety).

diff --git a/cache.h b/cache.h
index e667d92..1271904 100644
--- a/cache.h
+++ b/cache.h
@@ -674,7 +674,7 @@ enum object_creation_mode {
 
 extern enum object_creation_mode object_creation_mode;
 
-extern char *notes_ref_name;
+extern const char *notes_ref_name;
 
 extern int grafts_replace_parents;
 
diff --git a/config.c b/config.c
index 72196a9..c2664c3 100644
--- a/config.c
+++ b/config.c
@@ -669,6 +669,9 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
 static void git_default_core_config(void)
 {
        const char *value = NULL;
+       int abbrev;
+       int level;
+       const char *comment;
        /* This needs a better name */
        git_config_get_bool("core.filemode", &trust_executable_bit);
        git_config_get_bool("core.trustctime", &trust_ctime);
@@ -690,13 +693,11 @@ static void git_default_core_config(void)
        git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
        git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);
 
-       int abbrev;
        if (!git_config_get_int("core.abbrev", &abbrev)) {
                if (abbrev >= minimum_abbrev && abbrev <= 40)
                        default_abbrev = abbrev;
        }
 
-       int level;
        if (!git_config_get_int("core.loosecompression", &level)) {
                if (level == -1)
                        level = Z_DEFAULT_COMPRESSION;
@@ -717,7 +718,7 @@ static void git_default_core_config(void)
                        zlib_compression_level = level;
        }
 
-       if (!git_config_get_ulong("core.packedgitwindowsize", (long unsigned int*)&packed_git_window_size)) {
+       if (!git_config_get_ulong("core.packedgitwindowsize", &packed_git_window_size)) {
                int pgsz_x2 = getpagesize() * 2;
 
                /* This value must be multiple of (pagesize * 2) */
@@ -728,8 +729,8 @@ static void git_default_core_config(void)
        }
 
        git_config_get_ulong("core.bigfilethreshold", &big_file_threshold);
-       git_config_get_ulong("core.packedgitlimit", (long unsigned int*)&packed_git_limit);
-       git_config_get_ulong("core.deltabasecachelimit", (long unsigned int*)&delta_base_cache_limit);
+       git_config_get_ulong("core.packedgitlimit", &packed_git_limit);
+       git_config_get_ulong("core.deltabasecachelimit", &delta_base_cache_limit);
 
        if (!git_config_get_value("core.autocrlf", &value)) {
                if (value && !strcasecmp(value, "input")) {
@@ -760,11 +761,10 @@ static void git_default_core_config(void)
                        die("core.autocrlf=input conflicts with core.eol=crlf");
        }
 
-       git_config_get_string("core.notesref", (const char**)&notes_ref_name);
+       git_config_get_string("core.notesref", &notes_ref_name);
        git_config_get_string("core.pager", &pager_program);
        git_config_get_string("core.editor", &editor_program);
 
-       const char *comment;
        if (!git_config_get_string("core.commentchar", &comment)) {
                if (!strcasecmp(comment, "auto"))
                        auto_comment_line_char = 1;
diff --git a/environment.c b/environment.c
index 565f652..21d4dbb 100644
--- a/environment.c
+++ b/environment.c
@@ -56,7 +56,7 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
-char *notes_ref_name;
+const char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
 int merge_log_config = -1;

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

Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Tanay Abhra]

> 
>> +	if > +	git_config_get_string("core.notesref", (const char**)&notes_ref_name);
> 
> This cast is needed only because notes_ref_name is declared as
> non-const, but a better fix would be to make the variable const, and
> remove the cast.

Same casts had to be used in imap-send.c patch, I will have to use an
intermediate variable there to remove the cast thus destroying the one
liners or will have to update the variable declarations.

Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>> 
>>> +	if > +	git_config_get_string("core.notesref", (const char**)&notes_ref_name);
>> 
>> This cast is needed only because notes_ref_name is declared as
>> non-const, but a better fix would be to make the variable const, and
>> remove the cast.
>
> Same casts had to be used in imap-send.c patch, I will have to use an
> intermediate variable there to remove the cast thus destroying the one
> liners or will have to update the variable declarations.

Updating the declaration like this should just work:

--- a/imap-send.c
+++ b/imap-send.c
@@ -1324,7 +1324,7 @@ static int split_msg(struct strbuf *all_msgs, struct strbuf *msg, int *ofs)
        return 1;
 }
 
-static char *imap_folder;
+static const char *imap_folder;
 
 static void git_imap_config(void)
 {
@@ -1332,7 +1332,7 @@ static void git_imap_config(void)
 
        git_config_get_bool("imap.sslverify", &server.ssl_verify);
        git_config_get_bool("imap.preformattedhtml", &server.use_html);
-       git_config_get_string("imap.folder", (const char**)&imap_folder);
+       git_config_get_string("imap.folder", &imap_folder);
 
        if (!git_config_get_value("imap.host", &val)) {
                if(!val)

In general, most strings one manipulates are "const char *", it's
frequent to modify a pointer to a string, but rather rare to modify the
string itself.

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

Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Junio C Hamano]

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

> In general, most strings one manipulates are "const char *", it's
> frequent to modify a pointer to a string, but rather rare to modify the
> string itself.

We seem to have a disagreement.  Unlike git_config_get_value() that
lets callers peek the only cached copy, git_config_get_string()
gives its caller a new copy that the caller needs to free.  Such a
new string can and should be given as mutable, I would say.

Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> In general, most strings one manipulates are "const char *", it's
>> frequent to modify a pointer to a string, but rather rare to modify the
>> string itself.
>
> We seem to have a disagreement.  Unlike git_config_get_value() that
> lets callers peek the only cached copy, git_config_get_string()
> gives its caller a new copy that the caller needs to free.  Such a
> new string can and should be given as mutable, I would say.

You're right (I guess you replied to this one before seeing my other
message). imap_folder could be declared const char *, but
git_config_get_string() shouldn't be the one to force this.

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

Re: [PATCH/RFC] rewrite `git_default_config()` using config-set API functions

[Ramsay Jones]

On 21/07/14 12:44, Tanay Abhra wrote:
> 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>
> ---
> Consider this as a proof of concept as the others callers have to be rewritten
> as well.
> I think that it is not so buggy as it passes all the tests.
> After the first six patches in the series which you have already seen there are
> five or four left which can rewritten without touching git_default_config().
> 
> Thus, this rewrite will serve as the base for rewriting other git_config()
> callers which pass control to git_default_config() at the end of the function.
> Also there are more than thirty direct callers to git_default_config()
> (i.e git_config(git_default_config, NULL)), so this rewrite solves them
> in one sweep.
> 
> Slight behaviour change, config_error_nonbool() has been replaced with
> die("Missing value for '%s'", var);.
> The original code also alerted the file name and the line number which we lose here.
> 
> Cheers,
> Tanay Abhra.
> 
>  advice.c |  18 ++--
>  advice.h |   2 +-
>  cache.h  |   2 +-
>  config.c | 287 ++++++++++++++++++++-------------------------------------------
>  ident.c  |  15 ++--
>  5 files changed, 104 insertions(+), 220 deletions(-)
> 

[snip]


> diff --git a/config.c b/config.c
> index fe9f399..72196a9 100644
> --- a/config.c
> +++ b/config.c
> @@ -666,88 +666,47 @@ int git_config_pathname(const char **dest, const char *var, const char *value)
>  	return 0;
>  }
> 
> -static int git_default_core_config(const char *var, const char *value)
> +static void git_default_core_config(void)
>  {
> +	const char *value = NULL;
>  	/* This needs a better name */
> -	if (!strcmp(var, "core.filemode")) {
> -		trust_executable_bit = git_config_bool(var, value);
> -		return 0;
> -	}
> -	if (!strcmp(var, "core.trustctime")) {
> -		trust_ctime = git_config_bool(var, value);
> -		return 0;
> -	}
> -	if (!strcmp(var, "core.checkstat")) {
> +	git_config_get_bool("core.filemode", &trust_executable_bit);
> +	git_config_get_bool("core.trustctime", &trust_ctime);
> +
> +	if (!git_config_get_value("core.checkstat", &value)) {
>  		if (!strcasecmp(value, "default"))
>  			check_stat = 1;
>  		else if (!strcasecmp(value, "minimal"))
>  			check_stat = 0;
>  	}
> 
> -	if (!strcmp(var, "core.quotepath")) {
> -		quote_path_fully = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.symlinks")) {
> -		has_symlinks = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.ignorecase")) {
> -		ignore_case = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.attributesfile"))
> -		return git_config_pathname(&git_attributes_file, var, value);
> -
> -	if (!strcmp(var, "core.bare")) {
> -		is_bare_repository_cfg = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.ignorestat")) {
> -		assume_unchanged = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.prefersymlinkrefs")) {
> -		prefer_symlink_refs = git_config_bool(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.logallrefupdates")) {
> -		log_all_ref_updates = git_config_bool(var, value);
> -		return 0;
> -	}
> +	git_config_get_bool("core.quotepath", &quote_path_fully);
> +	git_config_get_bool("core.symlinks", &has_symlinks);
> +	git_config_get_bool("core.ignorecase", &ignore_case);
> +	git_config_get_pathname("core.attributesfile", &git_attributes_file);
> +	git_config_get_bool("core.bare", &is_bare_repository_cfg);
> +	git_config_get_bool("core.ignorestat", &assume_unchanged);
> +	git_config_get_bool("core.prefersymlinkrefs", &prefer_symlink_refs);
> +	git_config_get_bool("core.logallrefupdates", &log_all_ref_updates);
> +	git_config_get_bool("core.warnambiguousrefs", &warn_ambiguous_refs);
> 
> -	if (!strcmp(var, "core.warnambiguousrefs")) {
> -		warn_ambiguous_refs = git_config_bool(var, value);
> -		return 0;
> +	int abbrev;

declaration after statement.

> +	if (!git_config_get_int("core.abbrev", &abbrev)) {
> +		if (abbrev >= minimum_abbrev && abbrev <= 40)
> +			default_abbrev = abbrev;
>  	}
> 
> -	if (!strcmp(var, "core.abbrev")) {
> -		int abbrev = git_config_int(var, value);
> -		if (abbrev < minimum_abbrev || abbrev > 40)
> -			return -1;
> -		default_abbrev = abbrev;
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.loosecompression")) {
> -		int level = git_config_int(var, value);
> +	int level;

ditto.

> +	if (!git_config_get_int("core.loosecompression", &level)) {
>  		if (level == -1)
>  			level = Z_DEFAULT_COMPRESSION;
>  		else if (level < 0 || level > Z_BEST_COMPRESSION)
>  			die("bad zlib compression level %d", level);
>  		zlib_compression_level = level;
>  		zlib_compression_seen = 1;
> -		return 0;
>  	}
> 
> -	if (!strcmp(var, "core.compression")) {
> -		int level = git_config_int(var, value);
> +	if (!git_config_get_int("core.compression", &level)) {
>  		if (level == -1)
>  			level = Z_DEFAULT_COMPRESSION;
>  		else if (level < 0 || level > Z_BEST_COMPRESSION)
> @@ -756,57 +715,39 @@ static int git_default_core_config(const char *var, const char *value)
>  		core_compression_seen = 1;
>  		if (!zlib_compression_seen)
>  			zlib_compression_level = level;
> -		return 0;
>  	}
> 
> -	if (!strcmp(var, "core.packedgitwindowsize")) {
> +	if (!git_config_get_ulong("core.packedgitwindowsize", (long unsigned int*)&packed_git_window_size)) {
>  		int pgsz_x2 = getpagesize() * 2;
> -		packed_git_window_size = git_config_ulong(var, value);
> 
>  		/* This value must be multiple of (pagesize * 2) */
>  		packed_git_window_size /= pgsz_x2;
>  		if (packed_git_window_size < 1)
>  			packed_git_window_size = 1;
>  		packed_git_window_size *= pgsz_x2;
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.bigfilethreshold")) {
> -		big_file_threshold = git_config_ulong(var, value);
> -		return 0;
>  	}
> 
> -	if (!strcmp(var, "core.packedgitlimit")) {
> -		packed_git_limit = git_config_ulong(var, value);
> -		return 0;
> -	}
> +	git_config_get_ulong("core.bigfilethreshold", &big_file_threshold);
> +	git_config_get_ulong("core.packedgitlimit", (long unsigned int*)&packed_git_limit);
> +	git_config_get_ulong("core.deltabasecachelimit", (long unsigned int*)&delta_base_cache_limit);
> 
> -	if (!strcmp(var, "core.deltabasecachelimit")) {
> -		delta_base_cache_limit = git_config_ulong(var, value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.autocrlf")) {
> +	if (!git_config_get_value("core.autocrlf", &value)) {
>  		if (value && !strcasecmp(value, "input")) {
>  			if (core_eol == EOL_CRLF)
> -				return error("core.autocrlf=input conflicts with core.eol=crlf");
> +				die("core.autocrlf=input conflicts with core.eol=crlf");
>  			auto_crlf = AUTO_CRLF_INPUT;
> -			return 0;
> -		}
> -		auto_crlf = git_config_bool(var, value);
> -		return 0;
> +		} else
> +			auto_crlf = git_config_bool("core.autocrlf", value);
>  	}
> 
> -	if (!strcmp(var, "core.safecrlf")) {
> -		if (value && !strcasecmp(value, "warn")) {
> +	if (!git_config_get_value("core.safecrlf", &value)) {
> +		if (value && !strcasecmp(value, "warn"))
>  			safe_crlf = SAFE_CRLF_WARN;
> -			return 0;
> -		}
> -		safe_crlf = git_config_bool(var, value);
> -		return 0;
> +		else
> +			safe_crlf = git_config_bool("core.safecrlf", value);
>  	}
> 
> -	if (!strcmp(var, "core.eol")) {
> +	if (!git_config_get_value("core.eol", &value)) {
>  		if (value && !strcasecmp(value, "lf"))
>  			core_eol = EOL_LF;
>  		else if (value && !strcasecmp(value, "crlf"))
> @@ -816,108 +757,74 @@ static int git_default_core_config(const char *var, const char *value)
>  		else
>  			core_eol = EOL_UNSET;
>  		if (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)
> -			return error("core.autocrlf=input conflicts with core.eol=crlf");
> -		return 0;
> +			die("core.autocrlf=input conflicts with core.eol=crlf");
>  	}
> 
> -	if (!strcmp(var, "core.notesref")) {
> -		notes_ref_name = xstrdup(value);
> -		return 0;
> -	}
> -
> -	if (!strcmp(var, "core.pager"))
> -		return git_config_string(&pager_program, var, value);
> -
> -	if (!strcmp(var, "core.editor"))
> -		return git_config_string(&editor_program, var, value);
> +	git_config_get_string("core.notesref", (const char**)&notes_ref_name);
> +	git_config_get_string("core.pager", &pager_program);
> +	git_config_get_string("core.editor", &editor_program);
> 
> -	if (!strcmp(var, "core.commentchar")) {
> -		const char *comment;
> -		int ret = git_config_string(&comment, var, value);
> -		if (ret)
> -			return ret;
> -		else if (!strcasecmp(comment, "auto"))
> +	const char *comment;

ditto. I gave up here.

ATB,
Ramsay Jones

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> [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.
> 	See [1] for the documentation of the new functions.
>
> This series builds on the top of 5def4132 in pu or topic[1] in the mailing list
> with name "git config cache & special querying API utilizing the cache".

It's now called ta/config-set (see last "What's cooking in git.git").

> All patches pass every test, but there is a catch, there is slight behaviour
> change in most of them where originally the callback returns
> config_error_nonbool() when it sees a NULL value for a key causing a die
> specified in git_parse_source in config.c.
>
> The die also prints the file name and the line number as,
>
> 	"die("bad config file line %d in %s", cf->linenr, cf->name);"
>
> We lose the fine grained error checking when switching to this method.

I think a first step would be something like this:

--- a/config.c
+++ b/config.c
@@ -656,6 +656,15 @@ int git_config_string(const char **dest, const char *var, const char *value)
        return 0;
 }
 
+// TODO: either make it static or export it properly
+int git_config_string_or_die(const char **dest, const char *var, const char *value)
+{
+       if (git_config_string(dest, var, value) < 0)
+               die("bad config file (TODO: file/line info)");
+       else
+               return 0;
+}
+
 int git_config_pathname(const char **dest, const char *var, const char *value)
 {
        if (!value)
@@ -1336,7 +1345,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, const char
 {
        const char *value;
        if (!git_configset_get_value(cs, key, &value))
-               return git_config_string(dest, key, value);
+               return git_config_string_or_die(dest, key, value);
        else
                return 1;
 }

In the original API, git_config_string was called at parsing time, hence
the file/line information was available through "cf". Here, we're
querying the cache which doesn't have this information yet.

I initially thought that managing properly file/line information would
be just an addition, but this example shows that it is actually needed
to be feature-complete wrt the old API. And I think we should be
feature-complete (i.e. make the code cleaner without harming the user).

So, I think it now makes sense to resurect your "file line info" patch:

  http://article.gmane.org/gmane.comp.version-control.git/253123

Now that the series is properly reviewed, avoid modifying existing
patches as much as possible, and add these file/line info on top of the
existing.

I think you need to:

1) Modify the hashmap data structure and the code that fills it in to
   store the file/line info (already done in your previous WIP patch).

2) Add a by-address parameter to git_configset_get_value that allows the
   user to get the file and line information. In your previous patch,
   that would mean returning a pointer to the corresponding struct
   key_source.

3) Pass this information to git_config_string_or_die, and die with the
   right message (with a helper like die_config(struct key_source *ks)
   that takes care of the formatting)

4) apply the same to git_config_get_<other than string>.

I'd actually add a step 0) before that: add a test that checks your
behavior change. The test should pass without your patches, and fail
with your current patch. Then, it should pass again once you completed
the work.

On a side note, re-reading your previous patch, I found this which
sounds suspicious:

+	struct config_hash_entry *e;
+	struct string_list_item *si;
+	struct key_source *ks = xmalloc(sizeof(*e));

Didn't you mean xmalloc(sizeof(*ks))?
                                ^^

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

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

[Tanay Abhra]

On 7/21/2014 6:21 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> [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.
>> 	See [1] for the documentation of the new functions.
>>
>> This series builds on the top of 5def4132 in pu or topic[1] in the mailing list
>> with name "git config cache & special querying API utilizing the cache".
> 
> It's now called ta/config-set (see last "What's cooking in git.git").
>

Noted. More below.

>> All patches pass every test, but there is a catch, there is slight behaviour
>> change in most of them where originally the callback returns
>> config_error_nonbool() when it sees a NULL value for a key causing a die
>> specified in git_parse_source in config.c.
>>
>> The die also prints the file name and the line number as,
>>
>> 	"die("bad config file line %d in %s", cf->linenr, cf->name);"
>>
>> We lose the fine grained error checking when switching to this method.
> 
> I think a first step would be something like this:
> 
> --- a/config.c
> +++ b/config.c
> @@ -656,6 +656,15 @@ int git_config_string(const char **dest, const char *var, const char *value)
>         return 0;
>  }
>  
> +// TODO: either make it static or export it properly
> +int git_config_string_or_die(const char **dest, const char *var, const char *value)
> +{
> +       if (git_config_string(dest, var, value) < 0)
> +               die("bad config file (TODO: file/line info)");
> +       else
> +               return 0;
> +}
> +
>  int git_config_pathname(const char **dest, const char *var, const char *value)
>  {
>         if (!value)
> @@ -1336,7 +1345,7 @@ int git_configset_get_string(struct config_set *cs, const char *key, const char
>  {
>         const char *value;
>         if (!git_configset_get_value(cs, key, &value))
> -               return git_config_string(dest, key, value);
> +               return git_config_string_or_die(dest, key, value);
>         else
>                 return 1;
>  }
> 
> In the original API, git_config_string was called at parsing time, hence
> the file/line information was available through "cf". Here, we're
> querying the cache which doesn't have this information yet.
> 
> I initially thought that managing properly file/line information would
> be just an addition, but this example shows that it is actually needed
> to be feature-complete wrt the old API. And I think we should be
> feature-complete (i.e. make the code cleaner without harming the user).
> 
> So, I think it now makes sense to resurect your "file line info" patch:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/253123
> 
> Now that the series is properly reviewed, avoid modifying existing
> patches as much as possible, and add these file/line info on top of the
> existing.
> 
> I think you need to:
> 
> 1) Modify the hashmap data structure and the code that fills it in to
>    store the file/line info (already done in your previous WIP patch).
> 
> 2) Add a by-address parameter to git_configset_get_value that allows the
>    user to get the file and line information. In your previous patch,
>    that would mean returning a pointer to the corresponding struct
>    key_source.

Will this extra complexity be good for "git_configset_get_value"?
Instead can we provide a function like die_config(char *key)
which prints
	die("bad config file line %d in %s", linenr, filename);?
A variation would be die_config_multi(char *key, char *value)
for multi valued keys.

> 3) Pass this information to git_config_string_or_die, and die with the
>    right message (with a helper like die_config(struct key_source *ks)
>    that takes care of the formatting)

No need for passing if we use the above method. We will just call die_config()
inside it for NULL values

> 4) apply the same to git_config_get_<other than string>.
>
> I'd actually add a step 0) before that: add a test that checks your
> behavior change. The test should pass without your patches, and fail
> with your current patch. Then, it should pass again once you completed
> the work.
>

Noted, I will add it.

> On a side note, re-reading your previous patch, I found this which
> sounds suspicious:
> 
> +	struct config_hash_entry *e;
> +	struct string_list_item *si;
> +	struct key_source *ks = xmalloc(sizeof(*e));
> 
> Didn't you mean xmalloc(sizeof(*ks))?
>

Yikes, Thanks.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/21/2014 6:21 PM, Matthieu Moy wrote:
>> 2) Add a by-address parameter to git_configset_get_value that allows the
>>    user to get the file and line information. In your previous patch,
>>    that would mean returning a pointer to the corresponding struct
>>    key_source.
>
> Will this extra complexity be good for "git_configset_get_value"?
> Instead can we provide a function like die_config(char *key)
> which prints
> 	die("bad config file line %d in %s", linenr, filename);?

Where would you call this function, and where would you take linenr and
filename?

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

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

[Tanay Abhra]

On 7/21/2014 7:15 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 7/21/2014 6:21 PM, Matthieu Moy wrote:
>>> 2) Add a by-address parameter to git_configset_get_value that allows the
>>>    user to get the file and line information. In your previous patch,
>>>    that would mean returning a pointer to the corresponding struct
>>>    key_source.
>>
>> Will this extra complexity be good for "git_configset_get_value"?
>> Instead can we provide a function like die_config(char *key)
>> which prints
>> 	die("bad config file line %d in %s", linenr, filename);?
> 
> Where would you call this function, and where would you take linenr and
> filename?
>

Usage can be like this,

if(!git_config_get_value(k, &v)) {
	if (!v) {
		config_error_nonbool(k);
		die_config(k);
		/* die_config calls git_config_get_value_multi for 'k',
		 * gets the string list with the util pointer containing
		 * the linenr and the file name, dies printing the message.
		 */
	} else
		/* do work */
}

Above example works just like the current code. Currently the callbacks
does not have the access to the linenr and file name anyway.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/21/2014 7:15 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> On 7/21/2014 6:21 PM, Matthieu Moy wrote:
>>>> 2) Add a by-address parameter to git_configset_get_value that allows the
>>>>    user to get the file and line information. In your previous patch,
>>>>    that would mean returning a pointer to the corresponding struct
>>>>    key_source.
>>>
>>> Will this extra complexity be good for "git_configset_get_value"?
>>> Instead can we provide a function like die_config(char *key)
>>> which prints
>>> 	die("bad config file line %d in %s", linenr, filename);?
>> 
>> Where would you call this function, and where would you take linenr and
>> filename?
>>
>
> Usage can be like this,
>
> if(!git_config_get_value(k, &v)) {
> 	if (!v) {
> 		config_error_nonbool(k);
> 		die_config(k);
> 		/* die_config calls git_config_get_value_multi for 'k',
> 		 * gets the string list with the util pointer containing
> 		 * the linenr and the file name, dies printing the message.
> 		 */
> 	} else
> 		/* do work */
> }

OK, so you query the cache twice (which is OK, it's cheap and happens
just once before dying). That would work too.

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