tboegi@xxxxxx writes: > From: Torsten Bögershausen <tboegi@xxxxxx> > > 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". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html