Re: [PATCH 6/6] wildmatch test: create & test files on disk in addition to in-memory

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

On Sun, Dec 24 2017, Johannes Sixt jotted:

> Am 23.12.2017 um 22:30 schrieb Ævar Arnfjörð Bjarmason:
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   a[]b                 |   0
>>   t/t3070-wildmatch.sh | 336 ++++++++++++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 319 insertions(+), 17 deletions(-)
>>   create mode 100644 a[]b
>>
>> diff --git a/a[]b b/a[]b
>> new file mode 100644
>> index 0000000000..e69de29bb2
>
> A big no-no! This file can't be created on Windows!

Urgh, that was a mistake of mine. Will be gone in v2.

>> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
>> index 47b479e423..d423bb01f3 100755
>> --- a/t/t3070-wildmatch.sh
>> +++ b/t/t3070-wildmatch.sh
>> @@ -4,31 +4,146 @@ test_description='wildmatch tests'
>>
>>   . ./test-lib.sh
>>
>> +create_test_file() {
>> +	file=$1
>> +
>> +	# `touch .` will succeed but obviously not do what we intend
>> +	# here.
>> +	test "$file" = "." && return 1
>> +	# We cannot create a file with an empty filename.
>> +	test "$file" = "" && return 1
>> +	# The tests that are testing that e.g. foo//bar is matched by
>> +	# foo/*/bar can't be tested on filesystems since there's no
>> +	# way we're getting a double slash.
>> +	echo "$file" | grep -q -F '//' && return 1
>> +	# When testing the difference between foo/bar and foo/bar/ we
>> +	# can't test the latter.
>> +	echo "$file" | grep -q -E '/$' && return 1
>> +
>> +	dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')
>
> Booh! Booh! So many fork()s! ;)
>
> 	case "$file" in
> 	*//*)
> 		# The tests that are testing that e.g. foo//bar is matched by
> 		# foo/*/bar can't be tested on filesystems since there's no
> 		# way we're getting a double slash.
> 		return 1;;
> 	*/)
> 		# When testing the difference between foo/bar and foo/bar/ we
> 		# can't test the latter.
> 		return 1;;
> 	esac
>
> 	dirs=${file%/*}

Thanks. Will fix.

>> +
>> +	# We touch "./$file" instead of "$file" because even an
>> +	# escaped "touch -- -" means something different.
>> +	if test "$file" != "$dirs"
>> +	then
>> +		mkdir -p -- "$dirs" 2>/dev/null &&
>> +		touch -- "./$file" 2>/dev/null &&
>> +		return 0
>> +	else
>> +		touch -- "./$file" 2>/dev/null &&
>> +		return 0
>> +	fi
>> +	return 1
>> +}
>> +
>>   wildtest() {
>> -	match_w_glob=$1
>> -	match_w_globi=$2
>> -	match_w_pathmatch=$3
>> -	match_w_pathmatchi=$4
>> -	text=$5
>> -	pattern=$6
>> +	if test "$#" = 6
>> +	then
>> +		# When test-wildmatch and git ls-files produce the same
>> +		# result.
>> +		match_w_glob=$1
>> +		match_f_w_glob=$match_w_glob
>> +		match_w_globi=$2
>> +		match_f_w_globi=$match_w_globi
>> +		match_w_pathmatch=$3
>> +		match_f_w_pathmatch=$match_w_pathmatch
>> +		match_w_pathmatchi=$4
>> +		match_f_w_pathmatchi=$match_w_pathmatchi
>> +		text=$5
>> +		pattern=$6
>> +	elif test "$#" = 10
>> +	then
>> +		match_w_glob=$1
>> +		match_w_globi=$2
>> +		match_w_pathmatch=$3
>> +		match_w_pathmatchi=$4
>> +		match_f_w_glob=$5
>> +		match_f_w_globi=$6
>> +		match_f_w_pathmatch=$7
>> +		match_f_w_pathmatchi=$8
>> +		text=$9
>> +		pattern=$10
>> +	fi
>>
>> +	# $1: Case sensitive glob match: test-wildmatch
>>   	if test "$match_w_glob" = 1
>>   	then
>> -		test_expect_success "wildmatch:     match '$text' '$pattern'" "
>> +		test_expect_success "wildmatch: match '$text' '$pattern'" "
>>   			test-wildmatch wildmatch '$text' '$pattern'
>>   		"
>>   	elif test "$match_w_glob" = 0
>>   	then
>> -		test_expect_success "wildmatch:  no match '$text' '$pattern'" "
>> +		test_expect_success "wildmatch: no match '$text' '$pattern'" "
>>   			! test-wildmatch wildmatch '$text' '$pattern'
>>   		"
>>   	else
>>   		test_expect_success "PANIC: Test framework error. Unknown matches value $match_w_glob" 'false'
>
> I think you can write this as 'say ...; exit 1'. See t0000*.

Thanks. Didn't see an existing idiom for this, will use that.

>>   	fi
>>
>> +	# $1: Case sensitive glob match: ls-files
>> +	if test "$match_f_w_glob" = 'E'
>> +	then
>> +		if create_test_file "$text"
>> +		then
>> +			test_expect_success "wildmatch(ls): match dies on '$pattern' '$text'" "
>> +				test_when_finished \"
>> +					rm -rf -- * &&
>
> Can we be a bit more careful with this rm -rf, please?
> There is only one similarly loose case in t/t7003-filter-branch.sh,
> and it is outside test_when_finished, i.e., it is well under control;
> this instance here inside test_when_finished is not.

I can create the files inside some subfolder, cd to that and then run
the ls-files there. It would also cover cases where we have pattern
matching text that starts with ".".

>> +					git reset
>> +				\" &&
>> +				git add -A &&
>> +				>expect.err &&
>> +				printf '%s' '$text' >expect &&
>> +				test_must_fail git --glob-pathspecs ls-files -z -- '$pattern'
>> +			"
>> +		else
>> +			test_expect_failure "wildmatch(ls): match skip '$pattern' '$text'" 'false'
>> +		fi
>> +	elif test "$match_f_w_glob" = 1
>> +	then
>> +		if create_test_file "$text"
>> +		then
>> +			test_expect_success "wildmatch(ls): match '$pattern' '$text'" "
>> +				test_when_finished \"
>> +					rm -rf -- * &&
>> +					git reset
>> +				\" &&
>> +				git add -A &&
>> +				>expect.err &&
>> +				printf '%s' '$text' >expect &&
>
> There are no single-quotes in any $text instances, right?

There's not, but maybe we should be more careful here and use here-docs.

>> +				git --glob-pathspecs ls-files -z -- '$pattern' 2>actual.err | tr -d '\0' >actual &&
>
> If possible, do not put git commands in the upstream of a pipe.
> It does not detect failures.

Thanks, will split these up.

> Unfortunately, printf '%s\0' foo is not portable. If it were,
> you could omit the tr invocation alltogether.
>
>> +				test_cmp expect.err actual.err &&
>> +				test_cmp expect actual
>> +			"
>> +		else
>> +			test_expect_failure "wildmatch(ls): match skip '$pattern' '$text'" 'false'
>> +		fi
>
> -- Hannes