{"thread":{"id":"43027","subject":"t0027 racy?","startedAt":"2016-08-08T15:05:15Z","lastAt":"2016-08-26T21:00:22Z","messageCount":45,"participants":["Johannes Schindelin","Jeff King","Torsten Bögershausen","Junio C Hamano","tboegi@web.de","Johannes Sixt","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"293355","messageId":"alpine.DEB.2.20.1608081556280.5786@virtualbox","threadId":"43027","inReplyTo":null,"subject":"t0027 racy?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-08T15:05:07Z","receivedAt":"2016-08-08T15:05:15Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nI remember that you did a ton of work on t0027. Now I see problems, and\nnot only that the entire script now takes a whopping 4 minutes 20 seconds\nto run on my high-end Windows machine.\n\nIt appears that t0027 fails randomly for me, in seemingly random places.\nSometimes all 1388 cases pass. Sometimes \"29 - commit NNO files crlf=true\nattr=auto LF\" fails. Sometimes it is \"24 - commit NNO files crlf=false\nattr=auto LF\". Sometimes it is \"114 - commit NNO files crlf=false\nattr=auto LF\", and sometimes \"111 - commit NNO files attr=auto aeol=lf\ncrlf=false CRLF_mix_LF\".\n\nWhen I run it with -i -v -x --tee, it passes every single time (taking\nover 5 minutes, just to make things worse)...\n\nAny idea about any possible races?\n\nCiao,\nDscho\n"},{"id":"293356","messageId":"20160808152926.mciovipy5qlnqegs@sigill.intra.peff.net","threadId":"43027","inReplyTo":"alpine.DEB.2.20.1608081556280.5786@virtualbox","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-08T15:29:26Z","receivedAt":"2016-08-08T15:29:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2016 at 05:05:07PM +0200, Johannes Schindelin wrote:\n\n> I remember that you did a ton of work on t0027. Now I see problems, and\n> not only that the entire script now takes a whopping 4 minutes 20 seconds\n> to run on my high-end Windows machine.\n> \n> It appears that t0027 fails randomly for me, in seemingly random places.\n> Sometimes all 1388 cases pass. Sometimes \"29 - commit NNO files crlf=true\n> attr=auto LF\" fails. Sometimes it is \"24 - commit NNO files crlf=false\n> attr=auto LF\". Sometimes it is \"114 - commit NNO files crlf=false\n> attr=auto LF\", and sometimes \"111 - commit NNO files attr=auto aeol=lf\n> crlf=false CRLF_mix_LF\".\n> \n> When I run it with -i -v -x --tee, it passes every single time (taking\n> over 5 minutes, just to make things worse)...\n> \n> Any idea about any possible races?\n\nTry:\n\n https://github.com/peff/git/blob/meta/stress\n\nwhich you can run as \"sh /path/to/stress t0027\" in the top-level of your\ngit repository. I got failure within about 30 seconds on t0027 (though 5\nminutes? Yeesh. It runs in 9s on my laptop. I weep for you).\n\nThe verbose output is not very exciting, though:\n\n\texpecting success: \n\t check_warning \"$lfwarn\" ${pfx}_LF.err\n\n\t--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000\n\t+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000\n\t@@ -1 +0,0 @@\n\t-warning: LF will be replaced by CRLF\n\tnot ok 114 - commit NNO files crlf=false attr=auto LF\n\n-Peff\n"},{"id":"293377","messageId":"05fafffb-1065-d240-51dc-a0872fd83c48@web.de","threadId":"43027","inReplyTo":"alpine.DEB.2.20.1608081556280.5786@virtualbox","subject":"Re: t0027 racy?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-08T18:24:21Z","receivedAt":"2016-08-08T18:24:35Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-08-08 17.05, Johannes Schindelin wrote:\n> Hi Torsten,\n> \n> I remember that you did a ton of work on t0027. Now I see problems, and\n> not only that the entire script now takes a whopping 4 minutes 20 seconds\n> to run on my high-end Windows machine.\n> \n> It appears that t0027 fails randomly for me, in seemingly random places.\n> Sometimes all 1388 cases pass. Sometimes \"29 - commit NNO files crlf=true\n> attr=auto LF\" fails. Sometimes it is \"24 - commit NNO files crlf=false\n> attr=auto LF\". Sometimes it is \"114 - commit NNO files crlf=false\n> attr=auto LF\", and sometimes \"111 - commit NNO files attr=auto aeol=lf\n> crlf=false CRLF_mix_LF\".\n> \n> When I run it with -i -v -x --tee, it passes every single time (taking\n> over 5 minutes, just to make things worse)...\n> \n> Any idea about any possible races?\nJust to double-check: I assume that you have this\ncommit ded2444ad8ab8128cae2b91b8efa57ea2dd8c7a5\nAuthor: Torsten Bögershausen \nDate: Mon Apr 25 18:56:27 2016 +0200\n\n t0027: make commit_chk_wrnNNO() reliable\nin your tree ?\n\n\nIs there a special pattern ?\nDid you\na) Update the machine ?\nb) Update Git code base ?\nor both ?\n(Yes, I know that this may be stupid questions)\n\nIs it only the NNO tests that fail ?\nDid they ever pass ?\n(I think I run them some time ago on a Virtual machine running Windows 7)\n\nI see only \"commit NNO files....\" in you report, they belong to\ncheck_warning(), which is called around line 126 in t0027.\n\n\ncheck_warning() does a grep on a file which has been re-directed from stderr.\n\nHow reproducible is the problem ?\nIf you add\nexit 0\nAfter the last \"commit_chk_wrnNNO\" line (line 418),\ndoes\nls -l crlf*.err\ngive you any hint ?\n\n\n\n\n"},{"id":"293389","messageId":"20160808203224.GA28431@tb-raspi","threadId":"43027","inReplyTo":"20160808152926.mciovipy5qlnqegs@sigill.intra.peff.net","subject":"Re: t0027 racy?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-08T20:32:24Z","receivedAt":"2016-08-08T20:33:17Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Mon, Aug 08, 2016 at 11:29:26AM -0400, Jeff King wrote:\n> On Mon, Aug 08, 2016 at 05:05:07PM +0200, Johannes Schindelin wrote:\n> \n> > I remember that you did a ton of work on t0027. Now I see problems, and\n> > not only that the entire script now takes a whopping 4 minutes 20 seconds\n> > to run on my high-end Windows machine.\n> > \n> > It appears that t0027 fails randomly for me, in seemingly random places.\n> > Sometimes all 1388 cases pass. Sometimes \"29 - commit NNO files crlf=true\n> > attr=auto LF\" fails. Sometimes it is \"24 - commit NNO files crlf=false\n> > attr=auto LF\". Sometimes it is \"114 - commit NNO files crlf=false\n> > attr=auto LF\", and sometimes \"111 - commit NNO files attr=auto aeol=lf\n> > crlf=false CRLF_mix_LF\".\n> > \n> > When I run it with -i -v -x --tee, it passes every single time (taking\n> > over 5 minutes, just to make things worse)...\n> > \n> > Any idea about any possible races?\n> \n> Try:\n> \n> https://github.com/peff/git/blob/meta/stress\n> \n> which you can run as \"sh /path/to/stress t0027\" in the top-level of your\n> git repository. I got failure within about 30 seconds on t0027 (though 5\n> minutes? Yeesh. It runs in 9s on my laptop. I weep for you).\n> \n> The verbose output is not very exciting, though:\n> \n> \texpecting success: \n> \t check_warning \"$lfwarn\" ${pfx}_LF.err\n> \n> \t--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000\n> \t+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000\n> \t@@ -1 +0,0 @@\n> \t-warning: LF will be replaced by CRLF\n> \tnot ok 114 - commit NNO files crlf=false attr=auto LF\n\n(I realized that t0027 is not yet self-explaining, I have it on my list)\n\nNNO_attr_auto_aeol_crlf_false_LF means:\n\nNNO: \"Not NOrmalized\" A file had been commited with CRLF in the repo\nattr_auto: .gitattributes has \"* text=auto\"\naeol_eol .gitattributes has \"eol=crlf\"\ncrlf_false git config core.autocrlf = false\nLF We commit a file with LF line endings.\n\nThis should happend:\n- The file is commited \"as is\", with LF line endings.\n- While commiting, git should print the warning\n- \"warning: LF will be replaced by CRLF\" to stderr\n- stderr is piped (redirected) into NNO_attr_auto_aeol_crlf_false_LF.err\n- we grep for \"will be replaced by\" in xx.err and pipe it into xx.err.actual\n\nThe rest is test_cmp, this is what you see.\nThe warning is missing, but should be there:\n\nThe file has LF, and after commit and a new checkout these LF will\nbe convertet into CRLF.\n\nSo why isn't the warning there (but here on my oldish machines)\n"},{"id":"293457","messageId":"20160809065110.GB17777@peff.net","threadId":"43027","inReplyTo":"20160808203224.GA28431@tb-raspi","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-09T06:51:10Z","receivedAt":"2016-08-09T06:51:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 08, 2016 at 08:32:24PM +0000, Torsten Bögershausen wrote:\n\n> > The verbose output is not very exciting, though:\n> > \n> > \texpecting success: \n> > \t check_warning \"$lfwarn\" ${pfx}_LF.err\n> > \n> > \t--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000\n> > \t+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000\n> > \t@@ -1 +0,0 @@\n> > \t-warning: LF will be replaced by CRLF\n> > \tnot ok 114 - commit NNO files crlf=false attr=auto LF\n> [...]\n> The warning is missing, but should be there:\n> \n> The file has LF, and after commit and a new checkout these LF will\n> be convertet into CRLF.\n> \n> So why isn't the warning there (but here on my oldish machines)\n\nTo be clear, the warning _is_ there when I just run t0027 by itself, and\nthe test passes. It's only under heavy load that it isn't. So it's a\nrace condition either in the test script or in git itself.\n\nUsually race conditions like these are due to one of:\n\n - git dying from SIGPIPE before it has a chance to output the command.\n But I don't see any pipes being used in the test script.\n\n - index raciness causing us to avoid reading file content. For\n example, if you do:\n\n echo foo >bar\n git add bar\n\n Then _usually_ \"bar\" and the index will have the same mtime. And\n therefore subsequent commands that need to refresh the index will\n re-read the content of \"bar\", because they cannot tell from the stat\n information if we have the latest version of \"bar\" in the index or\n not (it could have been written after the index update, but in the\n same second).\n\n But on a slow or heavily loaded system (or if you simply get unlucky\n in crossing the boundary to a new second), they'll have different\n mtimes. And therefore git knows it can skip reading the content from\n the filesystem.\n\n So if your test relies on git actually re-converting the file\n content, it would sometimes randomly fail.\n\nThe second one seems plausible, given the history of issues with\nchanging CRLF settings for an existing checkout. I'm not sure if it\nwould be feasible to reset the index completely before each tested\ncommand, but that would probably solve it.\n\n-Peff\n"},{"id":"293459","messageId":"20160809070345.rpdxjvmb6ujeefhv@sigill.intra.peff.net","threadId":"43027","inReplyTo":"20160809065110.GB17777@peff.net","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-09T07:03:45Z","receivedAt":"2016-08-09T07:03:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2016 at 02:51:10AM -0400, Jeff King wrote:\n\n> - index raciness causing us to avoid reading file content. For\n> example, if you do:\n> \n> echo foo >bar\n> git add bar\n> \n> Then _usually_ \"bar\" and the index will have the same mtime. And\n> therefore subsequent commands that need to refresh the index will\n> re-read the content of \"bar\", because they cannot tell from the stat\n> information if we have the latest version of \"bar\" in the index or\n> not (it could have been written after the index update, but in the\n> same second).\n> \n> But on a slow or heavily loaded system (or if you simply get unlucky\n> in crossing the boundary to a new second), they'll have different\n> mtimes. And therefore git knows it can skip reading the content from\n> the filesystem.\n> \n> So if your test relies on git actually re-converting the file\n> content, it would sometimes randomly fail.\n> \n> The second one seems plausible, given the history of issues with\n> changing CRLF settings for an existing checkout. I'm not sure if it\n> would be feasible to reset the index completely before each tested\n> command, but that would probably solve it.\n\nAnd indeed, this seems to fix it for me (at least it has been running in\na 16-way loop for 5 minutes, whereas before it died after 30 seconds or\nso):\n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 2860d2d..9f057ff 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -120,6 +120,7 @@ commit_chk_wrnNNO () {\n \t\tcp $f $fname &&\n \t\tprintf Z >>\"$fname\" &&\n \t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n+\t\ttouch $fname && # ensure index raciness\n \t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n \tdone\n \n\nI'm not sure if there is a more elegant solution, though (for instance,\nwhy not collect the output from \"git add\", which should have the same\nwarning, I would think).\n\n-Peff\n"},{"id":"293471","messageId":"alpine.DEB.2.20.1608091319100.5786@virtualbox","threadId":"43027","inReplyTo":"05fafffb-1065-d240-51dc-a0872fd83c48@web.de","subject":"Re: t0027 racy?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-09T11:25:16Z","receivedAt":"2016-08-09T11:25:26Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Mon, 8 Aug 2016, Torsten Bögershausen wrote:\n\n> On 2016-08-08 17.05, Johannes Schindelin wrote:\n> > \n> > I remember that you did a ton of work on t0027. Now I see problems,\n> > and not only that the entire script now takes a whopping 4 minutes 20\n> > seconds to run on my high-end Windows machine.\n> > \n> > It appears that t0027 fails randomly for me, in seemingly random\n> > places. Sometimes all 1388 cases pass. Sometimes \"29 - commit NNO\n> > files crlf=true attr=auto LF\" fails. Sometimes it is \"24 - commit NNO\n> > files crlf=false attr=auto LF\". Sometimes it is \"114 - commit NNO\n> > files crlf=false attr=auto LF\", and sometimes \"111 - commit NNO files\n> > attr=auto aeol=lf crlf=false CRLF_mix_LF\".\n> > \n> > When I run it with -i -v -x --tee, it passes every single time (taking\n> > over 5 minutes, just to make things worse)...\n> > \n> > Any idea about any possible races?\n>\n> Just to double-check: I assume that you have this\n> commit ded2444ad8ab8128cae2b91b8efa57ea2dd8c7a5\n> Author: Torsten Bögershausen \n> Date: Mon Apr 25 18:56:27 2016 +0200\n> \n> t0027: make commit_chk_wrnNNO() reliable\n> in your tree ?\n\nI tested this with multiple branches, but yes, the one I tested most was\nthe shears/pu branch of git-for-windows/git (which has all\nWindows-specific patches of our master branch rebased on top of pu). I\nalso tested with the pu branch as of yesterday.\n\n> Is there a special pattern ?\n\nNo. Just \"make -j15 DEVELOPER=1 -k test\".\n\n> Did you\n> a) Update the machine ?\n\nYep, it's up-to-date. Windows 10 Anniversary Update.\n\n> b) Update Git code base ?\n\nAs I said, several.\n\n> Is it only the NNO tests that fail ?\n\nAs I said, the failures are random, I just picked the 4 most recent ones.\n\n> Did they ever pass ?\n\nAs I said, if I run with -i -v -x --tee, everything passes.\n\n> I see only \"commit NNO files....\" in you report, they belong to\n> check_warning(), which is called around line 126 in t0027.\n\nI believe this is true. Some race, probably, leading to the commit *not*\nrefreshing the files. Or some such, this is just a guess on my side.\n\n> How reproducible is the problem ?\n\nNot very. That is, about half of the time t0027 passes even *without* -i\n-v -x --tee. And when it fails, it is anybody's guess which case fails.\n\n> If you add\n> exit 0\n> After the last \"commit_chk_wrnNNO\" line (line 418),\n> does\n> ls -l crlf*.err\n> give you any hint ?\n\nNo. It simply does not contain that warning that is expected.\n\nCiao,\nDscho"},{"id":"293474","messageId":"alpine.DEB.2.20.1608091325430.5786@virtualbox","threadId":"43027","inReplyTo":"20160809070345.rpdxjvmb6ujeefhv@sigill.intra.peff.net","subject":"Re: t0027 racy?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-09T11:27:35Z","receivedAt":"2016-08-09T11:28:12Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 9 Aug 2016, Jeff King wrote:\n\n> And indeed, this seems to fix it for me (at least it has been running in\n> a 16-way loop for 5 minutes, whereas before it died after 30 seconds or\n> so):\n> \n> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n> index 2860d2d..9f057ff 100755\n> --- a/t/t0027-auto-crlf.sh\n> +++ b/t/t0027-auto-crlf.sh\n> @@ -120,6 +120,7 @@ commit_chk_wrnNNO () {\n> \t\tcp $f $fname &&\n> \t\tprintf Z >>\"$fname\" &&\n> \t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n> +\t\ttouch $fname && # ensure index raciness\n> \t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n> \tdone\n\nI will test this patch as soon as my poor machine has less work.\n\n> I'm not sure if there is a more elegant solution, though (for instance,\n> why not collect the output from \"git add\", which should have the same\n> warning, I would think).\n\nThat should work, too. I am a bit overwhelmed with t0027, though, I do not\nreally understand what is going on. It tests *so many* things, and in no\ntest case is it clear to me what it tests and what it expects let alone\nwhy those expectations make sense.\n\nCiao,\nDscho\n"},{"id":"293475","messageId":"alpine.DEB.2.20.1608091329480.5786@virtualbox","threadId":"43027","inReplyTo":"20160808152926.mciovipy5qlnqegs@sigill.intra.peff.net","subject":"Re: t0027 racy?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-09T11:33:04Z","receivedAt":"2016-08-09T11:33:19Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 8 Aug 2016, Jeff King wrote:\n\n> I got failure within about 30 seconds on t0027 (though 5 minutes? Yeesh.\n> It runs in 9s on my laptop. I weep for you).\n\nYep. That is the price I (and all other Git for Windows developers) pay\nfor the decision to implement Git's entire test suite in Shell script,\nincluding expensive tests with over a 1,000 test cases such as t0027.\n\nCiao,\nDscho\n"},{"id":"293476","messageId":"20160809113337.GA1143@tb-raspi","threadId":"43027","inReplyTo":"20160809065110.GB17777@peff.net","subject":"Re: t0027 racy?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-09T11:33:37Z","receivedAt":"2016-08-09T11:33:54Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 09, 2016 at 02:51:10AM -0400, Jeff King wrote:\n> On Mon, Aug 08, 2016 at 08:32:24PM +0000, Torsten Bögershausen wrote:\n> \n> > > The verbose output is not very exciting, though:\n> > > \n> > > \texpecting success: \n> > > \t check_warning \"$lfwarn\" ${pfx}_LF.err\n> > > \n> > > \t--- NNO_attr_auto_aeol_crlf_false_LF.err.expect 2016-08-08 15:26:37.061701392 +0000\n> > > \t+++ NNO_attr_auto_aeol_crlf_false_LF.err.actual 2016-08-08 15:26:37.061701392 +0000\n> > > \t@@ -1 +0,0 @@\n> > > \t-warning: LF will be replaced by CRLF\n> > > \tnot ok 114 - commit NNO files crlf=false attr=auto LF\n> > [...]\n> > The warning is missing, but should be there:\n> > \n> > The file has LF, and after commit and a new checkout these LF will\n> > be convertet into CRLF.\n> > \n> > So why isn't the warning there (but here on my oldish machines)\n> \n> To be clear, the warning _is_ there when I just run t0027 by itself, and\n> the test passes. It's only under heavy load that it isn't. So it's a\n> race condition either in the test script or in git itself.\n> \n> Usually race conditions like these are due to one of:\n> \n> - git dying from SIGPIPE before it has a chance to output the command.\n> But I don't see any pipes being used in the test script.\n> \n> - index raciness causing us to avoid reading file content. For\n> example, if you do:\n> \n> echo foo >bar\n> git add bar\n> \n> Then _usually_ \"bar\" and the index will have the same mtime. And\n> therefore subsequent commands that need to refresh the index will\n> re-read the content of \"bar\", because they cannot tell from the stat\n> information if we have the latest version of \"bar\" in the index or\n> not (it could have been written after the index update, but in the\n> same second).\n> \n> But on a slow or heavily loaded system (or if you simply get unlucky\n> in crossing the boundary to a new second), they'll have different\n> mtimes. And therefore git knows it can skip reading the content from\n> the filesystem.\n> \n> So if your test relies on git actually re-converting the file\n> content, it would sometimes randomly fail.\n> \n> The second one seems plausible, given the history of issues with\n> changing CRLF settings for an existing checkout. I'm not sure if it\n> would be feasible to reset the index completely before each tested\n> command, but that would probably solve it.\nThe content of the file has been changed (we appended the letter 'Z' to it,\nso even if mtime is the same, st.st_size should differ.\nAnd it seems as if the commit is triggered, see below.\n> \n\n(I got the stress script working; no I can reproduce it nicely)\n\nIs it possible to vote for a 3rd option ?\nI added some more warnings, to have always some output in stderr. \n\nThis is the good case, this test case passed:\ncat NNO_attr_auto_aeol_crlf_true_LF.err \n\nwarning: check_safe_crlf in NNO_attr_auto_aeol_crlf_true_LF.txt 2\nwarning: LF was here in NNO_attr_auto_aeol_crlf_true_LF.txt..\nwarning: LF will be replaced by CRLF in NNO_attr_auto_aeol_crlf_true_LF.txt.\nThe file will have its original line endings in your working directory.\nwarning: check_safe_crlf in NNO_attr__aeol_crlf_true_CRLF.txt 0\nwarning: check_safe_crlf in NNO_attr__aeol_crlf_true_CRLF_mix_LF.txt 0\nwarning: check_safe_crlf in NNO_attr__aeol_crlf_true_LF.txt 0\nwarning: check_safe_crlf in NNO_attr__aeol_lf_true_CRLF.txt 0\nwarning: check_safe_crlf in NNO_attr__aeol_lf_true_CRLF_mix_LF.txt 0\nwarning: check_safe_crlf in NNO_attr__aeol_lf_true_LF.txt 0\nwarning: check_safe_crlf in NNO_attr_auto_aeol_lf_true_LF.txt 0\n[master 2decee0] commit_NNO_attr_auto_aeol_crlf_true_LF.txt\n Author: A U Thor \n 1 file changed, 2 insertions(+), 2 deletions(-)\n---------------------------\nAnd this one failed, the same code base, but a different file:\ncat commit_NNO_attr_auto_aeol_crlf_input_LF.\n\n[master ce2045a] commit_NNO_attr_auto_aeol_crlf_input_LF.txt\n Author: A U Thor \n 1 file changed, 2 insertions(+), 2 deletions(-)\n\n---------------------------\nBoth had been generated with a patched convert.c:\ngit diff convert.c\nindex 67d69b5..ae64a58 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -191,6 +191,7 @@ static enum eol output_eol(enum crlf_action crlf_action)\n static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n struct text_stat *stats, enum safe_crlf checksafe)\n {\n+ warning(\"check_safe_crlf in %s %d\", path, (int)checksafe);\n if (!checksafe)\n return;\n \n@@ -210,6 +211,7 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n * CRLFs would be added by checkout:\n * check if we have \"naked\" LFs\n */\n+ warning(\"LF was here in %s..\", path);\n if (stats->lonelf) {\n if (checksafe == SAFE_CRLF_WARN)\n warning(\"LF will be replaced by CRLF in %s.\\nThe\n\n---------------------\n\nIn the failing case, there is no warning at all printed,\neven if I would have expected the list of files to be printed.\n\nThe commit has been done, I think.\n\nSee t0027, around line 124:\n\t\tcp $f $fname &&\n\t\tprintf Z >>\"$fname\" &&\n\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n\nBoth stdout and stderr are piped into the .err file.\nThe content of stdout seems to be there always.\nThe content of stderr seems to be there most of the time.\nWhen the test case fails, the content of stderr seems to be completely missing.\n\nReverting f4c3edc0b156362a92bf9de4f0ec794e90a757f does not fix the problem\nReverting f4c3edc0b15 and 3b331e9267146961466 does not fix the problem.\n"},{"id":"293478","messageId":"20160809113821.5p36pksfmwrhuylv@sigill.intra.peff.net","threadId":"43027","inReplyTo":"alpine.DEB.2.20.1608091329480.5786@virtualbox","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-09T11:38:21Z","receivedAt":"2016-08-09T11:38:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2016 at 01:33:04PM +0200, Johannes Schindelin wrote:\n\n> On Mon, 8 Aug 2016, Jeff King wrote:\n> \n> > I got failure within about 30 seconds on t0027 (though 5 minutes? Yeesh.\n> > It runs in 9s on my laptop. I weep for you).\n> \n> Yep. That is the price I (and all other Git for Windows developers) pay\n> for the decision to implement Git's entire test suite in Shell script,\n> including expensive tests with over a 1,000 test cases such as t0027.\n\nTo be fair, it does not run unless you set GIT_TEST_LONG. Even _I_ don't\ndo that by default.\n\n-Peff\n"},{"id":"293481","messageId":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","threadId":"43027","inReplyTo":"20160809113337.GA1143@tb-raspi","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-09T11:49:38Z","receivedAt":"2016-08-09T11:49:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2016 at 11:33:37AM +0000, Torsten Bögershausen wrote:\n\n> > The second one seems plausible, given the history of issues with\n> > changing CRLF settings for an existing checkout. I'm not sure if it\n> > would be feasible to reset the index completely before each tested\n> > command, but that would probably solve it.\n> The content of the file has been changed (we appended the letter 'Z' to it,\n> so even if mtime is the same, st.st_size should differ.\n> And it seems as if the commit is triggered, see below.\n\nI don't think I made myself clear. It's not a question of whether there\nis something to commit. It's that when git asks the index \"what is the\nsha1 of the content at this path?\", the index may be able to answer\ndirectly (the file is up-to-date, so we return the index value), or it\nmay have to go to the filesystem and read the file content. It is this\nlatter which triggers convert_to_git(), which is what generates the\nmessage in question.\n\nFor a more stripped-down example, try:\n\n git add foo\n git commit -m msg\n\nversus:\n\n git add foo\n sleep 1\n git commit -m msg\n\nIn the latter case, we should not generally need convert_to_git() in the\n\"commit\" step. It was already done by \"git add\", and we reuse the cached\nresult.\n\nWhereas in the first one, we may run into the racy-index problem and\nhave to re-read the file to be on the safe side.\n\n-Peff\n"},{"id":"293485","messageId":"20160809125958.GA1501@tb-raspi","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"Re: t0027 racy?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-09T12:59:58Z","receivedAt":"2016-08-09T13:00:17Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Tue, Aug 09, 2016 at 07:49:38AM -0400, Jeff King wrote:\n> On Tue, Aug 09, 2016 at 11:33:37AM +0000, Torsten Bögershausen wrote:\n> \n> > > The second one seems plausible, given the history of issues with\n> > > changing CRLF settings for an existing checkout. I'm not sure if it\n> > > would be feasible to reset the index completely before each tested\n> > > command, but that would probably solve it.\n> > The content of the file has been changed (we appended the letter 'Z' to it,\n> > so even if mtime is the same, st.st_size should differ.\n> > And it seems as if the commit is triggered, see below.\n> \n> I don't think I made myself clear. It's not a question of whether there\n> is something to commit. It's that when git asks the index \"what is the\n> sha1 of the content at this path?\", the index may be able to answer\n> directly (the file is up-to-date, so we return the index value), or it\n> may have to go to the filesystem and read the file content. It is this\n> latter which triggers convert_to_git(), which is what generates the\n> message in question.\n> \n> For a more stripped-down example, try:\n> \n> git add foo\n> git commit -m msg\n> \n> versus:\n> \n> git add foo\n> sleep 1\n> git commit -m msg\n> \n> In the latter case, we should not generally need convert_to_git() in the\n> \"commit\" step. It was already done by \"git add\", and we reuse the cached\n> result.\n> \n> Whereas in the first one, we may run into the racy-index problem and\n> have to re-read the file to be on the safe side.\n> \n> -Peff\n\nThanks for the explanation, so there are 2 chances for a race.\n\nI assume that the suggested \"touch\" will fix race#2 in most cases.\nIn my understanding, the change of the file size will be more reliable:\n \n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 2860d2d..9933a9b 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -120,6 +120,7 @@ commit_chk_wrnNNO () {\n cp $f $fname &&\n printf Z >>\"$fname\" &&\n git -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n+ printf Z >>\"$fname\" &&\n git -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n done\n-------------------\nDoes anybody agree ?\nAnd, by the way, the convert warning may be issued twice, once in\n\"git add\" and once in \"git commit\".\n"},{"id":"293486","messageId":"20160809132744.kjzmkgt2qiugeolj@sigill.intra.peff.net","threadId":"43027","inReplyTo":"20160809125958.GA1501@tb-raspi","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-09T13:27:45Z","receivedAt":"2016-08-09T13:27:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 09, 2016 at 12:59:58PM +0000, Torsten Bögershausen wrote:\n\n> Thanks for the explanation, so there are 2 chances for a race.\n> \n> I assume that the suggested \"touch\" will fix race#2 in most cases.\n> In my understanding, the change of the file size will be more reliable:\n> \n> \n> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n> index 2860d2d..9933a9b 100755\n> --- a/t/t0027-auto-crlf.sh\n> +++ b/t/t0027-auto-crlf.sh\n> @@ -120,6 +120,7 @@ commit_chk_wrnNNO () {\n> cp $f $fname &&\n> printf Z >>\"$fname\" &&\n> git -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n> + printf Z >>\"$fname\" &&\n> git -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n> done\n> -------------------\n> Does anybody agree ?\n\nI think the mtime change is reliable. We know that the mtime on the file\nwill be greater than or equal to the index mtime (because it happened\nafterwards), so git will always look at the on-disk contents.\n\nWith your change, \"git commit\" will also always re-read the file from\ndisk, because it actually has new content (and you provide the filename\non the command line, so it is stage-and-commit, not just\n\"commit-the-index\").\n\nSo either is fine.\n\n> And, by the way, the convert warning may be issued twice, once in\n> \"git add\" and once in \"git commit\".\n\nYes, but you only save it from \"git commit\", so we can ignore what\nhappens from \"add\" here. But that's why I wondered if:\n\n git -c core.autocrlf=$crlf add $fname >\"${pfx}_$f.err\" 2>&1\n\nwould make more sense. We _know_ that we have to do convert_to_git() in\nthat step because the content is changed. And then you can ignore the\nwarnings from \"git commit\" (which are racy), or you can simply commit as\na whole later, as some other loops do.\n\nBut like Dscho, I do not actually understand what this test is checking.\nThe function is called commit_chk_wrnNNO(), so perhaps you really are\ninterested in what \"commit\" has to say. But IMHO that is not an\ninteresting test. We know that if it has to read the content from disk,\nit will call convert_to_git(), which is the exact same code path used by\n\"git add\". So I do not understand what it is accomplishing to make a\ncommit at all here.\n\n-Peff\n"},{"id":"293540","messageId":"20160809212802.GA4132@tb-raspi","threadId":"43027","inReplyTo":"20160809132744.kjzmkgt2qiugeolj@sigill.intra.peff.net","subject":"Re: t0027 racy?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-09T21:28:02Z","receivedAt":"2016-08-09T21:28:23Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"> git -c core.autocrlf=$crlf add $fname >\"${pfx}_$f.err\" 2>&1\n> \n> would make more sense. We _know_ that we have to do convert_to_git() in\n> that step because the content is changed. And then you can ignore the\n> warnings from \"git commit\" (which are racy), or you can simply commit as\n> a whole later, as some other loops do.\n> \n> But like Dscho, I do not actually understand what this test is checking.\n> The function is called commit_chk_wrnNNO(), so perhaps you really are\n> interested in what \"commit\" has to say. But IMHO that is not an\n> interesting test. We know that if it has to read the content from disk,\n> it will call convert_to_git(), which is the exact same code path used by\n> \"git add\". So I do not understand what it is accomplishing to make a\n> commit at all here.\n\nIt seems as if the test has been written without understanding the raciness.\nIt should commit files with different line endings on top of\na file with mixed line endings.\nThe warning should be checked (and here \"git add\" can be used,\nor the file can be commited directly).\nI'm not sure why the test ended up in doing both.\n\nHowever, doing it the right way triggers a bug in convert.c,\n(some warnings are missing, so I need some days to come up\nwith a proper patch)\n\nThanks for reporting & digging.\n"},{"id":"293606","messageId":"alpine.DEB.2.20.1608101423160.4924@virtualbox","threadId":"43027","inReplyTo":"20160809212802.GA4132@tb-raspi","subject":"Re: t0027 racy?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-10T12:28:30Z","receivedAt":"2016-08-10T19:16:26Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Tue, 9 Aug 2016, Torsten Bögershausen wrote:\n\n> > git -c core.autocrlf=$crlf add $fname >\"${pfx}_$f.err\" 2>&1\n> > \n> > would make more sense. We _know_ that we have to do convert_to_git() in\n> > that step because the content is changed. And then you can ignore the\n> > warnings from \"git commit\" (which are racy), or you can simply commit as\n> > a whole later, as some other loops do.\n> > \n> > But like Dscho, I do not actually understand what this test is checking.\n> > The function is called commit_chk_wrnNNO(), so perhaps you really are\n> > interested in what \"commit\" has to say. But IMHO that is not an\n> > interesting test. We know that if it has to read the content from disk,\n> > it will call convert_to_git(), which is the exact same code path used by\n> > \"git add\". So I do not understand what it is accomplishing to make a\n> > commit at all here.\n> \n> It seems as if the test has been written without understanding the raciness.\n> It should commit files with different line endings on top of\n> a file with mixed line endings.\n> The warning should be checked (and here \"git add\" can be used,\n> or the file can be commited directly).\n> I'm not sure why the test ended up in doing both.\n> \n> However, doing it the right way triggers a bug in convert.c,\n> (some warnings are missing, so I need some days to come up\n> with a proper patch)\n\nFWIW I would strongly prefer to use the warning of `git add` and not even\nbother with `git commit`. What we are interested in is the warning\nmessage, generated by convert_to_git(). Not using the first one and\ntriggering a second one merely adds unnecessary churn that increases the\nCO2 budget of running the test.\n\nOn that matter, I wonder whether there would be a chance to revamp t0027\nin a major way, with the following goals:\n\n- to make it very obvious to the casual reader what is being tested\n\n- to combine Git invocations when possible, e.g. running one big `git add`\n on a couple of files and then verify the relevant parts of the output\n\n- dramatically decreasing the time required to run the test, without\n sacrificing correctness (I would wager a bet that not only a few of\n those 1388 test cases essentially exercise identical code paths)\n\nCiao,\nDscho"},{"id":"293792","messageId":"20160811185812.GA18434@tb-raspi","threadId":"43027","inReplyTo":"alpine.DEB.2.20.1608101423160.4924@virtualbox","subject":"Re: t0027 racy?","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-11T18:58:12Z","receivedAt":"2016-08-11T18:58:27Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"[]\n> FWIW I would strongly prefer to use the warning of `git add` and not even\n> bother with `git commit`. What we are interested in is the warning\n> message, generated by convert_to_git(). \nThe commit is needed, because we check the content of the commit later.\n> Not using the first one and\n[]\n> \n> On that matter, I wonder whether there would be a chance to revamp t0027\n> in a major way, with the following goals:\n> \n> - to make it very obvious to the casual reader what is being tested\n> \n> - to combine Git invocations when possible, e.g. running one big `git add`\n> on a couple of files and then verify the relevant parts of the output\n> \n> - dramatically decreasing the time required to run the test, without\n> sacrificing correctness (I would wager a bet that not only a few of\n> those 1388 test cases essentially exercise identical code paths)\nGood ideas, I will work on a series that fixes bugs first, and then we\ncan see if there is room for optimization.\n\nWhat do you think about this as a starting point,\nmore things will follow.\nI like to here comments about the commit msg first ;-)\n\ncommit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e\nAuthor: Torsten Bögershausen \nDate: Thu Aug 11 18:47:29 2016 +0200\n\n t0027: Correct raciness in NNO test\n \n When a non-reversible CRLF conversion is done in \"git add\",\n a warning is printed on stderr.\n \n The commit_chk_wrnNNO() function in t0027 was written to test this,\n but did the wrong thing: Instead of looking at the warning\n from \"git add\", it looked at the warning from \"git commit\".\n \n Correct this and replace the commit for each and every file with a commit\n of all files in one go.\n \n The function commit_chk_wrnNNO() will to be renamed in a separate commit.\n Thanks to Jeff King for analizing t0027.\n Reporyed-By: Johannes Schindelin \n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 2860d2d..6e44382 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {\n \t\tfname=${pfx}_$f.txt &&\n \t\tcp $f $fname &&\n \t\tprintf Z >>\"$fname\" &&\n-\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n-\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n+\t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n \tdone\n \n \ttest_expect_success \"commit NNO files crlf=$crlf attr=$attr LF\" '\n@@ -394,11 +393,10 @@ test_expect_success 'commit files attr=crlf' '\n \n # attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"\" \"\" true \"\" \"\" \"\" \"\" \"\"\n commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n-\n-commit_chk_wrnNNO \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"auto\" \"\" false \"\" \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"auto\" \"\" true \"\" \"\" \"\" \"\" \"\"\n commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n for crlf in true false input\n do\n@@ -408,7 +406,7 @@ do\n \tcommit_chk_wrnNNO \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \tcommit_chk_wrnNNO \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n \tcommit_chk_wrnNNO auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n+\tcommit_chk_wrnNNO auto crlf \t$crlf \"\" \"\" \"\" \"\" \"\"\n \tcommit_chk_wrnNNO text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \tcommit_chk_wrnNNO text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n done\n@@ -417,7 +415,8 @@ commit_chk_wrnNNO \"text\" \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\n commit_chk_wrnNNO \"text\" \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n commit_chk_wrnNNO \"text\" \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \n-test_expect_success 'create files cleanup' '\n+test_expect_success 'commit NNO and cleanup' '\n+\tgit commit -m \"commit files on top of NNO\" &&\n \trm -f *.txt &&\n \tgit -c core.autocrlf=false reset --hard\n '\n"},{"id":"298465","messageId":"xmqqa8gj14mj.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160811185812.GA18434@tb-raspi","subject":"Re: t0027 racy?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-11T19:34:44Z","receivedAt":"2016-08-11T20:42:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Good ideas, I will work on a series that fixes bugs first, and then we\n> can see if there is room for optimization.\n>\n> What do you think about this as a starting point, more things will\n> follow.\n> I like to here comments about the commit msg first ;-)\n\nThroughout t0027 there is no mention on what NNO stands for. Are\nthey about operations that result in un-normalized index entries?\n\n> commit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e\n> Author: Torsten Bögershausen \n> Date: Thu Aug 11 18:47:29 2016 +0200\n>\n> t0027: Correct raciness in NNO test\n> \n> When a non-reversible CRLF conversion is done in \"git add\",\n> a warning is printed on stderr.\n> \n> The commit_chk_wrnNNO() function in t0027 was written to test this,\n> but did the wrong thing: Instead of looking at the warning\n> from \"git add\", it looked at the warning from \"git commit\".\n> \n> Correct this and replace the commit for each and every file with a commit\n> of all files in one go.\n> \n> The function commit_chk_wrnNNO() will to be renamed in a separate commit.\n> Thanks to Jeff King for analizing t0027.\n> Reporyed-By: Johannes Schindelin \n\nReporyed? \n\nAn obligatory \"comments-about-the-commit-msg\";-)\n"},{"id":"298830","messageId":"20160812072417.huwa4e2tvtcew67k@sigill.intra.peff.net","threadId":"43027","inReplyTo":"20160811185812.GA18434@tb-raspi","subject":"Re: t0027 racy?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-12T07:24:17Z","receivedAt":"2016-08-12T07:25:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 11, 2016 at 06:58:12PM +0000, Torsten Bögershausen wrote:\n\n> commit 3754404d3d1ea4a0cbbed4986cc4ac1b5fe6b66e\n> Author: Torsten Bögershausen \n> Date: Thu Aug 11 18:47:29 2016 +0200\n> \n> t0027: Correct raciness in NNO test\n> \n> When a non-reversible CRLF conversion is done in \"git add\",\n> a warning is printed on stderr.\n> \n> The commit_chk_wrnNNO() function in t0027 was written to test this,\n> but did the wrong thing: Instead of looking at the warning\n> from \"git add\", it looked at the warning from \"git commit\".\n\nMaybe add:\n\n This is racy because \"git commit\" may not have to do CRLF conversion\n at all if it can use the sha1 value from the index (which depends on\n whether \"add\" and \"commit\" run in a single second).\n\n> Thanks to Jeff King for analizing t0027.\n> Reporyed-By: Johannes Schindelin \n\ns/analizing/analyzing/; s/Reporyed/Reported/\n\n> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n> index 2860d2d..6e44382 100755\n> --- a/t/t0027-auto-crlf.sh\n> +++ b/t/t0027-auto-crlf.sh\n> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {\n> \t\tfname=${pfx}_$f.txt &&\n> \t\tcp $f $fname &&\n> \t\tprintf Z >>\"$fname\" &&\n> -\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n> -\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n> +\t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n> \tdone\n\nSo this is the meat of the fix.\n\n> @@ -394,11 +393,10 @@ test_expect_success 'commit files attr=crlf' '\n> \n> # attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n> commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n> -commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"\" \"\" true \"\" \"\" \"\" \"\" \"\"\n> commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n> -\n> -commit_chk_wrnNNO \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n> -commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"auto\" \"\" false \"\" \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"auto\" \"\" true \"\" \"\" \"\" \"\" \"\"\n> commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n\nI am not sure I understand why this reordering is necessary, but I guess\nit's to group like things together in a single commit? Might be worth a\nmention in the commit message.\n\n-Peff\n"},{"id":"298865","messageId":"1471020658-20708-1-git-send-email-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 0/2] Fix conversion warnings","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-12T16:50:58Z","receivedAt":"2016-08-12T16:41:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nconversion warnings in convert.c and t0027\n- Fix a raciness in t0027\n- Fix an older problem with the conversion warnings when a file\n has CRLF in the index, and is now commited with LF.\n- Do not yet rename NNO to something different.\n NNO means Not NOrmalized in the index.\n The files are changed and content with different line endings\n are added on top of the existing version.\n A replacement for commit_chk_wrnNNO will be send after these bugfixes.\n The term commit_chk_wrnNNO could be changed to\n NNO_in_repo_add_file_check_warning.\n Would NNO_addX__chk_wrn be more readable ?\n Or CRLFindex_add_wrn ?\n\nChanges against v0:\n- Added raciness comment from Peff, and removed the removal of a line.\nTorsten Bögershausen (2):\n t0027: Correct raciness in NNO test\n convert: missing `LF will be replaced by CRLF\n\n convert.c | 97 ++++++++++++++++++++++++++++++----------------------\n t/t0027-auto-crlf.sh | 10 +++---\n 2 files changed, 62 insertions(+), 45 deletions(-)\n\n-- \n2.0.0.rc1.6318.g0c2c796\n\n"},{"id":"298866","messageId":"1471020664-20784-1-git-send-email-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 2/2] convert: missing `LF will be replaced by CRLF","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-12T16:51:04Z","receivedAt":"2016-08-12T16:41:56Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen the \"new safer autocrlf-handling\" was introduced in c4805393, the logic\naround check_safe_crlf() should have been changed:\nOnce a file has been commited with CRLF (and core.autocrlf is true),\nline endings are no longer converted by \"git add\" or \"git checkout\".\nThe line endings will not normalized by Git.\nThe user may run e.g. dos2unix and commit the file.\n\nFactor out will_convert_lf_to_crlf() which returns when LF are converted\ninto CRLF at checkout.\n\nUpdate the logic around check_safe_crlf() and do the possible CRLF-LF\nconversion in \"git add\".\nSimulate the checkout (and a possible LF-CRLF conversion) with help of\nwill_convert_lf_to_crlf().\n---\n convert.c | 97 ++++++++++++++++++++++++++++++----------------------\n t/t0027-auto-crlf.sh | 6 ++--\n 2 files changed, 60 insertions(+), 43 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 67d69b5..077f5e6 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)\n }\n \n static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n- struct text_stat *stats, enum safe_crlf checksafe)\n+\t\t\t struct text_stat *old_stats, struct text_stat *new_stats,\n+\t\t\t enum safe_crlf checksafe)\n {\n-\tif (!checksafe)\n-\t\treturn;\n-\n-\tif (output_eol(crlf_action) == EOL_LF) {\n+\tif (old_stats->crlf && !new_stats->crlf ) {\n \t\t/*\n-\t\t * CRLFs would not be restored by checkout:\n-\t\t * check if we'd remove CRLFs\n+\t\t * CRLFs would not be restored by checkout\n \t\t */\n-\t\tif (stats->crlf) {\n-\t\t\tif (checksafe == SAFE_CRLF_WARN)\n-\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n-\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n-\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n-\t\t}\n-\t} else if (output_eol(crlf_action) == EOL_CRLF) {\n+\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\twarning(\"CRLF will be replaced by LF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n+\t\telse /* i.e. SAFE_CRLF_FAIL */\n+\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n+\t} else if (old_stats->lonelf && !new_stats->lonelf ) {\n \t\t/*\n-\t\t * CRLFs would be added by checkout:\n-\t\t * check if we have \"naked\" LFs\n+\t\t * CRLFs would be added by checkout\n \t\t */\n-\t\tif (stats->lonelf) {\n-\t\t\tif (checksafe == SAFE_CRLF_WARN)\n-\t\t\t\twarning(\"LF will be replaced by CRLF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n-\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n-\t\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n-\t\t}\n+\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\twarning(\"LF will be replaced by CRLF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n+\t\telse /* i.e. SAFE_CRLF_FAIL */\n+\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n \t}\n }\n \n@@ -233,12 +225,35 @@ static int has_cr_in_index(const char *path)\n \treturn has_cr;\n }\n \n+static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n+\t\t\t\t enum crlf_action crlf_action)\n+{\n+\tif (output_eol(crlf_action) != EOL_CRLF)\n+\t\treturn 0;\n+\t/* No \"naked\" LF? Nothing to convert, regardless. */\n+\tif (!stats->lonelf)\n+\t\treturn 0;\n+\n+\tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {\n+\t\t/* If we have any CR or CRLF line endings, we do not touch it */\n+\t\t/* This is the new safer autocrlf-handling */\n+\t\tif (stats->lonecr || stats->crlf)\n+\t\t\treturn 0;\n+\n+\t\tif (convert_is_binary(len, stats))\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+\n+}\n+\n static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t struct strbuf *buf,\n \t\t enum crlf_action crlf_action, enum safe_crlf checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n+\tint convert_crlf_into_lf;\n \n \tif (crlf_action == CRLF_BINARY ||\n \t (src && !len))\n@@ -252,6 +267,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\treturn 1;\n \n \tgather_stats(src, len, &stats);\n+\t/* Optimization: No CRLF? Nothing to convert, regardless. */\n+\tconvert_crlf_into_lf = !!stats.crlf;\n \n \tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {\n \t\tif (convert_is_binary(len, &stats))\n@@ -263,12 +280,24 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\tif (checksafe == SAFE_CRLF_RENORMALIZE)\n \t\t\tchecksafe = SAFE_CRLF_FALSE;\n \t\telse if (has_cr_in_index(path))\n-\t\t\treturn 0;\n+\t\t\tconvert_crlf_into_lf = 0;\n \t}\n-\tcheck_safe_crlf(path, crlf_action, &stats, checksafe);\n-\n-\t/* Optimization: No CRLF? Nothing to convert, regardless. */\n-\tif (!stats.crlf)\n+\tif (checksafe && len) {\n+\t\tstruct text_stat new_stats;\n+\t\tmemcpy(&new_stats, &stats, sizeof(new_stats));\n+\t\t/* simulate \"git add\" */\n+\t\tif (convert_crlf_into_lf) {\n+\t\t\tnew_stats.lonelf += new_stats.crlf;\n+\t\t\tnew_stats.crlf = 0;\n+\t\t}\n+\t\t/* simulate \"git checkout\" */\n+\t\tif (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {\n+\t\t\tnew_stats.crlf += new_stats.lonelf;\n+\t\t\tnew_stats.lonelf = 0;\n+\t\t}\n+\t\tcheck_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);\n+\t}\n+\tif (!convert_crlf_into_lf)\n \t\treturn 0;\n \n \t/*\n@@ -314,21 +343,9 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n \t\treturn 0;\n \n \tgather_stats(src, len, &stats);\n-\n-\t/* No \"naked\" LF? Nothing to convert, regardless. */\n-\tif (!stats.lonelf)\n+\tif (!will_convert_lf_to_crlf(len, &stats, crlf_action))\n \t\treturn 0;\n \n-\tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {\n-\t\t/* If we have any CR or CRLF line endings, we do not touch it */\n-\t\t/* This is the new safer autocrlf-handling */\n-\t\tif (stats.lonecr || stats.crlf )\n-\t\t\treturn 0;\n-\n-\t\tif (convert_is_binary(len, &stats))\n-\t\t\treturn 0;\n-\t}\n-\n \t/* are we \"faking\" in place editing ? */\n \tif (src == buf->buf)\n \t\tto_free = strbuf_detach(buf, NULL);\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex ab6e962..7de8364 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -393,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '\n \n # attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"\" \"\" true \"\" \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n \n commit_chk_wrnNNO \"auto\" \"\" false \"\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"auto\" \"\" true \"\" \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n for crlf in true false input\n do\n@@ -407,7 +407,7 @@ do\n \tcommit_chk_wrnNNO \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \tcommit_chk_wrnNNO \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n \tcommit_chk_wrnNNO auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO auto crlf \t$crlf \"\" \"\" \"\" \"\" \"\"\n+\tcommit_chk_wrnNNO auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n \tcommit_chk_wrnNNO text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \tcommit_chk_wrnNNO text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n done\n-- \n2.0.0.rc1.6318.g0c2c796\n\n"},{"id":"298867","messageId":"1471020662-20746-1-git-send-email-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 1/2] t0027: Correct raciness in NNO test","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-12T16:51:02Z","receivedAt":"2016-08-12T16:42:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen a non-reversible CRLF conversion is done in \"git add\",\na warning is printed on stderr (or Git dies, depending on checksafe)\n\nThe function commit_chk_wrnNNO() in t0027 was written to test this,\nbut did the wrong thing: Instead of looking at the warning\nfrom \"git add\", it looked at the warning from \"git commit\".\n\nThis is racy because \"git commit\" may not have to do CRLF conversion\nat all if it can use the sha1 value from the index (which depends on\nwhether \"add\" and \"commit\" run in a single second).\n\nCorrect this and replace the commit for each and every file with a commit\nof all files in one go.\n\nThe function commit_chk_wrnNNO() will to be renamed in a separate commit.\nThanks to Jeff King for analyzing t0027.\n\nReported-By: Johannes Schindelin \n---\n t/t0027-auto-crlf.sh | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 2860d2d..ab6e962 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {\n \t\tfname=${pfx}_$f.txt &&\n \t\tcp $f $fname &&\n \t\tprintf Z >>\"$fname\" &&\n-\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n-\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n+\t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n \tdone\n \n \ttest_expect_success \"commit NNO files crlf=$crlf attr=$attr LF\" '\n@@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '\n \n # attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"\" \"\" true \"\" \"\" \"\" \"\" \"\"\n commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n \n-commit_chk_wrnNNO \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"auto\" \"\" false \"\" \"\" \"\" \"\" \"\"\n+commit_chk_wrnNNO \"auto\" \"\" true \"\" \"\" \"\" \"\" \"\"\n commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n for crlf in true false input\n do\n@@ -408,7 +407,7 @@ do\n \tcommit_chk_wrnNNO \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \tcommit_chk_wrnNNO \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n \tcommit_chk_wrnNNO auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n+\tcommit_chk_wrnNNO auto crlf \t$crlf \"\" \"\" \"\" \"\" \"\"\n \tcommit_chk_wrnNNO text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \tcommit_chk_wrnNNO text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n done\n@@ -417,7 +416,8 @@ commit_chk_wrnNNO \"text\" \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\n commit_chk_wrnNNO \"text\" \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n commit_chk_wrnNNO \"text\" \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \n-test_expect_success 'create files cleanup' '\n+test_expect_success 'commit NNO and cleanup' '\n+\tgit commit -m \"commit files on top of NNO\" &&\n \trm -f *.txt &&\n \tgit -c core.autocrlf=false reset --hard\n '\n-- \n2.0.0.rc1.6318.g0c2c796\n\n"},{"id":"298876","messageId":"xmqqeg5tyivl.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"1471020664-20784-1-git-send-email-tboegi@web.de","subject":"Re: [PATCH v1 2/2] convert: missing `LF will be replaced by CRLF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T17:52:46Z","receivedAt":"2016-08-12T17:52:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n>\n> When the \"new safer autocrlf-handling\" was introduced in c4805393, the logic\n> around check_safe_crlf() should have been changed:\n\nShould have been changed from what to what? And instead it was\nchanged to which other logic? What is the difference between the\nideal this change tries to bring in and what c4805393 did?\n\n> Once a file has been commited with CRLF (and core.autocrlf is true),\n> line endings are no longer converted by \"git add\" or \"git checkout\".\n> The line endings will not normalized by Git.\n\ns/will not/& be/;\n\n> The user may run e.g. dos2unix and commit the file.\n\nIt is unclear what this sentence wants to say. The user may overwrite\nthe file with random contents and commit it, too. So what?\n\nWhat is missing from this statement is what problem is being worked\naround. Do you mean: \"Because Git does not normalize once CRLF is\nin the index, if the user wants to have a normalized content in the\nrepository to correct that mistake, the user needs to do dos2unix to\nfix it and commit the fixed result\"?\n\n> Factor out will_convert_lf_to_crlf() which returns when LF are converted\n> into CRLF at checkout.\n>\n> Update the logic around check_safe_crlf() and do the possible CRLF-LF\n> conversion in \"git add\".\n> Simulate the checkout (and a possible LF-CRLF conversion) with help of\n> will_convert_lf_to_crlf().\n\nThese two paragraphs are all \"what I did to solve X\", but what X is\nis not very clear.\n\n> ---\n\nMissing sign-off.\n\n> convert.c | 97 ++++++++++++++++++++++++++++++----------------------\n> t/t0027-auto-crlf.sh | 6 ++--\n> 2 files changed, 60 insertions(+), 43 deletions(-)\n>\n> diff --git a/convert.c b/convert.c\n> index 67d69b5..077f5e6 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)\n> }\n> \n> static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n> - struct text_stat *stats, enum safe_crlf checksafe)\n> +\t\t\t struct text_stat *old_stats, struct text_stat *new_stats,\n> +\t\t\t enum safe_crlf checksafe)\n> {\n> -\tif (!checksafe)\n> -\t\treturn;\n> -\n> -\tif (output_eol(crlf_action) == EOL_LF) {\n> +\tif (old_stats->crlf && !new_stats->crlf ) {\n\nSpace before \")\"\n"},{"id":"298877","messageId":"xmqqa8ghyiov.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"1471020662-20746-1-git-send-email-tboegi@web.de","subject":"Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T17:56:48Z","receivedAt":"2016-08-12T17:56:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n>\n> When a non-reversible CRLF conversion is done in \"git add\",\n> a warning is printed on stderr (or Git dies, depending on checksafe)\n>\n> The function commit_chk_wrnNNO() in t0027 was written to test this,\n> but did the wrong thing: Instead of looking at the warning\n> from \"git add\", it looked at the warning from \"git commit\".\n>\n> This is racy because \"git commit\" may not have to do CRLF conversion\n> at all if it can use the sha1 value from the index (which depends on\n> whether \"add\" and \"commit\" run in a single second).\n>\n> Correct this and replace the commit for each and every file with a commit\n> of all files in one go.\n>\n> The function commit_chk_wrnNNO() will to be renamed in a separate commit.\n> Thanks to Jeff King for analyzing t0027.\n>\n> Reported-By: Johannes Schindelin \n> ---\n\nMissing sign-off. The reasoning above looks very clearly explained.\n\nThanks.\n\n\n> t/t0027-auto-crlf.sh | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n> index 2860d2d..ab6e962 100755\n> --- a/t/t0027-auto-crlf.sh\n> +++ b/t/t0027-auto-crlf.sh\n> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {\n> \t\tfname=${pfx}_$f.txt &&\n> \t\tcp $f $fname &&\n> \t\tprintf Z >>\"$fname\" &&\n> -\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n> -\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n> +\t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n> \tdone\n> \n> \ttest_expect_success \"commit NNO files crlf=$crlf attr=$attr LF\" '\n> @@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '\n> \n> # attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n> commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n> -commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"\" \"\" true \"\" \"\" \"\" \"\" \"\"\n> commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n> \n> -commit_chk_wrnNNO \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n> -commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"auto\" \"\" false \"\" \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"auto\" \"\" true \"\" \"\" \"\" \"\" \"\"\n> commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n> for crlf in true false input\n> do\n> @@ -408,7 +407,7 @@ do\n> \tcommit_chk_wrnNNO \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n> \tcommit_chk_wrnNNO \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n> \tcommit_chk_wrnNNO auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n> -\tcommit_chk_wrnNNO auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n> +\tcommit_chk_wrnNNO auto crlf \t$crlf \"\" \"\" \"\" \"\" \"\"\n> \tcommit_chk_wrnNNO text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n> \tcommit_chk_wrnNNO text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n> done\n> @@ -417,7 +416,8 @@ commit_chk_wrnNNO \"text\" \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\n> commit_chk_wrnNNO \"text\" \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n> commit_chk_wrnNNO \"text\" \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n> \n> -test_expect_success 'create files cleanup' '\n> +test_expect_success 'commit NNO and cleanup' '\n> +\tgit commit -m \"commit files on top of NNO\" &&\n> \trm -f *.txt &&\n> \tgit -c core.autocrlf=false reset --hard\n> '\n"},{"id":"298976","messageId":"57AF4FCB.7090409@kdbg.org","threadId":"43027","inReplyTo":"1471020662-20746-1-git-send-email-tboegi@web.de","subject":"Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-08-13T16:50:19Z","receivedAt":"2016-08-13T16:50:29Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.08.2016 um 18:51 schrieb tboegi@web.de:\n> From: Torsten Bögershausen \n>\n> When a non-reversible CRLF conversion is done in \"git add\",\n> a warning is printed on stderr (or Git dies, depending on checksafe)\n>\n> The function commit_chk_wrnNNO() in t0027 was written to test this,\n> but did the wrong thing: Instead of looking at the warning\n> from \"git add\", it looked at the warning from \"git commit\".\n>\n> This is racy because \"git commit\" may not have to do CRLF conversion\n> at all if it can use the sha1 value from the index (which depends on\n> whether \"add\" and \"commit\" run in a single second).\n>\n> Correct this and replace the commit for each and every file with a commit\n> of all files in one go.\n\nThe new test code does not only fix the race condition, but also tests \ndifferent things, or prepares test cases in a different manner. I would \nhave appreciated an explanation why this is necessary. Is it \"on my \nmachine, the race condition was triggered consistently for a bunch of \ntests, and so I recorded the wrong expected output in the test cases\"?\n\n>\n> The function commit_chk_wrnNNO() will to be renamed in a separate commit.\n> Thanks to Jeff King for analyzing t0027.\n>\n> Reported-By: Johannes Schindelin \n> ---\n> t/t0027-auto-crlf.sh | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\n> index 2860d2d..ab6e962 100755\n> --- a/t/t0027-auto-crlf.sh\n> +++ b/t/t0027-auto-crlf.sh\n> @@ -119,8 +119,7 @@ commit_chk_wrnNNO () {\n> \t\tfname=${pfx}_$f.txt &&\n> \t\tcp $f $fname &&\n> \t\tprintf Z >>\"$fname\" &&\n> -\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n> -\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n> +\t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n> \tdone\n>\n> \ttest_expect_success \"commit NNO files crlf=$crlf attr=$attr LF\" '\n> @@ -394,11 +393,11 @@ test_expect_success 'commit files attr=crlf' '\n>\n> # attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n> commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n> -commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"\" \"\" true \"\" \"\" \"\" \"\" \"\"\n> commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n>\n> -commit_chk_wrnNNO \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n> -commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"auto\" \"\" false \"\" \"\" \"\" \"\" \"\"\n> +commit_chk_wrnNNO \"auto\" \"\" true \"\" \"\" \"\" \"\" \"\"\n> commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n> for crlf in true false input\n> do\n> @@ -408,7 +407,7 @@ do\n> \tcommit_chk_wrnNNO \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n> \tcommit_chk_wrnNNO \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n> \tcommit_chk_wrnNNO auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n> -\tcommit_chk_wrnNNO auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n> +\tcommit_chk_wrnNNO auto crlf \t$crlf \"\" \"\" \"\" \"\" \"\"\n> \tcommit_chk_wrnNNO text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n> \tcommit_chk_wrnNNO text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n> done\n> @@ -417,7 +416,8 @@ commit_chk_wrnNNO \"text\" \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\n> commit_chk_wrnNNO \"text\" \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n> commit_chk_wrnNNO \"text\" \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n>\n> -test_expect_success 'create files cleanup' '\n> +test_expect_success 'commit NNO and cleanup' '\n> +\tgit commit -m \"commit files on top of NNO\" &&\n> \trm -f *.txt &&\n> \tgit -c core.autocrlf=false reset --hard\n> '\n>\n\n"},{"id":"299267","messageId":"20160813212927.11320-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v2 1/1] convert: Correct NNO tests and missing `LF will be replaced by CRLF`","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-13T21:29:27Z","receivedAt":"2016-08-14T10:56:23Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nWhen a non-reversible CRLF conversion is done in \"git add\",\na warning is printed on stderr (or Git dies, depending on checksafe)\n\nThe function commit_chk_wrnNNO() in t0027 was written to test this,\nbut did the wrong thing: Instead of looking at the warning\nfrom \"git add\", it looked at the warning from \"git commit\".\n\nThis is racy because \"git commit\" may not have to do CRLF conversion\nat all if it can use the sha1 value from the index (which depends on\nwhether \"add\" and \"commit\" run in a single second).\n\nCorrect t0027 and replace the commit for each and every file with a commit\nof all files in one go.\nThe function commit_chk_wrnNNO() should be renamed in a separate commit.\n\nNow that t0027 does the right thing, it detects a bug in covert.c:\nThis sequence should generate the warning `LF will be replaced by CRLF`,\nbut does not:\n\n$ git init\n$ git config core.autocrlf false\n$ printf \"Line\\r\\n\" >file\n$ git add file\n$ git commit -m \"commit with CRLF\"\n$ git config core.autocrlf true\n$ printf \"Line\\n\" >file\n$ git add file\n\n\"git add\" calls crlf_to_git() in convert.c, which calls check_safe_crlf().\nWhen has_cr_in_index(path) is true, crlf_to_git() returns too early and\ncheck_safe_crlf() is not called at all.\n\nFactor out the code which determines if \"git checkout\" converts LF->CRLF\ninto will_convert_lf_to_crlf().\n\nUpdate the logic around check_safe_crlf() and \"simulate\" the possible\nLF->CRLF conversion at \"git checkout\" with help of will_convert_lf_to_crlf().\nThanks to Jeff King for analyzing t0027.\n\nReported-By: Johannes Schindelin \nSigned-off-by: Torsten Bögershausen \n---\n convert.c | 97 ++++++++++++++++++++++++++++++----------------------\n t/t0027-auto-crlf.sh | 6 ++--\n 2 files changed, 60 insertions(+), 43 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 67d69b5..077f5e6 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)\n }\n \n static void check_safe_crlf(const char *path, enum crlf_action crlf_action,\n- struct text_stat *stats, enum safe_crlf checksafe)\n+\t\t\t struct text_stat *old_stats, struct text_stat *new_stats,\n+\t\t\t enum safe_crlf checksafe)\n {\n-\tif (!checksafe)\n-\t\treturn;\n-\n-\tif (output_eol(crlf_action) == EOL_LF) {\n+\tif (old_stats->crlf && !new_stats->crlf ) {\n \t\t/*\n-\t\t * CRLFs would not be restored by checkout:\n-\t\t * check if we'd remove CRLFs\n+\t\t * CRLFs would not be restored by checkout\n \t\t */\n-\t\tif (stats->crlf) {\n-\t\t\tif (checksafe == SAFE_CRLF_WARN)\n-\t\t\t\twarning(\"CRLF will be replaced by LF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n-\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n-\t\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n-\t\t}\n-\t} else if (output_eol(crlf_action) == EOL_CRLF) {\n+\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\twarning(\"CRLF will be replaced by LF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n+\t\telse /* i.e. SAFE_CRLF_FAIL */\n+\t\t\tdie(\"CRLF would be replaced by LF in %s.\", path);\n+\t} else if (old_stats->lonelf && !new_stats->lonelf ) {\n \t\t/*\n-\t\t * CRLFs would be added by checkout:\n-\t\t * check if we have \"naked\" LFs\n+\t\t * CRLFs would be added by checkout\n \t\t */\n-\t\tif (stats->lonelf) {\n-\t\t\tif (checksafe == SAFE_CRLF_WARN)\n-\t\t\t\twarning(\"LF will be replaced by CRLF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n-\t\t\telse /* i.e. SAFE_CRLF_FAIL */\n-\t\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n-\t\t}\n+\t\tif (checksafe == SAFE_CRLF_WARN)\n+\t\t\twarning(\"LF will be replaced by CRLF in %s.\\nThe file will have its original line endings in your working directory.\", path);\n+\t\telse /* i.e. SAFE_CRLF_FAIL */\n+\t\t\tdie(\"LF would be replaced by CRLF in %s\", path);\n \t}\n }\n \n@@ -233,12 +225,35 @@ static int has_cr_in_index(const char *path)\n \treturn has_cr;\n }\n \n+static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,\n+\t\t\t\t enum crlf_action crlf_action)\n+{\n+\tif (output_eol(crlf_action) != EOL_CRLF)\n+\t\treturn 0;\n+\t/* No \"naked\" LF? Nothing to convert, regardless. */\n+\tif (!stats->lonelf)\n+\t\treturn 0;\n+\n+\tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {\n+\t\t/* If we have any CR or CRLF line endings, we do not touch it */\n+\t\t/* This is the new safer autocrlf-handling */\n+\t\tif (stats->lonecr || stats->crlf)\n+\t\t\treturn 0;\n+\n+\t\tif (convert_is_binary(len, stats))\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+\n+}\n+\n static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\t struct strbuf *buf,\n \t\t enum crlf_action crlf_action, enum safe_crlf checksafe)\n {\n \tstruct text_stat stats;\n \tchar *dst;\n+\tint convert_crlf_into_lf;\n \n \tif (crlf_action == CRLF_BINARY ||\n \t (src && !len))\n@@ -252,6 +267,8 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\treturn 1;\n \n \tgather_stats(src, len, &stats);\n+\t/* Optimization: No CRLF? Nothing to convert, regardless. */\n+\tconvert_crlf_into_lf = !!stats.crlf;\n \n \tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {\n \t\tif (convert_is_binary(len, &stats))\n@@ -263,12 +280,24 @@ static int crlf_to_git(const char *path, const char *src, size_t len,\n \t\tif (checksafe == SAFE_CRLF_RENORMALIZE)\n \t\t\tchecksafe = SAFE_CRLF_FALSE;\n \t\telse if (has_cr_in_index(path))\n-\t\t\treturn 0;\n+\t\t\tconvert_crlf_into_lf = 0;\n \t}\n-\tcheck_safe_crlf(path, crlf_action, &stats, checksafe);\n-\n-\t/* Optimization: No CRLF? Nothing to convert, regardless. */\n-\tif (!stats.crlf)\n+\tif (checksafe && len) {\n+\t\tstruct text_stat new_stats;\n+\t\tmemcpy(&new_stats, &stats, sizeof(new_stats));\n+\t\t/* simulate \"git add\" */\n+\t\tif (convert_crlf_into_lf) {\n+\t\t\tnew_stats.lonelf += new_stats.crlf;\n+\t\t\tnew_stats.crlf = 0;\n+\t\t}\n+\t\t/* simulate \"git checkout\" */\n+\t\tif (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {\n+\t\t\tnew_stats.crlf += new_stats.lonelf;\n+\t\t\tnew_stats.lonelf = 0;\n+\t\t}\n+\t\tcheck_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);\n+\t}\n+\tif (!convert_crlf_into_lf)\n \t\treturn 0;\n \n \t/*\n@@ -314,21 +343,9 @@ static int crlf_to_worktree(const char *path, const char *src, size_t len,\n \t\treturn 0;\n \n \tgather_stats(src, len, &stats);\n-\n-\t/* No \"naked\" LF? Nothing to convert, regardless. */\n-\tif (!stats.lonelf)\n+\tif (!will_convert_lf_to_crlf(len, &stats, crlf_action))\n \t\treturn 0;\n \n-\tif (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || crlf_action == CRLF_AUTO_CRLF) {\n-\t\t/* If we have any CR or CRLF line endings, we do not touch it */\n-\t\t/* This is the new safer autocrlf-handling */\n-\t\tif (stats.lonecr || stats.crlf )\n-\t\t\treturn 0;\n-\n-\t\tif (convert_is_binary(len, &stats))\n-\t\t\treturn 0;\n-\t}\n-\n \t/* are we \"faking\" in place editing ? */\n \tif (src == buf->buf)\n \t\tto_free = strbuf_detach(buf, NULL);\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 2860d2d..90db54c 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -119,8 +119,7 @@ commit_chk_wrnNNO () {\n \t\tfname=${pfx}_$f.txt &&\n \t\tcp $f $fname &&\n \t\tprintf Z >>\"$fname\" &&\n-\t\tgit -c core.autocrlf=$crlf add $fname 2>/dev/null &&\n-\t\tgit -c core.autocrlf=$crlf commit -m \"commit_$fname\" $fname >\"${pfx}_$f.err\" 2>&1\n+\t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n \tdone\n \n \ttest_expect_success \"commit NNO files crlf=$crlf attr=$attr LF\" '\n@@ -417,7 +416,8 @@ commit_chk_wrnNNO \"text\" \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\n commit_chk_wrnNNO \"text\" \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n commit_chk_wrnNNO \"text\" \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \n-test_expect_success 'create files cleanup' '\n+test_expect_success 'commit NNO and cleanup' '\n+\tgit commit -m \"commit files on top of NNO\" &&\n \trm -f *.txt &&\n \tgit -c core.autocrlf=false reset --hard\n '\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"299269","messageId":"20160813211805.GA10624@tb-raspi","threadId":"43027","inReplyTo":"57AF4FCB.7090409@kdbg.org","subject":"Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-13T21:18:05Z","receivedAt":"2016-08-14T11:23:08Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sat, Aug 13, 2016 at 06:50:19PM +0200, Johannes Sixt wrote:\n> Am 12.08.2016 um 18:51 schrieb tboegi@web.de:\n> >From: Torsten Bögershausen \n> >\n> >When a non-reversible CRLF conversion is done in \"git add\",\n> >a warning is printed on stderr (or Git dies, depending on checksafe)\n> >\n> >The function commit_chk_wrnNNO() in t0027 was written to test this,\n> >but did the wrong thing: Instead of looking at the warning\n> >from \"git add\", it looked at the warning from \"git commit\".\n> >\n> >This is racy because \"git commit\" may not have to do CRLF conversion\n> >at all if it can use the sha1 value from the index (which depends on\n> >whether \"add\" and \"commit\" run in a single second).\n> >\n> >Correct this and replace the commit for each and every file with a commit\n> >of all files in one go.\n> \n> The new test code does not only fix the race condition, but also\n> tests different things, or prepares test cases in a different\n> manner. I would have appreciated an explanation why this is\n> necessary. Is it \"on my machine, the race condition was triggered\n> consistently for a bunch of tests, and so I recorded the wrong\n> expected output in the test cases\"?\n> \nGood point. The origanal intention was to let t0027 pass, and fix\nthe bug in convert.c in a later commit.\n(and rename NNO in a 3rd commit, that is postponed until we figured this out).\n\nLooking at t0027, it turns out that the changes in the test matrix done in 1/2\nare reverted in 2/2.\nThis is not a good thing.\nEither (a) they should be marked as test_expect_failure in 1/2 and\ncorrected in 2/2,\nor (b) 1/2 and 2/2 are squashed together and the noise in t0027 is minimal.\nI'll send a patch for (b) in a second.\n"},{"id":"299272","messageId":"20160813212919.11282-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v2 0/1] convert: Correct NNO tests and missing `LF will be replaced by CRLF`","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-13T21:29:19Z","receivedAt":"2016-08-14T12:10:32Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nChange since v1:\n- The changes done in 1/2 in t0027 needed to be reverted in 2/2.\n Put both changes for convert.c and t0027 into a single commit\nThanks to Johannes Sixt.\nTorsten Bögershausen (1):\n convert: Correct NNO tests and missing `LF will be replaced by CRLF`\n\n convert.c | 97 ++++++++++++++++++++++++++++++----------------------\n t/t0027-auto-crlf.sh | 6 ++--\n 2 files changed, 60 insertions(+), 43 deletions(-)\n\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"299284","messageId":"xmqqpopbkrya.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160813211805.GA10624@tb-raspi","subject":"Re: [PATCH v1 1/2] t0027: Correct raciness in NNO test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-14T20:37:17Z","receivedAt":"2016-08-14T20:37:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Looking at t0027, it turns out that the changes in the test matrix done in 1/2\n> are reverted in 2/2.\n> This is not a good thing.\n> Either (a) they should be marked as test_expect_failure in 1/2 and\n> corrected in 2/2,\n> or (b) 1/2 and 2/2 are squashed together and the noise in t0027 is minimal.\n\nProbably (a) is a bit more kosher and useful when somebody wants to\ninspect the history of the test (\"the test was flaky and that hid a\nfew bugs, but we fix the timing-dependency which exposes the bugs\"\nin 1/2, \"this is the fix for the bug that the previous one exposed\"\nin 2/2). (b) might be a bit less work, though.\n\n"},{"id":"299541","messageId":"alpine.DEB.2.20.1608171445500.4924@virtualbox","threadId":"43027","inReplyTo":"20160813212919.11282-1-tboegi@web.de","subject":"Re: [PATCH v2 0/1] convert: Correct NNO tests and missing `LF will be replaced by CRLF`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-17T12:46:32Z","receivedAt":"2016-08-17T12:46:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Torsten,\n\nOn Sat, 13 Aug 2016, tboegi@web.de wrote:\n\n> From: Torsten Bögershausen \n> \n> Change since v1:\n> - The changes done in 1/2 in t0027 needed to be reverted in 2/2.\n> Put both changes for convert.c and t0027 into a single commit\n> Thanks to Johannes Sixt.\n> Torsten Bögershausen (1):\n> convert: Correct NNO tests and missing `LF will be replaced by CRLF`\n\nThank you so much for the fix!\n\nCiao,\nDscho"},{"id":"299690","messageId":"20160819094127.21101-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 0/1] Rename NotNormalized (NNO) into CRLF in index","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-19T09:41:27Z","receivedAt":"2016-08-19T09:38:30Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nHere comes the promised cleanup of t0027:\n- The wording NNO is removed and replaced by CRI\n- No code changes\n- Needs to go on top of next or pu or tb/t0027-raciness-fix\nTorsten Bögershausen (1):\n t0027: Rename NotNormalized (NNO) into CRLF in index\n\n t/t0027-auto-crlf.sh | 122 +++++++++++++++++++++++++--------------------------\n 1 file changed, 61 insertions(+), 61 deletions(-)\n\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"299691","messageId":"20160819094130.21140-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 1/1] t0027: Rename NotNormalized (NNO) into CRLF in index","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-19T09:41:30Z","receivedAt":"2016-08-19T09:38:35Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nOriginally NNO stands for content, that had been commited\n\"Not NOrmalized\", in other words files with CRLF in the index.\n\nMake more clear what should be tested:\n- commit a file with CRLF into the index\n- Change the content in the working tree\n- Run \"git add\" and check for the conversion warnings\n- Repeat for different content (text, LF, CRLF, mixed) and\n binary (LF and lone CR, CRLF with NUL)\n\nRename commit_chk_wrnNNO() into CRI_add_chk_wrn() and rename NNO into CRI.\n\nIntegrate create_NNO_files() into 'setup master'\n\nSigned-off-by: Torsten Bögershausen \n---\n t/t0027-auto-crlf.sh | 122 +++++++++++++++++++++++++--------------------------\n 1 file changed, 61 insertions(+), 61 deletions(-)\n\ndiff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex 90db54c..bfcf14b 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -49,24 +49,6 @@ create_gitattributes () {\n \t} >.gitattributes\n }\n \n-create_NNO_files () {\n-\tfor crlf in false true input\n-\tdo\n-\t\tfor attr in \"\" auto text -text\n-\t\tdo\n-\t\t\tfor aeol in \"\" lf crlf\n-\t\t\tdo\n-\t\t\t\tpfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}\n-\t\t\t\tcp CRLF_mix_LF ${pfx}_LF.txt &&\n-\t\t\t\tcp CRLF_mix_LF ${pfx}_CRLF.txt &&\n-\t\t\t\tcp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&\n-\t\t\t\tcp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&\n-\t\t\t\tcp CRLF_mix_LF ${pfx}_CRLF_nul.txt\n-\t\t\tdone\n-\t\tdone\n-\tdone\n-}\n-\n check_warning () {\n \tcase \"$1\" in\n \tLF_CRLF) echo \"warning: LF will be replaced by CRLF\" >\"$2\".expect ;;\n@@ -102,7 +84,7 @@ commit_check_warn () {\n \tcheck_warning \"$crlfnul\" ${pfx}_CRLF_nul.err\n }\n \n-commit_chk_wrnNNO () {\n+CRI_add_chk_wrn () {\n \tattr=$1 ; shift\n \taeol=$1 ; shift\n \tcrlf=$1 ; shift\n@@ -111,7 +93,7 @@ commit_chk_wrnNNO () {\n \tlfmixcrlf=$1 ; shift\n \tlfmixcr=$1 ; shift\n \tcrlfnul=$1 ; shift\n-\tpfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}\n+\tpfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}\n \t#Commit files on top of existing file\n \tcreate_gitattributes \"$attr\" $aeol &&\n \tfor f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n@@ -122,22 +104,22 @@ commit_chk_wrnNNO () {\n \t\tgit -c core.autocrlf=$crlf add $fname 2>\"${pfx}_$f.err\"\n \tdone\n \n-\ttest_expect_success \"commit NNO files crlf=$crlf attr=$attr LF\" '\n+\ttest_expect_success \"CRLF in index add file crlf=$crlf attr=$attr LF\" '\n \t\tcheck_warning \"$lfwarn\" ${pfx}_LF.err\n \t'\n-\ttest_expect_success \"commit NNO files attr=$attr aeol=$aeol crlf=$crlf CRLF\" '\n+\ttest_expect_success \"CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf CRLF\" '\n \t\tcheck_warning \"$crlfwarn\" ${pfx}_CRLF.err\n \t'\n \n-\ttest_expect_success \"commit NNO files attr=$attr aeol=$aeol crlf=$crlf CRLF_mix_LF\" '\n+\ttest_expect_success \"CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf CRLF_mix_LF\" '\n \t\tcheck_warning \"$lfmixcrlf\" ${pfx}_CRLF_mix_LF.err\n \t'\n \n-\ttest_expect_success \"commit NNO files attr=$attr aeol=$aeol crlf=$crlf LF_mix_cr\" '\n+\ttest_expect_success \"CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf LF_mix_cr\" '\n \t\tcheck_warning \"$lfmixcr\" ${pfx}_LF_mix_CR.err\n \t'\n \n-\ttest_expect_success \"commit NNO files attr=$attr aeol=$aeol crlf=$crlf CRLF_nul\" '\n+\ttest_expect_success \"CRLF in index add file attr=$attr aeol=$aeol crlf=$crlf CRLF_nul\" '\n \t\tcheck_warning \"$crlfnul\" ${pfx}_CRLF_nul.err\n \t'\n }\n@@ -199,7 +181,7 @@ check_files_in_repo () {\n \tcompare_files $crlfnul ${pfx}CRLF_nul.txt\n }\n \n-check_in_repo_NNO () {\n+check_in_repo_CRI () {\n \tattr=$1 ; shift\n \taeol=$1 ; shift\n \tcrlf=$1 ; shift\n@@ -208,7 +190,7 @@ check_in_repo_NNO () {\n \tlfmixcrlf=$1 ; shift\n \tlfmixcr=$1 ; shift\n \tcrlfnul=$1 ; shift\n-\tpfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}\n+\tpfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}\n \ttest_expect_success \"compare_files $lfname ${pfx}_LF.txt\" '\n \t\tcompare_files $lfname ${pfx}_LF.txt\n \t'\n@@ -329,8 +311,22 @@ test_expect_success 'setup master' '\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\r\\nLINEONE\\r\\nLINETWO\\rLINETHREE\" >CRLF_mix_CR &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\r\\nLINEONEQ\\r\\nLINETWO\\r\\nLINETHREE\" | q_to_nul >CRLF_nul &&\n \tprintf \"\\$Id: 0000000000000000000000000000000000000000 \\$\\nLINEONEQ\\nLINETWO\\nLINETHREE\" | q_to_nul >LF_nul &&\n-\tcreate_NNO_files CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF CRLF_mix_LF &&\n-\tgit -c core.autocrlf=false add NNO_*.txt &&\n+\tfor crlf in false true input\n+\tdo\n+\t\tfor attr in \"\" auto text -text\n+\t\tdo\n+\t\t\tfor aeol in \"\" lf crlf\n+\t\t\tdo\n+\t\t\t\tpfx=CRI_attr_${attr}_aeol_${aeol}_${crlf}\n+\t\t\t\tcp CRLF_mix_LF ${pfx}_LF.txt &&\n+\t\t\t\tcp CRLF_mix_LF ${pfx}_CRLF.txt &&\n+\t\t\t\tcp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&\n+\t\t\t\tcp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&\n+\t\t\t\tcp CRLF_mix_LF ${pfx}_CRLF_nul.txt\n+\t\t\tdone\n+\t\tdone\n+\tdone &&\n+\tgit -c core.autocrlf=false add CRI_*.txt &&\n \tgit commit -m \"mixed line endings\" &&\n \ttest_tick\n '\n@@ -391,33 +387,37 @@ test_expect_success 'commit files attr=crlf' '\n \tcommit_check_warn input \"crlf\" \"LF_CRLF\" \"\" \"LF_CRLF\" \"LF_CRLF\" \"\"\n '\n \n-# attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n-commit_chk_wrnNNO \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n-\n-commit_chk_wrnNNO \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n-commit_chk_wrnNNO \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n+# Files had been commited with CRLF. run \"git add\" with different content and\n+# check if\n+# warning: LF will be replaced by CRLF or\n+# warning: CRLF will be replaced by LF are written to stderr\n+# attr LF CRLF CRLFmixLF LF_mix_CR CRLFNUL\n+CRI_add_chk_wrn \"\" \"\" false \"\" \"\" \"\" \"\" \"\"\n+CRI_add_chk_wrn \"\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n+CRI_add_chk_wrn \"\" \"\" input \"\" \"\" \"\" \"\" \"\"\n+\n+CRI_add_chk_wrn \"auto\" \"\" false \"$WILC\" \"\" \"\" \"\" \"\"\n+CRI_add_chk_wrn \"auto\" \"\" true LF_CRLF \"\" \"\" \"\" \"\"\n+CRI_add_chk_wrn \"auto\" \"\" input \"\" \"\" \"\" \"\" \"\"\n for crlf in true false input\n do\n-\tcommit_chk_wrnNNO -text \"\" $crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO -text lf $crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO -text crlf $crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n-\tcommit_chk_wrnNNO \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n-\tcommit_chk_wrnNNO auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n-\tcommit_chk_wrnNNO text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n-\tcommit_chk_wrnNNO text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n+\tCRI_add_chk_wrn -text \"\" $crlf \"\" \"\" \"\" \"\" \"\"\n+\tCRI_add_chk_wrn -text lf $crlf \"\" \"\" \"\" \"\" \"\"\n+\tCRI_add_chk_wrn -text crlf $crlf \"\" \"\" \"\" \"\" \"\"\n+\tCRI_add_chk_wrn \"\" lf $crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n+\tCRI_add_chk_wrn \"\" crlf $crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n+\tCRI_add_chk_wrn auto lf \t$crlf \"\" \"\" \"\" \"\" \"\"\n+\tCRI_add_chk_wrn auto crlf \t$crlf LF_CRLF \"\" \"\" \"\" \"\"\n+\tCRI_add_chk_wrn text lf \t$crlf \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n+\tCRI_add_chk_wrn text crlf \t$crlf LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n done\n \n-commit_chk_wrnNNO \"text\" \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\" \"$WICL\"\n-commit_chk_wrnNNO \"text\" \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n-commit_chk_wrnNNO \"text\" \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n+CRI_add_chk_wrn text \"\" false \"$WILC\" \"$WICL\" \"$WAMIX\" \"$WILC\" \"$WICL\"\n+CRI_add_chk_wrn text \"\" true LF_CRLF \"\" LF_CRLF LF_CRLF \"\"\n+CRI_add_chk_wrn text \"\" input \"\" CRLF_LF CRLF_LF \"\" CRLF_LF\n \n-test_expect_success 'commit NNO and cleanup' '\n-\tgit commit -m \"commit files on top of NNO\" &&\n+test_expect_success 'commit files with CRLF in index CRI and cleanup' '\n+\tgit commit -m \"commit files on top of CRLF\" &&\n \trm -f *.txt &&\n \tgit -c core.autocrlf=false reset --hard\n '\n@@ -449,16 +449,16 @@ test_expect_success 'commit -text' '\n for crlf in true false input\n do\n \t# attr aeol LF CRLF CRLF_mix_LF LF_mix_CR CRLFNUL\n-\tcheck_in_repo_NNO \"\" \"\" $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO -text \"\" $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO -text lf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO -text crlf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO auto \"\" $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO auto lf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO auto crlf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n-\tcheck_in_repo_NNO text \"\" $crlf LF LF LF LF_mix_CR LF_nul\n-\tcheck_in_repo_NNO text lf $crlf LF LF LF LF_mix_CR LF_nul\n-\tcheck_in_repo_NNO text crlf $crlf LF LF LF LF_mix_CR LF_nul\n+\tcheck_in_repo_CRI \"\" \"\" $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI -text \"\" $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI -text lf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI -text crlf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI auto \"\" $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI auto lf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI auto crlf $crlf LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul\n+\tcheck_in_repo_CRI text \"\" $crlf LF LF LF LF_mix_CR LF_nul\n+\tcheck_in_repo_CRI text lf $crlf LF LF LF LF_mix_CR LF_nul\n+\tcheck_in_repo_CRI text crlf $crlf LF LF LF LF_mix_CR LF_nul\n done\n ################################################################################\n # Check how files in the repo are changed when they are checked out\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"299718","messageId":"xmqqlgzs3e89.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160819094127.21101-1-tboegi@web.de","subject":"Re: [PATCH v1 0/1] Rename NotNormalized (NNO) into CRLF in index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-19T16:39:18Z","receivedAt":"2016-08-19T16:39:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n>\n> Here comes the promised cleanup of t0027:\n> - The wording NNO is removed and replaced by CRI\n> - No code changes\n> - Needs to go on top of next or pu or tb/t0027-raciness-fix\n> Torsten Bögershausen (1):\n> t0027: Rename NotNormalized (NNO) into CRLF in index\n>\n> t/t0027-auto-crlf.sh | 122 +++++++++++++++++++++++++--------------------------\n> 1 file changed, 61 insertions(+), 61 deletions(-)\n\nUntil these acronyms are shown expanded upfront in the file (not in\nthe log message), this patch does not help the readers at all. They\nused to say \"What the heck is NNO?\" Now they will be left wondering\n\"What the heck is CRI?\".\n\nHave a comment before the first use of the acronym, perhaps?\n\n@@ -102,7 +84,7 @@ commit_check_warn () {\n \tcheck_warning \"$crlfnul\" ${pfx}_CRLF_nul.err\n }\n \n-commit_chk_wrnNNO () {\n+CRI_add_chk_wrn () {\n \tattr=$1 ; shift\n \taeol=$1 ; shift\n \tcrlf=$1 ; shift\n\n\nIncidentally it is a good place to describe what this humongous\nhelper function is meant to do.\n\n# Having CR in the indexed contents (CRI in short) poses such and\n# such tricky cases; this helper tests combinations of xyzzy, frotz\n# and nitfol exhaustively by running xyzzy through (a, b, c),\n# flipping frotz between (x, y), and ...\n\nor something like that (of course the blanks need to be filled),\nperhaps?\n"},{"id":"300171","messageId":"20160825155257.28968-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-25T15:52:57Z","receivedAt":"2016-08-25T15:43:33Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nThe man page for `git ls-files --eol` mentions the combination\nof text attributes \"text=auto eol=lf\" or \"text=auto eol=crlf\" as not\nsupported yet, but may be in the future.\nNow they are supported\n\nSigned-off-by: Torsten Bögershausen \n---\n Documentation/git-ls-files.txt | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 078b556..0d933ac 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -159,8 +159,7 @@ not accessible in the working tree.\n +\n is the attribute that is used when checking out or committing,\n it is either \"\", \"-text\", \"text\", \"text=auto\", \"text eol=lf\", \"text eol=crlf\".\n-Note: Currently Git does not support \"text=auto eol=lf\" or \"text=auto eol=crlf\",\n-that may change in the future.\n+Since Git 2.10 \"text=auto eol=lf\" and \"text=auto eol=crlf\" are supported.\n +\n Both the in the index (\"i/\")\n and in the working tree (\"w/\") are shown for regular files,\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"300172","messageId":"20160825155255.28929-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 0/3] Update eol documentation","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-25T15:52:55Z","receivedAt":"2016-08-25T15:43:44Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nSorry for posting this so late:\nWhile reviewing another patch I realized that the eol related\ndocumentation was not updated as it should be.\n\nTorsten Bögershausen (2):\n git ls-files: text=auto eol=lf is supported in Git 2.10\n gitattributes: Document the unified \"auto\" handling\n\n Documentation/git-ls-files.txt | 3 +--\n Documentation/gitattributes.txt | 24 ++++++++++++++++--------\n 2 files changed, 17 insertions(+), 10 deletions(-)\n\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"300173","messageId":"20160825155258.29007-1-tboegi@web.de","threadId":"43027","inReplyTo":"20160809114938.pcrvirrzrh6ldmnr@sigill.intra.peff.net","subject":"[PATCH v1 2/2] gitattributes: Document the unified \"auto\" handling","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-25T15:52:58Z","receivedAt":"2016-08-25T15:43:47Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nUpdate the documentation about text=auto:\ntext=auto now follows the core.autocrlf handling when files are not\nnormalized in the repository.\n\nFor a cross platform project recommend the usage of attributes for\nline-ending conversions.\n\nSigned-off-by: Torsten Bögershausen \n---\n Documentation/gitattributes.txt | 24 ++++++++++++++++--------\n 1 file changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 807577a..4012661 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -213,27 +213,35 @@ that text files that you introduce to the repository have their line\n endings normalized to LF when they are added, and that files that are\n already normalized in the repository stay normalized.\n \n+If you want to ensure that text files that any contributor introduces to\n+the repository have their line endings normalized, you could set the\n+`text` attribute to \"auto\" for _all_ files.\n+\n+------------------------\n+*\ttext=auto\n+------------------------\n+\n If you want to interoperate with a source code management system that\n enforces end-of-line normalization, or you simply want all text files\n in your repository to be normalized, you should instead set the `text`\n-attribute to \"auto\" for _all_ files.\n+attribute to \"text\" for text files.\n \n ------------------------\n-*\ttext=auto\n+*.txt\ttext\n ------------------------\n \n-This ensures that all files that Git considers to be text will have\n+This ensures that all files marked as text will have\n normalized (LF) line endings in the repository. The `core.eol`\n configuration variable controls which line endings Git will use for\n normalized files in your working directory; the default is to use the\n native line ending for your platform, or CRLF if `core.autocrlf` is\n set.\n \n-NOTE: When `text=auto` normalization is enabled in an existing\n-repository, any text files containing CRLFs should be normalized. If\n-they are not they will be normalized the next time someone tries to\n-change them, causing unfortunate misattribution. From a clean working\n-directory:\n+NOTE: When you have a cross-platform project using push and pull\n+to a central repository the text files containing CRLFs should be\n+normalized. All text files should have a text attribute, either\n+`text` or `text=auto`.\n+From a clean working directory:\n \n -------------------------------------------------\n $ echo \"* text=auto\" >>.gitattributes\n-- \n2.9.3.599.g2376d31.dirty\n\n"},{"id":"300192","messageId":"xmqqy43klhds.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160825155255.28929-1-tboegi@web.de","subject":"Re: [PATCH v1 0/3] Update eol documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T20:31:59Z","receivedAt":"2016-08-25T20:32:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n>\n> Sorry for posting this so late:\n> While reviewing another patch I realized that the eol related\n> documentation was not updated as it should be.\n>\n> Torsten Bögershausen (2):\n> git ls-files: text=auto eol=lf is supported in Git 2.10\n> gitattributes: Document the unified \"auto\" handling\n>\n> Documentation/git-ls-files.txt | 3 +--\n> Documentation/gitattributes.txt | 24 ++++++++++++++++--------\n> 2 files changed, 17 insertions(+), 10 deletions(-)\n\nThis [0/3] is meant to be a cover for [1/2] and [2/2]?\n\nI am trying to see if we broke format-patch recently, or it is a\nmanual editing error. The latter I do not care about; the former I\ndo.\n"},{"id":"300195","messageId":"xmqqtwe8lh3d.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160825155257.28968-1-tboegi@web.de","subject":"Re: [PATCH v1 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T20:38:14Z","receivedAt":"2016-08-25T20:46:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n>\n> The man page for `git ls-files --eol` mentions the combination\n> of text attributes \"text=auto eol=lf\" or \"text=auto eol=crlf\" as not\n> supported yet, but may be in the future.\n> Now they are supported\n\nThanks. I'll finish the sentence with a full-stop here ;-).\n\n>\n> Signed-off-by: Torsten Bögershausen \n> ---\n> Documentation/git-ls-files.txt | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\n> index 078b556..0d933ac 100644\n> --- a/Documentation/git-ls-files.txt\n> +++ b/Documentation/git-ls-files.txt\n> @@ -159,8 +159,7 @@ not accessible in the working tree.\n> +\n> is the attribute that is used when checking out or committing,\n> it is either \"\", \"-text\", \"text\", \"text=auto\", \"text eol=lf\", \"text eol=crlf\".\n> -Note: Currently Git does not support \"text=auto eol=lf\" or \"text=auto eol=crlf\",\n> -that may change in the future.\n> +Since Git 2.10 \"text=auto eol=lf\" and \"text=auto eol=crlf\" are supported.\n\nIt may be a good idea to have this for a while. Having this\nsentence would only help those who have been dissuaded by the\nexisting Note by telling them that the limitation is no longer\nthere, but that will quickly become unnecessary.\n\nWe'd eventually want to remove this sentence. I wonder if it is a\nbetter alternative to just remove the Note without adding new text,\nthough.\n\n> Both the in the index (\"i/\")\n> and in the working tree (\"w/\") are shown for regular files,\n"},{"id":"300197","messageId":"xmqqpoowlgpv.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160825155258.29007-1-tboegi@web.de","subject":"Re: [PATCH v1 2/2] gitattributes: Document the unified \"auto\" handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-25T20:46:20Z","receivedAt":"2016-08-25T20:46:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> +If you want to ensure that text files that any contributor introduces to\n> +the repository have their line endings normalized, you could set the\n> +`text` attribute to \"auto\" for _all_ files.\n> +\n> +------------------------\n> +*\ttext=auto\n> +------------------------\n> +\n\nThat is very understandable, especially that the text before this\nadded paragraph is about \"core.autocrlf\" configuration that is about\n\"your\" changes. It contrasts gitconfig vs gitattributes very well.\n\nHowever, it is no longer clear what \"you should instead\" in the\nexisting paragraph attempts to contrast with. \"If you want all text\nfiles, then use '* text=auto'\" is what is said previously. And your\nupdated example below says \"If you do not want that, and instead you\nwant X, do '*.txt text'\". But the value of X is reads the same as\nthe above one: \"you want all text files to be normalized\".\n\n> If you want to interoperate with a source code management system that\n> enforces end-of-line normalization, or you simply want all text files\n> in your repository to be normalized, you should instead set the `text`\n> -attribute to \"auto\" for _all_ files.\n> +attribute to \"text\" for text files.\n> \n> ------------------------\n> -*\ttext=auto\n> +*.txt\ttext\n> ------------------------\n\nIn short, the above is incoherent and not understandable, without\nupdating the three lines of introductory text you left untouched at\nthe beginning of the paragraph, when read in the (updated) context.\n\n> -This ensures that all files that Git considers to be text will have\n> +This ensures that all files marked as text will have\n\nThis is a good update of the description to match the updated example.\n"},{"id":"300224","messageId":"CA+P7+xr+Bi232Cexu-t+W0sSzGrKkS=A=9EWqivKjnw2GueGXQ@mail.gmail.com","threadId":"43027","inReplyTo":"xmqqy43klhds.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 0/3] Update eol documentation","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-08-26T01:00:33Z","receivedAt":"2016-08-26T01:07:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Aug 25, 2016 at 1:31 PM, Junio C Hamano wrote:\n> tboegi@web.de writes:\n>\n>> From: Torsten Bögershausen \n>>\n>> Sorry for posting this so late:\n>> While reviewing another patch I realized that the eol related\n>> documentation was not updated as it should be.\n>>\n>> Torsten Bögershausen (2):\n>> git ls-files: text=auto eol=lf is supported in Git 2.10\n>> gitattributes: Document the unified \"auto\" handling\n>>\n>> Documentation/git-ls-files.txt | 3 +--\n>> Documentation/gitattributes.txt | 24 ++++++++++++++++--------\n>> 2 files changed, 17 insertions(+), 10 deletions(-)\n>\n> This [0/3] is meant to be a cover for [1/2] and [2/2]?\n>\n> I am trying to see if we broke format-patch recently, or it is a\n> manual editing error. The latter I do not care about; the former I\n> do.\n> --\n\nYes. I recently changed some of format patch and would like to make\nsure I didn't break it...\n\nThanks,\nJake\n"},{"id":"300228","messageId":"3ab51b90-f0f8-52b1-e12f-adb151f031bf@web.de","threadId":"43027","inReplyTo":"xmqqy43klhds.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 0/3] Update eol documentation","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-08-26T07:03:31Z","receivedAt":"2016-08-26T07:05:01Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"\n\nOn 25/08/16 22:31, Junio C Hamano wrote:\n> []\n\n> This [0/3] is meant to be a cover for [1/2] and [2/2]?\n>\n> I am trying to see if we broke format-patch recently, or it is a\n> manual editing error. The latter I do not care about; the former I\n> do.\n>\n\nNo worry, 0/3 is patched by me by hand, sorry for the confusion.\n\n"},{"id":"300298","messageId":"20160826201842.28293-1-tboegi@web.de","threadId":"43027","inReplyTo":"xmqqpoowlgpv.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 0/2] Adjust the documentation to the unified \"auto\" handling","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-26T20:18:42Z","receivedAt":"2016-08-26T20:27:21Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nChanges since v1:\n- 1/2 is left unchanged\n- 2/2 is re-written and should be more consistant to read.\n\nTorsten Bögershausen (2):\n git ls-files: text=auto eol=lf is supported in Git 2.10\n gitattributes: Document the unified \"auto\" handling\n\n Documentation/git-ls-files.txt | 3 +--\n Documentation/gitattributes.txt | 58 +++++++++++++++++------------------------\n 2 files changed, 25 insertions(+), 36 deletions(-)\n\n-- \n2.9.0.243.g5c589a7\n\n"},{"id":"300299","messageId":"20160826201845.28341-1-tboegi@web.de","threadId":"43027","inReplyTo":"xmqqpoowlgpv.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-26T20:18:45Z","receivedAt":"2016-08-26T20:27:24Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nThe man page for `git ls-files --eol` mentions the combination\nof text attributes \"text=auto eol=lf\" or \"text=auto eol=crlf\" as not\nsupported yet, but may be in the future.\nNow they are supported\n\nSigned-off-by: Torsten Bögershausen \n---\n Documentation/git-ls-files.txt | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt\nindex 078b556..0d933ac 100644\n--- a/Documentation/git-ls-files.txt\n+++ b/Documentation/git-ls-files.txt\n@@ -159,8 +159,7 @@ not accessible in the working tree.\n +\n is the attribute that is used when checking out or committing,\n it is either \"\", \"-text\", \"text\", \"text=auto\", \"text eol=lf\", \"text eol=crlf\".\n-Note: Currently Git does not support \"text=auto eol=lf\" or \"text=auto eol=crlf\",\n-that may change in the future.\n+Since Git 2.10 \"text=auto eol=lf\" and \"text=auto eol=crlf\" are supported.\n +\n Both the in the index (\"i/\")\n and in the working tree (\"w/\") are shown for regular files,\n-- \n2.9.0.243.g5c589a7\n\n"},{"id":"300300","messageId":"20160826201848.28381-1-tboegi@web.de","threadId":"43027","inReplyTo":"xmqqpoowlgpv.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2 2/2] gitattributes: Document the unified \"auto\" handling","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2016-08-26T20:18:48Z","receivedAt":"2016-08-26T20:27:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen \n\nUpdate the documentation about text=auto:\ntext=auto now follows the core.autocrlf handling when files are not\nnormalized in the repository.\n\nFor a cross platform project recommend the usage of attributes for\nline-ending conversions.\n\nSigned-off-by: Torsten Bögershausen \n---\n Documentation/gitattributes.txt | 58 +++++++++++++++++------------------------\n 1 file changed, 24 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 807577a..7aff940 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -182,23 +182,6 @@ While Git normally leaves file contents alone, it can be configured to\n normalize line endings to LF in the repository and, optionally, to\n convert them to CRLF when files are checked out.\n \n-Here is an example that will make Git normalize .txt, .vcproj and .sh\n-files, ensure that .vcproj files have CRLF and .sh files have LF in\n-the working directory, and prevent .jpg files from being normalized\n-regardless of their content.\n-\n-------------------------\n-* text=auto\n-*.txt\t\ttext\n-*.vcproj\ttext eol=crlf\n-*.sh\t\ttext eol=lf\n-*.jpg\t\t-text\n-------------------------\n-\n-Other source code management systems normalize all text files in their\n-repositories, and there are two ways to enable similar automatic\n-normalization in Git.\n-\n If you simply want to have CRLF line endings in your working directory\n regardless of the repository you are working with, you can set the\n config variable \"core.autocrlf\" without using any attributes.\n@@ -208,35 +191,42 @@ config variable \"core.autocrlf\" without using any attributes.\n \tautocrlf = true\n ------------------------\n \n-This does not force normalization of all text files, but does ensure\n+This does not force normalization of text files, but does ensure\n that text files that you introduce to the repository have their line\n endings normalized to LF when they are added, and that files that are\n already normalized in the repository stay normalized.\n \n-If you want to interoperate with a source code management system that\n-enforces end-of-line normalization, or you simply want all text files\n-in your repository to be normalized, you should instead set the `text`\n-attribute to \"auto\" for _all_ files.\n+If you want to ensure that text files that any contributor introduces to\n+the repository have their line endings normalized, you can set the\n+`text` attribute to \"auto\" for _all_ files.\n \n ------------------------\n *\ttext=auto\n ------------------------\n \n-This ensures that all files that Git considers to be text will have\n-normalized (LF) line endings in the repository. The `core.eol`\n-configuration variable controls which line endings Git will use for\n-normalized files in your working directory; the default is to use the\n-native line ending for your platform, or CRLF if `core.autocrlf` is\n-set.\n+The attributes allow a fine-grained control, how the line endings\n+are converted.\n+Here is an example that will make Git normalize .txt, .vcproj and .sh\n+files, ensure that .vcproj files have CRLF and .sh files have LF in\n+the working directory, and prevent .jpg files from being normalized\n+regardless of their content.\n+\n+------------------------\n+* text=auto\n+*.txt\t\ttext\n+*.vcproj\ttext eol=crlf\n+*.sh\t\ttext eol=lf\n+*.jpg\t\t-text\n+------------------------\n+\n+NOTE: When `text=auto` conversion is enabled in a cross-platform\n+project using push and pull to a central repository the text files\n+containing CRLFs should be normalized.\n \n-NOTE: When `text=auto` normalization is enabled in an existing\n-repository, any text files containing CRLFs should be normalized. If\n-they are not they will be normalized the next time someone tries to\n-change them, causing unfortunate misattribution. From a clean working\n-directory:\n+From a clean working directory:\n \n -------------------------------------------------\n-$ echo \"* text=auto\" >>.gitattributes\n+$ echo \"* text=auto\" >.gitattributes\n $ rm .git/index # Remove the index to force Git to\n $ git reset # re-scan the working directory\n $ git status # Show files that will be normalized\n-- \n2.9.0.243.g5c589a7\n\n"},{"id":"300305","messageId":"xmqq7fb3fdzs.fsf@gitster.mtv.corp.google.com","threadId":"43027","inReplyTo":"20160826201848.28381-1-tboegi@web.de","subject":"Re: [PATCH v2 2/2] gitattributes: Document the unified \"auto\" handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-26T20:53:59Z","receivedAt":"2016-08-26T21:00:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n> From: Torsten Bögershausen \n>\n> Update the documentation about text=auto:\n> text=auto now follows the core.autocrlf handling when files are not\n> normalized in the repository.\n>\n> For a cross platform project recommend the usage of attributes for\n> line-ending conversions.\n>\n> Signed-off-by: Torsten Bögershausen \n> ---\n> Documentation/gitattributes.txt | 58 +++++++++++++++++------------------------\n> 1 file changed, 24 insertions(+), 34 deletions(-)\n\nLooks good. Let me merge this to 'master' before cutting -rc2.\n\nThanks.\n"}]}