[PATCH 0/5] t: replace incorrect test_must_fail usage (part 6)

[PATCH 0/5] t: replace incorrect test_must_fail usage (part 6)

[Denton Liu]

The overall scope of these patches is to replace inappropriate uses of
test_must_fail. IOW, we should only allow test_must_fail to run on `git`
and `test-tool`. Ultimately, we will conclude by making test_must_fail
error out on non-git commands. An advance view of the final series can
be found here[1].

This is the sixth and final(!) part. It cleans up instances of
`test_must_fail` that were introduced since the effort began. In
addition, it finally flips the switch and makes test_must_fail only
allow a whitelist of commands.

This series is based on the merge of 'master' and
'dl/test-must-fail-fixes-5'. In addition, this series was tested by
merging with 'seen~1' (to ignore the reftable failures) to ensure no
in-flight topics will require more changes.

The first part can be found here[2]. The second part can be found
here[3]. The third part can be found here[4]. The fourth part can be
found here[5]. The fifth part can be found here[6].

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/
[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/
[6]: https://lore.kernel.org/git/cover.1588162842.git.liu.denton@gmail.com/

Denton Liu (5):
  t3701: stop using `env` in force_color()
  t5324: reorder `run_with_limited_open_files test_might_fail`
  t7107: don't use test_must_fail()
  t9834: remove use of `test_might_fail p4`
  test-lib-functions: restrict test_must_fail usage

 t/t0000-basic.sh               | 18 ++++++++++++++++
 t/t3701-add-interactive.sh     | 10 +++++++--
 t/t5324-split-commit-graph.sh  |  2 +-
 t/t7107-reset-pathspec-file.sh |  9 ++++++--
 t/t9834-git-p4-file-dir-bug.sh |  2 +-
 t/test-lib-functions.sh        | 39 ++++++++++++++++++++++++++++++++++
 6 files changed, 74 insertions(+), 6 deletions(-)

-- 
2.27.0.383.g050319c2ae

[PATCH 1/5] t3701: stop using `env` in force_color()

[Denton Liu]

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 <liu.denton@gmail.com>
---
 t/t3701-add-interactive.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 49decbac71..c4c1e9b603 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -31,7 +31,13 @@ diff_cmp () {
 # indicates a dumb terminal, so we set that variable, too.
 
 force_color () {
-	env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
+	(
+		GIT_PAGER_IN_USE=true &&
+		export GIT_PAGER_IN_USE &&
+		TERM=vt100 &&
+		export TERM &&
+		"$@"
+	)
 }
 
 test_expect_success 'setup (initial)' '
@@ -604,7 +610,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 <y
+	force_color test_must_fail git add -p <y
 '
 
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
-- 
2.27.0.383.g050319c2ae

Re: [PATCH 1/5] t3701: stop using `env` in force_color()

[Eric Sunshine]

On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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 <liu.denton@gmail.com>
> ---
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> @@ -31,7 +31,13 @@ diff_cmp () {
>  force_color () {
> -       env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
> +       (
> +               GIT_PAGER_IN_USE=true &&
> +               export GIT_PAGER_IN_USE &&
> +               TERM=vt100 &&
> +               export TERM &&
> +               "$@"
> +       )
>  }

I'm having trouble understanding why this function was transformed the
way it was. I presume the subshell is to ensure that the variable
assignments don't escape the function context since you're dropping
'env', however, it seems the following would be simpler:

    force_color ()  {
        (GIT_PAGER_IN_USE=true TERM=vt100 "$@")
    }

Or, is there something non-obvious going on that I'm missing?

By the way, I'm wondering if the subshell deserves an in-code comment.
Whereas, we have somewhat settled upon the idiom 'test_must_fail env
FOO=bar ...' when we need to make sure variable assignments don't
escape the local context -- since 'FOO=bar test_must_fail ...' doesn't
make that guarantee under all shells -- the use of a subshell here to
achieve the same (if I'm understanding correctly) is not nearly so
obvious. The non-obviousness is due to "$@" being abstract -- someone
reading the code won't necessarily realize that the first element of
"$@" might be a shell function, thus would not necessarily understand
the use of a subshell.

Re: [PATCH 1/5] t3701: stop using `env` in force_color()

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:
>> 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 <liu.denton@gmail.com>
>> ---
>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>> @@ -31,7 +31,13 @@ diff_cmp () {
>>  force_color () {
>> -       env GIT_PAGER_IN_USE=true TERM=vt100 "$@"
>> +       (
>> +               GIT_PAGER_IN_USE=true &&
>> +               export GIT_PAGER_IN_USE &&
>> +               TERM=vt100 &&
>> +               export TERM &&
>> +               "$@"
>> +       )
>>  }
>
> I'm having trouble understanding why this function was transformed the
> way it was. I presume the subshell is to ensure that the variable
> assignments don't escape the function context since you're dropping
> 'env', however, it seems the following would be simpler:
>
>     force_color ()  {
>         (GIT_PAGER_IN_USE=true TERM=vt100 "$@")
>     }
>
> Or, is there something non-obvious going on that I'm missing?

Denton wants to be able to write

	force_color test_must_fail git foo blah

The only known kind of POSIX breaking behaviour by some shells makes
the assignment persist after the "$@" returns, and the subshell
would be a good way to protect the caller of force_color from the
effect of the assignment leaking, so if we only care about that
known breakage, both Denton's and your simplification should work
OK, but it is probably clearer to see that we are *not* relying on
any one-shot environment assignment at all in Denton's version.

I'd list two variables on a single export command, though ;-)

> By the way, I'm wondering if the subshell deserves an in-code comment.
> Whereas, we have somewhat settled upon the idiom 'test_must_fail env
> FOO=bar ...' when we need to make sure variable assignments don't
> escape the local context -- since 'FOO=bar test_must_fail ...' doesn't
> make that guarantee under all shells -- the use of a subshell here to
> achieve the same (if I'm understanding correctly) is not nearly so
> obvious. The non-obviousness is due to "$@" being abstract -- someone
> reading the code won't necessarily realize that the first element of
> "$@" might be a shell function, thus would not necessarily understand
> the use of a subshell.

Probably.

[PATCH 2/5] t5324: reorder `run_with_limited_open_files test_might_fail`

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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
 	)
-- 
2.27.0.383.g050319c2ae

[PATCH 3/5] t7107: don't use test_must_fail()

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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' '
-- 
2.27.0.383.g050319c2ae

[PATCH 4/5] t9834: remove use of `test_might_fail p4`

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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' '
-- 
2.27.0.383.g050319c2ae

[PATCH 5/5] test-lib-functions: restrict test_must_fail usage

[Denton Liu]

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

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>
---
 t/t0000-basic.sh        | 18 ++++++++++++++++++
 t/test-lib-functions.sh | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 2ff176cd5d..f5e4fb515d 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 env var3=c 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 env var3=c 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..16596b28ba 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -798,6 +798,31 @@ 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 () {
+	while test "$1" = "env"
+	do
+		shift
+		while test $# -gt 0
+		do
+			case "$1" in *?=*) ;; *) break ;; esac
+			shift
+		done
+	done
+
+	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 +842,15 @@ 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
+#
+# Just use '!' instead.
 
 test_must_fail () {
 	case "$1" in
@@ -828,6 +862,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
-- 
2.27.0.383.g050319c2ae

Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage

[Eric Sunshine]

On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:
> 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 case).

s/case/cases/

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> +# 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 () {
> +       while test "$1" = "env"

I was surprised to see a 'while' loop for stripping 'env'. Did you
actually run across cases in the test suite in which 'env' was
invoking 'env'? If so, were such cases legitimate (as opposed to
accidental)? Perhaps the commit message or an in-code comment could
help readers understand why it needs to strip multiple 'env's.

> +       do
> +               shift
> +               while test $# -gt 0
> +               do
> +                       case "$1" in *?=*) ;; *) break ;; esac
> +                       shift
> +               done
> +       done

Isn't '*?=*' the same as '?=', or am I misunderstanding the intention?
Also, I wonder how important it is to insist that there must be at
least one character before the '=' sign. (It doesn't necessarily hurt,
but I'm curious if it is protecting against legitimate weird cases.)

This logic would be easier to follow written this way:

    case "$1" in
        =) shift ;;
        *) break ;;
    esac

That is, place the 'shift' in the appropriate case-arm rather than
suspending it below all cases.

> +       case "$1" in
> +       git|__git*|test-tool|test-svn-fe|test_terminal)
> +               return 0
> +               ;;
> +       *)
> +               return 1
> +               ;;
> +       esac
> +}

Would it make sense to error out if "$1" has no value? That is, if the
author wrote:

    test_must_fail &&

or

    test_must_fail env foo=bar &&

then that surely is a programmer error, which could be diagnosed here
(though the original 'test_must_fail' didn't bother diagnosing that
problem so it may be overkill and outside the scope of this series to
do so here).

> @@ -817,6 +842,15 @@ list_contains () {
> +# 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
> +#
> +# Just use '!' instead.

I find this somewhat ambiguous; it's not clear at first sight what I'm
supposed to do with '!'. t/README is slightly clearer by saying "use
'! cmd' instead". It might be even clearer to spell it out explicitly
with an example:

    Instead use '!':

        ! grep pattern output

Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage

[Eric Sunshine]

On Tue, Jun 30, 2020 at 4:56 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:
> > +                       case "$1" in *?=*) ;; *) break ;; esac
>
> Isn't '*?=*' the same as '?=', or am I misunderstanding the intention?

Nevermind. My brain blipped out while reading that. Obviously '?=' is
not the same as '*?=*' here.

The rest of the review comments still stand (I think).

Re: [PATCH 5/5] test-lib-functions: restrict test_must_fail usage

[Denton Liu]

Hi Eric,

On Tue, Jun 30, 2020 at 04:56:22PM -0400, Eric Sunshine wrote:
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > +# 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 () {
> > +       while test "$1" = "env"
> 
> I was surprised to see a 'while' loop for stripping 'env'. Did you
> actually run across cases in the test suite in which 'env' was
> invoking 'env'? If so, were such cases legitimate (as opposed to
> accidental)? Perhaps the commit message or an in-code comment could
> help readers understand why it needs to strip multiple 'env's.

There are no cases of nested envs. When I was writing this, I wanted to
be as permissive as possible in case someone wrote something like this.
Now that you bring it up, though, I don't think it makes sense to
support this use case because I can't come up with any legitimate reason
to have nested envs. (If something comes up in the future, we can
reinstate this behaviour.)

> > +       do
> > +               shift
> > +               while test $# -gt 0
> > +               do
> > +                       case "$1" in *?=*) ;; *) break ;; esac
> > +                       shift
> > +               done
> > +       done

> Would it make sense to error out if "$1" has no value? That is, if the
> author wrote:
> 
>     test_must_fail &&
> 
> or
> 
>     test_must_fail env foo=bar &&
> 
> then that surely is a programmer error, which could be diagnosed here
> (though the original 'test_must_fail' didn't bother diagnosing that
> problem so it may be overkill and outside the scope of this series to
> do so here).

This is caught by the 

	case "$1" in
	git|__git*|test-tool|test-svn-fe|test_terminal)
		return 0
		;;
	*)
		return 1
		;;
	esac

part. It only emits

	test_must_fail: only 'git' is allowed:

if this happens but it's probably fine as I don't think we should do
much hand-holding for this case.

And I'll use the remainder of your suggestions.

Thanks,

Denton

[PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Denton Liu]

The overall scope of these patches is to replace inappropriate uses of
test_must_fail. IOW, we should only allow test_must_fail to run on `git`
and `test-tool`. Ultimately, we will conclude by making test_must_fail
error out on non-git commands. An advance view of the final series can
be found here[1].

This is the sixth and final(!) part. It cleans up instances of
`test_must_fail` that were introduced since the effort began. In
addition, it finally flips the switch and makes test_must_fail only
allow a whitelist of commands.

This series is based on the merge of 'master' and
'dl/test-must-fail-fixes-5'. In addition, this series was tested by
merging with 'seen~1' (to ignore the reftable failures) to ensure no
in-flight topics will require more changes.

The first part can be found here[2]. The second part can be found
here[3]. The third part can be found here[4]. The fourth part can be
found here[5]. The fifth part can be found here[6].

Changes since v1:

* Add a code comment to force_color()

* Do not allow nested env's in test_must_fail_acceptable()

* Clean up the env-processing case code

* Give an example on how to use `!`.

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/
[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/
[6]: https://lore.kernel.org/git/cover.1588162842.git.liu.denton@gmail.com/

Denton Liu (5):
  t3701: stop using `env` in force_color()
  t5324: reorder `run_with_limited_open_files test_might_fail`
  t7107: don't use test_must_fail()
  t9834: remove use of `test_might_fail p4`
  test-lib-functions: restrict test_must_fail usage

 t/t0000-basic.sh               | 18 +++++++++++++
 t/t3701-add-interactive.sh     | 13 ++++++++--
 t/t5324-split-commit-graph.sh  |  2 +-
 t/t7107-reset-pathspec-file.sh |  9 +++++--
 t/t9834-git-p4-file-dir-bug.sh |  2 +-
 t/test-lib-functions.sh        | 47 ++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  67d5b93fda ! 1:  654c864691 t3701: stop using `env` in force_color()
    @@ t/t3701-add-interactive.sh: diff_cmp () {
      
      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 &&
    -+		export GIT_PAGER_IN_USE &&
     +		TERM=vt100 &&
    -+		export TERM &&
    ++		export GIT_PAGER_IN_USE TERM &&
     +		"$@"
     +	)
      }
2:  aae48a89e5 = 2:  9ba997f7c1 t5324: reorder `run_with_limited_open_files test_might_fail`
3:  74bc29b18b = 3:  eb2052bf64 t7107: don't use test_must_fail()
4:  1287798e69 = 4:  92d3b38428 t9834: remove use of `test_might_fail p4`
5:  01e29450fe ! 5:  3ebbda6c57 test-lib-functions: restrict test_must_fail usage
    @@ Commit message
         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 case).
    +    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.
    @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel
     +'
     +
     +test_expect_success 'test_must_fail on a failing git command with env' '
    -+	test_must_fail env var1=a var2=b env var3=c git notacommand
    ++	test_must_fail env var1=a var2=b git notacommand
     +'
     +
     +test_expect_success 'test_must_fail rejects a non-git command' '
    @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel
     +'
     +
     +test_expect_success 'test_must_fail rejects a non-git command with env' '
    -+	! test_must_fail env var1=a var2=b env var3=c grep ^$ notafile 2>err &&
    ++	! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&
     +	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
     +'
     +
    @@ t/test-lib-functions.sh: list_contains () {
     +# and its corresponding variable settings will be stripped before we
     +# test the command being run.
     +test_must_fail_acceptable () {
    -+	while test "$1" = "env"
    -+	do
    ++	if test "$1" = "env"
    ++	then
     +		shift
     +		while test $# -gt 0
     +		do
    -+			case "$1" in *?=*) ;; *) break ;; esac
    -+			shift
    ++			case "$1" in
    ++			*?=*)
    ++				shift
    ++				;;
    ++			*)
    ++				break
    ++				;;
    ++			esac
     +		done
    -+	done
    ++	fi
     +
     +	case "$1" in
     +	git|__git*|test-tool|test-svn-fe|test_terminal)
    @@ t/test-lib-functions.sh: list_contains () {
     +#
     +#    test_must_fail grep pattern output
     +#
    -+# Just use '!' instead.
    ++# Instead use '!':
    ++#
    ++#    ! grep pattern output
      
      test_must_fail () {
      	case "$1" in
    @@ t/test-lib-functions.sh: test_must_fail () {
      	esac
     +	if ! test_must_fail_acceptable "$@"
     +	then
    -+	    echo >&7 "test_must_fail: only 'git' is allowed: $*"
    -+	    return 1
    ++		echo >&7 "test_must_fail: only 'git' is allowed: $*"
    ++		return 1
     +	fi
      	"$@" 2>&7
      	exit_code=$?
-- 
2.27.0.383.g050319c2ae

[PATCH v2 1/5] t3701: stop using `env` in force_color()

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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 <y
+	force_color test_must_fail git add -p <y
 '
 
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
-- 
2.27.0.383.g050319c2ae

[PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail`

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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
 	)
-- 
2.27.0.383.g050319c2ae

[PATCH v2 3/5] t7107: don't use test_must_fail()

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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' '
-- 
2.27.0.383.g050319c2ae

[PATCH v2 4/5] t9834: remove use of `test_might_fail p4`

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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' '
-- 
2.27.0.383.g050319c2ae

[PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage

[Denton Liu]

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

[RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Denton Liu]

The overall scope of these patches is to replace inappropriate uses of
test_must_fail. IOW, we should only allow test_must_fail to run on `git`
and `test-tool`. Ultimately, we will conclude by making test_must_fail
error out on non-git commands. An advance view of the final series can
be found here[1].

This is the sixth and final(!) part. It cleans up instances of
`test_must_fail` that were introduced since the effort began. In
addition, it finally flips the switch and makes test_must_fail only
allow a whitelist of commands.

This series is based on the merge of 'master' and
'dl/test-must-fail-fixes-5'. In addition, this series was tested by
merging with 'seen~1' (to ignore the reftable failures) to ensure no
in-flight topics will require more changes.

The first part can be found here[2]. The second part can be found
here[3]. The third part can be found here[4]. The fourth part can be
found here[5]. The fifth part can be found here[6].

Changes since v1:

* Add a code comment to force_color()

* Do not allow nested env's in test_must_fail_acceptable()

* Clean up the env-processing case code

* Give an example on how to use `!`.

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2
[2]: https://lore.kernel.org/git/cover.1576583819.git.liu.denton@gmail.com/
[3]: https://lore.kernel.org/git/cover.1577454401.git.liu.denton@gmail.com/
[4]: https://lore.kernel.org/git/cover.1585209554.git.liu.denton@gmail.com/
[5]: https://lore.kernel.org/git/cover.1587372771.git.liu.denton@gmail.com/
[6]: https://lore.kernel.org/git/cover.1588162842.git.liu.denton@gmail.com/

Denton Liu (5):
  t3701: stop using `env` in force_color()
  t5324: reorder `run_with_limited_open_files test_might_fail`
  t7107: don't use test_must_fail()
  t9834: remove use of `test_might_fail p4`
  test-lib-functions: restrict test_must_fail usage

 t/t0000-basic.sh               | 18 +++++++++++++
 t/t3701-add-interactive.sh     | 13 ++++++++--
 t/t5324-split-commit-graph.sh  |  2 +-
 t/t7107-reset-pathspec-file.sh |  9 +++++--
 t/t9834-git-p4-file-dir-bug.sh |  2 +-
 t/test-lib-functions.sh        | 47 ++++++++++++++++++++++++++++++++++
 6 files changed, 85 insertions(+), 6 deletions(-)

Range-diff against v1:
1:  67d5b93fda ! 1:  654c864691 t3701: stop using `env` in force_color()
    @@ t/t3701-add-interactive.sh: diff_cmp () {
      
      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 &&
    -+		export GIT_PAGER_IN_USE &&
     +		TERM=vt100 &&
    -+		export TERM &&
    ++		export GIT_PAGER_IN_USE TERM &&
     +		"$@"
     +	)
      }
2:  aae48a89e5 = 2:  9ba997f7c1 t5324: reorder `run_with_limited_open_files test_might_fail`
3:  74bc29b18b = 3:  eb2052bf64 t7107: don't use test_must_fail()
4:  1287798e69 = 4:  92d3b38428 t9834: remove use of `test_might_fail p4`
5:  01e29450fe ! 5:  3ebbda6c57 test-lib-functions: restrict test_must_fail usage
    @@ Commit message
         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 case).
    +    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.
    @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel
     +'
     +
     +test_expect_success 'test_must_fail on a failing git command with env' '
    -+	test_must_fail env var1=a var2=b env var3=c git notacommand
    ++	test_must_fail env var1=a var2=b git notacommand
     +'
     +
     +test_expect_success 'test_must_fail rejects a non-git command' '
    @@ t/t0000-basic.sh: test_expect_success 'very long name in the index handled sanel
     +'
     +
     +test_expect_success 'test_must_fail rejects a non-git command with env' '
    -+	! test_must_fail env var1=a var2=b env var3=c grep ^$ notafile 2>err &&
    ++	! test_must_fail env var1=a var2=b grep ^$ notafile 2>err &&
     +	grep -F "test_must_fail: only '"'"'git'"'"' is allowed" err
     +'
     +
    @@ t/test-lib-functions.sh: list_contains () {
     +# and its corresponding variable settings will be stripped before we
     +# test the command being run.
     +test_must_fail_acceptable () {
    -+	while test "$1" = "env"
    -+	do
    ++	if test "$1" = "env"
    ++	then
     +		shift
     +		while test $# -gt 0
     +		do
    -+			case "$1" in *?=*) ;; *) break ;; esac
    -+			shift
    ++			case "$1" in
    ++			*?=*)
    ++				shift
    ++				;;
    ++			*)
    ++				break
    ++				;;
    ++			esac
     +		done
    -+	done
    ++	fi
     +
     +	case "$1" in
     +	git|__git*|test-tool|test-svn-fe|test_terminal)
    @@ t/test-lib-functions.sh: list_contains () {
     +#
     +#    test_must_fail grep pattern output
     +#
    -+# Just use '!' instead.
    ++# Instead use '!':
    ++#
    ++#    ! grep pattern output
      
      test_must_fail () {
      	case "$1" in
    @@ t/test-lib-functions.sh: test_must_fail () {
      	esac
     +	if ! test_must_fail_acceptable "$@"
     +	then
    -+	    echo >&7 "test_must_fail: only 'git' is allowed: $*"
    -+	    return 1
    ++		echo >&7 "test_must_fail: only 'git' is allowed: $*"
    ++		return 1
     +	fi
      	"$@" 2>&7
      	exit_code=$?
-- 
2.27.0.383.g050319c2ae

[RESEND PATCH v2 1/5] t3701: stop using `env` in force_color()

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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 <y
+	force_color test_must_fail git add -p <y
 '
 
 test_expect_success 'diff.algorithm is passed to `git diff-files`' '
-- 
2.27.0.383.g050319c2ae

[RESEND PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail`

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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
 	)
-- 
2.27.0.383.g050319c2ae

[RESEND PATCH v2 3/5] t7107: don't use test_must_fail()

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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' '
-- 
2.27.0.383.g050319c2ae

[RESEND PATCH v2 4/5] t9834: remove use of `test_might_fail p4`

[Denton Liu]

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 <liu.denton@gmail.com>
---
 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' '
-- 
2.27.0.383.g050319c2ae

[RESEND PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage

[Denton Liu]

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

Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Junio C Hamano]

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

> The overall scope of these patches is to replace inappropriate uses of
> test_must_fail. IOW, we should only allow test_must_fail to run on `git`
> and `test-tool`. Ultimately, we will conclude by making test_must_fail
> error out on non-git commands. An advance view of the final series can
> be found here[1].
>
> This is the sixth and final(!) part. It cleans up instances of
> `test_must_fail` that were introduced since the effort began. In
> addition, it finally flips the switch and makes test_must_fail only
> allow a whitelist of commands.
>
> This series is based on the merge of 'master' and
> 'dl/test-must-fail-fixes-5'. In addition, this series was tested by
> merging with 'seen~1' (to ignore the reftable failures) to ensure no
> in-flight topics will require more changes.
>
> The first part can be found here[2]. The second part can be found
> here[3]. The third part can be found here[4]. The fourth part can be
> found here[5]. The fifth part can be found here[6].
>
> Changes since v1:
>
> * Add a code comment to force_color()
>
> * Do not allow nested env's in test_must_fail_acceptable()
>
> * Clean up the env-processing case code
>
> * Give an example on how to use `!`.

Thanks for a resend.  Now part #5 is in 'master', I can queue these
directly on top.

Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> Changes since v1:
>>
>> * Add a code comment to force_color()
>>
>> * Do not allow nested env's in test_must_fail_acceptable()
>>
>> * Clean up the env-processing case code
>>
>> * Give an example on how to use `!`.
>
> Thanks for a resend.  Now part #5 is in 'master', I can queue these
> directly on top.

It seems that the patch series lacks coverage for t9400 where we
have

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
	)
'

which obviously needs to be converted before we declare that it is a
hard error to feed a non-git command to test_must_fail.

[PATCH] t9400: don't use test_must_fail with cvs

[Denton Liu]

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 <liu.denton@gmail.com>
---
Hi Junio,

Thanks for pointing this out. It seems like the CI systems we use don't
cover cvs. Is this something that we want to address or is it fine to
leave it as a blindspot since no one really uses cvs anymore?

 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
 	)
 '
 
-- 
2.27.0.383.g050319c2ae

Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>> Changes since v1:
>>>
>>> * Add a code comment to force_color()
>>>
>>> * Do not allow nested env's in test_must_fail_acceptable()
>>>
>>> * Clean up the env-processing case code
>>>
>>> * Give an example on how to use `!`.
>>
>> Thanks for a resend.  Now part #5 is in 'master', I can queue these
>> directly on top.
>
> It seems that the patch series lacks coverage for t9400 where we
> have
>
> 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
> 	)
> '
>
> which obviously needs to be converted before we declare that it is a
> hard error to feed a non-git command to test_must_fail.

For today's integration cycle, I added a fix-up at the tip of the topic

https://github.com/git/git/commit/dde09ce2b7dd62eafda6339c1c31ccfeb0f39cee

Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Denton Liu]

Hi Junio,

On Tue, Jul 07, 2020 at 03:11:33PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > It seems that the patch series lacks coverage for t9400 where we
> > have
> >
> > 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
> > 	)
> > '
> >
> > which obviously needs to be converted before we declare that it is a
> > hard error to feed a non-git command to test_must_fail.
> 
> For today's integration cycle, I added a fix-up at the tip of the topic
> 
> https://github.com/git/git/commit/dde09ce2b7dd62eafda6339c1c31ccfeb0f39cee

Thanks. For the final version of this series, we should either queue
your patch or the one I just sent[0] just after the patch for t7107 so
that it comes before the we flip the switch on test_must_fail and also
so that the patches show up in increasing numerical order.

[0]: https://lore.kernel.org/git/4ca5e1f9c06ed509fc3165c550d0d665dd5b69c0.1594159495.git.liu.denton@gmail.com/

Re: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)

[Junio C Hamano]

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

> Thanks. For the final version of this series, we should either queue
> your patch or the one I just sent[0] just after the patch for t7107 so
> that it comes before the we flip the switch on test_must_fail and also
> so that the patches show up in increasing numerical order.

Ah, our mails crossed.  Surely the fix to cvs test must come before
the final step that tightens the test_must_fail helper.