| Message ID | patch-v3-3.9-14b08dfc162-20221125T093159Z-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). I didn't know about this behavior before, actually. Thanks for the explanation. > 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. I initially read this as testing that t/helper/test-config.c is doing the right thing, which would be the antipattern of writing tests for our tests. During Review Club, you mentioned that the motivation was something else, which IIRC is closer to exercising the internals of the configset API, which makes sense to me, thought it would be helpful to clarify that better in the commit message. > +test_expect_success 'emit multi values from configset with NULL entry' ' > + test_when_finished "rm -f my.config" && > + cat >my.config <<-\EOF && > + [a]key=x > + [a]key > + [a]key=y > + EOF > + cat >expect <<-\EOF && > + x > + (NULL) > + y > + EOF > + test-tool config configset_get_value_multi a.key my.config >actual && > + test_cmp expect actual > +' So if this meant to exercise configset_get_value_multi(), maybe it would be even clearer to just say so in the test name, e.g. 'configset_get_value_multi with NULL entry'. Side comment: by the end of the series, *_get_value_multi() has no legitimate callers outside of config.c and the test code, and if we remove it from config.h, this scenario wouldn't ever bother us in actual `git` usage, but we probably still want to test for it since we want to exercise the internals.
diff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh index b38e158d3b2..561e82f1808 100755 --- a/t/t1308-config-set.sh +++ b/t/t1308-config-set.sh @@ -146,6 +146,36 @@ test_expect_success 'find multiple values' ' check_config get_value_multi case.baz sam bat hask ' +test_expect_success 'emit multi values from configset with NULL entry' ' + test_when_finished "rm -f my.config" && + cat >my.config <<-\EOF && + [a]key=x + [a]key + [a]key=y + EOF + cat >expect <<-\EOF && + x + (NULL) + y + EOF + test-tool config configset_get_value_multi a.key my.config >actual && + test_cmp expect actual +' + +test_expect_success 'multi values from configset with a last NULL entry' ' + test_when_finished "rm -f my.config" && + cat >my.config <<-\EOF && + [a]key=x + [a]key=y + [a]key + EOF + cat >expect <<-\EOF && + (NULL) + EOF + test-tool config configset_get_value a.key my.config >actual && + test_cmp expect actual +' + 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 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. 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 | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)