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...
