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

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

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> We define the working tree file is clean if either:
>
>   * the result of running convert_to_git() on the working tree
>     contents matches what is in the index (because that would mean
>     doing another "git add" on the path is a no-op); OR
>
>   * the result of running convert_to_working_tree() on the content
>     in the index matches what is in the working tree (because that
>     would mean doing another "git checkout -f" on the path is a
>     no-op).
>
> Add an extra check in ce_compare_data() in read_cache.c, and adjust
> the test cases in t0025:
> When a file has CRLF in the index, and is checked out into the working tree,
> but left unchabged, it is not normalized at the next commit.
> Whenever the file is changed in the working tree, a line is added/deleted
> or dos2unix is run, it may be normalized at the next commit,
> depending on .gitattributes.
>
> This patch is a result of a longer discussion on the mailing list,
> how to fix the flaky t0025.

Currently, the codepaths that want to stop if it would lose
information from the index and/or the working tree for the path run
an equivalent of "diff-files path" to see there is any difference.
This indeed is overly strict for a path that has contents in the
index that wouldn't have been created by "clean" conversion (I am
using this word to mean anything convert_to_git() does, not limited
to the "clean" filter).

And it is sensible to allow them to proceed if the contents in the
working tree file for the path match what would be created by
"smudge" conversion of the contents in the index.

But breaking "diff-files" is not the right way to do so.  Teaching
"Am I safe to proceed" callers that paths that do not pass
"diff-files" test may still be safe to work on is.

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.

The stolen code from me to do the reverse conversion and comparison
may still be reusable, but it should not go to ce_compare_data().
Such a path that has contents in the index that does not match what
"clean"-ing the working tree contents would give us _must_ remain
dirty and return DATA_CHANGED to ie_modified().

I think the right approach to solve this (assuming that this is
worth solving, which I am yet not sure) would go like this:

 * Allocate an extra bit in options passed to ie_match_stat().  When
   this bit is set, allow it to perform the "Smudge the contents in
   the index and compare it with the working tree contents" check
   and declare "You can proceed, nothing will be lost by touching
   the working tree", but never mark the cache entry up-to-date
   while doing so.

 * Pass the flag from callers that want this semantics,
   e.g. verify_uptodate() in unpack-trees.c, but not from
   "diff-files".

It probably is necessary to enable the new behaviour _only_ when a
new command line option explicitly asks for it, because

 - this is rather expensive;

 - this will have to be done for pretty-much all dirty paths, not
   just the ones that the additional reverse test would declare
   clean;

 - most normal people would not have such a broken data in the
   index;

 - those who has such a broken data would want to "fix" the data in
   the index as a tree-wide flag day correction event, after which
   they do not want to pay the penalty of doing this reverse
   comparison check all the time; and

 - requiring an explicit command line option is not a burden when
   the user _does_ want to do such a one-time "fix".