{"thread":{"id":"63078","subject":"[GSoC PATCH] t9400: prefer test_path_* helper functions","startedAt":"2025-03-08T09:04:20Z","lastAt":"2025-03-18T22:06:45Z","messageCount":8,"participants":["Aryan Pathania","Patrick Steinhardt","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"513828","messageId":"20250308090358.25429-1-contact@aynp.dev","threadId":"63078","inReplyTo":null,"subject":"[GSoC PATCH] t9400: prefer test_path_* helper functions","fromName":"Aryan Pathania","fromEmail":"contact@aynp.dev","sentAt":"2025-03-08T09:03:58Z","receivedAt":"2025-03-08T09:04:20Z","isPatch":true,"sender":{"key":"contact@aynp.dev","avatar":null},"body":"use `test_path_*` instead of `test -[efd]` to avoid false complaints and\noutput when running the script with `-v` or `-x`\n\nSigned-off-by: Aryan Pathania <contact@aynp.dev>\n---\n t/t9400-git-cvsserver-server.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex e499c7f955..6c7cb1964c 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \\\n      true\n    fi &&\n    grep \"GITCVS emulation disabled\" cvs.log &&\n-   test ! -d cvswork2'\n+   test_path_is_missing cvswork2'\n \n rm -fr cvswork2\n test_expect_success 'gitcvs.ext.enabled = true' '\n@@ -276,7 +276,7 @@ test_expect_success 'gitcvs.ext.enabled = false' '\n \t\ttrue\n \tfi &&\n \tgrep \"GITCVS emulation disabled\" cvs.log &&\n-\ttest ! -d cvswork2\n+\ttest_path_is_missing cvswork2\n '\n \n rm -fr cvswork2\n@@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '\n \tGIT_DIR=\"$SERVERDIR\" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&\n \ttest_cmp cvswork cvswork2 &&\n-\ttest -f \"$SERVERDIR/gitcvs.ext.main.sqlite\" &&\n+\ttest_path_is_file_not_symlink \"$SERVERDIR/gitcvs.ext.main.sqlite\" &&\n \tcmp \"$SERVERDIR/gitcvs.main.sqlite\" \"$SERVERDIR/gitcvs.ext.main.sqlite\"\n '\n \n@@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '\n \tGIT_DIR=\"$SERVERDIR\" git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&\n \ttest_cmp cvswork cvswork2 &&\n-\ttest -f \"$SERVERDIR/gitcvs1.ext.main.sqlite\" &&\n-\ttest ! -f \"$SERVERDIR/gitcvs2.ext.main.sqlite\" &&\n+\ttest_path_is_file_not_symlink \"$SERVERDIR/gitcvs1.ext.main.sqlite\" &&\n+\ttest_path_is_missing \"$SERVERDIR/gitcvs2.ext.main.sqlite\" &&\n \tcmp \"$SERVERDIR/gitcvs.main.sqlite\" \"$SERVERDIR/gitcvs1.ext.main.sqlite\"\n '\n \n@@ -346,7 +346,7 @@ test_expect_failure \"cvs update w/o -d doesn't create subdir (TODO)\" '\n \tgit push gitcvs.git >/dev/null &&\n \tcd cvswork &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q update &&\n-\ttest ! -d test\n+\ttest_path_is_missing test\n '\n \n cd \"$WORKDIR\"\n@@ -379,7 +379,7 @@ test_expect_success 'cvs update (delete file)' '\n \tcd cvswork &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q update &&\n \ttest -z \"$(grep testfile1 CVS/Entries)\" &&\n-\ttest ! -f testfile1\n+\ttest_path_is_missing testfile1\n '\n \n cd \"$WORKDIR\"\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"513869","messageId":"Z86LrOEhH3CJOIey@pks.im","threadId":"63078","inReplyTo":"20250308090358.25429-1-contact@aynp.dev","subject":"Re: [GSoC PATCH] t9400: prefer test_path_* helper functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-10T06:50:20Z","receivedAt":"2025-03-10T06:50:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Mar 08, 2025 at 06:03:58PM +0900, Aryan Pathania wrote:\n> use `test_path_*` instead of `test -[efd]` to avoid false complaints and\n\nNit: We want to have full sentences in commit messages. Those sentences\nshould start with an upper-case letter and end with punctuation.\n\n> output when running the script with `-v` or `-x`\n\nHm. I'm not exactly sure what \"false complaints\" you are talking about\nhere. The benefit of `test_path_*`()` over `test -[efd]` is that they\nactually print information _why_ they have failed, which may help devs\nto figure out what's happening. So it's kind of the opposite of what you\nsay in the commit message: we're now printing _more_ output, not less.\n\n> Signed-off-by: Aryan Pathania <contact@aynp.dev>\n> ---\n>  t/t9400-git-cvsserver-server.sh | 14 +++++++-------\n>  1 file changed, 7 insertions(+), 7 deletions(-)\n> \n> diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\n> index e499c7f955..6c7cb1964c 100755\n> --- a/t/t9400-git-cvsserver-server.sh\n> +++ b/t/t9400-git-cvsserver-server.sh\n> @@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \\\n>       true\n>     fi &&\n>     grep \"GITCVS emulation disabled\" cvs.log &&\n> -   test ! -d cvswork2'\n> +   test_path_is_missing cvswork2'\n\nThis isn't quite equivalent: we've been checking that the path is not a\ndirectory before, but now we verify that the path doesn't exist. It's a\nsensible change in this context though as our new assertion is stronger\nthan the old one, but it might make sense to point out in the commit\nmessage.\n\n> @@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '\n>  \tGIT_DIR=\"$SERVERDIR\" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&\n>  \tGIT_CONFIG=\"$git_config\" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&\n>  \ttest_cmp cvswork cvswork2 &&\n> -\ttest -f \"$SERVERDIR/gitcvs.ext.main.sqlite\" &&\n> +\ttest_path_is_file_not_symlink \"$SERVERDIR/gitcvs.ext.main.sqlite\" &&\n>  \tcmp \"$SERVERDIR/gitcvs.main.sqlite\" \"$SERVERDIR/gitcvs.ext.main.sqlite\"\n>  '\n>  \n\nWe tend to use `test_path_is_file` rather than\n`test_path_is_file_not_symlink`, but I don't mind it too much.\n\nPatrick\n"},{"id":"514090","messageId":"r5572ospfh3d7nwniod36jcy5ikv5pkmiwtqj25ll7p5ts3zay@okbxrhy77iyv","threadId":"63078","inReplyTo":"Z86LrOEhH3CJOIey@pks.im","subject":"Re: [GSoC PATCH] t9400: prefer test_path_* helper functions","fromName":"Aryan Pathania","fromEmail":"contact@aynp.dev","sentAt":"2025-03-12T17:34:52Z","receivedAt":"2025-03-12T17:34:56Z","isPatch":true,"sender":{"key":"contact@aynp.dev","avatar":null},"body":"Hi Patrick,\nThank you so much for taking the time out of your schedule to review my\npatch.\n\n>> use `test_path_*` instead of `test -[efd]` to avoid false complaints and\n>\n>Nit: We want to have full sentences in commit messages. Those sentences\n>should start with an upper-case letter and end with punctuation.\nI understand. Sorry for not knowing this.\n\n>Hm. I'm not exactly sure what \"false complaints\" you are talking about\n>here. The benefit of `test_path_*`()` over `test -[efd]` is that they\n>actually print information _why_ they have failed, which may help devs\n>to figure out what's happening. So it's kind of the opposite of what you\n>say in the commit message: we're now printing _more_ output, not less.\nI'm really sorry, I got confused in using `! test_path_*` and `test !*`.\nBad mistake while writing the message.\n\n>This isn't quite equivalent: we've been checking that the path is not a\n>directory before, but now we verify that the path doesn't exist.\nI understand. I could not find `test_path_is_not_dir` or any equivalent\nfunction in `test-lib-functions.sh`. Maybe we can keep this stronger\ncheck. I'll mention in the commit message of next version of patch.\n\n>We tend to use `test_path_is_file` rather than\n>`test_path_is_file_not_symlink`, but I don't mind it too much.\nI believe `test -f` is equivalent to `test_path_is_file_not_symlink` and\nis a stronger check so maybe it's fine.\n\nI'll make the required changes in a new patch version. Sorry for the\ntrouble and mistakes.\n\n"},{"id":"514091","messageId":"xmqqplimxgkb.fsf@gitster.g","threadId":"63078","inReplyTo":"r5572ospfh3d7nwniod36jcy5ikv5pkmiwtqj25ll7p5ts3zay@okbxrhy77iyv","subject":"Re: [GSoC PATCH] t9400: prefer test_path_* helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-12T18:13:56Z","receivedAt":"2025-03-12T18:13:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aryan Pathania <contact@aynp.dev> writes:\n\n>>This isn't quite equivalent: we've been checking that the path is not a\n>>directory before, but now we verify that the path doesn't exist.\n> I understand. I could not find `test_path_is_not_dir` or any equivalent\n> function in `test-lib-functions.sh`. Maybe we can keep this stronger\n> check. I'll mention in the commit message of next version of patch.\n\nThat is exactly Patrick suggested (go back and read it).  I agree\nwith him that the updated stronger check is an improvement and it\ndeserves to be explained in the commit message.\n\n>>We tend to use `test_path_is_file` rather than\n>>`test_path_is_file_not_symlink`, but I don't mind it too much.\n> I believe `test -f` is equivalent to `test_path_is_file_not_symlink` and\n> is a stronger check so maybe it's fine.\n\nDon't believe what you think you know; if you are unsure, check to\nverify before you base your actions on them.\n\n    $ >this-is-file\n    $ ln -s this-is-file this-is-symlink\n    $ test -f this-is-symlink; echo $?\n    0\n    $ test -f this-is-file; echo $?\n    0\n\nAnd if you are not unsure, then learn to be unsure more often ;-)\n\nI do not see any reason in the part of the code Patrick commented on\nto insist that gitcvs.ext.main.sqlite file must be a regular file\nand not a symbolic link to another file.  Both test_path_is_file\nand its original before the patch, \"test -f\", would be more appropriate\nthan test_path_is_file_not_symlink, which was specifically invented\nfor use in t3903 where the tests used both files and symbolic links\nto make sure the operation being tested would not confuse one with\nthe other.\n\n> Sorry for the trouble and mistakes.\n\nNo need for that.  This is for both sides to experience to learn to\nwork well together.\n"},{"id":"514258","messageId":"gm67nhqkoa5adxz5ifckpvecn5f5hewubmcm62nr4hejlwbhsg@pmo472wkj7of","threadId":"63078","inReplyTo":"xmqqplimxgkb.fsf@gitster.g","subject":"Re: [GSoC PATCH] t9400: prefer test_path_* helper functions","fromName":"Aryan Pathania","fromEmail":"aryan.pathania2003@gmail.com","sentAt":"2025-03-14T12:00:22Z","receivedAt":"2025-03-14T12:00:26Z","isPatch":true,"sender":{"key":"aryan.pathania2003@gmail.com","avatar":null},"body":"Hi Junio, thanks a lot for taking time to review my patch.\n\n>That is exactly Patrick suggested (go back and read it).  I agree\n>with him that the updated stronger check is an improvement and it\n>deserves to be explained in the commit message.\nI apologize for miscomunication. I was agreeing with the suggestion. It\nwas redundant on my part.\n\n>    $ >this-is-file\n>    $ ln -s this-is-file this-is-symlink\n>    $ test -f this-is-symlink; echo $?\n>    0\n>    $ test -f this-is-file; echo $?\n>    0\n>\n>And if you are not unsure, then learn to be unsure more often ;-)\n\nAh, thank you. Sorry I din't know symlink were regular files. I'll be\nmore careful with other things next time.\n\n"},{"id":"514272","messageId":"xmqqtt7vilr7.fsf@gitster.g","threadId":"63078","inReplyTo":"gm67nhqkoa5adxz5ifckpvecn5f5hewubmcm62nr4hejlwbhsg@pmo472wkj7of","subject":"Re: [GSoC PATCH] t9400: prefer test_path_* helper functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-14T17:07:40Z","receivedAt":"2025-03-14T17:07:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Aryan Pathania <aryan.pathania2003@gmail.com> writes:\n\n> Hi Junio, thanks a lot for taking time to review my patch.\n>\n>>That is exactly Patrick suggested (go back and read it).  I agree\n>>with him that the updated stronger check is an improvement and it\n>>deserves to be explained in the commit message.\n> I apologize for miscomunication. I was agreeing with the suggestion.\n\nSorry for misreading what you wrote.  Mistakes happen, so please\ndon't over-apologize around here ;-)\n"},{"id":"514525","messageId":"20250318131033.48691-1-contact@aynp.dev","threadId":"63078","inReplyTo":"20250308090358.25429-1-contact@aynp.dev","subject":"[GSoC PATCH v2] Use `test_path_*` helper functions instead of `test -[efd]`.","fromName":"Aryan Pathania","fromEmail":"contact@aynp.dev","sentAt":"2025-03-18T13:10:33Z","receivedAt":"2025-03-18T13:11:16Z","isPatch":true,"sender":{"key":"contact@aynp.dev","avatar":null},"body":"Change testcase `gitcvs.enabled = false` to check for missing path\ninstead of a missing file. The change is justified as new assertion is\nstronger.\n\nAll other testcases remain equivalent.\n---\n t/t9400-git-cvsserver-server.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex e499c7f955..4ddde382e8 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -254,7 +254,7 @@ test_expect_success 'gitcvs.enabled = false' \\\n      true\n    fi &&\n    grep \"GITCVS emulation disabled\" cvs.log &&\n-   test ! -d cvswork2'\n+   test_path_is_missing cvswork2'\n \n rm -fr cvswork2\n test_expect_success 'gitcvs.ext.enabled = true' '\n@@ -276,7 +276,7 @@ test_expect_success 'gitcvs.ext.enabled = false' '\n \t\ttrue\n \tfi &&\n \tgrep \"GITCVS emulation disabled\" cvs.log &&\n-\ttest ! -d cvswork2\n+\ttest_path_is_missing cvswork2\n '\n \n rm -fr cvswork2\n@@ -285,7 +285,7 @@ test_expect_success 'gitcvs.dbname' '\n \tGIT_DIR=\"$SERVERDIR\" git config gitcvs.dbname %Ggitcvs.%a.%m.sqlite &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&\n \ttest_cmp cvswork cvswork2 &&\n-\ttest -f \"$SERVERDIR/gitcvs.ext.main.sqlite\" &&\n+\ttest_path_is_file \"$SERVERDIR/gitcvs.ext.main.sqlite\" &&\n \tcmp \"$SERVERDIR/gitcvs.main.sqlite\" \"$SERVERDIR/gitcvs.ext.main.sqlite\"\n '\n \n@@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '\n \tGIT_DIR=\"$SERVERDIR\" git config gitcvs.dbname %Ggitcvs2.%a.%m.sqlite &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q co -d cvswork2 main >cvs.log 2>&1 &&\n \ttest_cmp cvswork cvswork2 &&\n-\ttest -f \"$SERVERDIR/gitcvs1.ext.main.sqlite\" &&\n-\ttest ! -f \"$SERVERDIR/gitcvs2.ext.main.sqlite\" &&\n+\ttest_path_is_file \"$SERVERDIR/gitcvs1.ext.main.sqlite\" &&\n+\ttest_path_is_missing \"$SERVERDIR/gitcvs2.ext.main.sqlite\" &&\n \tcmp \"$SERVERDIR/gitcvs.main.sqlite\" \"$SERVERDIR/gitcvs1.ext.main.sqlite\"\n '\n \n@@ -346,7 +346,7 @@ test_expect_failure \"cvs update w/o -d doesn't create subdir (TODO)\" '\n \tgit push gitcvs.git >/dev/null &&\n \tcd cvswork &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q update &&\n-\ttest ! -d test\n+\ttest_path_is_missing test\n '\n \n cd \"$WORKDIR\"\n@@ -379,7 +379,7 @@ test_expect_success 'cvs update (delete file)' '\n \tcd cvswork &&\n \tGIT_CONFIG=\"$git_config\" cvs -Q update &&\n \ttest -z \"$(grep testfile1 CVS/Entries)\" &&\n-\ttest ! -f testfile1\n+\ttest_path_is_missing testfile1\n '\n \n cd \"$WORKDIR\"\n-- \n2.39.5 (Apple Git-154)\n\n"},{"id":"514560","messageId":"CAPig+cSnhjmni5FQhrWxfbtxkZq_sbHZp5CNAGHEQCaET5=iAg@mail.gmail.com","threadId":"63078","inReplyTo":"20250318131033.48691-1-contact@aynp.dev","subject":"Re: [GSoC PATCH v2] Use `test_path_*` helper functions instead of `test -[efd]`.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-18T22:06:33Z","receivedAt":"2025-03-18T22:06:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for your GSoC microproject submission. See comments below...\n\nOn Tue, Mar 18, 2025 at 9:11 AM Aryan Pathania <contact@aynp.dev> wrote:\n> Use `test_path_*` helper functions instead of `test -[efd]`\n\nAccording to Documentation/SubmittingPatches, you'd probably instead\nwant to compose the commit message summary line something like this:\n\n    t9400: use test_path_*` functions to improve diagnostic output\n\n> Change testcase `gitcvs.enabled = false` to check for missing path\n> instead of a missing file. The change is justified as new assertion is\n> stronger.\n\nThe description seems somehow outdated since this particular change is\nbeing made to more than just that one test, isn't it?\n\nRather than describing the new assertion as \"stronger\", it might make\nmore sense to state that it is more semantically in line with what is\nactually being tested (i.e. that the directory/path should not exist\nwhen, as expected, the command fails).\n\n> All other testcases remain equivalent.\n\nOkay, but...\n\n> @@ -296,8 +296,8 @@ test_expect_success 'gitcvs.ext.dbname' '\n>         test_cmp cvswork cvswork2 &&\n> -       test -f \"$SERVERDIR/gitcvs1.ext.main.sqlite\" &&\n> -       test ! -f \"$SERVERDIR/gitcvs2.ext.main.sqlite\" &&\n> +       test_path_is_file \"$SERVERDIR/gitcvs1.ext.main.sqlite\" &&\n> +       test_path_is_missing \"$SERVERDIR/gitcvs2.ext.main.sqlite\" &&\n\n... although `test_path_is_file` is a faithful replacement for `test\n-f`, it could be argued that `test_path_exists` would be a\nsemantically better choice, especially given the use of\n`test_path_is_missing` for the sibling case.\n\nThe same comment applies to one or two other changes in this patch.\n"}]}