Subject: Re: [PATCH v2 3/8] log tests: check if grep_config() is called by "log"-like cmds

From: Junio C Hamano <[email-masked]>
Date: Fri, 12 Nov 2021 09:09:18 -0800

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> Extend the tests added in my 9df46763ef1 (log: add exhaustive tests
> for pattern style options & config, 2017-05-20) to check not only
> whether "git log" handles "grep.patternType", but also "git show"
> etc.
>
> It's sufficient to check whether a PCRE regex matches for the purposes
> of this test, we otherwise assume that it's running the same code as
> "git log", whose behavior is tested more exhaustively by test added in
> 9df46763ef1e.

I do agree with the reasoning that it is sufficient to check with
only one kind, but it is unclear if PCRE is so special and if so
why.  Wouldn't testing with, say grep.patternType=extended, alone
also be sufficient?  The reason I am asking it is mostly because the
above description is insufficient to answer these questions, but
also because we can lose the prerequisite and gain a bit wider test
coverage, if we use a pattern that is not PCRE.

Knowing that "show", "whatchanged", "reflog", and "format-patch" all
share the same backend in builtin/log.c, I am not sure how much
value the new tests add.  Testing the "--grep-reflog" option and the
"git rev-list" command may exercise a bit different code path,
though.

> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  t/t4202-log.sh | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 7884e3d46b3..a114c49ef27 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -449,6 +449,22 @@ test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurati
>  	)
>  '
>  
> +for cmd in show whatchanged reflog format-patch
> +do
> +	myarg=
> +	if test "$cmd" = "format-patch"
> +	then
> +		myarg="HEAD~.."
> +	fi

I prefer to send the output that ought to contain the grep hits
consistently to "actual", i.e.

	case "$cmd" in
	format-patch) myarg="--stdout -1" ;;
	*) myarg= ;;
	esac	

instead of sending only the filenames.

I also prefer "case/esac" as it would be more concise when we need
to tweak myarg per command later.

> +	test_expect_success PCRE "$cmd: understands grep.patternType=perl, like 'log'" '
> +		git -c grep.patternType=fixed -C pattern-type $cmd --grep="1(?=\|2)" $myarg >actual &&
> +		test_must_be_empty actual &&
> +		git -c grep.patternType=perl -C pattern-type $cmd --grep="1(?=\|2)" $myarg >actual &&
> +		test_file_not_empty actual
> +	'
> +done
> +
>  test_expect_success 'log --author' '
>  	cat >expect <<-\EOF &&
>  	Author: <BOLD;RED>A U<RESET> Thor <[email-masked]>

Thanks.