Re: [add-default-config 5/5] fix return code on default + add tests
On Sun, Nov 12, 2017 at 03:00:40PM +, Soukaina NAIT HMID wrote: > diff --git a/builtin/config.c b/builtin/config.c > index eab81c5627091..29c5f55f27a57 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -261,9 +261,12 @@ static int get_value(const char *key_, const char > *regex_) > > if (values.nr == 0 && default_value) { > if(types == TYPE_INT || types == TYPE_BOOL || types == > TYPE_BOOL_OR_INT || types == TYPE_PATH ) { > - char* xstr = normalize_value(key, default_value); > - fwrite(xstr, 1, strlen(xstr), stdout); > - fwrite("\n", 1, 1, stdout); > + if(strlen(default_value)) { > + char* xstr = normalize_value(key, > default_value); > + fwrite(xstr, 1, strlen(xstr), stdout); > + fwrite("\n", 1, 1, stdout); > + ret = 0; > + } > } OK, fixing up the return value is a good thing (though again, I think if we place our default_value in the list earlier that will just fall out naturally). I'm not sure why we care if the default_value string is empty or not. It should be allowed to default to an empty string, I'd think. > diff --git a/t/t9904-default.sh b/t/t9904-default.sh > new file mode 100755 > index 0..8e838f512298b > --- /dev/null > +++ b/t/t9904-default.sh We usually try to group tests with similar-numbered ones. Most of the config tests are in the t13xx area. Probably "t1310-config-default.sh" would be the right place (or if there really are just a few tests, which I think may be all we need, they can just go into t1300). > +boolean() > +{ > + slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") && > + actual=$(git config --default "$1" --bool "$slot") && > + test "$actual" = "$2" > +} A minor style nit, but we usually prefer "test" instead of "[" for conditionals. It took me a while to figure out how this function was meant to be used. It might be worth adding a comment. Though most of it was due to the first line, which I think can just be written as: slot=${3:-no.such.slot} (or you could even just write that directly in the second line). That's a bit more idiomatic for our shell scripts. > +test_expect_success 'empty value for boolean' ' > + invalid_boolean "" > +' There are a lot of tests here about type interpretation, but I think that should be largely orthogonal to the --default feature. Once it's written in a way that's independent of the type, I think we can assume that if "--default" works for one type, it should work with others without being exhaustive. So I think what we really want to test from this series is: 1. --default kicks in when no matching config is found 2. --default does not kick in when config _is_ found 3. (optional) we complain about --default with non-get actions 4. --color works as a type for "get" operations 5. --color is not normalized for "set" operations; if you do: git config --color some.key red we should write "red" into the config file, not the ANSI codes. I know the reason you were looking into t4026 originally because it was the only spot that used --get-color in the whole test suite. But its use of "--get-color" is largely orthogonal to what it's testing. It cares about parsing the specific colors, but just didn't have another easy way to convince Git to parse a bunch of colors without having to pick the results out of diff or log output. I'd be OK with converting that to use "--color --default" instead of --get-color, but if we do so we should make sure that there's some coverage of "--get-color" elsewhere in the config tests (not checking every possible color variation, but just making sure that it can actually look up any color with it). -Peff
[add-default-config 5/5] fix return code on default + add tests
From: Soukaina NAIT HMIDSigned-off-by: Soukaina NAIT HMID --- builtin/config.c | 9 ++- t/t9904-default.sh | 232 + 2 files changed, 238 insertions(+), 3 deletions(-) create mode 100755 t/t9904-default.sh diff --git a/builtin/config.c b/builtin/config.c index eab81c5627091..29c5f55f27a57 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -261,9 +261,12 @@ static int get_value(const char *key_, const char *regex_) if (values.nr == 0 && default_value) { if(types == TYPE_INT || types == TYPE_BOOL || types == TYPE_BOOL_OR_INT || types == TYPE_PATH ) { - char* xstr = normalize_value(key, default_value); - fwrite(xstr, 1, strlen(xstr), stdout); - fwrite("\n", 1, 1, stdout); + if(strlen(default_value)) { + char* xstr = normalize_value(key, default_value); + fwrite(xstr, 1, strlen(xstr), stdout); + fwrite("\n", 1, 1, stdout); + ret = 0; + } } } free(values.items); diff --git a/t/t9904-default.sh b/t/t9904-default.sh new file mode 100755 index 0..8e838f512298b --- /dev/null +++ b/t/t9904-default.sh @@ -0,0 +1,232 @@ +#!/bin/sh + +test_description='Test default color/boolean/int/path' +. ./test-lib.sh + +boolean() +{ + slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") && + actual=$(git config --default "$1" --bool "$slot") && + test "$actual" = "$2" +} + +invalid_boolean() +{ + slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") && + test_must_fail git config --default "$1" --bool "$slot" +} + +test_expect_success 'empty value for boolean' ' + invalid_boolean "" +' + +test_expect_success 'true' ' + boolean "true" "true" +' + +test_expect_success '1 is true' ' + boolean "1" "true" +' + +test_expect_success 'non-zero is true' ' + boolean "5312" "true" +' + +test_expect_success 'false' ' + boolean "false" "false" +' + +test_expect_success '0 is false' ' + boolean "0" "false" +' + +test_expect_success 'invalid value' ' + invalid_boolean "ab" +' + +test_expect_success 'existing slot has priority = true' ' + git config bool.value true && + boolean "false" "true" "bool.value" +' + +test_expect_success 'existing slot has priority = false' ' + git config bool.value false && + boolean "true" "false" "bool.value" +' + +int() +{ + slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") && + actual=$(git config --default "$1" --int "$slot") && + test "$actual" = "$2" +} + +invalid_int() +{ + slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") && + test_must_fail git config "$1" --int "$slot" +} + +test_expect_success 'empty value for int' ' + invalid_int "" "" +' + +test_expect_success 'positive' ' + int "12345" "12345" +' + +test_expect_success 'negative' ' + int "-679032" "-679032" +' + +test_expect_success 'invalid value' ' + invalid_int "abc" +' +test_expect_success 'existing slot has priority = 123' ' + git config int.value 123 && + int "666" "123" "int.value" +' + +test_expect_success 'existing slot with bad value' ' + git config int.value abc && + invalid_int "123" "int.value" +' + +path() +{ + slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") && + actual=$(git config --default "$1" --path "$slot") && + test "$actual" = "$2" +} + +invalid_path() +{ + slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") && + test_must_fail git config "$1" --path "$slot" +} + +test_expect_success 'empty path is invalid' ' + invalid_path "" "" +' + +test_expect_success 'valid path' ' + path "/aa/bb/cc" "/aa/bb/cc" +' + +test_expect_success 'existing slot has priority = /to/the/moon' ' + git config path.value /to/the/moon && + path "/to/the/sun" "/to/the/moon" "path.value" +' +ESC=$(printf '\033') + +color() +{ + slot=$([ "$#" == 3 ] && echo $3 || echo "no.such.slot") && + actual=$(git config --default "$1" --color "$slot" ) && + test "$actual" = "${2:+$ESC}$2" +} + +invalid_color() +{ + slot=$([ "$#" == 2 ] && echo $2 || echo "no.such.slot") && + test_must_fail git config --default "$1" --color "$slot" +} + +test_expect_success 'reset' ' + color "reset" "[m" +' + +test_expect_success 'empty color is empty' ' + color "" "" +' + +test_expect_success 'attribute before color name' ' + color "bold red" "[1;31m" +' + +test_expect_success 'color name before attribute' ' + color "red bold" "[1;31m" +' + +test_expect_success 'attr fg bg' ' + color "ul blue red" "[4;34;41m" +' + +test_expect_success 'fg attr bg' ' + color "blue