Modernize Test Path Checking in Git’s Test Suite
PATCH 1/1 Modernize Test Path Checking in Git’s Test Suite
From: Sampriyo Guin <[email protected]>
test -(e|f|d) does not provide a proper error message when hit test failures. So test_path_exists, test_path_is_dir, test_parh_is_file used.
Added changes for files from t/t0007-git-var.sh
to t/t1700-split-index.sh.
---
[GSoC Patch] Modernize Test Path Checking in Git’s Test Suite
test -(e|f|d) does not provide a proper error message when hit test failures. So test_path_exists, test_path_is_dir, test_path_is_file used.
Added changes for files from t/t0007-git-var.sh to t/t1700-split-index.sh.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1923%2FRimoGuin%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1923/RimoGuin/master-v1
Pull-Request: https://github.com/git/git/pull/1923
t/t0007-git-var.sh | 2 +- t/t0081-find-pack.sh | 2 +- t/t0200-gettext-basic.sh | 4 ++-- t/t0410-partial-clone.sh | 2 +- t/t0601-reffiles-pack-refs.sh | 28 ++++++++++++++-------------- t/t1001-read-tree-m-2way.sh | 2 +- t/t1005-read-tree-reset.sh | 10 +++++----- t/t1300-config.sh | 4 ++-- t/t1403-show-ref.sh | 2 +- t/t1410-reflog.sh | 8 ++++---- t/t1420-lost-found.sh | 4 ++-- t/t1700-split-index.sh | 2 +- 12 files changed, 35 insertions(+), 35 deletions(-)
t/t0007-git-var.sh +1 -1
@@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' ' test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' shellpath=$(git var GIT_SHELL_PATH) && case "$shellpath" in - [A-Z]:/*/sh.exe) test -f "$shellpath";; + [A-Z]:/*/sh.exe) test_path_is_file "$shellpath";; *) return 1;; esac '
t/t0081-find-pack.sh +1 -1
@@ -32,7 +32,7 @@ test_expect_success 'repack everything into a single packfile' ' ".git/objects/pack/pack-"*".pack") true ;; *) false ;; esac && - test -f "$head_commit_pack" && + test_path_is_file "$head_commit_pack" && # Everything is in the same pack test "$head_commit_pack" = "$head_tree_pack" &&
t/t0200-gettext-basic.sh +2 -2
@@ -31,12 +31,12 @@ test_expect_success 'xgettext sanity: Comment extraction with --add-comments sto ' test_expect_success GETTEXT 'sanity: $TEXTDOMAINDIR exists without NO_GETTEXT=YesPlease' ' - test -d "$TEXTDOMAINDIR" && + test_path_is_dir "$TEXTDOMAINDIR" && test "$TEXTDOMAINDIR" = "$GIT_TEXTDOMAINDIR" ' test_expect_success GETTEXT 'sanity: Icelandic locale was compiled' ' - test -f "$TEXTDOMAINDIR/is/LC_MESSAGES/git.mo" + test_path_is_file "$TEXTDOMAINDIR/is/LC_MESSAGES/git.mo" ' # TODO: When we have more locales, generalize this to test them
t/t0410-partial-clone.sh +1 -1
@@ -606,7 +606,7 @@ test_expect_success 'gc stops traversal when a missing but promised object is re git -C repo gc && # Ensure that the promisor packfile still exists, and remove it - test -e repo/.git/objects/pack/pack-$HASH.pack && + test_path_exists repo/.git/objects/pack/pack-$HASH.pack && rm repo/.git/objects/pack/pack-$HASH.* && # Ensure that the single other pack contains the commit, but not the tree
t/t0601-reffiles-pack-refs.sh +14 -14
@@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' ' test_expect_success 'see if git pack-refs --prune remove ref files' ' git branch f && git pack-refs --all --prune && - ! test -f .git/refs/heads/f + ! test_path_is_file .git/refs/heads/f ' test_expect_success 'see if git pack-refs --prune removes empty dirs' ' git branch r/s/t && git pack-refs --all --prune && - ! test -e .git/refs/heads/r + ! test_path_exists .git/refs/heads/r ' test_expect_success 'git branch g should work when git branch g/h has been deleted' '
@@ -128,43 +128,43 @@ test_expect_success 'test excluded refs are not packed' ' git branch dont_pack2 && git branch pack_this && git pack-refs --all --exclude "refs/heads/dont_pack*" && - test -f .git/refs/heads/dont_pack1 && - test -f .git/refs/heads/dont_pack2 && - ! test -f .git/refs/heads/pack_this' + test_path_is_file .git/refs/heads/dont_pack1 && + test_path_is_file .git/refs/heads/dont_pack2 && + ! test_path_is_file .git/refs/heads/pack_this' test_expect_success 'test --no-exclude refs clears excluded refs' ' git branch dont_pack3 && git branch dont_pack4 && git pack-refs --all --exclude "refs/heads/dont_pack*" --no-exclude && - ! test -f .git/refs/heads/dont_pack3 && - ! test -f .git/refs/heads/dont_pack4' + ! test_path_is_file .git/refs/heads/dont_pack3 && + ! test_path_is_file .git/refs/heads/dont_pack4' test_expect_success 'test only included refs are packed' ' git branch pack_this1 && git branch pack_this2 && git tag dont_pack5 && git pack-refs --include "refs/heads/pack_this*" && - test -f .git/refs/tags/dont_pack5 && - ! test -f .git/refs/heads/pack_this1 && - ! test -f .git/refs/heads/pack_this2' + test_path_is_file .git/refs/tags/dont_pack5 && + ! test_path_is_file .git/refs/heads/pack_this1 && + ! test_path_is_file .git/refs/heads/pack_this2' test_expect_success 'test --no-include refs clears included refs' ' git branch pack1 && git branch pack2 && git pack-refs --include "refs/heads/pack*" --no-include && - test -f .git/refs/heads/pack1 && - test -f .git/refs/heads/pack2' + test_path_is_file .git/refs/heads/pack1 && + test_path_is_file .git/refs/heads/pack2' test_expect_success 'test --exclude takes precedence over --include' ' git branch dont_pack5 && git pack-refs --include "refs/heads/pack*" --exclude "refs/heads/pack*" && - test -f .git/refs/heads/dont_pack5' + test_path_is_file .git/refs/heads/dont_pack5' test_expect_success 'see if up-to-date packed refs are preserved' ' git branch q && git pack-refs --all --prune && git update-ref refs/heads/q refs/heads/q && - ! test -f .git/refs/heads/q + ! test_path_is_file .git/refs/heads/q ' test_expect_success 'pack, prune and repack' '
t/t1001-read-tree-m-2way.sh +1 -1
@@ -362,7 +362,7 @@ test_expect_success 'a/b (untracked) vs a case setup.' ' test_expect_success 'a/b (untracked) vs a, plus c/d case test.' ' read_tree_u_must_fail -u -m "$treeH" "$treeM" && git ls-files --stage && - test -f a/b + test_path_is_file a/b ' test_expect_success 'read-tree supports the super-prefix' '
t/t1005-read-tree-reset.sh +5 -5
@@ -40,7 +40,7 @@ test_expect_success 'reset should remove remnants from a failed merge' ' git ls-files -s && read_tree_u_must_succeed --reset -u HEAD && git ls-files -s >actual && - ! test -f old && + ! test_path_is_file old && test_cmp expect actual '
@@ -56,7 +56,7 @@ test_expect_success 'two-way reset should remove remnants too' ' git ls-files -s && read_tree_u_must_succeed --reset -u HEAD HEAD && git ls-files -s >actual && - ! test -f old && + ! test_path_is_file old && test_cmp expect actual '
@@ -72,7 +72,7 @@ test_expect_success 'Porcelain reset should remove remnants too' ' git ls-files -s && git reset --hard && git ls-files -s >actual && - ! test -f old && + ! test_path_is_file old && test_cmp expect actual '
@@ -88,7 +88,7 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' ' git ls-files -s && git checkout -f && git ls-files -s >actual && - ! test -f old && + ! test_path_is_file old && test_cmp expect actual '
@@ -104,7 +104,7 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' ' git ls-files -s && git checkout -f HEAD && git ls-files -s >actual && - ! test -f old && + ! test_path_is_file old && test_cmp expect actual '
t/t1300-config.sh +2 -2
@@ -1236,11 +1236,11 @@ test_expect_success SYMLINKS 'symlinked configuration' ' ln -s notyet myconfig && git config --file=myconfig test.frotz nitfol && test -h myconfig && - test -f notyet && + test_path_is_file notyet && test "z$(git config --file=notyet test.frotz)" = znitfol && git config --file=myconfig test.xyzzy rezrov && test -h myconfig && - test -f notyet && + test_path_is_file notyet && cat >expect <<-\EOF && nitfol rezrov
t/t1403-show-ref.sh +1 -1
@@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' remove_object() { file=$(sha1_file "$*") && - test -e "$file" && + test_path_exists "$file" && rm -f "$file" } &&
t/t1410-reflog.sh +4 -4
@@ -130,10 +130,10 @@ test_expect_success 'pass through -- to sub-command' ' test_expect_success rewind ' test_tick && git reset --hard HEAD~2 && - test -f C && - test -f A/B/E && - ! test -f F && - ! test -f A/G && + test_path_is_file C && + test_path_is_file A/B/E && + ! test_path_is_file F && + ! test_path_is_file A/G && check_have A B C D E F G H I J K L &&
t/t1420-lost-found.sh +2 -2
@@ -29,8 +29,8 @@ test_expect_success 'lost and found something' ' git reset --hard HEAD^ && git fsck --lost-found && test 2 = $(ls .git/lost-found/*/* | wc -l) && - test -f .git/lost-found/commit/$(cat lost-commit) && - test -f .git/lost-found/other/$(cat lost-other) + test_path_is_file .git/lost-found/commit/$(cat lost-commit) && + test_path_is_file .git/lost-found/other/$(cat lost-other) ' test_done
t/t1700-split-index.sh +1 -1
@@ -460,7 +460,7 @@ test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is cd ro && test_commit initial && git update-index --split-index && - test -f .git/sharedindex.* + test_path_is_file .git/sharedindex.* ) && cp ro/.git/index new-index && test_when_finished "chmod u+w ro/.git" &&
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Signature · 1 line
gitgitgadget
Quoted 5 lines · Sampriyo Guin via GitGitGadget
From: Sampriyo Guin <[email protected]>
test -(e|f|d) does not provide a proper error message when hit test failures. So
test_path_exists,test_path_is_dir,test_parh_is_fileused.
Correct but ...
t/t0007-git-var.sh test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' ' · line 162
Quoted 13 lines
t/t0007-git-var.sh +1
-1@@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' ' test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' ' shellpath=$(git var GIT_SHELL_PATH) && case "$shellpath" in - [A-Z]:/*/sh.exe) test -f "$shellpath";; + [A-Z]:/*/sh.exe) test_path_is_file "$shellpath";; *) return 1;; esac '
... this one is iffy. How well does it mesh with the "return 1" case in the same case/esac? I do not use Windows, but if GIT_SHELL_PATH set by that system is not in a form that the platform expects (i.e. a drive letter and then path to a file "sh.exe"), it is just as grave a problem worth reporting as the path given is not a regular file, yet "return 1" case does not give any specific error message (instead it just lets the test fail), so it feels a bit uneven to make the "test -f" failure alone louder than it currently is.
t/t0601-reffiles-pack-refs.sh test_expect_success 'see if a branch still exists after git pack-refs --prune' ' · line 82
Quoted 11 lines
t/t0601-reffiles-pack-refs.sh +1
-1@@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' ' test_expect_success 'see if git pack-refs --prune remove ref files' ' git branch f && git pack-refs --all --prune && - ! test -f .git/refs/heads/f + ! test_path_is_file .git/refs/heads/f '
This conversion is wrong. We are expecting that the file does *not* exist, and your complaint and the reason why you are rewriting this line is that "! test -f" does not give loud message when that file does exist. What does "! test_path_is_file foo" do when 'foo' exists?
You can find the implementation of test_path_is_file with "git grep" and see for yourself.
# debugging-friendly alternatives to "test [-f|-d|-e]"
# The commands test the existence or non-existence of $1
test_path_is_file () {
test "$#" -ne 1 && BUG "1 param"
if ! test -f "$1"
then
echo "File $1 doesn't exist"
false
fi
}The test wants to make sure that 'f' is not a file. So you want to see a loud complaint when 'f' exists as a file. Does it do so when you say
! test_path_is_file .git/refs/heads/f
in this test? No, it will not enter the "then" block and silently succeed, and that return status is negated by that "!", so the test will notice that the expectation is not met, but you didn't achieve your goal of making it louder when it fail. Worse, if the file is not there, as the test expects when Git is working correctly, your
! test_path_is_file .git/refs/heads/f
will enter the "then" block, complain that the file does not exist, returns a failure, and your "!" will turn it into a success. The test passes, but the user is given an error message when the test is run with "-v" option.
t/t0601-reffiles-pack-refs.sh test_expect_success 'see if a branch still exists after git pack-refs --prune' ' · line 88
Quoted 6 lines
t/t0601-reffiles-pack-refs.sh +1
-1test_expect_success 'see if git pack-refs --prune removes empty dirs' ' git branch r/s/t && git pack-refs --all --prune && - ! test -e .git/refs/heads/r + ! test_path_exists .git/refs/heads/r '
Ditto.
I'll stop here. I think all "! test_path_foo" changes in this patch are wrong.
Unlike "test_grep" that lets you write "test_grep ! foo bar" to make sure that file 'bar' has no 'foo' in it (and complains loudly if 'foo' appears in 'bar'), test_path_foo family of helper functions do not let you write "test_path_exists ! no-such-file" unfortunately. So my recommendation for a microproject sample is to just drop these negations from the changes and stop there.
If this were a patch for real, it would make sense to give a preliminary update to test_path_foo helpers to allow them to take negated "test_path_exists ! no-such-file" form, and then convert the negative form as well, but I think that would be a bit more than what a typical microproject would take.
Thanks.
Quoted 10 lines · Junio C Hamano
t/t0007-git-var.sh test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' ' · line 162
Quoted 8 lines · Sampriyo Guin via GitGitGadget
test_expect_successMINGW 'GIT_SHELL_PATHpoints to a suitable shell' '
shellpath=$(git varGIT_SHELL_PATH) &&
case "$shellpath" int/t0007-git-var.sh +1
-1- [A-Z]:/*/sh.exe) test -f "$shellpath";; + [A-Z]:/*/sh.exe) test_path_is_file "$shellpath";; *) return 1;; esac '
Quoted 9 lines
... this one is iffy. How well does it mesh with the "return 1" case in the same case/esac? I do not use Windows, but if
GIT_SHELL_PATHset by that system is not in a form that the platform expects (i.e. a drive letter and then path to a file "sh.exe"), it is just as grave a problem worth reporting as the path given is not a regular file, yet "return 1" case does not give any specific error message (instead it just lets the test fail), so it feels a bit uneven to make the "test -f" failure alone louder than it currently is.
Thanks for stating what I was thinking upon seeing this change.
Quoted 30 lines
t/t0601-reffiles-pack-refs.sh test_expect_success 'see if a branch still exists after git pack-refs --prune' ' · line 82
Quoted 6 lines
test_expect_success'see if git pack-refs --prune remove ref files' 'git branch f && git pack-refs --all --prune &&t/t0601-reffiles-pack-refs.sh +1
-1- ! test -f .git/refs/heads/f + ! test_path_is_file .git/refs/heads/f 'This conversion is wrong. [...]
test_path_is_file () { test "$#" -ne 1 && BUG "1 param" if ! test -f "$1" then echo "File $1 doesn't exist" false fi }The test wants to make sure that 'f' is not a file. So you want to see a loud complaint when 'f' exists as a file. Does it do so when you say
!
test_path_is_file.git/refs/heads/fin this test? No, it will not enter the "then" block and silently succeed, and that return status is negated by that "!", so the test will notice that the expectation is not met, but you didn't achieve your goal of making it louder when it fail. Worse, if the file is not there, as the test expects when Git is working correctly, your
Quoted 6 lines
!
test_path_is_file.git/refs/heads/fwill enter the "then" block, complain that the file does not exist, returns a failure, and your "!" will turn it into a success. The test passes, but the user is given an error message when the test is run with "-v" option.
And, again, you've said everything I was going to say, thus saving me the effort of doing so.
Referring to the other thread at [*], perhaps this (avoiding ! in front of test_path_*) is yet another clarification which ought to be added to the microproject description in order to lead candidates in a more profitable direction.
[*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com/
Quoted 9 lines
I'll stop here. I think all "!
test_path_foo" changes in this patch are wrong.Unlike "
test_grep" that lets you write "test_grep! foo bar" to make sure that file 'bar' has no 'foo' in it (and complains loudly if 'foo' appears in 'bar'),test_path_foofamily of helper functions do not let you write "test_path_exists! no-such-file" unfortunately. So my recommendation for a microproject sample is to just drop these negations from the changes and stop there.
One other recommendation I would make is to restrict the microproject submission to just a single test script (rather than updating twelve of them) in order to avoid exhausting the pool for other potential candidates.
Quoted 6 lines · Eric Sunshine
Referring to the other thread at [*], perhaps this (avoiding
!in front of test_path_*) is yet another clarification which ought to be added to the microproject description in order to lead candidates in a more profitable direction.[*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com/
While mentors do that, I'd prefer to see others extend support for
test_path_is_file ! this-should-not-be-file
so that the students have a better tool to work with.
Quoted 4 lines
One other recommendation I would make is to restrict the microproject submission to just a single test script (rather than updating twelve of them) in order to avoid exhausting the pool for other potential candidates.
Yes, you made that point in the other thread, and I agree with it 100%.
Thanks.
PATCH v2 1/1 Modernize Test Path Checking: test -(e|f|d)
In reply to Sampriyo Guin via GitGitGadgetFrom: Sampriyo Guin <[email protected]>
test -f changed to test_path_is_file
test -f does not provide a proper error message so test_path_is_file used Changed for t/t2003-checkout-cache-mkdir.sh
---
[GSoC Patch v2] Modernize Test Path Checking in Git’s Test Suite
test -(e|f|d) does not provide a proper error message when hit test failures. So test_path_exists, test_path_is_dir, test_path_is_file used.
Added changes for files from t/t0007-git-var.sh to t/t1700-split-index.sh.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1923%2FRimoGuin%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1923/RimoGuin/master-v2
Pull-Request: https://github.com/git/git/pull/1923
Range-diff vs v1:
1: 432dbe6fb82 < -: ----------- [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite -: ----------- > 1: 0f74a82d681 [GSoC Patch v2]Modernize Test Path Checking: test -(e|f|d)
t/t2003-checkout-cache-mkdir.sh | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
t/t2003-checkout-cache-mkdir.sh +16 -16
@@ -24,16 +24,16 @@ test_expect_success SYMLINKS 'have symlink in place where dir is expected.' ' mkdir path2 && ln -s path2 path1 && git checkout-index -f -a && - test ! -h path1 && test -d path1 && - test -f path1/file1 && test ! -f path2/file1 + test ! -h path1 && test_path_is_dir path1 && + test_path_is_file path1/file1 && test ! -f path2/file1 ' test_expect_success 'use --prefix=path2/' ' rm -fr path0 path1 path2 && mkdir path2 && git checkout-index --prefix=path2/ -f -a && - test -f path2/path0 && - test -f path2/path1/file1 && + test_path_is_file path2/path0 && + test_path_is_file path2/path1/file1 && test ! -f path0 && test ! -f path1/file1 '
@@ -41,8 +41,8 @@ test_expect_success 'use --prefix=path2/' ' test_expect_success 'use --prefix=tmp-' ' rm -fr path0 path1 path2 tmp* && git checkout-index --prefix=tmp- -f -a && - test -f tmp-path0 && - test -f tmp-path1/file1 && + test_path_is_file tmp-path0 && + test_path_is_file tmp-path1/file1 && test ! -f path0 && test ! -f path1/file1 '
@@ -52,8 +52,8 @@ test_expect_success 'use --prefix=tmp- but with a conflicting file and dir' ' echo nitfol >tmp-path1 && mkdir tmp-path0 && git checkout-index --prefix=tmp- -f -a && - test -f tmp-path0 && - test -f tmp-path1/file1 && + test_path_is_file tmp-path0 && + test_path_is_file tmp-path1/file1 && test ! -f path0 && test ! -f path1/file1 '
@@ -63,9 +63,9 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary/ where tmp is a symlink' ' mkdir tmp1 tmp1/orary && ln -s tmp1 tmp && git checkout-index --prefix=tmp/orary/ -f -a && - test -d tmp1/orary && - test -f tmp1/orary/path0 && - test -f tmp1/orary/path1/file1 && + test_path_is_dir tmp1/orary && + test_path_is_file tmp1/orary/path0 && + test_path_is_file tmp1/orary/path1/file1 && test -h tmp '
@@ -74,8 +74,8 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' ' mkdir tmp1 && ln -s tmp1 tmp && git checkout-index --prefix=tmp/orary- -f -a && - test -f tmp1/orary-path0 && - test -f tmp1/orary-path1/file1 && + test_path_is_file tmp1/orary-path0 && + test_path_is_file tmp1/orary-path1/file1 && test -h tmp '
@@ -84,10 +84,10 @@ test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' ' mkdir tmp1 && ln -s tmp1 tmp-path1 && git checkout-index --prefix=tmp- -f -a && - test -f tmp-path0 && + test_path_is_file tmp-path0 && test ! -h tmp-path1 && - test -d tmp-path1 && - test -f tmp-path1/file1 + test_path_is_dir tmp-path1 && + test_path_is_file tmp-path1/file1 ' test_expect_success 'apply filter from working tree .gitattributes with --prefix' '
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Signature · 1 line
gitgitgadget