threads / rfc / 59225

RFC patcht9160: Change test -(d | f) to test_path_is_*

Subject: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*

## tl;dr

4 messages between Feb 10, 2023 and Feb 14, 2023. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Vinayak Dev· Feb 10, 2023, 08:01 UTC · lore

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(-)

Show changes to t/t9160-git-svn-preserve-empty-dirs.sh +10 −10

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"
 '

-- 
2.39.1

Eric Sunshine· Feb 10, 2023, 11:08 UTC · re: Vinayak Dev · lore

Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*

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/

Vinayak Dev· Feb 10, 2023, 11:32 UTC · re: Eric Sunshine · lore

Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*

On Fri, 10 Feb 2023 at 16:38, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 5 quoted lines

>
> 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.

Show 13 quoted lines

> > 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.

Show 9 quoted lines

> > 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

Eric Sunshine· Feb 14, 2023, 23:30 UTC · re: Vinayak Dev · lore

Re: [RFC][GSoC][PATCH] t9160: Change test -(d | f) to test_path_is_*

On Fri, Feb 10, 2023 at 6:32 AM Vinayak Dev <vinayakdev.sci@gmail.com> wrote:

Show 8 quoted lines

> 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.

Show 7 quoted lines

> > 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.