Re: Help/Advice needed on diff bug in xutils.c

[Thell Fowler <git@tbfowler.name>]

Johannes Schindelin (Johannes.Schindelin@gmx.de) wrote on Aug 5, 2009:

> On Tue, 4 Aug 2009, Thell Fowler wrote:
>
>> There is a bug in git diff (ignoring whitespace) that does not take into
>> account a trailing space at the end of a line at the end of a file when
>> no new line follows.

First, please forgive my hubris at thinking I had _found_ a bug when my 
ignorance of the issue was so obvious.  I am definitely humbled after 
reading every post in the archive having to do with whitespace and diff.

>>
>> Here is the example of the bug:
>> mkdir test_ws_eof
>> cd test_ws_eof
>> git init
>> echo -n "Test" > test.txt
>> git add .
>> git commit -m'test'
>> git symbolic-ref HEAD refs/heads/with_space
>> rm .git/index
>> git clean -f
>> echo -n "Test ">test.txt
>> git add .
>> git commit -m'test'
>> # Ignoring all whitespace there shouldn't be a diff.
>> git diff -w master -- test.txt
>> # Ignoring space at eol there shouldn't be a diff
>> git diff --ignore-space-at-eol master -- test.txt
>> # Ignoring with -b might have a case for a diff showing.
>> git diff -b master -- test.txt
>
> If you turn that into a patch to, say, t/t4015-diff-whitespace.sh (adding
> a test_expect_failure for a known bug), it is much easier to convince
> developers to work on the issue.
>

Thank you.  In progress.

I am curious if t4015 is planned to be to be rewritten to follow what 
Junio had outlined and Giuseppe implemented for 
t4107-apply-ignore-whitespace.sh to make the spaces more obvious to the reader:

One other question on the making of the test in regards to the 
following quote from:
http://article.gmane.org/gmane.comp.version-control.git/124765
where Junio C Hamano wrote:

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

The test being implemented was t4107-apply-ignore-whitespace.sh.

Are there any plans to have t4015-diff-whitespace.sh tests rewritten in 
the same fashion?

> First, your coding style is different from the surrounding code.  I think
> it goes without saying that this should be fixed.
>
> Second, you do not need the parentheses at all (and therefore they should
> go).
>
> Third, libxdiff does not assume to be fed NUL delimited strings.
>

You're absolutely right, I'll be more aware in the future.

> Fourth, that condition "ptr + 1 < top" is already doing what you tried to
> accomplish here.
>
> So I guess that you need to do add "ptr + 1 < top" checks
> instead.
>

I'll give it another go.

Thank you for the advice Dscho.

-- 
Thell