{"thread":{"id":"11525","subject":"An interaction with ce_match_stat_basic() and autocrlf","startedAt":"2008-01-08T12:12:24Z","lastAt":"2008-01-10T02:11:31Z","messageCount":6,"participants":["Junio C Hamano","Linus Torvalds","Pēteris Kļaviņš"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"64729","messageId":"7vfxx8tt1z.fsf@gitster.siamese.dyndns.org","threadId":"11525","inReplyTo":null,"subject":"An interaction with ce_match_stat_basic() and autocrlf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-08T12:12:24Z","receivedAt":"2008-01-08T12:12:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There is an interesting interaction with the stat matching and\nautocrlf.\n\n    $ git init\n    $ git config core.autocrlf true\n    $ echo a >a.txt\n    $ git add a.txt\n    $ unix2dos a.txt\n    $ git diff\n    diff --git a/a.txt b/a.txt\n\nAt this point, the index records a blob with LF line ending,\nwhile the work tree file has the same content with CRLF line\nending.  And the funny thing is that once you get into this\nsituation it is unfixable short of \"git add a.txt\".  Most\nnotably, \"git update-index --refresh\" (and the equilvalent\nauto-refresh that is implicitly run by \"git diff\" Porcelain)\nwill not update the cached stat information.\n\nThis is caused partly by the breakage in size_only codepath of\ndiff.c::diff_populate_filespec().  When taking the file contents\nfrom the work tree, it just gets stat data and thinks it got the\nfinal size, but it should actually convert the blob data into\ncanonical format.  diff.c::diffcore_skip_stat_unmatch() is\nfooled by this and declares that the path is modified.\n\nThis can be fixed by not returning early even when size_only is\nasked in the codepath.  It will make everything quite a lot more\nexpensive, as there currently is not a cheap way to ask \"is this\npath going to be munged by autocrlf or clean filter\", but\ngetting the correct result is more important than getting a\nquick but wrong result.\n\nBut that is just a half of the story.\n\n (1) It won't make the entry stat clean, as refresh_index()\n     later called from builtin-diff.c to clean up the stat\n     dirtiness works without paying attention to the autocrlf\n     conversion.\n\n (2) It won't help lower-level diff-files and internal callers\n     to ce_match_stat() that checks if the path were touched.\n     The \"read-tree -m -u\" codepath uses it to avoid touching\n     the path with local modifications.  The standard way to\n     clear the stat-dirtiness with \"git update-index --refresh\"\n     still needs to be fixed anyway.\n\nI was going to conclude this message by saying \"I need to sleep\non this to see if I can come up with a clean solution\", but it\nappears I do not have much time left for actually sleeping X-<.\n"},{"id":"64747","messageId":"alpine.LFD.1.00.0801080748080.3148@woody.linux-foundation.org","threadId":"11525","inReplyTo":"7vfxx8tt1z.fsf@gitster.siamese.dyndns.org","subject":"Re: An interaction with ce_match_stat_basic() and autocrlf","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-08T16:10:11Z","receivedAt":"2008-01-08T16:10:11Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 8 Jan 2008, Junio C Hamano wrote:\n> \n> This is caused partly by the breakage in size_only codepath of\n> diff.c::diff_populate_filespec().\n\nOnly partially.\n\nThe more fundamental behaviour (that of git update-index) is caused by \nie_modified() thinking that when DATA_CHANGED is true, it cannot possibly \nneed to call \"ce_modified_check_fs()\":\n\n>From ie_modified():\n\n        /* Immediately after read-tree or update-index --cacheinfo,\n         * the length field is zero.  For other cases the ce_size\n         * should match the SHA1 recorded in the index entry.\n         */\n        if ((changed & DATA_CHANGED) && ce->ce_size != htonl(0))\n                return changed;\n\nand that DATA_CHANGED comes from ce_match_stat_basic() which notices that \nthe size has changed.\n\nSimilarly, I think that the problem with \"diff\" not realizing they might \nbe the same comes from ie_match_stat(), which has a similar problem in not \nrealizing that DATA_CHANGED could possibly still mean that it's the same.\n\nThis patch should fix it, but I suspect we should think hard about that \nchange to ie_modified(), and see what the performance issues are (ie that \ncode has tried to avoid doing the more expensive ce_modified_check_fs() \nfor a reason).\n\nThe change to diff.c is similarly interesting. It is logically wrong to \nuse the worktree_file there (since we have to read the object anyway), but \nsince \"reuse_worktree_file\" is also tied into the whole refresh logic, I \nthink the diff.c change is correct.\n\nI dunno. This is not meant to be applied, it is meant to be thought about.\n\n\t\tLinus\n\n---\n diff.c       |    2 +-\n read-cache.c |    2 ++\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b18c140..9f699b7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1512,7 +1512,7 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int\n \tce = active_cache[pos];\n \tif ((lstat(name, &st) < 0) ||\n \t    !S_ISREG(st.st_mode) || /* careful! */\n-\t    ce_match_stat(ce, &st, 0) ||\n+\t    ce_modified(ce, &st, 0) ||\n \t    hashcmp(sha1, ce->sha1))\n \t\treturn 0;\n \t/* we return 1 only when we can stat, it is a regular file,\ndiff --git a/read-cache.c b/read-cache.c\nindex 7db5588..e1fc880 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -253,12 +253,14 @@ int ie_modified(struct index_state *istate,\n \tif (changed & (MODE_CHANGED | TYPE_CHANGED))\n \t\treturn changed;\n \n+#if 0\n \t/* Immediately after read-tree or update-index --cacheinfo,\n \t * the length field is zero.  For other cases the ce_size\n \t * should match the SHA1 recorded in the index entry.\n \t */\n \tif ((changed & DATA_CHANGED) && ce->ce_size != htonl(0))\n \t\treturn changed;\n+#endif\n \n \tchanged_fs = ce_modified_check_fs(ce, st);\n \tif (changed_fs)\n"},{"id":"64753","messageId":"fm0au5$i65$1@ger.gmane.org","threadId":"11525","inReplyTo":"7vfxx8tt1z.fsf@gitster.siamese.dyndns.org","subject":"Re: An interaction with ce_match_stat_basic() and autocrlf","fromName":"Pēteris Kļaviņš","fromEmail":"klavins@netspace.net.au","sentAt":"2008-01-08T17:12:18Z","receivedAt":"2008-01-08T17:12:18Z","isPatch":false,"sender":{"key":"klavins@netspace.net.au","avatar":"https://gravatar.com/avatar/7bb2403e1c2330c5c199171858cb3b7c1e9780f0cf0dbf44f2468fe4a6a8b079?d=mp&s=160"},"body":"> At this point, the index records a blob with LF line ending,\n> while the work tree file has the same content with CRLF line\n> ending.\n\nI think this needs more than just sleeping on.\n\nThere are two separate problems related to crlf treatment in git that \nmanifest themselves in the quirks you see in the current implementation:\n\n(1) The fact that the index may be misaligned with the work tree. Junio's \nexample demonstrates this well. I have resorted to\n\n$ rm -rf *\n$ git reset --hard\n\nin the past to get a work tree that passes\n\n$ git status\n\nwithout false positives after changing the value of autocrlf.\n\n(2) The fact that repository content may be mangled in an indeterminate way \nbecause of the current work tree <-> repository transformation algorithm. \nWhile criticism in the past has mainly been levelled at not knowing whether \na truly binary file will be correctly determined as such, content can be \nlost in the round trip work tree -> repository -> work tree much more \nsimply:\n\n$ git init\n$ git config core.autocrlf true\n$ echo ab | tr ab \\\\r\\\\n >a.txt\n$ od -t a a.txt\n0000000  cr  nl  nl\n0000003\n$ git add a.txt\n$ git commit\n$ rm a.txt\n$ git reset --hard\n$ od -t a a.txt\n0000000  cr  nl  cr  nl\n0000004\n\nIn summary, it irks me that autocrlf true mode is a second cousin of \nautocrlf false and I think that there *should* be an acceptable \ndeterministic solution to this.\n\nThe solution to (2) seems easier than (1): could the transformation \nalgorithm be made deterministic and changed to something like \"convert all \ncrlf pairs to lf if and only if no singleton cr or lf exist in the file \nbefore conversion\"? If a binary file gets mangled in error, it would be an \neasy transformation with standard tools to get the file back again. If an \notherwise text file has mixed lf and crlf endings, or additional cr or lf \nsprinkled randomly through it, the file is not transformed.\n\nGiven a deterministic transformation algorithm, the solution to (1) boils \ndown to recording for each file in the work tree whether the transformation \nalgorithm was used or not in arriving at the file's current contents, \ntogether with a way of telling git to force the use of the transformation \nalgorithm or not for a particular file. It seems to me the place that this \ninformation *should* be recorded is the index, given that both .git/config \nand .gitattributes can be changed independently of the work tree. Recording \nthe information in the index would mean that both autocrlf true and autocrlf \nfalse clones of the same repository would produce equally valid work trees \nwith no loss of information. I am however not well versed enough in git \ninternals at the moment to know whether this is an acceptable solution or \nnot. \n"},{"id":"64757","messageId":"alpine.LFD.1.00.0801080927370.3148@woody.linux-foundation.org","threadId":"11525","inReplyTo":"fm0au5$i65$1@ger.gmane.org","subject":"Re: An interaction with ce_match_stat_basic() and autocrlf","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-08T17:30:40Z","receivedAt":"2008-01-08T17:30:40Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 8 Jan 2008, Pēteris Kļaviņš wrote:\n> \n> In summary, it irks me that autocrlf true mode is a second cousin of autocrlf\n> false and I think that there *should* be an acceptable deterministic solution\n> to this.\n\nWell, I think the real issue is simply that most the main git developers \ndo development on architectures where CRLF just isn't an issue.\n\nSo it's not that autocrlf is a \"second cousin\", it's that\n\n - CRLF is stupid to begin with, and slightly anathemical to the git \n   worldview of trying to be as exact as possible.\n\n - ..and almost nobody in the git community is actually affected, so \n   people don't even notice when it's an issue.\n\nPeople who actually care and use crlf are probably best off sending in \ntest-cases for particular behaviour they notice. \n\n\t\tLinus\n"},{"id":"64760","messageId":"7vr6gsry6l.fsf@gitster.siamese.dyndns.org","threadId":"11525","inReplyTo":"alpine.LFD.1.00.0801080748080.3148@woody.linux-foundation.org","subject":"Re: An interaction with ce_match_stat_basic() and autocrlf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-08T18:04:34Z","receivedAt":"2008-01-08T18:04:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Tue, 8 Jan 2008, Junio C Hamano wrote:\n>> \n>> This is caused partly by the breakage in size_only codepath of\n>> diff.c::diff_populate_filespec().\n>\n> Only partially.\n\nAgreed.  That's why it is \"just a half of the story\".\n\n> The more fundamental behaviour (that of git update-index) is caused by \n> ie_modified() thinking that when DATA_CHANGED is true, it cannot possibly \n> need to call \"ce_modified_check_fs()\":\n> ...\n> Similarly, I think that the problem with \"diff\" not realizing they might \n> be the same comes from ie_match_stat(), which has a similar problem in not \n> realizing that DATA_CHANGED could possibly still mean that it's the same.\n\nYes, I think your patch to ie_modified() should take care of the\nissue from the diff-files front-end side, which is the right\napproach.  The optimization diffcore_populate_filespec() makes\nwhen asked to do size_only, which predates the addition of\nconvert_to_git(), needs to be updated regardless, though.  The\nsize field in diffcore_filespec is never about on-filesystem\nsize.\n\n> This patch should fix it, but I suspect we should think hard about that \n> change to ie_modified(), and see what the performance issues are (ie that \n> code has tried to avoid doing the more expensive ce_modified_check_fs() \n> for a reason).\n\nI think the reason was I simply avoided doing any unnecessary\noperation that goes to the filesystem.  We did not even have\nthat modified_check_fs() code before the racy-git safety, and\nwhen I added it I do not think I benched it with a real-life\nworkload; the logic there was simply a valid optimization back\nthen.\n\nIt is not anymore.  Addition of convert_to_git() made cached\nstat info essentially ineffective in the sense that:\n\n (1) if a user changes the work tree files in such a way that\n     does not change convert_to_git() output, the index will say\n     \"file contents in external representation has definitely\n     changed, the sizes no longer match\".  We need to actually\n     go to the data to find out that there is no change at the\n     canonical level.\n\n (2) if a user changes the crlf setting (or .gitattributes)\n     without touching the work tree files, the index will say\n     \"unchanged and do not have to compare\".  We need to\n     actually go to the data to find out that they do not match\n     anymore.\n\nThe latter is an opposite issue of what I brought up in this\nthread.  I personally do not want to \"fix\" it --- it means\ndestroying one of the most important optimizations.  The use\ncase is essentially a one-shot operation for a user to \"fix\" a\nbroken crlf setting, and having to re-checkout everything is a\nsmall cost to pay to maintain it.\n\nBut the former is something we should be able to deal with\nsanely.\n"},{"id":"64892","messageId":"7vzlvea0q4.fsf@gitster.siamese.dyndns.org","threadId":"11525","inReplyTo":"alpine.LFD.1.00.0801080748080.3148@woody.linux-foundation.org","subject":"Re: An interaction with ce_match_stat_basic() and autocrlf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-10T02:11:31Z","receivedAt":"2008-01-10T02:11:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> This patch should fix it, but I suspect we should think hard about that \n> change to ie_modified(), and see what the performance issues are (ie that \n> code has tried to avoid doing the more expensive ce_modified_check_fs() \n> for a reason).\n>\n> The change to diff.c is similarly interesting. It is logically wrong to \n> use the worktree_file there (since we have to read the object anyway), but \n> since \"reuse_worktree_file\" is also tied into the whole refresh logic, I \n> think the diff.c change is correct.\n>\n> I dunno. This is not meant to be applied, it is meant to be thought about.\n\nThere are a few cases around the changing value of autocrlf (and\nfilter attributes --- anything that affects convert_to_git() and\nconvert_to_working_tree()).\n\n * The cached stat information matches the work tree, but user\n   changed convert_to_working_tree().  \"git diff\" reports\n   nothing.  The user needs to remove the work tree file and\n   check it out again.\n\n * The cached stat information matches the work tree, but user\n   changed convert_to_git().  Again, diff reports nothing.  The\n   user needs to \"git add\" to cause rehashing.\n\n * The cached stat information does not match.  What the working\n   tree file stores hasn't changed, but convert_to_git() was\n   changed.\n\n   The fact that the working tree \"file\" contents did not change\n   does not have much significance in this case.  What defines\n   the \"contents\" as far as git is concerned is the combination\n   of the working tree file contents _and_ what convert_to_git()\n   does to it.\n\n   Depending on the nature of the change to convert_to_git(),\n   \"git diff-files\" may or may not report real changes in this\n   case.\n\n * The working tree file has changed, and convert_to_git() also\n   has changed.\n\n   Depending on the nature of the change to convert_to_git(),\n   \"git diff\" may or may not report change in this case.  The\n   most extreme case is when unix2dos is run on the working tree\n   file and convert_to_git() is made to strip CR.  The object\n   registered in the index won't change in this case.\n\n   But in practice, the most problematic case also falls into\n   this category.  The user has _real_ changes to the work tree\n   file, but at the same time flipped convert_to_git() to\n   operate differently from before.  Users should not be making\n   such a change, not because of git, but because a commit like\n   that will be impossible to review (and understand three\n   months later while archaeologying).\n\nThe ie_modified() change you suggested will not be hurt by the\nfirst two cases (which I see are one-shot events and re-checkout\nand re-add are good enough solution to them, and I do not want\nthem to hurt the performance for normal use cases).\n\nI originally thought it was a _bug_, but I suspect the false\npositive changes reported by \"git diff\" is even a good thing.\n"}]}