lib-submodule-update: pass 'test_must_fail' as an argument

When we run a test helper function in test_submodule_switch_common(), we
sometimes specify a whole helper function as the $command. When we do
this, in some test cases, we just mark the whole function with
`test_must_fail`. However, it's possible that the helper function might
fail earlier or later than expected due to an introduced bug. If this
happens, then the test case will still report as passing but it should
really be marked as failing since it didn't actually display the
intended behaviour.

Instead of invoking `test_must_fail $command`, pass the string
"test_must_fail" as the second argument in case where the git command is
expected to fail.

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"

In the other cases, test_submodule_switch() and
test_submodule_forced_switch(), instead of passing in the git command
directly, wrap it using the git_test_func() and pass the git arguments
using the global variable $gitcmd. Unfortunately, since closures aren't
a thing in shell scripts, the global variable is necessary. Another
unfortunate result is that the "git_test_func" will used as the test
case name when $command is printed but it's worth it for the cleaner
code.

Finally, as an added bonus, `test_must_fail` will now only run on git
commands.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 7c3ba1be007d75a1187901d128933bc900a6b1ce..8e9f310bcb6aa2e3752410c7e44e9dcd716d55aa 100755 (executable)
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -303,8 +303,12 @@ test_submodule_content () {
 # update" is run. And even then that command doesn't delete the work tree of
 # a removed submodule.
 #
+# The first argument of the callback function will be the name of the submodule.
+#
 # Removing a submodule containing a .git directory must fail even when forced
-# to protect the history!
+# to protect the history! If we are testing this case, the second argument of
+# the callback function will be 'test_must_fail', else it will be the empty
+# string.
 #
 
 # Internal function; use test_submodule_switch_func(), test_submodule_switch(),
@@ -443,7 +447,7 @@ test_submodule_switch_common () {
                (
                        cd submodule_update &&
                        git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
-                       test_must_fail $command replace_sub1_with_directory &&
+                       $command replace_sub1_with_directory test_must_fail &&
                        test_superproject_content origin/add_sub1 &&
                        test_submodule_content sub1 origin/add_sub1
                )
@@ -456,7 +460,7 @@ test_submodule_switch_common () {
                        cd submodule_update &&
                        git branch -t replace_sub1_with_directory origin/replace_sub1_with_directory &&
                        replace_gitfile_with_git_dir sub1 &&
-                       test_must_fail $command replace_sub1_with_directory &&
+                       $command replace_sub1_with_directory test_must_fail &&
                        test_superproject_content origin/add_sub1 &&
                        test_git_directory_is_unchanged sub1 &&
                        test_submodule_content sub1 origin/add_sub1
@@ -470,7 +474,7 @@ test_submodule_switch_common () {
                (
                        cd submodule_update &&
                        git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
-                       test_must_fail $command replace_sub1_with_file &&
+                       $command replace_sub1_with_file test_must_fail &&
                        test_superproject_content origin/add_sub1 &&
                        test_submodule_content sub1 origin/add_sub1
                )
@@ -484,7 +488,7 @@ test_submodule_switch_common () {
                        cd submodule_update &&
                        git branch -t replace_sub1_with_file origin/replace_sub1_with_file &&
                        replace_gitfile_with_git_dir sub1 &&
-                       test_must_fail $command replace_sub1_with_file &&
+                       $command replace_sub1_with_file test_must_fail &&
                        test_superproject_content origin/add_sub1 &&
                        test_git_directory_is_unchanged sub1 &&
                        test_submodule_content sub1 origin/add_sub1
@@ -559,12 +563,25 @@ test_submodule_switch_common () {
 # conditions, set the appropriate KNOWN_FAILURE_* variable used in the tests
 # below to 1.
 #
-# Use as follows:
+# The first argument of the callback function will be the name of the submodule.
+#
+# Removing a submodule containing a .git directory must fail even when forced
+# to protect the history! If we are testing this case, the second argument of
+# the callback function will be 'test_must_fail', else it will be the empty
+# string.
+#
+# 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 () {
-#   target=$1
-#   # Do something here that updates the worktree and index to match target,
-#   # but not any submodule directories.
+#   ...prepare for `git some-command` to be run...
+#   $2 git some-command "$1" &&
+#   if test -n "$2"
+#   then
+#     return
+#   fi &&
+#   ...check the state after git some-command is run...
 # }
 # test_submodule_switch_func "my_func"
 test_submodule_switch_func () {
@@ -580,23 +597,35 @@ test_submodule_switch_func () {
                        cd submodule_update &&
                        git branch -t add_sub1 origin/add_sub1 &&
                        >sub1 &&
-                       test_must_fail $command add_sub1 &&
+                       $command add_sub1 test_must_fail &&
                        test_superproject_content origin/no_submodule &&
                        test_must_be_empty sub1
                )
        '
 }
 
+# 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
+}
+
+git_test_func () {
+       may_only_be_test_must_fail "$2" &&
+       $2 git $gitcmd "$1"
+}
+
 test_submodule_switch () {
-       test_submodule_switch_func "git $1"
+       gitcmd="$1"
+       test_submodule_switch_func "git_test_func"
 }
 
 # Same as test_submodule_switch(), except that throwing away local changes in
 # the superproject is allowed.
 test_submodule_forced_switch () {
-       command="$1"
+       gitcmd="$1"
+       command="git_test_func"
        KNOWN_FAILURE_FORCED_SWITCH_TESTS=1
-       test_submodule_switch_common "git $command"
+       test_submodule_switch_common "$command"
 
        # When forced, a file in the superproject does not prevent creating a
        # submodule of the same name.

diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index 788605ccc08f31d6c21f0232b2e2f0efbddbcb3c..0ad3a07bf470939a8fa53f38e83fa609f2d7371f 100755 (executable)
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -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"
 }
 
 test_submodule_switch_func "git_rebase"
@@ -35,7 +36,8 @@ git_rebase_interactive () {
        test_cmp expect actual &&
        set_fake_editor &&
        echo "fake-editor.sh" >.git/info/exclude &&
-       git rebase -i "$1"
+       may_only_be_test_must_fail "$2" &&
+       $2 git rebase -i "$1"
 }
 
 test_submodule_switch_func "git_rebase_interactive"

diff --git a/t/t3513-revert-submodule.sh b/t/t3513-revert-submodule.sh
index 95a7f644717b871656a9dd04740d1dc96fb2a3ee..a759f12cbb1d6b7676d639806cdbe94e1cc201ac 100755 (executable)
--- a/t/t3513-revert-submodule.sh
+++ b/t/t3513-revert-submodule.sh
@@ -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 &&
        git revert HEAD &&
        rm -rf * &&
        tar xf "$TRASH_DIRECTORY/tmp.tar" &&

diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
index 6a7e801ca042642baf532d6a3ae45048e2520d7e..a52e53dd2da481e3c24a82d9599a58c0d1c5ab43 100755 (executable)
--- 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 &&

diff --git a/t/t4137-apply-submodule.sh b/t/t4137-apply-submodule.sh
index b645e303a0ddab8e638f31a8bdf1c8f8410af9b4..07d52625375d4bec97db0a14d74a4fd154681b5e 100755 (executable)
--- a/t/t4137-apply-submodule.sh
+++ b/t/t4137-apply-submodule.sh
@@ -6,13 +6,17 @@ test_description='git apply handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 apply_index () {
-       git diff --ignore-submodules=dirty "..$1" | git apply --index -
+       git diff --ignore-submodules=dirty "..$1" >diff &&
+       may_only_be_test_must_fail "$2" &&
+       $2 git apply --index diff
 }
 
 test_submodule_switch_func "apply_index"
 
 apply_3way () {
-       git diff --ignore-submodules=dirty "..$1" | git apply --3way -
+       git diff --ignore-submodules=dirty "..$1" >diff &&
+       may_only_be_test_must_fail "$2" &&
+       $2 git apply --3way diff
 }
 
 test_submodule_switch_func "apply_3way"

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 1b179d5f45c62fc563daf0791c3870502362740c..a7ba08f728c0b870bf899ee82220a1708699dcb0 100755 (executable)
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -6,13 +6,17 @@ test_description='git am handling submodules'
 . "$TEST_DIRECTORY"/lib-submodule-update.sh
 
 am () {
-       git format-patch --stdout --ignore-submodules=dirty "..$1" | git am -
+       git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+       may_only_be_test_must_fail "$2" &&
+       $2 git am patch
 }
 
 test_submodule_switch_func "am"
 
 am_3way () {
-       git format-patch --stdout --ignore-submodules=dirty "..$1" | git am --3way -
+       git format-patch --stdout --ignore-submodules=dirty "..$1" >patch &&
+       may_only_be_test_must_fail "$2" &&
+       $2 git am --3way patch
 }
 
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1

diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index f911bf631eb7c2293b51f65171e0c2c7fcab738d..1d75e3b12ba1641d6bad4a8f76e84df9a9bf59f7 100755 (executable)
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -13,7 +13,8 @@ reset_branch_to_HEAD () {
 
 git_pull () {
        reset_branch_to_HEAD "$1" &&
-       git pull
+       may_only_be_test_must_fail "$2" &&
+       $2 git pull
 }
 
 # pulls without conflicts
@@ -21,21 +22,24 @@ test_submodule_switch_func "git_pull"
 
 git_pull_ff () {
        reset_branch_to_HEAD "$1" &&
-       git pull --ff
+       may_only_be_test_must_fail "$2" &&
+       $2 git pull --ff
 }
 
 test_submodule_switch_func "git_pull_ff"
 
 git_pull_ff_only () {
        reset_branch_to_HEAD "$1" &&
-       git pull --ff-only
+       may_only_be_test_must_fail "$2" &&
+       $2 git pull --ff-only
 }
 
 test_submodule_switch_func "git_pull_ff_only"
 
 git_pull_noff () {
        reset_branch_to_HEAD "$1" &&
-       git pull --no-ff
+       may_only_be_test_must_fail "$2" &&
+       $2 git pull --no-ff
 }
 
 KNOWN_FAILURE_NOFF_MERGE_DOESNT_CREATE_EMPTY_SUBMODULE_DIR=1

diff --git a/t/t6041-bisect-submodule.sh b/t/t6041-bisect-submodule.sh
index 0e0cdf638d7baf55042d6c55ba7afaf7e8932743..df1eff0fb831e3ede980157c577c575feab04a4d 100755 (executable)
--- a/t/t6041-bisect-submodule.sh
+++ b/t/t6041-bisect-submodule.sh
@@ -10,7 +10,12 @@ git_bisect () {
        ls -1pR * >>expect &&
        tar cf "$TRASH_DIRECTORY/tmp.tar" * &&
        GOOD=$(git rev-parse --verify HEAD) &&
-       git checkout "$1" &&
+       may_only_be_test_must_fail "$2" &&
+       $2 git checkout "$1" &&
+       if test -n "$2"
+       then
+               return
+       fi &&
        echo "foo" >bar &&
        git add bar &&
        git commit -m "bisect bad" &&