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

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

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

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.