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

Subsystems: the rest

3 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Matthieu Moy <hidden>
Date: 2016-06-15 23:01:52

Tanay Abhra [off-list ref] writes:

quoted hunk

diff --git a/test-config.c b/test-config.c
new file mode 100644
index 0000000..dc313c2
--- /dev/null
+++ b/test-config.c

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

The configset is initialized, but never cleared.

As a result, valgrind --leak-check=full complains with "definitely lost"
items.

I think it would make sense to apply something like this to get a
valgrind-clean test-config.c (I checked, it now passes without
warnings):

diff --git a/test-config.c b/test-config.c
index dc313c2..07b61ef 100644
--- a/test-config.c
+++ b/test-config.c

@@ -41,17 +41,17 @@ int main(int argc, char **argv)
 
 	if (argc < 2) {
 		fprintf(stderr, "Please, provide a command name on the command-line\n");
-		return 1;
+		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);
-			return 0;
+			goto exit0;

[...]
 
 	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
+	goto exit1;
+
+exit0:
+	git_configset_clear(&cs);
+	return 0;
+
+exit1:
+	git_configset_clear(&cs);
 	return 1;
+
+exit2:
+	git_configset_clear(&cs);
+	return 2;
 }

I'll resend as a proper "git am"-able patch right after.

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

[PATCH 2/2] Better tests for error cases

From: Matthieu Moy <hidden>
Date: 2016-06-15 23:01:52

Signed-off-by: Matthieu Moy <redacted>
---
Consider squashing this into PATCH 2/2

Probably not sufficient.

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

diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh
index 87a29f1..f1e9e76 100755
--- a/t/t1308-config-set.sh
+++ b/t/t1308-config-set.sh

@@ -167,4 +167,26 @@ test_expect_success 'find value_list for a key from a configset' '
 	test_cmp expect actual
 '
 
+test_expect_success 'proper error on non-existant files' '
+	echo "Error reading configuration file non-existant-file." >expect &&
+	test_must_fail test-config configset_get_value foo.bar non-existant-file 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_must_fail 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_must_fail 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
index 07b61ef..49f8cd7 100644
--- a/test-config.c
+++ b/test-config.c

@@ -86,8 +86,10 @@ 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]))
+			if (git_configset_add_file(&cs, argv[i])) {
+				fprintf(stderr, "Error reading configuration file %s.\n", argv[i]);
 				goto exit2;
+			}
 		}
 		if (!git_configset_get_value(&cs, argv[2], &v)) {
 			if (!v)

@@ -101,8 +103,10 @@ 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]))
+			if (git_configset_add_file(&cs, argv[i])) {
+				fprintf(stderr, "Error reading configuration file %s.\n", argv[i]);
 				goto exit2;
+			}
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
 		if (strptr) {

-- 
2.0.0.262.gdafc651

[fixup PATCH 1/2] Call configset_clear

From: Matthieu Moy <hidden>
Date: 2016-06-15 23:01:52

Signed-off-by: Matthieu Moy <redacted>
---
Consider squashing this into PATCH 2/2.

 test-config.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/test-config.c b/test-config.c
index dc313c2..07b61ef 100644
--- a/test-config.c
+++ b/test-config.c

@@ -41,17 +41,17 @@ int main(int argc, char **argv)
 
 	if (argc < 2) {
 		fprintf(stderr, "Please, provide a command name on the command-line\n");
-		return 1;
+		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);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
 		strptr = git_config_get_value_multi(argv[2]);

@@ -63,46 +63,46 @@ int main(int argc, char **argv)
 				else
 					printf("%s\n", v);
 			}
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_int")) {
 		if (!git_config_get_int(argv[2], &val)) {
 			printf("%d\n", val);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_bool")) {
 		if (!git_config_get_bool(argv[2], &val)) {
 			printf("%d\n", val);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (!strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
 			if (git_configset_add_file(&cs, argv[i]))
-				return 2;
+				goto exit2;
 		}
 		if (!git_configset_get_value(&cs, argv[2], &v)) {
 			if (!v)
 				printf("(NULL)\n");
 			else
 				printf("%s\n", v);
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	} else if (!strcmp(argv[1], "configset_get_value_multi")) {
 		for (i = 3; i < argc; i++) {
 			if (git_configset_add_file(&cs, argv[i]))
-				return 2;
+				goto exit2;
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
 		if (strptr) {

@@ -113,13 +113,25 @@ int main(int argc, char **argv)
 				else
 					printf("%s\n", v);
 			}
-			return 0;
+			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			return 1;
+			goto exit1;
 		}
 	}
 
 	fprintf(stderr, "%s: Please check the syntax and the function name\n", argv[0]);
+	goto exit1;
+
+exit0:
+	git_configset_clear(&cs);
+	return 0;
+
+exit1:
+	git_configset_clear(&cs);
 	return 1;
+
+exit2:
+	git_configset_clear(&cs);
+	return 2;
 }

-- 
2.0.0.262.gdafc651

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help