[v4,1/1] t3600: use test_path_is_* functions

Commit Message

Derrick Stolee via GitGitGadget Feb. 28, 2019, 10:26 a.m. UTC

From: Rohit Ashiwal <[e-mail]>

Replace `test -(d|f|e)` calls in t3600-rm.sh

Previously we were using `test -(d|f|e)` to verify
the presence of a directory/file, but we already
have helper functions, viz, `test_path_is_dir`,
`test_path_is_file` and `test_path_is_missing`
with better functionality.

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

Signed-off-by: Rohit Ashiwal <[e-mail]>
---
 t/t3600-rm.sh | 160 ++++++++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 76 deletions(-)

Comments

Rohit Ashiwal Feb. 28, 2019, 7:02 p.m. UTC | #1

Hey people

I had a discussion with Rafael over the #git irc channel and Thanks to
him I was able to find these minute mistakes:

1. Commit message was less than 50 chars which should be around 72 chars
   according to coding guide lines. Should I change this to match 72?

2. My changes had some uneven use of tabs and spaces, which I made
   considering that pre-existing code had them too. Is there a
   possibility to change the whole code according to CodingGuidelines?
   If yes should I only change my code according to guidelines or the
   whole file?

3. There is no helper function for `test -s` but Rafael suggested we can
   make use of other helper functions to provide similar functionality,
   if we can.

Open to suggestions and debate. These will be fixed in next revision
accordingly.

Thanks
Rohit

Junio C Hamano March 1, 2019, 2:51 a.m. UTC | #2

Rohit Ashiwal <[e-mail]> writes:

> 1. Commit message was less than 50 chars which should be around 72 chars
>    according to coding guide lines. Should I change this to match 72?

Simple things do not need that many letters to tell ;-)  The
suggestion of 72 is about the maximum.  

If you are doing something in a single patch that needs a longer
title, it generally is a sign that you are trying to do too much in
a single patch and should be splitting the patch into more
digestable smaller steps.  And the purpose of having a maximum is to
nudge patch authors to realize that.

> 2. My changes had some uneven use of tabs and spaces, which I made
>    considering that pre-existing code had them too. Is there a
>    possibility to change the whole code according to CodingGuidelines?
>    If yes should I only change my code according to guidelines or the
>    whole file?

I think you are talking about t3600, which uses an ancient style.
If this were a real project, then the preferred order would be

 - A preliminary patch (or a series of patches) that modernizes
   existing tests in t3600.  Just style updates and adding or
   removing nothing else.

 - Update test that use "test -f" and friends to use the helpers in
   t3600.

> 3. There is no helper function for `test -s` but Rafael suggested we can
>    make use of other helper functions to provide similar functionality,
>    if we can.

If we often see if a path is an non-empty file in our tests (not
limited to t3600), then it may make sense to add a new helper
test_path_is_non_empty_file in t/test-lib-functions.sh next to where
test_path_is_file and friends are defined.

Thanks.

[jch: I am still mostly offline til the next week, but I had a
chance to sit in front of my mailbox long enough, so...]

Rohit Ashiwal March 1, 2019, 1:13 p.m. UTC | #3

Hey!

I'm a little confused as you never provide a clear indication to
where shall I proceed? :-

On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano <[e-mail]> wrote:

>
> Simple things do not need that many letters to tell ;-)  The
> suggestion of 72 is about the maximum. 
>

Totally agree on this!

>
> I think you are talking about t3600, which uses an ancient style.
> If this were a real project, then the preferred order would be
>
>  - A preliminary patch (or a series of patches) that modernizes
>    existing tests in t3600.  Just style updates and adding or
>    removing nothing else.
>
>  - Update test that use "test -f" and friends to use the helpers in
>    t3600.
>

Yes, this is a microproject after all. But I think I can work on this as
if it were a real project, should I proceed according to this plan? (I have
a lot of free time over this weekend)

>
> If we often see if a path is an non-empty file in our tests (not
> limited to t3600), then it may make sense to add a new helper
> test_path_is_non_empty_file in t/test-lib-functions.sh next to where
> test_path_is_file and friends are defined.
>

Since my project does not deal with `test-lib-functions.sh`, I think I
should not edit it anyway, but I'd be more than happy to add a new
member to `test_path_is_*` family.

Thanks
Rohit

Rafael Ascensão March 2, 2019, 4:24 a.m. UTC | #4

On Fri, Mar 01, 2019 at 06:43:26PM +0530, Rohit Ashiwal wrote:
> >
> > Simple things do not need that many letters to tell ;-)  The
> > suggestion of 72 is about the maximum. 
> >
> 
> Totally agree on this!
>

I was bikeshedding the patch and mentioned that the commit message body
is usually wrapped at 72 because I noticed you were wrapping the body at
50.

So to make things clear, when you're writing the subject, i.e. the first
line, you should aim towards 50 and do not exceed 72.

The body, i.e. 3rd line until EOF is usually wrapped at 72.

There are exceptions, these are guidelines. Sometimes commits will break
the first rule. (Merges are the most common example I can think of, but
you won't be doing any as a contributor).
Pre-formatted content, like the output of a program, will break the
second. Look at 3b41fb0cb217f4b4491f2e67ce4183e5d2a5d873 for an example.

But my nitpick wasn't necessarily because I didn't agree about the way
you line wrapped the patch. It was about figuring out if you had a
misconfigured editor (that could also be the cause of tabs and spaces
mix), which you later mentioned was probably the case.

Cheers,
Rafael Ascensão

Thomas Gummerer March 2, 2019, 2:46 p.m. UTC | #5

On 03/01, Rohit Ashiwal wrote:
> Hey!
> 
> I'm a little confused as you never provide a clear indication to
> where shall I proceed? :-
> 
> On Fri, 01 Mar 2019 11:51:46 +0900 Junio C Hamano <[e-mail]> wrote:
> > I think you are talking about t3600, which uses an ancient style.
> > If this were a real project, then the preferred order would be
> >
> >  - A preliminary patch (or a series of patches) that modernizes
> >    existing tests in t3600.  Just style updates and adding or
> >    removing nothing else.
> >
> >  - Update test that use "test -f" and friends to use the helpers in
> >    t3600.
> >
> 
> Yes, this is a microproject after all. But I think I can work on this as
> if it were a real project, should I proceed according to this plan? (I have
> a lot of free time over this weekend)

Yes, I think it would be good to make those changes, to try and get
this merged.  Having the microproject merged is not a requirement (its
main purpose is to see how students communicate on the mailing list,
and to get them familiar with the workflow ahead of GSoC), but it can
be a nice achievement in itself.

That said, I would also encourage you to start thinking about a
project proposal, as that is another important part that should be
done for the application.

> >
> > If we often see if a path is an non-empty file in our tests (not
> > limited to t3600), then it may make sense to add a new helper
> > test_path_is_non_empty_file in t/test-lib-functions.sh next to where
> > test_path_is_file and friends are defined.
> >
> 
> Since my project does not deal with `test-lib-functions.sh`, I think I
> should not edit it anyway, but I'd be more than happy to add a new
> member to `test_path_is_*` family.

It is up to you how far you would like to go with this.  If you want
to add the helper, to make further cleanups in t3600, I think that
would be a good thing to do (after double checking that it would be
useful in other test files as well), and should be done in a separate
patch.  Then you can use it in the same patch as using the helpers for
"test -f" etc.

> Thanks
> Rohit

Rohit Ashiwal March 2, 2019, 4:21 p.m. UTC | #6

Hey! Thomas

Thank you for replying over my woes.

>
> It is up to you how far you would like to go with this.  If you want
> to add the helper, to make further cleanups in t3600, I think that
> would be a good thing to do (after double checking that it would be
> useful in other test files as well), and should be done in a separate
> patch.  Then you can use it in the same patch as using the helpers for
> "test -f" etc.
>

I guess I should work on this one first. I checked and around 18 test
files use `test -s`, it will be useful nonetheless.

>
> Yes, I think it would be good to make those changes, to try and get
> this merged.  Having the microproject merged is not a requirement (its
> main purpose is to see how students communicate on the mailing list,
> and to get them familiar with the workflow ahead of GSoC), but it can
> be a nice achievement in itself.
>

Yes, it is a nice experience to interact with people who "run" git over
which most of the open source community depends for code sharing and
collaboration.

>
> That said, I would also encourage you to start thinking about a
> project proposal, as that is another important part that should be
> done for the application.
>

That is really encouraging, I'll try to finish my work as soon as
possible and work on the proposal side by side!

Thanks
Rohit

Patch

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 04e5d42bd3..3a5bd97df7 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -27,8 +27,10 @@  embedded' &&
 "
 
 test_expect_success \
-    'Pre-check that foo exists and is in index before git rm foo' \
-    '[ -f foo ] && git ls-files --error-unmatch foo'
+    'Pre-check that foo exists and is in index before git rm foo' '
+	 test_path_is_file foo &&
+	 git ls-files --error-unmatch foo
+'
 
 test_expect_success \
     'Test that git rm foo succeeds' \
@@ -70,20 +72,26 @@  test_expect_success \
      git rm --cached -f foo'
 
 test_expect_success \
-    'Post-check that foo exists but is not in index after git rm foo' \
-    '[ -f foo ] && test_must_fail git ls-files --error-unmatch foo'
+    'Post-check that foo exists but is not in index after git rm 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"' \
-    '[ -f bar ] && git ls-files --error-unmatch bar'
+    'Pre-check that bar exists and is in index before "git rm bar"' '
+	 test_path_is_file bar &&
+	 git ls-files --error-unmatch bar
+'
 
 test_expect_success \
     'Test that "git rm bar" succeeds' \
     'git rm bar'
 
 test_expect_success \
-    'Post-check that bar does not exist and is not in index after "git rm -f bar"' \
-    '! [ -f bar ] && test_must_fail git ls-files --error-unmatch bar'
+    'Post-check that bar does not exist and is not in index after "git rm -f bar"' '
+	 test_path_is_missing bar &&
+	 test_must_fail git ls-files --error-unmatch bar
+'
 
 test_expect_success \
     'Test that "git rm -- -q" succeeds (remove a file that looks like an option)' \
@@ -137,15 +145,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
 '
@@ -159,15 +167,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
 '
@@ -194,21 +202,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' '
@@ -232,7 +240,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
 
 '
 
@@ -254,7 +262,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
@@ -292,7 +300,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 &&
@@ -314,7 +322,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 &&
@@ -325,7 +333,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
 '
@@ -343,12 +351,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 &&
@@ -359,8 +367,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 &&
@@ -371,7 +379,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
 '
 
@@ -381,8 +389,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
 '
@@ -393,14 +401,14 @@  test_expect_success 'rm will error out on a modified .gitmodules file unless sta
 	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_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
 '
@@ -413,8 +421,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
 '
@@ -424,12 +432,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
 '
@@ -439,12 +447,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
 '
@@ -481,7 +489,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
 '
@@ -493,12 +501,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 &&
@@ -512,12 +520,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 &&
@@ -531,12 +539,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
 '
@@ -552,13 +560,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 &&
@@ -570,7 +578,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
 '
@@ -586,8 +594,8 @@  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_i18ngrep Migrating output.err
@@ -614,7 +622,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
 '
@@ -624,12 +632,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
 '
@@ -639,12 +647,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
 '
@@ -654,12 +662,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
 '
@@ -673,8 +681,8 @@  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_i18ngrep Migrating output.err