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

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

[Tanay Abhra]

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

This series aims to do these three things,

* Use the config-set API to rewrite git_config().

* Solve any legacy bugs in the previous system while at it.

* To be feature complete compared to the previous git_config() implementation,
  which I think it is now. (added the line number and file name info just for
  completeness)

Also, I haven't yet checked the exact improvements but still as a teaser,
git status now only rereads the configuration files twice instead of four
times.

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

Tanay Abhra (7):

 Documentation/technical/api-config.txt |  5 ++
 cache.h                                |  1 +
 config.c                               | 93 +++++++++++++++++++++++++++++++---
 t/t1308-config-set.sh                  | 17 +++++++
 test-config.c                          | 10 ++++
 userdiff.c                             | 14 ++++-
 6 files changed, 131 insertions(+), 9 deletions(-)

-- 
1.9.0.GIT

[PATCH 1/7] config.c: fix accuracy of line number in errors

[Tanay Abhra]

If a callback returns a negative value to `git_config*()` family,
they call `die()` while printing the line number and the file name.
Currently the printed line number is off by one, thus printing the
wrong line number.

Make `linenr` point to the line we just parsed during the call
to callback to get accurate line number in error messages.

Discovered-by: Tanay Abhra <tanayabh@gmail.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
---
 config.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index 22971e9..6db8f97 100644
--- a/config.c
+++ b/config.c
@@ -244,6 +244,7 @@ static int get_next_char(void)
 		cf->linenr++;
 	if (c == EOF) {
 		cf->eof = 1;
+		cf->linenr++;
 		c = '\n';
 	}
 	return c;
@@ -319,6 +320,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 {
 	int c;
 	char *value;
+	int ret;
 
 	/* Get the full name */
 	for (;;) {
@@ -341,7 +343,15 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
 		if (!value)
 			return -1;
 	}
-	return fn(name->buf, value, data);
+	/*
+	 * We already consumed the \n, but we need linenr to point to
+	 * the line we just parsed during the call to fn to get
+	 * accurate line number in error messages.
+	 */
+	cf->linenr--;
+	ret = fn(name->buf, value, data);
+	cf->linenr++;
+	return ret;
 }
 
 static int get_extended_base_var(struct strbuf *name, int c)
-- 
1.9.0.GIT

Re: [PATCH 1/7] config.c: fix accuracy of line number in errors

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> If a callback returns a negative value to `git_config*()` family,
> they call `die()` while printing the line number and the file name.
> Currently the printed line number is off by one, thus printing the
> wrong line number.
>
> Make `linenr` point to the line we just parsed during the call
> to callback to get accurate line number in error messages.
>
> Discovered-by: Tanay Abhra <tanayabh@gmail.com>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>

Thanks.

I am not sure what to read in these two lines.  Was the fix done by
you or Matthieu?

> ---
>  config.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index 22971e9..6db8f97 100644
> --- a/config.c
> +++ b/config.c
> @@ -244,6 +244,7 @@ static int get_next_char(void)
>  		cf->linenr++;
>  	if (c == EOF) {
>  		cf->eof = 1;
> +		cf->linenr++;
>  		c = '\n';
>  	}
>  	return c;
> @@ -319,6 +320,7 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
>  {
>  	int c;
>  	char *value;
> +	int ret;
>  
>  	/* Get the full name */
>  	for (;;) {
> @@ -341,7 +343,15 @@ static int get_value(config_fn_t fn, void *data, struct strbuf *name)
>  		if (!value)
>  			return -1;
>  	}
> -	return fn(name->buf, value, data);
> +	/*
> +	 * We already consumed the \n, but we need linenr to point to
> +	 * the line we just parsed during the call to fn to get
> +	 * accurate line number in error messages.
> +	 */
> +	cf->linenr--;
> +	ret = fn(name->buf, value, data);
> +	cf->linenr++;
> +	return ret;
>  }
>  
>  static int get_extended_base_var(struct strbuf *name, int c)

Re: [PATCH 1/7] config.c: fix accuracy of line number in errors

[Tanay Abhra]

On 7/24/2014 3:19 AM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> If a callback returns a negative value to `git_config*()` family,
>> they call `die()` while printing the line number and the file name.
>> Currently the printed line number is off by one, thus printing the
>> wrong line number.
>>
>> Make `linenr` point to the line we just parsed during the call
>> to callback to get accurate line number in error messages.
>>
>> Discovered-by: Tanay Abhra <tanayabh@gmail.com>
>> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> 
> Thanks.
> 
> I am not sure what to read in these two lines.  Was the fix done by
> you or Matthieu?
>

I misunderstood the meaning of the message trailers. I will correct
it in the next re roll.

Re: [PATCH 1/7] config.c: fix accuracy of line number in errors

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/24/2014 3:19 AM, Junio C Hamano wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> If a callback returns a negative value to `git_config*()` family,
>>> they call `die()` while printing the line number and the file name.
>>> Currently the printed line number is off by one, thus printing the
>>> wrong line number.
>>>
>>> Make `linenr` point to the line we just parsed during the call
>>> to callback to get accurate line number in error messages.
>>>
>>> Discovered-by: Tanay Abhra <tanayabh@gmail.com>
>>> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>> 
>> Thanks.
>> 
>> I am not sure what to read in these two lines.  Was the fix done by
>> you or Matthieu?
>>
>
> I misunderstood the meaning of the message trailers. I will correct
> it in the next re roll.

Well, that does not really answer my question, and it does not tell
us if you now understand the meaning of trailers correctly, or
replaced an earlier misunderstanding with a different
misunderstanding ;-)

More importantly, it would not help me interpret what you mean when
you write a sequence like this:

	From: tanay
        S-o-b: matthieu
        S-o-b: tanay

which does not quite make sense and leave the reader puzzled.

If the patch was authored by you and helped by matthieu, the first
S-o-b would be lying.  If the patch was written by matthieu and you
are merely relaying, then the authorship From is lying.  If the
patch was written by you and matthieu is relaying, then the two
S-o-bs are in incorrect order.

Re: [PATCH 1/7] config.c: fix accuracy of line number in errors

[Tanay Abhra]

On 7/25/2014 12:01 AM, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> On 7/24/2014 3:19 AM, Junio C Hamano wrote:
>>> Tanay Abhra <tanayabh@gmail.com> writes:
>>>
>>>> If a callback returns a negative value to `git_config*()` family,
>>>> they call `die()` while printing the line number and the file name.
>>>> Currently the printed line number is off by one, thus printing the
>>>> wrong line number.
>>>>
>>>> Make `linenr` point to the line we just parsed during the call
>>>> to callback to get accurate line number in error messages.
>>>>
>>>> Discovered-by: Tanay Abhra <tanayabh@gmail.com>
>>>> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>

This patch should have looked like,

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

<commit message>

Commit-message-by: Tanay Abhra <tanayabh@gmail.com>

>>>
>>> Thanks.
>>>
>>> I am not sure what to read in these two lines.  Was the fix done by
>>> you or Matthieu?
>>>
>>
>> I misunderstood the meaning of the message trailers. I will correct
>> it in the next re roll.
> 
> Well, that does not really answer my question, and it does not tell
> us if you now understand the meaning of trailers correctly, or
> replaced an earlier misunderstanding with a different
> misunderstanding ;-)
> 
> More importantly, it would not help me interpret what you mean when
> you write a sequence like this:
> 
> 	From: tanay
>         S-o-b: matthieu
>         S-o-b: tanay
>

Actually it was because that patch contained fixup patch from Matthieu
squashed into it, that is why there were two signoffs.

> which does not quite make sense and leave the reader puzzled.
> 
> If the patch was authored by you and helped by matthieu, the first
> S-o-b would be lying.  If the patch was written by matthieu and you
> are merely relaying, then the authorship From is lying.  If the
> patch was written by you and matthieu is relaying, then the two
> S-o-bs are in incorrect order.
> 

Re: [PATCH 1/7] config.c: fix accuracy of line number in errors

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/25/2014 12:01 AM, Junio C Hamano wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> On 7/24/2014 3:19 AM, Junio C Hamano wrote:
>>>> Tanay Abhra <tanayabh@gmail.com> writes:
>>>>
>>>>> If a callback returns a negative value to `git_config*()` family,
>>>>> they call `die()` while printing the line number and the file name.
>>>>> Currently the printed line number is off by one, thus printing the
>>>>> wrong line number.
>>>>>
>>>>> Make `linenr` point to the line we just parsed during the call
>>>>> to callback to get accurate line number in error messages.
>>>>>
>>>>> Discovered-by: Tanay Abhra <tanayabh@gmail.com>
>>>>> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>
> This patch should have looked like,
>
> From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>
> <commit message>
>
> Commit-message-by: Tanay Abhra <tanayabh@gmail.com>

Yes, the code itself was mine, and Tanay wrote the commit message.

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

[PATCH 2/7] rewrite git_config() to use the config-set API

[Tanay Abhra]

Of all the functions in `git_config*()` family, `git_config()` has the
most invocations in the whole code base. Each `git_config()` invocation
causes config file rereads which can be avoided using the config-set API.

Use the config-set API to rewrite `git_config()` to use the config caching
layer to avoid config file rereads on each invocation during a git process
lifetime. First invocation constructs the cache, and after that for each
successive invocation, `git_config()` feeds values from the config cache
instead of rereading the configuration files.

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

diff --git a/config.c b/config.c
index 6db8f97..a7fb9a4 100644
--- a/config.c
+++ b/config.c
@@ -1232,11 +1232,36 @@ int git_config_with_options(config_fn_t fn, void *data,
 	return ret;
 }
 
-int git_config(config_fn_t fn, void *data)
+static int git_config_raw(config_fn_t fn, void *data)
 {
 	return git_config_with_options(fn, data, NULL, 1);
 }
 
+static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
+{
+	int i;
+	struct string_list *strptr;
+	struct config_set_element *entry;
+	struct hashmap_iter iter;
+	hashmap_iter_init(&cs->config_hash, &iter);
+	while ((entry = hashmap_iter_next(&iter))) {
+		strptr = &entry->value_list;
+		for (i = 0; i < strptr->nr; i++) {
+			if (fn(entry->key, strptr->items[i].string, data) < 0)
+				die("bad config file line in (TODO: file/line info)");
+		}
+	}
+	return 0;
+}
+
+static void git_config_check_init(void);
+
+int git_config(config_fn_t fn, void *data)
+{
+	git_config_check_init();
+	return configset_iter(&the_config_set, fn, data);
+}
+
 static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
 {
 	struct config_set_element k;
@@ -1418,7 +1443,7 @@ static void git_config_check_init(void)
 	if (the_config_set.hash_initialized)
 		return;
 	git_configset_init(&the_config_set);
-	git_config(config_set_callback, &the_config_set);
+	git_config_raw(config_set_callback, &the_config_set);
 }
 
 void git_config_clear(void)
-- 
1.9.0.GIT

Re: [PATCH 2/7] rewrite git_config() to use the config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> -int git_config(config_fn_t fn, void *data)
> +static int git_config_raw(config_fn_t fn, void *data)

As you noticed already, this change breaks several tests. You are going
to repair them later in the series, but your patch series produces a
non-bisectable history.

The history should pass tests at each commit. If needed, you can ensure
that with eg.

git rebase HEAD~7 --exec "make && cd t/ && ./t1308-config-set.sh && ./t4018-diff-funcname.sh" -i

(or --exec 'make test', but that takes really long)

So, this patch should come later in the series (not hard, just a
reordering with rebase -i).

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

Re: [PATCH 2/7] rewrite git_config() to use the config-set API

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
> +{
> +	int i;
> +	struct string_list *strptr;
> +	struct config_set_element *entry;
> +	struct hashmap_iter iter;
> +	hashmap_iter_init(&cs->config_hash, &iter);
> +	while ((entry = hashmap_iter_next(&iter))) {
> +		strptr = &entry->value_list;
> +		for (i = 0; i < strptr->nr; i++) {
> +			if (fn(entry->key, strptr->items[i].string, data) < 0)
> +				die("bad config file line in (TODO: file/line info)");

One more reason to reorder (but that will actually be slightly more than
"rebase -i", you'll have a few conflicts to fix) is to avoid this TODO.
Put the patch after the line number patch and you'll be able to provide
the right information directly.

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

Re: [PATCH 2/7] rewrite git_config() to use the config-set API

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> Of all the functions in `git_config*()` family, `git_config()` has the
> most invocations in the whole code base. Each `git_config()` invocation
> causes config file rereads which can be avoided using the config-set API.
>
> Use the config-set API to rewrite `git_config()` to use the config caching
> layer to avoid config file rereads on each invocation during a git process
> lifetime. First invocation constructs the cache, and after that for each
> successive invocation, `git_config()` feeds values from the config cache
> instead of rereading the configuration files.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  config.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)

This is the natural logical conclusion ;-)

> diff --git a/config.c b/config.c
> index 6db8f97..a7fb9a4 100644
> --- a/config.c
> +++ b/config.c
> @@ -1232,11 +1232,36 @@ int git_config_with_options(config_fn_t fn, void *data,
>  	return ret;
>  }
>  
> -int git_config(config_fn_t fn, void *data)
> +static int git_config_raw(config_fn_t fn, void *data)
>  {
>  	return git_config_with_options(fn, data, NULL, 1);
>  }
>  
> +static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
> +{
> +	int i;
> +	struct string_list *strptr;

We know string_list would hold strings, so naming the variable
strptr does not give us much information.  As this is a list of
values you would get by querying for a variable (or "key"), perhaps
name it "values" or something?

> +	struct config_set_element *entry;
> +	struct hashmap_iter iter;

Have a blank line between the local variable definitions (above) and
the first executable statement (below); it would make it easier to
read, especially because out codebase do not have decl-after-stmt.

> +	hashmap_iter_init(&cs->config_hash, &iter);
> +	while ((entry = hashmap_iter_next(&iter))) {
> +		strptr = &entry->value_list;
> +		for (i = 0; i < strptr->nr; i++) {
> +			if (fn(entry->key, strptr->items[i].string, data) < 0)
> +				die("bad config file line in (TODO: file/line info)");
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void git_config_check_init(void);
> +
> +int git_config(config_fn_t fn, void *data)
> +{
> +	git_config_check_init();
> +	return configset_iter(&the_config_set, fn, data);
> +}

Perhaps if you define this function at the right place in the file
you do not have to add an forward decl of git_config_check_init()?

>  static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
>  {
>  	struct config_set_element k;
> @@ -1418,7 +1443,7 @@ static void git_config_check_init(void)
>  	if (the_config_set.hash_initialized)
>  		return;
>  	git_configset_init(&the_config_set);
> -	git_config(config_set_callback, &the_config_set);
> +	git_config_raw(config_set_callback, &the_config_set);
>  }
>  
>  void git_config_clear(void)

Re: [PATCH 2/7] rewrite git_config() to use the config-set API

[Matthieu Moy]

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

>> +int git_config(config_fn_t fn, void *data)
>> +{
>> +	git_config_check_init();
>> +	return configset_iter(&the_config_set, fn, data);
>> +}
>
> Perhaps if you define this function at the right place in the file
> you do not have to add an forward decl of git_config_check_init()?

OTOH, having git_config right next to git_config_raw and configset_iter
is a good thing, and keeping git_config_raw where git_config used to be
seems logical.

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

[PATCH 3/7] add a test for semantic errors in config files

[Tanay Abhra]

Semantic errors (for example, for alias.* variables NULL values are
not allowed) in configuration files cause a die printing the line
number and file name of the offending value.

Add a test documenting that such errors cause a die printing the
accurate line number and file name.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 t/t1308-config-set.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 7fdf840..bd033df 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -197,4 +197,12 @@ test_expect_success 'proper error on error in custom config files' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check line errors for malformed values' '
+	cp .git/config .git/config.old &&
+	test_when_finished "mv .git/config.old .git/config" &&
+	echo "[alias]\n br" >.git/config &&
+	test_expect_code 128 git br 2>result &&
+	grep "fatal: bad config file line 2 in .git/config" result
+'
+
 test_done
-- 
1.9.0.GIT

Re: [PATCH 3/7] add a test for semantic errors in config files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +test_expect_success 'check line errors for malformed values' '
> +	cp .git/config .git/config.old &&
> +	test_when_finished "mv .git/config.old .git/config" &&
> +	echo "[alias]\n br" >.git/config &&
> +	test_expect_code 128 git br 2>result &&
> +	grep "fatal: bad config file line 2 in .git/config" result
> +'
> +

The test fails at this point in history. I guess the problem will
disapear once you've put PATCH 2 at the right point in the series.

Another option is to mark the test as test_expect_failure when you
introduce it, and change it to test_expect_success when you fix it
(probably not applicable here, but it's a trick I find elegant).

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

Re: [PATCH 3/7] add a test for semantic errors in config files

[Junio C Hamano]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> +test_expect_success 'check line errors for malformed values' '
>> +	cp .git/config .git/config.old &&

Should this be "mv" not "cp"?  You will be overwriting the file from
scratch in the later part of this test.

>> +	test_when_finished "mv .git/config.old .git/config" &&
>> +	echo "[alias]\n br" >.git/config &&

Is the use of \n portable?

> Another option is to mark the test as test_expect_failure when you
> introduce it, and change it to test_expect_success when you fix it
> (probably not applicable here, but it's a trick I find elegant).

Yes, I agree that it is a good practice to document an existing
breakage in an early patch #1, and then make a fix and flip
expect-failure to expect-success in the patch #2.

Breaking the code and documenting the breakage by expecting a
failure in one patch, and then later fixing the breakage and
flipping the expectation in another patch, is a bit less nice,
though ;-)

Re: [PATCH 3/7] add a test for semantic errors in config files

[Tanay Abhra]

On 7/24/2014 3:41 AM, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> +test_expect_success 'check line errors for malformed values' '
>>> +	cp .git/config .git/config.old &&
> 
> Should this be "mv" not "cp"?  You will be overwriting the file from
> scratch in the later part of this test.
>

Noted.

>>> +	test_when_finished "mv .git/config.old .git/config" &&
>>> +	echo "[alias]\n br" >.git/config &&
> 
> Is the use of \n portable?
> 

Yes, you are right, will replace with printf in the next patch.

Re: [PATCH 3/7] add a test for semantic errors in config files

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

>>>> +	test_when_finished "mv .git/config.old .git/config" &&
>>>> +	echo "[alias]\n br" >.git/config &&
>> 
>> Is the use of \n portable?
>> 
>
> Yes, you are right, will replace with printf in the next patch.

... or a cat >.git/config <<\EOF, since this is the construct used
elsewhere in the script.

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

Re: [PATCH 3/7] add a test for semantic errors in config files

[Junio C Hamano]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>>>>> +	test_when_finished "mv .git/config.old .git/config" &&
>>>>> +	echo "[alias]\n br" >.git/config &&
>>> 
>>> Is the use of \n portable?
>>> 
>>
>> Yes, you are right, will replace with printf in the next patch.
>
> ... or a cat >.git/config <<\EOF, since this is the construct used
> elsewhere in the script.

That sounds much better ;-)  Thanks.

[PATCH 4/7] add line number and file name info to `config_set`

[Tanay Abhra]

Store file name and line number for each key-value pair in the cache.
Use the information to print line number and file name in errors raised
by `git_config()` which now uses the configuration files caching layer
internally.

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

diff --git a/config.c b/config.c
index a7fb9a4..43a951f 100644
--- a/config.c
+++ b/config.c
@@ -1237,9 +1237,15 @@ static int git_config_raw(config_fn_t fn, void *data)
 	return git_config_with_options(fn, data, NULL, 1);
 }
 
+struct key_value_info {
+	const char *filename;
+	int linenr;
+};
+
 static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 {
 	int i;
+	struct key_value_info *kv_info;
 	struct string_list *strptr;
 	struct config_set_element *entry;
 	struct hashmap_iter iter;
@@ -1247,8 +1253,15 @@ static int configset_iter(struct config_set *cs, config_fn_t fn, void *data)
 	while ((entry = hashmap_iter_next(&iter))) {
 		strptr = &entry->value_list;
 		for (i = 0; i < strptr->nr; i++) {
-			if (fn(entry->key, strptr->items[i].string, data) < 0)
-				die("bad config file line in (TODO: file/line info)");
+			if (fn(entry->key, strptr->items[i].string, data) < 0) {
+				kv_info = strptr->items[i].util;
+				if (!kv_info->linenr)
+					die("unable to parse command-line config");
+				else
+					die("bad config file line %d in %s",
+						kv_info->linenr,
+						kv_info->filename);
+			}
 		}
 	}
 	return 0;
@@ -1287,6 +1300,8 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
 static int configset_add_value(struct config_set *cs, const char *key, const char *value)
 {
 	struct config_set_element *e;
+	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
+	struct string_list_item *si;
 	e = configset_find_element(cs, key);
 	/*
 	 * Since the keys are being fed by git_config*() callback mechanism, they
@@ -1299,7 +1314,16 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 		string_list_init(&e->value_list, 1);
 		hashmap_add(&cs->config_hash, e);
 	}
-	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	si = string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+	if (cf) {
+		kv_info->filename = strintern(cf->name);
+		kv_info->linenr = cf->linenr;
+	} else {
+		/* for values read from `git_config_from_parameters()` */
+		kv_info->filename = strintern("parameter");
+		kv_info->linenr = 0;
+	}
+	si->util = kv_info;
 
 	return 0;
 }
@@ -1326,7 +1350,7 @@ void git_configset_clear(struct config_set *cs)
 	hashmap_iter_init(&cs->config_hash, &iter);
 	while ((entry = hashmap_iter_next(&iter))) {
 		free(entry->key);
-		string_list_clear(&entry->value_list, 0);
+		string_list_clear(&entry->value_list, 1);
 	}
 	hashmap_free(&cs->config_hash, 1);
 	cs->hash_initialized = 0;
-- 
1.9.0.GIT

Re: [PATCH 4/7] add line number and file name info to `config_set`

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> @@ -1287,6 +1300,8 @@ static struct config_set_element *configset_find_element(struct config_set *cs,
>  static int configset_add_value(struct config_set *cs, const char *key, const char *value)
>  {
>  	struct config_set_element *e;
> +	struct key_value_info *kv_info = xmalloc(sizeof(*kv_info));
> +	struct string_list_item *si;

I have this suspicion that we may want to eventually build a custom
"config_value_list" API that is very similar to what string_list
does, with the only difference from string_list being that the util
item of config_value_item (i.e. a parallel to string_list_item)
would not be a "void *" that points at anything, but is "struct
key_value_info" embedded within, so that we do not have to waste a
pointer and fragmented allocation.

I suspect such a config_value_list API must be built on top of a
kind of generics which C does not allow, which would mean we would
be doing some preprocessor macro tricks (similar to the way how
commit-slab.h allows different kinds of payload) that lets us build
a templated "string-list" API, discarding the existing
"string-list.[ch]" and replacing them with something like these two
liners:

    declare_generic_string_list(string_list, void *); /* in string-list.h */
    define_generic_string_list(string_list, void *); /* in string-list.c */

And at that point,

    declare_generic_string_list(config_value_list, struct key_value);
    define_generic_string_list(config_value_list, struct key_value);

would give us an API declaration and implementation that parallel
that of string-list, but with "struct key_value" in the util field
of each item.

But that kind of clean-up can come much later after this series
settles, and what this patch has is fine for now.

[PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Tanay Abhra]

t4018-diff-funcname.sh fails for the new `git_config()` which uses the
configuration files caching layer internally.
The test introduced in commit d64d6cdc checks that whether `xfuncname` takes
precedence over `funcname` variable which was not guaranteed by config API
previously and worked only because values were parsed and fed in order. The
new  `git_config()` only guarantees precedence order for variables with the
same name.

Also `funcname` variable is deprecated and not documented properly.
`xfuncname` is mentioned in the docs and takes precedence over `funcname`.
Instead of removing `funcname` variable, enforce `xfuncname` precedence over
`funcname` when the variables have the same subsection. Remove dependency
that required values to be fed to userdiff_config() in parsing order for the
test to succeed.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
Note: this the only test that failed for the new git_config() rewrite.

 userdiff.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/userdiff.c b/userdiff.c
index fad52d6..a51bc89 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -2,6 +2,7 @@
 #include "userdiff.h"
 #include "cache.h"
 #include "attr.h"
+#include "string-list.h"
 
 static struct userdiff_driver *drivers;
 static int ndrivers;
@@ -211,9 +212,12 @@ int userdiff_config(const char *k, const char *v)
 	struct userdiff_driver *drv;
 	const char *name, *type;
 	int namelen;
+	char *subsection = NULL;
+	static struct string_list xflag = STRING_LIST_INIT_DUP;
 
 	if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
 		return 0;
+	subsection = xstrndup(name, namelen);
 
 	drv = userdiff_find_by_namelen(name, namelen);
 	if (!drv) {
@@ -224,10 +228,16 @@ int userdiff_config(const char *k, const char *v)
 		drv->binary = -1;
 	}
 
-	if (!strcmp(type, "funcname"))
+	if (!strcmp(type, "funcname") && !unsorted_string_list_has_string(&xflag, subsection)) {
+		free (subsection);
 		return parse_funcname(&drv->funcname, k, v, 0);
-	if (!strcmp(type, "xfuncname"))
+	}
+	if (!strcmp(type, "xfuncname")) {
+		string_list_append(&xflag, subsection);
+		free (subsection);
 		return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
+	}
+	free(subsection);
 	if (!strcmp(type, "binary"))
 		return parse_tristate(&drv->binary, k, v);
 	if (!strcmp(type, "command"))
-- 
1.9.0.GIT

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Also `funcname` variable is deprecated and not documented properly.

I'd say "purposely undocumented (see 45d9414fa5, Brandon Casey, Sep 18
2008)".

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

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Eric Sunshine]

On Wed, Jul 23, 2014 at 2:42 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
> t4018-diff-funcname.sh fails for the new `git_config()` which uses the
> configuration files caching layer internally.
> The test introduced in commit d64d6cdc checks that whether `xfuncname` takes

s/that//

> precedence over `funcname` variable which was not guaranteed by config API
> previously and worked only because values were parsed and fed in order. The
> new  `git_config()` only guarantees precedence order for variables with the

s/\s+/ /

> same name.
>
> Also `funcname` variable is deprecated and not documented properly.
> `xfuncname` is mentioned in the docs and takes precedence over `funcname`.
> Instead of removing `funcname` variable, enforce `xfuncname` precedence over
> `funcname` when the variables have the same subsection. Remove dependency
> that required values to be fed to userdiff_config() in parsing order for the
> test to succeed.
>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
> Note: this the only test that failed for the new git_config() rewrite.
>
>  userdiff.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/userdiff.c b/userdiff.c
> index fad52d6..a51bc89 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -2,6 +2,7 @@
>  #include "userdiff.h"
>  #include "cache.h"
>  #include "attr.h"
> +#include "string-list.h"
>
>  static struct userdiff_driver *drivers;
>  static int ndrivers;
> @@ -211,9 +212,12 @@ int userdiff_config(const char *k, const char *v)
>         struct userdiff_driver *drv;
>         const char *name, *type;
>         int namelen;
> +       char *subsection = NULL;
> +       static struct string_list xflag = STRING_LIST_INIT_DUP;
>
>         if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
>                 return 0;
> +       subsection = xstrndup(name, namelen);
>
>         drv = userdiff_find_by_namelen(name, namelen);
>         if (!drv) {
> @@ -224,10 +228,16 @@ int userdiff_config(const char *k, const char *v)
>                 drv->binary = -1;
>         }
>
> -       if (!strcmp(type, "funcname"))
> +       if (!strcmp(type, "funcname") && !unsorted_string_list_has_string(&xflag, subsection)) {
> +               free (subsection);
>                 return parse_funcname(&drv->funcname, k, v, 0);
> -       if (!strcmp(type, "xfuncname"))
> +       }
> +       if (!strcmp(type, "xfuncname")) {
> +               string_list_append(&xflag, subsection);
> +               free (subsection);
>                 return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
> +       }
> +       free(subsection);
>         if (!strcmp(type, "binary"))
>                 return parse_tristate(&drv->binary, k, v);
>         if (!strcmp(type, "command"))
> --
> 1.9.0.GIT

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> t4018-diff-funcname.sh fails for the new `git_config()` which uses the
> configuration files caching layer internally.
> The test introduced in commit d64d6cdc checks that whether `xfuncname` takes
> precedence over `funcname` variable which was not guaranteed by config API
> previously and worked only because values were parsed and fed in order.

The above is hard to understand.  Do you mean that the old code and
test used whichever (between funcname and xfuncname) appeared last
in the configuration file?

For example, if I had

	diff.foo.xfuncname "Beer$"

in my ~/.gitconfig and then for a particular project I wanted to use
a custom pattern in its .git/config but it was sufficient to define
the pattern without using extended regexp, I would be tempted to say

	diff.foo.funcname "Wine$"

With the "last one of xfuncname or funcname wins" rule, I do not
have to worry about having xfuncname in ~/.gitconfig when I do so,
but with "xfuncname is stronger than funcname" rule, it suddenly
forces me to go back and check if I have something stronger in my
lower-precedence configuration file (i.e. ~/.gitconfig) when I am
trying to override with a higher-precedence configuration file
(i.e. the project-specific .git/config).

If that is what this patch is trying to change, I am not sure if I
agree that is a good change.  They are two ways to spell the same
information, and I would find it more natural if a later funcname
spelled in BRE allowed me to override an earlier xfuncname spelled
in ERE.

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Matthieu Moy]

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

> in my ~/.gitconfig and then for a particular project I wanted to use
> a custom pattern in its .git/config but it was sufficient to define
> the pattern without using extended regexp, I would be tempted to say
>
> 	diff.foo.funcname "Wine$"

The point is: to do that, you need to know that funcname exists, hence
read archives of the doc before 2008 (it disapeared in
45d9414fa5599b41578625961b53e18a9b9148c7) or read the code. And if I
read correctly the commit message for the commit, funcname is not
portable so you should use xfuncname anyway.

The real reason for this change is to let the code work with the new
git_config, but I think it's reasonable not to bother with the original
behavior for a feature deprecated so long ago.

That said, I think the commit message should be improved (recall the
drawbacks of funcname).

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

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> in my ~/.gitconfig and then for a particular project I wanted to use
>> a custom pattern in its .git/config but it was sufficient to define
>> the pattern without using extended regexp, I would be tempted to say
>>
>> 	diff.foo.funcname "Wine$"
>
> The point is: to do that, you need to know that funcname exists, hence
> read archives of the doc before 2008 (it disapeared in
> 45d9414fa5599b41578625961b53e18a9b9148c7) or read the code. And if I
> read correctly the commit message for the commit, funcname is not
> portable so you should use xfuncname anyway.
>
> The real reason for this change is to let the code work with the new
> git_config, but I think it's reasonable not to bother with the original
> behavior for a feature deprecated so long ago.

We may be able to explain funcname vs xfuncname away, but the
problem is much deeper, and it is not just you and Tanay, but
anybody who thought that it is a natural and logical conclusion
to emulate git_config() by iterating over in-core hashtable X-<

We have assumed that it is OK for git_config() to call its callbacks
with any random order of keys as long as the values for the same key
are given in the same order as the original "read from file every
time" implementation would have called.  The assumption overlooked
the possibility that values given to two different keys can
semantically interact.  $foo.funcname and $foo.xfuncname being two
different ways to define the same piece of information is merely one
of the many ways such interactions can happen.  For example, by
assuming that orders of callbacks across keys does not matter, we
would be breaking scripts that expected to be able to deal with
something like this:

	[character]
        	name = Snoopy
                color = white black

                name = Hobbes
                color = yellow white black

For *our* own data, we can force them to be spelled this way instead:

	[character "Snoopy"]
        	color = white black
	[character "Hobbes"]
        	color = yellow white black

but that misses the point, as our config file format and its
semantics are not only about .git/config but the system is designed
and promoted to be a way for third-party applications to store their
own data as well.

Because we do not condone using libgit.a as a library to link into
third-party applications, git_config() at the C level may not have
to read a random third-party data and call its callback in the
predictable way, but this shows us that "git config -l" interface
must return the data in the same order as we have done always by
either using the _raw interface to return things uncached, or
teaching by git_config() to also be predicatable.

> That said, I think the commit message should be improved (recall the
> drawbacks of funcname).

In any case, I agree that the log message and the subject is wrong.
It is not that we want to give xfuncname precedence over funcname;
such a behaviour is undesirable if both were in wide use.  It is
papering over the fallout that comes from the loss of ordering
information, which is imposed by the data structure chosen to
represent a config_set.

I haven't formed a firm opinion whether preserving the order is
necessary at git_config() iteration level yet.  If the only in-core
user that will be broken by not doing so is xfuncname vs funcname,
it may not be too bad, but it will constrain us in the future, which
is a lot bigger problem.  We will be forbidden from correcting a UI
mistake by using the approach we took to transtion "funcname" over
to "xfuncname" (i.e. giving users "funcname" and allowing the
platform BRE parser that may or may not allow the users to write
backslashed ERE is spreading possible incompatibility among the
participants of a project; explicitly stating that the value is ERE
has fewer compatibility issues), while still supporting "funcname"
and have them interact in a sane way.

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Tanay Abhra]

> 
> I haven't formed a firm opinion whether preserving the order is
> necessary at git_config() iteration level yet.  If the only in-core
> user that will be broken by not doing so is xfuncname vs funcname,
> it may not be too bad, but it will constrain us in the future, which
> is a lot bigger problem.

If we give users a choice between two methods git_config_raw() &
git_config_cache(), it would give us a easy fallback if we want
to do something like support xfuncname and funcname for performing
the same function differently.

For core the only test failing was xfuncname vs funcname,
so the situation is not as bad as you think. One course of action
would be leave git_config() as it is, so that third party apps
may not be broken. Provide a function like git_config_cache(),
then rename all the git_config() calls in core to git_config_cache(),
fallback to git_config() where it is not applicable (for example,
git config -l). I haven't thought a lot about it and I haven't
heard Matthieu's opinion about it yet.

Also can you name any third party apps that use the git_config()
system on which I can test the patches. I have seen gitk and gitgui
using the config files to save data.

Cheers,
Tanay Abhra.

  We will be forbidden from correcting a UI
> mistake by using the approach we took to transtion "funcname" over
> to "xfuncname" (i.e. giving users "funcname" and allowing the
> platform BRE parser that may or may not allow the users to write
> backslashed ERE is spreading possible incompatibility among the
> participants of a project; explicitly stating that the value is ERE
> has fewer compatibility issues), while still supporting "funcname"
> and have them interact in a sane way.
> 

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> For core the only test failing was xfuncname vs funcname,

Being a little pessimistic: there may be other cases where the hashtable
magically gives the right order for existing tests, but that would fail
for untested use-cases.

But I can't think of any such case.

> so the situation is not as bad as you think. One course of action
> would be leave git_config() as it is, so that third party apps
> may not be broken. Provide a function like git_config_cache(),
> then rename all the git_config() calls in core to git_config_cache(),
> fallback to git_config() where it is not applicable (for example,
> git config -l).

I think Junio's point about "git config -l" is correct: we should just
keep git_config_raw there.

OTOH, renaming git_config to git_config_cache seems a lot of code churn,
so I'd keep the name git_config. Perhaps git_config_raw is no longer a
good name and it could be called git_config_ordered.

If we keep one call to git_config_raw there, then maybe we can use it
for xfuncname/funcname too, and keep the behavior unchanged.

> Also can you name any third party apps that use the git_config()
> system on which I can test the patches.

There are probably tons of. I can think of git-multimail.

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

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[John Keeping]

On Thu, Jul 24, 2014 at 08:47:45PM +0200, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> > Also can you name any third party apps that use the git_config()
> > system on which I can test the patches.
> 
> There are probably tons of. I can think of git-multimail.

CGit [1] is probably the only one that links with libgit.a and uses the
internal C API.

[1] http://git.zx2c4.com/cgit/

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Junio C Hamano]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> For core the only test failing was xfuncname vs funcname,
>
> Being a little pessimistic: there may be other cases where the hashtable
> magically gives the right order for existing tests, but that would fail
> for untested use-cases.
>
> But I can't think of any such case.
>
>> so the situation is not as bad as you think. One course of action
>> would be leave git_config() as it is, so that third party apps
>> may not be broken. Provide a function like git_config_cache(),
>> then rename all the git_config() calls in core to git_config_cache(),
>> fallback to git_config() where it is not applicable (for example,
>> git config -l).
>
> I think Junio's point about "git config -l" is correct: we should just
> keep git_config_raw there.

I have a slight preference of making git_config() do the right thing
and take advantage of caching behaviour, to be honest.  How much
extra effort is necessary in your code to carry a piece of
information, in addition to what lets you say "here are the values
associated with that key in the order we read from the files", in
order to be able to say "by the way, here is the order of key-value
pairs, if you want to show everything we read in the same order"?
If it would be excessive, using _raw() could be an easy way to punt,
but because we know it is easy to decide to punt, I'd like to see us
see if a real solution is feasible first before punting.

>> Also can you name any third party apps that use the git_config()
>> system on which I can test the patches.
>
> There are probably tons of. I can think of git-multimail.

This question does not really matter.

Among the various points, I actually think the last one you omitted
from your quote, closing door to one useful way to fix UI mistakes
in the future, is the most serious one.

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Tanay Abhra]

On 7/25/2014 12:50 AM, Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> For core the only test failing was xfuncname vs funcname,
>>
>> Being a little pessimistic: there may be other cases where the hashtable
>> magically gives the right order for existing tests, but that would fail
>> for untested use-cases.
>>
>> But I can't think of any such case.
>>
>>> so the situation is not as bad as you think. One course of action
>>> would be leave git_config() as it is, so that third party apps
>>> may not be broken. Provide a function like git_config_cache(),
>>> then rename all the git_config() calls in core to git_config_cache(),
>>> fallback to git_config() where it is not applicable (for example,
>>> git config -l).
>>
>> I think Junio's point about "git config -l" is correct: we should just
>> keep git_config_raw there.
> 
> I have a slight preference of making git_config() do the right thing
> and take advantage of caching behaviour, to be honest.  How much
> extra effort is necessary in your code to carry a piece of
> information, in addition to what lets you say "here are the values
> associated with that key in the order we read from the files", in
> order to be able to say "by the way, here is the order of key-value
> pairs, if you want to show everything we read in the same order"?
> If it would be excessive, using _raw() could be an easy way to punt,
> but because we know it is easy to decide to punt, I'd like to see us
> see if a real solution is feasible first before punting.
>

I am thinking over it, lets see if there is a way before we take the
easy route.

>>> Also can you name any third party apps that use the git_config()
>>> system on which I can test the patches.
>>
>> There are probably tons of. I can think of git-multimail.
> 
> This question does not really matter.
> 
> Among the various points, I actually think the last one you omitted
> from your quote, closing door to one useful way to fix UI mistakes
> in the future, is the most serious one.
>

If we take the easy way out, fixing UI mistakes would be easier,
just replace git_config_cache() with git_config_raw() for such cases.

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> If we take the easy way out, fixing UI mistakes would be easier,
> just replace git_config_cache() with git_config_raw() for such cases.

I do not think that would fly well.

The kind of "let's migrate funcname users to xfuncname while still
supporting the old uses" change will be done in the callback
functions like userdiff_config().  But it is very typical that
multiple config callback functions are cascaded (e.g. many
eventually end up calling git_default_core_config()); in order to a
workaround you suggest to help a callback in deep in a cascade chain
would require you to see which ones among all the callback functions
will eventually call the callback you are updating for migration and
update all git_config() calls to git_config_raw().

If you fix it properly (assuming it is feasible; I haven't heard if
you even tried to see how much work it would involve), you do not
need to introduce "git_config_cached()" (or "_raw()" for that
matter), which is a huge plus.  git_config() would instead do the
right thing automatically, giving the same semantics except that it
does not read the same file twice when it is known that the file has
not changed.

I can understand "If we take the easy way out, we do not have to do
much".  But it would not make it any easier---it essentially closes
the door for "oops, funcname is error-prone; let's give people
xfuncname" type of migration.

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Ramsay Jones]

On 24/07/14 20:54, Junio C Hamano wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> If we take the easy way out, fixing UI mistakes would be easier,
>> just replace git_config_cache() with git_config_raw() for such cases.
> 
> I do not think that would fly well.
> 
> The kind of "let's migrate funcname users to xfuncname while still
> supporting the old uses" change will be done in the callback
> functions like userdiff_config().  But it is very typical that
> multiple config callback functions are cascaded (e.g. many
> eventually end up calling git_default_core_config()); in order to a
> workaround you suggest to help a callback in deep in a cascade chain
> would require you to see which ones among all the callback functions
> will eventually call the callback you are updating for migration and
> update all git_config() calls to git_config_raw().
> 
> If you fix it properly (assuming it is feasible; I haven't heard if
> you even tried to see how much work it would involve), you do not
> need to introduce "git_config_cached()" (or "_raw()" for that
> matter), which is a huge plus.  git_config() would instead do the
> right thing automatically, giving the same semantics except that it
> does not read the same file twice when it is known that the file has
> not changed.
> 

I haven't been following this conversation too closely, so if I have
grasped the wrong end of this stick, please accept my apologies! ;-)

Usually if you need to iterate the values in a hash-table in the order
of key insertion, you would simply link the hash-table entries into a
linked list. This assumes that the keys are distinct, or if not, that
you are using a 'multi-map' type of hash-table. Here, if memory serves
me, you are doing the 'multi' bit yourself within the single hash-table
entry for a given key; so its not quite so easy.

However, I think it you could create a list of <pointer to hash-table
entry, string-list index> pairs in the config_set and use that to do
the iteration. A bit ugly, but it should work.

HTH

ATB,
Ramsay Jones

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Tanay Abhra]

On 7/25/2014 2:52 AM, Ramsay Jones wrote:
> On 24/07/14 20:54, Junio C Hamano wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> If we take the easy way out, fixing UI mistakes would be easier,
>>> just replace git_config_cache() with git_config_raw() for such cases.
>>
>> I do not think that would fly well.
>>
>> The kind of "let's migrate funcname users to xfuncname while still
>> supporting the old uses" change will be done in the callback
>> functions like userdiff_config().  But it is very typical that
>> multiple config callback functions are cascaded (e.g. many
>> eventually end up calling git_default_core_config()); in order to a
>> workaround you suggest to help a callback in deep in a cascade chain
>> would require you to see which ones among all the callback functions
>> will eventually call the callback you are updating for migration and
>> update all git_config() calls to git_config_raw().
>>
>> If you fix it properly (assuming it is feasible; I haven't heard if
>> you even tried to see how much work it would involve), you do not
>> need to introduce "git_config_cached()" (or "_raw()" for that
>> matter), which is a huge plus.  git_config() would instead do the
>> right thing automatically, giving the same semantics except that it
>> does not read the same file twice when it is known that the file has
>> not changed.
>>
> 
> I haven't been following this conversation too closely, so if I have
> grasped the wrong end of this stick, please accept my apologies! ;-)
> 
> Usually if you need to iterate the values in a hash-table in the order
> of key insertion, you would simply link the hash-table entries into a
> linked list. This assumes that the keys are distinct, or if not, that
> you are using a 'multi-map' type of hash-table. Here, if memory serves
> me, you are doing the 'multi' bit yourself within the single hash-table
> entry for a given key; so its not quite so easy.
> 
> However, I think it you could create a list of <pointer to hash-table
> entry, string-list index> pairs in the config_set and use that to do
> the iteration. A bit ugly, but it should work.
>

Thanks for the advice, that is exactly what I am doing.

> HTH
> 
> ATB,
> Ramsay Jones
> 
> 
> 

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/25/2014 2:52 AM, Ramsay Jones wrote:
>> However, I think it you could create a list of <pointer to hash-table
>> entry, string-list index> pairs in the config_set and use that to do
>> the iteration. A bit ugly, but it should work.
>
> Thanks for the advice, that is exactly what I am doing.

I'd just replace "list" with "array" and use
Documentation/technical/api-allocation-growing.txt.

But I can't think of a better way.

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

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Junio C Hamano]

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

> Tanay Abhra <tanayabh@gmail.com> writes:
>
>> On 7/25/2014 2:52 AM, Ramsay Jones wrote:
>>> However, I think it you could create a list of <pointer to hash-table
>>> entry, string-list index> pairs in the config_set and use that to do
>>> the iteration. A bit ugly, but it should work.
>>
>> Thanks for the advice, that is exactly what I am doing.
>
> I'd just replace "list" with "array" and use
> Documentation/technical/api-allocation-growing.txt.
>
> But I can't think of a better way.

Presumably this array will reflect the order the source file told us
about the keys and their values; I wonder if the <filename, lineno>
information we already have can be used (or unified) with it?

Is the "when the last variable in a section disappears, make the
section header go away, with sensible handling of comments around
it" part of this GSoC project?  If so, I suspect that the feature
would also want to know where things appear in the original file, so
this data structure may play a role there, too.

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Tanay Abhra <tanayabh@gmail.com> writes:
>>
>>> On 7/25/2014 2:52 AM, Ramsay Jones wrote:
>>>> However, I think it you could create a list of <pointer to hash-table
>>>> entry, string-list index> pairs in the config_set and use that to do
>>>> the iteration. A bit ugly, but it should work.
>>>
>>> Thanks for the advice, that is exactly what I am doing.
>>
>> I'd just replace "list" with "array" and use
>> Documentation/technical/api-allocation-growing.txt.
>>
>> But I can't think of a better way.
>
> Presumably this array will reflect the order the source file told us
> about the keys and their values; I wonder if the <filename, lineno>
> information we already have can be used (or unified) with it?

I've thought about this too, and I think it would be really hard.

First, there are several files in the picture (eg. /etc/gitconfig,
~/.gitconfig and .git/config), and even included files hence it's not
even a lexical order (file, line).

Then, even if we had a way to order elements with (file, line),
iterating over the hashtable in this order wouldn't be easy (the naive
way, "get the smallest, get the second smallest, ..." would be O(n^2)).

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

Re: [PATCH 5/7] enforce `xfuncname` precedence over `funcname`

[Junio C Hamano]

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

>>> I'd just replace "list" with "array" and use
>>> Documentation/technical/api-allocation-growing.txt.
>>>
>>> But I can't think of a better way.
>>
>> Presumably this array will reflect the order the source file told us
>> about the keys and their values; I wonder if the <filename, lineno>
>> information we already have can be used (or unified) with it?
>
> I've thought about this too, and I think it would be really hard.

Yeah, I do not offhand think of a good solution, either, without
going pointer-crazy and have bidirectional link everywhere X-<.

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

[Tanay Abhra]

Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.

It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,

	if (!git_config_get_value(key, &value)) {
		/* NULL values not allowed */
		if (!value)
			git_config_die(key);
		else
			/* do work */
	}

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt |  5 +++++
 cache.h                                |  1 +
 config.c                               | 24 ++++++++++++++++++++++--
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 8a86e45..14571e7 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -150,6 +150,11 @@ as well as retrieval for the queried variable, including:
 	Similar to `git_config_get_string`, but expands `~` or `~user` into
 	the user's home directory when found at the beginning of the path.
 
+`void git_die_config(const char *key)`::
+
+	Dies printing the line number and the file name of the highest
+	priority value for the configuration variable `key`.
+
 See test-config.c for usage examples.
 
 Value Parsing Helpers
diff --git a/cache.h b/cache.h
index 2f63fd1..fc886c3 100644
--- a/cache.h
+++ b/cache.h
@@ -1380,6 +1380,7 @@ extern int git_config_get_bool(const char *key, int *dest);
 extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
 extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
+extern void git_die_config(const char *key);
 
 extern int committer_ident_sufficiently_given(void);
 extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index 43a951f..f0c9805 100644
--- a/config.c
+++ b/config.c
@@ -1491,8 +1491,12 @@ const struct string_list *git_config_get_value_multi(const char *key)
 
 int git_config_get_string(const char *key, const char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_string(&the_config_set, key, dest);
+	ret = git_configset_get_string(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
 }
 
 int git_config_get_int(const char *key, int *dest)
@@ -1527,8 +1531,24 @@ int git_config_get_maybe_bool(const char *key, int *dest)
 
 int git_config_get_pathname(const char *key, const char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_pathname(&the_config_set, key, dest);
+	ret = git_configset_get_pathname(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
+}
+
+void git_die_config(const char *key)
+{
+	const struct string_list *strptr;
+	struct key_value_info *kv_info;
+	strptr = git_config_get_value_multi(key);
+	kv_info = strptr->items[strptr->nr - 1].util;
+	if (!kv_info->linenr)
+		die("unable to parse command-line config");
+	else
+		die("bad config file line %d in %s",kv_info->linenr, kv_info->filename);
 }
 
 /*
-- 
1.9.0.GIT

[PATCH 7/7] Add tests for `git_config_get_string()`

[Tanay Abhra]

Add tests for `git_config_get_string()`, check whether it
dies printing the line number and the file name if an NULL
value is retrieved for the given key.

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 t/t1308-config-set.sh |  9 +++++++++
 test-config.c         | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index bd033df..1cb453b 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh
@@ -119,6 +119,15 @@ test_expect_success 'find integer value for a key' '
 	check_config get_int lamb.chop 65
 '
 
+test_expect_success 'find string value for a key' '
+	check_config get_string case.baz hask
+'
+
+test_expect_success 'check line error when NULL string is queried' '
+	test_expect_code 128 test-config get_string case.foo 2>result &&
+	grep "fatal: bad config file line 7 in .git/config" result
+'
+
 test_expect_success 'find integer if value is non parse-able' '
 	check_config expect_code 128 get_int lamb.head
 '
diff --git a/test-config.c b/test-config.c
index 9dd1b22..994741a 100644
--- a/test-config.c
+++ b/test-config.c
@@ -16,6 +16,8 @@
  *
  * get_bool -> print bool value for the entered key or die
  *
+ * get_string -> print string value for the entered key or die
+ *
  * configset_get_value -> returns value with the highest priority for the entered key
  * 			from a config_set constructed from files entered as arguments.
  *
@@ -84,6 +86,14 @@ int main(int argc, char **argv)
 			printf("Value not found for \"%s\"\n", argv[2]);
 			goto exit1;
 		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_string")) {
+		if (!git_config_get_string(argv[2], &v)) {
+			printf("%s\n", v);
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
 			int err;
-- 
1.9.0.GIT

Re: [PATCH 7/7] Add tests for `git_config_get_string()`

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> Add tests for `git_config_get_string()`, check whether it
> dies printing the line number and the file name if an NULL

a NULL (no n).

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

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> This series builds on the top of 5def4132 (ta/config-set) in pu or
> topic[1]

Not exactly: 5def4132 has been replaced in pu, and it does not contain
tests, hence PATCH 3 does not apply on top of 5def4132. The series
applies to 0912a24, but does not compile, since you use strintern which
is in master but not in 0912a24.

OK, applied and compiled. Let the review begin.

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

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

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> This series builds on the top of 5def4132 (ta/config-set) in pu or topic[1]
> in the mailing list with name "git config cache & special querying API utilizing
> the cache".
>
> This series aims to do these three things,
>
> * Use the config-set API to rewrite git_config().
>
> * Solve any legacy bugs in the previous system while at it.
>
> * To be feature complete compared to the previous git_config() implementation,
>   which I think it is now. (added the line number and file name info just for
>   completeness)

Sounds promising.

Are you done with the original series, or do you still want to fix
the const-ness issue with the string pointer before working on
follow-up topics like this one?

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

[Tanay Abhra]

> 
> Are you done with the original series, or do you still want to fix
> the const-ness issue with the string pointer before working on
> follow-up topics like this one?
>

I am attaching the v12 with two new functions git_configset_get_string() &
git_configset_get_string_const().

Diff between v11 and v12 is appended below for easy review.

-- 8< --
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 8a86e45..815c1ee 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -138,13 +138,18 @@ as well as retrieval for the queried variable, including:
 	Similar to `git_config_get_bool`, except that it returns -1 on error
 	rather than dying.

-`int git_config_get_string(const char *key, const char **dest)`::
+`int git_config_get_string_const(const char *key, const char **dest)`::

 	Allocates and copies the retrieved string into the `dest` parameter for
 	the configuration variable `key`; if NULL string is given, prints an
 	error message and returns -1. When the configuration variable `key` is
 	not found, returns 1 without touching `dest`.

+`int git_config_get_string(const char *key, char **dest)`::
+
+	Similar to `git_config_get_string_const`, except that retrieved value
+	copied into the `dest` parameter is a mutable string.
+
 `int git_config_get_pathname(const char *key, const char **dest)`::

 	Similar to `git_config_get_string`, but expands `~` or `~user` into
diff --git a/cache.h b/cache.h
index 2f63fd1..7292aef 100644
--- a/cache.h
+++ b/cache.h
@@ -1361,7 +1361,8 @@ extern int git_configset_add_file(struct config_set *cs, const char *filename);
 extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);
 extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);
 extern void git_configset_clear(struct config_set *cs);
-extern int git_configset_get_string(struct config_set *cs, const char *key, const char **dest);
+extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);
+extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);
 extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);
 extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);
 extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);
@@ -1373,7 +1374,8 @@ extern int git_config_get_value(const char *key, const char **value);
 extern const struct string_list *git_config_get_value_multi(const char *key);
 extern void git_config_clear(void);
 extern void git_config_iter(config_fn_t fn, void *data);
-extern int git_config_get_string(const char *key, const char **dest);
+extern int git_config_get_string_const(const char *key, const char **dest);
+extern int git_config_get_string(const char *key, char **dest);
 extern int git_config_get_int(const char *key, int *dest);
 extern int git_config_get_ulong(const char *key, unsigned long *dest);
 extern int git_config_get_bool(const char *key, int *dest);
diff --git a/config.c b/config.c
index 22971e9..0d799e0 100644
--- a/config.c
+++ b/config.c
@@ -1332,7 +1332,7 @@ const struct string_list *git_configset_get_value_multi(struct config_set *cs, c
 	return e ? &e->value_list : NULL;
 }

-int git_configset_get_string(struct config_set *cs, const char *key, const char **dest)
+int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
 {
 	const char *value;
 	if (!git_configset_get_value(cs, key, &value))
@@ -1341,6 +1341,19 @@ int git_configset_get_string(struct config_set *cs, const char *key, const char
 		return 1;
 }

+int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		if (!value)
+			return config_error_nonbool(key);
+		*dest = xstrdup(value);
+		return 0;
+	}
+	else
+		return 1;
+}
+
 int git_configset_get_int(struct config_set *cs, const char *key, int *dest)
 {
 	const char *value;
@@ -1430,7 +1443,13 @@ const struct string_list *git_config_get_value_multi(const char *key)
 	return git_configset_get_value_multi(&the_config_set, key);
 }

-int git_config_get_string(const char *key, const char **dest)
+int git_config_get_string_const(const char *key, const char **dest)
+{
+	git_config_check_init();
+	return git_configset_get_string_const(&the_config_set, key, dest);
+}
+
+int git_config_get_string(const char *key, char **dest)
 {
 	git_config_check_init();
 	return git_configset_get_string(&the_config_set, key, dest);
-- 8< --

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> I am attaching the v12 with two new functions git_configset_get_string() &
> git_configset_get_string_const().

Didn't you intend to change git_config_get_string to let it die in case
of error instead of returning an error code?

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

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

[Tanay Abhra]

On 7/24/2014 9:09 PM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> I am attaching the v12 with two new functions git_configset_get_string() &
>> git_configset_get_string_const().
> 
> Didn't you intend to change git_config_get_string to let it die in case
> of error instead of returning an error code?
>

Yup, but it is the part of the new series not the old one.
I sent this revised patch just for solving the constness
debate.
Also, I would need git_die_config() to print a proper
error message which comes in the 7 part series not this one.

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> On 7/24/2014 9:09 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> I am attaching the v12 with two new functions git_configset_get_string() &
>>> git_configset_get_string_const().
>> 
>> Didn't you intend to change git_config_get_string to let it die in case
>> of error instead of returning an error code?
>>
>
> Yup, but it is the part of the new series not the old one.
> I sent this revised patch just for solving the constness
> debate.
> Also, I would need git_die_config() to print a proper
> error message which comes in the 7 part series not this one.

Ah, right, you don't have line numbers yet, so you can't die properly.

So, hopefully, this part can be considered done, and we can continue
building on top.

As a side effect, I guess Junio will apply this on top of master so the
string interning API will be available immediately at the tip of the
branch.

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

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

[Junio C Hamano]

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

> As a side effect, I guess Junio will apply this on top of master so the
> string interning API will be available immediately at the tip of the
> branch.

Thanks for a heads-up.  The two-patch series does not use the
interning, but does use string_list_init(), which is recent, and
I will have to apply on a newer base than I have been applying its
earlier iterations.

[PATCH v12 1/2] add `config_set` API for caching config-like files

[Tanay Abhra]

Currently `git_config()` uses a callback mechanism and file rereads for
config values. Due to this approach, it is not uncommon for the config
files to be parsed several times during the run of a git program, with
different callbacks picking out different variables useful to themselves.

Add a `config_set`, that can be used to construct an in-memory cache for
config-like files that the caller specifies (i.e., files like `.gitmodules`,
`~/.gitconfig` etc.). Add two external functions `git_configset_get_value`
and `git_configset_get_value_multi` for querying from the config sets.
`git_configset_get_value` follows `last one wins` semantic (i.e. if there
are multiple matches for the queried key in the files of the configset the
value returned will be the last entry in `value_list`).
`git_configset_get_value_multi` returns a list of values sorted in order of
increasing priority (i.e. last match will be at the end of the list). Add
type specific query functions like `git_configset_get_bool` and similar.

Add a default `config_set`, `the_config_set` to cache all key-value pairs
read from usual config files (repo specific .git/config, user wide
~/.gitconfig, XDG config and the global /etc/gitconfig). `the_config_set`
is populated using `git_config()`.

Add two external functions `git_config_get_value` and
`git_config_get_value_multi` for querying in a non-callback manner from
`the_config_set`. Also, add type specific query functions that are
implemented as a thin wrapper around the `config_set` API.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt | 142 +++++++++++++++++
 cache.h                                |  32 ++++
 config.c                               | 282 +++++++++++++++++++++++++++++++++
 setup.c                                |   9 ++
 4 files changed, 465 insertions(+)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 230b3a0..815c1ee 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -77,6 +77,86 @@ To read a specific file in git-config format, use
 `git_config_from_file`. This takes the same callback and data parameters
 as `git_config`.

+Querying For Specific Variables
+-------------------------------
+
+For programs wanting to query for specific variables in a non-callback
+manner, the config API provides two functions `git_config_get_value`
+and `git_config_get_value_multi`. They both read values from an internal
+cache generated previously from reading the config files.
+
+`int git_config_get_value(const char *key, const char **value)`::
+
+	Finds the highest-priority value for the configuration variable `key`,
+	stores the pointer to it in `value` and returns 0. When the
+	configuration variable `key` is not found, returns 1 without touching
+	`value`. The caller should not free or modify `value`, as it is owned
+	by the cache.
+
+`const struct string_list *git_config_get_value_multi(const char *key)`::
+
+	Finds and returns the value list, sorted in order of increasing priority
+	for the configuration variable `key`. When the configuration variable
+	`key` is not found, returns NULL. The caller should not free or modify
+	the returned pointer, as it is owned by the cache.
+
+`void git_config_clear(void)`::
+
+	Resets and invalidates the config cache.
+
+The config API also provides type specific API functions which do conversion
+as well as retrieval for the queried variable, including:
+
+`int git_config_get_int(const char *key, int *dest)`::
+
+	Finds and parses the value to an integer for the configuration variable
+	`key`. Dies on error; otherwise, stores the value of the parsed integer in
+	`dest` and returns 0. When the configuration variable `key` is not found,
+	returns 1 without touching `dest`.
+
+`int git_config_get_ulong(const char *key, unsigned long *dest)`::
+
+	Similar to `git_config_get_int` but for unsigned longs.
+
+`int git_config_get_bool(const char *key, int *dest)`::
+
+	Finds and parses the value into a boolean value, for the configuration
+	variable `key` respecting keywords like "true" and "false". Integer
+	values are converted into true/false values (when they are non-zero or
+	zero, respectively). Other values cause a die(). If parsing is successful,
+	stores the value of the parsed result in `dest` and returns 0. When the
+	configuration variable `key` is not found, returns 1 without touching
+	`dest`.
+
+`int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)`::
+
+	Similar to `git_config_get_bool`, except that integers are copied as-is,
+	and `is_bool` flag is unset.
+
+`int git_config_get_maybe_bool(const char *key, int *dest)`::
+
+	Similar to `git_config_get_bool`, except that it returns -1 on error
+	rather than dying.
+
+`int git_config_get_string_const(const char *key, const char **dest)`::
+
+	Allocates and copies the retrieved string into the `dest` parameter for
+	the configuration variable `key`; if NULL string is given, prints an
+	error message and returns -1. When the configuration variable `key` is
+	not found, returns 1 without touching `dest`.
+
+`int git_config_get_string(const char *key, char **dest)`::
+
+	Similar to `git_config_get_string_const`, except that retrieved value
+	copied into the `dest` parameter is a mutable string.
+
+`int git_config_get_pathname(const char *key, const char **dest)`::
+
+	Similar to `git_config_get_string`, but expands `~` or `~user` into
+	the user's home directory when found at the beginning of the path.
+
+See test-config.c for usage examples.
+
 Value Parsing Helpers
 ---------------------

@@ -134,6 +214,68 @@ int read_file_with_include(const char *file, config_fn_t fn, void *data)
 `git_config` respects includes automatically. The lower-level
 `git_config_from_file` does not.

+Custom Configsets
+-----------------
+
+A `config_set` can be used to construct an in-memory cache for
+config-like files that the caller specifies (i.e., files like `.gitmodules`,
+`~/.gitconfig` etc.). For example,
+
+---------------------------------------
+struct config_set gm_config;
+git_configset_init(&gm_config);
+int b;
+/* we add config files to the config_set */
+git_configset_add_file(&gm_config, ".gitmodules");
+git_configset_add_file(&gm_config, ".gitmodules_alt");
+
+if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore", &b)) {
+	/* hack hack hack */
+}
+
+/* when we are done with the configset */
+git_configset_clear(&gm_config);
+----------------------------------------
+
+Configset API provides functions for the above mentioned work flow, including:
+
+`void git_configset_init(struct config_set *cs)`::
+
+	Initializes the config_set `cs`.
+
+`int git_configset_add_file(struct config_set *cs, const char *filename)`::
+
+	Parses the file and adds the variable-value pairs to the `config_set`,
+	dies if there is an error in parsing the file. Returns 0 on success, or
+	-1 if the file does not exist or is inaccessible. The user has to decide
+	if he wants to free the incomplete configset or continue using it when
+	the function returns -1.
+
+`int git_configset_get_value(struct config_set *cs, const char *key, const char **value)`::
+
+	Finds the highest-priority value for the configuration variable `key`
+	and config set `cs`, stores the pointer to it in `value` and returns 0.
+	When the configuration variable `key` is not found, returns 1 without
+	touching `value`. The caller should not free or modify `value`, as it
+	is owned by the cache.
+
+`const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)`::
+
+	Finds and returns the value list, sorted in order of increasing priority
+	for the configuration variable `key` and config set `cs`. When the
+	configuration variable `key` is not found, returns NULL. The caller
+	should not free or modify the returned pointer, as it is owned by the cache.
+
+`void git_configset_clear(struct config_set *cs)`::
+
+	Clears `config_set` structure, removes all saved variable-value pairs.
+
+In addition to above functions, the `config_set` API provides type specific
+functions in the vein of `git_config_get_int` and family but with an extra
+parameter, pointer to struct `config_set`.
+They all behave similarly to the `git_config_get*()` family described in
+"Querying For Specific Variables" above.
+
 Writing Config Files
 --------------------

diff --git a/cache.h b/cache.h
index fcb511d..7292aef 100644
--- a/cache.h
+++ b/cache.h
@@ -1351,6 +1351,38 @@ extern int parse_config_key(const char *var,
 			    const char **subsection, int *subsection_len,
 			    const char **key);

+struct config_set {
+	struct hashmap config_hash;
+	int hash_initialized;
+};
+
+extern void git_configset_init(struct config_set *cs);
+extern int git_configset_add_file(struct config_set *cs, const char *filename);
+extern int git_configset_get_value(struct config_set *cs, const char *key, const char **value);
+extern const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);
+extern void git_configset_clear(struct config_set *cs);
+extern int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest);
+extern int git_configset_get_string(struct config_set *cs, const char *key, char **dest);
+extern int git_configset_get_int(struct config_set *cs, const char *key, int *dest);
+extern int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);
+extern int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);
+extern int git_configset_get_bool_or_int(struct config_set *cs, const char *key, int *is_bool, int *dest);
+extern int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest);
+extern int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest);
+
+extern int git_config_get_value(const char *key, const char **value);
+extern const struct string_list *git_config_get_value_multi(const char *key);
+extern void git_config_clear(void);
+extern void git_config_iter(config_fn_t fn, void *data);
+extern int git_config_get_string_const(const char *key, const char **dest);
+extern int git_config_get_string(const char *key, char **dest);
+extern int git_config_get_int(const char *key, int *dest);
+extern int git_config_get_ulong(const char *key, unsigned long *dest);
+extern int git_config_get_bool(const char *key, int *dest);
+extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
+extern int git_config_get_maybe_bool(const char *key, int *dest);
+extern int git_config_get_pathname(const char *key, const char **dest);
+
 extern int committer_ident_sufficiently_given(void);
 extern int author_ident_sufficiently_given(void);

diff --git a/config.c b/config.c
index 9767c4b..0d799e0 100644
--- a/config.c
+++ b/config.c
@@ -9,6 +9,8 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 #include "quote.h"
+#include "hashmap.h"
+#include "string-list.h"

 struct config_source {
 	struct config_source *prev;
@@ -33,10 +35,23 @@ struct config_source {
 	long (*do_ftell)(struct config_source *c);
 };

+struct config_set_element {
+	struct hashmap_entry ent;
+	char *key;
+	struct string_list value_list;
+};
+
 static struct config_source *cf;

 static int zlib_compression_seen;

+/*
+ * Default config_set that contains key-value pairs from the usual set of config
+ * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG
+ * config file and the global /etc/gitconfig)
+ */
+static struct config_set the_config_set;
+
 static int config_file_fgetc(struct config_source *conf)
 {
 	return fgetc(conf->u.file);
@@ -1212,6 +1227,270 @@ int git_config(config_fn_t fn, void *data)
 	return git_config_with_options(fn, data, NULL, 1);
 }

+static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)
+{
+	struct config_set_element k;
+	struct config_set_element *found_entry;
+	char *normalized_key;
+	int ret;
+	/*
+	 * `key` may come from the user, so normalize it before using it
+	 * for querying entries from the hashmap.
+	 */
+	ret = git_config_parse_key(key, &normalized_key, NULL);
+
+	if (ret)
+		return NULL;
+
+	hashmap_entry_init(&k, strhash(normalized_key));
+	k.key = normalized_key;
+	found_entry = hashmap_get(&cs->config_hash, &k, NULL);
+	free(normalized_key);
+	return found_entry;
+}
+
+static int configset_add_value(struct config_set *cs, const char *key, const char *value)
+{
+	struct config_set_element *e;
+	e = configset_find_element(cs, key);
+	/*
+	 * Since the keys are being fed by git_config*() callback mechanism, they
+	 * are already normalized. So simply add them without any further munging.
+	 */
+	if (!e) {
+		e = xmalloc(sizeof(*e));
+		hashmap_entry_init(e, strhash(key));
+		e->key = xstrdup(key);
+		string_list_init(&e->value_list, 1);
+		hashmap_add(&cs->config_hash, e);
+	}
+	string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
+
+	return 0;
+}
+
+static int config_set_element_cmp(const struct config_set_element *e1,
+				 const struct config_set_element *e2, const void *unused)
+{
+	return strcmp(e1->key, e2->key);
+}
+
+void git_configset_init(struct config_set *cs)
+{
+	hashmap_init(&cs->config_hash, (hashmap_cmp_fn)config_set_element_cmp, 0);
+	cs->hash_initialized = 1;
+}
+
+void git_configset_clear(struct config_set *cs)
+{
+	struct config_set_element *entry;
+	struct hashmap_iter iter;
+	if (!cs->hash_initialized)
+		return;
+
+	hashmap_iter_init(&cs->config_hash, &iter);
+	while ((entry = hashmap_iter_next(&iter))) {
+		free(entry->key);
+		string_list_clear(&entry->value_list, 0);
+	}
+	hashmap_free(&cs->config_hash, 1);
+	cs->hash_initialized = 0;
+}
+
+static int config_set_callback(const char *key, const char *value, void *cb)
+{
+	struct config_set *cs = cb;
+	configset_add_value(cs, key, value);
+	return 0;
+}
+
+int git_configset_add_file(struct config_set *cs, const char *filename)
+{
+	return git_config_from_file(config_set_callback, filename, cs);
+}
+
+int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
+{
+	const struct string_list *values = NULL;
+	/*
+	 * Follows "last one wins" semantic, i.e., if there are multiple matches for the
+	 * queried key in the files of the configset, the value returned will be the last
+	 * value in the value list for that key.
+	 */
+	values = git_configset_get_value_multi(cs, key);
+
+	if (!values)
+		return 1;
+	assert(values->nr > 0);
+	*value = values->items[values->nr - 1].string;
+	return 0;
+}
+
+const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
+{
+	struct config_set_element *e = configset_find_element(cs, key);
+	return e ? &e->value_list : NULL;
+}
+
+int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value))
+		return git_config_string(dest, key, value);
+	else
+		return 1;
+}
+
+int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		if (!value)
+			return config_error_nonbool(key);
+		*dest = xstrdup(value);
+		return 0;
+	}
+	else
+		return 1;
+}
+
+int git_configset_get_int(struct config_set *cs, const char *key, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_int(key, value);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_ulong(key, value);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_bool(struct config_set *cs, const char *key, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_bool(key, value);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_bool_or_int(struct config_set *cs, const char *key,
+				int *is_bool, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_bool_or_int(key, value, is_bool);
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_maybe_bool(struct config_set *cs, const char *key, int *dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value)) {
+		*dest = git_config_maybe_bool(key, value);
+		if (*dest == -1)
+			return -1;
+		return 0;
+	} else
+		return 1;
+}
+
+int git_configset_get_pathname(struct config_set *cs, const char *key, const char **dest)
+{
+	const char *value;
+	if (!git_configset_get_value(cs, key, &value))
+		return git_config_pathname(dest, key, value);
+	else
+		return 1;
+}
+
+static void git_config_check_init(void)
+{
+	if (the_config_set.hash_initialized)
+		return;
+	git_configset_init(&the_config_set);
+	git_config(config_set_callback, &the_config_set);
+}
+
+void git_config_clear(void)
+{
+	if (!the_config_set.hash_initialized)
+		return;
+	git_configset_clear(&the_config_set);
+}
+
+int git_config_get_value(const char *key, const char **value)
+{
+	git_config_check_init();
+	return git_configset_get_value(&the_config_set, key, value);
+}
+
+const struct string_list *git_config_get_value_multi(const char *key)
+{
+	git_config_check_init();
+	return git_configset_get_value_multi(&the_config_set, key);
+}
+
+int git_config_get_string_const(const char *key, const char **dest)
+{
+	git_config_check_init();
+	return git_configset_get_string_const(&the_config_set, key, dest);
+}
+
+int git_config_get_string(const char *key, char **dest)
+{
+	git_config_check_init();
+	return git_configset_get_string(&the_config_set, key, dest);
+}
+
+int git_config_get_int(const char *key, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_int(&the_config_set, key, dest);
+}
+
+int git_config_get_ulong(const char *key, unsigned long *dest)
+{
+	git_config_check_init();
+	return git_configset_get_ulong(&the_config_set, key, dest);
+}
+
+int git_config_get_bool(const char *key, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_bool(&the_config_set, key, dest);
+}
+
+int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_bool_or_int(&the_config_set, key, is_bool, dest);
+}
+
+int git_config_get_maybe_bool(const char *key, int *dest)
+{
+	git_config_check_init();
+	return git_configset_get_maybe_bool(&the_config_set, key, dest);
+}
+
+int git_config_get_pathname(const char *key, const char **dest)
+{
+	git_config_check_init();
+	return git_configset_get_pathname(&the_config_set, key, dest);
+}
+
 /*
  * Find all the stuff for git_config_set() below.
  */
@@ -1707,6 +1986,9 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	lock = NULL;
 	ret = 0;

+	/* Invalidate the config cache */
+	git_config_clear();
+
 out_free:
 	if (lock)
 		rollback_lock_file(lock);
diff --git a/setup.c b/setup.c
index 0a22f8b..793369d 100644
--- a/setup.c
+++ b/setup.c
@@ -625,6 +625,15 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)
 	int one_filesystem = 1;

 	/*
+	 * We may have read an incomplete configuration before
+	 * setting-up the git directory. If so, clear the cache so
+	 * that the next queries to the configuration reload complete
+	 * configuration (including the per-repo config file that we
+	 * ignored previously).
+	 */
+	git_config_clear();
+
+	/*
 	 * Let's assume that we are in a git repository.
 	 * If it turns out later that we are somewhere else, the value will be
 	 * updated accordingly.
-- 
1.9.0.GIT

Re: [PATCH v12 1/2] add `config_set` API for caching config-like files

[Junio C Hamano]

Tanay Abhra <tanayabh@gmail.com> writes:

> +int git_configset_get_string_const(struct config_set *cs, const char *key, const char **dest)
> +{
> +	const char *value;
> +	if (!git_configset_get_value(cs, key, &value))
> +		return git_config_string(dest, key, value);
> +	else
> +		return 1;
> +}
> +
> +int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
> +{
> +	const char *value;
> +	if (!git_configset_get_value(cs, key, &value)) {
> +		if (!value)
> +			return config_error_nonbool(key);
> +		*dest = xstrdup(value);
> +		return 0;
> +	}
> +	else
> +		return 1;
> +}

Hmm, do we really need duplicate and an almost identical
implementations?

I would have expected either one of these two (expecting the latter
more than the former from the discussion):

 1. Because git_configset_get_string_const() returns a const string
    not to be touched by the caller, it does not even xstrdup(),
    while git_configset_get_string() does.

 2. These behave identically, and the only reason we have two is
    because some callers want to receive the string in "char *"
    while others want to use "const char *".

If you were going route #1, then you would need duplicate but subtly
different implementations; get_string_const() variant would not be
calling git_config_string() because it does xstrdup() the value and
give the caller a new string the caller has to free.

If you were going route #2, the I would have expected there is only
one implementation, _get_string(), and _get_string_const() would
call it while casting the dest parameter it receives.

[PATCH v12 2/2] test-config: add tests for the config_set API

[Tanay Abhra]

Expose the `config_set` C API as a set of simple commands in order to
facilitate testing. Add tests for the `config_set` API as well as for
`git_config_get_*()` family for the usual config files.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 .gitignore            |   1 +
 Makefile              |   1 +
 t/t1308-config-set.sh | 200 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test-config.c         | 142 +++++++++++++++++++++++++++++++++++
 4 files changed, 344 insertions(+)
 create mode 100755 t/t1308-config-set.sh
 create mode 100644 test-config.c

diff --git a/.gitignore b/.gitignore
index 81e12c0..5bfb234 100644
--- a/.gitignore
+++ b/.gitignore
@@ -178,6 +178,7 @@
 /gitweb/static/gitweb.min.*
 /test-chmtime
 /test-ctype
+/test-config
 /test-date
 /test-delta
 /test-dump-cache-tree
diff --git a/Makefile b/Makefile
index 2320de5..b7462e3 100644
--- a/Makefile
+++ b/Makefile
@@ -551,6 +551,7 @@ PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))

 TEST_PROGRAMS_NEED_X += test-chmtime
 TEST_PROGRAMS_NEED_X += test-ctype
+TEST_PROGRAMS_NEED_X += test-config
 TEST_PROGRAMS_NEED_X += test-date
 TEST_PROGRAMS_NEED_X += test-delta
 TEST_PROGRAMS_NEED_X += test-dump-cache-tree
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
new file mode 100755
index 0000000..7fdf840
--- /dev/null
+++ b/t/t1308-config-set.sh
@@ -0,0 +1,200 @@
+#!/bin/sh
+
+test_description='Test git config-set API in different settings'
+
+. ./test-lib.sh
+
+# 'check_config get_* section.key value' verifies that the entry for
+# section.key is 'value'
+check_config () {
+	if test "$1" = expect_code
+	then
+		expect_code="$2" && shift && shift
+	else
+		expect_code=0
+	fi &&
+	op=$1 key=$2 && shift && shift &&
+	if test $# != 0
+	then
+		printf "%s\n" "$@"
+	fi >expect &&
+	test_expect_code $expect_code test-config "$op" "$key" >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'setup default config' '
+	cat >.git/config <<\EOF
+	[case]
+		penguin = very blue
+		Movie = BadPhysics
+		UPPERCASE = true
+		MixedCase = true
+		my =
+		foo
+		baz = sam
+	[Cores]
+		WhatEver = Second
+		baz = bar
+	[cores]
+		baz = bat
+	[CORES]
+		baz = ball
+	[my "Foo bAr"]
+		hi = mixed-case
+	[my "FOO BAR"]
+		hi = upper-case
+	[my "foo bar"]
+		hi = lower-case
+	[case]
+		baz = bat
+		baz = hask
+	[lamb]
+		chop = 65
+		head = none
+	[goat]
+		legs = 4
+		head = true
+		skin = false
+		nose = 1
+		horns
+	EOF
+'
+
+test_expect_success 'get value for a simple key' '
+	check_config get_value case.penguin "very blue"
+'
+
+test_expect_success 'get value for a key with value as an empty string' '
+	check_config get_value case.my ""
+'
+
+test_expect_success 'get value for a key with value as NULL' '
+	check_config get_value case.foo "(NULL)"
+'
+
+test_expect_success 'upper case key' '
+	check_config get_value case.UPPERCASE "true" &&
+	check_config get_value case.uppercase "true"
+'
+
+test_expect_success 'mixed case key' '
+	check_config get_value case.MixedCase "true" &&
+	check_config get_value case.MIXEDCASE "true" &&
+	check_config get_value case.mixedcase "true"
+'
+
+test_expect_success 'key and value with mixed case' '
+	check_config get_value case.Movie "BadPhysics"
+'
+
+test_expect_success 'key with case sensitive subsection' '
+	check_config get_value "my.Foo bAr.hi" "mixed-case" &&
+	check_config get_value "my.FOO BAR.hi" "upper-case" &&
+	check_config get_value "my.foo bar.hi" "lower-case"
+'
+
+test_expect_success 'key with case insensitive section header' '
+	check_config get_value cores.baz "ball" &&
+	check_config get_value Cores.baz "ball" &&
+	check_config get_value CORES.baz "ball" &&
+	check_config get_value coreS.baz "ball"
+'
+
+test_expect_success 'key with case insensitive section header & variable' '
+	check_config get_value CORES.BAZ "ball" &&
+	check_config get_value cores.baz "ball" &&
+	check_config get_value cores.BaZ "ball" &&
+	check_config get_value cOreS.bAz "ball"
+'
+
+test_expect_success 'find value with misspelled key' '
+	check_config expect_code 1 get_value "my.fOo Bar.hi" "Value not found for \"my.fOo Bar.hi\""
+'
+
+test_expect_success 'find value with the highest priority' '
+	check_config get_value case.baz "hask"
+'
+
+test_expect_success 'find integer value for a key' '
+	check_config get_int lamb.chop 65
+'
+
+test_expect_success 'find integer if value is non parse-able' '
+	check_config expect_code 128 get_int lamb.head
+'
+
+test_expect_success 'find bool value for the entered key' '
+	check_config get_bool goat.head 1 &&
+	check_config get_bool goat.skin 0 &&
+	check_config get_bool goat.nose 1 &&
+	check_config get_bool goat.horns 1 &&
+	check_config get_bool goat.legs 1
+'
+
+test_expect_success 'find multiple values' '
+	check_config get_value_multi case.baz sam bat hask
+'
+
+test_expect_success 'find value from a configset' '
+	cat >config2 <<-\EOF &&
+	[case]
+		baz = lama
+	[my]
+		new = silk
+	[case]
+		baz = ball
+	EOF
+	echo silk >expect &&
+	test-config configset_get_value my.new config2 .git/config >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find value with highest priority from a configset' '
+	echo hask >expect &&
+	test-config configset_get_value case.baz config2 .git/config >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'find value_list for a key from a configset' '
+	cat >except <<-\EOF &&
+	sam
+	bat
+	hask
+	lama
+	ball
+	EOF
+	test-config configset_get_value case.baz config2 .git/config >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'proper error on non-existent files' '
+	echo "Error (-1) reading configuration file non-existent-file." >expect &&
+	test_expect_code 2 test-config configset_get_value foo.bar non-existent-file 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success POSIXPERM,SANITY 'proper error on non-accessible files' '
+	chmod -r .git/config &&
+	test_when_finished "chmod +r .git/config" &&
+	echo "Error (-1) reading configuration file .git/config." >expect &&
+	test_expect_code 2 test-config configset_get_value foo.bar .git/config 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'proper error on error in default config files' '
+	cp .git/config .git/config.old &&
+	test_when_finished "mv .git/config.old .git/config" &&
+	echo "[" >>.git/config &&
+	echo "fatal: bad config file line 35 in .git/config" >expect &&
+	test_expect_code 128 test-config get_value foo.bar 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'proper error on error in custom config files' '
+	echo "[" >>syntax-error &&
+	echo "fatal: bad config file line 1 in syntax-error" >expect &&
+	test_expect_code 128 test-config configset_get_value foo.bar syntax-error 2>actual &&
+	test_cmp expect actual
+'
+
+test_done
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 0000000..9dd1b22
--- /dev/null
+++ b/test-config.c
@@ -0,0 +1,142 @@
+#include "cache.h"
+#include "string-list.h"
+
+/*
+ * This program exposes the C API of the configuration mechanism
+ * as a set of simple commands in order to facilitate testing.
+ *
+ * Reads stdin and prints result of command to stdout:
+ *
+ * get_value -> prints the value with highest priority for the entered key
+ *
+ * get_value_multi -> prints all values for the entered key in increasing order
+ *		     of priority
+ *
+ * get_int -> print integer value for the entered key or die
+ *
+ * get_bool -> print bool value for the entered key or die
+ *
+ * configset_get_value -> returns value with the highest priority for the entered key
+ * 			from a config_set constructed from files entered as arguments.
+ *
+ * configset_get_value_multi -> returns value_list for the entered key sorted in
+ * 				ascending order of priority from a config_set
+ * 				constructed from files entered as arguments.
+ *
+ * Examples:
+ *
+ * To print the value with highest priority for key "foo.bAr Baz.rock":
+ * 	test-config get_value "foo.bAr Baz.rock"
+ *
+ */
+
+
+int main(int argc, char **argv)
+{
+	int i, val;
+	const char *v;
+	const struct string_list *strptr;
+	struct config_set cs;
+	git_configset_init(&cs);
+
+	if (argc < 2) {
+		fprintf(stderr, "Please, provide a command name on the command-line\n");
+		goto exit1;
+	} else if (argc == 3 && !strcmp(argv[1], "get_value")) {
+		if (!git_config_get_value(argv[2], &v)) {
+			if (!v)
+				printf("(NULL)\n");
+			else
+				printf("%s\n", v);
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
+		strptr = git_config_get_value_multi(argv[2]);
+		if (strptr) {
+			for (i = 0; i < strptr->nr; i++) {
+				v = strptr->items[i].string;
+				if (!v)
+					printf("(NULL)\n");
+				else
+					printf("%s\n", v);
+			}
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_int")) {
+		if (!git_config_get_int(argv[2], &val)) {
+			printf("%d\n", val);
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
+	} else if (argc == 3 && !strcmp(argv[1], "get_bool")) {
+		if (!git_config_get_bool(argv[2], &val)) {
+			printf("%d\n", val);
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
+	} else if (!strcmp(argv[1], "configset_get_value")) {
+		for (i = 3; i < argc; i++) {
+			int err;
+			if ((err = git_configset_add_file(&cs, argv[i]))) {
+				fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]);
+				goto exit2;
+			}
+		}
+		if (!git_configset_get_value(&cs, argv[2], &v)) {
+			if (!v)
+				printf("(NULL)\n");
+			else
+				printf("%s\n", v);
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
+	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
+		for (i = 3; i < argc; i++) {
+			int err;
+			if ((err = git_configset_add_file(&cs, argv[i]))) {
+				fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]);
+				goto exit2;
+			}
+		}
+		strptr = git_configset_get_value_multi(&cs, argv[2]);
+		if (strptr) {
+			for (i = 0; i < strptr->nr; i++) {
+				v = strptr->items[i].string;
+				if (!v)
+					printf("(NULL)\n");
+				else
+					printf("%s\n", v);
+			}
+			goto exit0;
+		} else {
+			printf("Value not found for \"%s\"\n", argv[2]);
+			goto exit1;
+		}
+	}
+
+	die("%s: Please check the syntax and the function name", argv[0]);
+
+exit0:
+	git_configset_clear(&cs);
+	return 0;
+
+exit1:
+	git_configset_clear(&cs);
+	return 1;
+
+exit2:
+	git_configset_clear(&cs);
+	return 2;
+}
-- 
1.9.0.GIT

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

[Tanay Abhra]

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

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

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

[PATCH V2]: All the suggestions in [2] incorporated. git_config() now follows
	correct parsing order. Reordered the patches. Removed xfuncname patch
	as it was unnecssary.

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

This series aims to do these three things,

* Use the config-set API to rewrite git_config().

* Solve any legacy bugs in the previous system while at it.

* To be feature complete compared to the previous git_config() implementation,
  which I think it is now. (added the line number and file name info just for
  completeness)

Also, I haven't yet checked the exact improvements but still as a teaser,
git status now only rereads the configuration files twice instead of four
times.

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

Tanay Abhra (7):
  config.c: fix accuracy of line number in errors
  add line number and file name info to `config_set`
  change `git_config()` return value to void
  rewrite git_config() to use the config-set API
  add a test for semantic errors in config files
  config: add `git_die_config()` to the config-set API
  add tests for `git_config_get_string_const()`

 Documentation/technical/api-config.txt |   5 ++
 branch.c                               |   5 +-
 cache.h                                |  27 ++++++-
 config.c                               | 131 +++++++++++++++++++++++++++++----
 t/t1308-config-set.sh                  |  21 ++++++
 t/t4055-diff-context.sh                |   2 +-
 test-config.c                          |  10 +++
 7 files changed, 181 insertions(+), 20 deletions(-)

-- 
1.9.0.GIT

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

[Tanay Abhra]

Add `git_die_config` that dies printing the line number and the file name
of the highest priority value for the configuration variable `key`.

It has usage in non-callback based config value retrieval where we can
raise an error and die if there is a semantic error.
For example,

	if (!git_config_get_value(key, &value)) {
		/* NULL values not allowed */
		if (!value)
			git_config_die(key);
		else
			/* do work */
	}

Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
 Documentation/technical/api-config.txt |  5 +++++
 cache.h                                |  1 +
 config.c                               | 27 +++++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index 815c1ee..e7ec7cc 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -155,6 +155,11 @@ as well as retrieval for the queried variable, including:
 	Similar to `git_config_get_string`, but expands `~` or `~user` into
 	the user's home directory when found at the beginning of the path.
 
+`void git_die_config(const char *key)`::
+
+	Dies printing the line number and the file name of the highest
+	priority value for the configuration variable `key`.
+
 See test-config.c for usage examples.
 
 Value Parsing Helpers
diff --git a/cache.h b/cache.h
index 4d3c6bd..243f618 100644
--- a/cache.h
+++ b/cache.h
@@ -1406,6 +1406,7 @@ extern int git_config_get_bool(const char *key, int *dest);
 extern int git_config_get_bool_or_int(const char *key, int *is_bool, int *dest);
 extern int git_config_get_maybe_bool(const char *key, int *dest);
 extern int git_config_get_pathname(const char *key, const char **dest);
+extern void git_die_config(const char *key);
 
 extern int committer_ident_sufficiently_given(void);
 extern int author_ident_sufficiently_given(void);
diff --git a/config.c b/config.c
index 4937515..03369b8 100644
--- a/config.c
+++ b/config.c
@@ -1515,8 +1515,12 @@ const struct string_list *git_config_get_value_multi(const char *key)
 
 int git_config_get_string_const(const char *key, const char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_string_const(&the_config_set, key, dest);
+	ret = git_configset_get_string_const(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
 }
 
 int git_config_get_string(const char *key, char **dest)
@@ -1557,10 +1561,29 @@ int git_config_get_maybe_bool(const char *key, int *dest)
 
 int git_config_get_pathname(const char *key, const char **dest)
 {
+	int ret;
 	git_config_check_init();
-	return git_configset_get_pathname(&the_config_set, key, dest);
+	ret = git_configset_get_pathname(&the_config_set, key, dest);
+	if (ret < 0)
+		git_die_config(key);
+	return ret;
 }
 
+void git_die_config(const char *key)
+{
+	const struct string_list *values;
+	struct key_value_info *kv_info;
+	values = git_config_get_value_multi(key);
+	kv_info = values->items[values->nr - 1].util;
+	if (!kv_info->linenr)
+		die("unable to parse '%s' from command-line config", key);
+	else
+		die("bad config variable '%s' at file line %d in %s",
+			key,
+			kv_info->linenr,
+			kv_info->filename);
+ }
+
 /*
  * Find all the stuff for git_config_set() below.
  */
-- 
1.9.0.GIT

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

[Matthieu Moy]

Tanay Abhra <tanayabh@gmail.com> writes:

> +	if (!kv_info->linenr)
> +		die("unable to parse '%s' from command-line config", key);
> +	else
> +		die("bad config variable '%s' at file line %d in %s",

And two more missing _(...).

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