Torsten, you've been quite active in fixing various glitches around the EOL conversion in the latter half of last year. Have any thoughts to share on this topic?
Thanks. Mike Crowe <m...@mcrowe.com> writes: > On Monday 20 February 2017 at 13:25:01 -0800, Junio C Hamano wrote: >> This almost makes me suspect that the place that checks lengths of >> one and two in order to refrain from running more expensive content >> comparison you found earlier need to ask would_convert_to_git() >> before taking the short-cut, something along the lines of this (for >> illustration purposes only, not even compile-tested). The "almost" >> comes to me because I do not offhand know the performance implications >> of making calls to would_convert_to_git() here. >> >> diff.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/diff.c b/diff.c >> index 051761be40..094d5913da 100644 >> --- a/diff.c >> +++ b/diff.c >> @@ -4921,9 +4921,10 @@ static int diff_filespec_check_stat_unmatch(struct >> diff_filepair *p) >> * differences. >> * >> * 2. At this point, the file is known to be modified, >> - * with the same mode and size, and the object >> - * name of one side is unknown. Need to inspect >> - * the identical contents. >> + * with the same mode and size, the object >> + * name of one side is unknown, or size comparison >> + * cannot be depended upon. Need to inspect the >> + * contents. >> */ >> if (!DIFF_FILE_VALID(p->one) || /* (1) */ >> !DIFF_FILE_VALID(p->two) || >> @@ -4931,7 +4932,16 @@ static int diff_filespec_check_stat_unmatch(struct >> diff_filepair *p) >> (p->one->mode != p->two->mode) || >> diff_populate_filespec(p->one, CHECK_SIZE_ONLY) || >> diff_populate_filespec(p->two, CHECK_SIZE_ONLY) || >> - (p->one->size != p->two->size) || >> + >> + /* >> + * only if eol and other conversions are not involved, >> + * we can say that two contents of different sizes >> + * cannot be the same without checking their contents. >> + */ >> + (!would_convert_to_git(p->one->path) && >> + !would_convert_to_git(p->two->path) && >> + (p->one->size != p->two->size)) || >> + >> !diff_filespec_is_identical(p->one, p->two)) /* (2) */ >> p->skip_stat_unmatch_result = 1; >> return p->skip_stat_unmatch_result; >> >> > > Thanks for investigating this. I think you are correct that I was misguided > in my previous "fix". However, your change above does fix the problem for > me. > > It looks like the main cost of convert_to_git is in convert_attrs which > ends up doing various path operations in attr.c. After that, both > apply_filter and crlf_to_git return straight away if there's nothing to do. > > I experimented several times with running "git diff -quiet" after touching > every file in Git's own worktree and any difference in total time was lost > in the noise. > > I've further improved my test case. Tests 3 and 4 fail without the above > change but pass with it. Unfortunately I'm still unable to get those tests > to fail without the above fix unless the sleeps are present. I've tried > using the "touch -r .datetime" technique from racy-git.txt but it doesn't > help. It seems that I'm unable to stop Git from using its cache without > sleeping. :( > > diff --git a/t/t4063-diff-converted.sh b/t/t4063-diff-converted.sh > new file mode 100755 > index 0000000..31a730d > --- /dev/null > +++ b/t/t4063-diff-converted.sh > @@ -0,0 +1,44 @@ > +#!/bin/sh > +# > +# Copyright (c) 2017 Mike Crowe > +# > +# These tests ensure that files changing line endings in the presence > +# of .gitattributes to indicate that line endings should be ignored > +# don't cause 'git diff' or 'git diff --quiet' to think that they have > +# been changed. > +# > +# The sleeps are necessary to reproduce the problem for reasons that I > +# don't understand. > + > +test_description='git diff with files that require CRLF conversion' > + > +. ./test-lib.sh > + > +test_expect_success setup ' > + echo "* text=auto" > .gitattributes && > + printf "Hello\r\nWorld\r\n" > crlf.txt && > + printf "Hello\nWorld\n" > lf.txt && > + git add .gitattributes crlf.txt lf.txt && > + git commit -m "initial" && echo three > +' > +test_expect_success 'noisy diff works on file that requires CRLF conversion' > ' > + git status >/dev/null && > + git diff >/dev/null && > + sleep 1 && > + touch crlf.txt lf.txt && > + git diff >/dev/null > +' > +test_expect_success 'quiet diff works on file that requires CRLF conversion > with no changes' ' > + git status && > + git diff --quiet && > + sleep 1 && > + touch crlf.txt lf.txt && > + git diff --quiet > +' > + > +test_expect_success 'quiet diff works on file with line-ending change that > has no effect on repository' ' > + printf "Hello\nWorld\n" > crlf.txt && > + printf "Hello\r\nWorld\r\n" > lf.txt && > + git diff --quiet > +' > +test_done > > > Mike.
