threads / patch / 65177

patcht9200: use helpers to replace test -f <path> and test -d <path>

Subject: [GSoC PATCH] t9200: use helpers to replace test -f <path> and test -d <path>

## tl;dr

16 messages between Mar 9, 2026 and Mar 12, 2026. Diffs are folded; open one to read it.

replies: 15people: 2as markdown or json

Pablo Sabater· Mar 9, 2026, 15:09 UTC · lore
Replaced 'test -f' and 'test -d' with 'test_path_is_file' and 'test_path_is_dir'

I've used 'git grep "test -f" t/t9*.sh' to find a file without the fix done as specified on the microproject information I've done '9*' because the ones I've found first had already fix patches. I've taken as example another patch sent 't4131' from Junio C Hamano https://lore.kernel.org/git/xmqq1rpodn25.fsf@gitster.c.googlers.com/#r

Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
 t/t9200-git-cvsexportcommit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t9200-git-cvsexportcommit.sh +2 −2
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index a44eabf0d8..4507e8e6db 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
 rm -rf "$CVSROOT" "$CVSWORK"
 
 cvs init &&
-test -d "$CVSROOT" &&
+test_path_is_dir "$CVSROOT" &&
 cvs -Q co -d "$CVSWORK" . &&
 echo >empty &&
 git add empty &&
@@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
 	git commit -m "Added attic_gremlin" &&
 	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
 	(cd "$CVSWORK" && cvs -Q update -d) &&
-	test -f "$CVSWORK/attic_gremlin"
+	test_path_is_file "$CVSWORK/attic_gremlin"
 '
 
 # the state of the CVS sandbox may be indeterminate for ' space'

-- 
2.43.0
Junio C Hamano· Mar 9, 2026, 15:20 UTC · re: Pablo Sabater · lore

Re: [GSoC PATCH] t9200: use helpers to replace test -f <path> and test -d <path>

Pablo Sabater <pabloosabaterr@gmail.com> writes:
Show 5 quoted lines
> Replaced 'test -f' and 'test -d' with 'test_path_is_file' and 'test_path_is_dir'
>
> I've used 'git grep "test -f" t/t9*.sh' to find a file without the fix done as specified on the microproject information
> I've done '9*' because the ones I've found first had already fix patches.
> I've taken as example another patch sent 't4131' from Junio C Hamano https://lore.kernel.org/git/xmqq1rpodn25.fsf@gitster.c.googlers.com/#r

After studying Documentation/{SubmittingPatches,CodingGuidelines}, use the list archive to find what instructions GSoC participant candidates have received regarding the proposed log messages in their microproject submissions.

Thanks.
Pablo· Mar 9, 2026, 15:39 UTC · re: Junio C Hamano · lore

Re: [GSoC PATCH] t9200: use helpers to replace test -f <path> and test -d <path>

> After studying Documentation/{SubmittingPatches,CodingGuidelines},
> use the list archive to find what instructions GSoC participant
> candidates have received regarding the proposed log messages in
> their microproject submissions.
Thanks for the feedback, I'll work on that right now, once done I'll send a v2.
Pablo
Pablo Sabater· Mar 9, 2026, 16:28 UTC · re: Pablo Sabater · lore

[GSoC PATCH v2] t9200: replace test -f/-d with modern path helpers

Replace old style 'test -f' and 'test -d' with modern helpers 'test_path_is_file' and 'test_path_is_dir' respectively.

The instances were found with:
	git grep "test -[efd]" t/
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
 t/t9200-git-cvsexportcommit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t9200-git-cvsexportcommit.sh +2 −2
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index a44eabf0d8..4507e8e6db 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
 rm -rf "$CVSROOT" "$CVSWORK"
 
 cvs init &&
-test -d "$CVSROOT" &&
+test_path_is_dir "$CVSROOT" &&
 cvs -Q co -d "$CVSWORK" . &&
 echo >empty &&
 git add empty &&
@@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
 	git commit -m "Added attic_gremlin" &&
 	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
 	(cd "$CVSWORK" && cvs -Q update -d) &&
-	test -f "$CVSWORK/attic_gremlin"
+	test_path_is_file "$CVSWORK/attic_gremlin"
 '
 
 # the state of the CVS sandbox may be indeterminate for ' space'

-- 
2.43.0
Junio C Hamano· Mar 9, 2026, 21:03 UTC · re: Pablo Sabater · lore

Re: [GSoC PATCH v2] t9200: replace test -f/-d with modern path helpers

Pablo Sabater <pabloosabaterr@gmail.com> writes:
> Replace old style 'test -f' and 'test -d' with modern helpers
> 'test_path_is_file' and 'test_path_is_dir' respectively.

OK. Being "modern" does not automatically mean "better", and it would be helpful to say why we do this change for those relatively unexperienced who will read "git log" later and find this commit. Perhaps

    Replace ... with ..., because it makes debugging a failing test
    easier by loudly reporting what expectation was not met.
or something.  The patch text looks good.
Thanks.
Show 8 quoted lines
> The instances were found with:
>
> 	git grep "test -[efd]" t/
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  t/t9200-git-cvsexportcommit.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
OK.
Show 22 quoted lines
> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> index a44eabf0d8..4507e8e6db 100755
> --- a/t/t9200-git-cvsexportcommit.sh
> +++ b/t/t9200-git-cvsexportcommit.sh
> @@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
>  rm -rf "$CVSROOT" "$CVSWORK"
>  
>  cvs init &&
> -test -d "$CVSROOT" &&
> +test_path_is_dir "$CVSROOT" &&
>  cvs -Q co -d "$CVSWORK" . &&
>  echo >empty &&
>  git add empty &&
> @@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
>  	git commit -m "Added attic_gremlin" &&
>  	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
>  	(cd "$CVSWORK" && cvs -Q update -d) &&
> -	test -f "$CVSWORK/attic_gremlin"
> +	test_path_is_file "$CVSWORK/attic_gremlin"
>  '
>  
>  # the state of the CVS sandbox may be indeterminate for ' space'
Pablo· Mar 9, 2026, 22:54 UTC · re: Junio C Hamano · lore

Re: [GSoC PATCH v2] t9200: replace test -f/-d with modern path helpers

Junio C Hamano <gitster@pobox.com> writes:
> OK.  Being "modern" does not automatically mean "better", and it
> would be helpful to say why we do this change for those relatively
> unexperienced who will read "git log" later and find this commit.
> Perhaps
Okay, thanks. I'll do that on the v3
Pablo
Pablo Sabater· Mar 9, 2026, 23:01 UTC · re: Pablo Sabater · lore

[GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Replace old style 'test -f' and 'test -d' with helpers 'test_path_is_file' and 'test_path_is_dir' respectively, which make debugging a failing test easier by loudly reporting what expectation was not met.

The instances were found with:
	git grep "test -[efd]" t/
Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
 t/t9200-git-cvsexportcommit.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t9200-git-cvsexportcommit.sh +2 −2
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index a44eabf0d8..4507e8e6db 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
 rm -rf "$CVSROOT" "$CVSWORK"
 
 cvs init &&
-test -d "$CVSROOT" &&
+test_path_is_dir "$CVSROOT" &&
 cvs -Q co -d "$CVSWORK" . &&
 echo >empty &&
 git add empty &&
@@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
 	git commit -m "Added attic_gremlin" &&
 	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
 	(cd "$CVSWORK" && cvs -Q update -d) &&
-	test -f "$CVSWORK/attic_gremlin"
+	test_path_is_file "$CVSWORK/attic_gremlin"
 '
 
 # the state of the CVS sandbox may be indeterminate for ' space'

-- 
2.43.0
Junio C Hamano· Mar 9, 2026, 23:26 UTC · re: Pablo Sabater · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Pablo Sabater <pabloosabaterr@gmail.com> writes:
Show 13 quoted lines
> Replace old style 'test -f' and 'test -d' with helpers
> 'test_path_is_file' and 'test_path_is_dir' respectively,
> which make debugging a failing test easier by loudly
> reporting what expectation was not met.
>
> The instances were found with:
>
> 	git grep "test -[efd]" t/
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  t/t9200-git-cvsexportcommit.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
Looking good.  Will queue.  Thanks.
Show 22 quoted lines
> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> index a44eabf0d8..4507e8e6db 100755
> --- a/t/t9200-git-cvsexportcommit.sh
> +++ b/t/t9200-git-cvsexportcommit.sh
> @@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
>  rm -rf "$CVSROOT" "$CVSWORK"
>  
>  cvs init &&
> -test -d "$CVSROOT" &&
> +test_path_is_dir "$CVSROOT" &&
>  cvs -Q co -d "$CVSWORK" . &&
>  echo >empty &&
>  git add empty &&
> @@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
>  	git commit -m "Added attic_gremlin" &&
>  	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
>  	(cd "$CVSWORK" && cvs -Q update -d) &&
> -	test -f "$CVSWORK/attic_gremlin"
> +	test_path_is_file "$CVSWORK/attic_gremlin"
>  '
>  
>  # the state of the CVS sandbox may be indeterminate for ' space'
Pablo· Mar 11, 2026, 10:59 UTC · re: Pablo Sabater · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Adding project mentors to CC.
Thanks.

El mar, 10 mar 2026 a las 0:01, Pablo Sabater (<pabloosabaterr@gmail.com>) escribió:

Show 40 quoted lines
>
> Replace old style 'test -f' and 'test -d' with helpers
> 'test_path_is_file' and 'test_path_is_dir' respectively,
> which make debugging a failing test easier by loudly
> reporting what expectation was not met.
>
> The instances were found with:
>
>         git grep "test -[efd]" t/
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  t/t9200-git-cvsexportcommit.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> index a44eabf0d8..4507e8e6db 100755
> --- a/t/t9200-git-cvsexportcommit.sh
> +++ b/t/t9200-git-cvsexportcommit.sh
> @@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
>  rm -rf "$CVSROOT" "$CVSWORK"
>
>  cvs init &&
> -test -d "$CVSROOT" &&
> +test_path_is_dir "$CVSROOT" &&
>  cvs -Q co -d "$CVSWORK" . &&
>  echo >empty &&
>  git add empty &&
> @@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
>         git commit -m "Added attic_gremlin" &&
>         git cvsexportcommit -w "$CVSWORK" -c HEAD &&
>         (cd "$CVSWORK" && cvs -Q update -d) &&
> -       test -f "$CVSWORK/attic_gremlin"
> +       test_path_is_file "$CVSWORK/attic_gremlin"
>  '
>
>  # the state of the CVS sandbox may be indeterminate for ' space'
> --
> 2.43.0
>
Junio C Hamano· Mar 11, 2026, 17:52 UTC · re: Pablo Sabater · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Pablo Sabater <pabloosabaterr@gmail.com> writes:
> Replace old style 'test -f' and 'test -d' with helpers
> 'test_path_is_file' and 'test_path_is_dir' respectively,
> which make debugging a failing test easier by loudly
> reporting what expectation was not met.
Well explained.
> The instances were found with:
>
> 	git grep "test -[efd]" t/

People seem to add the above to their test-path helper patches, but unless the coverage of the work is fairly thorough and you want to say "all the similar issues should be found with this command and I addressed all of them", I do not see much point saying how you found one of them and addressed it.

You could have used "git grep -e <pattern> -- t/\*.sh", or you could have been working to fix something in t9200 and noticed these while you were doing something else to the file.

I do not see it as too huge a deal and it is probably not a cause to send in another iteration once it is already written, though.

Show 18 quoted lines
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  t/t9200-git-cvsexportcommit.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> index a44eabf0d8..4507e8e6db 100755
> --- a/t/t9200-git-cvsexportcommit.sh
> +++ b/t/t9200-git-cvsexportcommit.sh
> @@ -31,7 +31,7 @@ export CVSROOT CVSWORK GIT_DIR
>  rm -rf "$CVSROOT" "$CVSWORK"
>  
>  cvs init &&
> -test -d "$CVSROOT" &&
> +test_path_is_dir "$CVSROOT" &&
>  cvs -Q co -d "$CVSWORK" . &&
>  echo >empty &&
>  git add empty &&

Our test-path helpers should work even outside test_expect_* functions, so this is not wrong per-se, but it somehow looks a bit unusual. A related clean-up would be to wrap the CVS initialization part inside another "do we even have a working CVS installation to make it worth our time testing 'git cvsexportcommit' command?" check, i.e.,

	if ! cvs init || ! test -d "$CVSROOT" || ! cvs -Q co -d "$CVSWORK" .
        then
		skip_all="cvs repository set-up fails"
		test_done
	fi
and then move the git initialization part to its own test, e.g.,
	test_expect_success 'git setup' '
		echo >empty &&
		git add empty &&
		git commit -q -a -m Initial
	'
Show 7 quoted lines
> @@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
>  	git commit -m "Added attic_gremlin" &&
>  	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
>  	(cd "$CVSWORK" && cvs -Q update -d) &&
> -	test -f "$CVSWORK/attic_gremlin"
> +	test_path_is_file "$CVSWORK/attic_gremlin"
>  '
OK.
>  
>  # the state of the CVS sandbox may be indeterminate for ' space'
Pablo· Mar 11, 2026, 19:06 UTC · re: Junio C Hamano · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Show 5 quoted lines
> Our test-path helpers should work even outside test_expect_*
> functions, so this is not wrong per-se, but it somehow looks a bit
> unusual.  A related clean-up would be to wrap the CVS initialization
> part inside another "do we even have a working CVS installation to
> make it worth our time testing 'git cvsexportcommit' command?"

Thanks for the feedback, I can send a separate patch to wrap the CVS in a skip_all git move the git setup

Junio C Hamano· Mar 11, 2026, 19:42 UTC · re: Pablo · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Pablo <pabloosabaterr@gmail.com> writes:
Show 9 quoted lines
>> Our test-path helpers should work even outside test_expect_*
>> functions, so this is not wrong per-se, but it somehow looks a bit
>> unusual.  A related clean-up would be to wrap the CVS initialization
>> part inside another "do we even have a working CVS installation to
>> make it worth our time testing 'git cvsexportcommit' command?"
>
>
> Thanks for the feedback, I can send a separate patch to wrap the CVS
> in a skip_all git move the git setup

Yeah, but if we are going to do so eventually, it would be pointless to use the path helper in that "set up CVS environment and make sure we got a sensible directory structure" check, no? Upon failure, we will hit test_done that loudly says that their CVS installation is not working as we expect.

Pablo· Mar 11, 2026, 19:49 UTC · re: Junio C Hamano · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Show 5 quoted lines
> Yeah, but if we are going to do so eventually, it would be pointless
> to use the path helper in that "set up CVS environment and make sure
> we got a sensible directory structure" check, no?  Upon failure, we
> will hit test_done that loudly says that their CVS installation is
> not working as we expect.

Yeah, the new patch will change it back to test -d because it ends up in a if condition instead of an assertion. Would you prefer to drop that hunk from my v3 or should I send a v4 ?

Junio C Hamano· Mar 11, 2026, 20:31 UTC · re: Pablo · lore

Re: [GSoC PATCH v3] t9200: replace test -f/-d with modern path helpers

Pablo <pabloosabaterr@gmail.com> writes:
Show 9 quoted lines
>> Yeah, but if we are going to do so eventually, it would be pointless
>> to use the path helper in that "set up CVS environment and make sure
>> we got a sensible directory structure" check, no?  Upon failure, we
>> will hit test_done that loudly says that their CVS installation is
>> not working as we expect.
>
> Yeah, the new patch will change it back to test -d because it ends up
> in a if condition instead of an assertion.
> Would you prefer to drop that hunk from my v3 or should I send a v4 ?

Yup, let me mark the "cvs setup failure" one ready for 'next'. The other hunk that updates "test -[efd]" can become a separate patch.

Thanks.
Pablo Sabater· Mar 12, 2026, 17:33 UTC · re: Pablo Sabater · lore

[GSoC PATCH v4] t9200: replace test -f with modern path helper

Replace old style 'test -f' with helper 'test_path_is_file', which make debugging a failing test easier by loudly reporting what expectation was not met.

Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
---
Changes from v3:
The first hunk was dropped from this patch, and sent as a separate patch.
https://lore.kernel.org/git/20260311194002.190195-1-pabloosabaterr@gmail.com/
 t/t9200-git-cvsexportcommit.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t9200-git-cvsexportcommit.sh +1 −1
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index a44eabf0d8..15a91931a2 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
 	git commit -m "Added attic_gremlin" &&
 	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
 	(cd "$CVSWORK" && cvs -Q update -d) &&
-	test -f "$CVSWORK/attic_gremlin"
+	test_path_is_file "$CVSWORK/attic_gremlin"
 '
 
 # the state of the CVS sandbox may be indeterminate for ' space'

-- 
2.43.0
Junio C Hamano· Mar 12, 2026, 17:37 UTC · re: Pablo Sabater · lore

Re: [GSoC PATCH v4] t9200: replace test -f with modern path helper

Pablo Sabater <pabloosabaterr@gmail.com> writes:
Show 7 quoted lines
> Replace old style 'test -f' with helper
> 'test_path_is_file', which make debugging
> a failing test easier by loudly reporting
> what expectation was not met.
>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
Will queue.  Looks good.  Thanks.
Show 20 quoted lines
> Changes from v3:
> The first hunk was dropped from this patch, and sent as a separate patch.
> https://lore.kernel.org/git/20260311194002.190195-1-pabloosabaterr@gmail.com/
>
>  t/t9200-git-cvsexportcommit.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> index a44eabf0d8..15a91931a2 100755
> --- a/t/t9200-git-cvsexportcommit.sh
> +++ b/t/t9200-git-cvsexportcommit.sh
> @@ -303,7 +303,7 @@ test_expect_success 're-commit a removed filename which remains in CVS attic' '
>  	git commit -m "Added attic_gremlin" &&
>  	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
>  	(cd "$CVSWORK" && cvs -Q update -d) &&
> -	test -f "$CVSWORK/attic_gremlin"
> +	test_path_is_file "$CVSWORK/attic_gremlin"
>  '
>  
>  # the state of the CVS sandbox may be indeterminate for ' space'

← back to recent threads