On Wed, Oct 9, 2024 at 6:47 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Samuel Adekunle Abraham via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Abraham Samuel Adekunle <abrahamadekunle50@xxxxxxxxx> > > > > This test script uses "test - [def]", but when a test fails because > > the file passed to it does not exist, > > it fails silently without an error message. > > Use test_path_* helper functions, which are designed to give better > > error messages when their expectations are not met. > > > > I have added a mechanical validation that applies the same transformation > > done in this patch, when the test script is passed to a sed script as shown > > below. > > > > sed -e 's/^\( *\)test -f /\1test_path_is_file /' \ > > -e 's/^\( *\)test -d /\1test_path_is_dir /' \ > > -e 's/^\( *\)test -e /\1test_path_exists /' \ > > -e 's/^\( *\)! test -[edf] /\1test_path_is_missing /' \ > > -e 's/^\( *\)test ! -[edf] /\1test_path_is_missing /' \ > > "$1" >foo.sh > > > > Reviewers can use the sed script to tranform the original test script and > > compare the result in foo.sh with the results of applying the patch. > > You will see an instance of "!(test -e 3)" which was manually replaced with > > ""test_path_is_missing 3", and everything else should match. > > > > Careful and deliberate observation was done to check instances where > > "test ! - [df] foo" was used in the test script to make sure that the test > > instances were expecting foo to EITHER be a file or a directory, and NOT a > > possibility of being both as this would make replacing "test ! -f foo" with > > "test_path_is_missing foo" unreasonable. > > In the tests control flow, foo has been created as EITHER a > > reguar file OR a directory and should NOT exist > > after "git clean" or "git clean -d", as the case maybe, has been called. > > This made it reasonable to replace > > "test ! -[df] foo" with "test_path_is_missing foo". > > This is a good place to stop (but perhaps have a paragraph break > before "In the tests control"). The "examples" you have below is > like telling readers to go read the patch and verify the correctness > of it themselves, which is not adding much value. > > Other than that, looking very good. > > Thanks. Thank you very much, I have submitted an updated patch. > > > t/t7300-clean.sh | 370 +++++++++++++++++++++++------------------------ > > 1 file changed, 185 insertions(+), 185 deletions(-) > > > > diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh > > index 0aae0dee670..5c97eb0dfe9 100755 > > --- a/t/t7300-clean.sh > > +++ b/t/t7300-clean.sh > > @@ -29,15 +29,15 @@ test_expect_success 'git clean with skip-worktree .gitignore' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so && > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so && > > git update-index --no-skip-worktree .gitignore && > > git checkout .gitignore > > ' > > @@ -47,15 +47,15 @@ test_expect_success 'git clean' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -64,15 +64,15 @@ test_expect_success 'git clean src/' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean src/ && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -81,15 +81,15 @@ test_expect_success 'git clean src/ src/' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean src/ src/ && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -98,16 +98,16 @@ test_expect_success 'git clean with prefix' ' > > mkdir -p build docs src/test && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c && > > (cd src/ && git clean) && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test ! -f src/part3.c && > > - test -f src/test/1.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file src/test/1.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -163,16 +163,16 @@ test_expect_success 'git clean -d with prefix and path' ' > > mkdir -p build docs src/feature && > > touch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o build/lib.so && > > (cd src/ && git clean -d feature/) && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test -f src/part3.c && > > - test ! -f src/feature/file.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_file src/part3.c && > > + test_path_is_missing src/feature/file.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -182,16 +182,16 @@ test_expect_success SYMLINKS 'git clean symbolic link' ' > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > ln -s docs/manual.txt src/part4.c && > > git clean && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test ! -f src/part4.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_missing src/part4.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -199,13 +199,13 @@ test_expect_success 'git clean with wildcard' ' > > > > touch a.clean b.clean other.c && > > git clean "*.clean" && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.clean && > > - test ! -f b.clean && > > - test -f other.c > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.clean && > > + test_path_is_missing b.clean && > > + test_path_is_file other.c > > > > ' > > > > @@ -214,15 +214,15 @@ test_expect_success 'git clean -n' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -n && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_file src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -231,15 +231,15 @@ test_expect_success 'git clean -d' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -d && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test ! -d docs && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_missing docs && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -248,16 +248,16 @@ test_expect_success 'git clean -d src/ examples/' ' > > mkdir -p build docs examples && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so examples/1.c && > > git clean -d src/ examples/ && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test ! -f src/part3.c && > > - test ! -f examples/1.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_missing examples/1.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -266,15 +266,15 @@ test_expect_success 'git clean -x' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -x && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test ! -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_missing obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -283,15 +283,15 @@ test_expect_success 'git clean -d -x' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -d -x && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test ! -d docs && > > - test ! -f obj.o && > > - test ! -d build > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_missing docs && > > + test_path_is_missing obj.o && > > + test_path_is_missing build > > > > ' > > > > @@ -300,15 +300,15 @@ test_expect_success 'git clean -d -x with ignored tracked directory' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -d -x -e src && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test -f src/part3.c && > > - test ! -d docs && > > - test ! -f obj.o && > > - test ! -d build > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_file src/part3.c && > > + test_path_is_missing docs && > > + test_path_is_missing obj.o && > > + test_path_is_missing build > > > > ' > > > > @@ -317,15 +317,15 @@ test_expect_success 'git clean -X' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -X && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test -f src/part3.c && > > - test -f docs/manual.txt && > > - test ! -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_file src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_missing obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -334,15 +334,15 @@ test_expect_success 'git clean -d -X' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -d -X && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test -f src/part3.c && > > - test -f docs/manual.txt && > > - test ! -f obj.o && > > - test ! -d build > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_file src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_missing obj.o && > > + test_path_is_missing build > > > > ' > > > > @@ -351,15 +351,15 @@ test_expect_success 'git clean -d -X with ignored tracked directory' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -d -X -e src && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test ! -f obj.o && > > - test ! -d build > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_missing obj.o && > > + test_path_is_missing build > > > > ' > > > > @@ -382,29 +382,29 @@ test_expect_success 'clean.requireForce and -n' ' > > mkdir -p build docs && > > touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && > > git clean -n && > > - test -f Makefile && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test -f a.out && > > - test -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file Makefile && > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_file a.out && > > + test_path_is_file src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > test_expect_success 'clean.requireForce and -f' ' > > > > git clean -f && > > - test -f README && > > - test -f src/part1.c && > > - test -f src/part2.c && > > - test ! -f a.out && > > - test ! -f src/part3.c && > > - test -f docs/manual.txt && > > - test -f obj.o && > > - test -f build/lib.so > > + test_path_is_file README && > > + test_path_is_file src/part1.c && > > + test_path_is_file src/part2.c && > > + test_path_is_missing a.out && > > + test_path_is_missing src/part3.c && > > + test_path_is_file docs/manual.txt && > > + test_path_is_file obj.o && > > + test_path_is_file build/lib.so > > > > ' > > > > @@ -453,11 +453,11 @@ test_expect_success 'nested git work tree' ' > > test_commit deeply.nested deeper.world > > ) && > > git clean -f -d && > > - test -f foo/.git/index && > > - test -f foo/hello.world && > > - test -f baz/boo/.git/index && > > - test -f baz/boo/deeper.world && > > - ! test -d bar > > + test_path_is_file foo/.git/index && > > + test_path_is_file foo/hello.world && > > + test_path_is_file baz/boo/.git/index && > > + test_path_is_file baz/boo/deeper.world && > > + test_path_is_missing bar > > ' > > > > test_expect_success 'should clean things that almost look like git but are not' ' > > @@ -624,9 +624,9 @@ test_expect_success 'force removal of nested git work tree' ' > > test_commit deeply.nested deeper.world > > ) && > > git clean -f -f -d && > > - ! test -d foo && > > - ! test -d bar && > > - ! test -d baz > > + test_path_is_missing foo && > > + test_path_is_missing bar && > > + test_path_is_missing baz > > ' > > > > test_expect_success 'git clean -e' ' > > @@ -638,10 +638,10 @@ test_expect_success 'git clean -e' ' > > touch known 1 2 3 && > > git add known && > > git clean -f -e 1 -e 2 && > > - test -e 1 && > > - test -e 2 && > > - ! (test -e 3) && > > - test -e known > > + test_path_exists 1 && > > + test_path_exists 2 && > > + test_path_is_missing 3 && > > + test_path_exists known > > ) > > ' > > > > @@ -649,7 +649,7 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' ' > > mkdir foo && > > chmod a= foo && > > git clean -dfx foo && > > - ! test -d foo > > + test_path_is_missing foo > > ' > > > > test_expect_success 'git clean -d respects pathspecs (dir is prefix of pathspec)' ' > > > > base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1
