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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 5, 2016, 08:05 UTC
Message-ID: <xmqqtwklwdrh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <56DA896A.3050201@web.de>

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

Show 37 quoted lines

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