* [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
* 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 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
* [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