Re: [GSoC][PATCH v8 05/20] stash: add tests for `git stash show` config

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2018, 22:07 UTC
Message-ID: <xmqqpnxz33v4.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <65603caf56cfaeb1dff92546b7efe38dccb2d071.1535665109.git.ungureanupaulsebastian@gmail.com>

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

Show 41 quoted lines

> This commit introduces tests for `git stash show`
> config. It tests all the cases where `stash.showStat`
> and `stash.showPatch` are unset or set to true / false.
>
> Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
> ---
>  t/t3907-stash-show-config.sh | 81 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100755 t/t3907-stash-show-config.sh
>
> diff --git a/t/t3907-stash-show-config.sh b/t/t3907-stash-show-config.sh
> new file mode 100755
> index 0000000000..8fe369c1a1
> --- /dev/null
> +++ b/t/t3907-stash-show-config.sh
> @@ -0,0 +1,81 @@
> +#!/bin/sh
> +
> +test_description='Test git stash show configuration.'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	test_commit file
> +'
> +
> +# takes three parameters:
> +# 1. the stash.showStat value (or "<unset>")
> +# 2. the stash.showPatch value (or "<unset>")
> +# 3. the diff options of the expected output (or nothing for no output)
> +test_stat_and_patch () {
> +	if test "<unset>" = "$1"
> +	then
> +		test_might_fail git config --unset stash.showStat
> +	else
> +		test_config stash.showStat "$1"
> +	fi &&
> +
> +	if test "<unset>" = "$2"
> +	then
> +		test_might_fail git config --unset stash.showPatch

I think you are trying to protect yourself from an error triggered by unsetting what is not set, but for that, test_unconfig is probably a better choice, as it still catches errors of other types and ignores only that "unset a variable that is not set" error.

Show 6 quoted lines

> +	else
> +		test_config stash.showPatch "$2"
> +	fi &&
> +
> +	shift &&
> +	shift &&

You can use "shift 2 &&" here (not worth a reroll).

> +	echo 2 >file.t &&

> +	git diff "$@" >expect &&

When the caller does not give $3 to this function, it does not look at 'expect'. I think it is clearer if you did

	if test $# != 0
	then
		git diff "$@" >expect
	fi &&

here, and ...

> +	git stash &&
> +	git stash show >actual &&
> +
> +	if test -z "$1"

... wrote this as

	if test $# = 0

The only difference between '-z "$1"' and '$# = 0' is when he caller passes an empty string to the function as $3, which you never do, so the distinction is theoretical, but using $# makes your intention clear that you do not mean to treat an empty string any specially.

Show 44 quoted lines

> +	then
> +		test_must_be_empty actual
> +	else
> +		test_cmp expect actual
> +	fi
> +}
> +
> +test_expect_success 'showStat unset showPatch unset' '
> +	test_stat_and_patch "<unset>" "<unset>" --stat
> +'
> +
> +test_expect_success 'showStat unset showPatch false' '
> +	test_stat_and_patch "<unset>" false --stat
> +'
> +
> +test_expect_success 'showStat unset showPatch true' '
> +	test_stat_and_patch "<unset>" true --stat -p
> +'
> +
> +test_expect_success 'showStat false showPatch unset' '
> +	test_stat_and_patch false "<unset>"
> +'
> +
> +test_expect_success 'showStat false showPatch false' '
> +	test_stat_and_patch false false
> +'
> +
> +test_expect_success 'showStat false showPatch true' '
> +	test_stat_and_patch false true -p
> +'
> +
> +test_expect_success 'showStat true showPatch unset' '
> +	test_stat_and_patch true "<unset>" --stat
> +'
> +
> +test_expect_success 'showStat true showPatch false' '
> +	test_stat_and_patch true false --stat
> +'
> +
> +test_expect_success 'showStat true showPatch true' '
> +	test_stat_and_patch true true --stat -p
> +'
> +
> +test_done