{"thread":{"id":"62666","subject":"[GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers","startedAt":"2024-12-18T11:17:46Z","lastAt":"2024-12-27T16:15:37Z","messageCount":7,"participants":["Meet Soni","karthik nayak","Junio C Hamano","Ghanshyam Thakkar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"509283","messageId":"20241218111715.1030357-1-meetsoni3017@gmail.com","threadId":"62666","inReplyTo":null,"subject":"[GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2024-12-18T11:17:15Z","receivedAt":"2024-12-18T11:17:46Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"test -f does not provide verbose error message on test failures, so use\ntest_path_is_file, test_path_is_missing instead.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\n t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh\nindex d6975ca48d..1a251485e1 100755\n--- a/t/t7611-merge-abort.sh\n+++ b/t/t7611-merge-abort.sh\n@@ -54,13 +54,13 @@ test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '\n '\n \n test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\"\n '\n \n test_expect_success 'fails without MERGE_HEAD (completed merge)' '\n \tgit merge clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \t# Merge successfully completed\n \tpost_merge_head=\"$(git rev-parse HEAD)\" &&\n \ttest_must_fail git merge --abort 2>output &&\n@@ -68,7 +68,7 @@ test_expect_success 'fails without MERGE_HEAD (completed merge)' '\n '\n \n test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$post_merge_head\" = \"$(git rev-parse HEAD)\"\n '\n \n@@ -79,10 +79,10 @@ test_expect_success 'Forget previous merge' '\n test_expect_success 'Abort after --no-commit' '\n \t# Redo merge, but stop before creating merge commit\n \tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort non-conflicting merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \ttest -z \"$(git diff --staged)\"\n@@ -91,10 +91,10 @@ test_expect_success 'Abort after --no-commit' '\n test_expect_success 'Abort after conflicts' '\n \t# Create conflicting merge\n \ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort conflicting merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \ttest -z \"$(git diff --staged)\"\n@@ -105,7 +105,7 @@ test_expect_success 'Clean merge with dirty index fails' '\n \tgit add foo &&\n \tgit diff --staged > expect &&\n \ttest_must_fail git merge clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \tgit diff --staged > actual &&\n@@ -114,7 +114,7 @@ test_expect_success 'Clean merge with dirty index fails' '\n \n test_expect_success 'Conflicting merge with dirty index fails' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \tgit diff --staged > actual &&\n@@ -129,10 +129,10 @@ test_expect_success 'Reset index (but preserve worktree changes)' '\n \n test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '\n \tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -141,10 +141,10 @@ test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '\n \n test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -159,7 +159,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '\n \techo xyzzy >> bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge --no-commit clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -168,7 +168,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '\n \n test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -183,7 +183,7 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '\n \techo bart > bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge --no-commit clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -194,7 +194,7 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \techo barf > bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n\nbase-commit: d882f382b3d939d90cfa58d17b17802338f05d66\n-- \n2.34.1\n\n"},{"id":"509300","messageId":"CAOLa=ZQbB=mSyHJpd+yVHKAW_jAvL3jt_Z=z-yQuKHJ=ie2gHg@mail.gmail.com","threadId":"62666","inReplyTo":"20241218111715.1030357-1-meetsoni3017@gmail.com","subject":"Re: [GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-12-18T15:23:16Z","receivedAt":"2024-12-18T15:23:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Meet Soni <meetsoni3017@gmail.com> writes:\n\n> test -f does not provide verbose error message on test failures, so use\n> test_path_is_file, test_path_is_missing instead.\n\nWhile `test -f` checks to ensure that the file exists and is a regular\nfile, I also notice that the patch contains changing `test ! -f`. This\nis a bit more tricky, since:\n1. It can be used to check if a regular file doesn't exist\n2. It can be used to check if a directory exists instead of a file\n\nThe commit message only talks about the former.\n\nThe patch itself look great, but I just noticed that the subject\nmentions 'GSoC'. As Patrick has already mentioned in your previous email\nto the list [1], we still don't have any plans with regards to GSoC 25.\nSo marking a patch in that context, doesn't make much sense (yet).\n\n[1]: https://lore.kernel.org/git/Z2AwhvaE4DLAxzDy@pks.im/\n\n[snip]\n"},{"id":"509390","messageId":"20241220130632.11826-1-meetsoni3017@gmail.com","threadId":"62666","inReplyTo":"20241218111715.1030357-1-meetsoni3017@gmail.com","subject":"[PATCH v2] t7611: replace test -f with test_path_is* helpers","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2024-12-20T13:06:32Z","receivedAt":"2024-12-20T13:06:56Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Replace `test -f` and `test ! -f` with `test_path_is_file` and\n`test_path_is_missing` for better verbosity.\n\nWhile `test -f` ensures that the file exists and is a regular file,\n`test_path_is_file` provides clearer error messages on failure. Similarly,\n`test ! -f`, used to check either the absence of a regular file or the\npresence of a directory, has been replaced with `test_path_is_missing` for\nbetter readability and consistent handling of such cases.\n\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\nUpdated commit message and subject according to review.\n\n t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh\nindex d6975ca48d..1a251485e1 100755\n--- a/t/t7611-merge-abort.sh\n+++ b/t/t7611-merge-abort.sh\n@@ -54,13 +54,13 @@ test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '\n '\n \n test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\"\n '\n \n test_expect_success 'fails without MERGE_HEAD (completed merge)' '\n \tgit merge clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \t# Merge successfully completed\n \tpost_merge_head=\"$(git rev-parse HEAD)\" &&\n \ttest_must_fail git merge --abort 2>output &&\n@@ -68,7 +68,7 @@ test_expect_success 'fails without MERGE_HEAD (completed merge)' '\n '\n \n test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$post_merge_head\" = \"$(git rev-parse HEAD)\"\n '\n \n@@ -79,10 +79,10 @@ test_expect_success 'Forget previous merge' '\n test_expect_success 'Abort after --no-commit' '\n \t# Redo merge, but stop before creating merge commit\n \tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort non-conflicting merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \ttest -z \"$(git diff --staged)\"\n@@ -91,10 +91,10 @@ test_expect_success 'Abort after --no-commit' '\n test_expect_success 'Abort after conflicts' '\n \t# Create conflicting merge\n \ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort conflicting merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \ttest -z \"$(git diff --staged)\"\n@@ -105,7 +105,7 @@ test_expect_success 'Clean merge with dirty index fails' '\n \tgit add foo &&\n \tgit diff --staged > expect &&\n \ttest_must_fail git merge clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \tgit diff --staged > actual &&\n@@ -114,7 +114,7 @@ test_expect_success 'Clean merge with dirty index fails' '\n \n test_expect_success 'Conflicting merge with dirty index fails' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \tgit diff --staged > actual &&\n@@ -129,10 +129,10 @@ test_expect_success 'Reset index (but preserve worktree changes)' '\n \n test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '\n \tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -141,10 +141,10 @@ test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '\n \n test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -159,7 +159,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '\n \techo xyzzy >> bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge --no-commit clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -168,7 +168,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '\n \n test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -183,7 +183,7 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '\n \techo bart > bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge --no-commit clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -194,7 +194,7 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \techo barf > bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n-- \n2.34.1\n\n"},{"id":"509391","messageId":"xmqqcyhmh22q.fsf@gitster.g","threadId":"62666","inReplyTo":"20241220130632.11826-1-meetsoni3017@gmail.com","subject":"Re: [PATCH v2] t7611: replace test -f with test_path_is* helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-20T14:16:29Z","receivedAt":"2024-12-20T14:16:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Meet Soni <meetsoni3017@gmail.com> writes:\n\n> Replace `test -f` and `test ! -f` with `test_path_is_file` and\n> `test_path_is_missing` for better verbosity.\n\nOK.  \"verbosity\" -> \"debuggability\" perhaps, but that is minor.\n\n> While `test -f` ensures that the file exists and is a regular file,\n> `test_path_is_file` provides clearer error messages on failure.\n\nCorrect.\n\n> Similarly,\n> `test ! -f`, used to check either the absence of a regular file or the\n> presence of a directory, has been replaced with `test_path_is_missing` for\n> better readability and consistent handling of such cases.\n\nThis is misleading.  If you rewrite \"test ! -f\" that intends to be\nhappy when it sees a directory, you would be changing the meaning of\nthe program if you replace it with test_path_is_missing.  Rather,\nwhen you see \"test ! -f foo\", you should first think if the intent\nof the test script there is to allow presence of \"foo\" that is not a\nfile, and if that is not the case, in other words, the \"test ! -f\"\nyou see should have been written \"test ! -e\" (i.e. \"I do not want to\nsee 'foo' there on the filesystem, no matter what kind of filesystem\nentity it is!\"), it would give us the same meaning with better\ndebuggability to rewrite such \"test ! -f\" with test_path_is_missing.\n\nIOW, unlike \"test -f foo\" that can pretty much blindly replaceable\nwith \"test_path_is_file foo\" without thinking at all, you must be a\nbit more careful.  And _if_ you did such a more careful analysis\nbefore replacing \"test ! -f\", then you should restate the above,\nperhaps something along the lines of ...\n\n    On the other hand, `test ! -f` literally means there should not\n    be a file (implication is that we are OK if there is a directory\n    or device nodes or other things at the given path).  But by\n    looking at each of these in the test individually, many of them\n    should rather have said \"test ! -e\", i.e. \"there shouldn't be\n    anything at the given path on the filesystem\".  Rewrite these\n    cases to test_path_is_missing for better debuggability.\n\nI didn't check myself if all of the \"test ! -f\" you touched are\nindeed the original should have said \"test ! -e\".  Hopefully you\nhave done it yourself?\n\nThanks.\n"},{"id":"509602","messageId":"20241227105345.10184-1-meetsoni3017@gmail.com","threadId":"62666","inReplyTo":"20241220130632.11826-1-meetsoni3017@gmail.com","subject":"[PATCH v3] t7611: replace test -f with test_path_is* helpers","fromName":"Meet Soni","fromEmail":"meetsoni3017@gmail.com","sentAt":"2024-12-27T10:53:45Z","receivedAt":"2024-12-27T10:54:07Z","isPatch":true,"sender":{"key":"meetsoni3017@gmail.com","avatar":"https://avatars.githubusercontent.com/u/92802561?v=4"},"body":"Replace `test -f` and `test ! -f` with `test_path_is_file` and\n`test_path_is_missing` for better debuggability.\n\nWhile `test -f` ensures that the file exists and is a regular file,\n`test_path_is_file` provides clearer error messages on failure. On the\nother hand, `test ! -f`, used to check either the absence of a regular\nfile or the presence of any other filesystem object, but looking at\nthem in the test individually, all of them should've said `test ! e`,\ni.e. \"there shouldn't be anything at given path on filesystem.\"\nReplaced these cases with `test_path_is_missing` for better\ndebuggability.\n\nHelped-by: karthik nayak <karthik.188@gmail.com>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Meet Soni <meetsoni3017@gmail.com>\n---\nApologies for late response.\nUpdated commit message for better clarification of changes made.\n t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh\nindex d6975ca48d..1a251485e1 100755\n--- a/t/t7611-merge-abort.sh\n+++ b/t/t7611-merge-abort.sh\n@@ -54,13 +54,13 @@ test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '\n '\n \n test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\"\n '\n \n test_expect_success 'fails without MERGE_HEAD (completed merge)' '\n \tgit merge clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \t# Merge successfully completed\n \tpost_merge_head=\"$(git rev-parse HEAD)\" &&\n \ttest_must_fail git merge --abort 2>output &&\n@@ -68,7 +68,7 @@ test_expect_success 'fails without MERGE_HEAD (completed merge)' '\n '\n \n test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$post_merge_head\" = \"$(git rev-parse HEAD)\"\n '\n \n@@ -79,10 +79,10 @@ test_expect_success 'Forget previous merge' '\n test_expect_success 'Abort after --no-commit' '\n \t# Redo merge, but stop before creating merge commit\n \tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort non-conflicting merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \ttest -z \"$(git diff --staged)\"\n@@ -91,10 +91,10 @@ test_expect_success 'Abort after --no-commit' '\n test_expect_success 'Abort after conflicts' '\n \t# Create conflicting merge\n \ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort conflicting merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \ttest -z \"$(git diff --staged)\"\n@@ -105,7 +105,7 @@ test_expect_success 'Clean merge with dirty index fails' '\n \tgit add foo &&\n \tgit diff --staged > expect &&\n \ttest_must_fail git merge clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \tgit diff --staged > actual &&\n@@ -114,7 +114,7 @@ test_expect_success 'Clean merge with dirty index fails' '\n \n test_expect_success 'Conflicting merge with dirty index fails' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff)\" &&\n \tgit diff --staged > actual &&\n@@ -129,10 +129,10 @@ test_expect_success 'Reset index (but preserve worktree changes)' '\n \n test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '\n \tgit merge --no-commit clean_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -141,10 +141,10 @@ test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '\n \n test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest -f .git/MERGE_HEAD &&\n+\ttest_path_is_file .git/MERGE_HEAD &&\n \t# Abort merge\n \tgit merge --abort &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -159,7 +159,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '\n \techo xyzzy >> bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge --no-commit clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -168,7 +168,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '\n \n test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -183,7 +183,7 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '\n \techo bart > bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge --no-commit clean_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n@@ -194,7 +194,7 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '\n \techo barf > bar &&\n \tgit diff > expect &&\n \ttest_must_fail git merge conflict_branch &&\n-\ttest ! -f .git/MERGE_HEAD &&\n+\ttest_path_is_missing .git/MERGE_HEAD &&\n \ttest \"$pre_merge_head\" = \"$(git rev-parse HEAD)\" &&\n \ttest -z \"$(git diff --staged)\" &&\n \tgit diff > actual &&\n-- \n2.34.1\n\n"},{"id":"509607","messageId":"D6MH7E17E6I0.3IG5103E7XXP3@gmail.com","threadId":"62666","inReplyTo":"20241227105345.10184-1-meetsoni3017@gmail.com","subject":"Re: [PATCH v3] t7611: replace test -f with test_path_is* helpers","fromName":"Ghanshyam Thakkar","fromEmail":"shyamthakkar001@gmail.com","sentAt":"2024-12-27T12:19:17Z","receivedAt":"2024-12-27T12:19:23Z","isPatch":true,"sender":{"key":"shyamthakkar001@gmail.com","avatar":"https://avatars.githubusercontent.com/u/72698233?v=4"},"body":"On Fri Dec 27, 2024 at 4:23 PM IST, Meet Soni wrote:\n> Replace `test -f` and `test ! -f` with `test_path_is_file` and\n> `test_path_is_missing` for better debuggability.\n>\n> While `test -f` ensures that the file exists and is a regular file,\n> `test_path_is_file` provides clearer error messages on failure. On the\n> other hand, `test ! -f`, used to check either the absence of a regular\n> file or the presence of any other filesystem object, but looking at\n> them in the test individually, all of them should've said `test ! e`,\n> i.e. \"there shouldn't be anything at given path on filesystem.\"\n> Replaced these cases with `test_path_is_missing` for better\n> debuggability.\n\n'Replaced' -> 'Replace'. Cf. https://git-scm.com/docs/SubmittingPatches#imperative-mood\n\nOther than that, this LGTM.\n\nThanks.\n"},{"id":"509641","messageId":"xmqqmsght848.fsf@gitster.g","threadId":"62666","inReplyTo":"D6MH7E17E6I0.3IG5103E7XXP3@gmail.com","subject":"Re: [PATCH v3] t7611: replace test -f with test_path_is* helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T16:15:35Z","receivedAt":"2024-12-27T16:15:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ghanshyam Thakkar\" <shyamthakkar001@gmail.com> writes:\n\n> On Fri Dec 27, 2024 at 4:23 PM IST, Meet Soni wrote:\n>> Replace `test -f` and `test ! -f` with `test_path_is_file` and\n>> `test_path_is_missing` for better debuggability.\n>>\n>> While `test -f` ensures that the file exists and is a regular file,\n>> `test_path_is_file` provides clearer error messages on failure. On the\n>> other hand, `test ! -f`, used to check either the absence of a regular\n>> file or the presence of any other filesystem object, but looking at\n>> them in the test individually, all of them should've said `test ! e`,\n>> i.e. \"there shouldn't be anything at given path on filesystem.\"\n>> Replaced these cases with `test_path_is_missing` for better\n>> debuggability.\n>\n> 'Replaced' -> 'Replace'. Cf. https://git-scm.com/docs/SubmittingPatches#imperative-mood\n>\n> Other than that, this LGTM.\n\nThanks, both.  Tweaked the log message before applying.  No need to\nresend.\n\nQueued.\n"}]}