diff mbox series

[GSoC,v3,3/3] t3600: use helpers to replace test -d/f/e/s <path>

Message ID 20190304120801.28763-4-rohit.ashiwal265@gmail.com (mailing list archive)
State New, archived
Headers show
Series Use helper functions in test script | expand

Commit Message

Rohit Ashiwal March 4, 2019, 12:08 p.m. UTC 
Previously  we  were  using  `test -(d|f|e|s)`  to  verify  the  presence of a
directory/file, but we already have helper functions, viz, `test_path_is_dir`,
`test_path_is_file`,    `test_path_is_missing`    and    `test_file_not_empty`
with better functionality.

These helper functions make code more readable and informative to someone new,
also these functions have better error messages.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 t/t3600-rm.sh | 150 +++++++++++++++++++++++++-------------------------
 1 file changed, 75 insertions(+), 75 deletions(-)

Comments

Eric Sunshine March 5, 2019, 12:42 a.m. UTC | #1 
On Mon, Mar 4, 2019 at 7:09 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> Previously  we  were  using  `test -(d|f|e|s)`  to  verify  the  presence of a
> directory/file, but we already have helper functions, viz, `test_path_is_dir`,
> `test_path_is_file`,    `test_path_is_missing`    and    `test_file_not_empty`
> with better functionality.

As with the commit message of 2/3, many of the words in this message
are separated by multiple spaced. Please fold out the excess so there
is only a single space between words.

Also, no need to say "previously" since readers know that the patch is
changing something. Rewrite in imperative mood:

    Take advantage of helper functions test_path_is_dir(),
    test_path_is_missing(), etc. to replace `test -d|f|e|s` since the
    functions make the code more readable and have better error
    messages.

> These helper functions make code more readable and informative to someone new,
> also these functions have better error messages.
>
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Rohit Ashiwal March 5, 2019, 1:42 p.m. UTC | #2 
Hey Eric

On 2019-03-05 0:42 Eric Sunshine <sunshine@sunshineco.com> wrote:
> As with the commit message of 2/3, many of the words in this message
> are separated by multiple spaced. Please fold out the excess so there
> is only a single space between words.
>
> Also, no need to say "previously" since readers know that the patch is
> changing something. Rewrite in imperative mood:

Okay, I'll keep that in mind from next time onwards. The spaces were
provided to make the commit message look aesthetically pleasing.

These changes aside, is there anything you would like to add to the review?
or is it good to go for a merge?

Thanks for advice
Rohit
Eric Sunshine March 5, 2019, 2:03 p.m. UTC | #3 
On Tue, Mar 5, 2019 at 8:43 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> On 2019-03-05 0:42 Eric Sunshine <sunshine@sunshineco.com> wrote:
> > As with the commit message of 2/3, many of the words in this message
> > are separated by multiple spaced. Please fold out the excess so there
> > is only a single space between words.
> >
> > Also, no need to say "previously" since readers know that the patch is
> > changing something. Rewrite in imperative mood:
>
> Okay, I'll keep that in mind from next time onwards. The spaces were
> provided to make the commit message look aesthetically pleasing.
>
> These changes aside, is there anything you would like to add to the review?
> or is it good to go for a merge?

I don't understand your question.
Rohit Ashiwal March 5, 2019, 2:21 p.m. UTC | #4 
Eric

I was asking if this patch is good enough to be added to the existing code? Does this patch look good?


Regards
Rohit
Eric Sunshine March 5, 2019, 2:57 p.m. UTC | #5 
On Tue, Mar 5, 2019 at 9:22 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> I was asking if this patch is good enough to be added to the
> existing code? Does this patch look good?

I didn't review the patch with a critical-enough eye to be able to say
that every change maintains fidelity with the original code. As
mentioned in [1]:

    [...] an important reason for limiting the scope of this change
    [...] is to ease the burden on people who review your submission.
    Large patch series tend to tax reviewers heavily, even (and often)
    when repetitive and simple, like replacing `test -d` with
    `test_path_is_dir()`. The shorter and more concise a patch series
    is, the more likely that it will receive quality reviews.

This patch, due to its length and repetitive nature, falls under the
category of being tedious to review, which makes it all the more
likely that a reviewer will overlook a problem.

And, it's not always obvious at a glance that a change is correct. For
instance, taking a look at the final patch band:

    - ! test -d submod &&
    - ! test -d submod/subsubmod/.git &&
    + test_path_is_missing submod &&
    + test_path_is_missing submod/subsubmod/.git &&

Superficially, the transformation seems straightforward. However, that
doesn't mean it maintains fidelity with the original or even means the
same thing. To review this change properly requires understanding the
original intent of "! test -d".

The meaning of that expression can vary depending upon the context. Is
it checking that that path is not a directory (but it is okay if a
plain file exists there)? Or does it merely care about existence
(neither directory nor any other type of entry)? If the latter, then
the transformation is probably correct, however, if the former, then
it likely isn't correct. So, understanding the overall context of the
test is important for judging if a particular change is correct, and
many (volunteer) reviewers simply don't have the time to delve that
deeply to make a proper judgment.

[1]: https://public-inbox.org/git/CAPig+cSZZaCT0G3hysmjn_tNvZmYGp=5cXpZHkdphbWXnONSVQ@mail.gmail.com/
Rohit Ashiwal March 5, 2019, 11:38 p.m. UTC | #6 
Hey Eric

On Tue, 5 Mar 2019 09:57:40 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote:
> This patch, due to its length and repetitive nature, falls under the
> category of being tedious to review, which makes it all the more
> likely that a reviewer will overlook a problem.

Yes, I clearly understand that this patch has become too big to review.
It will require time to carefully review and reviewers are doing their
best to maintain the utmost quality of code.

> And, it's not always obvious at a glance that a change is correct. For
> instance, taking a look at the final patch band:
>
>     - ! test -d submod &&
>     - ! test -d submod/subsubmod/.git &&
>     + test_path_is_missing submod &&
>     + test_path_is_missing submod/subsubmod/.git &&

Duy actually confirms that this transformation is correct in this[1] email.
(I know that, it was given as an example, but I'll leave the link anyway).

Thanks
Rohit

[1]: https://public-inbox.org/git/CACsJy8BYeLvB7BSM_Jt4vwfGsEBuhaCZfzGPOHe=B=7cvnRwrg@mail.gmail.com/
Junio C Hamano March 8, 2019, 5:38 a.m. UTC | #7 
Eric Sunshine <sunshine@sunshineco.com> writes:

> This patch, due to its length and repetitive nature, falls under the
> category of being tedious to review, which makes it all the more
> likely that a reviewer will overlook a problem.
>
> And, it's not always obvious at a glance that a change is correct. For
> instance, taking a look at the final patch band:
>
>     - ! test -d submod &&
>     - ! test -d submod/subsubmod/.git &&
>     + test_path_is_missing submod &&
>     + test_path_is_missing submod/subsubmod/.git &&
>
> Superficially, the transformation seems straightforward. However, that
> doesn't mean it maintains fidelity with the original or even means the
> same thing. To review this change properly requires understanding the
> original intent of "! test -d".
>
> ... , and
> many (volunteer) reviewers simply don't have the time to delve that
> deeply to make a proper judgment.

True.  The microproject was supposed to be a gentle introduction to
and a practice session of the process of modifying, committing,
submitting, and responding to reviews.  Learning the usual Git
contributor workflow, without spending too much community resources
like reviewers' time (as opposed to the real "here is my itch; let's
improve the system, and please help me doing so").

In any case, I have spent some time with the patch and I think the
changes are generaly OK; some (like using "test_path_is_missing foo"
instead of "test ! -f foo" after "git rm foo" to ensure the path no
longer exists) are improvements.

An unrelated tangent, but what do you think of this patch?  In the
context of testing "git rm", if foo is a dangling symbolic link,
"git rm foo && test_path_is_missing foo" would need something like
this to work correctly, I would think.

 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 681c41ba32..dfe0d4aff4 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -603,7 +603,7 @@ test_file_not_empty () {
 }
 
 test_path_is_missing () {
-	if test -e "$1"
+	if test -e "$1" || test -L "$1"
 	then
 		echo "Path exists:"
 		ls -ld "$1"
Eric Sunshine March 8, 2019, 9:51 a.m. UTC | #8 
On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
> An unrelated tangent, but what do you think of this patch?  In the
> context of testing "git rm", if foo is a dangling symbolic link,
> "git rm foo && test_path_is_missing foo" would need something like
> this to work correctly, I would think.
>
>  test_path_is_missing () {
> -       if test -e "$1"
> +       if test -e "$1" || test -L "$1"
>         then
>                 echo "Path exists:"
>                 ls -ld "$1"

Makes sense. Won't we also want:

    test_path_exists () {
    -    if ! test -e "$1"
    +   if ! test -e "$1" && ! test -L "$1"
       then
            echo "Path $1 doesn't exist. $2"

or something like that?
Junio C Hamano March 11, 2019, 1:54 a.m. UTC | #9 
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Mar 8, 2019 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote:
>> An unrelated tangent, but what do you think of this patch?  In the
>> context of testing "git rm", if foo is a dangling symbolic link,
>> "git rm foo && test_path_is_missing foo" would need something like
>> this to work correctly, I would think.
>>
>>  test_path_is_missing () {
>> -       if test -e "$1"
>> +       if test -e "$1" || test -L "$1"
>>         then
>>                 echo "Path exists:"
>>                 ls -ld "$1"
>
> Makes sense. Won't we also want:
>
>     test_path_exists () {
>     -    if ! test -e "$1"
>     +   if ! test -e "$1" && ! test -L "$1"
>        then
>             echo "Path $1 doesn't exist. $2"
>
> or something like that?

That would make them symmetric, but what I was driving at with "In
the context of testing git rm" was that I highly suspect that among
other existing users of test_path_is_missing there are some that
want to consider a dangling symbolic link as if it is not there (and
vice versa for test_path_exists), and it may not be a good idea to
unconditionally declare that, unlike the underlying "test" command
that dereferences symlinks for most operations, our wrapper does not
dereference symbolic links, which is what the "what do you think?"
patch and your addtion do.
diff mbox series

Patch

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 8b03897a65..85ae7dc1e4 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -26,7 +26,7 @@  test_expect_success FUNNYNAMES 'add files with funny names' '
 '
 
 test_expect_success 'Pre-check that foo exists and is in index before git rm foo' '
-	test -f foo &&
+	test_path_is_file foo &&
 	git ls-files --error-unmatch foo
 '
 
@@ -69,12 +69,12 @@  test_expect_success 'Test that git rm --cached -f foo works in case where --cach
 '
 
 test_expect_success 'Post-check that foo exists but is not in index after git rm foo' '
-	test -f foo &&
+	test_path_is_file foo &&
 	test_must_fail git ls-files --error-unmatch foo
 '
 
 test_expect_success 'Pre-check that bar exists and is in index before "git rm bar"' '
-	test -f bar &&
+	test_path_is_file bar &&
 	git ls-files --error-unmatch bar
 '
 
@@ -83,7 +83,7 @@  test_expect_success 'Test that "git rm bar" succeeds' '
 '
 
 test_expect_success 'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
-	! test -f bar &&
+	test_path_is_missing bar &&
 	test_must_fail git ls-files --error-unmatch bar
 '
 
@@ -138,15 +138,15 @@  test_expect_success 'Re-add foo and baz' '
 test_expect_success 'Modify foo -- rm should refuse' '
 	echo >>foo &&
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'Modified foo -- rm -f should work' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch bar
 '
@@ -160,15 +160,15 @@  test_expect_success 'Re-add foo and baz for HEAD tests' '
 
 test_expect_success 'foo is different in index from HEAD -- rm should refuse' '
 	test_must_fail git rm foo baz &&
-	test -f foo &&
-	test -f baz &&
+	test_path_is_file foo &&
+	test_path_is_file baz &&
 	git ls-files --error-unmatch foo baz
 '
 
 test_expect_success 'but with -f it should work.' '
 	git rm -f foo baz &&
-	test ! -f foo &&
-	test ! -f baz &&
+	test_path_is_missing foo &&
+	test_path_is_missing baz &&
 	test_must_fail git ls-files --error-unmatch foo &&
 	test_must_fail git ls-files --error-unmatch baz
 '
@@ -195,21 +195,21 @@  test_expect_success 'Recursive test setup' '
 
 test_expect_success 'Recursive without -r fails' '
 	test_must_fail git rm frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r but dirty' '
 	echo qfwfq >>frotz/nitfol &&
 	test_must_fail git rm -r frotz &&
-	test -d frotz &&
-	test -f frotz/nitfol
+	test_path_is_dir frotz &&
+	test_path_is_file frotz/nitfol
 '
 
 test_expect_success 'Recursive with -r -f' '
 	git rm -f -r frotz &&
-	! test -f frotz/nitfol &&
-	! test -d frotz
+	test_path_is_missing frotz/nitfol &&
+	test_path_is_missing frotz
 '
 
 test_expect_success 'Remove nonexistent file returns nonzero exit status' '
@@ -236,7 +236,7 @@  test_expect_success 'refresh index before checking if it is up-to-date' '
 	git reset --hard &&
 	test-tool chmtime -86400 frotz/nitfol &&
 	git rm frotz/nitfol &&
-	test ! -f frotz/nitfol
+	test_path_is_missing frotz/nitfol
 '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
@@ -257,7 +257,7 @@  test_expect_success 'rm removes subdirectories recursively' '
 	echo content >dir/subdir/subsubdir/file &&
 	git add dir/subdir/subsubdir/file &&
 	git rm -f dir/subdir/subsubdir/file &&
-	! test -d dir
+	test_path_is_missing dir
 '
 
 cat >expect <<EOF
@@ -295,7 +295,7 @@  test_expect_success 'rm removes empty submodules from work tree' '
 	git add .gitmodules &&
 	git commit -m "add submodule" &&
 	git rm submod &&
-	test ! -e submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -317,7 +317,7 @@  test_expect_success 'rm removes work tree of unmodified submodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -328,7 +328,7 @@  test_expect_success 'rm removes a submodule with a trailing /' '
 	git reset --hard &&
 	git submodule update &&
 	git rm submod/ &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -346,12 +346,12 @@  test_expect_success 'rm of a populated submodule with different HEAD fails unles
 	git submodule update &&
 	git -C submod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -362,8 +362,8 @@  test_expect_success 'rm --cached leaves work tree of populated submodules and .g
 	git reset --hard &&
 	git submodule update &&
 	git rm --cached submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.cached actual &&
 	git config -f .gitmodules submodule.sub.url &&
@@ -374,7 +374,7 @@  test_expect_success 'rm --dry-run does not touch the submodule or .gitmodules' '
 	git reset --hard &&
 	git submodule update &&
 	git rm -n submod &&
-	test -f submod/.git &&
+	test_path_is_file submod/.git &&
 	git diff-index --exit-code HEAD
 '
 
@@ -384,8 +384,8 @@  test_expect_success 'rm does not complain when no .gitmodules file is found' '
 	git rm .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect.both_deleted actual
 '
@@ -395,15 +395,15 @@  test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	git submodule update &&
 	git config -f .gitmodules foo.bar true &&
 	test_must_fail git rm submod >actual 2>actual.err &&
-	test -s actual.err &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_file_not_empty actual.err &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git diff-files --quiet -- submod &&
 	git add .gitmodules &&
 	git rm submod >actual 2>actual.err &&
 	test_must_be_empty actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -416,8 +416,8 @@  test_expect_success 'rm issues a warning when section is not found in .gitmodule
 	echo "warning: Could not find section in .gitmodules where path=submod" >expect.err &&
 	git rm submod >actual 2>actual.err &&
 	test_i18ncmp expect.err actual.err &&
-	! test -d submod &&
-	! test -f submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno >actual &&
 	test_cmp expect actual
 '
@@ -427,12 +427,12 @@  test_expect_success 'rm of a populated submodule with modifications fails unless
 	git submodule update &&
 	echo X >submod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -442,12 +442,12 @@  test_expect_success 'rm of a populated submodule with untracked files fails unle
 	git submodule update &&
 	echo X >submod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -484,7 +484,7 @@  test_expect_success 'rm removes work tree of unmodified conflicted submodule' '
 	git submodule update &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -496,12 +496,12 @@  test_expect_success 'rm of a conflicted populated submodule with different HEAD
 	git -C submod checkout HEAD^ &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -515,12 +515,12 @@  test_expect_success 'rm of a conflicted populated submodule with modifications f
 	echo X >submod/empty &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual &&
 	test_must_fail git config -f .gitmodules submodule.sub.url &&
@@ -534,12 +534,12 @@  test_expect_success 'rm of a conflicted populated submodule with untracked files
 	echo X >submod/untracked &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -556,13 +556,13 @@  test_expect_success 'rm of a conflicted populated submodule with a .git director
 	) &&
 	test_must_fail git merge conflict2 &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	test_must_fail git rm -f submod &&
-	test -d submod &&
-	test -d submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_dir submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.conflict actual &&
 	git merge --abort &&
@@ -574,7 +574,7 @@  test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
 	git reset --hard &&
 	test_must_fail git merge conflict2 &&
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -591,10 +591,10 @@  test_expect_success 'rm of a populated submodule with a .git directory migrates
 		rm -r ../.git/modules/sub
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '
 
@@ -620,7 +620,7 @@  test_expect_success 'setup subsubmodule' '
 
 test_expect_success 'rm recursively removes work tree of unmodified submodules' '
 	git rm submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -630,12 +630,12 @@  test_expect_success 'rm of a populated nested submodule with different nested HE
 	git submodule update --recursive &&
 	git -C submod/subsubmod checkout HEAD^ &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -645,12 +645,12 @@  test_expect_success 'rm of a populated nested submodule with nested modification
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/empty &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_inside actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -660,12 +660,12 @@  test_expect_success 'rm of a populated nested submodule with nested untracked fi
 	git submodule update --recursive &&
 	echo X >submod/subsubmod/untracked &&
 	test_must_fail git rm submod &&
-	test -d submod &&
-	test -f submod/.git &&
+	test_path_is_dir submod &&
+	test_path_is_file submod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect.modified_untracked actual &&
 	git rm -f submod &&
-	test ! -d submod &&
+	test_path_is_missing submod &&
 	git status -s -uno --ignore-submodules=none >actual &&
 	test_cmp expect actual
 '
@@ -680,10 +680,10 @@  test_expect_success "rm absorbs submodule's nested .git directory" '
 		GIT_WORK_TREE=. git config --unset core.worktree
 	) &&
 	git rm submod 2>output.err &&
-	! test -d submod &&
-	! test -d submod/subsubmod/.git &&
+	test_path_is_missing submod &&
+	test_path_is_missing submod/subsubmod/.git &&
 	git status -s -uno --ignore-submodules=none >actual &&
-	test -s actual &&
+	test_file_not_empty actual &&
 	test_i18ngrep Migrating output.err
 '