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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 12, 2021, 17:09 UTC
Message-ID: <xmqqsfw1s3lt.fsf@gitster.g>
In-Reply-To: <patch-v2-3.8-41e38ebb32c-20211110T013632Z-avarab@gmail.com>

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

Show 9 quoted lines

> 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.

Show 20 quoted lines

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  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.

Show 11 quoted lines

> +	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 <author@example.com>

Thanks.