Re: [PATCH 1/3] git reset --hard gives clean working tree

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

Torsten Bögershausen <tboegi@web.de> writes:

> On 11.02.16 19:49, Junio C Hamano wrote:
>
>> I did not continue the approach I illustrated because I realized and
>> finally convinced myself that touching ce_compare_data() is a wrong
>> solution--it breaks "diff-files".
>>
>> Imagine if you have contents in the index that wouldn't have been
>> left by a "clean" conversion of what is in the working tree.  You
>> then run "git checkout -f".  Now the contents in the working tree
>> will still not convert back to what is in the index with another
>> "clean" conversion, but it should pass the "Am I safe to proceed"
>> check, namely, it matches what convert_to_worktree() would give.
>>
>> But imagine further what would happen when you add an extra blank
>> line at the end of the file in the working tree (i.e. "echo >>file")
>> and then run "diff-files -p".
>>
>> The illustration patch I gave broke "diff-files" in such a way that
>> before such an addition of an extra blank line, it would have said
>> "No changes".  And if you run "diff-files" after adding that extra
>> blank line, you will see whole bunch of changes, not just the extra
>> blank line at the end.
>>
>> This is sufficient to convince me that the approach is broken.
> []
> Would something like this make sense?
> (The main part is in diff.c, the rest needs some polishing)
>
> commit e494c31fd2f0f8a638ff14d1b8ae3f3a6d56a107
> Author: Torsten Bögershausen <tboegi@web.de>
> Date:   Sat Mar 5 07:51:08 2016 +0100
>
>     Text files needs to be normalized before diffing
>    
>     Whenever a text file is stored with CRLF in the index, Git changes
>     CRLF into LF at the next commit.
>     (text file means the attribute "text" or "crlf" of "eol" is set).

I do not think I can endorse this approach, as I cannot explain why
it could possibly make sense to treat as if CRLF conversion is
somehow special among all the convert_to_git()/convert_to_worktree()
conversions, which your patch does to the diff code.

Comparisons between contents from two tree objects and comparisons
between contents from a tree object and the index must happen
without conversion, and comparisons between contents from the tree
in the HEAD commit and contents from the working tree must be done
in line with the HEAD vs index comparison to serve as a preview of
what would happen after adding the contents taken from the working
tree to the index, which means we should compare what is in the
index (without conversion) and the result of running the whole
convert_to_git() conversion on what is in the working tree.  It
feels fundamentally wrong to apply only CRLF conversion without any
other conversion, whether the direction of the conversion is from
worktree to git or the other way around, when comparing two things.

When the user has CRLF data in the index and the user tell the
attribute system so that the next "add" would result in "fixing" the
indexed lines to be terminated with LF, "diff-files" _should_ show
that correction as a change, I think.