The overall scope of these patches is to replace inappropriate uses of test_must_fail. IOW, we should only allow test_must_fail to run on `git` and `test-tool`. Ultimately, we will conclude by making test_must_fail error out on non-git commands. An advance view of the final series can be found here[1]. This is the first part. It focuses on t[01]*.sh and also t/lib-git-p4. [1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2 Denton Liu (15): test-lib-functions: introduce test_non_git_might_fail() t/lib-git-p4: use test_path_is_missing() t0000: replace test_must_fail with ! for run_sub_test_lib_test() t0003: use named parameters in attr_check() t0003: use test_must_be_empty() t0003: don't use `test_must_fail attr_check` t0020: drop redirections to /dev/null t0020: s/test_must_fail has_cr/! has_cr/ t0020: use ! check_packed_refs_marked t1306: convert `test_might_fail rm` to `rm -f` t1307: reorder `nongit test_must_fail` t1409: let sed open its own files t1409: use test_path_is_missing() t1501: remove use of `test_might_fail cp` t1507: teach full_name() to accept `!` arg t/lib-git-p4.sh | 2 +- t/t0000-basic.sh | 14 +++++----- t/t0003-attributes.sh | 51 ++++++++++++++++++----------------- t/t0020-crlf.sh | 20 +++++++------- t/t1306-xdg-files.sh | 8 +++--- t/t1307-config-blob.sh | 2 +- t/t1409-avoid-packing-refs.sh | 16 +++++------ t/t1501-work-tree.sh | 2 +- t/t1507-rev-parse-upstream.sh | 14 +++++++--- t/test-lib-functions.sh | 9 +++++++ 10 files changed, 77 insertions(+), 61 deletions(-) -- 2.24.0.627.geba02921db
In a future commit, we will be preventing the use of the test_must_fail()-family of functions (including test_might_fail()) on non-git comands. To prep for this, introduce the test_non_git_might_fail() function which is used to replace non-git invocations of test_might_fail(). The test_non_git_might_fail() function is a lightweight replacement, always masking the return status of the command and returning a non-error exit code. Unlike test_might_fail(), it does not check for abnormal exit conditions such as a segv. This is because we are not in the business of checking the sanity of the external environment and we can assume that it works properly. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/test-lib-functions.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 284c52d076..61d27f1ec6 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -891,6 +891,15 @@ test_expect_code () { return 1 } 7>&2 2>&4 +# Similar to test_might_fail, but much simpler. This is intended for use +# with non-git commands that we can assume will work sanely so we don't +# need to check for conditions such as a segv. + +test_non_git_might_fail () { + "$@" 2>&7 + return 0 +} + # test_cmp is a helper function to compare actual and expected output. # You can use it like: # -- 2.24.0.627.geba02921db
Previously, cleanup_git() would use `test_must_fail test -d` to ensure that the directory is removed. However, test_must_fail should only be used for git commands. Use test_path_is_missing() instead to check that the directory has been removed. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/lib-git-p4.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh index 547b9f88e1..5aff2abe8b 100644 --- a/t/lib-git-p4.sh +++ b/t/lib-git-p4.sh @@ -175,7 +175,7 @@ stop_and_cleanup_p4d () { cleanup_git () { retry_until_success rm -r "$git" - test_must_fail test -d "$git" && + test_path_is_missing "$git" && retry_until_success mkdir "$git" } -- 2.24.0.627.geba02921db
The test_must_fail function should only be used for git commands since we should assume that external commands work sanely. We use test_must_fail to test run_sub_test_lib_test() but that function does not invoke any git commands internally. Replace these instances of `test_must_fail` with `!`. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t0000-basic.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 8a81a249d0..d60ad4b78b 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' " " test_expect_success 'pretend we have a partially passing test suite' " - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ partial-pass '2/3 tests passing' <<-\\EOF && test_expect_success 'passing test #1' 'true' test_expect_success 'failing test #2' 'false' @@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su " test_expect_success 'pretend we have a pass, fail, and known breakage' " - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ mixed-results1 'mixed results #1' <<-\\EOF && test_expect_success 'passing test' 'true' test_expect_success 'failing test' 'false' @@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' " " test_expect_success 'pretend we have a mix of all possible results' " - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ mixed-results2 'mixed results #2' <<-\\EOF && test_expect_success 'passing test' 'true' test_expect_success 'passing test' 'true' @@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' " " test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ t1234-verbose "test verbose" --verbose <<-\EOF && test_expect_success "passing test" true test_expect_success "test with output" "echo foo" @@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' ' test_expect_success 'test --verbose-only' ' - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ t2345-verbose-only-2 "test verbose-only=2" \ --verbose-only=2 <<-\EOF && test_expect_success "passing test" true @@ -834,7 +834,7 @@ then fi test_expect_success 'tests clean up even on failures' " - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF && test_expect_success 'tests clean up even after a failure' ' touch clean-after-failure && @@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' " " test_expect_success 'test_atexit is run' " - test_must_fail run_sub_test_lib_test \ + ! run_sub_test_lib_test \ atexit-cleanup 'Run atexit commands' -i <<-\\EOF && test_expect_success 'tests clean up even after a failure' ' > ../../clean-atexit && -- 2.24.0.627.geba02921db
We had named the parameters in attr_check() but $2 was being used instead of $expect. Make all variable accesses in attr_check() use named variables instead of numbered arguments for clarity. While we're at it, add variable assignments to the &&-chain. These aren't ever expected to fail but for stylistic purposes, include them anyway for stylistic purposes. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t0003-attributes.sh | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 71e63d8b50..c47d4cfbcd 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -5,19 +5,20 @@ test_description=gitattributes . ./test-lib.sh attr_check () { - path="$1" expect="$2" + path="$1" && + expect="$2" && + git_opts="$3" && - git $3 check-attr test -- "$path" >actual 2>err && - echo "$path: test: $2" >expect && + git $git_opts check-attr test -- "$path" >actual 2>err && + echo "$path: test: $expect" >expect && test_cmp expect actual && test_line_count = 0 err } attr_check_quote () { - - path="$1" - quoted_path="$2" - expect="$3" + path="$1" && + quoted_path="$2" && + expect="$3" && git check-attr test -- "$path" >actual && echo "\"$quoted_path\": test: $expect" >expect && -- 2.24.0.627.geba02921db
In several places, we used `test_line_count = 0` to check for an empty file. Although this is correct, it's overkill. Use test_must_be_empty() instead because it's more suited for this purpose. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t0003-attributes.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index c47d4cfbcd..53a730e2ee 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -12,7 +12,7 @@ attr_check () { git $git_opts check-attr test -- "$path" >actual 2>err && echo "$path: test: $expect" >expect && test_cmp expect actual && - test_line_count = 0 err + test_must_be_empty err } attr_check_quote () { @@ -245,7 +245,7 @@ EOF git check-attr foo -- "a/b/f" >>actual 2>>err && git check-attr foo -- "a/b/c/f" >>actual 2>>err && test_cmp expect actual && - test_line_count = 0 err + test_must_be_empty err ' test_expect_success '"**" with no slashes test' ' @@ -266,7 +266,7 @@ EOF git check-attr foo -- "a/b/f" >>actual 2>>err && git check-attr foo -- "a/b/c/f" >>actual 2>>err && test_cmp expect actual && - test_line_count = 0 err + test_must_be_empty err ' test_expect_success 'using --git-dir and --work-tree' ' -- 2.24.0.627.geba02921db
In an effort to remove test_must_fail for all invocations not related to git or test-tool, replace invocations of `test_must_fail attr_check` with a plain attr_check call with the $expect argument set to the actual value output by git. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t0003-attributes.sh | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 53a730e2ee..8d4343afdb 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -28,7 +28,7 @@ attr_check_quote () { test_expect_success 'open-quoted pathname' ' echo "\"a test=a" >.gitattributes && - test_must_fail attr_check a a + attr_check a unspecified ' @@ -113,20 +113,20 @@ test_expect_success 'attribute test' ' test_expect_success 'attribute matching is case sensitive when core.ignorecase=0' ' - test_must_fail attr_check F f "-c core.ignorecase=0" && - test_must_fail attr_check a/F f "-c core.ignorecase=0" && - test_must_fail attr_check a/c/F f "-c core.ignorecase=0" && - test_must_fail attr_check a/G a/g "-c core.ignorecase=0" && - test_must_fail attr_check a/B/g a/b/g "-c core.ignorecase=0" && - test_must_fail attr_check a/b/G a/b/g "-c core.ignorecase=0" && - test_must_fail attr_check a/b/H a/b/h "-c core.ignorecase=0" && - test_must_fail attr_check a/b/D/g "a/b/d/*" "-c core.ignorecase=0" && - test_must_fail attr_check oNoFf unset "-c core.ignorecase=0" && - test_must_fail attr_check oFfOn set "-c core.ignorecase=0" && + attr_check F unspecified "-c core.ignorecase=0" && + attr_check a/F unspecified "-c core.ignorecase=0" && + attr_check a/c/F unspecified "-c core.ignorecase=0" && + attr_check a/G unspecified "-c core.ignorecase=0" && + attr_check a/B/g a/g "-c core.ignorecase=0" && + attr_check a/b/G unspecified "-c core.ignorecase=0" && + attr_check a/b/H unspecified "-c core.ignorecase=0" && + attr_check a/b/D/g a/g "-c core.ignorecase=0" && + attr_check oNoFf unspecified "-c core.ignorecase=0" && + attr_check oFfOn unspecified "-c core.ignorecase=0" && attr_check NO unspecified "-c core.ignorecase=0" && - test_must_fail attr_check a/b/D/NO "a/b/d/*" "-c core.ignorecase=0" && + attr_check a/b/D/NO unspecified "-c core.ignorecase=0" && attr_check a/b/d/YES a/b/d/* "-c core.ignorecase=0" && - test_must_fail attr_check a/E/f "A/e/F" "-c core.ignorecase=0" + attr_check a/E/f f "-c core.ignorecase=0" ' @@ -150,8 +150,8 @@ test_expect_success 'attribute matching is case insensitive when core.ignorecase ' test_expect_success CASE_INSENSITIVE_FS 'additional case insensitivity tests' ' - test_must_fail attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=0" && - test_must_fail attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=0" && + attr_check a/B/D/g a/g "-c core.ignorecase=0" && + attr_check A/B/D/NO unspecified "-c core.ignorecase=0" && attr_check A/b/h a/b/h "-c core.ignorecase=1" && attr_check a/B/D/g "a/b/d/*" "-c core.ignorecase=1" && attr_check A/B/D/NO "a/b/d/*" "-c core.ignorecase=1" -- 2.24.0.627.geba02921db
Since output is silenced when running without `-v` and debugging output is useful with `-v`, remove redirections to /dev/null as it is not useful. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t0020-crlf.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh index 854da0ae16..8281babde0 100755 --- a/t/t0020-crlf.sh +++ b/t/t0020-crlf.sh @@ -5,7 +5,7 @@ test_description='CRLF conversion' . ./test-lib.sh has_cr() { - tr '\015' Q <"$1" | grep Q >/dev/null + tr '\015' Q <"$1" | grep Q } # add or remove CRs to disk file in-place -- 2.24.0.627.geba02921db
The test_must_fail function should only be used for git commands since we should assume that external commands work sanely. Since has_cr() just wraps a tr and grep pipeline, replace `test_must_fail has_cr` with `! has_cr`. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t0020-crlf.sh | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh index 8281babde0..4940e05a2e 100755 --- a/t/t0020-crlf.sh +++ b/t/t0020-crlf.sh @@ -159,8 +159,8 @@ test_expect_success 'checkout with autocrlf=input' ' rm -f tmp one dir/two three && git config core.autocrlf input && git read-tree --reset -u HEAD && - test_must_fail has_cr one && - test_must_fail has_cr dir/two && + ! has_cr one && + ! has_cr dir/two && git update-index -- one dir/two && test "$one" = $(git hash-object --stdin <one) && test "$two" = $(git hash-object --stdin <dir/two) && @@ -237,9 +237,9 @@ test_expect_success '.gitattributes says two is binary' ' git config core.autocrlf true && git read-tree --reset -u HEAD && - test_must_fail has_cr dir/two && + ! has_cr dir/two && verbose has_cr one && - test_must_fail has_cr three + ! has_cr three ' test_expect_success '.gitattributes says two is input' ' @@ -248,7 +248,7 @@ test_expect_success '.gitattributes says two is input' ' echo "two crlf=input" >.gitattributes && git read-tree --reset -u HEAD && - test_must_fail has_cr dir/two + ! has_cr dir/two ' test_expect_success '.gitattributes says two and three are text' ' @@ -270,7 +270,7 @@ test_expect_success 'in-tree .gitattributes (1)' ' rm -rf tmp one dir .gitattributes patch.file three && git read-tree --reset -u HEAD && - test_must_fail has_cr one && + ! has_cr one && verbose has_cr three ' @@ -280,7 +280,7 @@ test_expect_success 'in-tree .gitattributes (2)' ' git read-tree --reset HEAD && git checkout-index -f -q -u -a && - test_must_fail has_cr one && + ! has_cr one && verbose has_cr three ' @@ -291,7 +291,7 @@ test_expect_success 'in-tree .gitattributes (3)' ' git checkout-index -u .gitattributes && git checkout-index -u one dir/two three && - test_must_fail has_cr one && + ! has_cr one && verbose has_cr three ' @@ -302,7 +302,7 @@ test_expect_success 'in-tree .gitattributes (4)' ' git checkout-index -u one dir/two three && git checkout-index -u .gitattributes && - test_must_fail has_cr one && + ! has_cr one && verbose has_cr three ' -- 2.24.0.627.geba02921db
The test_must_fail function should only be used for git commands since we should assume that external commands work sanely. Since check_packed_refs_marked() just wraps a grep invocation, replace `test_must_fail check_packed_refs_marked` with `! check_packed_refs_marked`. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1409-avoid-packing-refs.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh index e5cb8a252d..c46848eb8e 100755 --- a/t/t1409-avoid-packing-refs.sh +++ b/t/t1409-avoid-packing-refs.sh @@ -46,7 +46,7 @@ test_expect_success 'check that marking the packed-refs file works' ' git for-each-ref >actual && test_cmp expected actual && git pack-refs --all && - test_must_fail check_packed_refs_marked && + ! check_packed_refs_marked && git for-each-ref >actual2 && test_cmp expected actual2 ' @@ -80,7 +80,7 @@ test_expect_success 'touch packed-refs on delete of packed' ' git pack-refs --all && mark_packed_refs && git update-ref -d refs/heads/packed-delete && - test_must_fail check_packed_refs_marked + ! check_packed_refs_marked ' test_expect_success 'leave packed-refs untouched on update of loose' ' -- 2.24.0.627.geba02921db
The test_must_fail() family of functions (including test_might_fail()) should only be used on git commands. Replace `test_might_fail rm` with `rm -f` so that we don't use `test_might_fail` on a non-git command. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1306-xdg-files.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/t/t1306-xdg-files.sh b/t/t1306-xdg-files.sh index 21e139a313..dd87b43be1 100755 --- a/t/t1306-xdg-files.sh +++ b/t/t1306-xdg-files.sh @@ -153,7 +153,7 @@ test_expect_success 'Checking attributes in both XDG and local attributes files' test_expect_success 'Checking attributes in a non-XDG global attributes file' ' - test_might_fail rm .gitattributes && + rm -f .gitattributes && echo "f attr_f=test" >"$HOME"/my_gitattributes && git config core.attributesfile "$HOME"/my_gitattributes && echo "f: attr_f: test" >expected && @@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' ' test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' ' mkdir -p "$HOME"/.config/git && >"$HOME"/.config/git/config && - test_might_fail rm "$HOME"/.gitconfig && + rm -f "$HOME"/.gitconfig && git config --global user.name "write_config" && echo "[user]" >expected && echo " name = write_config" >>expected && @@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' ' test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' ' - test_might_fail rm "$HOME"/.gitconfig && - test_might_fail rm "$HOME"/.config/git/config && + rm -f "$HOME"/.gitconfig && + rm -f "$HOME"/.config/git/config && git config --global user.name "write_gitconfig" && echo "[user]" >expected && echo " name = write_gitconfig" >>expected && -- 2.24.0.627.geba02921db
In the future, we plan on only allowing `test_must_fail` to work on a restricted subset of commands, including `git`. Reorder the commands so that `nongit` comes before `test_must_fail`. This way, `test_must_fail` operates on a git command. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1307-config-blob.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh index 37dc689d8c..002e6d3388 100755 --- a/t/t1307-config-blob.sh +++ b/t/t1307-config-blob.sh @@ -74,7 +74,7 @@ test_expect_success 'can parse blob ending with CR' ' ' test_expect_success 'config --blob outside of a repository is an error' ' - test_must_fail nongit git config --blob=foo --list + nongit test_must_fail git config --blob=foo --list ' test_done -- 2.24.0.627.geba02921db
In one case, we were using a redirection operator to feed input into sed. However, since sed is capable of opening its own files, make sed open its own files instead of redirecting input into it. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1409-avoid-packing-refs.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh index c46848eb8e..f74d890e82 100755 --- a/t/t1409-avoid-packing-refs.sh +++ b/t/t1409-avoid-packing-refs.sh @@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily' # shouldn't upset readers, and it should be omitted if the file is # ever rewritten. mark_packed_refs () { - sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new && + sed -e "s/^\(#.*\)/\1 t1409 /" .git/packed-refs >.git/packed-refs.new && mv .git/packed-refs.new .git/packed-refs } -- 2.24.0.627.geba02921db
The test_must_fail() function should only be used for git commands since we should assume that external commands work sanely. Replace `test_must_fail test -f` with `test_path_is_missing` since we expect these files to not exist. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1409-avoid-packing-refs.sh | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh index f74d890e82..be12fb6350 100755 --- a/t/t1409-avoid-packing-refs.sh +++ b/t/t1409-avoid-packing-refs.sh @@ -27,15 +27,15 @@ test_expect_success 'setup' ' ' test_expect_success 'do not create packed-refs file gratuitously' ' - test_must_fail test -f .git/packed-refs && + test_path_is_missing .git/packed-refs && git update-ref refs/heads/foo $A && - test_must_fail test -f .git/packed-refs && + test_path_is_missing .git/packed-refs && git update-ref refs/heads/foo $B && - test_must_fail test -f .git/packed-refs && + test_path_is_missing .git/packed-refs && git update-ref refs/heads/foo $C $B && - test_must_fail test -f .git/packed-refs && + test_path_is_missing .git/packed-refs && git update-ref -d refs/heads/foo && - test_must_fail test -f .git/packed-refs + test_path_is_missing .git/packed-refs ' test_expect_success 'check that marking the packed-refs file works' ' -- 2.24.0.627.geba02921db
The test_must_fail() family of functions (including test_might_fail()) should only be used on git commands. Replace test_might_fail() with test_non_git_might_fail(). Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1501-work-tree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh index 3498d3d55e..067301a5ab 100755 --- a/t/t1501-work-tree.sh +++ b/t/t1501-work-tree.sh @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' ' mkdir work && mkdir -p repo.git/repos/foo && cp repo.git/HEAD repo.git/index repo.git/repos/foo && - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo && sane_unset GIT_DIR GIT_CONFIG GIT_WORK_TREE ' -- 2.24.0.627.geba02921db
Before, we were running `test_must_fail full_name`. However, `test_must_fail` should only be used on git commands. Teach full_name() to accept `!` as a potential first argument which will prepend `test_must_fail` to the enclosed git command. This increases the granularity of test_must_fail since it no longer runs on the `cd` as well. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t1507-rev-parse-upstream.sh | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh index 8b4cf8a6e3..9a941d68d8 100755 --- a/t/t1507-rev-parse-upstream.sh +++ b/t/t1507-rev-parse-upstream.sh @@ -29,8 +29,14 @@ test_expect_success 'setup' ' ' full_name () { + fail= && + if test "x$1" = 'x!' + then + fail=test_must_fail && + shift + fi && (cd clone && - git rev-parse --symbolic-full-name "$@") + $fail git rev-parse --symbolic-full-name "$@") } commit_subject () { @@ -79,7 +85,7 @@ test_expect_success 'upstream of branch with @ at end' ' ' test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' ' - test_must_fail full_name refs/heads/my-side@{upstream} + full_name ! refs/heads/my-side@{upstream} ' test_expect_success 'my-side@{u} resolves to correct commit' ' @@ -91,9 +97,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' ' ' test_expect_success 'not-tracking@{u} fails' ' - test_must_fail full_name non-tracking@{u} && + full_name ! non-tracking@{u} && (cd clone && git checkout --no-track -b non-tracking) && - test_must_fail full_name non-tracking@{u} + full_name ! non-tracking@{u} ' test_expect_success '<branch>@{u}@{1} resolves correctly' ' -- 2.24.0.627.geba02921db
Denton Liu <liu.denton@gmail.com> writes:
> The overall scope of these patches is to replace inappropriate uses of
> test_must_fail. IOW, we should only allow test_must_fail to run on `git`
> and `test-tool`. Ultimately, we will conclude by making test_must_fail
> error out on non-git commands. An advance view of the final series can
> be found here[1].
Wow. It is a bit sad to see that we had so many of them.
Denton Liu <liu.denton@gmail.com> writes: > We had named the parameters in attr_check() but $2 was being used > instead of $expect. Make all variable accesses in attr_check() use named > variables instead of numbered arguments for clarity. > > While we're at it, add variable assignments to the &&-chain. These > aren't ever expected to fail but for stylistic purposes, include them > anyway for stylistic purposes. That is OK but do so like this instead for brevity? attr_check () { - path="$1" expect="$2" + path="$1" expect="$2" git_opts="$3" && Giving (temporary) names to incoming parameters is so common that such a convention to save vertical screen real estate may be worth it. It is sad that you can pass only $IFS-free things in $3 due to the way it is used---perhaps it is sufficient for the purpose of tests in this script, but it still looks sad (just a comment without anything that is actionable). > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > t/t0003-attributes.sh | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh > index 71e63d8b50..c47d4cfbcd 100755 > --- a/t/t0003-attributes.sh > +++ b/t/t0003-attributes.sh > @@ -5,19 +5,20 @@ test_description=gitattributes > . ./test-lib.sh > > attr_check () { > - path="$1" expect="$2" > + path="$1" && > + expect="$2" && > + git_opts="$3" && > > - git $3 check-attr test -- "$path" >actual 2>err && > - echo "$path: test: $2" >expect && > + git $git_opts check-attr test -- "$path" >actual 2>err && > + echo "$path: test: $expect" >expect && > test_cmp expect actual && > test_line_count = 0 err > } > > attr_check_quote () { > - > - path="$1" > - quoted_path="$2" > - expect="$3" > + path="$1" && > + quoted_path="$2" && > + expect="$3" && > > git check-attr test -- "$path" >actual && > echo "\"$quoted_path\": test: $expect" >expect &&
On Tue, Dec 17, 2019 at 7:01 AM Denton Liu <liu.denton@gmail.com> wrote: > We had named the parameters in attr_check() but $2 was being used > instead of $expect. Make all variable accesses in attr_check() use named > variables instead of numbered arguments for clarity. > > While we're at it, add variable assignments to the &&-chain. These > aren't ever expected to fail but for stylistic purposes, include them > anyway for stylistic purposes. As a justification, "stylistic purposes" isn't very strong. However, a solid justification is that keeping the &&-chain intact even for these assignments means that if someone comes along in the future and inserts code _above_ the assignments, and if that code could fail in some way, then the intact &&-chain will ensure that that failure is noticed rather than going unnoticed. > Signed-off-by: Denton Liu <liu.denton@gmail.com>
Denton Liu <liu.denton@gmail.com> writes:
> test_expect_success 'open-quoted pathname' '
> echo "\"a test=a" >.gitattributes &&
> - test_must_fail attr_check a a
> + attr_check a unspecified
> '
I offhand do not think of a reason why "git check-attr" must be
given freedom to choose from more than one answers (which is what
the original test that said "as long as it does not report 'a', we
are happy" allowed the command to), so it probably is OK to make the
tests more strict like this patch does.
Thanks.
Denton Liu <liu.denton@gmail.com> writes: > The test_must_fail() family of functions (including test_might_fail()) > should only be used on git commands. Replace `test_might_fail rm` with > `rm -f` so that we don't use `test_might_fail` on a non-git command. "rm -f X" can return non-zero status if "X" exists and cannot be removed (e.g. "X" is a directory, X is in a directory you cannot write to). The only thing "-f" prevents the command from returning non-zero status is when "X" does not exist. That means that this change will change the behaviour. Let's see if it does in a good way or a bad way. > test_expect_success 'Checking attributes in a non-XDG global attributes file' ' > - test_might_fail rm .gitattributes && > + rm -f .gitattributes && This is so that leftover .gitattributes from previous tests will not affect the outcome of this test. If .gitattributes left by earlier tests cannot be removed for whatever reason, we would want to know about it, so changing to "rm -f" to make the tests more strict is a good move. > echo "f attr_f=test" >"$HOME"/my_gitattributes && > git config core.attributesfile "$HOME"/my_gitattributes && > echo "f: attr_f: test" >expected && > @@ -165,7 +165,7 @@ test_expect_success 'Checking attributes in a non-XDG global attributes file' ' > test_expect_success 'write: xdg file exists and ~/.gitconfig doesn'\''t' ' > mkdir -p "$HOME"/.config/git && > >"$HOME"/.config/git/config && > - test_might_fail rm "$HOME"/.gitconfig && > + rm -f "$HOME"/.gitconfig && Likewise. > git config --global user.name "write_config" && > echo "[user]" >expected && > echo " name = write_config" >>expected && > @@ -183,8 +183,8 @@ test_expect_success 'write: xdg file exists and ~/.gitconfig exists' ' > > > test_expect_success 'write: ~/.config/git/ exists and config file doesn'\''t' ' > - test_might_fail rm "$HOME"/.gitconfig && > - test_might_fail rm "$HOME"/.config/git/config && > + rm -f "$HOME"/.gitconfig && > + rm -f "$HOME"/.config/git/config && Likewise, but I think it makes more sense to remove them with a single invocation of "rm -f". > git config --global user.name "write_gitconfig" && > echo "[user]" >expected && > echo " name = write_gitconfig" >>expected && Thanks. In short, I think all of the changes in the patch are good.
Denton Liu <liu.denton@gmail.com> writes: > Subject: Re: [PATCH 12/15] t1409: let sed open its own files s/files/input file/; This is borderline Meh to me, but since the patch has already been written, let's use it ;-) > In one case, we were using a redirection operator to feed input into > sed. However, since sed is capable of opening its own files, make sed > open its own files instead of redirecting input into it. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > t/t1409-avoid-packing-refs.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh > index c46848eb8e..f74d890e82 100755 > --- a/t/t1409-avoid-packing-refs.sh > +++ b/t/t1409-avoid-packing-refs.sh > @@ -8,7 +8,7 @@ test_description='avoid rewriting packed-refs unnecessarily' > # shouldn't upset readers, and it should be omitted if the file is > # ever rewritten. > mark_packed_refs () { > - sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new && > + sed -e "s/^\(#.*\)/\1 t1409 /" .git/packed-refs >.git/packed-refs.new && > mv .git/packed-refs.new .git/packed-refs > }
Denton Liu <liu.denton@gmail.com> writes: > The test_must_fail() function should only be used for git commands since > we should assume that external commands work sanely. Replace > `test_must_fail test -f` with `test_path_is_missing` since we expect > these files to not exist. s/these files/these paths/; "! test -f FOO" allowed FOO to be a directory, but by using test_path_is_missing we are no longer allowing FOO to exist in any form. I think the changes are correct, but rephrasing "files" to "paths" would emphasize the reason why it is a good change, I would say. Thanks. > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > t/t1409-avoid-packing-refs.sh | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh > index f74d890e82..be12fb6350 100755 > --- a/t/t1409-avoid-packing-refs.sh > +++ b/t/t1409-avoid-packing-refs.sh > @@ -27,15 +27,15 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'do not create packed-refs file gratuitously' ' > - test_must_fail test -f .git/packed-refs && > + test_path_is_missing .git/packed-refs && > git update-ref refs/heads/foo $A && > - test_must_fail test -f .git/packed-refs && > + test_path_is_missing .git/packed-refs && > git update-ref refs/heads/foo $B && > - test_must_fail test -f .git/packed-refs && > + test_path_is_missing .git/packed-refs && > git update-ref refs/heads/foo $C $B && > - test_must_fail test -f .git/packed-refs && > + test_path_is_missing .git/packed-refs && > git update-ref -d refs/heads/foo && > - test_must_fail test -f .git/packed-refs > + test_path_is_missing .git/packed-refs > ' > > test_expect_success 'check that marking the packed-refs file works' '
On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:
> Since output is silenced when running without `-v` and debugging output
> is useful with `-v`, remove redirections to /dev/null as it is not
> useful.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> @@ -5,7 +5,7 @@ test_description='CRLF conversion'
> has_cr() {
> - tr '\015' Q <"$1" | grep Q >/dev/null
> + tr '\015' Q <"$1" | grep Q
> }
I'm not sure that I agree with this change since dropping >/dev/null
doesn't improve the situation when someone is trying to debug a
failing test. What this change will do is fill the -v output with a
bunch of lines ending with "Q" when CR is expected -- the normal
_successful_ case for about half the calls to has_cr() -- so -v output
will become a lot noisier without really adding much, if any,
debugging value. If you really want to help people trying to diagnose
failures, I could see you replacing has_cr() with two new functions
which actually provide useful diagnostic output; for instance
something like:
expect_cr () {
if ! tr '\015' Q <"$1" | grep Q >/dev/null
then
echo "missing CR termination in: $1" >&2 &&
return 1
fi
}
expect_no_cr () {
if tr '\015' Q <"$1" | grep Q >/dev/null
then
echo "unexpected CR termination in: $1" >&2 &&
return 1
fi
}
However, I'm not convinced that introducing and debugging these
functions is worth the effort over simply dropping this patch from the
series.
On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote: > t0020: s/test_must_fail has_cr/! has_cr/ Cryptic patch subject. How about being consistent with other patch subjects in this series? t0020: don't use `test_must_fail has_cr` > The test_must_fail function should only be used for git commands since > we should assume that external commands work sanely. Since has_cr() just > wraps a tr and grep pipeline, replace `test_must_fail has_cr` with > `! has_cr`. > > Signed-off-by: Denton Liu <liu.denton@gmail.com>
On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote:
> The test_must_fail() family of functions (including test_might_fail())
> should only be used on git commands. Replace test_might_fail() with
> test_non_git_might_fail().
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh
> @@ -350,7 +350,7 @@ test_expect_success 'Multi-worktree setup' '
> cp repo.git/HEAD repo.git/index repo.git/repos/foo &&
> - test_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
> + test_non_git_might_fail cp repo.git/sharedindex.* repo.git/repos/foo &&
Can you update the commit message to show that this change is indeed
the correct choice so that readers of this patch don't have to go
through the same investigative reasoning process you went through to
convince yourself that it is correct. In other words, why was
test_might_fail() used in the first place? Is it because there might
not be a sharedindex.* file? Or is it because repo.git/repos/foo might
not be writable? Or what? The answer to these questions should explain
to the reader, for instance, why you chose to use
`test_non_git_might_fail cp ...` as opposed to a simple `cp -f ...`.
On Tue, Dec 17, 2019 at 7:02 AM Denton Liu <liu.denton@gmail.com> wrote: > Before, we were running `test_must_fail full_name`. However, > `test_must_fail` should only be used on git commands. Teach full_name() > to accept `!` as a potential first argument which will prepend > `test_must_fail` to the enclosed git command. This increases the > granularity of test_must_fail since it no longer runs on the `cd` as > well. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh > @@ -29,8 +29,14 @@ test_expect_success 'setup' ' > full_name () { > + fail= && > + if test "x$1" = 'x!' > + then > + fail=test_must_fail && > + shift > + fi && > (cd clone && > - git rev-parse --symbolic-full-name "$@") > + $fail git rev-parse --symbolic-full-name "$@") > } Yuck. These days this is entirely unnecessary. My suggestion is to drop the full_name() function altogether and just invoke git-rev-parse directly at the (few) call sites, taking advantage of the fact that we now have -C. So... > @@ -79,7 +85,7 @@ test_expect_success 'upstream of branch with @ at end' ' > test_expect_success 'refs/heads/my-side@{upstream} does not resolve to my-side{upstream}' ' > - test_must_fail full_name refs/heads/my-side@{upstream} > + full_name ! refs/heads/my-side@{upstream} > ' This just becomes: test_must_fail git -C clone rev-parse --symbolic-full-name refs/heads/my-side@{upstream} Similarly for the other call sites. > @@ -91,9 +97,9 @@ test_expect_success 'my-side@{u} resolves to correct commit' ' > test_expect_success 'not-tracking@{u} fails' ' > - test_must_fail full_name non-tracking@{u} && > + full_name ! non-tracking@{u} && > (cd clone && git checkout --no-track -b non-tracking) && > - test_must_fail full_name non-tracking@{u} > + full_name ! non-tracking@{u} > '
Am 17.12.19 um 13:01 schrieb Denton Liu: > The test_must_fail function should only be used for git commands since > we should assume that external commands work sanely. We use > test_must_fail to test run_sub_test_lib_test() but that function does > not invoke any git commands internally. Replace these instances of > `test_must_fail` with `!`. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > t/t0000-basic.sh | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > index 8a81a249d0..d60ad4b78b 100755 > --- a/t/t0000-basic.sh > +++ b/t/t0000-basic.sh > @@ -155,7 +155,7 @@ test_expect_success 'pretend we have a fully passing test suite' " > " > > test_expect_success 'pretend we have a partially passing test suite' " > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > partial-pass '2/3 tests passing' <<-\\EOF && It is a very uncommon situation (read: I doubt that it ever occurs) in our test suite that we expect a shell function to fail, but that we do *not* care at all which of its sub-commands actually failed. We actually do care which sub-command failed. Therefore, we have, e.g., the idiom "test_i18n_grep ! ...". And in fact, in the case of run_sub_test_lib_test we have the form run_sub_test_lib_test_err to check for error exit in the subordinate test. All of the cases you change here should use it. > test_expect_success 'passing test #1' 'true' > test_expect_success 'failing test #2' 'false' > @@ -219,7 +219,7 @@ test_expect_success 'pretend we have fixed one of two known breakages (run in su > " > > test_expect_success 'pretend we have a pass, fail, and known breakage' " > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > mixed-results1 'mixed results #1' <<-\\EOF && > test_expect_success 'passing test' 'true' > test_expect_success 'failing test' 'false' > @@ -238,7 +238,7 @@ test_expect_success 'pretend we have a pass, fail, and known breakage' " > " > > test_expect_success 'pretend we have a mix of all possible results' " > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > mixed-results2 'mixed results #2' <<-\\EOF && > test_expect_success 'passing test' 'true' > test_expect_success 'passing test' 'true' > @@ -274,7 +274,7 @@ test_expect_success 'pretend we have a mix of all possible results' " > " > > test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > t1234-verbose "test verbose" --verbose <<-\EOF && > test_expect_success "passing test" true > test_expect_success "test with output" "echo foo" > @@ -301,7 +301,7 @@ test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' > ' > > test_expect_success 'test --verbose-only' ' > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > t2345-verbose-only-2 "test verbose-only=2" \ > --verbose-only=2 <<-\EOF && > test_expect_success "passing test" true > @@ -834,7 +834,7 @@ then > fi > > test_expect_success 'tests clean up even on failures' " > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF && > test_expect_success 'tests clean up even after a failure' ' > touch clean-after-failure && > @@ -863,7 +863,7 @@ test_expect_success 'tests clean up even on failures' " > " > > test_expect_success 'test_atexit is run' " > - test_must_fail run_sub_test_lib_test \ > + ! run_sub_test_lib_test \ > atexit-cleanup 'Run atexit commands' -i <<-\\EOF && > test_expect_success 'tests clean up even after a failure' ' > > ../../clean-atexit && > -- Hannes
The overall scope of these patches is to replace inappropriate uses of test_must_fail. IOW, we should only allow test_must_fail to run on `git` and `test-tool`. Ultimately, we will conclude by making test_must_fail error out on non-git commands. An advance view of the final series can be found here[1]. This is the first part. It focuses on t[01]*.sh and also t/lib-git-p4. Changes since v1: * Incorporate review comments by Junio, Eric and J6t * Further cleanup of t1507 before inlining full_name() (since we didn't want to inline full_name() into a bad command substitution) [1]: (may be rebased at any time) https://github.com/Denton-L/git/tree/ready/cleanup-test-must-fail2 Denton Liu (16): test-lib-functions: introduce test_non_git_might_fail() t/lib-git-p4: use test_path_is_missing() t0000: replace test_must_fail with run_sub_test_lib_test_err() t0003: use named parameters in attr_check() t0003: use test_must_be_empty() t0003: don't use `test_must_fail attr_check` t0020: don't use `test_must_fail has_cr` t0020: use ! check_packed_refs_marked t1306: convert `test_might_fail rm` to `rm -f` t1307: reorder `nongit test_must_fail` t1409: let sed open its own input file t1409: use test_path_is_missing() t1501: remove use of `test_might_fail cp` t1507: stop losing return codes of git commands t1507: run commands within test_expect_success t1507: inline full_name() t/lib-git-p4.sh | 2 +- t/t0000-basic.sh | 14 ++--- t/t0003-attributes.sh | 47 +++++++-------- t/t0020-crlf.sh | 18 +++--- t/t1306-xdg-files.sh | 8 +-- t/t1307-config-blob.sh | 2 +- t/t1409-avoid-packing-refs.sh | 16 +++--- t/t1501-work-tree.sh | 2 +- t/t1507-rev-parse-upstream.sh | 104 +++++++++++++++++++--------------- t/test-lib-functions.sh | 9 +++ 10 files changed, 121 insertions(+), 101 deletions(-) Range-diff against v1: 1: fcfccebd7a ! 1: 778ae9052b t0000: replace test_must_fail with ! for run_sub_test_lib_test() @@ Metadata Author: Denton Liu <liu.denton@gmail.com> ## Commit message ## - t0000: replace test_must_fail with ! for run_sub_test_lib_test() + t0000: replace test_must_fail with run_sub_test_lib_test_err() The test_must_fail function should only be used for git commands since we should assume that external commands work sanely. We use test_must_fail to test run_sub_test_lib_test() but that function does - not invoke any git commands internally. Replace these instances of - `test_must_fail` with `!`. + not invoke any git commands internally. Even better, we have a function + that's exactly meant to be used when we expect to have a failing test + suite: run_sub_test_lib_test_err()! + + Replace `test_must_fail run_sub_test_lib_test` with + `run_sub_test_lib_test_err`. ## t/t0000-basic.sh ## @@ t/t0000-basic.sh: test_expect_success 'pretend we have a fully passing test suite' " @@ t/t0000-basic.sh: test_expect_success 'pretend we have a fully passing test suit test_expect_success 'pretend we have a partially passing test suite' " - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ partial-pass '2/3 tests passing' <<-\\EOF && test_expect_success 'passing test #1' 'true' test_expect_success 'failing test #2' 'false' @@ t/t0000-basic.sh: test_expect_success 'pretend we have fixed one of two known br test_expect_success 'pretend we have a pass, fail, and known breakage' " - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ mixed-results1 'mixed results #1' <<-\\EOF && test_expect_success 'passing test' 'true' test_expect_success 'failing test' 'false' @@ t/t0000-basic.sh: test_expect_success 'pretend we have a pass, fail, and known b test_expect_success 'pretend we have a mix of all possible results' " - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ mixed-results2 'mixed results #2' <<-\\EOF && test_expect_success 'passing test' 'true' test_expect_success 'passing test' 'true' @@ t/t0000-basic.sh: test_expect_success 'pretend we have a mix of all possible res test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ t1234-verbose "test verbose" --verbose <<-\EOF && test_expect_success "passing test" true test_expect_success "test with output" "echo foo" @@ t/t0000-basic.sh: test_expect_success C_LOCALE_OUTPUT 'test --verbose' ' test_expect_success 'test --verbose-only' ' - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ t2345-verbose-only-2 "test verbose-only=2" \ --verbose-only=2 <<-\EOF && test_expect_success "passing test" true @@ t/t0000-basic.sh: then test_expect_success 'tests clean up even on failures' " - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF && test_expect_success 'tests clean up even after a failure' ' touch clean-after-failure && @@ t/t0000-basic.sh: test_expect_success 'tests clean up even on failures' " test_expect_success 'test_atexit is run' " - test_must_fail run_sub_test_lib_test \ -+ ! run_sub_test_lib_test \ ++ run_sub_test_lib_test_err \ atexit-cleanup 'Run atexit commands' -i <<-\\EOF && test_expect_success 'tests clean up even after a failure' ' > ../../clean-atexit && 2: f1acb2a0df ! 2: dbc82d45c6 t0003: use named parameters in attr_check() @@ Commit message variables instead of numbered arguments for clarity. While we're at it, add variable assignments to the &&-chain. These - aren't ever expected to fail but for stylistic purposes, include them - anyway for stylistic purposes. + aren't ever expected to fail but if a future developer ever adds some + code above the assignments and they could fail in some way, the intact + &&-chain will ensure that the failure is caught. ## t/t0003-attributes.sh ## @@ t/t0003-attributes.sh: test_description=gitattributes @@ t/t0003-attributes.sh: test_description=gitattributes attr_check () { - path="$1" expect="$2" -+ path="$1" && -+ expect="$2" && -+ git_opts="$3" && ++ path="$1" expect="$2" git_opts="$3" && - git $3 check-attr test -- "$path" >actual 2>err && - echo "$path: test: $2" >expect && @@ t/t0003-attributes.sh: test_description=gitattributes - path="$1" - quoted_path="$2" - expect="$3" -+ path="$1" && -+ quoted_path="$2" && -+ expect="$3" && ++ path="$1" quoted_path="$2" expect="$3" && git check-attr test -- "$path" >actual && echo "\"$quoted_path\": test: $expect" >expect && 3: 055caa5c08 = 3: e06a06cff5 t0003: use test_must_be_empty() 4: 3afa3a16ca = 4: 219011f983 t0003: don't use `test_must_fail attr_check` 5: d228dcfdc7 < -: ---------- t0020: drop redirections to /dev/null 6: 8adc5cd5aa ! 5: 8da6c96b39 t0020: s/test_must_fail has_cr/! has_cr/ @@ Metadata Author: Denton Liu <liu.denton@gmail.com> ## Commit message ## - t0020: s/test_must_fail has_cr/! has_cr/ + t0020: don't use `test_must_fail has_cr` The test_must_fail function should only be used for git commands since we should assume that external commands work sanely. Since has_cr() just 7: b77b474422 = 6: 27550eaae6 t0020: use ! check_packed_refs_marked 8: d39422505f = 7: c19f6344a4 t1306: convert `test_might_fail rm` to `rm -f` 9: 2dd91c5568 = 8: d6ea8a6df0 t1307: reorder `nongit test_must_fail` 10: 0b7d19a7e1 ! 9: d57dfe95e2 t1409: let sed open its own files @@ Metadata Author: Denton Liu <liu.denton@gmail.com> ## Commit message ## - t1409: let sed open its own files + t1409: let sed open its own input file In one case, we were using a redirection operator to feed input into - sed. However, since sed is capable of opening its own files, make sed - open its own files instead of redirecting input into it. + sed. However, since sed is capable of opening its own input file, make + sed do that instead of redirecting input into it. ## t/t1409-avoid-packing-refs.sh ## @@ t/t1409-avoid-packing-refs.sh: test_description='avoid rewriting packed-refs unnecessarily' 11: 27b3296242 ! 10: eacf4e0fb4 t1409: use test_path_is_missing() @@ Commit message The test_must_fail() function should only be used for git commands since we should assume that external commands work sanely. Replace `test_must_fail test -f` with `test_path_is_missing` since we expect - these files to not exist. + these paths to not exist. ## t/t1409-avoid-packing-refs.sh ## @@ t/t1409-avoid-packing-refs.sh: test_expect_success 'setup' ' 12: 3d36511d5d ! 11: 83e47748bc t1501: remove use of `test_might_fail cp` @@ Commit message should only be used on git commands. Replace test_might_fail() with test_non_git_might_fail(). + The `test_might_fail cp` line was introduced in 466e8d5d66 (t1501: fix + test with split index, 2015-03-24). It is necessary because there might + exist some index files in `repo.git/sharedindex.*` and, if they exist, + we want to copy them over. However, if they don't exist, we don't want + to error out because we expect that possibility. As a result, we want to + keep the "might fail" semantics so we use test_non_git_might_fail(). + ## t/t1501-work-tree.sh ## @@ t/t1501-work-tree.sh: test_expect_success 'Multi-worktree setup' ' mkdir work && 13: cd392a74ac < -: ---------- t1507: teach full_name() to accept `!` arg -: ---------- > 12: 9e20865f94 t1507: stop losing return codes of git commands -: ---------- > 13: 7c61ac6b67 t1507: run commands within test_expect_success -: ---------- > 14: d09370455f t1507: inline full_name() -- 2.24.1.703.g2f499f1283
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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).
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.
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.
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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