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

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

[Pablo Sabater]

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

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

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

[Junio C Hamano]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

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

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

[Pablo]

> 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

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

[Pablo Sabater]

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

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

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

[Junio C Hamano]

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.

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

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

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

[Pablo]

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

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

[Pablo Sabater]

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

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

[Junio C Hamano]

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


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

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

[Pablo]

Adding project mentors to CC.

Thanks.

El mar, 10 mar 2026 a las 0:01, Pablo Sabater
(<pabloosabaterr@gmail.com>) escribió:
>
> 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
>

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

[Junio C Hamano]

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.

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

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

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

[Pablo]

> 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

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

[Junio C Hamano]

Pablo <pabloosabaterr@gmail.com> writes:

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

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

[Pablo]

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

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

[Junio C Hamano]

Pablo <pabloosabaterr@gmail.com> writes:

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

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

[Pablo Sabater]

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

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

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

[Junio C Hamano]

Pablo Sabater <pabloosabaterr@gmail.com> writes:

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

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