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

Torsten Bögershausen [off-list ref] writes:

On 11.02.16 19:49, Junio C Hamano wrote:

quoted

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 [off-list ref]
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.

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.

Fair enough.
There are 2 users here:
User 1 commits files with CRLF into the index, and later decides
to set the "text eol=crlf" attribute on it, without normalizing the repo.

User 2 does a simple "git clone", which includes checkout.
Running "git diff" tells user 2, that his work tree is dirty.

My conclusion is, that we could suppress the normalization for text files,
(as we do it for core.autocrlf with the new safer CRLF handling)
meaning that "git diff" and "git status" is clean and that files stay with CRLF
in the index.
Does this make sense ?