patch, 5 partst3701: stop using `env` in force_color()

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

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

On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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

Probably.

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

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

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

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(+)

On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote:

s/case/cases/

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.

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.

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

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

On Tue, Jun 30, 2020 at 4:56 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

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

Hi Eric,

On Tue, Jun 30, 2020 at 04:56:22PM -0400, Eric Sunshine wrote:

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

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

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=$?

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

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

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(+)

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

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

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=$?

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

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(+)

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

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

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

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

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

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.

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

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

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

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

Hi Junio,

On Tue, Jul 07, 2020 at 03:11:33PM -0700, Junio C Hamano wrote:

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/

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

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