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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2020, 23:54 UTC
Message-ID: <xmqq4kr1bao8.fsf@gitster.c.googlers.com>
In-Reply-To: <7b9c19b3606f31b12a79591a41847dcb0a071751.1592942452.git.liu.denton@gmail.com>

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

Show 11 quoted lines

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

Show 11 quoted lines

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

Show 10 quoted lines

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