{"thread":{"id":"41216","subject":"[PATCH] travis-ci: run previously failed tests first, then slowest to fastest","startedAt":"2016-01-19T09:24:29Z","lastAt":"2016-02-03T08:31:03Z","messageCount":41,"participants":["larsxschneider@gmail.com","Jeff King","Junio C Hamano","Mike Hommey","Johannes Schindelin","Lars Schneider","brian m. carlson","Thomas Gummerer","Clemens Buchacher","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"276338","messageId":"1453195469-51696-1-git-send-email-larsxschneider@gmail.com","threadId":"41216","inReplyTo":null,"subject":"[PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-19T09:24:29Z","receivedAt":"2016-01-19T09:24:29Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nUse the Travis-CI cache feature to store prove test results and make them\navailable in subsequent builds. This allows to run previously failed tests\nfirst and run remaining tests in slowest to fastest order. As a result it\nis less likely that Travis-CI needs to wait for a single test at the end\nwhich speeds up the test suite execution by ~2 min.\n\nUnfortunately the cache feature is only available (for free) on the\nTravis-CI Linux environment.\n\nSuggested-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n .travis.yml | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex c3bf9c6..f34726b 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -1,5 +1,9 @@\n language: c\n\n+cache:\n+ directories:\n+ - $HOME/.prove-cache\n+\n os:\n - linux\n - osx\n@@ -18,7 +22,7 @@ env:\n - P4_VERSION=\"15.2\"\n - GIT_LFS_VERSION=\"1.1.0\"\n - DEFAULT_TEST_TARGET=prove\n- - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n+ - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n - GIT_TEST_OPTS=\"--verbose --tee\"\n - CFLAGS=\"-g -O2 -Wall -Werror\"\n - GIT_TEST_CLONE_2GB=YesPlease\n@@ -67,6 +71,8 @@ before_install:\n p4 -V | grep Rev.;\n echo \"$(tput setaf 6)Git-LFS Version$(tput sgr0)\";\n git-lfs version;\n+ mkdir -p $HOME/.prove-cache;\n+ ln -s $HOME/.prove-cache/.prove t/.prove;\n\n before_script: make --jobs=2\n\n--\n2.5.1\n"},{"id":"276360","messageId":"20160119191234.GA17562@sigill.intra.peff.net","threadId":"41216","inReplyTo":"1453195469-51696-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T19:12:35Z","receivedAt":"2016-01-19T19:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider \n> \n> Use the Travis-CI cache feature to store prove test results and make them\n> available in subsequent builds. This allows to run previously failed tests\n> first and run remaining tests in slowest to fastest order. As a result it\n> is less likely that Travis-CI needs to wait for a single test at the end\n> which speeds up the test suite execution by ~2 min.\n\nThanks, this makes sense, and the patch looks good.\n\n> @@ -18,7 +22,7 @@ env:\n> - P4_VERSION=\"15.2\"\n> - GIT_LFS_VERSION=\"1.1.0\"\n> - DEFAULT_TEST_TARGET=prove\n> - - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n> + - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n\nHave you tried bumping --jobs here? I usually use \"16\" on my local box.\n\nI also looked into the Travis \"container\" thing. It's not clear to me\nfrom their page:\n\n https://docs.travis-ci.com/user/workers/container-based-infrastructure/\n\nwhether we're using the new, faster container infrastructure or not. It\ndepends on when Travis \"recognized\" the repo, but I'm not quite sure\nwhat that means. Should we be adding \"sudo: false\" to the top-level of\nthe yaml file?\n\n-Peff\n"},{"id":"276361","messageId":"xmqqmvs19w5n.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"1453195469-51696-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T20:00:52Z","receivedAt":"2016-01-19T20:00:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"larsxschneider@gmail.com writes:\n\n> From: Lars Schneider \n>\n> Use the Travis-CI cache feature to store prove test results and make them\n> available in subsequent builds. This allows to run previously failed tests\n> first and run remaining tests in slowest to fastest order. As a result it\n> is less likely that Travis-CI needs to wait for a single test at the end\n> which speeds up the test suite execution by ~2 min.\n>\n> Unfortunately the cache feature is only available (for free) on the\n> Travis-CI Linux environment.\n>\n> Suggested-by: Jeff King \n> Signed-off-by: Lars Schneider \n> ---\n> .travis.yml | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n\nThis is cute, but isn't it useful even outside Travis's context? I\nam not suggesting to touch anything other than .travis.yml file in\nthis patch, but if I wanted to get the benefit from the idea in this\npatch when I run my tests manually, I can just tell prove to use the\ncached states, no?\n\nIOW, I am confused by the beginning of the log message that says\nthis is taking advantage of \"the Travis-CI cache feature\". This\nimprovement looks to me like using the feature of \"prove\" that\nallows us to run slower tests first, and does not have much to do\nwith Travis.\n\nYou are relying on the assumption that things under $HOME/ is stable\nwhile things under t/ (or in our source tree in general) are not,\nand I think that is a sensible thing to take advantage of, but are\nwe sure that they are running in an environment where \"ln -s\" would\nwork? Otherwise, it may be more robust to copy $HOME/.prove to\nt/.prove before starting to test and then copy it back once the\ntests are done.\n\nThanks.\n\n>\n> diff --git a/.travis.yml b/.travis.yml\n> index c3bf9c6..f34726b 100644\n> --- a/.travis.yml\n> +++ b/.travis.yml\n> @@ -1,5 +1,9 @@\n> language: c\n>\n> +cache:\n> + directories:\n> + - $HOME/.prove-cache\n> +\n> os:\n> - linux\n> - osx\n> @@ -18,7 +22,7 @@ env:\n> - P4_VERSION=\"15.2\"\n> - GIT_LFS_VERSION=\"1.1.0\"\n> - DEFAULT_TEST_TARGET=prove\n> - - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n> + - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n> - GIT_TEST_OPTS=\"--verbose --tee\"\n> - CFLAGS=\"-g -O2 -Wall -Werror\"\n> - GIT_TEST_CLONE_2GB=YesPlease\n> @@ -67,6 +71,8 @@ before_install:\n> p4 -V | grep Rev.;\n> echo \"$(tput setaf 6)Git-LFS Version$(tput sgr0)\";\n> git-lfs version;\n> + mkdir -p $HOME/.prove-cache;\n> + ln -s $HOME/.prove-cache/.prove t/.prove;\n>\n> before_script: make --jobs=2\n>\n> --\n> 2.5.1\n"},{"id":"276377","messageId":"xmqqio2p89mb.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"xmqqmvs19w5n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T22:53:00Z","receivedAt":"2016-01-19T22:53:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This is cute, but isn't it useful even outside Travis's context? I\n> am not suggesting to touch anything other than .travis.yml file in\n> this patch, but if I wanted to get the benefit from the idea in this\n> patch when I run my tests manually, I can just tell prove to use the\n> cached states, no?\n\nIt seems that exporting something like\n\n GIT_PROVE_OPTS=\"--timer --state=slow,save -j8\" \n\nwhen running \"make DEFAULT_TEST_TARGET=prove test\" does give me the\nsame benefit by leaving the stats from the previous run in t/.prove\nwhen making the test scheduling decisions.\n\nOne thing I noticed but didn't dig further to fix was that this\n\"prove --state\" business did not seem to work well together with\n\n make T=\"...list of tests...\" test\n\nthat limits the set of tests to perform. For example:\n\n $ rm -f t/.prove\n $ make -j4 GIT_PROVE_OPTS=\"--timer --state=slow,save -j8\" \\\n\t T=\"$( cd t && echo t0???-*.sh)\" \\\n DEFAULT_TEST_TARGET=prove test\n\nruns all test in 0xxx series and populates t/.prove with them. And\nimmediately after that, with t/.prove still there:\n\n $ make -j4 GIT_PROVE_OPTS=\"--timer --state=slow,save -j8\" \\\n DEFAULT_TEST_TARGET=prove \\\n T=t0000-basic.sh test\n\ndoes not limit the test to only 0000, but ends up running all the\nothers recorded in t/.prove file, it seems.\n\nI would imagine that this would not affect your use case negatively,\nas it is unlikely that your automated tests are skipping different\nset of tests in each run.\n"},{"id":"276378","messageId":"xmqqegdd8997.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160119191234.GA17562@sigill.intra.peff.net","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T23:00:52Z","receivedAt":"2016-01-19T23:00:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:\n>\n>> From: Lars Schneider \n>> \n>> Use the Travis-CI cache feature to store prove test results and make them\n>> available in subsequent builds. This allows to run previously failed tests\n>> first and run remaining tests in slowest to fastest order. As a result it\n>> is less likely that Travis-CI needs to wait for a single test at the end\n>> which speeds up the test suite execution by ~2 min.\n>\n> Thanks, this makes sense, and the patch looks good.\n>\n>> @@ -18,7 +22,7 @@ env:\n>> - P4_VERSION=\"15.2\"\n>> - GIT_LFS_VERSION=\"1.1.0\"\n>> - DEFAULT_TEST_TARGET=prove\n>> - - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n>> + - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n>\n> Have you tried bumping --jobs here? I usually use \"16\" on my local box.\n\nI think 3 comes from this:\n\n http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674\n\n>\n> I also looked into the Travis \"container\" thing. It's not clear to me\n> from their page:\n>\n> https://docs.travis-ci.com/user/workers/container-based-infrastructure/\n>\n> whether we're using the new, faster container infrastructure or not.\n> ...\n> depends on when Travis \"recognized\" the repo, but I'm not quite sure\n> what that means. Should we be adding \"sudo: false\" to the top-level of\n> the yaml file?\n\nIn an earlier discussion\n\n http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279495\n\nI found that we were not eligible for container-based sandbox as the\nversion of travis-yaml back then used \"sudo\". I do not seem to find\nthe use of sudo in the recent one we have in my tree, so it would be\nbeneficial if somebody interested in Travis CI look into this.\n"},{"id":"276379","messageId":"20160119230633.GA31142@sigill.intra.peff.net","threadId":"41216","inReplyTo":"xmqqio2p89mb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T23:06:33Z","receivedAt":"2016-01-19T23:06:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 02:53:00PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > This is cute, but isn't it useful even outside Travis's context? I\n> > am not suggesting to touch anything other than .travis.yml file in\n> > this patch, but if I wanted to get the benefit from the idea in this\n> > patch when I run my tests manually, I can just tell prove to use the\n> > cached states, no?\n> \n> It seems that exporting something like\n> \n> GIT_PROVE_OPTS=\"--timer --state=slow,save -j8\" \n> \n> when running \"make DEFAULT_TEST_TARGET=prove test\" does give me the\n> same benefit by leaving the stats from the previous run in t/.prove\n> when making the test scheduling decisions.\n\nYes, I've been using this on my local machine for years (which is why I\nsuggested it to Lars for the Travis build). I have also noticed that my\ntest runs take about as much time as the longest-running test, and do\nnot fully utilize all of my processors. I suspect we could drop the\nrun-time of the test suite substantially by splitting a few of the\nlonger tests.\n\nYou also wrote earlier:\n\n> IOW, I am confused by the beginning of the log message that says\n> this is taking advantage of \"the Travis-CI cache feature\". This\n> improvement looks to me like using the feature of \"prove\" that\n> allows us to run slower tests first, and does not have much to do\n> with Travis.\n\nThe interesting Travis feature we are using is that we are allowed to\nstore some data from run-to-run. So we use the Travis feature that lets\nus use the prove feature. :)\n\n> One thing I noticed but didn't dig further to fix was that this\n> \"prove --state\" business did not seem to work well together with\n> \n> make T=\"...list of tests...\" test\n> \n> that limits the set of tests to perform.\n\nRight, it does not do what you want. The \"prove --state\" feature is not\njust about ordering, but also about selecting. When run via \"make\", we\nalways give prove the full list of tests. But you can also do:\n\n prove --state=failed\n\nmanually to just run whatever failed on the last run.\n\nI don't know if there is a way to tell prove \"use the state for\nordering, but don't otherwise select from it\", which would do what you\nwant above.\n\nYou can also note that if we ever delete a test script, it will still be\nmentioned in prove's state file. I think prove is smart enough to\nrealize it went away and not bother you.\n\n-Peff\n"},{"id":"276381","messageId":"xmqq60yp8837.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160119230633.GA31142@sigill.intra.peff.net","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T23:26:04Z","receivedAt":"2016-01-19T23:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You can also note that if we ever delete a test script, it will still be\n> mentioned in prove's state file. I think prove is smart enough to\n> realize it went away and not bother you.\n\nThe inverse might be more problematic. When we add a new test\nscript (which we still do from time to time), does prove notice\nthat we asked it to run more tests than it already knows about?\n"},{"id":"276383","messageId":"20160119232710.GA31181@sigill.intra.peff.net","threadId":"41216","inReplyTo":"20160119230633.GA31142@sigill.intra.peff.net","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T23:27:10Z","receivedAt":"2016-01-19T23:27:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 06:06:33PM -0500, Jeff King wrote:\n\n> > It seems that exporting something like\n> > \n> > GIT_PROVE_OPTS=\"--timer --state=slow,save -j8\" \n> > \n> > when running \"make DEFAULT_TEST_TARGET=prove test\" does give me the\n> > same benefit by leaving the stats from the previous run in t/.prove\n> > when making the test scheduling decisions.\n> \n> Yes, I've been using this on my local machine for years (which is why I\n> suggested it to Lars for the Travis build). I have also noticed that my\n> test runs take about as much time as the longest-running test, and do\n> not fully utilize all of my processors. I suspect we could drop the\n> run-time of the test suite substantially by splitting a few of the\n> longer tests.\n\nHere are the numbers for that:\n\n $ time make ;# configured to use prove --state=slow,save -j16\n [...]\n real 0m47.035s\n user 1m6.884s\n sys 0m19.892s\n\n $ grep -v '^\\.\\.\\.' .prove |\n perl -MYAML -e '\n local $/;\n $x = YAML::Load(<>)->{tests};\n print int($x->{$_}->{elapsed}), \" $_\\n\" for keys(%$x)\n ' |\n sort -rn |\n head\n 39 t3404-rebase-interactive.sh\n 29 t3421-rebase-topology-linear.sh\n 27 t9001-send-email.sh\n 16 t9500-gitweb-standalone-no-errors.sh\n 15 t3425-rebase-topology-merges.sh\n 14 t6030-bisect-porcelain.sh\n 13 t7610-mergetool.sh\n 13 t5572-pull-submodule.sh\n 13 t3426-rebase-submodule.sh\n 12 t3415-rebase-autosquash.sh\n\nSo we're running t3404 for the majority of the time. I guess that\ndoesn't tell us how full our pipelines are for the rest of the time,\nthough. It could be worth splitting some of those long tests and seeing\nif that improves run-time, though.\n\n-Peff\n"},{"id":"276384","messageId":"20160119232950.GB31181@sigill.intra.peff.net","threadId":"41216","inReplyTo":"xmqq60yp8837.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T23:29:50Z","receivedAt":"2016-01-19T23:29:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 03:26:04PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > You can also note that if we ever delete a test script, it will still be\n> > mentioned in prove's state file. I think prove is smart enough to\n> > realize it went away and not bother you.\n> \n> The inverse might be more problematic. When we add a new test\n> script (which we still do from time to time), does prove notice\n> that we asked it to run more tests than it already knows about?\n\nYes. It runs the union of the state-file and what you give it on the\ncommand line. E.g.:\n\n $ rm .prove\n $ prove --state=slow,save t0000-basic.sh\n No saved state, selection will be empty\n t0000-basic.sh .. ok \n All tests successful.\n Files=1, Tests=77, 1 wallclock secs ( 0.03 usr 0.00 sys + 0.08 cusr 0.06 csys = 0.17 CPU)\n Result: PASS\n\n $ prove --state=slow,save t0001-init.sh\n t0000-basic.sh .. ok \n t0001-init.sh ... ok \n All tests successful.\n Files=2, Tests=113, 1 wallclock secs ( 0.03 usr 0.00 sys + 0.06 cusr 0.10 csys = 0.19 CPU)\n Result: PASS\n\n-Peff\n"},{"id":"276385","messageId":"xmqq1t9d87vx.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"xmqq60yp8837.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T23:30:26Z","receivedAt":"2016-01-19T23:30:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> You can also note that if we ever delete a test script, it will still be\n>> mentioned in prove's state file. I think prove is smart enough to\n>> realize it went away and not bother you.\n>\n> The inverse might be more problematic. When we add a new test\n> script (which we still do from time to time), does prove notice\n> that we asked it to run more tests than it already knows about?\n\nHeh, I should have tested before sending it out. It seems that it\ndoes notice what's missing from t/.prove so it is safe.\n"},{"id":"276389","messageId":"20160120002606.GA9359@glandium.org","threadId":"41216","inReplyTo":"xmqqegdd8997.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-01-20T00:26:06Z","receivedAt":"2016-01-20T00:26:06Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:\n> Jeff King writes:\n> \n> > On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:\n> >\n> >> From: Lars Schneider \n> >> \n> >> Use the Travis-CI cache feature to store prove test results and make them\n> >> available in subsequent builds. This allows to run previously failed tests\n> >> first and run remaining tests in slowest to fastest order. As a result it\n> >> is less likely that Travis-CI needs to wait for a single test at the end\n> >> which speeds up the test suite execution by ~2 min.\n> >\n> > Thanks, this makes sense, and the patch looks good.\n> >\n> >> @@ -18,7 +22,7 @@ env:\n> >> - P4_VERSION=\"15.2\"\n> >> - GIT_LFS_VERSION=\"1.1.0\"\n> >> - DEFAULT_TEST_TARGET=prove\n> >> - - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n> >> + - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n> >\n> > Have you tried bumping --jobs here? I usually use \"16\" on my local box.\n> \n> I think 3 comes from this:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674\n\nHaving recently looked into this, the relevant travis-ci documentation\nis:\nhttps://docs.travis-ci.com/user/ci-environment/\n\nwhich says all environments have 2 cores, so you won't get much from\nanything higher than -j3.\n\nThe following document also says something slightly different:\nhttps://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM\n\n\"Travis CI VMs run on 1.5 virtual cores.\"\n\n> > I also looked into the Travis \"container\" thing. It's not clear to me\n> > from their page:\n> >\n> > https://docs.travis-ci.com/user/workers/container-based-infrastructure/\n> >\n> > whether we're using the new, faster container infrastructure or not.\n> > ...\n> > depends on when Travis \"recognized\" the repo, but I'm not quite sure\n> > what that means. Should we be adding \"sudo: false\" to the top-level of\n> > the yaml file?\n> \n> In an earlier discussion\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279495\n> \n> I found that we were not eligible for container-based sandbox as the\n> version of travis-yaml back then used \"sudo\". I do not seem to find\n> the use of sudo in the recent one we have in my tree, so it would be\n> beneficial if somebody interested in Travis CI look into this.\n\nThe https://docs.travis-ci.com/user/ci-environment/ document says the\ndefault is \"sudo: false\" for repositories enabled in 2015 or later, which\nI assume is the case for the git repository. \"sudo: required\" is the\ndefault for repositories enabled before 2015.\n\nMike\n"},{"id":"276395","messageId":"xmqqfuxt6n00.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160120002606.GA9359@glandium.org","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-20T01:46:55Z","receivedAt":"2016-01-20T01:46:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:\n>\n>> I think 3 comes from this:\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674\n>\n> Having recently looked into this, the relevant travis-ci documentation\n> is:\n> https://docs.travis-ci.com/user/ci-environment/\n>\n> which says all environments have 2 cores, so you won't get much from\n> anything higher than -j3.\n>\n> The following document also says something slightly different:\n> https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM\n>\n> \"Travis CI VMs run on 1.5 virtual cores.\"\n\nYup, that 1.5 was already mentioned in the earlier thread, but many\ntests are mostly I/O bound, so 1.5 (or 2 for that matter) does not\nmean we should not go higher than -j2 or -j3. What I meant was that\nthe 3 comes from the old discussion \"let's be nice to those who\noffer this to us for free\".\n"},{"id":"276397","messageId":"20160120015310.GB24541@sigill.intra.peff.net","threadId":"41216","inReplyTo":"20160120002606.GA9359@glandium.org","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-20T01:53:10Z","receivedAt":"2016-01-20T01:53:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2016 at 09:26:06AM +0900, Mike Hommey wrote:\n\n> Having recently looked into this, the relevant travis-ci documentation\n> is:\n> https://docs.travis-ci.com/user/ci-environment/\n> \n> which says all environments have 2 cores, so you won't get much from\n> anything higher than -j3.\n\nFWIW, I settled on \"-j16\" on my 8-core (well, hyperthreaded quad-core)\nmachine after experimenting. That's running the tests on a RAM-disk,\nthough. On a slower filesystem where fsync() actually does something,\nyou're going to get a lot more I/O stalls, and want a bigger CPU to\nprocess multiplier.\n\nHere are actual numbers from my machine:\n\n -j | time (user+sys)\n ---+------------------\n 1 | 5m18s (41s+17s)\n 2 | 2m24s (41s+14s)\n 4 | 1m15s (46s+13s)\n 8 | 0m56s (65s+18s)\n 16 | 0m53s (76s+24s)\n 32 | 0m57s (78s+25s)\n\nNote that the CPU-second times will go up with more threads because of\nthe frequency scaling.\n\nSo yeah, -j3 might not be that unreasonable, depending on the filesystem\nresponse times.\n\n> The https://docs.travis-ci.com/user/ci-environment/ document says the\n> default is \"sudo: false\" for repositories enabled in 2015 or later, which\n> I assume is the case for the git repository. \"sudo: required\" is the\n> default for repositories enabled before 2015.\n\nThanks. The document I saw used the word \"recognized\", and I didn't\nquite know what they meant. We just enabled this a month or two ago, so\nwe should be running on the new format.\n\n-Peff\n"},{"id":"276398","messageId":"20160120015602.GC24541@sigill.intra.peff.net","threadId":"41216","inReplyTo":"xmqqfuxt6n00.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-20T01:56:03Z","receivedAt":"2016-01-20T01:56:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 05:46:55PM -0800, Junio C Hamano wrote:\n\n> > \"Travis CI VMs run on 1.5 virtual cores.\"\n> \n> Yup, that 1.5 was already mentioned in the earlier thread, but many\n> tests are mostly I/O bound, so 1.5 (or 2 for that matter) does not\n> mean we should not go higher than -j2 or -j3. What I meant was that\n> the 3 comes from the old discussion \"let's be nice to those who\n> offer this to us for free\".\n\nI am very appreciative that we can use Travis for free, but I doubt they\ncare much one way or the other how we parallelize. Everything is\nsandboxed enough that we should not be able to cause problems for them\nor other customers. It's all CPU seconds to them (or should be, anyway).\n\nThe thing that _would_ probably bother them is throwing too many builds\nat it (right now we are building the integration branches; it would be\nuseful information to build individual topics, too, but that would\nincrease the number of CPU seconds we ask them for).\n\n-Peff\n"},{"id":"276415","messageId":"alpine.DEB.2.20.1601200846010.2964@virtualbox","threadId":"41216","inReplyTo":"xmqqmvs19w5n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-01-20T07:55:20Z","receivedAt":"2016-01-20T07:55:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 19 Jan 2016, Junio C Hamano wrote:\n\n> larsxschneider@gmail.com writes:\n> \n> > From: Lars Schneider \n> >\n> > Use the Travis-CI cache feature to store prove test results and make\n> > them available in subsequent builds. This allows to run previously\n> > failed tests first and run remaining tests in slowest to fastest\n> > order. As a result it is less likely that Travis-CI needs to wait for\n> > a single test at the end which speeds up the test suite execution by\n> > ~2 min.\n> >\n> > Unfortunately the cache feature is only available (for free) on the\n> > Travis-CI Linux environment.\n> >\n> > Suggested-by: Jeff King \n> > Signed-off-by: Lars Schneider \n> > ---\n> > .travis.yml | 8 +++++++-\n> > 1 file changed, 7 insertions(+), 1 deletion(-)\n> \n> This is cute, but isn't it useful even outside Travis's context? I am\n> not suggesting to touch anything other than .travis.yml file in this\n> patch, but if I wanted to get the benefit from the idea in this patch\n> when I run my tests manually, I can just tell prove to use the cached\n> states, no?\n\nYou are basically talking about prove's support to modify behavior based\non previous runs. This patch does not introduce that support, it was\nintroduced long ago: 5099b99 (test-lib: Adjust output to be valid TAP\nformat, 2010-06-24). This commit also advertises that feature in t/README.\n\n> IOW, I am confused by the beginning of the log message that says\n> this is taking advantage of \"the Travis-CI cache feature\".\n\nLars' patch is really about Travis and its useful feature to retain state\nfrom previous runs. AFAICT this is the feature Lars is talking about, and\nit is absolutely necessary to make use of this prove feature (don't try\nthis with BuildHive, for example, its workspaces are typically\ngarbage-collected before the next CI run).\n\nAs such, I think it makes sense to talk about the Travis-CI feature in the\ncommit message (and not about the prove feature because we introduced\nsupport for prove much, much earlier in Git's history, and advertised its\nbenefits at that time, as I stated above).\n\nCiao,\nDscho\n"},{"id":"276416","messageId":"711209F5-E034-459E-8E85-BF8BC32B2E86@gmail.com","threadId":"41216","inReplyTo":"xmqqmvs19w5n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-20T09:04:44Z","receivedAt":"2016-01-20T09:04:44Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 19 Jan 2016, at 21:00, Junio C Hamano wrote:\n\n> larsxschneider@gmail.com writes:\n> \n>> From: Lars Schneider \n>> \n>> Use the Travis-CI cache feature to store prove test results and make them\n>> available in subsequent builds. This allows to run previously failed tests\n>> first and run remaining tests in slowest to fastest order. As a result it\n>> is less likely that Travis-CI needs to wait for a single test at the end\n>> which speeds up the test suite execution by ~2 min.\n>> \n>> Unfortunately the cache feature is only available (for free) on the\n>> Travis-CI Linux environment.\n>> \n>> Suggested-by: Jeff King \n>> Signed-off-by: Lars Schneider \n>> ---\n>> .travis.yml | 8 +++++++-\n>> 1 file changed, 7 insertions(+), 1 deletion(-)\n> \n> This is cute, but isn't it useful even outside Travis's context? I\n> am not suggesting to touch anything other than .travis.yml file in\n> this patch, but if I wanted to get the benefit from the idea in this\n> patch when I run my tests manually, I can just tell prove to use the\n> cached states, no?\n> \n> IOW, I am confused by the beginning of the log message that says\n> this is taking advantage of \"the Travis-CI cache feature\". This\n> improvement looks to me like using the feature of \"prove\" that\n> allows us to run slower tests first, and does not have much to do\n> with Travis.\n> \n> You are relying on the assumption that things under $HOME/ is stable\n> while things under t/ (or in our source tree in general) are not,\n> and I think that is a sensible thing to take advantage of, but are\n> we sure that they are running in an environment where \"ln -s\" would\n> work? Otherwise, it may be more robust to copy $HOME/.prove to\n> t/.prove before starting to test and then copy it back once the\n> tests are done.\n\nOK, looks like my wording was not ideal. One important thing to know is that \n$HOME is *not* stable. These TravisCI machines start *always* in a completely \nclean state. That's why prove cannot store and use it's cache. With the following \nstatement I instruct Travis to cache my \".prove-cache\" directory. As a consequence\nTravis CI will automatically restore this directory whenever it starts a new instance\nfor the git job. It will also save the content of this directory when the job is done.\n\n>> +cache:\n>> + directories:\n>> + - $HOME/.prove-cache\n\nThe Travis CI cache works only on a directory basis. Since I don't want to cache\nthe entire /t directory I came up with the $HOME/.prove-cache directory. I also used\na file link to leverage the automated save/restore feature for the $HOME/.prove-cache\ndirectory. If I would not use a link then I would need to copy the updated .prove file \nfrom t/ to .prove-cache after the test run.\n\nWould the following first sentence for the commit message be less ambiguous?\n\n\"Use the Travis-CI cache feature to make the prove test results cache persistent \nacross subsequent build jobs. This allows to run previously...\"\n\nThanks,\nLars\n\n\n>> \n>> diff --git a/.travis.yml b/.travis.yml\n>> index c3bf9c6..f34726b 100644\n>> --- a/.travis.yml\n>> +++ b/.travis.yml\n>> @@ -1,5 +1,9 @@\n>> language: c\n>> \n>> +cache:\n>> + directories:\n>> + - $HOME/.prove-cache\n>> +\n>> os:\n>> - linux\n>> - osx\n>> @@ -18,7 +22,7 @@ env:\n>> - P4_VERSION=\"15.2\"\n>> - GIT_LFS_VERSION=\"1.1.0\"\n>> - DEFAULT_TEST_TARGET=prove\n>> - - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n>> + - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n>> - GIT_TEST_OPTS=\"--verbose --tee\"\n>> - CFLAGS=\"-g -O2 -Wall -Werror\"\n>> - GIT_TEST_CLONE_2GB=YesPlease\n>> @@ -67,6 +71,8 @@ before_install:\n>> p4 -V | grep Rev.;\n>> echo \"$(tput setaf 6)Git-LFS Version$(tput sgr0)\";\n>> git-lfs version;\n>> + mkdir -p $HOME/.prove-cache;\n>> + ln -s $HOME/.prove-cache/.prove t/.prove;\n>> \n>> before_script: make --jobs=2\n>> \n>> --\n>> 2.5.1\n"},{"id":"276417","messageId":"9ED87AF7-5880-4E40-8859-0DA0F1DF1883@gmail.com","threadId":"41216","inReplyTo":"20160120002606.GA9359@glandium.org","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-20T09:10:28Z","receivedAt":"2016-01-20T09:10:28Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 20 Jan 2016, at 01:26, Mike Hommey wrote:\n\n> On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:\n>> Jeff King writes:\n>> \n>>> On Tue, Jan 19, 2016 at 10:24:29AM +0100, larsxschneider@gmail.com wrote:\n>>> \n>>>> From: Lars Schneider \n>>>> \n>>>> Use the Travis-CI cache feature to store prove test results and make them\n>>>> available in subsequent builds. This allows to run previously failed tests\n>>>> first and run remaining tests in slowest to fastest order. As a result it\n>>>> is less likely that Travis-CI needs to wait for a single test at the end\n>>>> which speeds up the test suite execution by ~2 min.\n>>> \n>>> Thanks, this makes sense, and the patch looks good.\n>>> \n>>>> @@ -18,7 +22,7 @@ env:\n>>>> - P4_VERSION=\"15.2\"\n>>>> - GIT_LFS_VERSION=\"1.1.0\"\n>>>> - DEFAULT_TEST_TARGET=prove\n>>>> - - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n>>>> + - GIT_PROVE_OPTS=\"--timer --jobs 3 --state=failed,slow,save\"\n>>> \n>>> Have you tried bumping --jobs here? I usually use \"16\" on my local box.\n>> \n>> I think 3 comes from this:\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674\n> \n> Having recently looked into this, the relevant travis-ci documentation\n> is:\n> https://docs.travis-ci.com/user/ci-environment/\n> \n> which says all environments have 2 cores, so you won't get much from\n> anything higher than -j3.\n> \n> The following document also says something slightly different:\n> https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM\n> \n> \"Travis CI VMs run on 1.5 virtual cores.\"\n> \n>>> I also looked into the Travis \"container\" thing. It's not clear to me\n>>> from their page:\n>>> \n>>> https://docs.travis-ci.com/user/workers/container-based-infrastructure/\n>>> \n>>> whether we're using the new, faster container infrastructure or not.\n>>> ...\n>>> depends on when Travis \"recognized\" the repo, but I'm not quite sure\n>>> what that means. Should we be adding \"sudo: false\" to the top-level of\n>>> the yaml file?\n>> \n>> In an earlier discussion\n>> \n>> http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279495\n>> \n>> I found that we were not eligible for container-based sandbox as the\n>> version of travis-yaml back then used \"sudo\". I do not seem to find\n>> the use of sudo in the recent one we have in my tree, so it would be\n>> beneficial if somebody interested in Travis CI look into this.\n> \n> The https://docs.travis-ci.com/user/ci-environment/ document says the\n> default is \"sudo: false\" for repositories enabled in 2015 or later, which\n> I assume is the case for the git repository. \"sudo: required\" is the\n> default for repositories enabled before 2015.\n> \n\nI made the Git job run on the new container-based infrastructure for Linux.\nWe can add \"sudo: false\" to make this more explicit!\n\nThanks,\nLars\n"},{"id":"276418","messageId":"DBA834D2-BFC9-4A2F-94D9-A1D0D60377BD@gmail.com","threadId":"41216","inReplyTo":"xmqqfuxt6n00.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-01-20T09:22:16Z","receivedAt":"2016-01-20T09:22:16Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 20 Jan 2016, at 02:46, Junio C Hamano wrote:\n\n> Mike Hommey writes:\n> \n>> On Tue, Jan 19, 2016 at 03:00:52PM -0800, Junio C Hamano wrote:\n>> \n>>> I think 3 comes from this:\n>>> \n>>> http://thread.gmane.org/gmane.comp.version-control.git/279348/focus=279674\n>> \n>> Having recently looked into this, the relevant travis-ci documentation\n>> is:\n>> https://docs.travis-ci.com/user/ci-environment/\n>> \n>> which says all environments have 2 cores, so you won't get much from\n>> anything higher than -j3.\n>> \n>> The following document also says something slightly different:\n>> https://docs.travis-ci.com/user/speeding-up-the-build#Parallelizing-your-build-on-one-VM\n>> \n>> \"Travis CI VMs run on 1.5 virtual cores.\"\n> \n> Yup, that 1.5 was already mentioned in the earlier thread, but many\n> tests are mostly I/O bound, so 1.5 (or 2 for that matter) does not\n> mean we should not go higher than -j2 or -j3. What I meant was that\n> the 3 comes from the old discussion \"let's be nice to those who\n> offer this to us for free\".\n\nI tested different settings and found that running prove with \"-j5\" seems to be\nthe fastest option for the Travis CI machines. However, I also noticed that \nI got more test failures with higher parallelism (Dscho reported similar \nobservations [1]).\n\nEspecially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X builds\nwhen I increase the parallelism:\n\nnot ok 4 - text=true causes a CRLF file to be normalized \nnot ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files \n\nAnyone an idea why that might be the case?\n\nThanks,\nLars\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/279660\n[2] https://travis-ci.org/larsxschneider/git/jobs/103461538\n[3] https://travis-ci.org/larsxschneider/git/jobs/103461458"},{"id":"276450","messageId":"xmqqfuxs56qb.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"711209F5-E034-459E-8E85-BF8BC32B2E86@gmail.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-20T20:35:56Z","receivedAt":"2016-01-20T20:35:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> On 19 Jan 2016, at 21:00, Junio C Hamano wrote:\n>\n>> IOW, I am confused by the beginning of the log message that says\n>> this is taking advantage of \"the Travis-CI cache feature\". This\n>> improvement looks to me like using the feature of \"prove\" that\n>> allows us to run slower tests first, and does not have much to do\n>> with Travis.\n>> \n>> You are relying on the assumption that things under $HOME/ is stable\n>> while things under t/ (or in our source tree in general) are not,\n>> and I think that is a sensible thing to take advantage of, but are\n>> we sure that they are running in an environment where \"ln -s\" would\n>> work? Otherwise, it may be more robust to copy $HOME/.prove to\n>> t/.prove before starting to test and then copy it back once the\n>> tests are done.\n>\n> OK, looks like my wording was not ideal. One important thing to know is that \n> $HOME is *not* stable. These TravisCI machines start *always* in a completely \n> clean state.\n\nAh, that is what I missed. Travis makes everything transient by\ndefault (which is a sensible thing to do for CI), but it lets you\ndeclare some things are to be made stable, and that is the \"cache\"\nfeature you are taking advantage of in Travis.\n\nThe log message needs to be clarified in a reroll, but thanks for\nclarifying it for me in advance ;-)\n\nThat only leaves one question from me: Is 'ln -s' safe enough?\nWould copying back and forth make it more robust?\n\nI am guessint the answers are Yes and No, in which case the patch\ntext can (and should) stay as-is.\n\nThanks.\n"},{"id":"276527","messageId":"20160122023359.GA686558@vauxhall.crustytoothpaste.net","threadId":"41216","inReplyTo":"DBA834D2-BFC9-4A2F-94D9-A1D0D60377BD@gmail.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2016-01-22T02:33:59Z","receivedAt":"2016-01-22T02:33:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, Jan 20, 2016 at 10:22:16AM +0100, Lars Schneider wrote:\n> I tested different settings and found that running prove with \"-j5\" seems to be\n> the fastest option for the Travis CI machines. However, I also noticed that\n> I got more test failures with higher parallelism (Dscho reported similar\n> observations [1]).\n> \n> Especially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X builds\n> when I increase the parallelism:\n> \n> not ok 4 - text=true causes a CRLF file to be normalized\n> not ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files\n> \n> Anyone an idea why that might be the case?\n\nI've seen this on my personal box too[0] when running make -j4 all test.\nI wasn't able to pin down why it was occurring, but if we're going to\nrun the tests in parallel, it's probably worth spending some time\nfiguring it out.\n\n[0] Debian amd64/sid, ThinkPad X220.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"276530","messageId":"20160122055255.GA14657@sigill.intra.peff.net","threadId":"41216","inReplyTo":"20160122023359.GA686558@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-22T05:52:55Z","receivedAt":"2016-01-22T05:52:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 22, 2016 at 02:33:59AM +0000, brian m. carlson wrote:\n\n> > Especially t0025-crlf-auto.sh failed multiple times ([2], [3]) on the OS X builds\n> > when I increase the parallelism:\n> > \n> > not ok 4 - text=true causes a CRLF file to be normalized\n> > not ok 9 - text=auto, autocrlf=true _does_ normalize CRLF files\n> > \n> > Anyone an idea why that might be the case?\n> \n> I've seen this on my personal box too[0] when running make -j4 all test.\n> I wasn't able to pin down why it was occurring, but if we're going to\n> run the tests in parallel, it's probably worth spending some time\n> figuring it out.\n\nInteresting. I run the test suite in parallel probably a dozen times\nper day, and I've never seen this. However, I was able to trigger it\neventually with:\n\n for i in 1 2 3 4 5 6 7 8\n do\n (while ./t0025-crlf-auto.sh --root=/var/ram/git-tests/foo-$i -v -i >/tmp/foo-$i 2>&1\n do\n : nothing\n done\n echo FAILED $i\n ) &\n done\n\nI get a few of the threads failing (in test 4) after 2-3 minutes. The\n\"-v\" output is pretty unenlightening, though. I don't see anything\nracy-looking in the test unless it is something with \"read-tree\" and\nstat mtimes.\n\n-Peff\n"},{"id":"276531","messageId":"20160122060720.GA15681@sigill.intra.peff.net","threadId":"41216","inReplyTo":"20160122055255.GA14657@sigill.intra.peff.net","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-22T06:07:20Z","receivedAt":"2016-01-22T06:07:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 22, 2016 at 12:52:55AM -0500, Jeff King wrote:\n\n> I get a few of the threads failing (in test 4) after 2-3 minutes. The\n> \"-v\" output is pretty unenlightening, though. I don't see anything\n> racy-looking in the test unless it is something with \"read-tree\" and\n> stat mtimes.\n\nAnd indeed, doing this:\n\ndiff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh\nindex c164b46..d34775b 100755\n--- a/t/t0025-crlf-auto.sh\n+++ b/t/t0025-crlf-auto.sh\n@@ -56,6 +56,7 @@ test_expect_success 'text=true causes a CRLF file to be normalized' '\n \n \trm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&\n \techo \"CRLFonly text\" > .gitattributes &&\n+\tsleep 1 &&\n \tgit read-tree --reset -u HEAD &&\n \n \t# Note, \"normalized\" means that git will normalize it if added\n\nlet me run for over 5 minutes with no failures in test 4 (I eventually\ndid hit one in test 9). I don't claim to understand what is going on,\nthough.\n\n-Peff\n"},{"id":"276629","messageId":"20160124143403.GL7100@hank","threadId":"41216","inReplyTo":"20160122060720.GA15681@sigill.intra.peff.net","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-01-24T14:34:03Z","receivedAt":"2016-01-24T14:34:03Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 01/22, Jeff King wrote:\n> On Fri, Jan 22, 2016 at 12:52:55AM -0500, Jeff King wrote:\n>\n> > I get a few of the threads failing (in test 4) after 2-3 minutes. The\n> > \"-v\" output is pretty unenlightening, though. I don't see anything\n> > racy-looking in the test unless it is something with \"read-tree\" and\n> > stat mtimes.\n>\n> And indeed, doing this:\n>\n> diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh\n> index c164b46..d34775b 100755\n> --- a/t/t0025-crlf-auto.sh\n> +++ b/t/t0025-crlf-auto.sh\n> @@ -56,6 +56,7 @@ test_expect_success 'text=true causes a CRLF file to be normalized' '\n>\n> \trm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&\n> \techo \"CRLFonly text\" > .gitattributes &&\n> +\tsleep 1 &&\n> \tgit read-tree --reset -u HEAD &&\n>\n> \t# Note, \"normalized\" means that git will normalize it if added\n>\n> let me run for over 5 minutes with no failures in test 4 (I eventually\n> did hit one in test 9). I don't claim to understand what is going on,\n> though.\n\nI don't think this is the right solution though, I think this mostly\nlessens the load on the filesystem so the flakiness doesn't occur,\nespecially on your system, where it seems hard to trigger in the first\nplace :)\n\nI actually hit the same problem occasionally when running the test\nsuite before, but was always to lazy to try to reproduce it. Thanks\nto your reproduction I think I was able to track the underlying\nproblem down.\n\nMy analysis is in the commit message below.\n\n--->8---\nSubject: [PATCH] entry: fix up to date marking\n\nwrite_entry always marks cache entries up to date when\nstate->refresh_cache is set. This is however not always accurate,\nif core.autocrlf is set in the config, a file with cr and lf line\nendings exists and the file attribute is set to text or crlf in the\ngitattributes.\n\nMost notably this makes t0025 flaky. When calling deleting the files\nthat will be adjusted through the automated crlf handling, and then\ncalling `git read-tree --reset -u HEAD`, this leads to a race between\ngit read-tree and the filesystem. The test currently only passes\nmost of the time, because the filesystem usually isn't synced between\nthe call to unpack_trees() and write_locked_index().\n\nCurrently the sequence of 1) remove files with cr and lf as line\nendings, 2) `git read-tree --reset -u HEAD` 3) checking the status of\nthe changed files succeeds, because the index and the files are written\nat the same time, so they have the same mtime. Thus when reading the\nindex the next time, the files are recognized as racy, and the actual\ncontents on the disk are checked for changes.\n\nIf the index and the files have different mtimes however, the entry is\nwritten to the index as up to date because of the flag set in\nentry.c:write_entry(), and the contents on the filesystem are not\nactually checked again, because the stat data in the index matches.\n\nThe failures in t0025 can be consistently reproduced by introducing a\ncall to sync() between the call to unpack_trees() and\nwrite_index_locked().\n\nInstead of blindly marking and entry up to date in write_entry(), check\nif the contents may change on disk first, and strip the CE_UPTODATE flag\nin that case. Because the flag is not set, the cache entry will go\nthrough the racy check when writing the index the first time, and\nsmudged if appropriate.\n\nThis fixes the flaky test as well as the underlying problem.\n\nSigned-off-by: Thomas Gummerer \n---\n entry.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/entry.c b/entry.c\nindex 582c400..102fdfa 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -214,6 +214,8 @@ finish:\n \t\tif (!fstat_done)\n \t\t\tlstat(ce->name, &st);\n \t\tfill_stat_cache_info(ce, &st);\n+\t\tif (would_convert_to_git(ce->name))\n+\t\t\tce->ce_flags &= ~CE_UPTODATE;\n \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\n \t}\n--\n2.7.0.75.g3ee1e0f.dirty\n"},{"id":"276672","messageId":"xmqqd1sqd9sq.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160124143403.GL7100@hank","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-24T20:03:49Z","receivedAt":"2016-01-24T20:03:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> My analysis is in the commit message below.\n>\n> --->8---\n> Subject: [PATCH] entry: fix up to date marking\n>\n> write_entry always marks cache entries up to date when\n> state->refresh_cache is set. This is however not always accurate,\n> if core.autocrlf is set in the config, a file with cr and lf line\n> endings exists and the file attribute is set to text or crlf in the\n> gitattributes.\n>\n> Most notably this makes t0025 flaky. When calling deleting the files\n> that will be adjusted through the automated crlf handling, and then\n> calling `git read-tree --reset -u HEAD`, this leads to a race between\n> git read-tree and the filesystem. The test currently only passes\n> most of the time, because the filesystem usually isn't synced between\n> the call to unpack_trees() and write_locked_index().\n>\n> Currently the sequence of 1) remove files with cr and lf as line\n> endings, 2) `git read-tree --reset -u HEAD` 3) checking the status of\n> the changed files succeeds, because the index and the files are written\n> at the same time, so they have the same mtime. Thus when reading the\n> index the next time, the files are recognized as racy, and the actual\n> contents on the disk are checked for changes.\n>\n> If the index and the files have different mtimes however, the entry is\n> written to the index as up to date because of the flag set in\n> entry.c:write_entry(), and the contents on the filesystem are not\n> actually checked again, because the stat data in the index matches.\n>\n> The failures in t0025 can be consistently reproduced by introducing a\n> call to sync() between the call to unpack_trees() and\n> write_index_locked().\n>\n> Instead of blindly marking and entry up to date in write_entry(), check\n> if the contents may change on disk first, and strip the CE_UPTODATE flag\n> in that case. Because the flag is not set, the cache entry will go\n> through the racy check when writing the index the first time, and\n> smudged if appropriate.\n\nSorry, but I am confused by all of the above.\n\nWe write the thing out with write_entry(), possibly applying smudge\nfilters and eol conversion to the \"git\" representation to create the\n\"working tree\" representation in this codepath, right? The resulting\nfile matches what the user's configuration told us to produce.\n\nUntil the working tree file is changed by somebody after the above\nhappens, we shouldn't have to check the contents of the file to see\nif there is a difference. By definition, that has to match the\ncontents expected to be there by Git.\n\nThe only case I can think of that the above does not hold is when\nthe smuge/clean and the eol conversion are not a correct round-trip\noperation pairs, but that would be a misconfiguration. Otherwise,\nwe'd be _always_ comparing the contents without relying on the\ncached stat info for any paths whose in-core and working tree\nrepresentations are different, not just those that are configured\nwith misbehaving smudge/clean pair, no?\n\nPuzzled... In this case, my hunch says that the patch is correct,\nyour analysis also is and it is only me who is missing some crucial\nbits in the analysis and getting confused.\n\nEnlightenment, please?\n\n>\n> This fixes the flaky test as well as the underlying problem.\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n> entry.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/entry.c b/entry.c\n> index 582c400..102fdfa 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -214,6 +214,8 @@ finish:\n> \t\tif (!fstat_done)\n> \t\t\tlstat(ce->name, &st);\n> \t\tfill_stat_cache_info(ce, &st);\n> +\t\tif (would_convert_to_git(ce->name))\n> +\t\t\tce->ce_flags &= ~CE_UPTODATE;\n> \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n> \t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\n> \t}\n> --\n> 2.7.0.75.g3ee1e0f.dirty\n"},{"id":"276676","messageId":"xmqq8u3ed45r.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"xmqqd1sqd9sq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-24T22:05:36Z","receivedAt":"2016-01-24T22:05:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Sorry, but I am confused by all of the above.\n>\n> We write the thing out with write_entry(), possibly applying smudge\n> filters and eol conversion to the \"git\" representation to create the\n> \"working tree\" representation in this codepath, right? The resulting\n> file matches what the user's configuration told us to produce.\n>\n> Until the working tree file is changed by somebody after the above\n> happens, we shouldn't have to check the contents of the file to see\n> if there is a difference. By definition, that has to match the\n> contents expected to be there by Git.\n>\n> The only case I can think of that the above does not hold is when\n> the smuge/clean and the eol conversion are not a correct round-trip\n> operation pairs, but that would be a misconfiguration. Otherwise,\n> we'd be _always_ comparing the contents without relying on the\n> cached stat info for any paths whose in-core and working tree\n> representations are different, not just those that are configured\n> with misbehaving smudge/clean pair, no?\n\nTo put it differently, if a blob stored at path has CRLF line\nendings and .gitattributes is changed after the fact to say that it\nmust have LF line endings, we should treat it as a broken transitory\nstate. There may have to be a way to \"fix\" an already \"wrong\" blob\nin the index that is milder than \"rm --cached && add .\", but I do\nnot think write_entry(), which is shared by all the normal codepaths\nthat writes out to the working tree, is the best place to do so, if\ndoing so forces the contents of the paths to be always re-checked,\njust in case the user is in such a broken transitory state.\n"},{"id":"276713","messageId":"20160125144250.GM7100@hank","threadId":"41216","inReplyTo":"xmqq8u3ed45r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-01-25T14:42:50Z","receivedAt":"2016-01-25T14:42:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 01/24, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > Sorry, but I am confused by all of the above.\n> >\n> > We write the thing out with write_entry(), possibly applying smudge\n> > filters and eol conversion to the \"git\" representation to create the\n> > \"working tree\" representation in this codepath, right? The resulting\n> > file matches what the user's configuration told us to produce.\n> >\n> > Until the working tree file is changed by somebody after the above\n> > happens, we shouldn't have to check the contents of the file to see\n> > if there is a difference. By definition, that has to match the\n> > contents expected to be there by Git.\n> >\n> > The only case I can think of that the above does not hold is when\n> > the smuge/clean and the eol conversion are not a correct round-trip\n> > operation pairs, but that would be a misconfiguration. Otherwise,\n> > we'd be _always_ comparing the contents without relying on the\n> > cached stat info for any paths whose in-core and working tree\n> > representations are different, not just those that are configured\n> > with misbehaving smudge/clean pair, no?\n>\n> To put it differently, if a blob stored at path has CRLF line\n> endings and .gitattributes is changed after the fact to say that it\n> must have LF line endings, we should treat it as a broken transitory\n> state.\n\nRight, I wasn't considering this as a broken state, because t0025 uses\njust this to transition between the states.\n\n> There may have to be a way to \"fix\" an already \"wrong\" blob\n> in the index that is milder than \"rm --cached && add .\", but I do\n> not think write_entry(), which is shared by all the normal codepaths\n> that writes out to the working tree, is the best place to do so, if\n> doing so forces the contents of the paths to be always re-checked,\n> just in case the user is in such a broken transitory state.\n\nMaybe I'm misunderstanding something, but the contents of the paths\nare only re-checked if we are in such a broken transition state, and\nthe file stored in git has crlf line endings, and thus would be\nnormalized. In this case we currently re-check the contents of that\nfile anyway, at least when the file and the index have the same mtime,\nand we actually show the correct output.\n\nI'm not too familiar with the eol conversion code, so I might be\nmissing something.\n\n--\nThomas\n"},{"id":"276721","messageId":"xmqqk2mxa7ug.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160125144250.GM7100@hank","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T17:26:31Z","receivedAt":"2016-01-25T17:26:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> On 01/24, Junio C Hamano wrote:\n>> To put it differently, if a blob stored at path has CRLF line\n>> endings and .gitattributes is changed after the fact to say that it\n>> must have LF line endings, we should treat it as a broken transitory\n>> state.\n>\n> Right, I wasn't considering this as a broken state, because t0025 uses\n> just this to transition between the states.\n>\n>> There may have to be a way to \"fix\" an already \"wrong\" blob\n>> in the index that is milder than \"rm --cached && add .\", but I do\n>> not think write_entry(), which is shared by all the normal codepaths\n>> that writes out to the working tree, is the best place to do so, if\n>> doing so forces the contents of the paths to be always re-checked,\n>> just in case the user is in such a broken transitory state.\n>\n> Maybe I'm misunderstanding something, but the contents of the paths\n> are only re-checked if we are in such a broken transition state, and\n\nWhat I do not understand here is how the added check ensures that\n\"only if in such a broken transition state\". would_convert_to_git()\ndoes not take the contents but is called only with the pathname to\nkey into the attributes, so in a typical cross platform project\nwhere all the source files are \"text\" and the repository can set\ncore.eol to adjust the end of line convention for its working tree,\nthe added check has no way to differentiate the paths that are\nrecorded with CRLF line endings in the object database by mistake,\ni.e. the ones in the broken transitory state, and all the other\npaths that are following the \"text\" and storing their blobs with LF\nline endings. The added check would trigger \"is it racy\" check to\nre-reads the contents that we have written out from the working tree\nfor the paths with \"wrong\" blobs, but how would it avoid doing so\nfor the paths whose blobs are already stored correctly?\n\nThe new code affects not just \"reset --hard\", but everybody who\nwrites out from the object database to the working tree and records\nthat these paths are checked out in the index. How does the new\ncode avoid destroying the performance for all paths?\n\n> the file stored in git has crlf line endings, and thus would be\n> normalized. In this case we currently re-check the contents of that\n> file anyway, at least when the file and the index have the same mtime,\n> and we actually show the correct output.\n\nThe right way to at that \"correct output\", I think, is that it\nhappens to be shown that way by accident. It is questionable that\nit is correct to report that such a path is modified. Immediately\nafter you check out a path to the working tree out of the index, via\n\"reset --hard\" and \"checkout $path\", by definition it ought to be\nthe same between the working tree file and the index.\n\nUnless it is in this transititory broken state, that is.\n\nThe \"by accident\" happens only because racy-git avoidance is being\nimplemented in one particular way. Is about protecting people from\nmaking changes to the working tree files immediately after their\nprevious contents are registered to the index (and the index files\nwritten to the disk), and immediately after we write things out of\nthe index and by definition the result and the indexed blob ought to\nmatch there is no reason to re-read and recheck unless the working\ntree files are further edited.\n\nThe current way the racy-git avoidance works by re-reading and\nre-checking the contents when the timestamps match is merely one\npossible implementation, and that is the only thing that produces\nyour \"correct\" output most of the time in this test. We could have\nwaited after writing the index time for a second before giving the\ncontrol back to the user instead, which would have also allowed us\nto implement the racy-git avoidance, and in that alternate world,\nall these transitory broken paths would have been correctly reported\nas unmodified.\n\nIOW, I would think the test in question is insisting a single\noutcome for an operation whose result is undefined, and it is\nharmful to twist the system by pessimizing the common cases just\nto cater to this transititory broken state.\n\nI am not saying that we shouldn't have support for users to fix\ntheir repository and get out of this transititory broken state. A\nrecent work by Torsten Bögershausen to have ls-files report the end\nof line convention used in the blob in the index and the settings\nthat affect conversion for each path (among other things) is a step\nin the right direction. With a support like that, those who noticed\nthat they by mistake added CRLF files to the index as-is when they\nwanted their project to be cross platform can recover from it by\nsetting necessary attributes (i.e. mark them as \"text\") and then\nfind paths that are broken out of \"ls-files --eol\" output to see\nwhich ones are not using lf end-of-line in the index.\n\nI do not think there is a canned command to help dealing with these\nbroken paths right now. You would have to check them out of the\nindex (you would get a CRLF file in the working tree in the example\nwe are discussing), fix the line endings (you would run dos2unix on\nit in this example, as you would want \"text=true\" attribute) and\n\"git add\" them to recover manually, but I can imagine that Torsten's\nwork can be extended to do all of these, without molesting the\nworking tree files, with minimum work by the end user. That is:\n\n * Reuse Torsten's \"ls-files --eol\" code to find paths that record\n the blob in the index that does not follow the eol convention\n specified for the path;\n\n * For each of these index entries, run convert_to_working_tree() on\n the indexed contents, and then on the result of it, run\n convert_to_git(). The result is the blob that the index ought to\n have had, if it were to be consistent with the attribute\n settings. So add that to the index.\n\n * Write the index out.\n\n * Tell the user to commit or commit it automatically with a canned\n log message \"fix broken encoding\" or something.\n"},{"id":"276739","messageId":"xmqqegd5fht9.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"xmqqk2mxa7ug.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T21:52:18Z","receivedAt":"2016-01-25T21:52:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am not saying that we shouldn't have support for users to fix\n> their repository and get out of this transititory broken state. A\n> recent work by Torsten Bögershausen to have ls-files report the end\n> of line convention used in the blob in the index and the settings\n> that affect conversion for each path (among other things) is a step\n> in the right direction. With a support like that, those who noticed\n> that they by mistake added CRLF files to the index as-is when they\n> wanted their project to be cross platform can recover from it by\n> setting necessary attributes (i.e. mark them as \"text\") and then\n> find paths that are broken out of \"ls-files --eol\" output to see\n> which ones are not using lf end-of-line in the index.\n>\n> I do not think there is a canned command to help dealing with these\n> broken paths right now. You would have to check them out of the\n> index (you would get a CRLF file in the working tree in the example\n> we are discussing), fix the line endings (you would run dos2unix on\n> it in this example, as you would want \"text=true\" attribute) and\n> \"git add\" them to recover manually, but I can imagine that Torsten's\n> work can be extended to do all of these, without molesting the\n> working tree files, with minimum work by the end user. That is:\n>\n> * Reuse Torsten's \"ls-files --eol\" code to find paths that record\n> the blob in the index that does not follow the eol convention\n> specified for the path;\n>\n> * For each of these index entries, run convert_to_working_tree() on\n> the indexed contents, and then on the result of it, run\n> convert_to_git(). The result is the blob that the index ought to\n> have had, if it were to be consistent with the attribute\n> settings. So add that to the index.\n>\n> * Write the index out.\n>\n> * Tell the user to commit or commit it automatically with a canned\n> log message \"fix broken encoding\" or something.\n\nHere is what I whipped up as a lunch-break hack. I do not claim\nthat \"git add\" would be the best place to do this, but it should be\nsufficient to illustrate the overall idea.\n\nThe user can say \"git add --fix-index\" and have a simplified version\nof the above happen, i.e. for each path in the index, if the\ncontents recorded there does not round-trip to the identical\ncontents when first converted to the working tree representation\n(i.e. passing through core.eol and smudge filter conversion) and\nthen converted back to the Git blob representation (i.e. clean\nfilter and core.crlf), and when the result is different from what we\nstarted from, we know we have an unnormalized blob registered in the\nindex, so we replace it. After this, \"git diff --cached\" would show\nthe correction made by this operation, and committing it would let\nyou fix the earlier mistake that added CRLF content when the path\nwas marked with text=true attribute.\n\nWe could go even fancier and attempt the round-trip twice or more.\nIt is possible that the in-index representation will not converge\nwhen you use a misconfigured pair of clean/smudge filters (e.g.\nusing \"gzip -d -c\" as the smudge filter, and then using \"gzip -c\"\nwithout \"-n\" option as the clean filter would most likely make the\nin-index representation fuzzy, as each time the cycle is run, the\ncompressed contents will be made with different timestamps, even\nthough the working tree representation will be the same), and an\noperation \"we screwed up the filters, please repair the damage!\"\nlike this \"add --fix-index\" is probably the best place to catch such\na misconfiguration.\n\n builtin/add.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 64 insertions(+)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 145f06e..36d3915 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -233,6 +233,7 @@ N_(\"The following paths are ignored by one of your .gitignore files:\\n\");\n \n static int verbose, show_only, ignored_too, refresh_only;\n static int ignore_add_errors, intent_to_add, ignore_missing;\n+static int fix_index;\n \n #define ADDREMOVE_DEFAULT 1\n static int addremove = ADDREMOVE_DEFAULT;\n@@ -263,6 +264,7 @@ static struct option builtin_add_options[] = {\n \tOPT_BOOL( 0 , \"refresh\", &refresh_only, N_(\"don't add, only refresh the index\")),\n \tOPT_BOOL( 0 , \"ignore-errors\", &ignore_add_errors, N_(\"just skip files which cannot be added because of errors\")),\n \tOPT_BOOL( 0 , \"ignore-missing\", &ignore_missing, N_(\"check if - even missing - files are ignored in dry run\")),\n+\tOPT_BOOL( 0 , \"fix-index\", &fix_index, N_(\"fix contents in the index that is inconsistent with the eol and clean/smudge filters\")),\n \tOPT_END(),\n };\n \n@@ -297,6 +299,64 @@ static int add_files(struct dir_struct *dir, int flags)\n \treturn exit_status;\n }\n \n+static int fix_index_roundtrip(int ac, const char **av, const char *prefix)\n+{\n+\tint i;\n+\n+\tif (ac)\n+\t\tdie(_(\"git add --fix-index does not take any other argument\"));\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tchar *contents;\n+\t\tunsigned long size;\n+\t\tenum object_type type;\n+\t\tunsigned char sha1[20];\n+\n+\t\tif (ce_stage(ce) || !S_ISREG(ce->ce_mode))\n+\t\t\tcontinue;\n+\t\tif (!would_convert_to_git(ce->name))\n+\t\t\tcontinue;\n+\n+\t\tcontents = read_sha1_file(ce->sha1, &type, &size);\n+\t\tif (type != OBJ_BLOB)\n+\t\t\tdie(_(\"object in the index at path '%s' is not a blob\"),\n+\t\t\t ce->name);\n+\n+\t\t/*\n+\t\t * Round-trip conversion; act as if we wrote it out to the\n+\t\t * working tree and then re-read it, with clean/smudge and\n+\t\t * eol conversions. Do we get the same result?\n+\t\t */\n+\t\tif (convert_to_working_tree(ce->name, contents, size, &buf))\n+\t\t\tstrbuf_add(&buf, contents, size);\n+\t\tfree(contents);\n+\n+\t\tcontents = strbuf_detach(&buf, &size);\n+\n+\t\tif (!convert_to_git(ce->name, contents, size, &buf, 0))\n+\t\t\tstrbuf_add(&buf, contents, size);\n+\t\tfree(contents);\n+\n+\t\t/* Hash the result - does it match? */\n+\t\thash_sha1_file(buf.buf, buf.len, \"blob\", sha1);\n+\t\tif (hashcmp(sha1, ce->sha1)) {\n+\t\t\thashcpy(ce->sha1, sha1);\n+\t\t\tactive_cache_changed = 1;\n+\t\t}\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n+\tif (active_cache_changed)\n+\t\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\t\t\tdie(_(\"Unable to write new index file\"));\n+\treturn 0;\n+}\n+\n int cmd_add(int argc, const char **argv, const char *prefix)\n {\n \tint exit_status = 0;\n@@ -318,6 +378,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tif (edit_interactive)\n \t\treturn(edit_patch(argc, argv, prefix));\n+\n+\tif (fix_index)\n+\t\texit(fix_index_roundtrip(argc, argv, prefix));\n+\n \targc--;\n \targv++;\n \n"},{"id":"276743","messageId":"20160125224140.GN7100@hank","threadId":"41216","inReplyTo":"xmqqk2mxa7ug.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-01-25T22:41:40Z","receivedAt":"2016-01-25T22:41:40Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 01/25, Junio C Hamano wrote:\n> Thomas Gummerer writes:\n>\n> > On 01/24, Junio C Hamano wrote:\n> >> To put it differently, if a blob stored at path has CRLF line\n> >> endings and .gitattributes is changed after the fact to say that it\n> >> must have LF line endings, we should treat it as a broken transitory\n> >> state.\n> >\n> > Right, I wasn't considering this as a broken state, because t0025 uses\n> > just this to transition between the states.\n> >\n> >> There may have to be a way to \"fix\" an already \"wrong\" blob\n> >> in the index that is milder than \"rm --cached && add .\", but I do\n> >> not think write_entry(), which is shared by all the normal codepaths\n> >> that writes out to the working tree, is the best place to do so, if\n> >> doing so forces the contents of the paths to be always re-checked,\n> >> just in case the user is in such a broken transitory state.\n> >\n> > Maybe I'm misunderstanding something, but the contents of the paths\n> > are only re-checked if we are in such a broken transition state, and\n>\n> What I do not understand here is how the added check ensures that\n> \"only if in such a broken transition state\". would_convert_to_git()\n> does not take the contents but is called only with the pathname to\n> key into the attributes, so in a typical cross platform project\n> where all the source files are \"text\" and the repository can set\n> core.eol to adjust the end of line convention for its working tree,\n> the added check has no way to differentiate the paths that are\n> recorded with CRLF line endings in the object database by mistake,\n> i.e. the ones in the broken transitory state, and all the other\n> paths that are following the \"text\" and storing their blobs with LF\n> line endings. The added check would trigger \"is it racy\" check to\n> re-reads the contents that we have written out from the working tree\n> for the paths with \"wrong\" blobs, but how would it avoid doing so\n> for the paths whose blobs are already stored correctly?\n>\n> The new code affects not just \"reset --hard\", but everybody who\n> writes out from the object database to the working tree and records\n> that these paths are checked out in the index. How does the new\n> code avoid destroying the performance for all paths?\n\nI misunderstood the way would_convert_to_git() works, I should have\nactually read the code, instead of just relying on my test, which was\neven wrong. Sorry about the confusion, my patch does indeed hurt\nthe performance.\n\n> > the file stored in git has crlf line endings, and thus would be\n> > normalized. In this case we currently re-check the contents of that\n> > file anyway, at least when the file and the index have the same mtime,\n> > and we actually show the correct output.\n>\n> The right way to at that \"correct output\", I think, is that it\n> happens to be shown that way by accident. It is questionable that\n> it is correct to report that such a path is modified. Immediately\n> after you check out a path to the working tree out of the index, via\n> \"reset --hard\" and \"checkout $path\", by definition it ought to be\n> the same between the working tree file and the index.\n>\n> Unless it is in this transititory broken state, that is.\n>\n> The \"by accident\" happens only because racy-git avoidance is being\n> implemented in one particular way. Is about protecting people from\n> making changes to the working tree files immediately after their\n> previous contents are registered to the index (and the index files\n> written to the disk), and immediately after we write things out of\n> the index and by definition the result and the indexed blob ought to\n> match there is no reason to re-read and recheck unless the working\n> tree files are further edited.\n>\n> The current way the racy-git avoidance works by re-reading and\n> re-checking the contents when the timestamps match is merely one\n> possible implementation, and that is the only thing that produces\n> your \"correct\" output most of the time in this test. We could have\n> waited after writing the index time for a second before giving the\n> control back to the user instead, which would have also allowed us\n> to implement the racy-git avoidance, and in that alternate world,\n> all these transitory broken paths would have been correctly reported\n> as unmodified.\n>\n> IOW, I would think the test in question is insisting a single\n> outcome for an operation whose result is undefined, and it is\n> harmful to twist the system by pessimizing the common cases just\n> to cater to this transititory broken state.\n>\n> I am not saying that we shouldn't have support for users to fix\n> their repository and get out of this transititory broken state. A\n> recent work by Torsten Bögershausen to have ls-files report the end\n> of line convention used in the blob in the index and the settings\n> that affect conversion for each path (among other things) is a step\n> in the right direction. With a support like that, those who noticed\n> that they by mistake added CRLF files to the index as-is when they\n> wanted their project to be cross platform can recover from it by\n> setting necessary attributes (i.e. mark them as \"text\") and then\n> find paths that are broken out of \"ls-files --eol\" output to see\n> which ones are not using lf end-of-line in the index.\n>\n> I do not think there is a canned command to help dealing with these\n> broken paths right now. You would have to check them out of the\n> index (you would get a CRLF file in the working tree in the example\n> we are discussing), fix the line endings (you would run dos2unix on\n> it in this example, as you would want \"text=true\" attribute) and\n> \"git add\" them to recover manually, but I can imagine that Torsten's\n> work can be extended to do all of these, without molesting the\n> working tree files, with minimum work by the end user. That is:\n>\n> * Reuse Torsten's \"ls-files --eol\" code to find paths that record\n> the blob in the index that does not follow the eol convention\n> specified for the path;\n>\n> * For each of these index entries, run convert_to_working_tree() on\n> the indexed contents, and then on the result of it, run\n> convert_to_git(). The result is the blob that the index ought to\n> have had, if it were to be consistent with the attribute\n> settings. So add that to the index.\n>\n> * Write the index out.\n>\n> * Tell the user to commit or commit it automatically with a canned\n> log message \"fix broken encoding\" or something.\n\nThanks for the thorough explanation, and the patch in the next email,\nI'll have a look at that tomorrow.\n\n--\nThomas\n"},{"id":"276892","messageId":"20160127151602.GA1690@ecki.hitronhub.home","threadId":"41216","inReplyTo":"xmqqegd5fht9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2016-01-27T15:16:02Z","receivedAt":"2016-01-27T15:16:02Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"I think Junio pointed me to this thread from \"[PATCH] optionally disable\ngitattributes\". Since I am not sure I am following everything correctly\nin this thread, allow me to recapitulate what I understood so far.\n\nFirstly, I think the racy'ness of t0025 is understood. It is due to the\nis_racy_timestamp check in read-cache.c's ie_match_stat. But for the\nmoment I would like to put this aside, because the issue can be\nreproduced reliably with this change to t0025:\n\ndiff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh\nindex c164b46..e30e9b3 100755\n--- a/t/t0025-crlf-auto.sh\n+++ b/t/t0025-crlf-auto.sh\n@@ -55,8 +55,11 @@ test_expect_success 'crlf=true causes a CRLF file to be normalized' '\n test_expect_success 'text=true causes a CRLF file to be normalized' '\n \n rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&\n- echo \"CRLFonly text\" > .gitattributes &&\n git read-tree --reset -u HEAD &&\n+ sleep 1 &&\n+ rm .git/index &&\n+ git reset &&\n+ echo \"CRLFonly text\" > .gitattributes &&\n \n # Note, \"normalized\" means that git will normalize it if added\n has_cr CRLFonly &&\n\nI intentionally wait for one second and then I remove and re-read the\nindex. Now the timestamps of CRLFonly and .git/index are different, so\nwe avoid the is_racy_timestamp check. From now on Git will not read the\ncontents of CRLFonly from disk again until either the index entry or the\nmtime of CRLFonly changes (maybe we also check the size, I am not sure).\n\nNow we add .gitattributes. This does not change the index entry, nor\ndoes it change the mtime of CRLFonly. Therefore the subsequent git diff\nturns out empty, and the test fails.\n\nI believe this behavior is expected. In gitattributes(5) we therefore\nrecommend using rm .git/index and git reset to \"force Git to rescan the\nworking directory.\" The test should be fixed accordingly, something\nlike:\n\ndiff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh\nindex c164b46..2917591 100755\n--- a/t/t0025-crlf-auto.sh\n+++ b/t/t0025-crlf-auto.sh\n@@ -57,6 +57,8 @@ test_expect_success 'text=true causes a CRLF file to be normalized' '\n rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&\n echo \"CRLFonly text\" > .gitattributes &&\n git read-tree --reset -u HEAD &&\n+ rm .git/index &&\n+ git reset &&\n \n # Note, \"normalized\" means that git will normalize it if added\n has_cr CRLFonly &&\n\n\n\nOn Mon, Jan 25, 2016 at 01:52:18PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > I do not think there is a canned command to help dealing with these\n> > broken paths right now.\n\nI think (rm .git/index && git reset) works well enough in most cases,\nbut not all:\n\n> We could go even fancier and attempt the round-trip twice or more.\n> It is possible that the in-index representation will not converge\n> when you use a misconfigured pair of clean/smudge filters\n\nThis can also happen with eol conversion, for example if you have files\nwith CRCRLF line endings. The eol conversion will remove only one CR.\nTwo conversions would be needed to achieve a normalized format. But\niterating (rm .git/index && git reset) does not help. Since we do not\ntouch the file on disk, after the first round, we have CRCRLF on disk\nand CRLF in the index. During the second round, Git reads CRCRLF from\ndisk again, converts it to CRLF, which matches the index. Even\ngit reset --hard will not checkout the CRLF version to the worktree.\n\nA possible solution is to iterate (rm -r * && git checkout -- . && git\nadd -u) until the work tree is clean. Quite ugly.\n\nA command like git add --fix-index would make this conversion less\npainful. It should be ok if the user has to run it several times in\ncorner cases like CRCRLF, but it would be nice to issue a warning if the\nindex is still not normalized after running git add --fix-index.\n\nRegarding the name of the option, maybe git add --renormalize-index\nwould be more consistent, since we also have the related merge option\n\"renormalize\", which is very similar. In fact possibly you can share\nsome code with it.\n\nYour patch looks good to me otherwise.\n\n\nComing back to \"[PATCH] optionally disable gitattributes\": The topics\nare related, because they both deal with the situation where the work\ntree has files which are not normalized according to gitattributes. But\nmy patch is more about saying: ok, I know I may have files which need to\nbe normalized, but I want to ignore this issue for now. Please disable\ngitattributes for now, because I want to work with the files as they are\ncommitted. Conversely, the discussion here is about how to reliably\ndetect and fix files which are not normalized.\n"},{"id":"276924","messageId":"xmqqd1sm9730.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160127151602.GA1690@ecki.hitronhub.home","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T19:05:07Z","receivedAt":"2016-01-27T19:05:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Coming back to \"[PATCH] optionally disable gitattributes\": The topics\n> are related, because they both deal with the situation where the work\n> tree has files which are not normalized according to gitattributes. But\n> my patch is more about saying: ok, I know I may have files which need to\n> be normalized, but I want to ignore this issue for now. Please disable\n> gitattributes for now, because I want to work with the files as they are\n> committed. Conversely, the discussion here is about how to reliably\n> detect and fix files which are not normalized.\n\nI primarily wanted to make sure that you understood the underlying\nissue, so that I do not have to go back to the basics in the other\nthread. And it is clear that you obviously do, which is good.\n\nHere, you seem to think that what t0025 wants to see happen is\nsensible, judging by the fact that you call \"rm .git/index && git\nreset\" a \"fix\".\n\nMy take on this is quite different. After a \"reset --hard HEAD\", we\nshould be able to trust the cached stat information and have \"diff\nHEAD\" say \"no changes\". That is what you essentially want in the\nother thread, if I understand you correctly, and in an ideal world\nwhere the filesystem timestamp has infinite precision, that is what\nwould happen in t0025, always \"breaking\" its expectation. The real\nworld has much coarser timestamp granularity than ideal, and that is\nwhy the test appear to be \"flaky\", failing to give \"correct\" outcome\nsome of the time--but I'd say that it is expecting a wrong thing.\n\nAn index entry that has data that does not round-trip when it goes\nthrough convert_to_working_tree() and then convert_to_git() \"breaks\"\nthis arrangement, and I'd view it as the user having an inconsistent\ndata. It is like you are in a repository that still has an unmerged\npaths--you cannot proceed before you resolve them.\n\nAnyway.\n\nAs to your patch in the other thread, here is what I think:\n\n (1) When you know (or perhaps your CI knows) that the working tree\n has never been modified since you did \"reset --hard HEAD\" (or\n its equivalent, like \"git checkout $branch\" from a clean\n state), these paths with inconsistent data would break the\n usual check to ask \"is the working tree clean?\" That is a\n problem and we need a way to ensure that the working tree is\n always judged to be clean immediately after \"reset --hard\n HEAD\". IOW, I agree with you that the issue you are trying to\n solve is worth solving.\n\n (2) Regardless of the \"inconsistent data breaking the cleanliness\n check\" issue, it may be handy to have a way to temporarily\n disable the attributes, i.e. allow us to ask \"what happens if\n there is no attributes defined?\" IOW, I am saying that the\n change in the patch is not without merit.\n\nIn addition to (1), I further think that this sequence should not\nreport that the path F is modified:\n\n # Write F from HEAD to the working tree, after passing it\n # through convert_to_working_tree()\n $ git reset --hard HEAD\n\n # Force the re-reading, without changing the contents at all\n $ cp F F.new\n $ mv F.new F\n\n $ git diff HEAD\n\nwhich is broken by paths with inconsistent data. Your CI would want\na way to make that happen.\n\nHowever, I do not think disabling attributes (i.e. (2)) is a\nsolution to the issue (i.e. (1)), which we just agreed to be an\nissue that is worth solving, for at least two reasons.\n\n * Even without any attributes, core.autocrlf setting can get the\n data in your index (whose lines can be terminated with CRLF) into\n the same \"inconsistent data\" situation. Disabling attribute\n handling would not have any effect on that codepath, I think.\n\n * The indexed data and the contents in the working tree file may\n match only because the clean/smudge transformation is done. If\n you disable attributes, re-checking by passing the working tree\n contents through convert_to_git() and comparing the result with\n what is in the index would tell you that they are different, even\n if the clean/smudge filter pair implements round-trip operations\n correctly.\n\nOne way to solve (1) I can think of is to change the definition of\nce_compare_data(), which is called by the code that does not trust\nthe cached stat data (including but not limited to the Racy Git\ncodepath). The current semantics of that function asks this\nquestion:\n\n We do not know if the working tree file and the indexed data\n match. Let's see if \"git add\" of that path would record the\n data that is identical to what is in the index.\n\nThis definition was cast in stone by 29e4d363 (Racy GIT, 2005-12-20)\nand has been with us since Git v1.0.0. But that does not have to be\nthe only sensible definition of this check. I wonder what would\nbreak if we ask this question instead:\n\n We do not know if the working tree file and the indexed data\n match. Let's see if \"git checkout\" of that path would leave the\n same data as what currently is in the working tree file.\n\nIf we did this, \"reset --hard HEAD\" followed by \"diff HEAD\" will by\ndefinition always report \"is clean\" as long as nobody changes files\nin the working tree, even with the inconsistent data in the index.\n\nThis still requires that convert_to_working_tree(), i.e. your smudge\nfilter, is deterministic, though, but I think that is a sensible\nassumption for sane people, even for those with inconsistent data in\nthe index.\n"},{"id":"276934","messageId":"xmqqmvrq7nok.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"xmqqd1sm9730.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-27T20:49:31Z","receivedAt":"2016-01-27T20:49:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> One way to solve (1) I can think of is to change the definition of\n> ce_compare_data(), which is called by the code that does not trust\n> the cached stat data (including but not limited to the Racy Git\n> codepath). The current semantics of that function asks this\n> question:\n>\n> We do not know if the working tree file and the indexed data\n> match. Let's see if \"git add\" of that path would record the\n> data that is identical to what is in the index.\n>\n> This definition was cast in stone by 29e4d363 (Racy GIT, 2005-12-20)\n> and has been with us since Git v1.0.0. But that does not have to be\n> the only sensible definition of this check. I wonder what would\n> break if we ask this question instead:\n>\n> We do not know if the working tree file and the indexed data\n> match. Let's see if \"git checkout\" of that path would leave the\n> same data as what currently is in the working tree file.\n>\n> If we did this, \"reset --hard HEAD\" followed by \"diff HEAD\" will by\n> definition always report \"is clean\" as long as nobody changes files\n> in the working tree, even with the inconsistent data in the index.\n>\n> This still requires that convert_to_working_tree(), i.e. your smudge\n> filter, is deterministic, though, but I think that is a sensible\n> assumption for sane people, even for those with inconsistent data in\n> the index.\n\nJust a few additional comments.\n\nThe primary reason why I originally chose \"does 'git add' of what is\nin the working tree give us the same blob in the index?\" as opposed\nto \"does 'git checkout' from the index again will give the same\nresult in the working tree?\" is because it is a lot less resource\nintensive and also is simpler. Back then I do not think we had a\nstreaming interface to hash huge contents from a file in the working\ntree, but it requires us to read the entire file from the filesystem\njust once, apply the convert_to_git() processing and then hash the\nresult, whether we keep the whole thing in core at once or process\nthe data in streaming fashion. Doing the other check will have to\ninflate the blob data and apply the convert_to_working_tree()\nprocessing, and also read the whole thing from the filesystem and\ncompare, which is more work at runtime. And for a sane set-up where\nthe data in the index does not contradict with the clean/smudge\nfilter and EOL settings, both would yield the same result.\n\nIf we were to switch the semantics of ce_compare_data(), we would\nwant a new sibling interface next to stream_blob_to_fd() that takes\na file descriptor opened on the file in the working tree for reading\n(fd), the object name (sha1), and the output filter, and works very\nsimilarly to stream_blob_to_fd(). The difference would be that we\nwould be reading from the fd (i.e. the file in the working tree) as\nwe read from the istream (i.e. the contents of the blob in the\nindex, after passing the convert_to_working_tree() filter) and\ncomparing them in the main loop. The filter parameter to the\nfunction would be obtained by calling get_stream_filter() just like\nhow write_entry() uses it to prepare the filter parameter to call\nstreaming_write_entry() with. That way, we can rely on future\nimprovement of the streaming interface to make sure we keep the data\nwe have to keep in core to the minimum.\n\nIOW, I am saying that the \"add --fix-index\" lunchbreak patch I sent\nearlier in the thread that has to hold the data in-core while\nprocessing is not a production quality patch ;-)\n"},{"id":"276957","messageId":"56A9B34A.1060205@web.de","threadId":"41216","inReplyTo":"xmqqd1sm9730.fsf@gitster.mtv.corp.google.com","subject":"Re: eol round trip Was: [PATCH] travis-ci: run previously failed ....","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-01-28T06:20:58Z","receivedAt":"2016-01-28T06:20:58Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 01/27/2016 08:05 PM, Junio C Hamano wrote:\n(Changed the topic, 2 notes inside)\n> Clemens Buchacher writes:\n>\n>> Coming back to \"[PATCH] optionally disable gitattributes\": The topics\n>> are related, because they both deal with the situation where the work\n>> tree has files which are not normalized according to gitattributes. But\n>> my patch is more about saying: ok, I know I may have files which need to\n>> be normalized, but I want to ignore this issue for now. Please disable\n>> gitattributes for now, because I want to work with the files as they are\n>> committed. Conversely, the discussion here is about how to reliably\n>> detect and fix files which are not normalized.\ngit ls-files --eol can detect that (as Junio pointed out)\n\n> I primarily wanted to make sure that you understood the underlying\n> issue, so that I do not have to go back to the basics in the other\n> thread. And it is clear that you obviously do, which is good.\n>\n> Here, you seem to think that what t0025 wants to see happen is\n> sensible, judging by the fact that you call \"rm .git/index && git\n> reset\" a \"fix\".\n>\n> My take on this is quite different. After a \"reset --hard HEAD\", we\n> should be able to trust the cached stat information and have \"diff\n> HEAD\" say \"no changes\". That is what you essentially want in the\n> other thread, if I understand you correctly, and in an ideal world\n> where the filesystem timestamp has infinite precision, that is what\n> would happen in t0025, always \"breaking\" its expectation. The real\n> world has much coarser timestamp granularity than ideal, and that is\n> why the test appear to be \"flaky\", failing to give \"correct\" outcome\n> some of the time--but I'd say that it is expecting a wrong thing.\n>\n> An index entry that has data that does not round-trip when it goes\n> through convert_to_working_tree() and then convert_to_git() \"breaks\"\n> this arrangement, and I'd view it as the user having an inconsistent\n> data. It is like you are in a repository that still has an unmerged\n> paths--you cannot proceed before you resolve them.\nThis is actually bringing some light to me: the round-trip test.\nThere are this \"well known but less well document\" situations where we \nbreak that rule:\n- files are checked in with CRLF into the repo.\n- .gittatributes is set to \"text\" later.\n2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\n- files with mixed line endings in the repo:\nSame here: 2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\n- files with CRCRLF line endings in the repo:\nSame here: 2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\nMy feeling is that we should simply say:\nYou user set attribute to \"text\" and by doing that, you promised to have \nfiles\nwith LF only in the index.\nIf you break that promise, Git does not know, what you really want.\n- It may be a situation where you write a shell script which for some \nreasons\n needs a '\\015' at the end of a line, and Git may treat it wrong by \nassuming\n that this is a CRLF line ending (end converts it into LF)\n- It may be that you want CRLF because you added a Windows .BAT file.\n It may be that you use git.git and another implementation of Git, \nwhich doesn't\n support attributes at all, so that a save way to do this is to just \ncommit CRLF.\n- It may be that this is a historical issue.\n Everybody using the project uses git that understands .gitattributes,\n so someone may fix it some day.\n\nCan Git make this decision ?\n\nWhen core.autocrlf is true (and no attributes are set), then the \nconversion of line ending is disabled.\nOn 01/27/2016 08:05 PM, Junio C Hamano wrote:\n(Changed the topic, 2 notes inside)\n> Clemens Buchacher writes:\n>\n>> Coming back to \"[PATCH] optionally disable gitattributes\": The topics\n>> are related, because they both deal with the situation where the work\n>> tree has files which are not normalized according to gitattributes. But\n>> my patch is more about saying: ok, I know I may have files which need to\n>> be normalized, but I want to ignore this issue for now. Please disable\n>> gitattributes for now, because I want to work with the files as they are\n>> committed. Conversely, the discussion here is about how to reliably\n>> detect and fix files which are not normalized.\ngit ls-files --eol can detect that (as Junio pointed out)\n\n> I primarily wanted to make sure that you understood the underlying\n> issue, so that I do not have to go back to the basics in the other\n> thread. And it is clear that you obviously do, which is good.\n>\n> Here, you seem to think that what t0025 wants to see happen is\n> sensible, judging by the fact that you call \"rm .git/index && git\n> reset\" a \"fix\".\n>\n> My take on this is quite different. After a \"reset --hard HEAD\", we\n> should be able to trust the cached stat information and have \"diff\n> HEAD\" say \"no changes\". That is what you essentially want in the\n> other thread, if I understand you correctly, and in an ideal world\n> where the filesystem timestamp has infinite precision, that is what\n> would happen in t0025, always \"breaking\" its expectation. The real\n> world has much coarser timestamp granularity than ideal, and that is\n> why the test appear to be \"flaky\", failing to give \"correct\" outcome\n> some of the time--but I'd say that it is expecting a wrong thing.\n>\n> An index entry that has data that does not round-trip when it goes\n> through convert_to_working_tree() and then convert_to_git() \"breaks\"\n> this arrangement, and I'd view it as the user having an inconsistent\n> data. It is like you are in a repository that still has an unmerged\n> paths--you cannot proceed before you resolve them.\nThis is actually bringing some light to me: the round-trip test.\nThere are this \"well known but less well document\" situations where we \nbreak that rule:\n- files are checked in with CRLF into the repo.\n- .gittatributes is set to \"text\" later.\n2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\n- files with mixed line endings in the repo:\nSame here: 2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\n- files with CRCRLF line endings in the repo:\nSame here: 2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\nMy feeling is that we should simply say:\nYou user set attribute to \"text\" and by doing that, you promised to have \nfiles\nwith LF only in the index.\nIf you break that promise, Git does not know, what you really want.\n- It may be a situation where you write a shell script which for some \nreasons\n needs a '\\015' at the end of a line, and Git may treat it wrong by \nassuming\n that this is a CRLF line ending (end converts it into LF)\n- It may be that you want CRLF because you added a Windows .BAT file.\n It may be that you use git.git and another implementation of Git, \nwhich doesn't\n support attributes at all, so that a save way to do this is to just \ncommit CRLF.\n- It may be that this is a historical issue.\n Everybody using the project uses git that understands .gitattributes,\n so someone may fix it some day.\n\nCan Git make this decision ?\n\nWhen core.autocrlf is true (and no attributes are set), then the \nconversion of line ending is disabled.\nOn 01/27/2016 08:05 PM, Junio C Hamano wrote:\n(Changed the topic, 2 notes inside)\n> Clemens Buchacher writes:\n>\n>> Coming back to \"[PATCH] optionally disable gitattributes\": The topics\n>> are related, because they both deal with the situation where the work\n>> tree has files which are not normalized according to gitattributes. But\n>> my patch is more about saying: ok, I know I may have files which need to\n>> be normalized, but I want to ignore this issue for now. Please disable\n>> gitattributes for now, because I want to work with the files as they are\n>> committed. Conversely, the discussion here is about how to reliably\n>> detect and fix files which are not normalized.\ngit ls-files --eol can detect that (as Junio pointed out)\n\n> I primarily wanted to make sure that you understood the underlying\n> issue, so that I do not have to go back to the basics in the other\n> thread. And it is clear that you obviously do, which is good.\n>\n> Here, you seem to think that what t0025 wants to see happen is\n> sensible, judging by the fact that you call \"rm .git/index && git\n> reset\" a \"fix\".\n>\n> My take on this is quite different. After a \"reset --hard HEAD\", we\n> should be able to trust the cached stat information and have \"diff\n> HEAD\" say \"no changes\". That is what you essentially want in the\n> other thread, if I understand you correctly, and in an ideal world\n> where the filesystem timestamp has infinite precision, that is what\n> would happen in t0025, always \"breaking\" its expectation. The real\n> world has much coarser timestamp granularity than ideal, and that is\n> why the test appear to be \"flaky\", failing to give \"correct\" outcome\n> some of the time--but I'd say that it is expecting a wrong thing.\n>\n> An index entry that has data that does not round-trip when it goes\n> through convert_to_working_tree() and then convert_to_git() \"breaks\"\n> this arrangement, and I'd view it as the user having an inconsistent\n> data. It is like you are in a repository that still has an unmerged\n> paths--you cannot proceed before you resolve them.\nThis is actually bringing some light to me: the round-trip test.\nThere are this \"well known but less well document\" situations where we \nbreak that rule:\n- files are checked in with CRLF into the repo.\n- .gittatributes is set to \"text\" later.\n2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\n- files with mixed line endings in the repo:\nSame here: 2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\n- files with CRCRLF line endings in the repo:\nSame here: 2 different ways to handle it:\n- keep the eol at checkout, normalize at checkin -> roundtrip broken\n- keep the eol at checkout and checkin -> roundtrip OK\n\nMy feeling is that we should simply say:\nYou user set attribute to \"text\" and by doing that, you promised to have \nfiles\nwith LF only in the index.\nIf you break that promise, Git does not know, what you really want.\n- It may be a situation where you write a shell script which for some \nreasons\n needs a '\\015' at the end of a line, and Git may treat it wrong by \nassuming\n that this is a CRLF line ending (end converts it into LF)\n- It may be that you want CRLF because you added a Windows .BAT file.\n It may be that you use git.git and another implementation of Git, \nwhich doesn't\n support attributes at all, so that a save way to do this is to just \ncommit CRLF.\n- It may be that this is a historical issue.\n Everybody using the project uses git that understands .gitattributes,\n so someone may fix it some day.\n\nCan Git make this decision ?\n\nWhen core.autocrlf is true (and no attributes are set), then the \nconversion of line endings is disabled.\nSee convert.v \"This is the new safer autocrlf handling\",\ncommit fd6cce9e\n\nSo the round trip is achieved when core.autocrlf=true,\nbut no longer when attributes are added.\n[]\n\n\n> Anyway.\n>\n> As to your patch in the other thread, here is what I think:\n>\n> (1) When you know (or perhaps your CI knows) that the working tree\n> has never been modified since you did \"reset --hard HEAD\" (or\n> its equivalent, like \"git checkout $branch\" from a clean\n> state), these paths with inconsistent data would break the\n> usual check to ask \"is the working tree clean?\" That is a\n> problem and we need a way to ensure that the working tree is\n> always judged to be clean immediately after \"reset --hard\n> HEAD\". IOW, I agree with you that the issue you are trying to\n> solve is worth solving.\n>\n> (2) Regardless of the \"inconsistent data breaking the cleanliness\n> check\" issue, it may be handy to have a way to temporarily\n> disable the attributes, i.e. allow us to ask \"what happens if\n> there is no attributes defined?\" IOW, I am saying that the\n> change in the patch is not without merit.\n>\n> In addition to (1), I further think that this sequence should not\n> report that the path F is modified:\n>\n> # Write F from HEAD to the working tree, after passing it\n> # through convert_to_working_tree()\n> $ git reset --hard HEAD\n>\n> # Force the re-reading, without changing the contents at all\n> $ cp F F.new\n> $ mv F.new F\n>\n> $ git diff HEAD\n>\n> which is broken by paths with inconsistent data. Your CI would want\n> a way to make that happen.\n>\n> However, I do not think disabling attributes (i.e. (2)) is a\n> solution to the issue (i.e. (1)), which we just agreed to be an\n> issue that is worth solving, for at least two reasons.\n>\n> * Even without any attributes, core.autocrlf setting can get the\n> data in your index (whose lines can be terminated with CRLF) into\n> the same \"inconsistent data\" situation. Disabling attribute\n> handling would not have any effect on that codepath, I think.\n>\nI don't think so, see above.\n"},{"id":"276958","messageId":"20160128070959.GA6815@ecki.hitronhub.home","threadId":"41216","inReplyTo":"xmqqmvrq7nok.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2016-01-28T07:10:00Z","receivedAt":"2016-01-28T07:10:00Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 27, 2016 at 12:49:31PM -0800, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > I wonder what would break if we ask this question instead:\n> >\n> > We do not know if the working tree file and the indexed data\n> > match. Let's see if \"git checkout\" of that path would leave the\n> > same data as what currently is in the working tree file.\n\nIf we do this, then git diff should show the diff between\nconvert_to_worktree(index state) and the worktree state. That would be\nnice, because the diff would actually show what we have in the worktree.\nIt keeps confusing me that with eol conversion enabled, git diff does\nnot actually show me the worktree state.\n\nHowever, even if the file is clean in that direction, there could be a\nmismatch between convert_to_git(worktree state) and the index state.\nThis will happen for example in t0025.4, where we have a CRLF file in\nthe index and the worktree, but convert_to_git converts it to a file\nwith LF line endings. Still, I do not see a problem if we also provide a\ncommand like git add --fix-index, which will force normalization of all\nfiles.\n\n> > If we did this, \"reset --hard HEAD\" followed by \"diff HEAD\" will by\n> > definition always report \"is clean\" as long as nobody changes files\n> > in the working tree, even with the inconsistent data in the index.\n\nYes, this is a more elegant and a more complete solution to the problem\nwhich prompted me to submit the GIT_ATTRIBUTES_DISABLED patch.\n\n> > This still requires that convert_to_working_tree(), i.e. your smudge\n> > filter, is deterministic, though, but I think that is a sensible\n> > assumption for sane people, even for those with inconsistent data in\n> > the index.\n\nDeterministic, yes. But not unchanging. When a smudge filter is added,\nor modified, or if the filter program changes, we still have to remove\nthe index before we can trust git diff again. The only way to avoid this\nwould be to somehow detect if the conversion itself changes. One could\nhash the attributes, but changes to the filter configuration or the\nfilter itself are hard to detect. So I think we have to live with this.\n\n> [...] Doing the other check will have to\n> inflate the blob data and apply the convert_to_working_tree()\n> processing, and also read the whole thing from the filesystem and\n> compare, which is more work at runtime.\n\nIf we assume that the smudge filter is deterministic, then we could also\nhash the output of convert_to_working_tree, and store the hash in the\nindex. With this optimization, the comparision would be less work,\nbecause we do not have to apply a filter again, whereas currently we\nhave to apply convert_to_git.\n\n> IOW, I am saying that the \"add --fix-index\" lunchbreak patch I sent\n> earlier in the thread that has to hold the data in-core while\n> processing is not a production quality patch ;-)\n\nOk. The existing implementation in renormalize_buffer (convert.c) works\nfor me, though.\n"},{"id":"277009","messageId":"xmqqk2mtmlu9.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160128070959.GA6815@ecki.hitronhub.home","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-28T21:32:30Z","receivedAt":"2016-01-28T21:32:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Jan 27, 2016 at 12:49:31PM -0800, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> \n>> > I wonder what would break if we ask this question instead:\n>> >\n>> > We do not know if the working tree file and the indexed data\n>> > match. Let's see if \"git checkout\" of that path would leave the\n>> > same data as what currently is in the working tree file.\n>\n> If we do this, then git diff should show the diff between\n> convert_to_worktree(index state) and the worktree state.\n\nI agree with you that, when ce_compare_data(), i.e. \"does the index\nmatch the working tree?\", says \"they match\", \"git diff\" (show me the\nchange to go from the index to the worknig tree) should show empty\nto be consistent, and for that to happen under the above definition\nof ce_compare_data(), \"git diff\" needs to be comparing the data in\nthe index after converting it to the working tree representation\nwith the data in the working tree.\n\nAnd that unfortunately is a very good reason why this approach\nshould not be taken. \"git diff\" (show me the change to go from the\nindex to the working tree) is a preview of what we would see in \"git\ndiff --cached\" (show me the change to go from HEAD to the index) if\nwe did \"git add\", and it is a preview of what we would see in \"git\nshow\" (show me the change of what the last commit did) if we did\n\"git commit -a\". It is crazy for these latter comparisons to happen\nin the working tree (aka \"smudged\") representation of the data, IOW,\nthese two must compare the \"clean\" representation. It also is crazy\nfor \"git diff\" to be using different representation from these two.\nThis alone makes the above idea a non-starter X-<.\n\nBesides, I do not think the above approach really solves the issue,\neither. After \"git reset --hard\" to have the contents in the index\ndumped to the working tree, if your core.autocrlf is flipped, \"git\ncheckout\" of the same path would result in a working tree\nrepresentation of the data that is different from what you have in\nthe working tree, so we would declare that the working tree is not\nclean, even though nobody actually touched them in the meantime.\nThis is less of an issue than having data in the index that is\ninconsistent with the convert_to_git() setting (i.e. eol and clean\nfilter conversion that happens when you \"git add\"), but it still is\nfundamentally the same issue.\n\nOh, bummer, I thought it was a nice approach.\n"},{"id":"277077","messageId":"20160130081306.GA2931@ecki.hitronhub.home","threadId":"41216","inReplyTo":"xmqqk2mtmlu9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2016-01-30T08:13:06Z","receivedAt":"2016-01-30T08:13:06Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Thu, Jan 28, 2016 at 01:32:30PM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> \n> > If we do this, then git diff should show the diff between\n> > convert_to_worktree(index state) and the worktree state.\n> \n> And that unfortunately is a very good reason why this approach\n> should not be taken.\n\nOk, then let's take a step back. I do not actually care if git diff and\nfriends say the worktree is clean or not. But I know that I did not make\nany modifications to the worktree, because I just did git reset --hard.\nAnd now I want to use commands like cherry-pick and checkout without\nfailure. But they can fail, because they essentially use git diff to\ncheck if there are worktree changes, and if so refuse to overwrite them.\n\nSo, if the check \"Am I allowed to modify the worktree file?\", would go\nthe extra mile to also check if the worktree is clean in the sense that\nconvert_to_worktree(index state) matches the worktree. If this is the\ncase, then it is safe to modify the file because it is the committed\nstate, and can be recovered.\n\nRegarding performance impact: We only need to do this extra check if the\nusual check convert_to_git(work tree) against index state fails, and\nconversion is in effect.\n\n> Besides, I do not think the above approach really solves the issue,\n> either. After \"git reset --hard\" to have the contents in the index\n> dumped to the working tree, if your core.autocrlf is flipped,\n\nIndeed, if the user configuration changes, then we cannot always detect\nthis (at least if the filter is an external program, and the behavior of\nthat changes). But the user is in control of that, and we can document\nthis limitation.\n\nOn the other hand, a user who simply follows an upstream repository by\ndoing git pull all the time, and who does not make changes to their\nconfiguration, can still run into this issue, because upstream could\nchange .gitattributes. This part we could actually detect by hashing the\nattributes for each index entry, and if that changes we re-evaluate the\nfile state.\n\nThis is also an issue only if a smudge filter is in place. The eol\nconversion which only acts in the convert_to_git direction is not\naffected.\n"},{"id":"277174","messageId":"xmqqlh74wb0r.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160130081306.GA2931@ecki.hitronhub.home","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T18:17:24Z","receivedAt":"2016-02-01T18:17:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Ok, then let's take a step back. I do not actually care if git diff and\n> friends say the worktree is clean or not.\n\nYou may not, but many existing scripts people have do.\n\n> But I know that I did not make\n> any modifications to the worktree, because I just did git reset --hard.\n> And now I want to use commands like cherry-pick and checkout without\n> failure. But they can fail, because they essentially use git diff to\n> check if there are worktree changes, and if so refuse to overwrite them.\n\nYes, exactly.\n\n> So, if the check \"Am I allowed to modify the worktree file?\", would go\n> the extra mile to also check if the worktree is clean in the sense that\n> convert_to_worktree(index state) matches the worktree. If this is the\n> case, then it is safe to modify the file because it is the committed\n> state, and can be recovered.\n\nSo in essense, the proposed \"fix\" is \"let's fix it in the right\nway\"?\n\nThe way we defined \"would we lose some changes that are only in the\nworking tree?\", aka \"is the working tree dirty wrt the index?\", has\nbeen to check if \"git add -u\" would change the states in the index.\nAnd for scripted Porcelains and end-user scripts, \"git diff-files\",\naka \"what change would 'git add -u' make to the states in the\nindex?\", has been the command to do the same check.\n\nYour proposal is to redefine \"is the working tree dirty?\"; it would\ncheck if \"git checkout -f\" would change what is in the working tree.\n\nI agree that indeed is \"would we lose some changes that are only in\nthe working tree\", and I think we can do that transparently for\n\"internal\" commands, i.e. without any end-user impact, as the new\ncheck would behave identically when they have sane contents--the\ndifference between the current check and the new check only exists\nwhen the contents in the index contradicts what the user specifies\nfor to-git conversion via eol or clean filter.\n\nWe would need a way for our scripted Porcelains and end-user scripts\nto ask that new question, though, but I think that is not something\ninsurmountable. A new option to \"diff-files\" or something, perhaps,\nwould be workable, but having a new \"git require-clean-work-tree\"\nplumbing, which would replace require_clean_work_tree shell helper\nin git-sh-setup, may be conceptually much cleaner, because the new\ndefinition of \"working tree being clean\" is no longer tied to what\n\"diff\" should say.\n\nI like that as a general direction.\n\n> Regarding performance impact: We only need to do this extra check if the\n> usual check convert_to_git(work tree) against index state fails, and\n> conversion is in effect.\n\nHow would you detect the failure, though? Having contents in the\nindex that contradicts the attributes and eol settings affects the\ncleanliness both ways. Running the working tree contents via to-git\nconversion and hashing may match the blob in the index, declaring\nthat the index entry is \"clean\", but passing the blob to to-worktree\nconversion may produce result different from what is in the\nworktree, which is \"falsely clean\". That is an equally important\ncase that is opposite from what we have been primarily discussing,\nwhich is \"falsely dirty\".\n\n>> Besides, I do not think the above approach really solves the issue,\n>> either. After \"git reset --hard\" to have the contents in the index\n>> dumped to the working tree, if your core.autocrlf is flipped,\n>\n> Indeed, if the user configuration changes, then we cannot always detect\n> this (at least if the filter is an external program, and the behavior of\n> that changes). But the user is in control of that, and we can document\n> this limitation.\n\nThat argument does not result in a very useful result, though.\nBecause the user is in control of what attributes and eol settings\nare in effect in her repository, we can just document that the\ncurrent check will give unspecified result if the indexed contents\ncontradict with that setting, e.g. when you have CRLF encoded data\nin the index but the eol conversion assumes LF in the repository.\nBut this discussion is an attempt to do better than that, no?\n\n> On the other hand, a user who simply follows an upstream repository by\n> doing git pull all the time, and who does not make changes to their\n> configuration, can still run into this issue, because upstream could\n> change .gitattributes. This part we could actually detect by hashing the\n> attributes for each index entry, and if that changes we re-evaluate the\n> file state.\n\nIf this has to bloat each index entry, I do not think solving the\nproblem is worth that cost of that overhead. I'd rather just say\n\"if you have inconsistent data, here is a workaround using 'reset'\nand then 'reset --hard'\" and be done with it.\n\n> This is also an issue only if a smudge filter is in place. The eol\n> conversion which only acts in the convert_to_git direction is not\n> affected.\n\nIIRC, autocrlf=true would strip CR at the end of line in to-git\nconversion, and would add CR in to-worktree conversion. So some eol\nconversion may only act in to-git, but some others do affect both,\nand without needing you to touch attributes.\n"},{"id":"277181","messageId":"20160201193340.GA892@ecki","threadId":"41216","inReplyTo":"xmqqlh74wb0r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2016-02-01T19:33:42Z","receivedAt":"2016-02-01T19:33:42Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Feb 01, 2016 at 10:17:24AM -0800, Junio C Hamano wrote:\n> \n> Your proposal is to redefine \"is the working tree dirty?\"; it would\n> check if \"git checkout -f\" would change what is in the working tree.\n\nI like this definition. Sounds obviously right.\n\n> > Regarding performance impact: We only need to do this extra check if the\n> > usual check convert_to_git(work tree) against index state fails, and\n> > conversion is in effect.\n> \n> How would you detect the failure, though? Having contents in the\n> index that contradicts the attributes and eol settings affects the\n> cleanliness both ways. Running the working tree contents via to-git\n> conversion and hashing may match the blob in the index, declaring\n> that the index entry is \"clean\", but passing the blob to to-worktree\n> conversion may produce result different from what is in the\n> worktree, which is \"falsely clean\".\n\nTrue. But this is what we do today, and I thought at first that we have\nto keep this behavior. The following enables eol conversion on git add,\nbut not on checkout:\n\n printf 'line 1\\r\\n' >dos.txt\n echo '* text' >.gitattributes\n git add dos.txt\n git commit\n\nAfter git add the worktree is considered clean, even though dos.txt\nstill has CRLF line endings, and rm dos.txt && git checkout dos.txt\nre-creates dos.txt with LF line endings. If we change the definition as\nproposed above, then the worktree would be dirty even though we just\ndid git add and git commit.\n\nSo I concluded that we have to treat the worktree clean if either git\nadd -u does not change the index state, _or_ git checkout -f does not\nchange the worktree state.\n\nBut doing only the git checkout -f check makes much more sense. Maybe we\ncan handle the above situation better by doing an implicit\ngit checkout -f after git commit. After all, I would\nexpect git commit to give me exactly the same state that I get later\nwhen I do git checkout for the same commit.\n\n> > On the other hand, a user who simply follows an upstream repository by\n> > doing git pull all the time, and who does not make changes to their\n> > configuration, can still run into this issue, because upstream could\n> > change .gitattributes. This part we could actually detect by hashing the\n> > attributes for each index entry, and if that changes we re-evaluate the\n> > file state.\n> \n> If this has to bloat each index entry, I do not think solving the\n> problem is worth that cost of that overhead. I'd rather just say\n> \"if you have inconsistent data, here is a workaround using 'reset'\n> and then 'reset --hard'\" and be done with it.\n\nWorks for me.\n\n> > This is also an issue only if a smudge filter is in place. The eol\n> > conversion which only acts in the convert_to_git direction is not\n> > affected.\n> \n> IIRC, autocrlf=true would strip CR at the end of line in to-git\n> conversion, and would add CR in to-worktree conversion. So some eol\n> conversion may only act in to-git, but some others do affect both,\n> and without needing you to touch attributes.\n\nI was somehow under the impression that autocrlf=true is discouraged,\nand setting the text attribute to true is the new recommended way to\nconfigure eol conversion. But I see that the Git for Windows installer\nstill offers autocrlf=true as the default option, so clearly we need to\nsupport it well.\n"},{"id":"277183","messageId":"56AFBF8E.7090809@web.de","threadId":"41216","inReplyTo":"xmqqlh74wb0r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-02-01T20:26:54Z","receivedAt":"2016-02-01T20:26:54Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2016-02-01 19.17, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n[]\n> \n> IIRC, autocrlf=true would strip CR at the end of line in to-git\n> conversion, and would add CR in to-worktree conversion. So some eol\n> conversion may only act in to-git, but some others do affect both,\n> and without needing you to touch attributes.\nThat depends, which version of Git you are running.\nIt has changed from the first version of core.autocrlf:\n\ncommit c4805393d73425a5f467f10fa434fb99bfba17ac\nAuthor: Finn Arne Gangstad \nDate: Wed May 12 00:37:57 2010 +0200\n\n autocrlf: Make it work also for un-normalized repositories\n\n Previously, autocrlf would only work well for normalized\n repositories. Any text files that contained CRLF in the repository\n would cause problems, and would be modified when handled with\n core.autocrlf set.\n\n Change autocrlf to not do any conversions to files that in the\n repository already contain a CR. git with autocrlf set will never\n create such a file, or change a LF only file to contain CRs, so the\n (new) assumption is that if a file contains a CR, it is intentional,\n and autocrlf should not change that.\n\n The following sequence should now always be a NOP even with autocrlf\n set (assuming a clean working directory):\n\n git checkout \n touch *\n git add -A . (will add nothing)\n git commit (nothing to commit)\n\n Previously this would break for any text file containing a CR.\n\n Some of you may have been folowing Eyvind's excellent thread about\n trying to make end-of-line translation in git a bit smoother.\n\n I decided to attack the problem from a different angle: Is it possible\n to make autocrlf behave non-destructively for all the previous problem cases?\n\n Stealing the problem from Eyvind's initial mail (paraphrased and\n summarized a bit):\n\n 1. Setting autocrlf globally is a pain since autocrlf does not work well\n with CRLF in the repo\n 2. Setting it in individual repos is hard since you do it \"too late\"\n (the clone will get it wrong)\n 3. If someone checks in a file with CRLF later, you get into problems again\n 4. If a repository once has contained CRLF, you can't tell autocrlf\n at which commit everything is sane again\n 5. autocrlf does needless work if you know that all your users want\n the same EOL style.\n\n I belive that this patch makes autocrlf a safe (and good) default\n setting for Windows, and this solves problems 1-4 (it solves 2 by being\n set by default, which is early enough for clone).\n\n I implemented it by looking for CR charactes in the index, and\n aborting any conversion attempt if this is found.\n-----------------------\nAnd my intention is to do a similar fix for the attributes.\nMore patches coming.\n"},{"id":"277282","messageId":"xmqq37tar9g2.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"20160201193340.GA892@ecki","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-02T23:14:53Z","receivedAt":"2016-02-02T23:14:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Mon, Feb 01, 2016 at 10:17:24AM -0800, Junio C Hamano wrote:\n>> \n>> Your proposal is to redefine \"is the working tree dirty?\"; it would\n>> check if \"git checkout -f\" would change what is in the working tree.\n>\n> I like this definition. Sounds obviously right.\n\nSo this is an illustration. The change to ce_compare_data() has a\nroom for improvement in that it assumes that it can always slurp the\nwhole blob in-core; it should try to use the streaming interface\nwhen it makes sense. Otherwise we would not be able to handle a\nblob that we used to be able to (as index_fd() streams), which would\nbe a regression.\n\nThe change to t0023 is merely an example that shows that existing\ntests assume the convert_to_git() way of defining the dirtyness of\nthe working tree. It used to be OK to have core.autocrlf set to true,\nhave LF terminated file on the working tree and add it to the index,\nand the resulting state was \"We just added it to the index, and\nnobody touched the index nor the working tree file--by definition\nthe working tree IS CLEAN\". With your updated semantics, that no\nlonger is true. \"We just added it, but if we check it out, we would\nnormalize the line ending to be CRLF on the working tree, so the\nworking tree is dirty\" is what happens.\n\nThere are tons of tests that would break the same way all of which\nneeds to be looked at and fixed if we were to go in this direction.\n\n\n read-cache.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++----\n t/t0023-crlf-am.sh | 2 +-\n 2 files changed, 50 insertions(+), 5 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 84616c8..c284f78 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,16 +156,61 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n \t\tce_mark_uptodate(ce);\n }\n \n+/*\n+ * Compare the data in buf with the data in the file pointed by fd and\n+ * return 0 if they are identical, and non-zero if they differ.\n+ */\n+static int compare_with_fd(const char *input, ssize_t len, int fd)\n+{\n+\tfor (;;) {\n+\t\tchar buf[1024 * 16];\n+\t\tssize_t chunk_len, read_len;\n+\n+\t\tchunk_len = sizeof(buf) < len ? sizeof(buf) : len;\n+\t\tread_len = xread(fd, buf, chunk_len ? chunk_len : 1);\n+\n+\t\tif (!read_len)\n+\t\t\t/* EOF on the working tree file */\n+\t\t\treturn !len ? 0 : -1;\n+\n+\t\tif (!len)\n+\t\t\t/* we expected there is nothing left */\n+\t\t\treturn -1;\n+\n+\t\tif (memcmp(buf, input, read_len))\n+\t\t\treturn -1;\n+\t\tinput += read_len;\n+\t\tlen -= read_len;\n+\t}\n+}\n+\n+/*\n+ * Does the file in the working tree match what is in the index?\n+ * That is, do we lose any data from the working tree copy if we\n+ * did a new \"git checkout\" of that path out of the index?\n+ */\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n \tint fd = open(ce->name, O_RDONLY);\n \n \tif (fd >= 0) {\n-\t\tunsigned char sha1[20];\n-\t\tif (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))\n-\t\t\tmatch = hashcmp(sha1, ce->sha1);\n-\t\t/* index_fd() closed the file descriptor already */\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\tvoid *data = read_sha1_file(ce->sha1, &type, &size);\n+\n+\t\tif (type == OBJ_BLOB) {\n+\t\t\tstruct strbuf worktree = STRBUF_INIT;\n+\t\t\tif (convert_to_working_tree(ce->name, data, size,\n+\t\t\t\t\t\t &worktree)) {\n+\t\t\t\tfree(data);\n+\t\t\t\tdata = strbuf_detach(&worktree, &size);\n+\t\t\t}\n+\t\t\tif (!compare_with_fd(data, size, fd))\n+\t\t\t\tmatch = 0;\n+\t\t}\n+\t\tfree(data);\n+\t\tclose(fd);\n \t}\n \treturn match;\n }\ndiff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh\nindex f9bbb91..5c086b4 100755\n--- a/t/t0023-crlf-am.sh\n+++ b/t/t0023-crlf-am.sh\n@@ -27,7 +27,7 @@ EOF\n test_expect_success 'setup' '\n \n \tgit config core.autocrlf true &&\n-\techo foo >bar &&\n+\tprintf \"%s\\r\\n\" foo >bar &&\n \tgit add bar &&\n \ttest_tick &&\n \tgit commit -m initial\n"},{"id":"277310","messageId":"xmqqio26nqk8.fsf@gitster.mtv.corp.google.com","threadId":"41216","inReplyTo":"xmqq37tar9g2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-03T08:31:03Z","receivedAt":"2016-02-03T08:31:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The change to t0023 is merely an example that shows that existing\n> tests assume the convert_to_git() way of defining the dirtyness of\n> the working tree. It used to be OK to have core.autocrlf set to true,\n> have LF terminated file on the working tree and add it to the index,\n> and the resulting state was \"We just added it to the index, and\n> nobody touched the index nor the working tree file--by definition\n> the working tree IS CLEAN\". With your updated semantics, that no\n> longer is true. \"We just added it, but if we check it out, we would\n> normalize the line ending to be CRLF on the working tree, so the\n> working tree is dirty\" is what happens.\n>\n> There are tons of tests that would break the same way all of which\n> needs to be looked at and fixed if we were to go in this direction.\n\nThat made me think further aloud. I haven't thought things through,\nbut I wonder what happens if we do both. That is, we define the\nworking tree file is clean if either:\n\n * the result of running convert_to_git() on the working tree\n contents matches what is in the index (because that would mean\n doing another \"git add\" on the path is a no-op); OR\n\n * the result of running convert_to_working_tree() on the content\n in the index matches what is in the working tree (because that\n would mean doing another \"git checkout -f\" on the path is a\n no-op).\n\nA possible downside (but again, I haven't thought things through, so\nthis may be a non-issue) of doing this is that it may make it even\nharder to \"fix\" an index entry or a working tree file that is\ninconsistent with the user's conversion settings. Even when \"git\nadd\" would allow the user to fix an index entry by applying (an\nupdated) convert_to_git() filter to the working tree file, because\nof the new rule that works in the opposite direction, we would end\nup saying \"the working tree file is clean, and there is no point\ndoing 'git add'\". And vice versa for fixing a working tree file by\nrunning \"git checkout\".\n\nAlso this will make \"update-index --refresh\" potentially take twice\nas long for paths that are not known to be clean and indeed dirty,\nas they would need to be processed twice.\n\nAn updated patch to do so would look like this. At least we don't\nhave to update the expectation t0023 makes with this approach.\n\n read-cache.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 61 insertions(+)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 84616c8..42d9452 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,17 +156,78 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n \t\tce_mark_uptodate(ce);\n }\n \n+/*\n+ * Compare the data in buf with the data in the file pointed by fd and\n+ * return 0 if they are identical, and non-zero if they differ.\n+ */\n+static int compare_with_fd(const char *input, ssize_t len, int fd)\n+{\n+\tfor (;;) {\n+\t\tchar buf[1024 * 16];\n+\t\tssize_t chunk_len, read_len;\n+\n+\t\tchunk_len = sizeof(buf) < len ? sizeof(buf) : len;\n+\t\tread_len = xread(fd, buf, chunk_len ? chunk_len : 1);\n+\n+\t\tif (!read_len)\n+\t\t\t/* EOF on the working tree file */\n+\t\t\treturn !len ? 0 : -1;\n+\n+\t\tif (!len)\n+\t\t\t/* we expected there is nothing left */\n+\t\t\treturn -1;\n+\n+\t\tif (memcmp(buf, input, read_len))\n+\t\t\treturn -1;\n+\t\tinput += read_len;\n+\t\tlen -= read_len;\n+\t}\n+}\n+\n+/*\n+ * Does the file in the working tree match what is in the index?\n+ */\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n \tint fd = open(ce->name, O_RDONLY);\n \n+\t/*\n+\t * Would another \"git add\" on the path change what is in the\n+\t * index for the path?\n+\t */\n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n \t\tif (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))\n \t\t\tmatch = hashcmp(sha1, ce->sha1);\n \t\t/* index_fd() closed the file descriptor already */\n \t}\n+\tif (!match)\n+\t\treturn match;\n+\n+\t/*\n+\t * Would another \"git checkout -f\" out of the index change\n+\t * what is in the working tree file?\n+\t */\n+\tfd = open(ce->name, O_RDONLY);\n+\tif (fd >= 0) {\n+\t\tenum object_type type;\n+\t\tunsigned long size;\n+\t\tvoid *data = read_sha1_file(ce->sha1, &type, &size);\n+\n+\t\tif (type == OBJ_BLOB) {\n+\t\t\tstruct strbuf worktree = STRBUF_INIT;\n+\t\t\tif (convert_to_working_tree(ce->name, data, size,\n+\t\t\t\t\t\t &worktree)) {\n+\t\t\t\tfree(data);\n+\t\t\t\tdata = strbuf_detach(&worktree, &size);\n+\t\t\t}\n+\t\t\tif (!compare_with_fd(data, size, fd))\n+\t\t\t\tmatch = 0;\n+\t\t}\n+\t\tfree(data);\n+\t\tclose(fd);\n+\t}\n \treturn match;\n }\n \n"}]}