Re: [PATCH v5 2/2] test-config: Add tests for the config_set API
Ramkumar Ramachandra artag...@gmail.com writes: A couple of quick nits. Tanay Abhra wrote: +test_expect_success 'clear default config' ' + rm -f .git/config +' Unnecessary; a fresh temporary directory is created for each test run. Hmm, fresh, but not empty. Anyway, the next test does a cat on this file, so it will erase its content, so the rm -f is actually not needed. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] test-config: Add tests for the config_set API
Tanay Abhra tanay...@gmail.com writes:
+test_expect_success 'get value for a simple key' '
+ echo very blue expect
+ test-config get_value core.penguin actual
+ test_cmp expect actual
+'
All these tests would greatly benefit from a helper like
test_expect_config () {
echo 1 expect
test-config get_value $2 actual
test_cmp expect actual
}
Then, all the 3-liners below would become 1-liners.
Should not block inclusion, but may be worth considering.
+test_expect_success 'get value for a key with value as an empty string' '
+ echo expect
+ test-config get_value core.my actual
+ test_cmp expect actual
+'
+
+test_expect_success 'get value for a key with value as NULL' '
+ echo (NULL) expect
+ test-config get_value core.foo actual
+ test_cmp expect actual
+'
[...]
+test_expect_success 'key with case sensitive subsection' '
+ echo mixed-case expect
+ echo upper-case expect
+ echo lower-case expect
+ test-config get_value my.Foo bAr.hi actual
+ test-config get_value my.FOO BAR.hi actual
+ test-config get_value my.foo bar.hi actual
+ test_cmp expect actual
+'
This would become a 3-liner with my helper.
+test_expect_success 'key with case insensitive section header' '
+ echo ball expect
+ echo ball expect
+ echo ball expect
+ test-config get_value cores.baz actual
+ test-config get_value Cores.baz actual
+ test-config get_value CORES.baz actual
+ test_cmp expect actual
+'
I think you miss a simple case: get_value with a case that doesn't exist
in the config file, like get_value coreS.baz.
+test_expect_success 'find value with the highest priority' '
+ echo hask expect
+ test-config get_value core.bazactual
Space before .
diff --git a/test-config.c b/test-config.c
No time for a real review of this file, but from a quick look, it seems
OK.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] test-config: Add tests for the config_set API
Tanay Abhra tanay...@gmail.com writes:
diff --git a/test-config.c b/test-config.c
new file mode 100644
index 000..45ccd0a
--- /dev/null
+++ b/test-config.c
@@ -0,0 +1,127 @@
+#include cache.h
+#include hashmap.h
Useless include, you're not using the hashmap directly.
+int main(int argc, char **argv)
+{
+ int i, no_of_files;
Unused no_of_files.
With
CFLAGS += -Wdeclaration-after-statement -Wall -Werror
in config.mak, my git.git refuses to compile with your patch. You should
have similar options for hacking on git.git.
+ if (argc == 3 !strcmp(argv[1], get_value)) {
You should do something like
if (argc 2) {
fprintf(stderr, Please, provide a command name on the command-line\n);
return 1;
}
before this. Otherwise, some argv[1] below are invalid on mis-use. No
need for thorough checks since it's just a test program, but better
avoid undefined behavior and segfaults anyway...
+ if (!git_config_get_value(argv[2], v)) {
+ if (!v)
+ printf((NULL)\n);
+ else
+ printf(%s\n, v);
+ return 0;
+ } else {
+ printf(Value not found for \%s\\n, argv[2]);
+ return -1;
Avoid returning negative values from main. Your shell's $? won't see -1,
but most likely 255 or so, but I think it even depends on the OS.
You don't seem to use main's return for anything but error, so 0 =
everything OK; 1 = some error is the common convention.
+ } else {
+ printf(Value not found for \%s\\n, argv[2]);
+ return -1;
+ }
+
+ } else if (!strcmp(argv[1], configset_get_value_multi)) {
Why a blank line before this else if and not before other else ifs?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/2] test-config: Add tests for the config_set API
A couple of quick nits. Tanay Abhra wrote: +test_expect_success 'clear default config' ' + rm -f .git/config +' Unnecessary; a fresh temporary directory is created for each test run. +test_expect_success 'initialize default config' ' You might want to mark this as setup. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
