| Message ID | 20230210080110.32983-1-vinayakdev.sci@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,GSoC] t9160: Change test -(d | f) to test_path_is_* | expand |
On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote: > Changes test -d and test -f commands in s/Changes/Change/ > t/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and > test_path_is_file respectively, which are helper functions defined in > t/test-lib-functions.sh This summarizes the changes made by the patch, but readers also want to know _why_ the changes are being made; in fact, "why" is more important. Previous patches similar to this one have explained that test_path_is_dir() and test_path_is_file() are superior to `test -d` and `test -f` because the functions provide diagnostic output when they fail, and that diagnostic output can make it easier to debug a failing tests. So, if you reroll the patch, focus on explaining the benefits of the functions rather than explaining the mechanical changes made by the patch. > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com> > > vinayakdsci (1): > Change test -(d | f) to corresponding test_path_* As with the other GSoC patch you submitted[1], this one is also missing the "---" line below your sign-off, which tells git-am where the commit message ends. As mentioned in [2] you may need to adjust your tools or workflow to prevent the "---" line from being stripped. The actual changes made by the patch are probably reasonable (though I don't have CVS libraries installed presently, so I wasn't able to actually test the changes). [1]: https://lore.kernel.org/git/20230209164552.8971-1-vinayakdev.sci@gmail.com/ [2]: https://lore.kernel.org/git/CAPig+cT1EtPO2FLvTsw3SjgCgk=ovNwY77hsX6p7ETKiq8aNog@mail.gmail.com/
On Fri, 10 Feb 2023 at 16:38, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote: > > Changes test -d and test -f commands in > > s/Changes/Change/ Sorry for this error, I actually sent the patch before I got your review of the prior one. > > t/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and > > test_path_is_file respectively, which are helper functions defined in > > t/test-lib-functions.sh > > This summarizes the changes made by the patch, but readers also want > to know _why_ the changes are being made; in fact, "why" is more > important. Previous patches similar to this one have explained that > test_path_is_dir() and test_path_is_file() are superior to `test -d` > and `test -f` because the functions provide diagnostic output when > they fail, and that diagnostic output can make it easier to debug a > failing tests. So, if you reroll the patch, focus on explaining the > benefits of the functions rather than explaining the mechanical > changes made by the patch. This is correct, I will make the message more informative about the relevant aspects of the changes. > > Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com> > > > > vinayakdsci (1): > > Change test -(d | f) to corresponding test_path_* > > As with the other GSoC patch you submitted[1], this one is also > missing the "---" line below your sign-off, which tells git-am where > the commit message ends. As mentioned in [2] you may need to adjust > your tools or workflow to prevent the "---" line from being stripped. I apologise for this mistake again. > The actual changes made by the patch are probably reasonable (though I > don't have CVS libraries installed presently, so I wasn't able to > actually test the changes). If by testing you mean running the test file to ensure there are no errors, I did do it thoroughly before sending the patch, so I can vouch for the changes :) Thanks a lot! Vinayak
On Fri, Feb 10, 2023 at 6:32 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote: > On Fri, 10 Feb 2023 at 16:38, Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Feb 10, 2023 at 3:05 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote: > > As with the other GSoC patch you submitted[1], this one is also > > missing the "---" line below your sign-off, which tells git-am where > > the commit message ends. As mentioned in [2] you may need to adjust > > your tools or workflow to prevent the "---" line from being stripped. > > I apologise for this mistake again. No need to apologize. Reviewers point out potential problems, not to place blame, but to help you improve the patch. > > The actual changes made by the patch are probably reasonable (though I > > don't have CVS libraries installed presently, so I wasn't able to > > actually test the changes). > > If by testing you mean running the test file to ensure there are no > errors, I did do it thoroughly before sending the patch, > so I can vouch for the changes :) That's very good to hear, and it's the sort of thing you can mention in the patch commentary[1] to help assure reviewers that the patch is sound. [1]: The commentary area of a patch is just _below_ the "---" line which follows your sign-off. You can use the commentary area to supply readers with notes which aren't necessarily relevant to the commit message itself, but which can help reviewers in other ways.
diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh index 36c6b1a12f..963b081a1c 100755 --- a/t/t9160-git-svn-preserve-empty-dirs.sh +++ b/t/t9160-git-svn-preserve-empty-dirs.sh @@ -61,15 +61,15 @@ test_expect_success 'clone svn repo with --preserve-empty-dirs' ' # "$GIT_REPO"/1 should only contain the placeholder file. test_expect_success 'directory empty from inception' ' - test -f "$GIT_REPO"/1/.gitignore && + test_path_is_file "$GIT_REPO"/1/.gitignore && test $(find "$GIT_REPO"/1 -type f | wc -l) = "1" ' # "$GIT_REPO"/2 and "$GIT_REPO"/3 should only contain the placeholder file. test_expect_success 'directory empty from subsequent svn commit' ' - test -f "$GIT_REPO"/2/.gitignore && + test_path_is_file "$GIT_REPO"/2/.gitignore && test $(find "$GIT_REPO"/2 -type f | wc -l) = "1" && - test -f "$GIT_REPO"/3/.gitignore && + test_path_is_file "$GIT_REPO"/3/.gitignore && test $(find "$GIT_REPO"/3 -type f | wc -l) = "1" ' @@ -77,7 +77,7 @@ test_expect_success 'directory empty from subsequent svn commit' ' # generated for every sub-directory at some point in the repo's history. test_expect_success 'add entry to previously empty directory' ' test $(find "$GIT_REPO"/4 -type f | wc -l) = "1" && - test -f "$GIT_REPO"/4/a/b/c/foo + test_path_is_file "$GIT_REPO"/4/a/b/c/foo ' # The HEAD~2 commit should not have introduced .gitignore placeholder files. @@ -108,8 +108,8 @@ test_expect_success 'placeholder namespace conflict with file' ' # "$GIT_REPO"/6/.placeholder should be a directory, and the "$GIT_REPO"/6 tree # should only contain one file: the placeholder. test_expect_success 'placeholder namespace conflict with directory' ' - test -d "$GIT_REPO"/6/.placeholder && - test -f "$GIT_REPO"/6/.placeholder/.placeholder && + test_path_is_dir "$GIT_REPO"/6/.placeholder && + test_path_is_file "$GIT_REPO"/6/.placeholder/.placeholder && test $(find "$GIT_REPO"/6 -type f | wc -l) = "1" ' @@ -134,18 +134,18 @@ test_expect_success 'second set of svn commits and rebase' ' # Check that --preserve-empty-dirs and --placeholder-file flag state # stays persistent over multiple invocations. test_expect_success 'flag persistence during subsqeuent rebase' ' - test -f "$GIT_REPO"/7/.placeholder && + test_path_is_file "$GIT_REPO"/7/.placeholder && test $(find "$GIT_REPO"/7 -type f | wc -l) = "1" ' # Check that placeholder files are properly removed when unnecessary, # even across multiple invocations. test_expect_success 'placeholder list persistence during subsqeuent rebase' ' - test -f "$GIT_REPO"/1/file1.txt && + test_path_is_file "$GIT_REPO"/1/file1.txt && test $(find "$GIT_REPO"/1 -type f | wc -l) = "1" && - test -f "$GIT_REPO"/5/file1.txt && - test -f "$GIT_REPO"/5/.placeholder && + test_path_is_file "$GIT_REPO"/5/file1.txt && + test_path_is_file "$GIT_REPO"/5/.placeholder && test $(find "$GIT_REPO"/5 -type f | wc -l) = "2" '
Changes test -d and test -f commands in t/t9160-git-svn-preserve-empty-dirs.sh to test_path_is_dir and test_path_is_file respectively, which are helper functions defined in t/test-lib-functions.sh Signed-off-by: Vinayak Dev <vinayakdev.sci@gmail.com> vinayakdsci (1): Change test -(d | f) to corresponding test_path_* t/t9160-git-svn-preserve-empty-dirs.sh | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)