{"thread":{"id":"47884","subject":"Duplicate safecrlf warning for racily clean index entry","startedAt":"2018-02-20T13:42:35Z","lastAt":"2018-02-21T17:20:50Z","messageCount":5,"participants":["Matt McCutchen","Torsten Bögershausen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"339708","messageId":"1519134146.6055.23.camel@mattmccutchen.net","threadId":"47884","inReplyTo":null,"subject":"Duplicate safecrlf warning for racily clean index entry","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2018-02-20T13:42:26Z","receivedAt":"2018-02-20T13:42:35Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"I noticed that if a file subject to a safecrlf warning is added to the\nindex in the same second that it is created, resulting in a \"racily\nclean\" index entry, then a subsequent \"git add\" command prints another\nsafecrlf warning.  I reproduced this on the current \"next\"\n(499d7c4f91).  The procedure:\n\n$ git init\n$ git config core.autocrlf true\n$ echo foo >file1 && git add file1 && git add file1\nwarning: LF will be replaced by CRLF in file1.\nThe file will have its original line endings in your working directory.\nwarning: LF will be replaced by CRLF in file1.\nThe file will have its original line endings in your working directory.\n$ echo bar >file2 && sleep 1 && git add file2 && git add file2\nwarning: LF will be replaced by CRLF in file2.\nThe file will have its original line endings in your working directory.\n\nThis came up when I ran the test suite for Braid on Windows\n(https://github.com/cristibalan/braid/issues/77).\n\nThe phenomenon actually seems to be more general: touching the file\ncauses the next \"git add\" to print a safecrlf warning, suggesting that\nthe warning occurs whenever the index entry is dirty.  One could argue\nthat a new warning is reasonable after touching the file, but it seems\nclear that \"racy cleanliness\" is an implementation detail that\nshouldn't have user-visible nondeterministic effects.\n\nIn either case, if \"git update-index --refresh\" (or \"git status\") is\nrun before \"git add\", then \"git add\" does not print the warning.  On\nthe other hand, if line endings in the working tree file are changed,\nthen git shows the file as having an unstaged change, even though the\ncontent that would be added to the index after CRLF conversion is\nidentical.  So it seems that git remembers the pre-conversion file\ncontent and uses it for \"git update-index --refresh\" and would just\nneed to use it for \"git add\" as well.\n\nThoughts about the proposed change?  Does someone want to work on it or\ngive me a pointer to where to get started?\n\nThanks,\nMatt\n"},{"id":"339797","messageId":"20180221075323.GA18213@tor.lan","threadId":"47884","inReplyTo":"1519134146.6055.23.camel@mattmccutchen.net","subject":"Re: Duplicate safecrlf warning for racily clean index entry","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2018-02-21T07:53:23Z","receivedAt":"2018-02-21T07:53:34Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Feb 20, 2018 at 08:42:26AM -0500, Matt McCutchen wrote:\n> I noticed that if a file subject to a safecrlf warning is added to the\n> index in the same second that it is created, resulting in a \"racily\n> clean\" index entry, then a subsequent \"git add\" command prints another\n> safecrlf warning.  I reproduced this on the current \"next\"\n> (499d7c4f91).  The procedure:\n> \n> $ git init\n> $ git config core.autocrlf true\n> $ echo foo >file1 && git add file1 && git add file1\n> warning: LF will be replaced by CRLF in file1.\n> The file will have its original line endings in your working directory.\n> warning: LF will be replaced by CRLF in file1.\n> The file will have its original line endings in your working directory.\n> $ echo bar >file2 && sleep 1 && git add file2 && git add file2\n> warning: LF will be replaced by CRLF in file2.\n> The file will have its original line endings in your working directory.\n> \n> This came up when I ran the test suite for Braid on Windows\n> (https://github.com/cristibalan/braid/issues/77).\n\nI think a .gitattributes file could/should be used. I'll answer\nthere seperatly.\n\n> \n> The phenomenon actually seems to be more general: touching the file\n> causes the next \"git add\" to print a safecrlf warning, suggesting that\n> the warning occurs whenever the index entry is dirty.  One could argue\n> that a new warning is reasonable after touching the file, but it seems\n> clear that \"racy cleanliness\" is an implementation detail that\n> shouldn't have user-visible nondeterministic effects.\n> \n> In either case, if \"git update-index --refresh\" (or \"git status\") is\n> run before \"git add\", then \"git add\" does not print the warning.  On\n> the other hand, if line endings in the working tree file are changed,\n> then git shows the file as having an unstaged change, even though the\n> content that would be added to the index after CRLF conversion is\n> identical.  So it seems that git remembers the pre-conversion file\n> content and uses it for \"git update-index --refresh\" and would just\n> need to use it for \"git add\" as well.\n> \n> Thoughts about the proposed change?  Does someone want to work on it or\n> give me a pointer to where to get started?\n\nGood analyzes, thanks for that.\n\nI don't hava a pointer, but what should happen ?\n2 warnings for 2 \"git add\" should be OK, I think.\n\n1 warning is part of the optimization, that Git does to handle\nhundrets and thousands of files efficciently.\n\nIs the 1/2 warning  real live problem  ?\n\n> \n> Thanks,\n> Matt\n"},{"id":"339805","messageId":"1519220864.3059.14.camel@mattmccutchen.net","threadId":"47884","inReplyTo":"1519134146.6055.23.camel@mattmccutchen.net","subject":"Re: Duplicate safecrlf warning for racily clean index entry","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2018-02-21T13:47:44Z","receivedAt":"2018-02-21T13:47:55Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Tue, 2018-02-20 at 08:42 -0500, Matt McCutchen wrote:\n> In either case, if \"git update-index --refresh\" (or \"git status\") is\n> run before \"git add\", then \"git add\" does not print the warning.  On\n> the other hand, if line endings in the working tree file are changed,\n> then git shows the file as having an unstaged change, even though the\n> content that would be added to the index after CRLF conversion is\n> identical.  So it seems that git remembers the pre-conversion file\n> content and uses it for \"git update-index --refresh\" and would just\n> need to use it for \"git add\" as well.\n\nOn further testing, this analysis is wrong.  What I was seeing is that\nif the size of the working tree file has changed, git reports an\nunstaged change.  (I suppose that reporting an unstaged change in this\ncase without checking whether the post-conversion content has changed\nmay be an important optimization.)  If the line endings are changed\nwithout changing the size or post-conversion content, then no unstaged\nchange is reported.  It does not appear that git saves the pre-\nconversion content.\n\nThus, if it were possible to create a file that doesn't need a safecrlf\nwarning, add it to the index, and then modify it so that it does need a\nsafecrlf warning without changing the size or post-conversion content,\nwe would have a bug where no warning is shown in the case where \"git\nstatus\" is run before the second \"git add\".  I believe this bug can't\noccur in the particular case of CRLF conversion without other filters\nbecause the file that doesn't need a safecrlf warning has a unique\nminimum (LF) or maximum (CRLF) size, though I presume it could occur\nwith custom filters.  My proposal would then be that \"git add\" should\nnot show a safecrlf warning if the size and post-conversion content\nhaven't changed; it would merely bring \"git add\" to parity with the\npotential bug in the \"git status\" case.\n\nMatt\n"},{"id":"339806","messageId":"1519221471.3059.23.camel@mattmccutchen.net","threadId":"47884","inReplyTo":"20180221075323.GA18213@tor.lan","subject":"Re: Duplicate safecrlf warning for racily clean index entry","fromName":"Matt McCutchen","fromEmail":"matt@mattmccutchen.net","sentAt":"2018-02-21T13:57:51Z","receivedAt":"2018-02-21T13:58:03Z","isPatch":false,"sender":{"key":"matt@mattmccutchen.net","avatar":"https://avatars.githubusercontent.com/u/8885753?v=4"},"body":"On Wed, 2018-02-21 at 08:53 +0100, Torsten Bögershausen wrote:\n> I don't hava a pointer, but what should happen ?\n> 2 warnings for 2 \"git add\" should be OK, I think.\n> \n> 1 warning is part of the optimization, that Git does to handle\n> hundrets and thousands of files efficciently.\n> \n> Is the 1/2 warning  real live problem  ?\n\nAs I've suggested, my opinion is that the nondeterministic second\nwarning can result in significant user confusion and should be avoided.\n (If it were always shown, I'd be less concerned.)  We'll see what the\ndecision-makers think.\n\nMatt\n"},{"id":"339811","messageId":"xmqqy3jmz2c7.fsf@gitster-ct.c.googlers.com","threadId":"47884","inReplyTo":"1519220864.3059.14.camel@mattmccutchen.net","subject":"Re: Duplicate safecrlf warning for racily clean index entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-21T17:20:40Z","receivedAt":"2018-02-21T17:20:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt McCutchen <matt@mattmccutchen.net> writes:\n\n> ... may be an important optimization.)  If the line endings are changed\n> without changing the size or post-conversion content, then no unstaged\n> change is reported.  It does not appear that git saves the pre-\n> conversion content.\n\nCorrect.  The cached-stat information is meant to be compared with\nthe size on the filesystem.  Based on your observation, it seems\nthat what you are seeing is not specific to safe-crlf thing.\n\nIf you reconfigure anything that affects the checkout conversion\ncodepath, including the \"smudge\" filter, an entry in the index that\nused to be up-to-date will still have cached-stat info like\ntimestamp and size that match the on-disk file, even though if you\n_were_ to check it out afresh out of the index, the reconfigured\ncheckout codepath may produce different file contents on-disk.  A\nconsequence of this is that you may cause Git to still say that the\npath is clean, even though it is no longer true.\n\nThere is no single \"right\" solution out of this situation, as it\ndepends on the reason why you made such a reconfiguration in the\nfirst place.\n\n - If the reason is because you found that what is stored in the\n   index is correct but their contents are checked out incorrectly\n   (e.g. both the index and the working tree files end their lines\n   with LF, but you want your working tree files to be converted to\n   CRLF, and you futzed with .gitattributes or core.crlf), then you\n   would want to \"correct\" it by checking them out, bypassing the\n   \"if the cached-stat information says we already have the matching\n   contents on disk, do not write the file out\" optimization.\n\n - If the reason is the other way around, then you would want to\n   \"correct\" the indexed contents by rehashing what you have on\n   disk.  Perhaps a recently added \"git add --renormalize\" is what\n   you are looking for.\n\n\n\n"}]}