From: Glen Choo <chooglen@google.com>
Date: Mon, 06 Feb 2023 10:40:08 GMT
Subject: Re: [PATCH v4 2/9] config tests: add "NULL" tests for *_get_value_multi()
Message-ID: <kl6lzg9rgjqv.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-v4-2.9-1f0f8bdcde9-20230202T131155Z-avarab@gmail.com>

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