Re: [PATCH v5.2] lib-submodule-update: pass 'test_must_fail' as an argument

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2020, 16:00 UTC
Message-ID: <xmqqd05o8ndw.fsf@gitster.c.googlers.com>
In-Reply-To: <9ea9d3ff08266e9f1062632c4b1ff1e5f23733bd.1592981264.git.liu.denton@gmail.com>

Denton Liu <liu.denton@gmail.com> writes:

> +# Ensures that the that the arg either contains "test_must_fail" or is empty.
> +may_only_be_test_must_fail () {
> +	test -z "$1" || test "$1" = test_must_fail || die

No error message? I guess it is OK as this is to only catch a bug in the test script, not a bug in Git being tested.

Show 8 quoted lines

> @@ -17,7 +17,8 @@ git_rebase () {
>  	git status -su >actual &&
>  	ls -1pR * >>actual &&
>  	test_cmp expect actual &&
> -	git rebase "$1"
> +	may_only_be_test_must_fail "$2" &&
> +	$2 git rebase "$1"
>  }

I would have expected the sanity check for incoming arguments would be done at the very beginning, but I guess it is OK (the same comment applies to all the others).

Show 11 quoted lines

> @@ -15,7 +15,12 @@ git_revert () {
>  	git status -su >expect &&
>  	ls -1pR * >>expect &&
>  	tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
> -	git checkout "$1" &&
> +	may_only_be_test_must_fail "$2" &&
> +	$2 git checkout "$1" &&
> +	if test -n "$2"
> +	then
> +		return
> +	fi &&

The addition to "git_rebase" does not need this "if must-fail was given, return early" because it was at the end of the helper, but here we have some post procedure that relies on the checkout to be successful, so after correctly failing the checkout we return.

It is somewhat tricky, but OK.  The same for the next one.

Show 18 quoted lines

> diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
> index 6a7e801ca0..a52e53dd2d 100755
> --- a/t/t3906-stash-submodule.sh
> +++ b/t/t3906-stash-submodule.sh
> @@ -8,7 +8,12 @@ test_description='stash can handle submodules'
>  git_stash () {
>  	git status -su >expect &&
>  	ls -1pR * >>expect &&
> -	git read-tree -u -m "$1" &&
> +	may_only_be_test_must_fail "$2" &&
> +	$2 git read-tree -u -m "$1" &&
> +	if test -n "$2"
> +	then
> +		return
> +	fi &&
>  	git stash &&
>  	git status -su >actual &&
>  	ls -1pR * >>actual &&

Looking good.  Will replace.

Thanks.