Re: [PATCH 04/29] log: add exhaustive tests for pattern style options & config

From: Junio C Hamano <gitster@pobox.com>
Date: May 12, 2017, 04:48 UTC
Message-ID: <xmqqwp9m7k9z.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170511091829.5634-5-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 6 quoted lines

> Add exhaustive tests for how the different grep.patternType options &
> the corresponding command-line options affect git-log.
> ...
> The patterns being passed to fixed/basic/extended/PCRE are carefully
> crafted to return the wrong thing if the grep engine were to pick any
> other matching method than the one it's told to use.

That is good; it may be even more helpful to the readers if they are given a brief in-code comment. I had to scratch head while reading them.

Show 15 quoted lines

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t4202-log.sh | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index f577990716..6d1411abea 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -262,7 +262,23 @@ test_expect_success 'log --grep -i' '
>  
>  test_expect_success 'log -F -E --grep=<ere> uses ere' '
>  	echo second >expect &&
> -	git log -1 --pretty="tformat:%s" -F -E --grep=s.c.nd >actual &&

	# BRE would need \(s\) to do the same.

Show 13 quoted lines

> +	git log -1 --pretty="tformat:%s" -F -E --grep="(s).c.nd" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success PCRE 'log -F -E --perl-regexp --grep=<pcre> uses PCRE' '
> +	test_when_finished "rm -rf num_commits" &&
> +	git init num_commits &&
> +	(
> +		cd num_commits &&
> +		test_commit 1d &&
> +		test_commit 2e
> +	) &&
> +	echo 2e >expect &&

	# In PCRE \d in [\d] is like saying "0-9", and match '2' in 2e

> +	git -C num_commits log -1 --pretty="tformat:%s" -F -E --perl-regexp --grep="[\d]" >actual &&
> +	test_cmp expect actual &&
> +	echo 1d >expect &&

	# In ERE [\d] is bs or letter 'd' and would not match '2' in '2e'
	# but does match 'd' in '1d'

Show 20 quoted lines

> +	git -C num_commits log -1 --pretty="tformat:%s" -F -E --grep="[\d]" >actual &&
>  	test_cmp expect actual
>  '
>  
> @@ -280,6 +296,65 @@ test_expect_success 'log with grep.patternType configuration and command line' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'log with various grep.patternType configurations & command-lines' '
> +	git init pattern-type &&
> +	(
> +		cd pattern-type &&
> +		test_commit 1 file A &&
> +		test_commit "(1|2)" file B &&
> +
> +		echo "(1|2)" >expect.fixed &&
> +		cp expect.fixed expect.basic &&
> +		cp expect.fixed expect.extended &&
> +		cp expect.fixed expect.perl &&
> +

		# Fixed finds these literally

> +		git -c grep.patternType=fixed log --pretty=tformat:%s \
> +			--grep="(1|2)" >actual.fixed &&

		# BRE matches (, |, and ) literally

> +		git -c grep.patternType=basic log --pretty=tformat:%s \
> +			--grep="(.|.)" >actual.basic &&

		# ERE needs | quoted with bs to match | literally

> +		git -c grep.patternType=extended log --pretty=tformat:%s \
> +			--grep="\|2" >actual.extended &&

If we use BRE by mistake, wouldn't this pattern actually find "(1|2)" anyway, without skipping it and show "1 file A" instead?

> +		if test_have_prereq PCRE
> +		then
> +			git -c grep.patternType=perl log --pretty=tformat:%s \
> +				--grep="[\d]\|" >actual.perl

			# Only PCRE would match [\d]\| with "(1|2)" due to [\d]

Show 8 quoted lines

> +		fi &&
> +		test_cmp expect.fixed actual.fixed &&
> +		test_cmp expect.basic actual.basic &&
> +		test_cmp expect.extended actual.extended &&
> +		if test_have_prereq PCRE
> +		then
> +			test_cmp expect.perl actual.perl
> +		fi &&

It could be just a style thing, but I would actually find it easier to follow if the above two "only with PCRE" tests, i.e. running test and taking its output in actual.perl and comparing it with expect.perl, were inside a single "if test_have_prereq PCRE" block.