test-lib-functions: restrict test_must_fail usage

In previous commits, we removed the usage of test_must_fail() for most
commands except for a set of pre-approved commands. Since that's done,
only allow test_must_fail() to run those pre-approved commands.

Obviously, we should allow `git`.

We allow `__git*` as some completion functions return an error code that
comes from a git invocation. It's good to avoid using test_must_fail
unnecessarily but it wouldn't hurt to err on the side of caution when
we're potentially wrapping a git command (like in these cases).

We also allow `test-tool` and `test-svn-fe` because these are helper
commands that are written by us and we want to catch their failure.

Finally, we allow `test_terminal` because `test_terminal` just wraps
around git commands. Also, we cannot rewrite
`test_must_fail test_terminal` as `test_terminal test_must_fail` because
test_must_fail() is a shell function and as a result, it cannot be
invoked from the test-terminal Perl script.

We opted to explicitly list the above tools instead of using a catch-all
such as `test[-_]*` because we want to be as restrictive as possible so
that in the future, someone would not accidentally introduce an
unrelated usage of test_must_fail() on an "unapproved" command.

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

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 2ff176cd5d954cb1c83f062adbdb28a7e14d2668..90bf1dbc8dbb7015b1b87eea5108e684e43f041b 100755 (executable)
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1271,4 +1271,22 @@ test_expect_success 'very long name in the index handled sanely' '
        test $len = 4098
 '
 
+test_expect_success 'test_must_fail on a failing git command' '
+       test_must_fail git notacommand
+'
+
+test_expect_success 'test_must_fail on a failing git command with env' '
+       test_must_fail env var1=a var2=b git notacommand
+'
+
+test_expect_success 'test_must_fail rejects a non-git command' '
+       ! test_must_fail grep ^$ notafile 2>err &&
+       grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+'
+
+test_expect_success 'test_must_fail rejects a non-git command with env' '
+       ! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&
+       grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
+'
+
 test_done

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3103be8a32393f736481139dd70793ac4e4d97b7..b791933ffd2eadb6a00a9ba2fc5344b457d6f040 100644 (file)
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -798,6 +798,37 @@ list_contains () {
        return 1
 }
 
+# Returns success if the arguments indicate that a command should be
+# accepted by test_must_fail(). If the command is run with env, the env
+# and its corresponding variable settings will be stripped before we
+# test the command being run.
+test_must_fail_acceptable () {
+       if test "$1" = "env"
+       then
+               shift
+               while test $# -gt 0
+               do
+                       case "$1" in
+                       *?=*)
+                               shift
+                               ;;
+                       *)
+                               break
+                               ;;
+                       esac
+               done
+       fi
+
+       case "$1" in
+       git|__git*|test-tool|test-svn-fe|test_terminal)
+               return 0
+               ;;
+       *)
+               return 1
+               ;;
+       esac
+}
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #
@@ -817,6 +848,17 @@ list_contains () {
 #     Multiple signals can be specified as a comma separated list.
 #     Currently recognized signal names are: sigpipe, success.
 #     (Don't use 'success', use 'test_might_fail' instead.)
+#
+# Do not use this to run anything but "git" and other specific testable
+# commands (see test_must_fail_acceptable()).  We are not in the
+# business of vetting system supplied commands -- in other words, this
+# is wrong:
+#
+#    test_must_fail grep pattern output
+#
+# Instead use '!':
+#
+#    ! grep pattern output
 
 test_must_fail () {
        case "$1" in
@@ -828,6 +870,11 @@ test_must_fail () {
                _test_ok=
                ;;
        esac
+       if ! test_must_fail_acceptable "$@"
+       then
+               echo >&7 "test_must_fail: only 'git' is allowed: $*"
+               return 1
+       fi
        "$@" 2>&7
        exit_code=$?
        if test $exit_code -eq 0 && ! list_contains "$_test_ok" success