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

Paul-Sebastian Ungureanu [off-list ref] writes:

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.

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

+	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

Hi Junio,

On Thu, 30 Aug 2018, Junio C Hamano wrote:

Paul-Sebastian Ungureanu [off-list ref] writes:

quoted

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

My mistake. I did not realize that tehere is a `test_unconfig` when I
suggested the current version.

quoted

+	shift &&
+	shift &&

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

Again, my mistake. I was not sure how portable that construct is. (Is it?)

quoted

+	echo 2 >file.t &&

quoted

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

quoted

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

Good advice in general (in this case, we know the callers).

Paul, when it comes to shell scripting, listen to Junio more than myself
because he is even more of a shell script wiz than I am.

Ciao,
Dscho