From 4d9e7c153df9b411d782759bbc8def1c8458f4a6 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 7 Jul 2020 02:04:34 -0400 Subject: t3701: stop using `env` in force_color() In a future patch, we plan on making the test_must_fail()-family of functions accept only git commands. Even though force_color() wraps an invocation of `env git`, test_must_fail() will not be able to figure this out since it will assume that force_color() is just some random function which is disallowed. Instead of using `env` in force_color() (which does not support shell functions), export the environment variables in a subshell. Write the invocation as `force_color test_must_fail git ...` since shell functions are now supported. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- t/t3701-add-interactive.sh | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 49decbac71..fb73a847cb 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -31,7 +31,16 @@ diff_cmp () { # indicates a dumb terminal, so we set that variable, too. force_color () { - env GIT_PAGER_IN_USE=true TERM=vt100 "$@" + # The first element of $@ may be a shell function, as a result POSIX + # does not guarantee that "one-shot assignment" will not persist after + # the function call. Thus, we prevent these variables from escaping + # this function's context with this subshell. + ( + GIT_PAGER_IN_USE=true && + TERM=vt100 && + export GIT_PAGER_IN_USE TERM && + "$@" + ) } test_expect_success 'setup (initial)' ' @@ -604,7 +613,7 @@ test_expect_success 'detect bogus diffFilter output' ' echo content >test && test_config interactive.diffFilter "sed 1d" && printf y >y && - test_must_fail force_color git add -p Date: Tue, 7 Jul 2020 02:04:35 -0400 Subject: t5324: reorder `run_with_limited_open_files test_might_fail` In the future, we plan on only allowing `test_might_fail` to work on a restricted subset of commands, including `git`. Reorder the commands so that `run_with_limited_open_files` comes before `test_might_fail`. This way, `test_might_fail` operates on a git command. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- t/t5324-split-commit-graph.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 269d0964a3..9b850ea907 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -399,7 +399,7 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'handles file descriptor exhaustion' for i in $(test_seq 64) do test_commit $i && - test_might_fail run_with_limited_open_files git commit-graph write \ + run_with_limited_open_files test_might_fail git commit-graph write \ --split=no-merge --reachable || return 1 done ) -- cgit v1.2.3 From c96050ff3442067e5848501f25f44a51606f0634 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 7 Jul 2020 02:04:36 -0400 Subject: t7107: don't use test_must_fail() We had a `test_must_fail verify_expect`. However, the git command in verify_expect() was not expected to fail; the test_cmp() was the failing command. Be more precise about testing failure by accepting an optional first argument of '!' which causes the result of the file comparison to be negated. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- t/t7107-reset-pathspec-file.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh index cad3a9de9e..15ccb14f7e 100755 --- a/t/t7107-reset-pathspec-file.sh +++ b/t/t7107-reset-pathspec-file.sh @@ -22,7 +22,12 @@ restore_checkpoint () { verify_expect () { git status --porcelain -- fileA.t fileB.t fileC.t fileD.t >actual && - test_cmp expect actual + if test "x$1" = 'x!' + then + ! test_cmp expect actual + else + test_cmp expect actual + fi } test_expect_success '--pathspec-from-file from stdin' ' @@ -131,7 +136,7 @@ test_expect_success 'quotes not compatible with --pathspec-file-nul' ' cat >expect <<-\EOF && D fileA.t EOF - test_must_fail verify_expect + verify_expect ! ' test_expect_success 'only touches what was listed' ' -- cgit v1.2.3 From 6e7b0ea864c61288bd5276f3e3cd9250e7a3802d Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 7 Jul 2020 02:04:37 -0400 Subject: t9834: remove use of `test_might_fail p4` The test_must_fail() family of functions (including test_might_fail()) should only be used on git commands. Replace test_might_fail() with a compound command wrapping the old p4 invocation that always returns 0. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- t/t9834-git-p4-file-dir-bug.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t9834-git-p4-file-dir-bug.sh b/t/t9834-git-p4-file-dir-bug.sh index 031e1f8668..dac67e89d7 100755 --- a/t/t9834-git-p4-file-dir-bug.sh +++ b/t/t9834-git-p4-file-dir-bug.sh @@ -10,7 +10,7 @@ repository.' test_expect_success 'start p4d' ' start_p4d && - test_might_fail p4 configure set submit.collision.check=0 + { p4 configure set submit.collision.check=0 || :; } ' test_expect_success 'init depot' ' -- cgit v1.2.3 From 41feac6f74719922bcc0bd44683518a6d6a8a943 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 7 Jul 2020 18:08:07 -0400 Subject: t9400: don't use test_must_fail with cvs We are using `test_must_fail cvs` to test that the cvs command fails as expected. However, test_must_fail() is used to ensure that commands fail in an expected way, not due to something like a segv. Since we are not in the business of verifying the sanity of the external world, replace `test_must_fail cvs` with `! cvs` and assume that the cvs command does not die unexpectedly. Signed-off-by: Denton Liu Signed-off-by: Junio C Hamano --- t/t9400-git-cvsserver-server.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index a5e5dca753..4a46f31c41 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -603,7 +603,7 @@ test_expect_success 'cvs server does not run with vanilla git-shell' ' cd cvswork && CVS_SERVER=$WORKDIR/remote-cvs && export CVS_SERVER && - test_must_fail cvs log merge + ! cvs log merge ) ' -- cgit v1.2.3 From 6a67c759489e1025665adf78326e9e0d0981bab5 Mon Sep 17 00:00:00 2001 From: Denton Liu Date: Tue, 7 Jul 2020 02:04:38 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- t/t0000-basic.sh | 18 ++++++++++++++++++ t/test-lib-functions.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 2ff176cd5d..90bf1dbc8d 100755 --- 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 3103be8a32..b791933ffd 100644 --- 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 -- cgit v1.2.3