Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Denton Liu <liu.denton@gmail.com>
To: Git Mailing List <git@vger.kernel.org>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)
Date: Wed,  1 Jul 2020 00:27:08 -0400
Message-ID: <cover.1593576601.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1593529394.git.liu.denton@gmail.com>

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


  parent reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30 15:03 [PATCH " Denton Liu
2020-06-30 15:03 ` [PATCH 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-06-30 21:48   ` Eric Sunshine
2020-06-30 22:06     ` Junio C Hamano
2020-06-30 15:03 ` [PATCH 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-06-30 15:03 ` [PATCH 3/5] t7107: don't use test_must_fail() Denton Liu
2020-06-30 15:03 ` [PATCH 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-06-30 15:03 ` [PATCH 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-06-30 20:56   ` Eric Sunshine
2020-06-30 21:59     ` Eric Sunshine
2020-06-30 23:39     ` Denton Liu
2020-07-01  4:27 ` Denton Liu [this message]
2020-07-01  4:27   ` [PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-01  4:27   ` [PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-01  4:27   ` [PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-01  4:27   ` [PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-01  4:27   ` [PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu
2020-07-07  6:04   ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 1/5] t3701: stop using `env` in force_color() Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 2/5] t5324: reorder `run_with_limited_open_files test_might_fail` Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 3/5] t7107: don't use test_must_fail() Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 4/5] t9834: remove use of `test_might_fail p4` Denton Liu
2020-07-07  6:04     ` [RESEND PATCH v2 5/5] test-lib-functions: restrict test_must_fail usage Denton Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cover.1593576601.git.liu.denton@gmail.com \
    --to=liu.denton@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git