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

[Junio C Hamano <gitster@pobox.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.

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

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

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