Thread (2 messages) 2 messages, 2 authors, 2026-03-11

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

From: Junio C Hamano <hidden>
Date: 2026-03-11 17:52:46

Possibly related (same subject, not in this thread)

Pablo Sabater [off-list ref] 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.
quoted hunk
Signed-off-by: Pablo Sabater <redacted>
---
 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
	'
quoted hunk
@@ -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'
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help