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

Tanay Abhra [off-list ref] writes:

implemented as a thin wrapper around the `config_set` API.

Signed-off-by: Matthieu Moy <redacted>
Signed-off-by: Tanay Abhra <redacted>
 Documentation/technical/api-config.txt | 137 +++++++++++++++++
 cache.h                                |  30 ++++
 config.c                               | 264 +++++++++++++++++++++++++++++++++
 3 files changed, 431 insertions(+)

(you broke the patch by removing the ---)

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

So, you're now ignoring the return value of git_config. What is the
rationale for this? In particular, why did you reject the "die"
possibility (I understood that you were inclined to take this option, so
I'm curious why you changed your mind).

OTOH, you're transmitting the return value without dying here:

+int git_configset_add_file(struct config_set *cs, const char *filename)
+{
+	return git_config_from_file(config_set_callback, filename, cs);
+}

and I think this one is correct, as we cannot tell in advance how
serious an error would be for any callers. And we do test this (I think
we can improve a bit, I'll send a fixup patch).

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

Signed-off-by: Matthieu Moy <redacted>
---
 t/t1308-config-set.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 4752fd9..ea031bf 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh

@@ -150,8 +150,8 @@ test_expect_success 'find value from a configset' '
 '
 
 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 &&
+	echo hask >expect &&
+	test-config configset_get_value case.baz config2 .git/config >actual &&
 	test_cmp expect actual
 '
 

@@ -163,7 +163,7 @@ test_expect_success 'find value_list for a key from a configset' '
 	lama
 	ball
 	EOF
-	test-config configset_get_value case.baz config2 .git/config  >actual &&
+	test-config configset_get_value case.baz config2 .git/config >actual &&
 	test_cmp expect actual
 '
 

@@ -173,7 +173,7 @@ test_expect_success 'proper error on non-existant files' '
 	test_cmp expect actual
 '
 
-test_expect_success 'proper error on non-accessible  files' '
+test_expect_success 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
 	echo "Error reading configuration file .git/config." >expect &&

@@ -184,14 +184,14 @@ test_expect_success 'proper error on non-accessible  files' '
 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 "[" >>.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 "[" >>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

-- 
2.0.0.262.gdafc651

Signed-off-by: Matthieu Moy <redacted>
---
I won't fight for this, but I think it makes sense.

 t/t1308-config-set.sh |  4 ++--
 test-config.c         | 10 ++++++----
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index ea031bf..f0307b7 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh

@@ -168,7 +168,7 @@ test_expect_success 'find value_list for a key from a configset' '
 '
 
 test_expect_success 'proper error on non-existant files' '
-	echo "Error reading configuration file non-existant-file." >expect &&
+	echo "Error (-1) reading configuration file non-existant-file." >expect &&
 	test_expect_code 2 test-config configset_get_value foo.bar non-existant-file 2>actual &&
 	test_cmp expect actual
 '

@@ -176,7 +176,7 @@ test_expect_success 'proper error on non-existant files' '
 test_expect_success 'proper error on non-accessible files' '
 	chmod -r .git/config &&
 	test_when_finished "chmod +r .git/config" &&
-	echo "Error reading configuration file .git/config." >expect &&
+	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
 '

diff --git a/test-config.c b/test-config.c
index cad35f4..9dd1b22 100644
--- a/test-config.c
+++ b/test-config.c

@@ -86,8 +86,9 @@ int main(int argc, char **argv)
 		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
-			if (git_configset_add_file(&cs, argv[i])) {
-				fprintf(stderr, "Error reading configuration file %s.\n", argv[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;
 			}
 		}

@@ -103,8 +104,9 @@ int main(int argc, char **argv)
 		}
 	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
 		for (i = 3; i < argc; i++) {
-			if (git_configset_add_file(&cs, argv[i])) {
-				fprintf(stderr, "Error reading configuration file %s.\n", argv[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;
 			}
 		}

-- 
2.0.0.262.gdafc651

Signed-off-by: Matthieu Moy <redacted>
---
 Documentation/technical/api-config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index fc0e379..8a86e45 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt

@@ -242,7 +242,7 @@ Configset API provides functions for the above mentioned work flow, including:
 
 	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 doesnot exist or is inaccessible. The user has to decide
+	-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.
 

-- 
2.0.0.262.gdafc651

On 7/16/2014 9:36 PM, Matthieu Moy wrote:

Tanay Abhra [off-list ref] writes:

quoted

implemented as a thin wrapper around the `config_set` API.

Signed-off-by: Matthieu Moy <redacted>
Signed-off-by: Tanay Abhra <redacted>
 Documentation/technical/api-config.txt | 137 +++++++++++++++++
 cache.h                                |  30 ++++
 config.c                               | 264 +++++++++++++++++++++++++++++++++
 3 files changed, 431 insertions(+)

(you broke the patch by removing the ---)

Yikes, sorry about that.

quoted

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

So, you're now ignoring the return value of git_config. What is the
rationale for this? In particular, why did you reject the "die"
possibility (I understood that you were inclined to take this option, so
I'm curious why you changed your mind).

The errors (non accessible, non existent files etc) were already being caught by
git_config_early(). Since git_config() only returns positive values except
the weird race case you mentioned, I thought the die confused the reader
of the patch more than it provided error checking. I also tried myself
simulating the race condition but failed. All the callers of git_config()
also ignore the return value, so I ended up ignoring the return value myself.

OTOH, you're transmitting the return value without dying here:

+int git_configset_add_file(struct config_set *cs, const char *filename)
+{
+	return git_config_from_file(config_set_callback, filename, cs);
+}

and I think this one is correct, as we cannot tell in advance how
serious an error would be for any callers. And we do test this (I think
we can improve a bit, I'll send a fixup patch).

After reading the commit log that you mentioned and some previous ones before
that I surmised that the official slant was to silently ignore nonexistent
files. Though an access_or_warn() check was placed on most of the files
like git attributes, since non accessible file errors may be a user configuration
error. So, I decided to ignore the return value.

But I do think that an access_or_warn() check should be put on git config --file
and git_configset_add_file since other parts of git follow it. What do
you think about it, still I will send followup patch correcting the git config
--file condition where it silently ignores the file access error and continues?

I think it would be unnecessary for the current iteration.
Currently git_configset_add_file has only two possible return values
-1 or 0. I could add specialized error values for ENOENT or ENOTDIR
or EACCES, but the logs show that we silently ignore the first two.
I can add an access warn for the third. What do you think?

Thanks,
Tanay.

On 7/16/2014 9:39 PM, Matthieu Moy wrote: