| Message ID | patch-v4-2.9-1f0f8bdcde9-20230202T131155Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | config API: make "multi" safe, fix numerous segfaults | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > A less well known edge case in the config format is that keys can be > value-less, a shorthand syntax for "true" boolean keys. I.e. these two > are equivalent as far as "--type=bool" is concerned: > > [a]key > [a]key = true > > But as far as our parser is concerned the values for these two are > NULL, and "true". I.e. for a sequence like: > > [a]key=x > [a]key > [a]key=y > > We get a "struct string_list" with "string" members with ".string" > values of: > > { "x", NULL, "y" } > > This behavior goes back to the initial implementation of > git_config_bool() in 17712991a59 (Add ".git/config" file parser, > 2005-10-10). > > When parts of the config_set API were tested for in [1] they didn't > add coverage for 3/4 of the "(NULL)" cases handled in > "t/helper/test-config.c". We'd test that case for "get_value", but not > "get_value_multi", "configset_get_value" and > "configset_get_value_multi". > > We now cover all of those cases, which in turn expose the details of > how this part of the config API works. Good to see a better coverage. With the "last one wins" semantics for half of these 4 cases, it may make sense to further extend the tests to cover cases where the last one is a valueless true, in addition to what is used in this patch (i.e. a valueless true in the middle of three). Thanks.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > +test_NULL_in_multi () { > + local op="$1" && > + local file="$2" && > + > + test_expect_success "$op: NULL value in config${file:+ in $file}" ' > + config="$file" && > + if test -z "$config" > + then > + config=.git/config && > + test_when_finished "mv $config.old $config" && > + mv "$config" "$config".old > + fi && > + > + cat >"$config" <<-\EOF && > + [a]key=x > + [a]key > + [a]key=y > + EOF > + case "$op" in > + *_multi) > + cat >expect <<-\EOF > + x > + (NULL) > + y > + EOF > + ;; > + *) > + cat >expect <<-\EOF > + y > + EOF > + ;; > + esac && > + test-tool config "$op" a.key $file >actual && > + test_cmp expect actual > + ' > +} > + > +test_NULL_in_multi "get_value_multi" > +test_NULL_in_multi "configset_get_value" "my.config" > +test_NULL_in_multi "configset_get_value_multi" "my.config" I frankly preferred v3's tests over this version. v3 is slightly verbose, but at least the lack of logic made it easy to read and understand. I'd be okay with it if we get a big DRY-ness benefit, but 2 conditionals for 3 cases seems quite un-DRY to me.
On Mon, Feb 06 2023, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> +test_NULL_in_multi () { >> + local op="$1" && >> + local file="$2" && >> + >> + test_expect_success "$op: NULL value in config${file:+ in $file}" ' >> + config="$file" && >> + if test -z "$config" >> + then >> + config=.git/config && >> + test_when_finished "mv $config.old $config" && >> + mv "$config" "$config".old >> + fi && >> + >> + cat >"$config" <<-\EOF && >> + [a]key=x >> + [a]key >> + [a]key=y >> + EOF >> + case "$op" in >> + *_multi) >> + cat >expect <<-\EOF >> + x >> + (NULL) >> + y >> + EOF >> + ;; >> + *) >> + cat >expect <<-\EOF >> + y >> + EOF >> + ;; >> + esac && >> + test-tool config "$op" a.key $file >actual && >> + test_cmp expect actual >> + ' >> +} >> + >> +test_NULL_in_multi "get_value_multi" >> +test_NULL_in_multi "configset_get_value" "my.config" >> +test_NULL_in_multi "configset_get_value_multi" "my.config" > > I frankly preferred v3's tests over this version. v3 is slightly > verbose, but at least the lack of logic made it easy to read and > understand. I'd be okay with it if we get a big DRY-ness benefit, but 2 > conditionals for 3 cases seems quite un-DRY to me. Note that the v3 version didn't test the get_value_multi(), adjusting the v3 version with copy/paste to test that as well is why I made this a function. From the CL's range-diff: - When the "t/t1308-config-set.sh" tests were added in [1] only one of - the three "(NULL)" lines in "t/helper/test-config.c" had any test - coverage. This change adds tests that stress the remaining two. + When parts of the config_set API were tested for in [1] they didn't + add coverage for 3/4 of the "(NULL)" cases handled in + "t/helper/test-config.c". We'd test that case for "get_value", but not + "get_value_multi", "configset_get_value" and + "configset_get_value_multi". + + We now cover all of those cases, which in turn expose the details of + how this part of the config API works. Of course that wouldn't address an outstanding point that we should just copy/paste these anyway, but maybe that addresses your feedback...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Mon, Feb 06 2023, Glen Choo wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> +test_NULL_in_multi () { >>> + local op="$1" && >>> + local file="$2" && >>> + >>> + test_expect_success "$op: NULL value in config${file:+ in $file}" ' >>> + config="$file" && >>> + if test -z "$config" >>> + then >>> + config=.git/config && >>> + test_when_finished "mv $config.old $config" && >>> + mv "$config" "$config".old >>> + fi && >>> + >>> + cat >"$config" <<-\EOF && >>> + [a]key=x >>> + [a]key >>> + [a]key=y >>> + EOF >>> + case "$op" in >>> + *_multi) >>> + cat >expect <<-\EOF >>> + x >>> + (NULL) >>> + y >>> + EOF >>> + ;; >>> + *) >>> + cat >expect <<-\EOF >>> + y >>> + EOF >>> + ;; >>> + esac && >>> + test-tool config "$op" a.key $file >actual && >>> + test_cmp expect actual >>> + ' >>> +} >>> + >>> +test_NULL_in_multi "get_value_multi" >>> +test_NULL_in_multi "configset_get_value" "my.config" >>> +test_NULL_in_multi "configset_get_value_multi" "my.config" >> >> I frankly preferred v3's tests over this version. v3 is slightly >> verbose, but at least the lack of logic made it easy to read and >> understand. I'd be okay with it if we get a big DRY-ness benefit, but 2 >> conditionals for 3 cases seems quite un-DRY to me. > > Note that the v3 version didn't test the get_value_multi(), adjusting > the v3 version with copy/paste to test that as well is why I made this a > function. From the CL's range-diff: > > - When the "t/t1308-config-set.sh" tests were added in [1] only one of > - the three "(NULL)" lines in "t/helper/test-config.c" had any test > - coverage. This change adds tests that stress the remaining two. > + When parts of the config_set API were tested for in [1] they didn't > + add coverage for 3/4 of the "(NULL)" cases handled in > + "t/helper/test-config.c". We'd test that case for "get_value", but not > + "get_value_multi", "configset_get_value" and > + "configset_get_value_multi". > + > + We now cover all of those cases, which in turn expose the details of > + how this part of the config API works. > > Of course that wouldn't address an outstanding point that we should just > copy/paste these anyway, but maybe that addresses your feedback... Ah, yes I noticed that (thanks for confirming), and I meant that I think it would be better to just copy/paste anyway.
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index b38e158d3b2..b172565f92a 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -146,6 +146,47 @@ test_expect_success 'find multiple values' ' check_config get_value_multi case.baz sam bat hask ' +test_NULL_in_multi () { + local op="$1" && + local file="$2" && + + test_expect_success "$op: NULL value in config${file:+ in $file}" ' + config="$file" && + if test -z "$config" + then + config=.git/config && + test_when_finished "mv $config.old $config" && + mv "$config" "$config".old + fi && + + cat >"$config" <<-\EOF && + [a]key=x + [a]key + [a]key=y + EOF + case "$op" in + *_multi) + cat >expect <<-\EOF + x + (NULL) + y + EOF + ;; + *) + cat >expect <<-\EOF + y + EOF + ;; + esac && + test-tool config "$op" a.key $file >actual && + test_cmp expect actual + ' +} + +test_NULL_in_multi "get_value_multi" +test_NULL_in_multi "configset_get_value" "my.config" +test_NULL_in_multi "configset_get_value_multi" "my.config" + test_expect_success 'find value from a configset' ' cat >config2 <<-\EOF && [case]
A less well known edge case in the config format is that keys can be value-less, a shorthand syntax for "true" boolean keys. I.e. these two are equivalent as far as "--type=bool" is concerned: [a]key [a]key = true But as far as our parser is concerned the values for these two are NULL, and "true". I.e. for a sequence like: [a]key=x [a]key [a]key=y We get a "struct string_list" with "string" members with ".string" values of: { "x", NULL, "y" } This behavior goes back to the initial implementation of git_config_bool() in 17712991a59 (Add ".git/config" file parser, 2005-10-10). When parts of the config_set API were tested for in [1] they didn't add coverage for 3/4 of the "(NULL)" cases handled in "t/helper/test-config.c". We'd test that case for "get_value", but not "get_value_multi", "configset_get_value" and "configset_get_value_multi". We now cover all of those cases, which in turn expose the details of how this part of the config API works. 1. 4c715ebb96a (test-config: add tests for the config_set API, 2014-07-28) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t1308-config-set.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)