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

["Torsten Bögershausen" <tboegi@web.de>]

On 11.02.16 19:49, Junio C Hamano wrote:
> 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.
[]
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).
   
    "git diff" reports that all lines with CRLF in the index as changed to LF.
    After cloning a repo, the work tree is not considered clean by Git, even if
    the user didn't change a single line.
   
    Avoid to report lines as changed by converting the content from the index into
    LF before running diff.

diff --git a/convert.c b/convert.c
index f524b8d..af8248d 100644
--- a/convert.c
+++ b/convert.c
@@ -231,9 +231,9 @@ static int has_cr_in_index(const char *path)
     return has_cr;
 }
 
-static int crlf_to_git(const char *path, const char *src, size_t len,
-               struct strbuf *buf,
-               enum crlf_action crlf_action, enum safe_crlf checksafe)
+static int crlf_to_git_internal(const char *path, const char *src, size_t len,
+                struct strbuf *buf,
+                enum crlf_action crlf_action, enum safe_crlf checksafe)
 {
     struct text_stat stats;
     char *dst;
@@ -852,7 +852,17 @@ const char *get_convert_attr_ascii(const char *path)
     return "";
 }
 
-int convert_to_git(const char *path, const char *src, size_t len,
+int convert_crlf_to_git(const char *path, const char *src, size_t len,
+            struct strbuf *buf,
+            enum safe_crlf checksafe)
+{
+    struct conv_attrs ca;
+    convert_attrs(&ca, path);
+    return crlf_to_git_internal(path, src, len, buf,
+                    ca.crlf_action, checksafe);
+
+}
+    int convert_to_git(const char *path, const char *src, size_t len,
                    struct strbuf *dst, enum safe_crlf checksafe)
 {
     int ret = 0;
@@ -874,7 +884,7 @@ int convert_to_git(const char *path, const char *src, size_t len,
         src = dst->buf;
         len = dst->len;
     }
-    ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
+    ret |= crlf_to_git_internal(path, src, len, dst, ca.crlf_action, checksafe);
     if (ret && dst) {
         src = dst->buf;
         len = dst->len;
@@ -894,7 +904,7 @@ void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
     if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
         die("%s: clean filter '%s' failed", path, ca.drv->name);
 
-    crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
+    crlf_to_git_internal(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
     ident_to_git(path, dst->buf, dst->len, dst, ca.ident);
 }
 
diff --git a/convert.h b/convert.h
index ccf436b..11eff02 100644
--- a/convert.h
+++ b/convert.h
@@ -52,6 +52,9 @@ extern void convert_to_git_filter_fd(const char *path, int fd,
                      struct strbuf *dst,
                      enum safe_crlf checksafe);
 extern int would_convert_to_git_filter_fd(const char *path);
+extern int convert_crlf_to_git(const char *path, const char *src, size_t len,
+                   struct strbuf *buf,
+                   enum safe_crlf checksafe);
 
 /*****************************************************************
  *
diff --git a/diff.c b/diff.c
index 059123c..8710a36 100644
--- a/diff.c
+++ b/diff.c
@@ -2730,6 +2730,7 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
  */
 int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
 {
+    struct strbuf buf = STRBUF_INIT;
     int size_only = flags & CHECK_SIZE_ONLY;
     int err = 0;
     /*
@@ -2756,7 +2757,6 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
 
     if (!s->sha1_valid ||
         reuse_worktree_file(s->path, s->sha1, 0)) {
-        struct strbuf buf = STRBUF_INIT;
         struct stat st;
         int fd;
 
@@ -2826,6 +2826,12 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
         if (!s->data)
             die("unable to read %s", sha1_to_hex(s->sha1));
         s->should_free = 1;
+        if (convert_crlf_to_git(s->path, s->data, s->size,
+                    &buf, SAFE_CRLF_FALSE)) {
+            size_t size = 0;
+            s->data = strbuf_detach(&buf, &size);
+            s->size = size;
+        }
     }
     return 0;
 }
diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index c164b46..dd1645b 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -39,7 +39,7 @@ test_expect_success 'default settings cause no changes' '
     test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff"
 '
 
-test_expect_success 'crlf=true causes a CRLF file to be normalized' '
+test_expect_success 'crlf=true on a CRLF file git diff is empty' '
 
     # Backwards compatibility check
     rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
@@ -49,19 +49,18 @@ test_expect_success 'crlf=true causes a CRLF file to be normalized' '
     # Note, "normalized" means that git will normalize it if added
     has_cr CRLFonly &&
     CRLFonlydiff=$(git diff CRLFonly) &&
-    test -n "$CRLFonlydiff"
+    test -z "$CRLFonlydiff"
 '
 
-test_expect_success 'text=true causes a CRLF file to be normalized' '
+test_expect_success 'eol=crlf gives CRLF with no diff' '
 
     rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-    echo "CRLFonly text" > .gitattributes &&
+    echo "CRLFonly text eol=crlf" > .gitattributes &&
     git read-tree --reset -u HEAD &&
-
-    # Note, "normalized" means that git will normalize it if added
-    has_cr CRLFonly &&
-    CRLFonlydiff=$(git diff CRLFonly) &&
-    test -n "$CRLFonlydiff"
+    >expect &&
+    git diff CRLFonly | tr "\015" Q >actual &&
+    test_cmp expect actual &&
+    has_cr CRLFonly
 '
 
 test_expect_success 'eol=crlf gives a normalized file CRLFs with autocrlf=false' '
@@ -114,7 +113,7 @@ test_expect_success 'autocrlf=true does not normalize CRLF files' '
     test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff"
 '
 
-test_expect_success 'text=auto, autocrlf=true _does_ normalize CRLF files' '
+test_expect_success 'text=auto, autocrlf=true git diff is empty' '
 
     rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
     git config core.autocrlf true &&
@@ -126,7 +125,7 @@ test_expect_success 'text=auto, autocrlf=true _does_ normalize CRLF files' '
     LFonlydiff=$(git diff LFonly) &&
     CRLFonlydiff=$(git diff CRLFonly) &&
     LFwithNULdiff=$(git diff LFwithNUL) &&
-    test -z "$LFonlydiff" -a -n "$CRLFonlydiff" -a -z "$LFwithNULdiff"
+    test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff"
 '
 
 test_expect_success 'text=auto, autocrlf=true does not normalize binary files' '