| Message ID | pull.1923.git.git.1742329571265.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [GSoC] Modernize Test Path Checking in Git’s Test Suite | expand |
"Sampriyo Guin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Sampriyo Guin <sampriyoguin@gmail.com> > > 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. Correct but ... > diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh > index 2b60317758c..6a8fe69c089 100755 > --- a/t/t0007-git-var.sh > +++ b/t/t0007-git-var.sh > @@ -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. > diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh > index aa7f6ecd813..f76471a3375 100755 > --- a/t/t0601-reffiles-pack-refs.sh > +++ b/t/t0601-reffiles-pack-refs.sh > @@ -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. > 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 > ' 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.
On Tue, Mar 18, 2025 at 5:14 PM Junio C Hamano <gitster@pobox.com> wrote: > "Sampriyo Guin via GitGitGadget" <gitgitgadget@gmail.com> writes: > > 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. Thanks for stating what I was thinking upon seeing this change. > > 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. [...] > > 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. 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/ > 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. 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.
Eric Sunshine <sunshine@sunshineco.com> writes: > 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. > 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.
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh index 2b60317758c..6a8fe69c089 100755 --- a/t/t0007-git-var.sh +++ b/t/t0007-git-var.sh @@ -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 ' diff --git a/t/t0081-find-pack.sh b/t/t0081-find-pack.sh index 5a628bf7356..cb2825769ca 100755 --- a/t/t0081-find-pack.sh +++ b/t/t0081-find-pack.sh @@ -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" && diff --git a/t/t0200-gettext-basic.sh b/t/t0200-gettext-basic.sh index 8853d8afb92..89d0899a5bd 100755 --- a/t/t0200-gettext-basic.sh +++ b/t/t0200-gettext-basic.sh @@ -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 diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 2a5bdbeeb87..615983067a9 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -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 diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh index aa7f6ecd813..f76471a3375 100755 --- a/t/t0601-reffiles-pack-refs.sh +++ b/t/t0601-reffiles-pack-refs.sh @@ -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' ' diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh index 4a88bb9ef0c..2e8d9384e1b 100755 --- a/t/t1001-read-tree-m-2way.sh +++ b/t/t1001-read-tree-m-2way.sh @@ -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' ' diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh index 6b5033d0ce3..12b127eb7e6 100755 --- a/t/t1005-read-tree-reset.sh +++ b/t/t1005-read-tree-reset.sh @@ -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 ' diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 51a85e83c27..9820d2348bc 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -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 diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index 9d698b3cc35..12f7b600244 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -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" } && diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh index 388fdf9ae57..429ff59d2cb 100755 --- a/t/t1410-reflog.sh +++ b/t/t1410-reflog.sh @@ -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 && diff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh index 2fb2f44f021..5fbb1d10ede 100755 --- a/t/t1420-lost-found.sh +++ b/t/t1420-lost-found.sh @@ -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 diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index ac4a5b2734c..38d6f19152a 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -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" &&