git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1)
@ 2019-12-17 12:01 Denton Liu
  2019-12-17 12:01 ` [PATCH 01/15] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
                   ` (16 more replies)
  0 siblings, 17 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

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 first part. It focuses on t[01]*.sh and also t/lib-git-p4.

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2

Denton Liu (15):
  test-lib-functions: introduce test_non_git_might_fail()
  t/lib-git-p4: use test_path_is_missing()
  t0000: replace test_must_fail with ! for run_sub_test_lib_test()
  t0003: use named parameters in attr_check()
  t0003: use test_must_be_empty()
  t0003: don't use `test_must_fail attr_check`
  t0020: drop redirections to /dev/null
  t0020: s/test_must_fail has_cr/! has_cr/
  t0020: use ! check_packed_refs_marked
  t1306: convert `test_might_fail rm` to `rm -f`
  t1307: reorder `nongit test_must_fail`
  t1409: let sed open its own files
  t1409: use test_path_is_missing()
  t1501: remove use of `test_might_fail cp`
  t1507: teach full_name() to accept `!` arg

 t/lib-git-p4.sh               |  2 +-
 t/t0000-basic.sh              | 14 +++++-----
 t/t0003-attributes.sh         | 51 ++++++++++++++++++-----------------
 t/t0020-crlf.sh               | 20 +++++++-------
 t/t1306-xdg-files.sh          |  8 +++---
 t/t1307-config-blob.sh        |  2 +-
 t/t1409-avoid-packing-refs.sh | 16 +++++------
 t/t1501-work-tree.sh          |  2 +-
 t/t1507-rev-parse-upstream.sh | 14 +++++++---
 t/test-lib-functions.sh       |  9 +++++++
 10 files changed, 77 insertions(+), 61 deletions(-)

-- 
2.24.0.627.geba02921db


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH 01/15] test-lib-functions: introduce test_non_git_might_fail()
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 12:01 ` [PATCH 02/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

In a future commit, we will be preventing the use of the
test_must_fail()-family of functions (including test_might_fail()) on
non-git comands. To prep for this, introduce the
test_non_git_might_fail() function which is used to replace non-git
invocations of test_might_fail().

The test_non_git_might_fail() function is a lightweight replacement,
always masking the return status of the command and returning a
non-error exit code. Unlike test_might_fail(), it does not check for
abnormal exit conditions such as a segv. This is because we are not in
the business of checking the sanity of the external environment and we
can assume that it works properly.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/test-lib-functions.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 284c52d076..61d27f1ec6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -891,6 +891,15 @@ test_expect_code () {
 	return 1
 } 7>&2 2>&4
 
+# Similar to test_might_fail, but much simpler. This is intended for use
+# with non-git commands that we can assume will work sanely so we don't
+# need to check for conditions such as a segv.
+
+test_non_git_might_fail () {
+	"$@" 2>&7
+	return 0
+}
+
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
 #
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 02/15] t/lib-git-p4: use test_path_is_missing()
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
  2019-12-17 12:01 ` [PATCH 01/15] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 12:01 ` [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test() Denton Liu
                   ` (14 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

Previously, cleanup_git() would use `test_must_fail test -d` to ensure
that the directory is removed. However, test_must_fail should only be
used for git commands. Use test_path_is_missing() instead to check that
the directory has been removed.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 547b9f88e1..5aff2abe8b 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -175,7 +175,7 @@ stop_and_cleanup_p4d () {
 
 cleanup_git () {
 	retry_until_success rm -r "$git"
-	test_must_fail test -d "$git" &&
+	test_path_is_missing "$git" &&
 	retry_until_success mkdir "$git"
 }
 
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test()
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
  2019-12-17 12:01 ` [PATCH 01/15] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
  2019-12-17 12:01 ` [PATCH 02/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 20:23   ` Johannes Sixt
  2019-12-17 12:01 ` [PATCH 04/15] t0003: use named parameters in attr_check() Denton Liu
                   ` (13 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. We use
test_must_fail to test run_sub_test_lib_test() but that function does
not invoke any git commands internally. Replace these instances of
`test_must_fail` with `!`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0000-basic.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 8a81a249d0..d60ad4b78b 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' "
 "
 
 test_expect_success 'pretend we have a partially passing test suite' "
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		partial-pass '2/3 tests passing' <<-\\EOF &&
 	test_expect_success 'passing test #1' 'true'
 	test_expect_success 'failing test #2' 'false'
@@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su
 "
 
 test_expect_success 'pretend we have a pass, fail, and known breakage' "
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		mixed-results1 'mixed results #1' <<-\\EOF &&
 	test_expect_success 'passing test' 'true'
 	test_expect_success 'failing test' 'false'
@@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' "
 "
 
 test_expect_success 'pretend we have a mix of all possible results' "
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		mixed-results2 'mixed results #2' <<-\\EOF &&
 	test_expect_success 'passing test' 'true'
 	test_expect_success 'passing test' 'true'
@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' "
 "
 
 test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		t1234-verbose "test verbose" --verbose <<-\EOF &&
 	test_expect_success "passing test" true
 	test_expect_success "test with output" "echo foo"
@@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
 '
 
 test_expect_success 'test --verbose-only' '
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		t2345-verbose-only-2 "test verbose-only=2" \
 		--verbose-only=2 <<-\EOF &&
 	test_expect_success "passing test" true
@@ -834,7 +834,7 @@ then
 fi
 
 test_expect_success 'tests clean up even on failures' "
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		touch clean-after-failure &&
@@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' "
 "
 
 test_expect_success 'test_atexit is run' "
-	test_must_fail run_sub_test_lib_test \
+	! run_sub_test_lib_test \
 		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		> ../../clean-atexit &&
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 04/15] t0003: use named parameters in attr_check()
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (2 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test() Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 18:35   ` Junio C Hamano
  2019-12-17 18:43   ` Eric Sunshine
  2019-12-17 12:01 ` [PATCH 05/15] t0003: use test_must_be_empty() Denton Liu
                   ` (12 subsequent siblings)
  16 siblings, 2 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

We had named the parameters in attr_check() but $2 was being used
instead of $expect. Make all variable accesses in attr_check() use named
variables instead of numbered arguments for clarity.

While we're at it, add variable assignments to the &&-chain. These
aren't ever expected to fail but for stylistic purposes, include them
anyway for stylistic purposes.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 71e63d8b50..c47d4cfbcd 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -5,19 +5,20 @@ test_description=gitattributes
 . ./test-lib.sh
 
 attr_check () {
-	path="$1" expect="$2"
+	path="$1" &&
+	expect="$2" &&
+	git_opts="$3" &&
 
-	git $3 check-attr test -- "$path" >actual 2>err &&
-	echo "$path: test: $2" >expect &&
+	git $git_opts check-attr test -- "$path" >actual 2>err &&
+	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual &&
 	test_line_count = 0 err
 }
 
 attr_check_quote () {
-
-	path="$1"
-	quoted_path="$2"
-	expect="$3"
+	path="$1" &&
+	quoted_path="$2" &&
+	expect="$3" &&
 
 	git check-attr test -- "$path" >actual &&
 	echo "\"$quoted_path\": test: $expect" >expect &&
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 05/15] t0003: use test_must_be_empty()
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (3 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 04/15] t0003: use named parameters in attr_check() Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 12:01 ` [PATCH 06/15] t0003: don't use `test_must_fail attr_check` Denton Liu
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

In several places, we used `test_line_count = 0` to check for an empty
file. Although this is correct, it's overkill. Use test_must_be_empty()
instead because it's more suited for this purpose.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index c47d4cfbcd..53a730e2ee 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -12,7 +12,7 @@ attr_check () {
 	git $git_opts check-attr test -- "$path" >actual 2>err &&
 	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 }
 
 attr_check_quote () {
@@ -245,7 +245,7 @@ EOF
 	git check-attr foo -- "a/b/f" >>actual 2>>err &&
 	git check-attr foo -- "a/b/c/f" >>actual 2>>err &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success '"**" with no slashes test' '
@@ -266,7 +266,7 @@ EOF
 	git check-attr foo -- "a/b/f" >>actual 2>>err &&
 	git check-attr foo -- "a/b/c/f" >>actual 2>>err &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success 'using --git-dir and --work-tree' '
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 06/15] t0003: don't use `test_must_fail attr_check`
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (4 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 05/15] t0003: use test_must_be_empty() Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 18:44   ` Junio C Hamano
  2019-12-17 12:01 ` [PATCH 07/15] t0020: drop redirections to /dev/null Denton Liu
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

In an effort to remove test_must_fail for all invocations not related to
git or test-tool, replace invocations of `test_must_fail attr_check`
with a plain attr_check call with the $expect argument set to the
actual value output by git.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 53a730e2ee..8d4343afdb 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -28,7 +28,7 @@ attr_check_quote () {
 
 test_expect_success 'open-quoted pathname' '
 	echo "\"a test=a" >.gitattributes &&
-	test_must_fail attr_check a a
+	attr_check a unspecified
 '
 
 
@@ -113,20 +113,20 @@ test_expect_success 'attribute test' '
 
 test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '
 
-	test_must_fail attr_check F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/c/F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/G a/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/B/g a/b/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/G a/b/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/H a/b/h "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=0" &&
-	test_must_fail attr_check oNoFf unset "-c core.ignorecase=0" &&
-	test_must_fail attr_check oFfOn set "-c core.ignorecase=0" &&
+	attr_check F unspecified "-c core.ignorecase=0" &&
+	attr_check a/F unspecified "-c core.ignorecase=0" &&
+	attr_check a/c/F unspecified "-c core.ignorecase=0" &&
+	attr_check a/G unspecified "-c core.ignorecase=0" &&
+	attr_check a/B/g a/g "-c core.ignorecase=0" &&
+	attr_check a/b/G unspecified "-c core.ignorecase=0" &&
+	attr_check a/b/H unspecified "-c core.ignorecase=0" &&
+	attr_check a/b/D/g a/g "-c core.ignorecase=0" &&
+	attr_check oNoFf unspecified "-c core.ignorecase=0" &&
+	attr_check oFfOn unspecified "-c core.ignorecase=0" &&
 	attr_check NO unspecified "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/b/D/NO unspecified "-c core.ignorecase=0" &&
 	attr_check a/b/d/YES a/b/d/* "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/E/f "A/e/F" "-c core.ignorecase=0"
+	attr_check a/E/f f "-c core.ignorecase=0"
 
 '
 
@@ -150,8 +150,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase
 '
 
 test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '
-	test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" &&
-	test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/B/D/g a/g "-c core.ignorecase=0" &&
+	attr_check A/B/D/NO unspecified "-c core.ignorecase=0" &&
 	attr_check A/b/h a/b/h "-c core.ignorecase=1" &&
 	attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=1" &&
 	attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=1"
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 07/15] t0020: drop redirections to /dev/null
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (5 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 06/15] t0003: don't use `test_must_fail attr_check` Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 19:32   ` Eric Sunshine
  2019-12-17 12:01 ` [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/ Denton Liu
                   ` (9 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

Since output is silenced when running without `-v` and debugging output
is useful with `-v`, remove redirections to /dev/null as it is not
useful.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0020-crlf.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 854da0ae16..8281babde0 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -5,7 +5,7 @@ test_description='CRLF conversion'
 . ./test-lib.sh
 
 has_cr() {
-	tr '\015' Q <"$1" | grep Q >/dev/null
+	tr '\015' Q <"$1" | grep Q
 }
 
 # add or remove CRs to disk file in-place
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (6 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 07/15] t0020: drop redirections to /dev/null Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 19:36   ` Eric Sunshine
  2019-12-17 12:01 ` [PATCH 09/15] t0020: use ! check_packed_refs_marked Denton Liu
                   ` (8 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since has_cr() just
wraps a tr and grep pipeline, replace `test_must_fail has_cr` with
`! has_cr`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0020-crlf.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 8281babde0..4940e05a2e 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' '
 	rm -f tmp one dir/two three &&
 	git config core.autocrlf input &&
 	git read-tree --reset -u HEAD &&
-	test_must_fail has_cr one &&
-	test_must_fail has_cr dir/two &&
+	! has_cr one &&
+	! has_cr dir/two &&
 	git update-index -- one dir/two &&
 	test "$one" = $(git hash-object --stdin <one) &&
 	test "$two" = $(git hash-object --stdin <dir/two) &&
@@ -237,9 +237,9 @@ test_expect_success '.gitattributes says two is binary' '
 	git config core.autocrlf true &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr dir/two &&
+	! has_cr dir/two &&
 	verbose has_cr one &&
-	test_must_fail has_cr three
+	! has_cr three
 '
 
 test_expect_success '.gitattributes says two is input' '
@@ -248,7 +248,7 @@ test_expect_success '.gitattributes says two is input' '
 	echo "two crlf=input" >.gitattributes &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr dir/two
+	! has_cr dir/two
 '
 
 test_expect_success '.gitattributes says two and three are text' '
@@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' '
 	rm -rf tmp one dir .gitattributes patch.file three &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' '
 	git read-tree --reset HEAD &&
 	git checkout-index -f -q -u -a &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' '
 	git checkout-index -u .gitattributes &&
 	git checkout-index -u one dir/two three &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	git checkout-index -u one dir/two three &&
 	git checkout-index -u .gitattributes &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 09/15] t0020: use ! check_packed_refs_marked
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (7 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/ Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 12:01 ` [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since
check_packed_refs_marked() just wraps a grep invocation, replace
`test_must_fail check_packed_refs_marked` with
`! check_packed_refs_marked`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index e5cb8a252d..c46848eb8e 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' '
 	git for-each-ref >actual &&
 	test_cmp expected actual &&
 	git pack-refs --all &&
-	test_must_fail check_packed_refs_marked &&
+	! check_packed_refs_marked &&
 	git for-each-ref >actual2 &&
 	test_cmp expected actual2
 '
@@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' '
 	git pack-refs --all &&
 	mark_packed_refs &&
 	git update-ref -d refs/heads/packed-delete &&
-	test_must_fail check_packed_refs_marked
+	! check_packed_refs_marked
 '
 
 test_expect_success 'leave packed-refs untouched on update of loose' '
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f`
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (8 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 09/15] t0020: use ! check_packed_refs_marked Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 18:55   ` Junio C Hamano
  2019-12-17 12:01 ` [PATCH 11/15] t1307: reorder `nongit test_must_fail` Denton Liu
                   ` (6 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

The test_must_fail() family of functions (including test_might_fail())
should only be used on git commands. Replace `test_might_fail rm` with
`rm -f` so that we don't use `test_might_fail` on a non-git command.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1306-xdg-files.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 21e139a313..dd87b43be1 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files'
 
 
 test_expect_success 'Checking attributes in a non-XDG global attributes file' '
-	test_might_fail rm .gitattributes &&
+	rm -f .gitattributes &&
 	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
 	git config core.attributesfile "$HOME"/my_gitattributes &&
 	echo "f: attr_f: test" >expected &&
@@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
 	mkdir -p "$HOME"/.config/git &&
 	>"$HOME"/.config/git/config &&
-	test_might_fail rm "$HOME"/.gitconfig &&
+	rm -f "$HOME"/.gitconfig &&
 	git config --global user.name "write_config" &&
 	echo "[user]" >expected &&
 	echo "	name = write_config" >>expected &&
@@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
 
 
 test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
-	test_might_fail rm "$HOME"/.gitconfig &&
-	test_might_fail rm "$HOME"/.config/git/config &&
+	rm -f "$HOME"/.gitconfig &&
+	rm -f "$HOME"/.config/git/config &&
 	git config --global user.name "write_gitconfig" &&
 	echo "[user]" >expected &&
 	echo "	name = write_gitconfig" >>expected &&
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 11/15] t1307: reorder `nongit test_must_fail`
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (9 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 12:01 ` [PATCH 12/15] t1409: let sed open its own files Denton Liu
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

In the future, we plan on only allowing `test_must_fail` to work on a
restricted subset of commands, including `git`. Reorder the commands so
that `nongit` comes before `test_must_fail`. This way, `test_must_fail`
operates on a git command.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1307-config-blob.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
index 37dc689d8c..002e6d3388 100755
--- a/t/t1307-config-blob.sh
+++ b/t/t1307-config-blob.sh
@@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' '
 '
 
 test_expect_success 'config --blob outside of a repository is an error' '
-	test_must_fail nongit git config --blob=foo --list
+	nongit test_must_fail git config --blob=foo --list
 '
 
 test_done
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 12/15] t1409: let sed open its own files
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (10 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 11/15] t1307: reorder `nongit test_must_fail` Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 18:57   ` Junio C Hamano
  2019-12-17 12:01 ` [PATCH 13/15] t1409: use test_path_is_missing() Denton Liu
                   ` (4 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

In one case, we were using a redirection operator to feed input into
sed. However, since sed is capable of opening its own files, make sed
open its own files instead of redirecting input into it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index c46848eb8e..f74d890e82 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
 mark_packed_refs () {
-	sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new &&
+	sed -e "s/^\(#.*\)/\1 t1409 /" .git/packed-refs >.git/packed-refs.new &&
 	mv .git/packed-refs.new .git/packed-refs
 }
 
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 13/15] t1409: use test_path_is_missing()
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (11 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 12/15] t1409: let sed open its own files Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 19:01   ` Junio C Hamano
  2019-12-17 12:01 ` [PATCH 14/15] t1501: remove use of `test_might_fail cp` Denton Liu
                   ` (3 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test -f` with `test_path_is_missing` since we expect
these files to not exist.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f74d890e82..be12fb6350 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -27,15 +27,15 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'do not create packed-refs file gratuitously' '
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $A &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $B &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $C $B &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref -d refs/heads/foo &&
-	test_must_fail test -f .git/packed-refs
+	test_path_is_missing .git/packed-refs
 '
 
 test_expect_success 'check that marking the packed-refs file works' '
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 14/15] t1501: remove use of `test_might_fail cp`
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (12 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 13/15] t1409: use test_path_is_missing() Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 19:46   ` Eric Sunshine
  2019-12-17 12:01 ` [PATCH 15/15] t1507: teach full_name() to accept `!` arg Denton Liu
                   ` (2 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

The test_must_fail() family of functions (including test_might_fail())
should only be used on git commands. Replace test_might_fail() with
test_non_git_might_fail().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1501-work-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 3498d3d55e..067301a5ab 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
 	mkdir work &&
 	mkdir -p repo.git/repos/foo &&
 	cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
-	test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
+	test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
 	sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE
 '
 
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH 15/15] t1507: teach full_name() to accept `!` arg
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (13 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 14/15] t1501: remove use of `test_might_fail cp` Denton Liu
@ 2019-12-17 12:01 ` Denton Liu
  2019-12-17 19:54   ` Eric Sunshine
  2019-12-17 18:28 ` [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Junio C Hamano
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-17 12:01 UTC (permalink / raw)
  To: Git Mailing List

Before, we were running `test_must_fail full_name`. However,
`test_must_fail` should only be used on git commands. Teach full_name()
to accept `!` as a potential first argument which will prepend
`test_must_fail` to the enclosed git command. This increases the
granularity of test_must_fail since it no longer runs on the `cd` as
well.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 8b4cf8a6e3..9a941d68d8 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -29,8 +29,14 @@ test_expect_success 'setup' '
 '
 
 full_name () {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail=test_must_fail &&
+		shift
+	fi &&
 	(cd clone &&
-	 git rev-parse --symbolic-full-name "$@")
+	 $fail git rev-parse --symbolic-full-name "$@")
 }
 
 commit_subject () {
@@ -79,7 +85,7 @@ test_expect_success 'upstream of branch with @ at end' '
 '
 
 test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '
-	test_must_fail full_name refs/heads/my-side@{upstream}
+	full_name ! refs/heads/my-side@{upstream}
 '
 
 test_expect_success 'my-side@{u} resolves to correct commit' '
@@ -91,9 +97,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '
 '
 
 test_expect_success 'not-tracking@{u} fails' '
-	test_must_fail full_name non-tracking@{u} &&
+	full_name ! non-tracking@{u} &&
 	(cd clone && git checkout --no-track -b non-tracking) &&
-	test_must_fail full_name non-tracking@{u}
+	full_name ! non-tracking@{u}
 '
 
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
-- 
2.24.0.627.geba02921db


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* Re: [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1)
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (14 preceding siblings ...)
  2019-12-17 12:01 ` [PATCH 15/15] t1507: teach full_name() to accept `!` arg Denton Liu
@ 2019-12-17 18:28 ` Junio C Hamano
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
  16 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2019-12-17 18:28 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

Wow.  It is a bit sad to see that we had so many of them.


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 04/15] t0003: use named parameters in attr_check()
  2019-12-17 12:01 ` [PATCH 04/15] t0003: use named parameters in attr_check() Denton Liu
@ 2019-12-17 18:35   ` Junio C Hamano
  2019-12-17 18:43   ` Eric Sunshine
  1 sibling, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2019-12-17 18:35 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> We had named the parameters in attr_check() but $2 was being used
> instead of $expect. Make all variable accesses in attr_check() use named
> variables instead of numbered arguments for clarity.
>
> While we're at it, add variable assignments to the &&-chain. These
> aren't ever expected to fail but for stylistic purposes, include them
> anyway for stylistic purposes.

That is OK but do so like this instead for brevity?

 attr_check () {
- 	path="$1" expect="$2"
+ 	path="$1" expect="$2" git_opts="$3" &&

Giving (temporary) names to incoming parameters is so common that
such a convention to save vertical screen real estate may be worth
it.

It is sad that you can pass only $IFS-free things in $3 due to the
way it is used---perhaps it is sufficient for the purpose of tests
in this script, but it still looks sad (just a comment without
anything that is actionable).

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t0003-attributes.sh | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
> index 71e63d8b50..c47d4cfbcd 100755
> --- a/t/t0003-attributes.sh
> +++ b/t/t0003-attributes.sh
> @@ -5,19 +5,20 @@ test_description=gitattributes
>  . ./test-lib.sh
>  
>  attr_check () {
> -	path="$1" expect="$2"
> +	path="$1" &&
> +	expect="$2" &&
> +	git_opts="$3" &&
>  
> -	git $3 check-attr test -- "$path" >actual 2>err &&
> -	echo "$path: test: $2" >expect &&
> +	git $git_opts check-attr test -- "$path" >actual 2>err &&
> +	echo "$path: test: $expect" >expect &&
>  	test_cmp expect actual &&
>  	test_line_count = 0 err
>  }
>  
>  attr_check_quote () {
> -
> -	path="$1"
> -	quoted_path="$2"
> -	expect="$3"
> +	path="$1" &&
> +	quoted_path="$2" &&
> +	expect="$3" &&
>  
>  	git check-attr test -- "$path" >actual &&
>  	echo "\"$quoted_path\": test: $expect" >expect &&

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 04/15] t0003: use named parameters in attr_check()
  2019-12-17 12:01 ` [PATCH 04/15] t0003: use named parameters in attr_check() Denton Liu
  2019-12-17 18:35   ` Junio C Hamano
@ 2019-12-17 18:43   ` Eric Sunshine
  1 sibling, 0 replies; 64+ messages in thread
From: Eric Sunshine @ 2019-12-17 18:43 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

On Tue, Dec 17, 2019 at 7:01 AM Denton Liu <liu.denton@gmail.com> wrote:
> We had named the parameters in attr_check() but $2 was being used
> instead of $expect. Make all variable accesses in attr_check() use named
> variables instead of numbered arguments for clarity.
>
> While we're at it, add variable assignments to the &&-chain. These
> aren't ever expected to fail but for stylistic purposes, include them
> anyway for stylistic purposes.

As a justification, "stylistic purposes" isn't very strong. However, a
solid justification is that keeping the &&-chain intact even for these
assignments means that if someone comes along in the future and
inserts code _above_ the assignments, and if that code could fail in
some way, then the intact &&-chain will ensure that that failure is
noticed rather than going unnoticed.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 06/15] t0003: don't use `test_must_fail attr_check`
  2019-12-17 12:01 ` [PATCH 06/15] t0003: don't use `test_must_fail attr_check` Denton Liu
@ 2019-12-17 18:44   ` Junio C Hamano
  0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2019-12-17 18:44 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

>  test_expect_success 'open-quoted pathname' '
>  	echo "\"a test=a" >.gitattributes &&
> -	test_must_fail attr_check a a
> +	attr_check a unspecified
>  '

I offhand do not think of a reason why "git check-attr" must be
given freedom to choose from more than one answers (which is what
the original test that said "as long as it does not report 'a', we
are happy" allowed the command to), so it probably is OK to make the
tests more strict like this patch does.

Thanks.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f`
  2019-12-17 12:01 ` [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
@ 2019-12-17 18:55   ` Junio C Hamano
  0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2019-12-17 18:55 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> The test_must_fail() family of functions (including test_might_fail())
> should only be used on git commands. Replace `test_might_fail rm` with
> `rm -f` so that we don't use `test_might_fail` on a non-git command.

"rm -f X" can return non-zero status if "X" exists and cannot be
removed (e.g. "X" is a directory, X is in a directory you cannot
write to).  The only thing "-f" prevents the command from returning
non-zero status is when "X" does not exist.

That means that this change will change the behaviour.  Let's see if
it does in a good way or a bad way.

>  test_expect_success 'Checking attributes in a non-XDG global attributes file' '
> -	test_might_fail rm .gitattributes &&
> +	rm -f .gitattributes &&

This is so that leftover .gitattributes from previous tests will not
affect the outcome of this test.  If .gitattributes left by earlier
tests cannot be removed for whatever reason, we would want to know
about it, so changing to "rm -f" to make the tests more strict is a
good move.

>  	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
>  	git config core.attributesfile "$HOME"/my_gitattributes &&
>  	echo "f: attr_f: test" >expected &&
> @@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
>  test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
>  	mkdir -p "$HOME"/.config/git &&
>  	>"$HOME"/.config/git/config &&
> -	test_might_fail rm "$HOME"/.gitconfig &&
> +	rm -f "$HOME"/.gitconfig &&

Likewise.

>  	git config --global user.name "write_config" &&
>  	echo "[user]" >expected &&
>  	echo "	name = write_config" >>expected &&
> @@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
>  
>  
>  test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
> -	test_might_fail rm "$HOME"/.gitconfig &&
> -	test_might_fail rm "$HOME"/.config/git/config &&
> +	rm -f "$HOME"/.gitconfig &&
> +	rm -f "$HOME"/.config/git/config &&

Likewise, but I think it makes more sense to remove them with a
single invocation of "rm -f".

>  	git config --global user.name "write_gitconfig" &&
>  	echo "[user]" >expected &&
>  	echo "	name = write_gitconfig" >>expected &&

Thanks.  In short, I think all of the changes in the patch are good.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 12/15] t1409: let sed open its own files
  2019-12-17 12:01 ` [PATCH 12/15] t1409: let sed open its own files Denton Liu
@ 2019-12-17 18:57   ` Junio C Hamano
  0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2019-12-17 18:57 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> Subject: Re: [PATCH 12/15] t1409: let sed open its own files

s/files/input file/;

This is borderline Meh to me, but since the patch has already been
written, let's use it ;-)

> In one case, we were using a redirection operator to feed input into
> sed. However, since sed is capable of opening its own files, make sed
> open its own files instead of redirecting input into it.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t1409-avoid-packing-refs.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
> index c46848eb8e..f74d890e82 100755
> --- a/t/t1409-avoid-packing-refs.sh
> +++ b/t/t1409-avoid-packing-refs.sh
> @@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'
>  # shouldn't upset readers, and it should be omitted if the file is
>  # ever rewritten.
>  mark_packed_refs () {
> -	sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new &&
> +	sed -e "s/^\(#.*\)/\1 t1409 /" .git/packed-refs >.git/packed-refs.new &&
>  	mv .git/packed-refs.new .git/packed-refs
>  }

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 13/15] t1409: use test_path_is_missing()
  2019-12-17 12:01 ` [PATCH 13/15] t1409: use test_path_is_missing() Denton Liu
@ 2019-12-17 19:01   ` Junio C Hamano
  0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2019-12-17 19:01 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

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

> The test_must_fail() function should only be used for git commands since
> we should assume that external commands work sanely. Replace
> `test_must_fail test -f` with `test_path_is_missing` since we expect
> these files to not exist.

s/these files/these paths/; 

"! test -f FOO" allowed FOO to be a directory, but by using
test_path_is_missing we are no longer allowing FOO to exist in any
form.  I think the changes are correct, but rephrasing "files" to
"paths" would emphasize the reason why it is a good change, I would
say.

Thanks.


> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t1409-avoid-packing-refs.sh | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
> index f74d890e82..be12fb6350 100755
> --- a/t/t1409-avoid-packing-refs.sh
> +++ b/t/t1409-avoid-packing-refs.sh
> @@ -27,15 +27,15 @@ test_expect_success 'setup' '
>  '
>  
>  test_expect_success 'do not create packed-refs file gratuitously' '
> -	test_must_fail test -f .git/packed-refs &&
> +	test_path_is_missing .git/packed-refs &&
>  	git update-ref refs/heads/foo $A &&
> -	test_must_fail test -f .git/packed-refs &&
> +	test_path_is_missing .git/packed-refs &&
>  	git update-ref refs/heads/foo $B &&
> -	test_must_fail test -f .git/packed-refs &&
> +	test_path_is_missing .git/packed-refs &&
>  	git update-ref refs/heads/foo $C $B &&
> -	test_must_fail test -f .git/packed-refs &&
> +	test_path_is_missing .git/packed-refs &&
>  	git update-ref -d refs/heads/foo &&
> -	test_must_fail test -f .git/packed-refs
> +	test_path_is_missing .git/packed-refs
>  '
>  
>  test_expect_success 'check that marking the packed-refs file works' '

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 07/15] t0020: drop redirections to /dev/null
  2019-12-17 12:01 ` [PATCH 07/15] t0020: drop redirections to /dev/null Denton Liu
@ 2019-12-17 19:32   ` Eric Sunshine
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Sunshine @ 2019-12-17 19:32 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:
> Since output is silenced when running without `-v` and debugging output
> is useful with `-v`, remove redirections to /dev/null as it is not
> useful.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> @@ -5,7 +5,7 @@ test_description='CRLF conversion'
>  has_cr() {
> -       tr '\015' Q <"$1" | grep Q >/dev/null
> +       tr '\015' Q <"$1" | grep Q
>  }

I'm not sure that I agree with this change since dropping >/dev/null
doesn't improve the situation when someone is trying to debug a
failing test. What this change will do is fill the -v output with a
bunch of lines ending with "Q" when CR is expected -- the normal
_successful_ case for about half the calls to has_cr() -- so -v output
will become a lot noisier without really adding much, if any,
debugging value. If you really want to help people trying to diagnose
failures, I could see you replacing has_cr() with two new functions
which actually provide useful diagnostic output; for instance
something like:

    expect_cr () {
        if ! tr '\015' Q <"$1" | grep Q >/dev/null
        then
            echo "missing CR termination in: $1" >&2 &&
            return 1
        fi
    }

    expect_no_cr () {
        if tr '\015' Q <"$1" | grep Q >/dev/null
        then
            echo "unexpected CR termination in: $1" >&2 &&
            return 1
        fi
    }

However, I'm not convinced that introducing and debugging these
functions is worth the effort over simply dropping this patch from the
series.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/
  2019-12-17 12:01 ` [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/ Denton Liu
@ 2019-12-17 19:36   ` Eric Sunshine
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Sunshine @ 2019-12-17 19:36 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:
> t0020: s/test_must_fail has_cr/! has_cr/

Cryptic patch subject. How about being consistent with other patch
subjects in this series?

    t0020: don't use `test_must_fail has_cr`

> The test_must_fail function should only be used for git commands since
> we should assume that external commands work sanely. Since has_cr() just
> wraps a tr and grep pipeline, replace `test_must_fail has_cr` with
> `! has_cr`.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 14/15] t1501: remove use of `test_might_fail cp`
  2019-12-17 12:01 ` [PATCH 14/15] t1501: remove use of `test_might_fail cp` Denton Liu
@ 2019-12-17 19:46   ` Eric Sunshine
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Sunshine @ 2019-12-17 19:46 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:
> The test_must_fail() family of functions (including test_might_fail())
> should only be used on git commands. Replace test_might_fail() with
> test_non_git_might_fail().
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
> @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
>         cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
> -       test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
> +       test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&

Can you update the commit message to show that this change is indeed
the correct choice so that readers of this patch don't have to go
through the same investigative reasoning process you went through to
convince yourself that it is correct. In other words, why was
test_might_fail() used in the first place? Is it because there might
not be a sharedindex.* file? Or is it because repo.git/repos/foo might
not be writable? Or what? The answer to these questions should explain
to the reader, for instance, why you chose to use
`test_non_git_might_fail cp ...` as opposed to a simple `cp -f ...`.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 15/15] t1507: teach full_name() to accept `!` arg
  2019-12-17 12:01 ` [PATCH 15/15] t1507: teach full_name() to accept `!` arg Denton Liu
@ 2019-12-17 19:54   ` Eric Sunshine
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Sunshine @ 2019-12-17 19:54 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:
> Before, we were running `test_must_fail full_name`. However,
> `test_must_fail` should only be used on git commands. Teach full_name()
> to accept `!` as a potential first argument which will prepend
> `test_must_fail` to the enclosed git command. This increases the
> granularity of test_must_fail since it no longer runs on the `cd` as
> well.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
> @@ -29,8 +29,14 @@ test_expect_success 'setup' '
>  full_name () {
> +       fail= &&
> +       if test "x$1" = 'x!'
> +       then
> +               fail=test_must_fail &&
> +               shift
> +       fi &&
>         (cd clone &&
> -        git rev-parse --symbolic-full-name "$@")
> +        $fail git rev-parse --symbolic-full-name "$@")
>  }

Yuck. These days this is entirely unnecessary. My suggestion is to
drop the full_name() function altogether and just invoke git-rev-parse
directly at the (few) call sites, taking advantage of the fact that we
now have -C. So...

> @@ -79,7 +85,7 @@ test_expect_success 'upstream of branch with @ at end' '
>  test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '
> -       test_must_fail full_name refs/heads/my-side@{upstream}
> +       full_name ! refs/heads/my-side@{upstream}
>  '

This just becomes:

    test_must_fail git -C clone rev-parse --symbolic-full-name
refs/heads/my-side@{upstream}

Similarly for the other call sites.

> @@ -91,9 +97,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '
>  test_expect_success 'not-tracking@{u} fails' '
> -       test_must_fail full_name non-tracking@{u} &&
> +       full_name ! non-tracking@{u} &&
>         (cd clone && git checkout --no-track -b non-tracking) &&
> -       test_must_fail full_name non-tracking@{u}
> +       full_name ! non-tracking@{u}
>  '

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test()
  2019-12-17 12:01 ` [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test() Denton Liu
@ 2019-12-17 20:23   ` Johannes Sixt
  0 siblings, 0 replies; 64+ messages in thread
From: Johannes Sixt @ 2019-12-17 20:23 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List

Am 17.12.19 um 13:01 schrieb Denton Liu:
> The test_must_fail function should only be used for git commands since
> we should assume that external commands work sanely. We use
> test_must_fail to test run_sub_test_lib_test() but that function does
> not invoke any git commands internally. Replace these instances of
> `test_must_fail` with `!`.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  t/t0000-basic.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 8a81a249d0..d60ad4b78b 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' "
>  "
>  
>  test_expect_success 'pretend we have a partially passing test suite' "
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		partial-pass '2/3 tests passing' <<-\\EOF &&

It is a very uncommon situation (read: I doubt that it ever occurs) in
our test suite that we expect a shell function to fail, but that we do
*not* care at all which of its sub-commands actually failed. We actually
do care which sub-command failed. Therefore, we have, e.g., the idiom
"test_i18n_grep ! ...". And in fact, in the case of
run_sub_test_lib_test we have the form run_sub_test_lib_test_err to
check for error exit in the subordinate test. All of the cases you
change here should use it.

>  	test_expect_success 'passing test #1' 'true'
>  	test_expect_success 'failing test #2' 'false'
> @@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su
>  "
>  
>  test_expect_success 'pretend we have a pass, fail, and known breakage' "
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		mixed-results1 'mixed results #1' <<-\\EOF &&
>  	test_expect_success 'passing test' 'true'
>  	test_expect_success 'failing test' 'false'
> @@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' "
>  "
>  
>  test_expect_success 'pretend we have a mix of all possible results' "
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		mixed-results2 'mixed results #2' <<-\\EOF &&
>  	test_expect_success 'passing test' 'true'
>  	test_expect_success 'passing test' 'true'
> @@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' "
>  "
>  
>  test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		t1234-verbose "test verbose" --verbose <<-\EOF &&
>  	test_expect_success "passing test" true
>  	test_expect_success "test with output" "echo foo"
> @@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
>  '
>  
>  test_expect_success 'test --verbose-only' '
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		t2345-verbose-only-2 "test verbose-only=2" \
>  		--verbose-only=2 <<-\EOF &&
>  	test_expect_success "passing test" true
> @@ -834,7 +834,7 @@ then
>  fi
>  
>  test_expect_success 'tests clean up even on failures' "
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
>  	test_expect_success 'tests clean up even after a failure' '
>  		touch clean-after-failure &&
> @@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' "
>  "
>  
>  test_expect_success 'test_atexit is run' "
> -	test_must_fail run_sub_test_lib_test \
> +	! run_sub_test_lib_test \
>  		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
>  	test_expect_success 'tests clean up even after a failure' '
>  		> ../../clean-atexit &&
> 

-- Hannes

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v2 00/16] t: replace incorrect test_must_fail usage (part 1)
  2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                   ` (15 preceding siblings ...)
  2019-12-17 18:28 ` [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Junio C Hamano
@ 2019-12-19 22:22 ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 01/16] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
                     ` (16 more replies)
  16 siblings, 17 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

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 first part. It focuses on t[01]*.sh and also t/lib-git-p4.

Changes since v1:

* Incorporate review comments by Junio, Eric and J6t

* Further cleanup of t1507 before inlining full_name() (since we didn't
  want to inline full_name() into a bad command substitution)

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2

Denton Liu (16):
  test-lib-functions: introduce test_non_git_might_fail()
  t/lib-git-p4: use test_path_is_missing()
  t0000: replace test_must_fail with run_sub_test_lib_test_err()
  t0003: use named parameters in attr_check()
  t0003: use test_must_be_empty()
  t0003: don't use `test_must_fail attr_check`
  t0020: don't use `test_must_fail has_cr`
  t0020: use ! check_packed_refs_marked
  t1306: convert `test_might_fail rm` to `rm -f`
  t1307: reorder `nongit test_must_fail`
  t1409: let sed open its own input file
  t1409: use test_path_is_missing()
  t1501: remove use of `test_might_fail cp`
  t1507: stop losing return codes of git commands
  t1507: run commands within test_expect_success
  t1507: inline full_name()

 t/lib-git-p4.sh               |   2 +-
 t/t0000-basic.sh              |  14 ++---
 t/t0003-attributes.sh         |  47 +++++++--------
 t/t0020-crlf.sh               |  18 +++---
 t/t1306-xdg-files.sh          |   8 +--
 t/t1307-config-blob.sh        |   2 +-
 t/t1409-avoid-packing-refs.sh |  16 +++---
 t/t1501-work-tree.sh          |   2 +-
 t/t1507-rev-parse-upstream.sh | 104 +++++++++++++++++++---------------
 t/test-lib-functions.sh       |   9 +++
 10 files changed, 121 insertions(+), 101 deletions(-)

Range-diff against v1:
 1:  fcfccebd7a !  1:  778ae9052b t0000: replace test_must_fail with ! for run_sub_test_lib_test()
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t0000: replace test_must_fail with ! for run_sub_test_lib_test()
    +    t0000: replace test_must_fail with run_sub_test_lib_test_err()
     
         The test_must_fail function should only be used for git commands since
         we should assume that external commands work sanely. We use
         test_must_fail to test run_sub_test_lib_test() but that function does
    -    not invoke any git commands internally. Replace these instances of
    -    `test_must_fail` with `!`.
    +    not invoke any git commands internally. Even better, we have a function
    +    that's exactly meant to be used when we expect to have a failing test
    +    suite: run_sub_test_lib_test_err()!
    +
    +    Replace `test_must_fail run_sub_test_lib_test` with
    +    `run_sub_test_lib_test_err`.
     
      ## t/t0000-basic.sh ##
     @@ t/t0000-basic.sh: test_expect_success 'pretend we have a fully passing test suite' "
    @@ t/t0000-basic.sh: test_expect_success 'pretend we have a fully passing test suit
      
      test_expect_success 'pretend we have a partially passing test suite' "
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		partial-pass '2/3 tests passing' <<-\\EOF &&
      	test_expect_success 'passing test #1' 'true'
      	test_expect_success 'failing test #2' 'false'
    @@ t/t0000-basic.sh: test_expect_success 'pretend we have fixed one of two known br
      
      test_expect_success 'pretend we have a pass, fail, and known breakage' "
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		mixed-results1 'mixed results #1' <<-\\EOF &&
      	test_expect_success 'passing test' 'true'
      	test_expect_success 'failing test' 'false'
    @@ t/t0000-basic.sh: test_expect_success 'pretend we have a pass, fail, and known b
      
      test_expect_success 'pretend we have a mix of all possible results' "
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		mixed-results2 'mixed results #2' <<-\\EOF &&
      	test_expect_success 'passing test' 'true'
      	test_expect_success 'passing test' 'true'
    @@ t/t0000-basic.sh: test_expect_success 'pretend we have a mix of all possible res
      
      test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		t1234-verbose "test verbose" --verbose <<-\EOF &&
      	test_expect_success "passing test" true
      	test_expect_success "test with output" "echo foo"
    @@ t/t0000-basic.sh: test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
      
      test_expect_success 'test --verbose-only' '
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		t2345-verbose-only-2 "test verbose-only=2" \
      		--verbose-only=2 <<-\EOF &&
      	test_expect_success "passing test" true
    @@ t/t0000-basic.sh: then
      
      test_expect_success 'tests clean up even on failures' "
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
      	test_expect_success 'tests clean up even after a failure' '
      		touch clean-after-failure &&
    @@ t/t0000-basic.sh: test_expect_success 'tests clean up even on failures' "
      
      test_expect_success 'test_atexit is run' "
     -	test_must_fail run_sub_test_lib_test \
    -+	! run_sub_test_lib_test \
    ++	run_sub_test_lib_test_err \
      		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
      	test_expect_success 'tests clean up even after a failure' '
      		> ../../clean-atexit &&
 2:  f1acb2a0df !  2:  dbc82d45c6 t0003: use named parameters in attr_check()
    @@ Commit message
         variables instead of numbered arguments for clarity.
     
         While we're at it, add variable assignments to the &&-chain. These
    -    aren't ever expected to fail but for stylistic purposes, include them
    -    anyway for stylistic purposes.
    +    aren't ever expected to fail but if a future developer ever adds some
    +    code above the assignments and they could fail in some way, the intact
    +    &&-chain will ensure that the failure is caught.
     
      ## t/t0003-attributes.sh ##
     @@ t/t0003-attributes.sh: test_description=gitattributes
    @@ t/t0003-attributes.sh: test_description=gitattributes
      
      attr_check () {
     -	path="$1" expect="$2"
    -+	path="$1" &&
    -+	expect="$2" &&
    -+	git_opts="$3" &&
    ++	path="$1" expect="$2" git_opts="$3" &&
      
     -	git $3 check-attr test -- "$path" >actual 2>err &&
     -	echo "$path: test: $2" >expect &&
    @@ t/t0003-attributes.sh: test_description=gitattributes
     -	path="$1"
     -	quoted_path="$2"
     -	expect="$3"
    -+	path="$1" &&
    -+	quoted_path="$2" &&
    -+	expect="$3" &&
    ++	path="$1" quoted_path="$2" expect="$3" &&
      
      	git check-attr test -- "$path" >actual &&
      	echo "\"$quoted_path\": test: $expect" >expect &&
 3:  055caa5c08 =  3:  e06a06cff5 t0003: use test_must_be_empty()
 4:  3afa3a16ca =  4:  219011f983 t0003: don't use `test_must_fail attr_check`
 5:  d228dcfdc7 <  -:  ---------- t0020: drop redirections to /dev/null
 6:  8adc5cd5aa !  5:  8da6c96b39 t0020: s/test_must_fail has_cr/! has_cr/
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t0020: s/test_must_fail has_cr/! has_cr/
    +    t0020: don't use `test_must_fail has_cr`
     
         The test_must_fail function should only be used for git commands since
         we should assume that external commands work sanely. Since has_cr() just
 7:  b77b474422 =  6:  27550eaae6 t0020: use ! check_packed_refs_marked
 8:  d39422505f =  7:  c19f6344a4 t1306: convert `test_might_fail rm` to `rm -f`
 9:  2dd91c5568 =  8:  d6ea8a6df0 t1307: reorder `nongit test_must_fail`
10:  0b7d19a7e1 !  9:  d57dfe95e2 t1409: let sed open its own files
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t1409: let sed open its own files
    +    t1409: let sed open its own input file
     
         In one case, we were using a redirection operator to feed input into
    -    sed. However, since sed is capable of opening its own files, make sed
    -    open its own files instead of redirecting input into it.
    +    sed. However, since sed is capable of opening its own input file, make
    +    sed do that instead of redirecting input into it.
     
      ## t/t1409-avoid-packing-refs.sh ##
     @@ t/t1409-avoid-packing-refs.sh: test_description='avoid rewriting packed-refs unnecessarily'
11:  27b3296242 ! 10:  eacf4e0fb4 t1409: use test_path_is_missing()
    @@ Commit message
         The test_must_fail() function should only be used for git commands since
         we should assume that external commands work sanely. Replace
         `test_must_fail test -f` with `test_path_is_missing` since we expect
    -    these files to not exist.
    +    these paths to not exist.
     
      ## t/t1409-avoid-packing-refs.sh ##
     @@ t/t1409-avoid-packing-refs.sh: test_expect_success 'setup' '
12:  3d36511d5d ! 11:  83e47748bc t1501: remove use of `test_might_fail cp`
    @@ Commit message
         should only be used on git commands. Replace test_might_fail() with
         test_non_git_might_fail().
     
    +    The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix
    +    test with split index, 2015-03-24). It is necessary because there might
    +    exist some index files in `repo.git/sharedindex.*` and, if they exist,
    +    we want to copy them over. However, if they don't exist, we don't want
    +    to error out because we expect that possibility. As a result, we want to
    +    keep the "might fail" semantics so we use test_non_git_might_fail().
    +
      ## t/t1501-work-tree.sh ##
     @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' '
      	mkdir work &&
13:  cd392a74ac <  -:  ---------- t1507: teach full_name() to accept `!` arg
 -:  ---------- > 12:  9e20865f94 t1507: stop losing return codes of git commands
 -:  ---------- > 13:  7c61ac6b67 t1507: run commands within test_expect_success
 -:  ---------- > 14:  d09370455f t1507: inline full_name()
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v2 01/16] test-lib-functions: introduce test_non_git_might_fail()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 02/16] t/lib-git-p4: use test_path_is_missing() Denton Liu
                     ` (15 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In a future commit, we will be preventing the use of the
test_must_fail()-family of functions (including test_might_fail()) on
non-git comands. To prep for this, introduce the
test_non_git_might_fail() function which is used to replace non-git
invocations of test_might_fail().

The test_non_git_might_fail() function is a lightweight replacement,
always masking the return status of the command and returning a
non-error exit code. Unlike test_might_fail(), it does not check for
abnormal exit conditions such as a segv. This is because we are not in
the business of checking the sanity of the external environment and we
can assume that it works properly.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/test-lib-functions.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 284c52d076..61d27f1ec6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -891,6 +891,15 @@ test_expect_code () {
 	return 1
 } 7>&2 2>&4
 
+# Similar to test_might_fail, but much simpler. This is intended for use
+# with non-git commands that we can assume will work sanely so we don't
+# need to check for conditions such as a segv.
+
+test_non_git_might_fail () {
+	"$@" 2>&7
+	return 0
+}
+
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
 #
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 02/16] t/lib-git-p4: use test_path_is_missing()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
  2019-12-19 22:22   ` [PATCH v2 01/16] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 03/16] t0000: replace test_must_fail with run_sub_test_lib_test_err() Denton Liu
                     ` (14 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

Previously, cleanup_git() would use `test_must_fail test -d` to ensure
that the directory is removed. However, test_must_fail should only be
used for git commands. Use test_path_is_missing() instead to check that
the directory has been removed.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 547b9f88e1..5aff2abe8b 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -175,7 +175,7 @@ stop_and_cleanup_p4d () {
 
 cleanup_git () {
 	retry_until_success rm -r "$git"
-	test_must_fail test -d "$git" &&
+	test_path_is_missing "$git" &&
 	retry_until_success mkdir "$git"
 }
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 03/16] t0000: replace test_must_fail with run_sub_test_lib_test_err()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
  2019-12-19 22:22   ` [PATCH v2 01/16] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
  2019-12-19 22:22   ` [PATCH v2 02/16] t/lib-git-p4: use test_path_is_missing() Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 04/16] t0003: use named parameters in attr_check() Denton Liu
                     ` (13 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. We use
test_must_fail to test run_sub_test_lib_test() but that function does
not invoke any git commands internally. Even better, we have a function
that's exactly meant to be used when we expect to have a failing test
suite: run_sub_test_lib_test_err()!

Replace `test_must_fail run_sub_test_lib_test` with
`run_sub_test_lib_test_err`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0000-basic.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 8a81a249d0..3e440c078d 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' "
 "
 
 test_expect_success 'pretend we have a partially passing test suite' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		partial-pass '2/3 tests passing' <<-\\EOF &&
 	test_expect_success 'passing test #1' 'true'
 	test_expect_success 'failing test #2' 'false'
@@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su
 "
 
 test_expect_success 'pretend we have a pass, fail, and known breakage' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		mixed-results1 'mixed results #1' <<-\\EOF &&
 	test_expect_success 'passing test' 'true'
 	test_expect_success 'failing test' 'false'
@@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' "
 "
 
 test_expect_success 'pretend we have a mix of all possible results' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		mixed-results2 'mixed results #2' <<-\\EOF &&
 	test_expect_success 'passing test' 'true'
 	test_expect_success 'passing test' 'true'
@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' "
 "
 
 test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		t1234-verbose "test verbose" --verbose <<-\EOF &&
 	test_expect_success "passing test" true
 	test_expect_success "test with output" "echo foo"
@@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
 '
 
 test_expect_success 'test --verbose-only' '
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		t2345-verbose-only-2 "test verbose-only=2" \
 		--verbose-only=2 <<-\EOF &&
 	test_expect_success "passing test" true
@@ -834,7 +834,7 @@ then
 fi
 
 test_expect_success 'tests clean up even on failures' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		touch clean-after-failure &&
@@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' "
 "
 
 test_expect_success 'test_atexit is run' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		> ../../clean-atexit &&
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 04/16] t0003: use named parameters in attr_check()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (2 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 03/16] t0000: replace test_must_fail with run_sub_test_lib_test_err() Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 05/16] t0003: use test_must_be_empty() Denton Liu
                     ` (12 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

We had named the parameters in attr_check() but $2 was being used
instead of $expect. Make all variable accesses in attr_check() use named
variables instead of numbered arguments for clarity.

While we're at it, add variable assignments to the &&-chain. These
aren't ever expected to fail but if a future developer ever adds some
code above the assignments and they could fail in some way, the intact
&&-chain will ensure that the failure is caught.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 71e63d8b50..3569bef75d 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -5,19 +5,16 @@ test_description=gitattributes
 . ./test-lib.sh
 
 attr_check () {
-	path="$1" expect="$2"
+	path="$1" expect="$2" git_opts="$3" &&
 
-	git $3 check-attr test -- "$path" >actual 2>err &&
-	echo "$path: test: $2" >expect &&
+	git $git_opts check-attr test -- "$path" >actual 2>err &&
+	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual &&
 	test_line_count = 0 err
 }
 
 attr_check_quote () {
-
-	path="$1"
-	quoted_path="$2"
-	expect="$3"
+	path="$1" quoted_path="$2" expect="$3" &&
 
 	git check-attr test -- "$path" >actual &&
 	echo "\"$quoted_path\": test: $expect" >expect &&
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 05/16] t0003: use test_must_be_empty()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (3 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 04/16] t0003: use named parameters in attr_check() Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 06/16] t0003: don't use `test_must_fail attr_check` Denton Liu
                     ` (11 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In several places, we used `test_line_count = 0` to check for an empty
file. Although this is correct, it's overkill. Use test_must_be_empty()
instead because it's more suited for this purpose.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 3569bef75d..c30c736d3f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -10,7 +10,7 @@ attr_check () {
 	git $git_opts check-attr test -- "$path" >actual 2>err &&
 	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 }
 
 attr_check_quote () {
@@ -241,7 +241,7 @@ EOF
 	git check-attr foo -- "a/b/f" >>actual 2>>err &&
 	git check-attr foo -- "a/b/c/f" >>actual 2>>err &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success '"**" with no slashes test' '
@@ -262,7 +262,7 @@ EOF
 	git check-attr foo -- "a/b/f" >>actual 2>>err &&
 	git check-attr foo -- "a/b/c/f" >>actual 2>>err &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success 'using --git-dir and --work-tree' '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 06/16] t0003: don't use `test_must_fail attr_check`
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (4 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 05/16] t0003: use test_must_be_empty() Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 07/16] t0020: don't use `test_must_fail has_cr` Denton Liu
                     ` (10 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In an effort to remove test_must_fail for all invocations not related to
git or test-tool, replace invocations of `test_must_fail attr_check`
with a plain attr_check call with the $expect argument set to the
actual value output by git.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index c30c736d3f..b660593c20 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -24,7 +24,7 @@ attr_check_quote () {
 
 test_expect_success 'open-quoted pathname' '
 	echo "\"a test=a" >.gitattributes &&
-	test_must_fail attr_check a a
+	attr_check a unspecified
 '
 
 
@@ -109,20 +109,20 @@ test_expect_success 'attribute test' '
 
 test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '
 
-	test_must_fail attr_check F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/c/F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/G a/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/B/g a/b/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/G a/b/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/H a/b/h "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=0" &&
-	test_must_fail attr_check oNoFf unset "-c core.ignorecase=0" &&
-	test_must_fail attr_check oFfOn set "-c core.ignorecase=0" &&
+	attr_check F unspecified "-c core.ignorecase=0" &&
+	attr_check a/F unspecified "-c core.ignorecase=0" &&
+	attr_check a/c/F unspecified "-c core.ignorecase=0" &&
+	attr_check a/G unspecified "-c core.ignorecase=0" &&
+	attr_check a/B/g a/g "-c core.ignorecase=0" &&
+	attr_check a/b/G unspecified "-c core.ignorecase=0" &&
+	attr_check a/b/H unspecified "-c core.ignorecase=0" &&
+	attr_check a/b/D/g a/g "-c core.ignorecase=0" &&
+	attr_check oNoFf unspecified "-c core.ignorecase=0" &&
+	attr_check oFfOn unspecified "-c core.ignorecase=0" &&
 	attr_check NO unspecified "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/b/D/NO unspecified "-c core.ignorecase=0" &&
 	attr_check a/b/d/YES a/b/d/* "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/E/f "A/e/F" "-c core.ignorecase=0"
+	attr_check a/E/f f "-c core.ignorecase=0"
 
 '
 
@@ -146,8 +146,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase
 '
 
 test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '
-	test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" &&
-	test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/B/D/g a/g "-c core.ignorecase=0" &&
+	attr_check A/B/D/NO unspecified "-c core.ignorecase=0" &&
 	attr_check A/b/h a/b/h "-c core.ignorecase=1" &&
 	attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=1" &&
 	attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=1"
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 07/16] t0020: don't use `test_must_fail has_cr`
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (5 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 06/16] t0003: don't use `test_must_fail attr_check` Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 08/16] t0020: use ! check_packed_refs_marked Denton Liu
                     ` (9 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since has_cr() just
wraps a tr and grep pipeline, replace `test_must_fail has_cr` with
`! has_cr`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0020-crlf.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 854da0ae16..b63ba62e5d 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' '
 	rm -f tmp one dir/two three &&
 	git config core.autocrlf input &&
 	git read-tree --reset -u HEAD &&
-	test_must_fail has_cr one &&
-	test_must_fail has_cr dir/two &&
+	! has_cr one &&
+	! has_cr dir/two &&
 	git update-index -- one dir/two &&
 	test "$one" = $(git hash-object --stdin <one) &&
 	test "$two" = $(git hash-object --stdin <dir/two) &&
@@ -237,9 +237,9 @@ test_expect_success '.gitattributes says two is binary' '
 	git config core.autocrlf true &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr dir/two &&
+	! has_cr dir/two &&
 	verbose has_cr one &&
-	test_must_fail has_cr three
+	! has_cr three
 '
 
 test_expect_success '.gitattributes says two is input' '
@@ -248,7 +248,7 @@ test_expect_success '.gitattributes says two is input' '
 	echo "two crlf=input" >.gitattributes &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr dir/two
+	! has_cr dir/two
 '
 
 test_expect_success '.gitattributes says two and three are text' '
@@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' '
 	rm -rf tmp one dir .gitattributes patch.file three &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' '
 	git read-tree --reset HEAD &&
 	git checkout-index -f -q -u -a &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' '
 	git checkout-index -u .gitattributes &&
 	git checkout-index -u one dir/two three &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	git checkout-index -u one dir/two three &&
 	git checkout-index -u .gitattributes &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 08/16] t0020: use ! check_packed_refs_marked
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (6 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 07/16] t0020: don't use `test_must_fail has_cr` Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 09/16] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
                     ` (8 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since
check_packed_refs_marked() just wraps a grep invocation, replace
`test_must_fail check_packed_refs_marked` with
`! check_packed_refs_marked`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index e5cb8a252d..c46848eb8e 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' '
 	git for-each-ref >actual &&
 	test_cmp expected actual &&
 	git pack-refs --all &&
-	test_must_fail check_packed_refs_marked &&
+	! check_packed_refs_marked &&
 	git for-each-ref >actual2 &&
 	test_cmp expected actual2
 '
@@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' '
 	git pack-refs --all &&
 	mark_packed_refs &&
 	git update-ref -d refs/heads/packed-delete &&
-	test_must_fail check_packed_refs_marked
+	! check_packed_refs_marked
 '
 
 test_expect_success 'leave packed-refs untouched on update of loose' '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 09/16] t1306: convert `test_might_fail rm` to `rm -f`
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (7 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 08/16] t0020: use ! check_packed_refs_marked Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 10/16] t1307: reorder `nongit test_must_fail` Denton Liu
                     ` (7 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail() family of functions (including test_might_fail())
should only be used on git commands. Replace `test_might_fail rm` with
`rm -f` so that we don't use `test_might_fail` on a non-git command.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1306-xdg-files.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 21e139a313..dd87b43be1 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files'
 
 
 test_expect_success 'Checking attributes in a non-XDG global attributes file' '
-	test_might_fail rm .gitattributes &&
+	rm -f .gitattributes &&
 	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
 	git config core.attributesfile "$HOME"/my_gitattributes &&
 	echo "f: attr_f: test" >expected &&
@@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
 	mkdir -p "$HOME"/.config/git &&
 	>"$HOME"/.config/git/config &&
-	test_might_fail rm "$HOME"/.gitconfig &&
+	rm -f "$HOME"/.gitconfig &&
 	git config --global user.name "write_config" &&
 	echo "[user]" >expected &&
 	echo "	name = write_config" >>expected &&
@@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
 
 
 test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
-	test_might_fail rm "$HOME"/.gitconfig &&
-	test_might_fail rm "$HOME"/.config/git/config &&
+	rm -f "$HOME"/.gitconfig &&
+	rm -f "$HOME"/.config/git/config &&
 	git config --global user.name "write_gitconfig" &&
 	echo "[user]" >expected &&
 	echo "	name = write_gitconfig" >>expected &&
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 10/16] t1307: reorder `nongit test_must_fail`
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (8 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 09/16] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 11/16] t1409: let sed open its own input file Denton Liu
                     ` (6 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In the future, we plan on only allowing `test_must_fail` to work on a
restricted subset of commands, including `git`. Reorder the commands so
that `nongit` comes before `test_must_fail`. This way, `test_must_fail`
operates on a git command.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1307-config-blob.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
index 37dc689d8c..002e6d3388 100755
--- a/t/t1307-config-blob.sh
+++ b/t/t1307-config-blob.sh
@@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' '
 '
 
 test_expect_success 'config --blob outside of a repository is an error' '
-	test_must_fail nongit git config --blob=foo --list
+	nongit test_must_fail git config --blob=foo --list
 '
 
 test_done
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 11/16] t1409: let sed open its own input file
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (9 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 10/16] t1307: reorder `nongit test_must_fail` Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 12/16] t1409: use test_path_is_missing() Denton Liu
                     ` (5 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In one case, we were using a redirection operator to feed input into
sed. However, since sed is capable of opening its own input file, make
sed do that instead of redirecting input into it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index c46848eb8e..f74d890e82 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
 mark_packed_refs () {
-	sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new &&
+	sed -e "s/^\(#.*\)/\1 t1409 /" .git/packed-refs >.git/packed-refs.new &&
 	mv .git/packed-refs.new .git/packed-refs
 }
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 12/16] t1409: use test_path_is_missing()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (10 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 11/16] t1409: let sed open its own input file Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 13/16] t1501: remove use of `test_might_fail cp` Denton Liu
                     ` (4 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test -f` with `test_path_is_missing` since we expect
these paths to not exist.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f74d890e82..be12fb6350 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -27,15 +27,15 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'do not create packed-refs file gratuitously' '
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $A &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $B &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $C $B &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref -d refs/heads/foo &&
-	test_must_fail test -f .git/packed-refs
+	test_path_is_missing .git/packed-refs
 '
 
 test_expect_success 'check that marking the packed-refs file works' '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (11 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 12/16] t1409: use test_path_is_missing() Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:52     ` Eric Sunshine
  2019-12-19 22:22   ` [PATCH v2 14/16] t1507: stop losing return codes of git commands Denton Liu
                     ` (3 subsequent siblings)
  16 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail() family of functions (including test_might_fail())
should only be used on git commands. Replace test_might_fail() with
test_non_git_might_fail().

The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix
test with split index, 2015-03-24). It is necessary because there might
exist some index files in `repo.git/sharedindex.*` and, if they exist,
we want to copy them over. However, if they don't exist, we don't want
to error out because we expect that possibility. As a result, we want to
keep the "might fail" semantics so we use test_non_git_might_fail().

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1501-work-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 3498d3d55e..067301a5ab 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
 	mkdir work &&
 	mkdir -p repo.git/repos/foo &&
 	cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
-	test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
+	test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
 	sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE
 '
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 14/16] t1507: stop losing return codes of git commands
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (12 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 13/16] t1501: remove use of `test_might_fail cp` Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 15/16] t1507: run commands within test_expect_success Denton Liu
                     ` (2 subsequent siblings)
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The return code of git commands are lost when a command is in a
non-assignment command substitution in favour of the surrounding
command's. Rewrite instances of this so that git commands run
on their own.

In commit_subject(), use a `tformat` instead of `format` since,
previously, we were testing the output of a command substitution which
didn't care if there was a trailing newline since it was automatically
stripped. Since we use test_cmp() now, the trailing newline matters so
use `tformat` to always output it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 45 +++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 8b4cf8a6e3..d81f289ace 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -35,7 +35,7 @@ full_name () {
 
 commit_subject () {
 	(cd clone &&
-	 git show -s --pretty=format:%s "$@")
+	 git show -s --pretty=tformat:%s "$@")
 }
 
 error_message () {
@@ -44,18 +44,27 @@ error_message () {
 }
 
 test_expect_success '@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{upstream})" &&
-	test refs/remotes/origin/master = "$(full_name @{UPSTREAM})" &&
-	test refs/remotes/origin/master = "$(full_name @{UpSTReam})"
+	echo refs/remotes/origin/master >expect &&
+	full_name @{upstream} >actual &&
+	test_cmp expect actual &&
+	full_name @{UPSTREAM} >actual &&
+	test_cmp expect actual &&
+	full_name @{UpSTReam} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{u} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{u})" &&
-	test refs/remotes/origin/master = "$(full_name @{U})"
+	echo refs/remotes/origin/master >expect &&
+	full_name @{u} >actual &&
+	test_cmp expect actual &&
+	full_name @{U} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/side = "$(full_name my-side@{u})"
+	echo refs/remotes/origin/side >expect &&
+	full_name my-side@{u} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ in middle' '
@@ -86,8 +95,11 @@ test_expect_success 'my-side@{u} resolves to correct commit' '
 	git checkout side &&
 	test_commit 5 &&
 	(cd clone && git fetch) &&
-	test 2 = "$(commit_subject my-side)" &&
-	test 5 = "$(commit_subject my-side@{u})"
+	echo 2 >expect &&
+	commit_subject my-side >actual &&
+	test_cmp expect actual &&
+	echo 5 >expect &&
+	commit_subject my-side@{u} >actual
 '
 
 test_expect_success 'not-tracking@{u} fails' '
@@ -99,8 +111,11 @@ test_expect_success 'not-tracking@{u} fails' '
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
 	test_commit 6 &&
 	(cd clone && git fetch) &&
-	test 5 = $(commit_subject my-side@{u}@{1}) &&
-	test 5 = $(commit_subject my-side@{U}@{1})
+	echo 5 >expect &&
+	commit_subject my-side@{u}@{1} >actual &&
+	test_cmp expect actual &&
+	commit_subject my-side@{U}@{1} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
@@ -149,7 +164,9 @@ test_expect_success 'checkout other@{u}' '
 '
 
 test_expect_success 'branch@{u} works when tracking a local branch' '
-	test refs/heads/master = "$(full_name local-master@{u})"
+	echo refs/heads/master >expect &&
+	full_name local-master@{u} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'branch@{u} error message when no upstream' '
@@ -203,7 +220,9 @@ test_expect_success 'pull works when tracking a local branch' '
 
 # makes sense if the previous one succeeded
 test_expect_success '@{u} works when tracking a local branch' '
-	test refs/heads/master = "$(full_name @{u})"
+	echo refs/heads/master >expect &&
+	full_name @{u} >actual &&
+	test_cmp expect actual
 '
 
 commit=$(git rev-parse HEAD)
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 15/16] t1507: run commands within test_expect_success
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (13 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 14/16] t1507: stop losing return codes of git commands Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-19 22:22   ` [PATCH v2 16/16] t1507: inline full_name() Denton Liu
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The expected test style is to have all commands tested within a
test_expect_success block. Move the generation of the 'expect' text into
their corresponding blocks. While we're at it, insert a second
`commit=$(git rev-parse HEAD)` into the next test case so that it's
clear where $commit is coming from.

The biggest advantage of doing this is that we now check the return code
of `git rev-parse HEAD` so we can catch it in case it fails.

This patch is best viewed with `--color-moved --ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 40 +++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index d81f289ace..f68b77e7ba 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -225,32 +225,32 @@ test_expect_success '@{u} works when tracking a local branch' '
 	test_cmp expect actual
 '
 
-commit=$(git rev-parse HEAD)
-cat >expect <<EOF
-commit $commit
-Reflog: master@{0} (C O Mitter <committer@example.com>)
-Reflog message: branch: Created from HEAD
-Author: A U Thor <author@example.com>
-Date:   Thu Apr 7 15:15:13 2005 -0700
-
-    3
-EOF
 test_expect_success 'log -g other@{u}' '
+	commit=$(git rev-parse HEAD) &&
+	cat >expect <<-EOF &&
+	commit $commit
+	Reflog: master@{0} (C O Mitter <committer@example.com>)
+	Reflog message: branch: Created from HEAD
+	Author: A U Thor <author@example.com>
+	Date:   Thu Apr 7 15:15:13 2005 -0700
+
+	    3
+	EOF
 	git log -1 -g other@{u} >actual &&
 	test_cmp expect actual
 '
 
-cat >expect <<EOF
-commit $commit
-Reflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter <committer@example.com>)
-Reflog message: branch: Created from HEAD
-Author: A U Thor <author@example.com>
-Date:   Thu Apr 7 15:15:13 2005 -0700
-
-    3
-EOF
-
 test_expect_success 'log -g other@{u}@{now}' '
+	commit=$(git rev-parse HEAD) &&
+	cat >expect <<-EOF &&
+	commit $commit
+	Reflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter <committer@example.com>)
+	Reflog message: branch: Created from HEAD
+	Author: A U Thor <author@example.com>
+	Date:   Thu Apr 7 15:15:13 2005 -0700
+
+	    3
+	EOF
 	git log -1 -g other@{u}@{now} >actual &&
 	test_cmp expect actual
 '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v2 16/16] t1507: inline full_name()
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (14 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 15/16] t1507: run commands within test_expect_success Denton Liu
@ 2019-12-19 22:22   ` Denton Liu
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
  16 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-19 22:22 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

Before, we were running `test_must_fail full_name`. However,
`test_must_fail` should only be used on git commands. Inline full_name()
so that we can use test_must_fail on the git command directly.

When full_name() was introduced in 28fb84382b (Introduce
<branch>@{upstream} notation, 2009-09-10), the `git -C` option wasn't
available yet (since it was introduced in 44e1e4d67d (git: run in a
directory given with -C option, 2013-09-09)). As a result, the helper
function removed the need to manually cd each time. However, since
`git -C` is available now, we can just use that instead and inline
full_name().

An alternate approach was taken where we taught full_name() to accept an
optional `!` arg to trigger test_must_fail behavior. However, this added
more unnecessary complexity than inlining so we inline instead.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index f68b77e7ba..dfc0d96d8a 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -28,11 +28,6 @@ test_expect_success 'setup' '
 	)
 '
 
-full_name () {
-	(cd clone &&
-	 git rev-parse --symbolic-full-name "$@")
-}
-
 commit_subject () {
 	(cd clone &&
 	 git show -s --pretty=tformat:%s "$@")
@@ -45,50 +40,50 @@ error_message () {
 
 test_expect_success '@{upstream} resolves to correct full name' '
 	echo refs/remotes/origin/master >expect &&
-	full_name @{upstream} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{upstream} >actual &&
 	test_cmp expect actual &&
-	full_name @{UPSTREAM} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{UPSTREAM} >actual &&
 	test_cmp expect actual &&
-	full_name @{UpSTReam} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{UpSTReam} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '@{u} resolves to correct full name' '
 	echo refs/remotes/origin/master >expect &&
-	full_name @{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{u} >actual &&
 	test_cmp expect actual &&
-	full_name @{U} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{U} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
 	echo refs/remotes/origin/side >expect &&
-	full_name my-side@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name my-side@{u} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ in middle' '
-	full_name fun@ny@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name fun@ny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
 	test_cmp expect actual &&
-	full_name fun@ny@{U} >actual &&
+	git -C clone rev-parse --symbolic-full-name fun@ny@{U} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ at start' '
-	full_name @funny@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name @funny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ at end' '
-	full_name funny@@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name funny@@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '
-	test_must_fail full_name refs/heads/my-side@{upstream}
+	test_must_fail git -C clone rev-parse --symbolic-full-name refs/heads/my-side@{upstream}
 '
 
 test_expect_success 'my-side@{u} resolves to correct commit' '
@@ -103,9 +98,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '
 '
 
 test_expect_success 'not-tracking@{u} fails' '
-	test_must_fail full_name non-tracking@{u} &&
+	test_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u} &&
 	(cd clone && git checkout --no-track -b non-tracking) &&
-	test_must_fail full_name non-tracking@{u}
+	test_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u}
 '
 
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
@@ -165,7 +160,7 @@ test_expect_success 'checkout other@{u}' '
 
 test_expect_success 'branch@{u} works when tracking a local branch' '
 	echo refs/heads/master >expect &&
-	full_name local-master@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name local-master@{u} >actual &&
 	test_cmp expect actual
 '
 
@@ -221,7 +216,7 @@ test_expect_success 'pull works when tracking a local branch' '
 # makes sense if the previous one succeeded
 test_expect_success '@{u} works when tracking a local branch' '
 	echo refs/heads/master >expect &&
-	full_name @{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{u} >actual &&
 	test_cmp expect actual
 '
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* Re: [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`
  2019-12-19 22:22   ` [PATCH v2 13/16] t1501: remove use of `test_might_fail cp` Denton Liu
@ 2019-12-19 22:52     ` Eric Sunshine
  2019-12-19 23:19       ` Denton Liu
  0 siblings, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2019-12-19 22:52 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Johannes Sixt, Junio C Hamano

On Thu, Dec 19, 2019 at 5:22 PM Denton Liu <liu.denton@gmail.com> wrote:
> The test_must_fail() family of functions (including test_might_fail())
> should only be used on git commands. Replace test_might_fail() with
> test_non_git_might_fail().
>
> The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix
> test with split index, 2015-03-24). It is necessary because there might
> exist some index files in `repo.git/sharedindex.*` and, if they exist,
> we want to copy them over. However, if they don't exist, we don't want
> to error out because we expect that possibility. As a result, we want to
> keep the "might fail" semantics so we use test_non_git_might_fail().

Thanks for adding this paragraph to help the reader understand why you
chose this transformation. However...

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
> @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
>         cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
> -       test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
> +       test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&

I can't say I'm a fan of patch 1 introducing the function
test_non_git_might_fail() for this one particular case. I'd rather see
this case follow existing precedence by being written this way:

    { cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&

which is the idiomatic way this sort of thing is handled in existing tests.

While it's true that it may look a bit cryptic to people new to shell
scripting, as with any idiom, it's understood at a glance by people
familiar with it. That bit about "at a glance" is important: it's much
easier to comprehend idiomatic code than code which you have to spend
a lot of time _reading_ (and "test_non_git_might_fail" is quite a
mouthful, or eyeful, or something, which takes a lot more effort to
read and understand).

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`
  2019-12-19 22:52     ` Eric Sunshine
@ 2019-12-19 23:19       ` Denton Liu
  2019-12-20 17:32         ` Eric Sunshine
  0 siblings, 1 reply; 64+ messages in thread
From: Denton Liu @ 2019-12-19 23:19 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Git Mailing List, Johannes Sixt, Junio C Hamano

Hi Eric,

On Thu, Dec 19, 2019 at 05:52:56PM -0500, Eric Sunshine wrote:
> > diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
> > @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
> >         cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
> > -       test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
> > +       test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
> 
> I can't say I'm a fan of patch 1 introducing the function
> test_non_git_might_fail() for this one particular case. I'd rather see
> this case follow existing precedence by being written this way:
> 
>     { cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&
> 
> which is the idiomatic way this sort of thing is handled in existing tests.
> 
> While it's true that it may look a bit cryptic to people new to shell
> scripting, as with any idiom, it's understood at a glance by people
> familiar with it. That bit about "at a glance" is important: it's much
> easier to comprehend idiomatic code than code which you have to spend
> a lot of time _reading_ (and "test_non_git_might_fail" is quite a
> mouthful, or eyeful, or something, which takes a lot more effort to
> read and understand).

The reason why I chose to do this was because I found myself writing the
above many times in (currently unsent) later test cases that I cleaned
up. As a result, I felt like it could be wrapped up more nicely with a
helper function. That being said, if you think that open coding the
idiom looks nicer, I can reroll to inline it.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v2 13/16] t1501: remove use of `test_might_fail cp`
  2019-12-19 23:19       ` Denton Liu
@ 2019-12-20 17:32         ` Eric Sunshine
  0 siblings, 0 replies; 64+ messages in thread
From: Eric Sunshine @ 2019-12-20 17:32 UTC (permalink / raw)
  To: Denton Liu; +Cc: Git Mailing List, Johannes Sixt, Junio C Hamano

On Thu, Dec 19, 2019 at 6:18 PM Denton Liu <liu.denton@gmail.com> wrote:
> On Thu, Dec 19, 2019 at 05:52:56PM -0500, Eric Sunshine wrote:
> > [...] I'd rather see:
> >     { cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&
> > which is the idiomatic way this sort of thing is handled in existing tests.
> >
> > While it's true that it may look a bit cryptic to people new to shell
> > scripting, as with any idiom, it's understood at a glance by people
> > familiar with it. That bit about "at a glance" is important: it's much
> > easier to comprehend idiomatic code than code which you have to spend
> > a lot of time _reading_ [...]
>
> The reason why I chose to do this was because I found myself writing the
> above many times in (currently unsent) later test cases that I cleaned
> up. As a result, I felt like it could be wrapped up more nicely with a
> helper function. That being said, if you think that open coding the
> idiom looks nicer, I can reroll to inline it.

I wouldn't say that the idiom "looks nicer", but rather that it has
value specifically because it is an idiom, therefore it reduces
cognitive load (as explained above).

Idioms aside, the new function test_non_git_might_fail() may increase
cognitive load because the reader needs to remember its purpose and
behavior since it's a black box compared to the open-coded version,
yet adds no actual value. Compare that with, say, test_must_fail()
which not only adds value but would significantly increase cognitive
load if open-coded, thus is well justified. That's not to say that
one-liner functions can't necessarily have value; a well named
function or one which introduces an important abstraction may very
well be worthwhile, but I can't convince myself that
test_non_git_might_fail() succeeds in that way.

So, to answer your question, my preference leans toward open-coding this.

^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1)
  2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
                     ` (15 preceding siblings ...)
  2019-12-19 22:22   ` [PATCH v2 16/16] t1507: inline full_name() Denton Liu
@ 2019-12-20 18:15   ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 01/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
                       ` (14 more replies)
  16 siblings, 15 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

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 first part. It focuses on t[01]*.sh and also t/lib-git-p4.

Changes since v2:

* Drop the inclusion of test_non_git_must_fail() and open code it
  instead

Changes since v1:

* Incorporate review comments by Junio, Eric and J6t

* Further cleanup of t1507 before inlining full_name() (since we didn't
  want to inline full_name() into a bad command substitution)

[1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2

Denton Liu (15):
  t/lib-git-p4: use test_path_is_missing()
  t0000: replace test_must_fail with run_sub_test_lib_test_err()
  t0003: use named parameters in attr_check()
  t0003: use test_must_be_empty()
  t0003: don't use `test_must_fail attr_check`
  t0020: don't use `test_must_fail has_cr`
  t0020: use ! check_packed_refs_marked
  t1306: convert `test_might_fail rm` to `rm -f`
  t1307: reorder `nongit test_must_fail`
  t1409: let sed open its own input file
  t1409: use test_path_is_missing()
  t1501: remove use of `test_might_fail cp`
  t1507: stop losing return codes of git commands
  t1507: run commands within test_expect_success
  t1507: inline full_name()

 t/lib-git-p4.sh               |   2 +-
 t/t0000-basic.sh              |  14 ++---
 t/t0003-attributes.sh         |  47 +++++++--------
 t/t0020-crlf.sh               |  18 +++---
 t/t1306-xdg-files.sh          |   8 +--
 t/t1307-config-blob.sh        |   2 +-
 t/t1409-avoid-packing-refs.sh |  16 +++---
 t/t1501-work-tree.sh          |   2 +-
 t/t1507-rev-parse-upstream.sh | 104 +++++++++++++++++++---------------
 9 files changed, 112 insertions(+), 101 deletions(-)

Range-diff against v2:
 1:  85cee92765 <  -:  ---------- test-lib-functions: introduce test_non_git_might_fail()
 2:  3d64837af1 =  1:  cf3dd04b8a t/lib-git-p4: use test_path_is_missing()
 3:  778ae9052b =  2:  51a2226726 t0000: replace test_must_fail with run_sub_test_lib_test_err()
 4:  dbc82d45c6 =  3:  9374fcd8db t0003: use named parameters in attr_check()
 5:  e06a06cff5 =  4:  7f8808a850 t0003: use test_must_be_empty()
 6:  219011f983 =  5:  b725d53dbe t0003: don't use `test_must_fail attr_check`
 7:  8da6c96b39 =  6:  f6ef6d245c t0020: don't use `test_must_fail has_cr`
 8:  27550eaae6 =  7:  fc32b8d584 t0020: use ! check_packed_refs_marked
 9:  c19f6344a4 =  8:  7e29b0154e t1306: convert `test_might_fail rm` to `rm -f`
10:  d6ea8a6df0 =  9:  cf43579d65 t1307: reorder `nongit test_must_fail`
11:  d57dfe95e2 = 10:  e2fe278bfa t1409: let sed open its own input file
12:  eacf4e0fb4 = 11:  1eef3f4bc5 t1409: use test_path_is_missing()
13:  83e47748bc ! 12:  fddd224225 t1501: remove use of `test_might_fail cp`
    @@ Commit message
     
         The test_must_fail() family of functions (including test_might_fail())
         should only be used on git commands. Replace test_might_fail() with
    -    test_non_git_might_fail().
    +    a compound command wrapping the old cp invocation that always returns 0.
     
         The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix
         test with split index, 2015-03-24). It is necessary because there might
         exist some index files in `repo.git/sharedindex.*` and, if they exist,
         we want to copy them over. However, if they don't exist, we don't want
         to error out because we expect that possibility. As a result, we want to
    -    keep the "might fail" semantics so we use test_non_git_might_fail().
    +    keep the "might fail" semantics so we always return 0, even if the
    +    underlying cp errors out.
     
      ## t/t1501-work-tree.sh ##
     @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' '
    @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' '
      	mkdir -p repo.git/repos/foo &&
      	cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
     -	test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
    -+	test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
    ++	{ cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&
      	sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE
      '
      
14:  9e20865f94 = 13:  63ca18207d t1507: stop losing return codes of git commands
15:  7c61ac6b67 = 14:  44a410d57a t1507: run commands within test_expect_success
16:  d09370455f = 15:  4fe445279b t1507: inline full_name()
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v3 01/15] t/lib-git-p4: use test_path_is_missing()
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 02/15] t0000: replace test_must_fail with run_sub_test_lib_test_err() Denton Liu
                       ` (13 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

Previously, cleanup_git() would use `test_must_fail test -d` to ensure
that the directory is removed. However, test_must_fail should only be
used for git commands. Use test_path_is_missing() instead to check that
the directory has been removed.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/lib-git-p4.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 547b9f88e1..5aff2abe8b 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -175,7 +175,7 @@ stop_and_cleanup_p4d () {
 
 cleanup_git () {
 	retry_until_success rm -r "$git"
-	test_must_fail test -d "$git" &&
+	test_path_is_missing "$git" &&
 	retry_until_success mkdir "$git"
 }
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 02/15] t0000: replace test_must_fail with run_sub_test_lib_test_err()
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
  2019-12-20 18:15     ` [PATCH v3 01/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 03/15] t0003: use named parameters in attr_check() Denton Liu
                       ` (12 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. We use
test_must_fail to test run_sub_test_lib_test() but that function does
not invoke any git commands internally. Even better, we have a function
that's exactly meant to be used when we expect to have a failing test
suite: run_sub_test_lib_test_err()!

Replace `test_must_fail run_sub_test_lib_test` with
`run_sub_test_lib_test_err`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0000-basic.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 8a81a249d0..3e440c078d 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' "
 "
 
 test_expect_success 'pretend we have a partially passing test suite' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		partial-pass '2/3 tests passing' <<-\\EOF &&
 	test_expect_success 'passing test #1' 'true'
 	test_expect_success 'failing test #2' 'false'
@@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su
 "
 
 test_expect_success 'pretend we have a pass, fail, and known breakage' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		mixed-results1 'mixed results #1' <<-\\EOF &&
 	test_expect_success 'passing test' 'true'
 	test_expect_success 'failing test' 'false'
@@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' "
 "
 
 test_expect_success 'pretend we have a mix of all possible results' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		mixed-results2 'mixed results #2' <<-\\EOF &&
 	test_expect_success 'passing test' 'true'
 	test_expect_success 'passing test' 'true'
@@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' "
 "
 
 test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		t1234-verbose "test verbose" --verbose <<-\EOF &&
 	test_expect_success "passing test" true
 	test_expect_success "test with output" "echo foo"
@@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' '
 '
 
 test_expect_success 'test --verbose-only' '
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		t2345-verbose-only-2 "test verbose-only=2" \
 		--verbose-only=2 <<-\EOF &&
 	test_expect_success "passing test" true
@@ -834,7 +834,7 @@ then
 fi
 
 test_expect_success 'tests clean up even on failures' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		touch clean-after-failure &&
@@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' "
 "
 
 test_expect_success 'test_atexit is run' "
-	test_must_fail run_sub_test_lib_test \
+	run_sub_test_lib_test_err \
 		atexit-cleanup 'Run atexit commands' -i <<-\\EOF &&
 	test_expect_success 'tests clean up even after a failure' '
 		> ../../clean-atexit &&
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 03/15] t0003: use named parameters in attr_check()
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
  2019-12-20 18:15     ` [PATCH v3 01/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
  2019-12-20 18:15     ` [PATCH v3 02/15] t0000: replace test_must_fail with run_sub_test_lib_test_err() Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 04/15] t0003: use test_must_be_empty() Denton Liu
                       ` (11 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

We had named the parameters in attr_check() but $2 was being used
instead of $expect. Make all variable accesses in attr_check() use named
variables instead of numbered arguments for clarity.

While we're at it, add variable assignments to the &&-chain. These
aren't ever expected to fail but if a future developer ever adds some
code above the assignments and they could fail in some way, the intact
&&-chain will ensure that the failure is caught.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 71e63d8b50..3569bef75d 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -5,19 +5,16 @@ test_description=gitattributes
 . ./test-lib.sh
 
 attr_check () {
-	path="$1" expect="$2"
+	path="$1" expect="$2" git_opts="$3" &&
 
-	git $3 check-attr test -- "$path" >actual 2>err &&
-	echo "$path: test: $2" >expect &&
+	git $git_opts check-attr test -- "$path" >actual 2>err &&
+	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual &&
 	test_line_count = 0 err
 }
 
 attr_check_quote () {
-
-	path="$1"
-	quoted_path="$2"
-	expect="$3"
+	path="$1" quoted_path="$2" expect="$3" &&
 
 	git check-attr test -- "$path" >actual &&
 	echo "\"$quoted_path\": test: $expect" >expect &&
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 04/15] t0003: use test_must_be_empty()
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (2 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 03/15] t0003: use named parameters in attr_check() Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 05/15] t0003: don't use `test_must_fail attr_check` Denton Liu
                       ` (10 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In several places, we used `test_line_count = 0` to check for an empty
file. Although this is correct, it's overkill. Use test_must_be_empty()
instead because it's more suited for this purpose.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 3569bef75d..c30c736d3f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -10,7 +10,7 @@ attr_check () {
 	git $git_opts check-attr test -- "$path" >actual 2>err &&
 	echo "$path: test: $expect" >expect &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 }
 
 attr_check_quote () {
@@ -241,7 +241,7 @@ EOF
 	git check-attr foo -- "a/b/f" >>actual 2>>err &&
 	git check-attr foo -- "a/b/c/f" >>actual 2>>err &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success '"**" with no slashes test' '
@@ -262,7 +262,7 @@ EOF
 	git check-attr foo -- "a/b/f" >>actual 2>>err &&
 	git check-attr foo -- "a/b/c/f" >>actual 2>>err &&
 	test_cmp expect actual &&
-	test_line_count = 0 err
+	test_must_be_empty err
 '
 
 test_expect_success 'using --git-dir and --work-tree' '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 05/15] t0003: don't use `test_must_fail attr_check`
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (3 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 04/15] t0003: use test_must_be_empty() Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 06/15] t0020: don't use `test_must_fail has_cr` Denton Liu
                       ` (9 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In an effort to remove test_must_fail for all invocations not related to
git or test-tool, replace invocations of `test_must_fail attr_check`
with a plain attr_check call with the $expect argument set to the
actual value output by git.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0003-attributes.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index c30c736d3f..b660593c20 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -24,7 +24,7 @@ attr_check_quote () {
 
 test_expect_success 'open-quoted pathname' '
 	echo "\"a test=a" >.gitattributes &&
-	test_must_fail attr_check a a
+	attr_check a unspecified
 '
 
 
@@ -109,20 +109,20 @@ test_expect_success 'attribute test' '
 
 test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' '
 
-	test_must_fail attr_check F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/c/F f "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/G a/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/B/g a/b/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/G a/b/g "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/H a/b/h "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=0" &&
-	test_must_fail attr_check oNoFf unset "-c core.ignorecase=0" &&
-	test_must_fail attr_check oFfOn set "-c core.ignorecase=0" &&
+	attr_check F unspecified "-c core.ignorecase=0" &&
+	attr_check a/F unspecified "-c core.ignorecase=0" &&
+	attr_check a/c/F unspecified "-c core.ignorecase=0" &&
+	attr_check a/G unspecified "-c core.ignorecase=0" &&
+	attr_check a/B/g a/g "-c core.ignorecase=0" &&
+	attr_check a/b/G unspecified "-c core.ignorecase=0" &&
+	attr_check a/b/H unspecified "-c core.ignorecase=0" &&
+	attr_check a/b/D/g a/g "-c core.ignorecase=0" &&
+	attr_check oNoFf unspecified "-c core.ignorecase=0" &&
+	attr_check oFfOn unspecified "-c core.ignorecase=0" &&
 	attr_check NO unspecified "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/b/D/NO unspecified "-c core.ignorecase=0" &&
 	attr_check a/b/d/YES a/b/d/* "-c core.ignorecase=0" &&
-	test_must_fail attr_check a/E/f "A/e/F" "-c core.ignorecase=0"
+	attr_check a/E/f f "-c core.ignorecase=0"
 
 '
 
@@ -146,8 +146,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase
 '
 
 test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' '
-	test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" &&
-	test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" &&
+	attr_check a/B/D/g a/g "-c core.ignorecase=0" &&
+	attr_check A/B/D/NO unspecified "-c core.ignorecase=0" &&
 	attr_check A/b/h a/b/h "-c core.ignorecase=1" &&
 	attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=1" &&
 	attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=1"
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 06/15] t0020: don't use `test_must_fail has_cr`
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (4 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 05/15] t0003: don't use `test_must_fail attr_check` Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 07/15] t0020: use ! check_packed_refs_marked Denton Liu
                       ` (8 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since has_cr() just
wraps a tr and grep pipeline, replace `test_must_fail has_cr` with
`! has_cr`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t0020-crlf.sh | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 854da0ae16..b63ba62e5d 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' '
 	rm -f tmp one dir/two three &&
 	git config core.autocrlf input &&
 	git read-tree --reset -u HEAD &&
-	test_must_fail has_cr one &&
-	test_must_fail has_cr dir/two &&
+	! has_cr one &&
+	! has_cr dir/two &&
 	git update-index -- one dir/two &&
 	test "$one" = $(git hash-object --stdin <one) &&
 	test "$two" = $(git hash-object --stdin <dir/two) &&
@@ -237,9 +237,9 @@ test_expect_success '.gitattributes says two is binary' '
 	git config core.autocrlf true &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr dir/two &&
+	! has_cr dir/two &&
 	verbose has_cr one &&
-	test_must_fail has_cr three
+	! has_cr three
 '
 
 test_expect_success '.gitattributes says two is input' '
@@ -248,7 +248,7 @@ test_expect_success '.gitattributes says two is input' '
 	echo "two crlf=input" >.gitattributes &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr dir/two
+	! has_cr dir/two
 '
 
 test_expect_success '.gitattributes says two and three are text' '
@@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' '
 	rm -rf tmp one dir .gitattributes patch.file three &&
 	git read-tree --reset -u HEAD &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' '
 	git read-tree --reset HEAD &&
 	git checkout-index -f -q -u -a &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' '
 	git checkout-index -u .gitattributes &&
 	git checkout-index -u one dir/two three &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
@@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' '
 	git checkout-index -u one dir/two three &&
 	git checkout-index -u .gitattributes &&
 
-	test_must_fail has_cr one &&
+	! has_cr one &&
 	verbose has_cr three
 '
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 07/15] t0020: use ! check_packed_refs_marked
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (5 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 06/15] t0020: don't use `test_must_fail has_cr` Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 08/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
                       ` (7 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail function should only be used for git commands since
we should assume that external commands work sanely. Since
check_packed_refs_marked() just wraps a grep invocation, replace
`test_must_fail check_packed_refs_marked` with
`! check_packed_refs_marked`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index e5cb8a252d..c46848eb8e 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' '
 	git for-each-ref >actual &&
 	test_cmp expected actual &&
 	git pack-refs --all &&
-	test_must_fail check_packed_refs_marked &&
+	! check_packed_refs_marked &&
 	git for-each-ref >actual2 &&
 	test_cmp expected actual2
 '
@@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' '
 	git pack-refs --all &&
 	mark_packed_refs &&
 	git update-ref -d refs/heads/packed-delete &&
-	test_must_fail check_packed_refs_marked
+	! check_packed_refs_marked
 '
 
 test_expect_success 'leave packed-refs untouched on update of loose' '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 08/15] t1306: convert `test_might_fail rm` to `rm -f`
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (6 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 07/15] t0020: use ! check_packed_refs_marked Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 09/15] t1307: reorder `nongit test_must_fail` Denton Liu
                       ` (6 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail() family of functions (including test_might_fail())
should only be used on git commands. Replace `test_might_fail rm` with
`rm -f` so that we don't use `test_might_fail` on a non-git command.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1306-xdg-files.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh
index 21e139a313..dd87b43be1 100755
--- a/t/t1306-xdg-files.sh
+++ b/t/t1306-xdg-files.sh
@@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files'
 
 
 test_expect_success 'Checking attributes in a non-XDG global attributes file' '
-	test_might_fail rm .gitattributes &&
+	rm -f .gitattributes &&
 	echo "f attr_f=test" >"$HOME"/my_gitattributes &&
 	git config core.attributesfile "$HOME"/my_gitattributes &&
 	echo "f: attr_f: test" >expected &&
@@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' '
 test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' '
 	mkdir -p "$HOME"/.config/git &&
 	>"$HOME"/.config/git/config &&
-	test_might_fail rm "$HOME"/.gitconfig &&
+	rm -f "$HOME"/.gitconfig &&
 	git config --global user.name "write_config" &&
 	echo "[user]" >expected &&
 	echo "	name = write_config" >>expected &&
@@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' '
 
 
 test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' '
-	test_might_fail rm "$HOME"/.gitconfig &&
-	test_might_fail rm "$HOME"/.config/git/config &&
+	rm -f "$HOME"/.gitconfig &&
+	rm -f "$HOME"/.config/git/config &&
 	git config --global user.name "write_gitconfig" &&
 	echo "[user]" >expected &&
 	echo "	name = write_gitconfig" >>expected &&
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 09/15] t1307: reorder `nongit test_must_fail`
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (7 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 08/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 10/15] t1409: let sed open its own input file Denton Liu
                       ` (5 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In the future, we plan on only allowing `test_must_fail` to work on a
restricted subset of commands, including `git`. Reorder the commands so
that `nongit` comes before `test_must_fail`. This way, `test_must_fail`
operates on a git command.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1307-config-blob.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
index 37dc689d8c..002e6d3388 100755
--- a/t/t1307-config-blob.sh
+++ b/t/t1307-config-blob.sh
@@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' '
 '
 
 test_expect_success 'config --blob outside of a repository is an error' '
-	test_must_fail nongit git config --blob=foo --list
+	nongit test_must_fail git config --blob=foo --list
 '
 
 test_done
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 10/15] t1409: let sed open its own input file
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (8 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 09/15] t1307: reorder `nongit test_must_fail` Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 11/15] t1409: use test_path_is_missing() Denton Liu
                       ` (4 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

In one case, we were using a redirection operator to feed input into
sed. However, since sed is capable of opening its own input file, make
sed do that instead of redirecting input into it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index c46848eb8e..f74d890e82 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily'
 # shouldn't upset readers, and it should be omitted if the file is
 # ever rewritten.
 mark_packed_refs () {
-	sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new &&
+	sed -e "s/^\(#.*\)/\1 t1409 /" .git/packed-refs >.git/packed-refs.new &&
 	mv .git/packed-refs.new .git/packed-refs
 }
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 11/15] t1409: use test_path_is_missing()
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (9 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 10/15] t1409: let sed open its own input file Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:15     ` [PATCH v3 12/15] t1501: remove use of `test_might_fail cp` Denton Liu
                       ` (3 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The test_must_fail() function should only be used for git commands since
we should assume that external commands work sanely. Replace
`test_must_fail test -f` with `test_path_is_missing` since we expect
these paths to not exist.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1409-avoid-packing-refs.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index f74d890e82..be12fb6350 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -27,15 +27,15 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'do not create packed-refs file gratuitously' '
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $A &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $B &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref refs/heads/foo $C $B &&
-	test_must_fail test -f .git/packed-refs &&
+	test_path_is_missing .git/packed-refs &&
 	git update-ref -d refs/heads/foo &&
-	test_must_fail test -f .git/packed-refs
+	test_path_is_missing .git/packed-refs
 '
 
 test_expect_success 'check that marking the packed-refs file works' '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 12/15] t1501: remove use of `test_might_fail cp`
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (10 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 11/15] t1409: use test_path_is_missing() Denton Liu
@ 2019-12-20 18:15     ` Denton Liu
  2019-12-20 18:16     ` [PATCH v3 13/15] t1507: stop losing return codes of git commands Denton Liu
                       ` (2 subsequent siblings)
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:15 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

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 cp invocation that always returns 0.

The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix
test with split index, 2015-03-24). It is necessary because there might
exist some index files in `repo.git/sharedindex.*` and, if they exist,
we want to copy them over. However, if they don't exist, we don't want
to error out because we expect that possibility. As a result, we want to
keep the "might fail" semantics so we always return 0, even if the
underlying cp errors out.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1501-work-tree.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
index 3498d3d55e..b75558040f 100755
--- a/t/t1501-work-tree.sh
+++ b/t/t1501-work-tree.sh
@@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
 	mkdir work &&
 	mkdir -p repo.git/repos/foo &&
 	cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
-	test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
+	{ cp repo.git/sharedindex.* repo.git/repos/foo || :; } &&
 	sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE
 '
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 13/15] t1507: stop losing return codes of git commands
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (11 preceding siblings ...)
  2019-12-20 18:15     ` [PATCH v3 12/15] t1501: remove use of `test_might_fail cp` Denton Liu
@ 2019-12-20 18:16     ` Denton Liu
  2019-12-20 18:16     ` [PATCH v3 14/15] t1507: run commands within test_expect_success Denton Liu
  2019-12-20 18:16     ` [PATCH v3 15/15] t1507: inline full_name() Denton Liu
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:16 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The return code of git commands are lost when a command is in a
non-assignment command substitution in favour of the surrounding
command's. Rewrite instances of this so that git commands run
on their own.

In commit_subject(), use a `tformat` instead of `format` since,
previously, we were testing the output of a command substitution which
didn't care if there was a trailing newline since it was automatically
stripped. Since we use test_cmp() now, the trailing newline matters so
use `tformat` to always output it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 45 +++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index 8b4cf8a6e3..d81f289ace 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -35,7 +35,7 @@ full_name () {
 
 commit_subject () {
 	(cd clone &&
-	 git show -s --pretty=format:%s "$@")
+	 git show -s --pretty=tformat:%s "$@")
 }
 
 error_message () {
@@ -44,18 +44,27 @@ error_message () {
 }
 
 test_expect_success '@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{upstream})" &&
-	test refs/remotes/origin/master = "$(full_name @{UPSTREAM})" &&
-	test refs/remotes/origin/master = "$(full_name @{UpSTReam})"
+	echo refs/remotes/origin/master >expect &&
+	full_name @{upstream} >actual &&
+	test_cmp expect actual &&
+	full_name @{UPSTREAM} >actual &&
+	test_cmp expect actual &&
+	full_name @{UpSTReam} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{u} resolves to correct full name' '
-	test refs/remotes/origin/master = "$(full_name @{u})" &&
-	test refs/remotes/origin/master = "$(full_name @{U})"
+	echo refs/remotes/origin/master >expect &&
+	full_name @{u} >actual &&
+	test_cmp expect actual &&
+	full_name @{U} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
-	test refs/remotes/origin/side = "$(full_name my-side@{u})"
+	echo refs/remotes/origin/side >expect &&
+	full_name my-side@{u} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ in middle' '
@@ -86,8 +95,11 @@ test_expect_success 'my-side@{u} resolves to correct commit' '
 	git checkout side &&
 	test_commit 5 &&
 	(cd clone && git fetch) &&
-	test 2 = "$(commit_subject my-side)" &&
-	test 5 = "$(commit_subject my-side@{u})"
+	echo 2 >expect &&
+	commit_subject my-side >actual &&
+	test_cmp expect actual &&
+	echo 5 >expect &&
+	commit_subject my-side@{u} >actual
 '
 
 test_expect_success 'not-tracking@{u} fails' '
@@ -99,8 +111,11 @@ test_expect_success 'not-tracking@{u} fails' '
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
 	test_commit 6 &&
 	(cd clone && git fetch) &&
-	test 5 = $(commit_subject my-side@{u}@{1}) &&
-	test 5 = $(commit_subject my-side@{U}@{1})
+	echo 5 >expect &&
+	commit_subject my-side@{u}@{1} >actual &&
+	test_cmp expect actual &&
+	commit_subject my-side@{U}@{1} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success '@{u} without specifying branch fails on a detached HEAD' '
@@ -149,7 +164,9 @@ test_expect_success 'checkout other@{u}' '
 '
 
 test_expect_success 'branch@{u} works when tracking a local branch' '
-	test refs/heads/master = "$(full_name local-master@{u})"
+	echo refs/heads/master >expect &&
+	full_name local-master@{u} >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'branch@{u} error message when no upstream' '
@@ -203,7 +220,9 @@ test_expect_success 'pull works when tracking a local branch' '
 
 # makes sense if the previous one succeeded
 test_expect_success '@{u} works when tracking a local branch' '
-	test refs/heads/master = "$(full_name @{u})"
+	echo refs/heads/master >expect &&
+	full_name @{u} >actual &&
+	test_cmp expect actual
 '
 
 commit=$(git rev-parse HEAD)
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 14/15] t1507: run commands within test_expect_success
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (12 preceding siblings ...)
  2019-12-20 18:16     ` [PATCH v3 13/15] t1507: stop losing return codes of git commands Denton Liu
@ 2019-12-20 18:16     ` Denton Liu
  2019-12-20 18:16     ` [PATCH v3 15/15] t1507: inline full_name() Denton Liu
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:16 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

The expected test style is to have all commands tested within a
test_expect_success block. Move the generation of the 'expect' text into
their corresponding blocks. While we're at it, insert a second
`commit=$(git rev-parse HEAD)` into the next test case so that it's
clear where $commit is coming from.

The biggest advantage of doing this is that we now check the return code
of `git rev-parse HEAD` so we can catch it in case it fails.

This patch is best viewed with `--color-moved --ignore-all-space`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 40 +++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index d81f289ace..f68b77e7ba 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -225,32 +225,32 @@ test_expect_success '@{u} works when tracking a local branch' '
 	test_cmp expect actual
 '
 
-commit=$(git rev-parse HEAD)
-cat >expect <<EOF
-commit $commit
-Reflog: master@{0} (C O Mitter <committer@example.com>)
-Reflog message: branch: Created from HEAD
-Author: A U Thor <author@example.com>
-Date:   Thu Apr 7 15:15:13 2005 -0700
-
-    3
-EOF
 test_expect_success 'log -g other@{u}' '
+	commit=$(git rev-parse HEAD) &&
+	cat >expect <<-EOF &&
+	commit $commit
+	Reflog: master@{0} (C O Mitter <committer@example.com>)
+	Reflog message: branch: Created from HEAD
+	Author: A U Thor <author@example.com>
+	Date:   Thu Apr 7 15:15:13 2005 -0700
+
+	    3
+	EOF
 	git log -1 -g other@{u} >actual &&
 	test_cmp expect actual
 '
 
-cat >expect <<EOF
-commit $commit
-Reflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter <committer@example.com>)
-Reflog message: branch: Created from HEAD
-Author: A U Thor <author@example.com>
-Date:   Thu Apr 7 15:15:13 2005 -0700
-
-    3
-EOF
-
 test_expect_success 'log -g other@{u}@{now}' '
+	commit=$(git rev-parse HEAD) &&
+	cat >expect <<-EOF &&
+	commit $commit
+	Reflog: master@{Thu Apr 7 15:17:13 2005 -0700} (C O Mitter <committer@example.com>)
+	Reflog message: branch: Created from HEAD
+	Author: A U Thor <author@example.com>
+	Date:   Thu Apr 7 15:15:13 2005 -0700
+
+	    3
+	EOF
 	git log -1 -g other@{u}@{now} >actual &&
 	test_cmp expect actual
 '
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

* [PATCH v3 15/15] t1507: inline full_name()
  2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
                       ` (13 preceding siblings ...)
  2019-12-20 18:16     ` [PATCH v3 14/15] t1507: run commands within test_expect_success Denton Liu
@ 2019-12-20 18:16     ` Denton Liu
  14 siblings, 0 replies; 64+ messages in thread
From: Denton Liu @ 2019-12-20 18:16 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Eric Sunshine, Johannes Sixt, Junio C Hamano

Before, we were running `test_must_fail full_name`. However,
`test_must_fail` should only be used on git commands. Inline full_name()
so that we can use test_must_fail on the git command directly.

When full_name() was introduced in 28fb84382b (Introduce
<branch>@{upstream} notation, 2009-09-10), the `git -C` option wasn't
available yet (since it was introduced in 44e1e4d67d (git: run in a
directory given with -C option, 2013-09-09)). As a result, the helper
function removed the need to manually cd each time. However, since
`git -C` is available now, we can just use that instead and inline
full_name().

An alternate approach was taken where we taught full_name() to accept an
optional `!` arg to trigger test_must_fail behavior. However, this added
more unnecessary complexity than inlining so we inline instead.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1507-rev-parse-upstream.sh | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh
index f68b77e7ba..dfc0d96d8a 100755
--- a/t/t1507-rev-parse-upstream.sh
+++ b/t/t1507-rev-parse-upstream.sh
@@ -28,11 +28,6 @@ test_expect_success 'setup' '
 	)
 '
 
-full_name () {
-	(cd clone &&
-	 git rev-parse --symbolic-full-name "$@")
-}
-
 commit_subject () {
 	(cd clone &&
 	 git show -s --pretty=tformat:%s "$@")
@@ -45,50 +40,50 @@ error_message () {
 
 test_expect_success '@{upstream} resolves to correct full name' '
 	echo refs/remotes/origin/master >expect &&
-	full_name @{upstream} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{upstream} >actual &&
 	test_cmp expect actual &&
-	full_name @{UPSTREAM} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{UPSTREAM} >actual &&
 	test_cmp expect actual &&
-	full_name @{UpSTReam} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{UpSTReam} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '@{u} resolves to correct full name' '
 	echo refs/remotes/origin/master >expect &&
-	full_name @{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{u} >actual &&
 	test_cmp expect actual &&
-	full_name @{U} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{U} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'my-side@{upstream} resolves to correct full name' '
 	echo refs/remotes/origin/side >expect &&
-	full_name my-side@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name my-side@{u} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ in middle' '
-	full_name fun@ny@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name fun@ny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
 	test_cmp expect actual &&
-	full_name fun@ny@{U} >actual &&
+	git -C clone rev-parse --symbolic-full-name fun@ny@{U} >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ at start' '
-	full_name @funny@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name @funny@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'upstream of branch with @ at end' '
-	full_name funny@@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name funny@@{u} >actual &&
 	echo refs/remotes/origin/side >expect &&
 	test_cmp expect actual
 '
 
 test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' '
-	test_must_fail full_name refs/heads/my-side@{upstream}
+	test_must_fail git -C clone rev-parse --symbolic-full-name refs/heads/my-side@{upstream}
 '
 
 test_expect_success 'my-side@{u} resolves to correct commit' '
@@ -103,9 +98,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' '
 '
 
 test_expect_success 'not-tracking@{u} fails' '
-	test_must_fail full_name non-tracking@{u} &&
+	test_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u} &&
 	(cd clone && git checkout --no-track -b non-tracking) &&
-	test_must_fail full_name non-tracking@{u}
+	test_must_fail git -C clone rev-parse --symbolic-full-name non-tracking@{u}
 '
 
 test_expect_success '<branch>@{u}@{1} resolves correctly' '
@@ -165,7 +160,7 @@ test_expect_success 'checkout other@{u}' '
 
 test_expect_success 'branch@{u} works when tracking a local branch' '
 	echo refs/heads/master >expect &&
-	full_name local-master@{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name local-master@{u} >actual &&
 	test_cmp expect actual
 '
 
@@ -221,7 +216,7 @@ test_expect_success 'pull works when tracking a local branch' '
 # makes sense if the previous one succeeded
 test_expect_success '@{u} works when tracking a local branch' '
 	echo refs/heads/master >expect &&
-	full_name @{u} >actual &&
+	git -C clone rev-parse --symbolic-full-name @{u} >actual &&
 	test_cmp expect actual
 '
 
-- 
2.24.1.703.g2f499f1283


^ permalink raw reply related	[flat|nested] 64+ messages in thread

end of thread, other threads:[~2019-12-20 18:14 UTC | newest]

Thread overview: 64+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-17 12:01 [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
2019-12-17 12:01 ` [PATCH 01/15] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
2019-12-17 12:01 ` [PATCH 02/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
2019-12-17 12:01 ` [PATCH 03/15] t0000: replace test_must_fail with ! for run_sub_test_lib_test() Denton Liu
2019-12-17 20:23   ` Johannes Sixt
2019-12-17 12:01 ` [PATCH 04/15] t0003: use named parameters in attr_check() Denton Liu
2019-12-17 18:35   ` Junio C Hamano
2019-12-17 18:43   ` Eric Sunshine
2019-12-17 12:01 ` [PATCH 05/15] t0003: use test_must_be_empty() Denton Liu
2019-12-17 12:01 ` [PATCH 06/15] t0003: don't use `test_must_fail attr_check` Denton Liu
2019-12-17 18:44   ` Junio C Hamano
2019-12-17 12:01 ` [PATCH 07/15] t0020: drop redirections to /dev/null Denton Liu
2019-12-17 19:32   ` Eric Sunshine
2019-12-17 12:01 ` [PATCH 08/15] t0020: s/test_must_fail has_cr/! has_cr/ Denton Liu
2019-12-17 19:36   ` Eric Sunshine
2019-12-17 12:01 ` [PATCH 09/15] t0020: use ! check_packed_refs_marked Denton Liu
2019-12-17 12:01 ` [PATCH 10/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
2019-12-17 18:55   ` Junio C Hamano
2019-12-17 12:01 ` [PATCH 11/15] t1307: reorder `nongit test_must_fail` Denton Liu
2019-12-17 12:01 ` [PATCH 12/15] t1409: let sed open its own files Denton Liu
2019-12-17 18:57   ` Junio C Hamano
2019-12-17 12:01 ` [PATCH 13/15] t1409: use test_path_is_missing() Denton Liu
2019-12-17 19:01   ` Junio C Hamano
2019-12-17 12:01 ` [PATCH 14/15] t1501: remove use of `test_might_fail cp` Denton Liu
2019-12-17 19:46   ` Eric Sunshine
2019-12-17 12:01 ` [PATCH 15/15] t1507: teach full_name() to accept `!` arg Denton Liu
2019-12-17 19:54   ` Eric Sunshine
2019-12-17 18:28 ` [PATCH 00/15] t: replace incorrect test_must_fail usage (part 1) Junio C Hamano
2019-12-19 22:22 ` [PATCH v2 00/16] " Denton Liu
2019-12-19 22:22   ` [PATCH v2 01/16] test-lib-functions: introduce test_non_git_might_fail() Denton Liu
2019-12-19 22:22   ` [PATCH v2 02/16] t/lib-git-p4: use test_path_is_missing() Denton Liu
2019-12-19 22:22   ` [PATCH v2 03/16] t0000: replace test_must_fail with run_sub_test_lib_test_err() Denton Liu
2019-12-19 22:22   ` [PATCH v2 04/16] t0003: use named parameters in attr_check() Denton Liu
2019-12-19 22:22   ` [PATCH v2 05/16] t0003: use test_must_be_empty() Denton Liu
2019-12-19 22:22   ` [PATCH v2 06/16] t0003: don't use `test_must_fail attr_check` Denton Liu
2019-12-19 22:22   ` [PATCH v2 07/16] t0020: don't use `test_must_fail has_cr` Denton Liu
2019-12-19 22:22   ` [PATCH v2 08/16] t0020: use ! check_packed_refs_marked Denton Liu
2019-12-19 22:22   ` [PATCH v2 09/16] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
2019-12-19 22:22   ` [PATCH v2 10/16] t1307: reorder `nongit test_must_fail` Denton Liu
2019-12-19 22:22   ` [PATCH v2 11/16] t1409: let sed open its own input file Denton Liu
2019-12-19 22:22   ` [PATCH v2 12/16] t1409: use test_path_is_missing() Denton Liu
2019-12-19 22:22   ` [PATCH v2 13/16] t1501: remove use of `test_might_fail cp` Denton Liu
2019-12-19 22:52     ` Eric Sunshine
2019-12-19 23:19       ` Denton Liu
2019-12-20 17:32         ` Eric Sunshine
2019-12-19 22:22   ` [PATCH v2 14/16] t1507: stop losing return codes of git commands Denton Liu
2019-12-19 22:22   ` [PATCH v2 15/16] t1507: run commands within test_expect_success Denton Liu
2019-12-19 22:22   ` [PATCH v2 16/16] t1507: inline full_name() Denton Liu
2019-12-20 18:15   ` [PATCH v3 00/15] t: replace incorrect test_must_fail usage (part 1) Denton Liu
2019-12-20 18:15     ` [PATCH v3 01/15] t/lib-git-p4: use test_path_is_missing() Denton Liu
2019-12-20 18:15     ` [PATCH v3 02/15] t0000: replace test_must_fail with run_sub_test_lib_test_err() Denton Liu
2019-12-20 18:15     ` [PATCH v3 03/15] t0003: use named parameters in attr_check() Denton Liu
2019-12-20 18:15     ` [PATCH v3 04/15] t0003: use test_must_be_empty() Denton Liu
2019-12-20 18:15     ` [PATCH v3 05/15] t0003: don't use `test_must_fail attr_check` Denton Liu
2019-12-20 18:15     ` [PATCH v3 06/15] t0020: don't use `test_must_fail has_cr` Denton Liu
2019-12-20 18:15     ` [PATCH v3 07/15] t0020: use ! check_packed_refs_marked Denton Liu
2019-12-20 18:15     ` [PATCH v3 08/15] t1306: convert `test_might_fail rm` to `rm -f` Denton Liu
2019-12-20 18:15     ` [PATCH v3 09/15] t1307: reorder `nongit test_must_fail` Denton Liu
2019-12-20 18:15     ` [PATCH v3 10/15] t1409: let sed open its own input file Denton Liu
2019-12-20 18:15     ` [PATCH v3 11/15] t1409: use test_path_is_missing() Denton Liu
2019-12-20 18:15     ` [PATCH v3 12/15] t1501: remove use of `test_might_fail cp` Denton Liu
2019-12-20 18:16     ` [PATCH v3 13/15] t1507: stop losing return codes of git commands Denton Liu
2019-12-20 18:16     ` [PATCH v3 14/15] t1507: run commands within test_expect_success Denton Liu
2019-12-20 18:16     ` [PATCH v3 15/15] t1507: inline full_name() Denton Liu

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 NNTP newsgroup(s).