[PATCH] [GSoC][PATCH] t9160:modernize test path checking

[PATCH] [GSoC][PATCH] t9160:modernize test path checking

[Hoda Salim via GitGitGadget]

From: HodaSalim <hoda.s.salim@gmail.com>

Replace old-style path checks with Git's dedicated test helpers:
- test -f → test_path_is_file
- test -d → test_path_is_dir
- test -s → test_file_not_empty

Found using: git grep "test -[efd]" t/

This improves test readability and provides better error messages
when path checks fail.

Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
---
    [GSoC][PATCH] t9160:modernize test path checking

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2160%2FHodaSalim%2Fmicroproject%2Fmodernize-t9160-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2160/HodaSalim/microproject/modernize-t9160-v1
Pull-Request: https://github.com/git/git/pull/2160

 t/t9160-git-svn-preserve-empty-dirs.sh | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 36c6b1a12f..b89c1cb93a 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.
@@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
 
 # "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
 test_expect_success 'placeholder namespace conflict with file' '
-	test -s "$GIT_REPO"/5/.placeholder
+	test_file_not_empty "$GIT_REPO"/5/.placeholder
 '
 
 # "$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"
 '
 

base-commit: 68cb7f9e92a5d8e9824f5b52ac3d0a9d8f653dbe
-- 
gitgitgadget

[PATCH v2][GSoC] t9160:modernize test path checking

[Hoda Salim]

From: HodaSalim <hoda.s.salim@gmail.com>

Replace old-style path checks with Git's dedicated test helpers:
- test -f → test_path_is_file
- test -d → test_path_is_dir
- test -s → test_file_not_empty

Fix typos with the word "subsequent"

Found using: git grep "test -[efd]" t/

This improves test readability and provides better error messages
when path checks fail.

Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
---
 t/t9160-git-svn-preserve-empty-dirs.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 36c6b1a12f..de32cf2542 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.
@@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
 
 # "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
 test_expect_success 'placeholder namespace conflict with file' '
-	test -s "$GIT_REPO"/5/.placeholder
+	test_file_not_empty "$GIT_REPO"/5/.placeholder
 '
 
 # "$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"
 '
 
@@ -133,19 +133,19 @@ 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_expect_success 'flag persistence during subsequent rebase' '
+	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_expect_success 'placeholder list persistence during subsequent rebase' '
+	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.50.1 (Apple Git-155)

Re: [PATCH v2][GSoC] t9160:modernize test path checking

[Junio C Hamano]

Hoda Salim <hoda.s.salim@gmail.com> writes:

> From: HodaSalim <hoda.s.salim@gmail.com>
>
> Replace old-style path checks with Git's dedicated test helpers:
> - test -f → test_path_is_file
> - test -d → test_path_is_dir
> - test -s → test_file_not_empty
>
> Fix typos with the word "subsequent"
>
> Found using: git grep "test -[efd]" t/
>
> This improves test readability and provides better error messages
> when path checks fail.

For a small change like this, the above is sufficient as all the
necessary things are described, but for future reference, we prefer
to explain things in this order:

 - Give an observation on how the current system works in the
   present tense (so no need to say "Currently X is Y", or
   "Previously X was Y" to describe the state before your change;
   just "X is Y" is enough), and discuss what you perceive as a
   problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to somebody editing the codebase to "make it so",
   instead of saying "This commit does X".

You are going backwards ;-).

Will queue.  Thanks (and thanks for all the reviewers who helped
polish the draft to get to this v2).


>
> Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
> ---
>  t/t9160-git-svn-preserve-empty-dirs.sh | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
> index 36c6b1a12f..de32cf2542 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.
> @@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
>  
>  # "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
>  test_expect_success 'placeholder namespace conflict with file' '
> -	test -s "$GIT_REPO"/5/.placeholder
> +	test_file_not_empty "$GIT_REPO"/5/.placeholder
>  '
>  
>  # "$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"
>  '
>  
> @@ -133,19 +133,19 @@ 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_expect_success 'flag persistence during subsequent rebase' '
> +	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_expect_success 'placeholder list persistence during subsequent rebase' '
> +	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"
>  '

Re: [PATCH v2][GSoC] t9160:modernize test path checking

[Hoda Salim]

> Will queue.  Thanks (and thanks for all the reviewers who helped
> polish the draft to get to this v2).

Yeah, sorry about that. The patch was initially submitted using
GitGitGadget and I didn't really do a great job in the beginning to
reply to the GitGitGadget email.
Pushkar was the maintainer that kindly pointed out the misspelling. I
added him to the CC for visibility

Thank You,
Hoda


On Mon, Feb 2, 2026 at 9:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Hoda Salim <hoda.s.salim@gmail.com> writes:
>
> > From: HodaSalim <hoda.s.salim@gmail.com>
> >
> > Replace old-style path checks with Git's dedicated test helpers:
> > - test -f → test_path_is_file
> > - test -d → test_path_is_dir
> > - test -s → test_file_not_empty
> >
> > Fix typos with the word "subsequent"
> >
> > Found using: git grep "test -[efd]" t/
> >
> > This improves test readability and provides better error messages
> > when path checks fail.
>
> For a small change like this, the above is sufficient as all the
> necessary things are described, but for future reference, we prefer
> to explain things in this order:
>
>  - Give an observation on how the current system works in the
>    present tense (so no need to say "Currently X is Y", or
>    "Previously X was Y" to describe the state before your change;
>    just "X is Y" is enough), and discuss what you perceive as a
>    problem in it.
>
>  - Propose a solution (optional---often, problem description
>    trivially leads to an obvious solution in reader's minds).
>
>  - Give commands to somebody editing the codebase to "make it so",
>    instead of saying "This commit does X".
>
> You are going backwards ;-).
>
> Will queue.  Thanks (and thanks for all the reviewers who helped
> polish the draft to get to this v2).
>
>
> >
> > Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
> > ---
> >  t/t9160-git-svn-preserve-empty-dirs.sh | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
> > index 36c6b1a12f..de32cf2542 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.
> > @@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
> >
> >  # "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
> >  test_expect_success 'placeholder namespace conflict with file' '
> > -     test -s "$GIT_REPO"/5/.placeholder
> > +     test_file_not_empty "$GIT_REPO"/5/.placeholder
> >  '
> >
> >  # "$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"
> >  '
> >
> > @@ -133,19 +133,19 @@ 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_expect_success 'flag persistence during subsequent rebase' '
> > +     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_expect_success 'placeholder list persistence during subsequent rebase' '
> > +     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"
> >  '