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

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Tue, Aug 4, 2009 at 9:25 AM, Junio C Hamano<gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> 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);

This is also a problem with some other parts of that patch. I'll apply
the suggestion you mention below to all of them.

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

I'll add a comment for that that.

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

Right. I'll make it do something else too.

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

Indeed.

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

Ah, interesting. This was the first test I wrote (mostly copied from
the original patch) and I didn't have a clear idea of the difference
between the expected failures (although the comment about  'fixing'
that the test printed when some failing case didn't should have put me
on the right track).

I'll resend with a fixed test.

-- 
Giuseppe "Oblomov" Bilotta