Re: [PATCH] t9902: split test to run on appropriate systems

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 08 2022, Adam Dinwoodie wrote:

> The "FUNNYNAMES" test prerequisite passes on Cygwin, as the Cygwin
> file system interface has a workaround for the underlying operating
> system's lack of support for tabs, newlines or quotes.  However, it does
> not add support for backslash, which is treated as a directory
> separator, meaning one of the tests added by 48803821b1 ("completion:
> handle unusual characters for sparse-checkout", 2022-02-07) will fail on
> Cygwin.
>
> To avoid this failure while still getting maximal test coverage, split
> that test into two: test handling of paths that include tabs on anything
> that has the FUNNYNAMES prerequisite, but skip testing handling of paths
> that include backslashes unless both FUNNYNAMES is set and the system is
> not Cygwin.
>
> It might be nice to have more granularity than "FUNNYNAMES" and its
> sibling "FUNNIERNAMES" provide, so that tests could be run based on
> specific individual characters supported by the file system being
> tested, but that seems like it would make the prerequisite checks in
> this area much more verbose for very little gain.

For getting the release out the door this seems like a sensible isolated
fix, but I don't see why we wouldn't get more granularity here,
i.e. something like the below.

I converted all the straightforward cases, where these tests were either
a bit misleading, or we'd actually skip testing on some systems
needlessly e.g. if they supported \t in a name but not \n.

This leaves only 8 remaining cases of FUNNYNAMES, all of those similarly
seem like subtle potential issues. I.e. we're creating files with
characters like "?" or "*" in the name.

But the prerequisite never checks for that, we're just implicitly
assuming that a FS that can do [\t\n"] an also do [*?+] or whatever.

In the case of the "rm" test we'd unconditionally create a file with a
space in its name, but then conditional on FUNNYNAMES remove it.

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index e74a318ac33..7714fc4852f 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -17,12 +17,7 @@ test_expect_success 'Initialize test directory' '
 	git commit -m "add normal files"
 '
 
-if test_have_prereq !FUNNYNAMES
-then
-	say 'Your filesystem does not allow tabs in filenames.'
-fi
-
-test_expect_success FUNNYNAMES 'add files with funny names' '
+test_expect_success FS_NAME_TAB,FS_NAME_NEWLINE 'add files with funny names' '
 	touch -- "tab	embedded" "newline${LF}embedded" &&
 	git add -- "tab	embedded" "newline${LF}embedded" &&
 	git commit -m "add files with tabs and newlines"
@@ -94,8 +89,12 @@ test_expect_success 'Test that "git rm -- -q" succeeds (remove a file that looks
 	git rm -- -q
 '
 
-test_expect_success FUNNYNAMES 'Test that "git rm -f" succeeds with embedded space, tab, or newline characters.' '
-	git rm -f "space embedded" "tab	embedded" "newline${LF}embedded"
+test_expect_success FS_NAME_TAB,FS_NAME_NEWLINE 'Test that "git rm -f" succeeds with embedded tab or newline characters.' '
+	git rm -f "tab	embedded" "newline${LF}embedded"
+'
+
+test_expect_success 'Test that "git rm -f" succeeds with embedded space.' '
+	git rm -f "space embedded"
 '
 
 test_expect_success SANITY 'Test that "git rm -f" fails if its rm fails' '
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 9a292bac70c..5dbe688ab10 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -482,7 +482,7 @@ test_expect_success '--combined-all-paths and --cc' '
 	test_cmp expect actual
 '
 
-test_expect_success FUNNYNAMES 'setup for --combined-all-paths with funny names' '
+test_expect_success FS_NAME_TAB 'setup for --combined-all-paths with funny names' '
 	git branch side1d &&
 	git branch side2d &&
 	git checkout side1d &&
@@ -507,7 +507,7 @@ test_expect_success FUNNYNAMES 'setup for --combined-all-paths with funny names'
 	head=$(git rev-parse HEAD)
 '
 
-test_expect_success FUNNYNAMES '--combined-all-paths and --raw and funny names' '
+test_expect_success FS_NAME_TAB '--combined-all-paths and --raw and funny names' '
 	cat <<-EOF >expect &&
 	::100644 100644 100644 $side1df $side2df $headf RR	"file\twith\ttabs"	"i\tam\ttabbed"	"fickle\tnaming"
 	EOF
@@ -516,13 +516,13 @@ test_expect_success FUNNYNAMES '--combined-all-paths and --raw and funny names'
 	test_cmp expect actual
 '
 
-test_expect_success FUNNYNAMES '--combined-all-paths and --raw -and -z and funny names' '
+test_expect_success FS_NAME_TAB '--combined-all-paths and --raw -and -z and funny names' '
 	printf "$head\0::100644 100644 100644 $side1df $side2df $headf RR\0file\twith\ttabs\0i\tam\ttabbed\0fickle\tnaming\0" >expect &&
 	git diff-tree -c -M --raw --combined-all-paths -z HEAD >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success FUNNYNAMES '--combined-all-paths and --cc and funny names' '
+test_expect_success FS_NAME_TAB '--combined-all-paths and --cc and funny names' '
 	cat <<-\EOF >expect &&
 	--- "a/file\twith\ttabs"
 	--- "a/i\tam\ttabbed"
diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh
index 6bc3fb97a75..582666c691b 100755
--- a/t/t4135-apply-weird-filenames.sh
+++ b/t/t4135-apply-weird-filenames.sh
@@ -53,9 +53,9 @@ try_filename() {
 
 try_filename 'plain'            'postimage.txt'
 try_filename 'with spaces'      'post image.txt'
-try_filename 'with tab'         'post	image.txt' FUNNYNAMES
+try_filename 'with tab'         'post	image.txt' FS_NAME_TAB
 try_filename 'with backslash'   'post\image.txt' BSLASHPSPEC
-try_filename 'with quote'       '"postimage".txt' FUNNYNAMES success failure success
+try_filename 'with quote'       '"postimage".txt' FS_NAME_QUOTE success failure success
 
 test_expect_success 'whitespace-damaged traditional patch' '
 	echo postimage >expected &&
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 69356011713..5b880552a05 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -82,7 +82,7 @@ test_expect_success setup '
 	# Still a no-op.
 	function dummy() {}
 	EOF
-	if test_have_prereq FUNNYNAMES
+	if test_have_prereq FS_NAME_QUOTE
 	then
 		echo unusual >"\"unusual\" pathname" &&
 		echo unusual >"t/nested \"unusual\" pathname"
@@ -525,7 +525,7 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success FUNNYNAMES "grep $L should quote unusual pathnames" '
+	test_expect_success FS_NAME_QUOTE "grep $L should quote unusual pathnames" '
 		cat >expected <<-EOF &&
 		${HC}"\"unusual\" pathname":unusual
 		${HC}"t/nested \"unusual\" pathname":unusual
@@ -534,7 +534,7 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success FUNNYNAMES "grep $L in subdir should quote unusual relative pathnames" '
+	test_expect_success FS_NAME_QUOTE "grep $L in subdir should quote unusual relative pathnames" '
 		cat >expected <<-EOF &&
 		${HC}"nested \"unusual\" pathname":unusual
 		EOF
@@ -545,7 +545,7 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success FUNNYNAMES "grep -z $L with unusual pathnames" '
+	test_expect_success FS_NAME_QUOTE "grep -z $L with unusual pathnames" '
 		cat >expected <<-EOF &&
 		${HC}"unusual" pathname:unusual
 		${HC}t/nested "unusual" pathname:unusual
@@ -555,7 +555,7 @@ do
 		test_cmp expected actual-replace-null
 	'
 
-	test_expect_success FUNNYNAMES "grep -z $L in subdir with unusual relative pathnames" '
+	test_expect_success FS_NAME_QUOTE "grep -z $L in subdir with unusual relative pathnames" '
 		cat >expected <<-EOF &&
 		${HC}nested "unusual" pathname:unusual
 		EOF
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0f..014f822a089 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -66,11 +66,11 @@ test_expect_success 'prompt - unborn branch' '
 	test_cmp expected "$actual"
 '
 
-if test_have_prereq !FUNNYNAMES; then
+if test_have_prereq !FS_NAME_NEWLINE; then
 	say 'Your filesystem does not allow newlines in filenames.'
 fi
 
-test_expect_success FUNNYNAMES 'prompt - with newline in path' '
+test_expect_success FS_NAME_NEWLINE 'prompt - with newline in path' '
     repo_with_newline="repo
 with
 newline" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 531cef097db..3b2fcd8afdf 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1700,20 +1700,29 @@ test_lazy_prereq CASE_INSENSITIVE_FS '
 	test "$(cat CamelCase)" != good
 '
 
-test_lazy_prereq FUNNYNAMES '
-	test_have_prereq !MINGW &&
-	touch -- \
-		"FUNNYNAMES tab	embedded" \
-		"FUNNYNAMES \"quote embedded\"" \
-		"FUNNYNAMES newline
+test_lazy_prereq FS_NAME_TAB '
+	touch -- "FUNNYNAMES tab	embedded" 2>/dev/null &&
+	rm -- "FUNNYNAMES tab	embedded"  2>/dev/null
+'
+test_lazy_prereq FS_NAME_QUOTE '
+	touch -- "FUNNYNAMES \"quote embedded\"" 2>/dev/null &&
+	rm -- "FUNNYNAMES \"quote embedded\""  2>/dev/null
+'
+test_lazy_prereq FS_NAME_NEWLINE '
+	touch -- "FUNNYNAMES newline
 embedded" 2>/dev/null &&
-	rm -- \
-		"FUNNYNAMES tab	embedded" \
-		"FUNNYNAMES \"quote embedded\"" \
-		"FUNNYNAMES newline
+	rm -- "FUNNYNAMES newline
 embedded" 2>/dev/null
 '
 
+# Please use a more specific FS_NAME_* check if possible.
+test_lazy_prereq FUNNYNAMES '
+	test_have_prereq !MINGW &&
+	test_have_prereq FS_NAME_TAB &&
+	test_have_prereq FS_NAME_QUOTE &&
+	test_have_prereq FS_NAME_NEWLINE
+'
+
 test_lazy_prereq UTF8_NFD_TO_NFC '
 	# check whether FS converts nfd unicode to nfc
 	auml=$(printf "\303\244")