{"thread":{"id":"63157","subject":"[PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite","startedAt":"2025-03-18T20:26:14Z","lastAt":"2025-03-19T06:04:01Z","messageCount":5,"participants":["Sampriyo Guin via GitGitGadget","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"514550","messageId":"pull.1923.git.git.1742329571265.gitgitgadget@gmail.com","threadId":"63157","inReplyTo":null,"subject":"[PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite","fromName":"Sampriyo Guin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-18T20:26:11Z","receivedAt":"2025-03-18T20:26:14Z","isPatch":true,"sender":{"key":"sampriyoguin@gmail.com","avatar":null},"body":"From: Sampriyo Guin <sampriyoguin@gmail.com>\n\ntest -(e|f|d) does not provide a proper error message when hit\ntest failures. So test_path_exists, test_path_is_dir,\ntest_parh_is_file used.\nAdded changes for files from t/t0007-git-var.sh\nto t/t1700-split-index.sh.\n\nSigned-off-by: Sampriyo Guin <sampriyoguin@gmail.com>\n---\n    [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite\n    \n    test -(e|f|d) does not provide a proper error message when hit test\n    failures. So test_path_exists, test_path_is_dir, test_path_is_file used.\n    \n    Added changes for files from t/t0007-git-var.sh to\n    t/t1700-split-index.sh.\n    \n    Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1923%2FRimoGuin%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1923/RimoGuin/master-v1\nPull-Request: https://github.com/git/git/pull/1923\n\n t/t0007-git-var.sh            |  2 +-\n t/t0081-find-pack.sh          |  2 +-\n t/t0200-gettext-basic.sh      |  4 ++--\n t/t0410-partial-clone.sh      |  2 +-\n t/t0601-reffiles-pack-refs.sh | 28 ++++++++++++++--------------\n t/t1001-read-tree-m-2way.sh   |  2 +-\n t/t1005-read-tree-reset.sh    | 10 +++++-----\n t/t1300-config.sh             |  4 ++--\n t/t1403-show-ref.sh           |  2 +-\n t/t1410-reflog.sh             |  8 ++++----\n t/t1420-lost-found.sh         |  4 ++--\n t/t1700-split-index.sh        |  2 +-\n 12 files changed, 35 insertions(+), 35 deletions(-)\n\ndiff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\nindex 2b60317758c..6a8fe69c089 100755\n--- a/t/t0007-git-var.sh\n+++ b/t/t0007-git-var.sh\n@@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n \tshellpath=$(git var GIT_SHELL_PATH) &&\n \tcase \"$shellpath\" in\n-\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n+\t[A-Z]:/*/sh.exe) test_path_is_file \"$shellpath\";;\n \t*) return 1;;\n \tesac\n '\ndiff --git a/t/t0081-find-pack.sh b/t/t0081-find-pack.sh\nindex 5a628bf7356..cb2825769ca 100755\n--- a/t/t0081-find-pack.sh\n+++ b/t/t0081-find-pack.sh\n@@ -32,7 +32,7 @@ test_expect_success 'repack everything into a single packfile' '\n \t\t\".git/objects/pack/pack-\"*\".pack\") true ;;\n \t\t*) false ;;\n \tesac &&\n-\ttest -f \"$head_commit_pack\" &&\n+\ttest_path_is_file \"$head_commit_pack\" &&\n \n \t# Everything is in the same pack\n \ttest \"$head_commit_pack\" = \"$head_tree_pack\" &&\ndiff --git a/t/t0200-gettext-basic.sh b/t/t0200-gettext-basic.sh\nindex 8853d8afb92..89d0899a5bd 100755\n--- a/t/t0200-gettext-basic.sh\n+++ b/t/t0200-gettext-basic.sh\n@@ -31,12 +31,12 @@ test_expect_success 'xgettext sanity: Comment extraction with --add-comments sto\n '\n \n test_expect_success GETTEXT 'sanity: $TEXTDOMAINDIR exists without NO_GETTEXT=YesPlease' '\n-    test -d \"$TEXTDOMAINDIR\" &&\n+    test_path_is_dir \"$TEXTDOMAINDIR\" &&\n     test \"$TEXTDOMAINDIR\" = \"$GIT_TEXTDOMAINDIR\"\n '\n \n test_expect_success GETTEXT 'sanity: Icelandic locale was compiled' '\n-    test -f \"$TEXTDOMAINDIR/is/LC_MESSAGES/git.mo\"\n+    test_path_is_file \"$TEXTDOMAINDIR/is/LC_MESSAGES/git.mo\"\n '\n \n # TODO: When we have more locales, generalize this to test them\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 2a5bdbeeb87..615983067a9 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -606,7 +606,7 @@ test_expect_success 'gc stops traversal when a missing but promised object is re\n \tgit -C repo gc &&\n \n \t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n+\ttest_path_exists repo/.git/objects/pack/pack-$HASH.pack &&\n \trm repo/.git/objects/pack/pack-$HASH.* &&\n \n \t# Ensure that the single other pack contains the commit, but not the tree\ndiff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\nindex aa7f6ecd813..f76471a3375 100755\n--- a/t/t0601-reffiles-pack-refs.sh\n+++ b/t/t0601-reffiles-pack-refs.sh\n@@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' '\n test_expect_success 'see if git pack-refs --prune remove ref files' '\n \tgit branch f &&\n \tgit pack-refs --all --prune &&\n-\t! test -f .git/refs/heads/f\n+\t! test_path_is_file .git/refs/heads/f\n '\n \n test_expect_success 'see if git pack-refs --prune removes empty dirs' '\n \tgit branch r/s/t &&\n \tgit pack-refs --all --prune &&\n-\t! test -e .git/refs/heads/r\n+\t! test_path_exists .git/refs/heads/r\n '\n \n test_expect_success 'git branch g should work when git branch g/h has been deleted' '\n@@ -128,43 +128,43 @@ test_expect_success 'test excluded refs are not packed' '\n \tgit branch dont_pack2 &&\n \tgit branch pack_this &&\n \tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" &&\n-\ttest -f .git/refs/heads/dont_pack1 &&\n-\ttest -f .git/refs/heads/dont_pack2 &&\n-\t! test -f .git/refs/heads/pack_this'\n+\ttest_path_is_file .git/refs/heads/dont_pack1 &&\n+\ttest_path_is_file .git/refs/heads/dont_pack2 &&\n+\t! test_path_is_file .git/refs/heads/pack_this'\n \n test_expect_success 'test --no-exclude refs clears excluded refs' '\n \tgit branch dont_pack3 &&\n \tgit branch dont_pack4 &&\n \tgit pack-refs --all --exclude \"refs/heads/dont_pack*\" --no-exclude &&\n-\t! test -f .git/refs/heads/dont_pack3 &&\n-\t! test -f .git/refs/heads/dont_pack4'\n+\t! test_path_is_file .git/refs/heads/dont_pack3 &&\n+\t! test_path_is_file .git/refs/heads/dont_pack4'\n \n test_expect_success 'test only included refs are packed' '\n \tgit branch pack_this1 &&\n \tgit branch pack_this2 &&\n \tgit tag dont_pack5 &&\n \tgit pack-refs --include \"refs/heads/pack_this*\" &&\n-\ttest -f .git/refs/tags/dont_pack5 &&\n-\t! test -f .git/refs/heads/pack_this1 &&\n-\t! test -f .git/refs/heads/pack_this2'\n+\ttest_path_is_file .git/refs/tags/dont_pack5 &&\n+\t! test_path_is_file .git/refs/heads/pack_this1 &&\n+\t! test_path_is_file .git/refs/heads/pack_this2'\n \n test_expect_success 'test --no-include refs clears included refs' '\n \tgit branch pack1 &&\n \tgit branch pack2 &&\n \tgit pack-refs --include \"refs/heads/pack*\" --no-include &&\n-\ttest -f .git/refs/heads/pack1 &&\n-\ttest -f .git/refs/heads/pack2'\n+\ttest_path_is_file .git/refs/heads/pack1 &&\n+\ttest_path_is_file .git/refs/heads/pack2'\n \n test_expect_success 'test --exclude takes precedence over --include' '\n \tgit branch dont_pack5 &&\n \tgit pack-refs --include \"refs/heads/pack*\" --exclude \"refs/heads/pack*\" &&\n-\ttest -f .git/refs/heads/dont_pack5'\n+\ttest_path_is_file .git/refs/heads/dont_pack5'\n \n test_expect_success 'see if up-to-date packed refs are preserved' '\n \tgit branch q &&\n \tgit pack-refs --all --prune &&\n \tgit update-ref refs/heads/q refs/heads/q &&\n-\t! test -f .git/refs/heads/q\n+\t! test_path_is_file .git/refs/heads/q\n '\n \n test_expect_success 'pack, prune and repack' '\ndiff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh\nindex 4a88bb9ef0c..2e8d9384e1b 100755\n--- a/t/t1001-read-tree-m-2way.sh\n+++ b/t/t1001-read-tree-m-2way.sh\n@@ -362,7 +362,7 @@ test_expect_success 'a/b (untracked) vs a case setup.' '\n test_expect_success 'a/b (untracked) vs a, plus c/d case test.' '\n \tread_tree_u_must_fail -u -m \"$treeH\" \"$treeM\" &&\n \tgit ls-files --stage &&\n-\ttest -f a/b\n+\ttest_path_is_file a/b\n '\n \n test_expect_success 'read-tree supports the super-prefix' '\ndiff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh\nindex 6b5033d0ce3..12b127eb7e6 100755\n--- a/t/t1005-read-tree-reset.sh\n+++ b/t/t1005-read-tree-reset.sh\n@@ -40,7 +40,7 @@ test_expect_success 'reset should remove remnants from a failed merge' '\n \tgit ls-files -s &&\n \tread_tree_u_must_succeed --reset -u HEAD &&\n \tgit ls-files -s >actual &&\n-\t! test -f old &&\n+\t! test_path_is_file old &&\n \ttest_cmp expect actual\n '\n \n@@ -56,7 +56,7 @@ test_expect_success 'two-way reset should remove remnants too' '\n \tgit ls-files -s &&\n \tread_tree_u_must_succeed --reset -u HEAD HEAD &&\n \tgit ls-files -s >actual &&\n-\t! test -f old &&\n+\t! test_path_is_file old &&\n \ttest_cmp expect actual\n '\n \n@@ -72,7 +72,7 @@ test_expect_success 'Porcelain reset should remove remnants too' '\n \tgit ls-files -s &&\n \tgit reset --hard &&\n \tgit ls-files -s >actual &&\n-\t! test -f old &&\n+\t! test_path_is_file old &&\n \ttest_cmp expect actual\n '\n \n@@ -88,7 +88,7 @@ test_expect_success 'Porcelain checkout -f should remove remnants too' '\n \tgit ls-files -s &&\n \tgit checkout -f &&\n \tgit ls-files -s >actual &&\n-\t! test -f old &&\n+\t! test_path_is_file old &&\n \ttest_cmp expect actual\n '\n \n@@ -104,7 +104,7 @@ test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '\n \tgit ls-files -s &&\n \tgit checkout -f HEAD &&\n \tgit ls-files -s >actual &&\n-\t! test -f old &&\n+\t! test_path_is_file old &&\n \ttest_cmp expect actual\n '\n \ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 51a85e83c27..9820d2348bc 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -1236,11 +1236,11 @@ test_expect_success SYMLINKS 'symlinked configuration' '\n \tln -s notyet myconfig &&\n \tgit config --file=myconfig test.frotz nitfol &&\n \ttest -h myconfig &&\n-\ttest -f notyet &&\n+\ttest_path_is_file notyet &&\n \ttest \"z$(git config --file=notyet test.frotz)\" = znitfol &&\n \tgit config --file=myconfig test.xyzzy rezrov &&\n \ttest -h myconfig &&\n-\ttest -f notyet &&\n+\ttest_path_is_file notyet &&\n \tcat >expect <<-\\EOF &&\n \tnitfol\n \trezrov\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 9d698b3cc35..12f7b600244 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' '\n \n \tremove_object() {\n \t\tfile=$(sha1_file \"$*\") &&\n-\t\ttest -e \"$file\" &&\n+\t\ttest_path_exists \"$file\" &&\n \t\trm -f \"$file\"\n \t} &&\n \ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 388fdf9ae57..429ff59d2cb 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -130,10 +130,10 @@ test_expect_success 'pass through -- to sub-command' '\n \n test_expect_success rewind '\n \ttest_tick && git reset --hard HEAD~2 &&\n-\ttest -f C &&\n-\ttest -f A/B/E &&\n-\t! test -f F &&\n-\t! test -f A/G &&\n+\ttest_path_is_file C &&\n+\ttest_path_is_file A/B/E &&\n+\t! test_path_is_file F &&\n+\t! test_path_is_file A/G &&\n \n \tcheck_have A B C D E F G H I J K L &&\n \ndiff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh\nindex 2fb2f44f021..5fbb1d10ede 100755\n--- a/t/t1420-lost-found.sh\n+++ b/t/t1420-lost-found.sh\n@@ -29,8 +29,8 @@ test_expect_success 'lost and found something' '\n \tgit reset --hard HEAD^ &&\n \tgit fsck --lost-found &&\n \ttest 2 = $(ls .git/lost-found/*/* | wc -l) &&\n-\ttest -f .git/lost-found/commit/$(cat lost-commit) &&\n-\ttest -f .git/lost-found/other/$(cat lost-other)\n+\ttest_path_is_file .git/lost-found/commit/$(cat lost-commit) &&\n+\ttest_path_is_file .git/lost-found/other/$(cat lost-other)\n '\n \n test_done\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex ac4a5b2734c..38d6f19152a 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -460,7 +460,7 @@ test_expect_success POSIXPERM,SANITY 'graceful handling when splitting index is\n \t\tcd ro &&\n \t\ttest_commit initial &&\n \t\tgit update-index --split-index &&\n-\t\ttest -f .git/sharedindex.*\n+\t\ttest_path_is_file .git/sharedindex.*\n \t) &&\n \tcp ro/.git/index new-index &&\n \ttest_when_finished \"chmod u+w ro/.git\" &&\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \ngitgitgadget\n"},{"id":"514554","messageId":"xmqq5xk611o3.fsf@gitster.g","threadId":"63157","inReplyTo":"pull.1923.git.git.1742329571265.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-18T21:14:52Z","receivedAt":"2025-03-18T21:14:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sampriyo Guin via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Sampriyo Guin <sampriyoguin@gmail.com>\n>\n> test -(e|f|d) does not provide a proper error message when hit\n> test failures. So test_path_exists, test_path_is_dir,\n> test_parh_is_file used.\n\nCorrect but ...\n\n> diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh\n> index 2b60317758c..6a8fe69c089 100755\n> --- a/t/t0007-git-var.sh\n> +++ b/t/t0007-git-var.sh\n> @@ -156,7 +156,7 @@ test_expect_success POSIXPERM 'GIT_SHELL_PATH points to a valid executable' '\n>  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n>  \tshellpath=$(git var GIT_SHELL_PATH) &&\n>  \tcase \"$shellpath\" in\n> -\t[A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n> +\t[A-Z]:/*/sh.exe) test_path_is_file \"$shellpath\";;\n>  \t*) return 1;;\n>  \tesac\n>  '\n\n... this one is iffy.  How well does it mesh with the \"return 1\"\ncase in the same case/esac?  I do not use Windows, but if\nGIT_SHELL_PATH set by that system is not in a form that the platform\nexpects (i.e. a drive letter and then path to a file \"sh.exe\"), it\nis just as grave a problem worth reporting as the path given is not\na regular file, yet \"return 1\" case does not give any specific error\nmessage (instead it just lets the test fail), so it feels a bit\nuneven to make the \"test -f\" failure alone louder than it currently\nis.\n\n> diff --git a/t/t0601-reffiles-pack-refs.sh b/t/t0601-reffiles-pack-refs.sh\n> index aa7f6ecd813..f76471a3375 100755\n> --- a/t/t0601-reffiles-pack-refs.sh\n> +++ b/t/t0601-reffiles-pack-refs.sh\n> @@ -78,13 +78,13 @@ test_expect_success 'see if a branch still exists after git pack-refs --prune' '\n>  test_expect_success 'see if git pack-refs --prune remove ref files' '\n>  \tgit branch f &&\n>  \tgit pack-refs --all --prune &&\n> -\t! test -f .git/refs/heads/f\n> +\t! test_path_is_file .git/refs/heads/f\n>  '\n\nThis conversion is wrong.  We are expecting that the file does *not*\nexist, and your complaint and the reason why you are rewriting this\nline is that \"! test -f\" does not give loud message when that file\ndoes exist.  What does \"! test_path_is_file foo\" do when 'foo'\nexists?\n\nYou can find the implementation of test_path_is_file with \"git grep\"\nand see for yourself.\n\n    # debugging-friendly alternatives to \"test [-f|-d|-e]\"\n    # The commands test the existence or non-existence of $1\n    test_path_is_file () {\n            test \"$#\" -ne 1 && BUG \"1 param\"\n            if ! test -f \"$1\"\n            then\n                    echo \"File $1 doesn't exist\"\n                    false\n            fi\n    }\n\nThe test wants to make sure that 'f' is not a file.  So you want to\nsee a loud complaint when 'f' exists as a file.  Does it do so when\nyou say\n\n\t! test_path_is_file .git/refs/heads/f\n\nin this test?  No, it will not enter the \"then\" block and silently\nsucceed, and that return status is negated by that \"!\", so the test\nwill notice that the expectation is not met, but you didn't achieve\nyour goal of making it louder when it fail.  Worse, if the file is\nnot there, as the test expects when Git is working correctly, your\n\n\t! test_path_is_file .git/refs/heads/f\n\nwill enter the \"then\" block, complain that the file does not exist,\nreturns a failure, and your \"!\" will turn it into a success.  The\ntest passes, but the user is given an error message when the test is\nrun with \"-v\" option.\n\n>  test_expect_success 'see if git pack-refs --prune removes empty dirs' '\n>  \tgit branch r/s/t &&\n>  \tgit pack-refs --all --prune &&\n> -\t! test -e .git/refs/heads/r\n> +\t! test_path_exists .git/refs/heads/r\n>  '\n\nDitto.\n\nI'll stop here.  I think all \"! test_path_foo\" changes in this patch\nare wrong.\n\nUnlike \"test_grep\" that lets you write \"test_grep ! foo bar\" to make\nsure that file 'bar' has no 'foo' in it (and complains loudly if\n'foo' appears in 'bar'), test_path_foo family of helper functions do\nnot let you write \"test_path_exists ! no-such-file\" unfortunately.\nSo my recommendation for a microproject sample is to just drop these\nnegations from the changes and stop there.\n\nIf this were a patch for real, it would make sense to give a\npreliminary update to test_path_foo helpers to allow them to take\nnegated \"test_path_exists ! no-such-file\" form, and then convert\nthe negative form as well, but I think that would be a bit more than\nwhat a typical microproject would take.\n\nThanks.\n"},{"id":"514558","messageId":"CAPig+cSXiKOBq_+9+uDTFOLf5F8+KPocwXVX2VzPQAxJHQL99w@mail.gmail.com","threadId":"63157","inReplyTo":"xmqq5xk611o3.fsf@gitster.g","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-18T21:49:24Z","receivedAt":"2025-03-18T21:49:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 18, 2025 at 5:14 PM Junio C Hamano <gitster@pobox.com> wrote:\n> \"Sampriyo Guin via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> >  test_expect_success MINGW 'GIT_SHELL_PATH points to a suitable shell' '\n> >       shellpath=$(git var GIT_SHELL_PATH) &&\n> >       case \"$shellpath\" in\n> > -     [A-Z]:/*/sh.exe) test -f \"$shellpath\";;\n> > +     [A-Z]:/*/sh.exe) test_path_is_file \"$shellpath\";;\n> >       *) return 1;;\n> >       esac\n> >  '\n>\n> ... this one is iffy.  How well does it mesh with the \"return 1\"\n> case in the same case/esac?  I do not use Windows, but if\n> GIT_SHELL_PATH set by that system is not in a form that the platform\n> expects (i.e. a drive letter and then path to a file \"sh.exe\"), it\n> is just as grave a problem worth reporting as the path given is not\n> a regular file, yet \"return 1\" case does not give any specific error\n> message (instead it just lets the test fail), so it feels a bit\n> uneven to make the \"test -f\" failure alone louder than it currently\n> is.\n\nThanks for stating what I was thinking upon seeing this change.\n\n> >  test_expect_success 'see if git pack-refs --prune remove ref files' '\n> >       git branch f &&\n> >       git pack-refs --all --prune &&\n> > -     ! test -f .git/refs/heads/f\n> > +     ! test_path_is_file .git/refs/heads/f\n> >  '\n>\n> This conversion is wrong.  [...]\n>\n>     test_path_is_file () {\n>             test \"$#\" -ne 1 && BUG \"1 param\"\n>             if ! test -f \"$1\"\n>             then\n>                     echo \"File $1 doesn't exist\"\n>                     false\n>             fi\n>     }\n>\n> The test wants to make sure that 'f' is not a file.  So you want to\n> see a loud complaint when 'f' exists as a file.  Does it do so when\n> you say\n>\n>         ! test_path_is_file .git/refs/heads/f\n>\n> in this test?  No, it will not enter the \"then\" block and silently\n> succeed, and that return status is negated by that \"!\", so the test\n> will notice that the expectation is not met, but you didn't achieve\n> your goal of making it louder when it fail.  Worse, if the file is\n> not there, as the test expects when Git is working correctly, your\n>\n>         ! test_path_is_file .git/refs/heads/f\n>\n> will enter the \"then\" block, complain that the file does not exist,\n> returns a failure, and your \"!\" will turn it into a success.  The\n> test passes, but the user is given an error message when the test is\n> run with \"-v\" option.\n\nAnd, again, you've said everything I was going to say, thus saving me\nthe effort of doing so.\n\nReferring to the other thread at [*], perhaps this (avoiding `!` in\nfront of test_path_*) is yet another clarification which ought to be\nadded to the microproject description in order to lead candidates in a\nmore profitable direction.\n\n[*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com/\n\n> I'll stop here.  I think all \"! test_path_foo\" changes in this patch\n> are wrong.\n>\n> Unlike \"test_grep\" that lets you write \"test_grep ! foo bar\" to make\n> sure that file 'bar' has no 'foo' in it (and complains loudly if\n> 'foo' appears in 'bar'), test_path_foo family of helper functions do\n> not let you write \"test_path_exists ! no-such-file\" unfortunately.\n> So my recommendation for a microproject sample is to just drop these\n> negations from the changes and stop there.\n\nOne other recommendation I would make is to restrict the microproject\nsubmission to just a single test script (rather than updating twelve\nof them) in order to avoid exhausting the pool for other potential\ncandidates.\n"},{"id":"514584","messageId":"xmqqldt1zyia.fsf@gitster.g","threadId":"63157","inReplyTo":"CAPig+cSXiKOBq_+9+uDTFOLf5F8+KPocwXVX2VzPQAxJHQL99w@mail.gmail.com","subject":"Re: [PATCH] [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-18T23:53:49Z","receivedAt":"2025-03-18T23:53:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Referring to the other thread at [*], perhaps this (avoiding `!` in\n> front of test_path_*) is yet another clarification which ought to be\n> added to the microproject description in order to lead candidates in a\n> more profitable direction.\n>\n> [*]: https://lore.kernel.org/git/CAPig+cRm+sc+Rk-4SuQ5CrPeZLG2Nzz9B7+6OZxCq7tV5mzmBA@mail.gmail.com/\n\nWhile mentors do that, I'd prefer to see others extend support for \n\n\ttest_path_is_file ! this-should-not-be-file\n\nso that the students have a better tool to work with.\n\n> One other recommendation I would make is to restrict the microproject\n> submission to just a single test script (rather than updating twelve\n> of them) in order to avoid exhausting the pool for other potential\n> candidates.\n\nYes, you made that point in the other thread, and I agree with it\n100%.\n\nThanks.\n"},{"id":"514604","messageId":"pull.1923.v2.git.git.1742364237330.gitgitgadget@gmail.com","threadId":"63157","inReplyTo":"pull.1923.git.git.1742329571265.gitgitgadget@gmail.com","subject":"[PATCH v2] [GSoC Patch v2]Modernize Test Path Checking: test -(e|f|d)","fromName":"Sampriyo Guin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-19T06:03:57Z","receivedAt":"2025-03-19T06:04:01Z","isPatch":true,"sender":{"key":"sampriyoguin@gmail.com","avatar":null},"body":"From: Sampriyo Guin <sampriyoguin@gmail.com>\n\ntest -f changed to test_path_is_file\ntest -f does not provide a proper error message so test_path_is_file used\nChanged for t/t2003-checkout-cache-mkdir.sh\n\nSigned-off-by: Sampriyo Guin <sampriyoguin@gmail.com>\n---\n    [GSoC Patch v2] Modernize Test Path Checking in Git’s Test Suite\n    \n    test -(e|f|d) does not provide a proper error message when hit test\n    failures. So test_path_exists, test_path_is_dir, test_path_is_file used.\n    \n    Added changes for files from t/t0007-git-var.sh to\n    t/t1700-split-index.sh.\n    \n    Signed-off-by: Sampriyo Guin <sampriyoguin@gmail.com>\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1923%2FRimoGuin%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1923/RimoGuin/master-v2\nPull-Request: https://github.com/git/git/pull/1923\n\nRange-diff vs v1:\n\n 1:  432dbe6fb82 < -:  ----------- [GSoC Patch] Modernize Test Path Checking in Git’s Test Suite\n -:  ----------- > 1:  0f74a82d681 [GSoC Patch v2]Modernize Test Path Checking: test -(e|f|d)\n\n\n t/t2003-checkout-cache-mkdir.sh | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh\nindex ff163cf6750..9d2f59e0d78 100755\n--- a/t/t2003-checkout-cache-mkdir.sh\n+++ b/t/t2003-checkout-cache-mkdir.sh\n@@ -24,16 +24,16 @@ test_expect_success SYMLINKS 'have symlink in place where dir is expected.' '\n \tmkdir path2 &&\n \tln -s path2 path1 &&\n \tgit checkout-index -f -a &&\n-\ttest ! -h path1 && test -d path1 &&\n-\ttest -f path1/file1 && test ! -f path2/file1\n+\ttest ! -h path1 && test_path_is_dir path1 &&\n+\ttest_path_is_file path1/file1 && test ! -f path2/file1\n '\n \n test_expect_success 'use --prefix=path2/' '\n \trm -fr path0 path1 path2 &&\n \tmkdir path2 &&\n \tgit checkout-index --prefix=path2/ -f -a &&\n-\ttest -f path2/path0 &&\n-\ttest -f path2/path1/file1 &&\n+\ttest_path_is_file path2/path0 &&\n+\ttest_path_is_file path2/path1/file1 &&\n \ttest ! -f path0 &&\n \ttest ! -f path1/file1\n '\n@@ -41,8 +41,8 @@ test_expect_success 'use --prefix=path2/' '\n test_expect_success 'use --prefix=tmp-' '\n \trm -fr path0 path1 path2 tmp* &&\n \tgit checkout-index --prefix=tmp- -f -a &&\n-\ttest -f tmp-path0 &&\n-\ttest -f tmp-path1/file1 &&\n+\ttest_path_is_file tmp-path0 &&\n+\ttest_path_is_file tmp-path1/file1 &&\n \ttest ! -f path0 &&\n \ttest ! -f path1/file1\n '\n@@ -52,8 +52,8 @@ test_expect_success 'use --prefix=tmp- but with a conflicting file and dir' '\n \techo nitfol >tmp-path1 &&\n \tmkdir tmp-path0 &&\n \tgit checkout-index --prefix=tmp- -f -a &&\n-\ttest -f tmp-path0 &&\n-\ttest -f tmp-path1/file1 &&\n+\ttest_path_is_file tmp-path0 &&\n+\ttest_path_is_file tmp-path1/file1 &&\n \ttest ! -f path0 &&\n \ttest ! -f path1/file1\n '\n@@ -63,9 +63,9 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary/ where tmp is a symlink' '\n \tmkdir tmp1 tmp1/orary &&\n \tln -s tmp1 tmp &&\n \tgit checkout-index --prefix=tmp/orary/ -f -a &&\n-\ttest -d tmp1/orary &&\n-\ttest -f tmp1/orary/path0 &&\n-\ttest -f tmp1/orary/path1/file1 &&\n+\ttest_path_is_dir tmp1/orary &&\n+\ttest_path_is_file tmp1/orary/path0 &&\n+\ttest_path_is_file tmp1/orary/path1/file1 &&\n \ttest -h tmp\n '\n \n@@ -74,8 +74,8 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' '\n \tmkdir tmp1 &&\n \tln -s tmp1 tmp &&\n \tgit checkout-index --prefix=tmp/orary- -f -a &&\n-\ttest -f tmp1/orary-path0 &&\n-\ttest -f tmp1/orary-path1/file1 &&\n+\ttest_path_is_file tmp1/orary-path0 &&\n+\ttest_path_is_file tmp1/orary-path1/file1 &&\n \ttest -h tmp\n '\n \n@@ -84,10 +84,10 @@ test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' '\n \tmkdir tmp1 &&\n \tln -s tmp1 tmp-path1 &&\n \tgit checkout-index --prefix=tmp- -f -a &&\n-\ttest -f tmp-path0 &&\n+\ttest_path_is_file tmp-path0 &&\n \ttest ! -h tmp-path1 &&\n-\ttest -d tmp-path1 &&\n-\ttest -f tmp-path1/file1\n+\ttest_path_is_dir tmp-path1 &&\n+\ttest_path_is_file tmp-path1/file1\n '\n \n test_expect_success 'apply filter from working tree .gitattributes with --prefix' '\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n-- \ngitgitgadget\n"}]}