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
[PATCH v5 2/2] test-config: Add tests for the config_set API
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: Tanay Abhra tanay...@gmail.com --- .gitignore | 1 + Makefile | 1 + t/t1308-config-hash.sh | 187 + test-config.c | 127 + 4 files changed, 316 insertions(+) create mode 100755 t/t1308-config-hash.sh create mode 100644 test-config.c diff --git a/.gitignore b/.gitignore index 42294e5..eeb66e2 100644 --- a/.gitignore +++ b/.gitignore @@ -176,6 +176,7 @@ /gitweb/static/gitweb.js /gitweb/static/gitweb.min.* /test-chmtime +/test-config /test-ctype /test-date /test-delta diff --git a/Makefile b/Makefile index d503f78..e875070 100644 --- a/Makefile +++ b/Makefile @@ -548,6 +548,7 @@ X = PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS)) TEST_PROGRAMS_NEED_X += test-chmtime +TEST_PROGRAMS_NEED_X += test-config TEST_PROGRAMS_NEED_X += test-ctype TEST_PROGRAMS_NEED_X += test-date TEST_PROGRAMS_NEED_X += test-delta diff --git a/t/t1308-config-hash.sh b/t/t1308-config-hash.sh new file mode 100755 index 000..eba7102 --- /dev/null +++ b/t/t1308-config-hash.sh @@ -0,0 +1,187 @@ +#!/bin/sh + +test_description='Test git config-hash API in different settings' + +. ./test-lib.sh + +test_expect_success 'clear default config' ' + rm -f .git/config +' + +test_expect_success 'initialize default config' ' + cat .git/config EOF + [core] + 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 + [core] + 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' ' + echo very blue expect + test-config get_value core.penguin actual + test_cmp expect actual +' + +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 'upper case key' ' + echo true expect + test-config get_value core.UPPERCASE actual + test_cmp expect actual +' + +test_expect_success 'mixed case key' ' + echo true expect + test-config get_value core.MixedCase actual + test_cmp expect actual +' + +test_expect_success 'key and value with mixed case' ' + echo BadPhysics expect + test-config get_value core.Movie 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 +' + +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 +' + +test_expect_success 'find value with misspelled key' ' + echo Value not found for \my.fOo Bar.hi\ expect + test_must_fail test-config get_value my.fOo Bar.hi actual + test_cmp expect actual +' + +test_expect_success 'find value with the highest priority' ' + echo hask expect + test-config get_value core.bazactual + test_cmp expect actual +' + +test_expect_success 'find integer value for a key' ' + echo 65 expect + test-config get_int lamb.chop actual + test_cmp expect actual +' + +test_expect_success 'find integer if value is non parse-able' ' + echo 65 expect + test_must_fail test-config get_int lamb.head actual + test_must_fail test_cmp expect actual +' + +test_expect_success 'find bool value for the entered key' ' + cat expect -\EOF + 1 + 0 + 1 + 1 + 1 +
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