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

Denton Liu [off-list ref] 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".

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.