Re: [PATCHv4] git apply: option to ignore whitespace differences

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2009, 07:25 UTC
Message-ID: <7v8wi03usx.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1249072858-21894-1-git-send-email-giuseppe.bilotta@gmail.com>

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

Show 14 quoted lines

> diff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh
> new file mode 100755
> index 0000000..8e3fce3
> --- /dev/null
> +++ b/t/t4107-apply-ignore-whitespace.sh
> @@ -0,0 +1,149 @@
> ...
> +cat > patch3.patch <<\EOF
> +diff --git a/main.c b/main.c
> +--- a/main.c
> ++++ b/main.c
> +@@ -10,1 +10,1 @@
> + 		print_int(func(i)); 
> +EOF

This part is triply troublesome in that:

 (1) the payload ends with trailing whitespace which can be eaten by MUAs
     (and I almost always use "git am" with --whitespace=fix);
 
 (2) even if we apply your patch correctly, it is very unobvious that you
     are testing the case where the line has an unwanted trailing
     whitespace; and

 (3) a hunk without any added/deleted lines is an obviously artificial
     test input that would not appear in real life, something one would
     never think of doing unless one knows how "git apply" internally
     works.  It makes the test too knowledgable about the implementation.

You can fix the first two issues by doing:

	sed -e 's/Z/ /g' >patch3.patch <<\EOF
        ...
        +Z 	print_int(func(i));Z
        EOF

to make invisible SP stand out more for the benefit of people reading the test script (I know you did not have leading SP before HT in yours, but the above illustrates the visibility issues). For other tests with test vector patches, visibility of whitespace is not much an issue, but this script is _all about_ whitespace, so anything that clarifies what is going on better would help.

Show 5 quoted lines

> +test_expect_success "S = patch1" \
> +    'git-apply patch1.patch'
> +
> +test_expect_failure "F = patch2" \
> +    'git-apply patch2.patch'

Please say

	test_expect_success "whitespace corrupted patch does not apply" '
		test_must_fail git apply patch2.patch
        '

instead.

"test_expect_failure" is a declaration that the command being tested (in this case "git apply") is faulty. It also is a request for somebody interested to later fix it (again, in this case "git apply") to make this test pass.

But you do _not_ want this to pass without an explicit ignore option, so test_expect_failure is inappropriate here.