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

[Junio C Hamano <gitster@pobox.com>]

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

> When $command is a helper function, the parent function calling
> test_submodule_switch_common() is test_submodule_switch_func(). For all
> test_submodule_switch_func() invocations, increase the granularity of
> the argument test helper function by prefixing the git invocation which is
> meant to fail with the second argument like this:
>
> 	$2 git checkout "$1"
>
> ...
> Finally, as an added bonus, `test_must_fail` will now only run on git
> commands.

> This patch replaces v5 4/4. Hopefully, this'll be the last iteration...

So three copies of v5.1 are identical replacement for 4/4, they seem.

> -			test_must_fail $command replace_sub1_with_directory &&
> +			$command replace_sub1_with_directory test_must_fail &&

> +# The following example uses `git some-command` as an example command to be
> +# tested. It updates the worktree and index to match a target, but not any
> +# submodule directories.
>  #
>  # my_func () {
> +#   ...prepare for `git some-command` to be run...
> +#   $2 git some-command "$1" &&
> +#   if test -n "$2"
> +#   then
> +#     return
> +#   fi &&

So, in practice $2 MUST BE either test_must_fail OR an empty
string.  Feeding the my_func anything other than these two values is
a BUG.  I wonder if we can somehow make sure to reduce mental burden
by readers.  Perhaps

> +git_test_func () {

	may_only_be_test_must_fail "$2"

> +	$2 git $gitcmd "$1"
> +}

where

	may_only_be_test_must_fail () {
		test -z "$1" || test "$1" = test_must_fail || die
	}

or something.  The idea is to make sure that this extra helper can
be named in such a way that it can serve as a documention to help
readers of "git_test_func ()" and others about "$2".

> diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
> index 788605ccc0..dd5daa53d3 100755
> --- a/t/t3426-rebase-submodule.sh
> +++ b/t/t3426-rebase-submodule.sh
> @@ -17,7 +17,7 @@ git_rebase () {
>  	git status -su >actual &&
>  	ls -1pR * >>actual &&
>  	test_cmp expect actual &&
> -	git rebase "$1"
> +	$2 git rebase "$1"

Likewise here.

Even without such a change this round reads much better than the
previous ones.

> An alternate design was considered where the global variable,
> $OVERWRITING_FAIL, is set from test_submodule_switch_common() and
> exposed to the helper function.  This would allow $command to be
> set to a more sensible value. However ...

By the way, I do not think this paragraph adds much value to the
message.  It wasn't clear how OVERWRITING_FAIL wanted to achieve its
goal back in its own commit, and without any actual code but with
only the four lines to go by, there is no chance that you can
convince anybody how $command could have been "more sensible value".

Thanks.