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: [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6)
Date: Tue, 7 Jul 2020 02:04:33 -0400 [thread overview]
Message-ID: <cover.1594101831.git.liu.denton@gmail.com> (raw)
In-Reply-To: <cover.1593576601.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
next prev parent reply other threads:[~2020-07-07 6:05 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-30 15:03 [PATCH 0/5] t: replace incorrect test_must_fail usage (part 6) 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 ` [PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Denton Liu
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 ` Denton Liu [this message]
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
2020-07-07 20:08 ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Junio C Hamano
2020-07-07 20:57 ` Junio C Hamano
2020-07-07 22:08 ` [PATCH] t9400: don't use test_must_fail with cvs Denton Liu
2020-07-07 22:11 ` [RESEND PATCH v2 0/5] t: replace incorrect test_must_fail usage (part 6) Junio C Hamano
2020-07-07 22:21 ` Denton Liu
2020-07-07 22:29 ` Junio C Hamano
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover.1594101831.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).