Re: [PATCH v3 2/2] diff: add -I<regex> that ignores matching changes

[Junio C Hamano <gitster@pobox.com>]

Michał Kępień <michal@isc.org> writes:

> +test_expect_success 'diff -I<regex>' '
> +	test_seq 50 >I.txt &&
> +	sed -e "s/13/ten and three/" -e "/7\$/d" <I.txt >J.txt &&
> +	echo >>J.txt &&
> +
> +	test_expect_code 1 git diff --no-index --ignore-blank-lines -I"ten.*e" -I"^[124-9]" I.txt J.txt >actual &&

Please wrap an overly long line like this oned, perhaps like:

	test_expect_code 1 git diff --no-index --ignore-blank-lines \
		-I"ten.*e" -I"^[124-9]" I.txt J.txt >actual &&

> +	test_expect_code 1 git diff --stat --no-index --ignore-blank-lines -I"ten.*e" -I"^[124-9]" I.txt J.txt >actual &&

> +	test_expect_code 129 git diff --no-index --ignore-matching-lines="^[124-9]" --ignore-matching-lines="^[124-9" I.txt J.txt >output 2>&1 &&

Cramming unrelated tests into a single one made me puzzled, staring
at this line for longer than necessary before realizing that this is
an attempt to catch a malformed regexp.  If this were in a separate
test with its own title, e.g.

	test_expect_success 'diff -I<regex>: setup' '
		... set up the sample input, perhaps sample history ...
		... perhaps prepare the expected output in "expect" ...
	'

	test_expect_success 'diff -I<regex> -p' '
		git diff --I"ten.*e" ... >actual &&
		test_cmp expect actual
	'

	test_expect_success 'diff -I<regex> --stat' '
		git diff --stat --I"ten.*e" ... >actual &&
		test_cmp expect actual
	'

	test_expect_success 'diff -I<regex>: detect malformed regex'
		test_must_fail git diff -I="[^124-9" ... 2>error &&
		test_i18ngrep "invalid regex" error
	'
		
It would have been much easier to follow.

It also is curious that it only tests --no-index, which is a bolted
on feature that may not exercise the codepath the users depend on
working correctly.  If this were tested with "git log -p" for a few
commits, the early destruction of regexp may have been caught,
especially with ASAN build.

Other than that, and also the premature destruction of regexp
pointed out by Phillip already, looks reasonably done.

Thanks.