{"thread":{"id":"59225","subject":"[RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*","startedAt":"2023-02-10T08:01:30Z","lastAt":"2023-02-14T23:31:01Z","messageCount":4,"participants":["Vinayak Dev","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471900","messageId":"20230210080110.32983-1-vinayakdev.sci@gmail.com","threadId":"59225","inReplyTo":null,"subject":"[RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-10T08:01:10Z","receivedAt":"2023-02-10T08:01:30Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"Changes test -d and test -f commands in\nt/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and\ntest_path_is_file respectively, which are helper functions defined in\nt/test-lib-functions.sh\n\nSigned-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>\n\n\nvinayakdsci (1):\n  Change test -(d | f) to corresponding test_path_*\n\n t/t9160-git-svn-preserve-empty-dirs.sh | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\n\ndiff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh\nindex 36c6b1a12f..963b081a1c 100755\n--- a/t/t9160-git-svn-preserve-empty-dirs.sh\n+++ b/t/t9160-git-svn-preserve-empty-dirs.sh\n@@ -61,15 +61,15 @@ test_expect_success 'clone svn repo with --preserve-empty-dirs' '\n \n # \"$GIT_REPO\"/1 should only contain the placeholder file.\n test_expect_success 'directory empty from inception' '\n-\ttest -f \"$GIT_REPO\"/1/.gitignore &&\n+\ttest_path_is_file \"$GIT_REPO\"/1/.gitignore &&\n \ttest $(find \"$GIT_REPO\"/1 -type f | wc -l) = \"1\"\n '\n \n # \"$GIT_REPO\"/2 and \"$GIT_REPO\"/3 should only contain the placeholder file.\n test_expect_success 'directory empty from subsequent svn commit' '\n-\ttest -f \"$GIT_REPO\"/2/.gitignore &&\n+\ttest_path_is_file \"$GIT_REPO\"/2/.gitignore &&\n \ttest $(find \"$GIT_REPO\"/2 -type f | wc -l) = \"1\" &&\n-\ttest -f \"$GIT_REPO\"/3/.gitignore &&\n+\ttest_path_is_file \"$GIT_REPO\"/3/.gitignore &&\n \ttest $(find \"$GIT_REPO\"/3 -type f | wc -l) = \"1\"\n '\n \n@@ -77,7 +77,7 @@ test_expect_success 'directory empty from subsequent svn commit' '\n # generated for every sub-directory at some point in the repo's history.\n test_expect_success 'add entry to previously empty directory' '\n \ttest $(find \"$GIT_REPO\"/4 -type f | wc -l) = \"1\" &&\n-\ttest -f \"$GIT_REPO\"/4/a/b/c/foo\n+\ttest_path_is_file \"$GIT_REPO\"/4/a/b/c/foo\n '\n \n # The HEAD~2 commit should not have introduced .gitignore placeholder files.\n@@ -108,8 +108,8 @@ test_expect_success 'placeholder namespace conflict with file' '\n # \"$GIT_REPO\"/6/.placeholder should be a directory, and the \"$GIT_REPO\"/6 tree\n # should only contain one file: the placeholder.\n test_expect_success 'placeholder namespace conflict with directory' '\n-\ttest -d \"$GIT_REPO\"/6/.placeholder &&\n-\ttest -f \"$GIT_REPO\"/6/.placeholder/.placeholder &&\n+\ttest_path_is_dir \"$GIT_REPO\"/6/.placeholder &&\n+\ttest_path_is_file \"$GIT_REPO\"/6/.placeholder/.placeholder &&\n \ttest $(find \"$GIT_REPO\"/6 -type f | wc -l) = \"1\"\n '\n \n@@ -134,18 +134,18 @@ test_expect_success 'second set of svn commits and rebase' '\n # Check that --preserve-empty-dirs and --placeholder-file flag state\n # stays persistent over multiple invocations.\n test_expect_success 'flag persistence during subsqeuent rebase' '\n-\ttest -f \"$GIT_REPO\"/7/.placeholder &&\n+\ttest_path_is_file \"$GIT_REPO\"/7/.placeholder &&\n \ttest $(find \"$GIT_REPO\"/7 -type f | wc -l) = \"1\"\n '\n \n # Check that placeholder files are properly removed when unnecessary,\n # even across multiple invocations.\n test_expect_success 'placeholder list persistence during subsqeuent rebase' '\n-\ttest -f \"$GIT_REPO\"/1/file1.txt &&\n+\ttest_path_is_file \"$GIT_REPO\"/1/file1.txt &&\n \ttest $(find \"$GIT_REPO\"/1 -type f | wc -l) = \"1\" &&\n \n-\ttest -f \"$GIT_REPO\"/5/file1.txt &&\n-\ttest -f \"$GIT_REPO\"/5/.placeholder &&\n+\ttest_path_is_file \"$GIT_REPO\"/5/file1.txt &&\n+\ttest_path_is_file \"$GIT_REPO\"/5/.placeholder &&\n \ttest $(find \"$GIT_REPO\"/5 -type f | wc -l) = \"2\"\n '\n \n-- \n2.39.1\n"},{"id":"471912","messageId":"CAPig+cS-9gbxb3tWqaWtsjfiQR60GQXpfCKM7-neu6AH8rUvcA@mail.gmail.com","threadId":"59225","inReplyTo":"20230210080110.32983-1-vinayakdev.sci@gmail.com","subject":"Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-10T11:08:29Z","receivedAt":"2023-02-10T11:08:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> Changes test -d and test -f commands in\n\ns/Changes/Change/\n\n> t/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and\n> test_path_is_file respectively, which are helper functions defined in\n> t/test-lib-functions.sh\n\nThis summarizes the changes made by the patch, but readers also want\nto know _why_ the changes are being made; in fact, \"why\" is more\nimportant. Previous patches similar to this one have explained that\ntest_path_is_dir() and test_path_is_file() are superior to `test -d`\nand `test -f` because the functions provide diagnostic output when\nthey fail, and that diagnostic output can make it easier to debug a\nfailing tests. So, if you reroll the patch, focus on explaining the\nbenefits of the functions rather than explaining the mechanical\nchanges made by the patch.\n\n> Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>\n>\n> vinayakdsci (1):\n>   Change test -(d | f) to corresponding test_path_*\n\nAs with the other GSoC patch you submitted[1], this one is also\nmissing the \"---\" line below your sign-off, which tells git-am where\nthe commit message ends. As mentioned in [2] you may need to adjust\nyour tools or workflow to prevent the \"---\" line from being stripped.\n\nThe actual changes made by the patch are probably reasonable (though I\ndon't have CVS libraries installed presently, so I wasn't able to\nactually test the changes).\n\n[1]: https://lore.kernel.org/git/20230209164552.8971-1-vinayakdev.sci@gmail.com/\n[2]: https://lore.kernel.org/git/CAPig+cT1EtPO2FLvTsw3SjgCgk=ovNwY77hsX6p7ETKiq8aNog@mail.gmail.com/\n"},{"id":"471914","messageId":"CADE8Naoq4N9EpSfb8=ADPRLeAsmjVhL+ayOzhS=syaZ0PQh71w@mail.gmail.com","threadId":"59225","inReplyTo":"CAPig+cS-9gbxb3tWqaWtsjfiQR60GQXpfCKM7-neu6AH8rUvcA@mail.gmail.com","subject":"Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*","fromName":"Vinayak Dev","fromEmail":"vinayakdev.sci@gmail.com","sentAt":"2023-02-10T11:32:30Z","receivedAt":"2023-02-10T11:32:55Z","isPatch":true,"sender":{"key":"vinayakdev.sci@gmail.com","avatar":"https://avatars.githubusercontent.com/u/104419489?v=4"},"body":"On Fri, 10 Feb 2023 at 16:38, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> > Changes test -d and test -f commands in\n>\n> s/Changes/Change/\n\nSorry for this error, I actually sent the patch before I got your\nreview of the prior one.\n\n> > t/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and\n> > test_path_is_file respectively, which are helper functions defined in\n> > t/test-lib-functions.sh\n>\n> This summarizes the changes made by the patch, but readers also want\n> to know _why_ the changes are being made; in fact, \"why\" is more\n> important. Previous patches similar to this one have explained that\n> test_path_is_dir() and test_path_is_file() are superior to `test -d`\n> and `test -f` because the functions provide diagnostic output when\n> they fail, and that diagnostic output can make it easier to debug a\n> failing tests. So, if you reroll the patch, focus on explaining the\n> benefits of the functions rather than explaining the mechanical\n> changes made by the patch.\n\nThis is correct, I will make the message more informative about the\nrelevant aspects of the changes.\n\n> > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com>\n> >\n> > vinayakdsci (1):\n> >   Change test -(d | f) to corresponding test_path_*\n>\n> As with the other GSoC patch you submitted[1], this one is also\n> missing the \"---\" line below your sign-off, which tells git-am where\n> the commit message ends. As mentioned in [2] you may need to adjust\n> your tools or workflow to prevent the \"---\" line from being stripped.\n\nI apologise for this mistake again.\n\n> The actual changes made by the patch are probably reasonable (though I\n> don't have CVS libraries installed presently, so I wasn't able to\n> actually test the changes).\n\nIf by testing you mean running the test file to ensure there are no\nerrors, I did do it thoroughly before sending the patch,\nso I can vouch for the changes :)\n\nThanks a lot!\nVinayak\n"},{"id":"472098","messageId":"CAPig+cQK_aW4m4LToYwDVF7Fw_cHjQVMpg7OS=ptwWjhwUCt8A@mail.gmail.com","threadId":"59225","inReplyTo":"CADE8Naoq4N9EpSfb8=ADPRLeAsmjVhL+ayOzhS=syaZ0PQh71w@mail.gmail.com","subject":"Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-02-14T23:30:46Z","receivedAt":"2023-02-14T23:31:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 10, 2023 at 6:32 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> On Fri, 10 Feb 2023 at 16:38, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:\n> > As with the other GSoC patch you submitted[1], this one is also\n> > missing the \"---\" line below your sign-off, which tells git-am where\n> > the commit message ends. As mentioned in [2] you may need to adjust\n> > your tools or workflow to prevent the \"---\" line from being stripped.\n>\n> I apologise for this mistake again.\n\nNo need to apologize. Reviewers point out potential problems, not to\nplace blame, but to help you improve the patch.\n\n> > The actual changes made by the patch are probably reasonable (though I\n> > don't have CVS libraries installed presently, so I wasn't able to\n> > actually test the changes).\n>\n> If by testing you mean running the test file to ensure there are no\n> errors, I did do it thoroughly before sending the patch,\n> so I can vouch for the changes :)\n\nThat's very good to hear, and it's the sort of thing you can mention\nin the patch commentary[1] to help assure reviewers that the patch is\nsound.\n\n[1]: The commentary area of a patch is just _below_ the \"---\" line\nwhich follows your sign-off. You can use the commentary area to supply\nreaders with notes which aren't necessarily relevant to the commit\nmessage itself, but which can help reviewers in other ways.\n"}]}