Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> On Mon, Feb 06 2023, Glen Choo wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
