{"thread":{"id":"40629","subject":"[RFC] t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" flaky?","startedAt":"2015-10-24T13:08:39Z","lastAt":"2015-11-05T16:21:53Z","messageCount":9,"participants":["Lars Schneider","Fredrik Medley","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"272165","messageId":"9A3BCDA2-5915-4287-A385-95A3ACCBB850@gmail.com","threadId":"40629","inReplyTo":null,"subject":"[RFC] t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" flaky?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-10-24T13:08:39Z","receivedAt":"2015-10-24T13:08:39Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Hi,\n\nwhile working on the Git CI integration I noticed that t5516 \"75 - deny fetch \nunreachable SHA1, allowtipsha1inwant=true\" (introduced in 68ee628) seems to be \nflaky on TravisCI. I get the following output in verbose mode:\n\n>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>\nexpecting success: \n mk_empty testrepo &&\n (\n cd testrepo &&\n git config uploadpack.allowtipsha1inwant $configallowtipsha1inwant &&\n git commit --allow-empty -m foo &&\n git commit --allow-empty -m bar &&\n git commit --allow-empty -m xyz\n ) &&\n SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&\n SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&\n SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&\n (\n cd testrepo &&\n git reset --hard $SHA1_2 &&\n git cat-file commit $SHA1_1 &&\n git cat-file commit $SHA1_3\n ) &&\n mk_empty shallow &&\n (\n cd shallow &&\n test_must_fail git fetch ../testrepo/.git $SHA1_3 &&\n test_must_fail git fetch ../testrepo/.git $SHA1_1 &&\n git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n git fetch ../testrepo/.git $SHA1_1 &&\n git cat-file commit $SHA1_1 &&\n test_must_fail git cat-file commit $SHA1_2 &&\n git fetch ../testrepo/.git $SHA1_2 &&\n git cat-file commit $SHA1_2 &&\n test_must_fail git fetch ../testrepo/.git $SHA1_3\n )\n \nInitialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/testrepo/.git/\n[master (root-commit) a6b22ca] foo\n Author: A U Thor \n[master 5e26403] bar\n Author: A U Thor \n[master 64ea4c1] xyz\n Author: A U Thor \nHEAD is now at 5e26403 bar\ntree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\nauthor A U Thor 1112912053 -0700\ncommitter C O Mitter 1112912053 -0700\nfoo\ntree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\nparent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad\nauthor A U Thor 1112912053 -0700\ncommitter C O Mitter 1112912053 -0700\nxyz\nInitialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/shallow/.git/\nfatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886\ntest_must_fail: died by signal: git fetch ../testrepo/.git 64ea4c133d59fa98e86a771eda009872d6ab2886\nnot ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true \n<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n\n\"git fetch ../testrepo/.git $SHA1_3\" seems to die as follows:\n1. fetch-pack.c:408 goto done;\n2. fetch-pack.c:447 done:\n3. fetch-pack.c:229 static void send_request... (send \"0009done\\n\" --> 9 bytes)\n4. write_or_die.c:74 write_or_die\n5. wrapper.c:331 write_in_full\n6. wrapper.c:281 xwrite\n7. wrapper.c:287 write --> just dies with exit code 141?!\n(use 4688abf to match the line numbers)\n\nYou can find the full logs about the CI run here:\nhttps://travis-ci.org/larsxschneider/git/jobs/86984110\n\nI repeatedly executed this test to demonstrate the flakiness:\nfailed after 100 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181753\nfailed after 1876 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181754\n(all executed on bbd2929 from https://github.com/larsxschneider/git)\n\nUnfortunately I was not able to make it fail on my local machine (OS X 10.9) \nnor on my VM (Ubuntu 14.10). Travis CI uses Ubuntu 12.04 LTS Server Edition 64 \nbit with a AUFS file system.\n\nHas anyone an idea what might causes these failures or can give me pointers how\nto investigate it?\n\nThank you,\nLars"},{"id":"272184","messageId":"CABA5-z=1N5=8huSr=BLmjj_KHLbMMiXdo0qok7Mc_ZOeB=J9jA@mail.gmail.com","threadId":"40629","inReplyTo":"9A3BCDA2-5915-4287-A385-95A3ACCBB850@gmail.com","subject":"Re: [RFC] t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" flaky?","fromName":"Fredrik Medley","fromEmail":"fredrik.medley@gmail.com","sentAt":"2015-10-25T07:18:49Z","receivedAt":"2015-10-25T07:18:49Z","isPatch":false,"sender":{"key":"fredrik.medley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5401799?v=4"},"body":"I think the following happens:\n1. The remote upload-pack finds out \"not our ref\"\n2. The remote send a response and close the pipe\n3. fetch-pack still tries to write commands to the remote upload-pack\n4. Because the connection has already been closed, writing will fail\nwith EPIPE which is detected by write_or_die() -> check_pipe()\nresulting in die(141)\n\nThe reason for the test to succeed, i.e. git-fetch fails with 128 (or\n1?), in most cases must be because it manages to write everything\nbefore the context switch to the remote upload-pack occurs.\n\nWhat is actually the wanted outcome? Should git-fetch try to continue\nto see if the already received response is enough to continue as\nnormal?\n\nBest regards,\nFredrik\n\n2015-10-24 15:08 GMT+02:00 Lars Schneider :\n> Hi,\n>\n> while working on the Git CI integration I noticed that t5516 \"75 - deny fetch\n> unreachable SHA1, allowtipsha1inwant=true\" (introduced in 68ee628) seems to be\n> flaky on TravisCI. I get the following output in verbose mode:\n>\n>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>\n> expecting success:\n> mk_empty testrepo &&\n> (\n> cd testrepo &&\n> git config uploadpack.allowtipsha1inwant $configallowtipsha1inwant &&\n> git commit --allow-empty -m foo &&\n> git commit --allow-empty -m bar &&\n> git commit --allow-empty -m xyz\n> ) &&\n> SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&\n> SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&\n> SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&\n> (\n> cd testrepo &&\n> git reset --hard $SHA1_2 &&\n> git cat-file commit $SHA1_1 &&\n> git cat-file commit $SHA1_3\n> ) &&\n> mk_empty shallow &&\n> (\n> cd shallow &&\n> test_must_fail git fetch ../testrepo/.git $SHA1_3 &&\n> test_must_fail git fetch ../testrepo/.git $SHA1_1 &&\n> git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n> git fetch ../testrepo/.git $SHA1_1 &&\n> git cat-file commit $SHA1_1 &&\n> test_must_fail git cat-file commit $SHA1_2 &&\n> git fetch ../testrepo/.git $SHA1_2 &&\n> git cat-file commit $SHA1_2 &&\n> test_must_fail git fetch ../testrepo/.git $SHA1_3\n> )\n>\n> Initialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/testrepo/.git/\n> [master (root-commit) a6b22ca] foo\n> Author: A U Thor \n> [master 5e26403] bar\n> Author: A U Thor \n> [master 64ea4c1] xyz\n> Author: A U Thor \n> HEAD is now at 5e26403 bar\n> tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n> author A U Thor 1112912053 -0700\n> committer C O Mitter 1112912053 -0700\n> foo\n> tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n> parent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad\n> author A U Thor 1112912053 -0700\n> committer C O Mitter 1112912053 -0700\n> xyz\n> Initialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/shallow/.git/\n> fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886\n> test_must_fail: died by signal: git fetch ../testrepo/.git 64ea4c133d59fa98e86a771eda009872d6ab2886\n> not ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\n> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n>\n> \"git fetch ../testrepo/.git $SHA1_3\" seems to die as follows:\n> 1. fetch-pack.c:408 goto done;\n> 2. fetch-pack.c:447 done:\n> 3. fetch-pack.c:229 static void send_request... (send \"0009done\\n\" --> 9 bytes)\n> 4. write_or_die.c:74 write_or_die\n> 5. wrapper.c:331 write_in_full\n> 6. wrapper.c:281 xwrite\n> 7. wrapper.c:287 write --> just dies with exit code 141?!\n> (use 4688abf to match the line numbers)\n>\n> You can find the full logs about the CI run here:\n> https://travis-ci.org/larsxschneider/git/jobs/86984110\n>\n> I repeatedly executed this test to demonstrate the flakiness:\n> failed after 100 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181753\n> failed after 1876 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181754\n> (all executed on bbd2929 from https://github.com/larsxschneider/git)\n>\n> Unfortunately I was not able to make it fail on my local machine (OS X 10.9)\n> nor on my VM (Ubuntu 14.10). Travis CI uses Ubuntu 12.04 LTS Server Edition 64\n> bit with a AUFS file system.\n>\n> Has anyone an idea what might causes these failures or can give me pointers how\n> to investigate it?\n>\n> Thank you,\n> Lars\n"},{"id":"272193","messageId":"CFEB6E3F-48A0-41D8-A8FD-D48B806461DB@gmail.com","threadId":"40629","inReplyTo":"CABA5-z=1N5=8huSr=BLmjj_KHLbMMiXdo0qok7Mc_ZOeB=J9jA@mail.gmail.com","subject":"Re: [RFC] t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" flaky?","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-10-25T13:47:58Z","receivedAt":"2015-10-25T13:47:58Z","isPatch":false,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"Thanks for your explanation Fredrik! However, I believe your 4. step is not what happens in the current implementation as the write call in wrapper.c dies directly. I see three ways to fix the problem:\n\n(1) Make upload-pack wait for a response (with timeout) before it closes the pipe. However, I believe this would not be in line with the general Git philosophy stated in \"git.c\" (added in 7559a1be):\n\"Many parts of Git have subprograms communicate via pipe, expect the upstream of a pipe to die with SIGPIPE when the downstream of a pipe does not need to read all that is written.\"\n\n(2) Ignore SIGPIPE errors when \"fetch-pack\" sends a \"done\" using \"sigchain_push(SIGPIPE, SIG_IGN)\" / \"sigchain_pop(SIGPIPE)\". However, then the check_pipe function (added in 756e676c) kicks in and we would need to work around that as well.\n\n(3) Add a method \"test_must_fail_or_die\" to \"test-lib-functions.sh\". This method accepts exit codes 129 wrote:\n\n> I think the following happens:\n> 1. The remote upload-pack finds out \"not our ref\"\n> 2. The remote send a response and close the pipe\n> 3. fetch-pack still tries to write commands to the remote upload-pack\n> 4. Because the connection has already been closed, writing will fail\n> with EPIPE which is detected by write_or_die() -> check_pipe()\n> resulting in die(141)\n> \n> The reason for the test to succeed, i.e. git-fetch fails with 128 (or\n> 1?), in most cases must be because it manages to write everything\n> before the context switch to the remote upload-pack occurs.\n> \n> What is actually the wanted outcome? Should git-fetch try to continue\n> to see if the already received response is enough to continue as\n> normal?\n> \n> Best regards,\n> Fredrik\n> \n> 2015-10-24 15:08 GMT+02:00 Lars Schneider :\n>> Hi,\n>> \n>> while working on the Git CI integration I noticed that t5516 \"75 - deny fetch\n>> unreachable SHA1, allowtipsha1inwant=true\" (introduced in 68ee628) seems to be\n>> flaky on TravisCI. I get the following output in verbose mode:\n>> \n>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> \n>> expecting success:\n>> mk_empty testrepo &&\n>> (\n>> cd testrepo &&\n>> git config uploadpack.allowtipsha1inwant $configallowtipsha1inwant &&\n>> git commit --allow-empty -m foo &&\n>> git commit --allow-empty -m bar &&\n>> git commit --allow-empty -m xyz\n>> ) &&\n>> SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&\n>> SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&\n>> SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&\n>> (\n>> cd testrepo &&\n>> git reset --hard $SHA1_2 &&\n>> git cat-file commit $SHA1_1 &&\n>> git cat-file commit $SHA1_3\n>> ) &&\n>> mk_empty shallow &&\n>> (\n>> cd shallow &&\n>> test_must_fail git fetch ../testrepo/.git $SHA1_3 &&\n>> test_must_fail git fetch ../testrepo/.git $SHA1_1 &&\n>> git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&\n>> git fetch ../testrepo/.git $SHA1_1 &&\n>> git cat-file commit $SHA1_1 &&\n>> test_must_fail git cat-file commit $SHA1_2 &&\n>> git fetch ../testrepo/.git $SHA1_2 &&\n>> git cat-file commit $SHA1_2 &&\n>> test_must_fail git fetch ../testrepo/.git $SHA1_3\n>> )\n>> \n>> Initialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/testrepo/.git/\n>> [master (root-commit) a6b22ca] foo\n>> Author: A U Thor \n>> [master 5e26403] bar\n>> Author: A U Thor \n>> [master 64ea4c1] xyz\n>> Author: A U Thor \n>> HEAD is now at 5e26403 bar\n>> tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>> author A U Thor 1112912053 -0700\n>> committer C O Mitter 1112912053 -0700\n>> foo\n>> tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n>> parent 5e26403b4485d7a44fd8b65dc3f71e21c0dd6fad\n>> author A U Thor 1112912053 -0700\n>> committer C O Mitter 1112912053 -0700\n>> xyz\n>> Initialized empty Git repository in /home/travis/build/larsxschneider/git/t/trash directory.t5516-fetch-push/shallow/.git/\n>> fatal: git upload-pack: not our ref 64ea4c133d59fa98e86a771eda009872d6ab2886\n>> test_must_fail: died by signal: git fetch ../testrepo/.git 64ea4c133d59fa98e86a771eda009872d6ab2886\n>> not ok 75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\n>> <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<\n>> \n>> \"git fetch ../testrepo/.git $SHA1_3\" seems to die as follows:\n>> 1. fetch-pack.c:408 goto done;\n>> 2. fetch-pack.c:447 done:\n>> 3. fetch-pack.c:229 static void send_request... (send \"0009done\\n\" --> 9 bytes)\n>> 4. write_or_die.c:74 write_or_die\n>> 5. wrapper.c:331 write_in_full\n>> 6. wrapper.c:281 xwrite\n>> 7. wrapper.c:287 write --> just dies with exit code 141?!\n>> (use 4688abf to match the line numbers)\n>> \n>> You can find the full logs about the CI run here:\n>> https://travis-ci.org/larsxschneider/git/jobs/86984110\n>> \n>> I repeatedly executed this test to demonstrate the flakiness:\n>> failed after 100 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181753\n>> failed after 1876 attempts: https://travis-ci.org/larsxschneider/git/jobs/87181754\n>> (all executed on bbd2929 from https://github.com/larsxschneider/git)\n>> \n>> Unfortunately I was not able to make it fail on my local machine (OS X 10.9)\n>> nor on my VM (Ubuntu 14.10). Travis CI uses Ubuntu 12.04 LTS Server Edition 64\n>> bit with a AUFS file system.\n>> \n>> Has anyone an idea what might causes these failures or can give me pointers how\n>> to investigate it?\n>> \n>> Thank you,\n>> Lars\n"},{"id":"272206","messageId":"xmqq8u6qluh2.fsf@gitster.mtv.corp.google.com","threadId":"40629","inReplyTo":"CFEB6E3F-48A0-41D8-A8FD-D48B806461DB@gmail.com","subject":"Re: [RFC] t5516 \"75 - deny fetch unreachable SHA1, allowtipsha1inwant=true\" flaky?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-25T17:38:01Z","receivedAt":"2015-10-25T17:38:01Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> (1) Make upload-pack wait for a response (with timeout) before it\n> closes the pipe. However, I believe this would not be in line with\n> the general Git philosophy stated in \"git.c\" (added in 7559a1be):\n> \"Many parts of Git have subprograms communicate via pipe, expect\n> the upstream of a pipe to die with SIGPIPE when the downstream of\n> a pipe does not need to read all that is written.\"\n\nWhile I think \"waiting\" with \"timeout\" is undesirable for the\nupload-pack / fetch-pack communication, you are rejecting it for a\nwrong reason.\n\nWhat you quoted is not even a Git philosophy. Yes, many parts do\nwant to behave like that, and the \"restore\" needs to make sure that\nsuch a behaviour happens by fixing the environment given to us by\nthe process spawning us.\n\nThat does not mean some parts of the system, if they want to run\nSIGPIPE ignored, must not do so because it goes \"against the\nphilosophy\". \n\nIt only means that these parts of the system, if they choose to,\nmust arrange to do so themselves. That \"restore\" thing is merely to\ngive us a known good state to start from.\n\n> (2) Ignore SIGPIPE errors when \"fetch-pack\" sends a \"done\" using\n> \"sigchain_push(SIGPIPE, SIG_IGN)\" /\n> \"sigchain_pop(SIGPIPE)\". However, then the check_pipe function\n> (added in 756e676c) kicks in and we would need to work around that\n> as well.\n\nI am not sure if we are looking at the issue the right way once we\nstart saying 'do this _when_ fetch sends ...'.\n\nThe communication between fetch and upload goes bidirectionally in\nan overlapping fashion, and at any point in the communication, not\nlimited to \"after fetch sends 'done'\", the other side can decide to\ndisconnect while one side is sending. If you are lucky, you will\nfinish sending the current batch and try to read from the other side\nand notice \"the remote end hung up unexpectedly\", and if you are\nunlucky, you find your write cannot go through due to broken pipe.\n\nDying with SIGPIPE would not give us a chance to clean things up\n(i.e. react to the \"not our ref\" error in a more application\nspecific way), so the current behaviour has a room for improvement,\nbut I suspect that changes required for \"dying more nicely\" would be\nrather large [*1*]; I am not convinced if it is worth the effort.\n\nThat leaves us to something along this line...\n\n> (3) Add a method \"test_must_fail_or_die\" to\n> \"test-lib-functions.sh\". This method accepts exit codes 129 too. Use the new method in t5516.\n\n... but I have to wonder if 129 writes:\n\n> That leaves us to something along this line...\n>\n>> (3) Add a method \"test_must_fail_or_die\" to\n>> \"test-lib-functions.sh\". This method accepts exit codes 129> too. Use the new method in t5516.\n>\n> ... but I have to wonder if 129 that the only error we expect, other than the orderly shutdown, is\n> reception of sigpipe.\n\nSo, how about doing something like this as a starter. All of the\nobject transport codepath share \"we may notice that the other end\ndisconnected, or that the other end explicitly told us it found an\nerror, and die, or the other end may have died, perhaps after giving\na human-readable error message, and we end up dying when we try to\ntell them more with SIGPIPE\", and that by itself is not a bug in the\nreal life---we will exit with non-zero status and that is a sign\nenough for the user to know that the command has failed.\n\n\n\n t/test-lib-functions.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 6dffb8b..b1f950d 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -579,6 +579,9 @@ test_must_fail () {\n \tif test $exit_code = 0; then\n \t\techo >&2 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n+\telif test $exit_code -eq 141; then\n+\t\techo >&2 \"test_must_fail: died with sigpipe: $*\"\n+\t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192; then\n \t\techo >&2 \"test_must_fail: died by signal: $*\"\n \t\treturn 1\n"},{"id":"272581","messageId":"xmqqr3kcjbll.fsf_-_@gitster.mtv.corp.google.com","threadId":"40629","inReplyTo":"xmqqvb9ojcmf.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] test: accept death by SIGPIPE as a valid failure mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-30T21:22:14Z","receivedAt":"2015-10-30T21:22:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On a local host, the object/history transport code often talks over\npipe with the other side. The other side may notice some (expected)\nfailure, send the error message either to our process or to the\nstandard error and hung up. In such codepaths, if timing were not\nunfortunate, our side would receive the report of (expected) failure\nfrom the other side over the pipe and die(). Otherwise, our side\nmay still be trying to talk to it and would die with a SIGPIPE.\n\nThis was observed as an intermittent breakage in t5516 by a few\npeople.\n\nIn the real-life scenario, either mode of death exits with a\nnon-zero status, and the user would learn that the command failed.\nThe test_must_fail helper should also know that dying with SIGPIPE\nis one of the valid failure modes when we are expecting the tested\noperation to notice problem and fail.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This time with a real commit log message. As the goal is to\n treat death-by-sigpipe just like a call to die(), this version\n silently lets the test pass instead of giving a message to the\n standard error stream.\n\n t/test-lib-functions.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 8f8858a..4f40ffe 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -556,6 +556,9 @@ test_must_fail () {\n \tif test $exit_code = 0; then\n \t\techo >&2 \"test_must_fail: command succeeded: $*\"\n \t\treturn 1\n+\telif test $exit_code -eq 141; then\n+\t\t# died with sigpipe\n+\t\treturn 0\n \telif test $exit_code -gt 129 && test $exit_code -le 192; then\n \t\techo >&2 \"test_must_fail: died by signal: $*\"\n \t\treturn 1\n-- \n2.6.2-456-gc12a6fe\n"},{"id":"272949","messageId":"20151105074730.GA6819@sigill.intra.peff.net","threadId":"40629","inReplyTo":"xmqqr3kcjbll.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test: accept death by SIGPIPE as a valid failure mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-05T07:47:31Z","receivedAt":"2015-11-05T07:47:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 30, 2015 at 02:22:14PM -0700, Junio C Hamano wrote:\n\n> On a local host, the object/history transport code often talks over\n> pipe with the other side. The other side may notice some (expected)\n> failure, send the error message either to our process or to the\n> standard error and hung up. In such codepaths, if timing were not\n> unfortunate, our side would receive the report of (expected) failure\n> from the other side over the pipe and die(). Otherwise, our side\n> may still be trying to talk to it and would die with a SIGPIPE.\n> \n> This was observed as an intermittent breakage in t5516 by a few\n> people.\n> \n> In the real-life scenario, either mode of death exits with a\n> non-zero status, and the user would learn that the command failed.\n> The test_must_fail helper should also know that dying with SIGPIPE\n> is one of the valid failure modes when we are expecting the tested\n> operation to notice problem and fail.\n\nSorry for the slow review; before commenting I wanted to dig into\nwhether this SIGPIPE ambiguity was avoidable in the first place.\n\nI think the answer is \"probably not\". We do call write_or_die() pretty\nconsistently in the network-aware programs. So we could ignore SIGPIPE,\nand then we would catch EPIPE (of course, we convert that into SIGPIPE\nin many places, but we do not have to do so). But since the SIGPIPE\nbehavior is global, that carries the risk of us failing to check a write\nagainst some other descriptor. It's probably not worth it.\n\nTeaching the tests to handle both cases seems like a reasonable\nworkaround. Changing test_must_fail covers a lot of cases; I wondered if\nthere are other tests that would not want to silently cover up a SIGPIPE\ndeath. But I could not really think of a plausible reason.\n\nSo I think your patch is the best thing to do.\n\n-Peff\n"},{"id":"272952","messageId":"457EAFF7-C649-4DEB-BB9E-E82851DC7F8A@gmail.com","threadId":"40629","inReplyTo":"20151105074730.GA6819@sigill.intra.peff.net","subject":"Re: [PATCH] test: accept death by SIGPIPE as a valid failure mode","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2015-11-05T09:34:53Z","receivedAt":"2015-11-05T09:34:53Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 05 Nov 2015, at 08:47, Jeff King wrote:\n> \n> On Fri, Oct 30, 2015 at 02:22:14PM -0700, Junio C Hamano wrote:\n> \n>> On a local host, the object/history transport code often talks over\n>> pipe with the other side. The other side may notice some (expected)\n>> failure, send the error message either to our process or to the\n>> standard error and hung up. In such codepaths, if timing were not\n>> unfortunate, our side would receive the report of (expected) failure\n>> from the other side over the pipe and die(). Otherwise, our side\n>> may still be trying to talk to it and would die with a SIGPIPE.\n>> \n>> This was observed as an intermittent breakage in t5516 by a few\n>> people.\n>> \n>> In the real-life scenario, either mode of death exits with a\n>> non-zero status, and the user would learn that the command failed.\n>> The test_must_fail helper should also know that dying with SIGPIPE\n>> is one of the valid failure modes when we are expecting the tested\n>> operation to notice problem and fail.\n> \n> Sorry for the slow review; before commenting I wanted to dig into\n> whether this SIGPIPE ambiguity was avoidable in the first place.\n> \n> I think the answer is \"probably not\". We do call write_or_die() pretty\n> consistently in the network-aware programs. So we could ignore SIGPIPE,\n> and then we would catch EPIPE (of course, we convert that into SIGPIPE\n> in many places, but we do not have to do so). But since the SIGPIPE\n> behavior is global, that carries the risk of us failing to check a write\n> against some other descriptor. It's probably not worth it.\n> \n> Teaching the tests to handle both cases seems like a reasonable\n> workaround. Changing test_must_fail covers a lot of cases; I wondered if\n> there are other tests that would not want to silently cover up a SIGPIPE\n> death. But I could not really think of a plausible reason.\n> \n> So I think your patch is the best thing to do.\n> \n> -Peff\n\nOh, I missed this email thread. I am still working on a stable Travis-CI integration and I ran into this issue a few times. I fixed it in my (not yet published) patch with an additional function \"test_must_fail_or_sigpipe\" that I've used for all tests affected by this issue. Modifying the \"test_must_fail\" function seemed too risky for me as I don't understand all possible implications. However, if you don't see a problem then this is fine with me.\n\n- Lars"},{"id":"272957","messageId":"xmqqvb9g76xq.fsf@gitster.mtv.corp.google.com","threadId":"40629","inReplyTo":"457EAFF7-C649-4DEB-BB9E-E82851DC7F8A@gmail.com","subject":"Re: [PATCH] test: accept death by SIGPIPE as a valid failure mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-05T16:21:53Z","receivedAt":"2015-11-05T16:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n> Oh, I missed this email thread. I am still working on a stable\n> Travis-CI integration and I ran into this issue a few times. I\n> fixed it in my (not yet published) patch with an additional\n> function \"test_must_fail_or_sigpipe\" that I've used for all tests\n> affected by this issue. Modifying the \"test_must_fail\" function\n> seemed too risky for me as I don't understand all possible\n> implications. However, if you don't see a problem then this is\n> fine with me.\n\nIt's not that I don't see a problem at all. You constructed a good\nsummary of the issues in three bullet points, that lead me to think\nthat it is the right approach to tweak the way the tests evaluate\nthe outcome, but then nothing came out of the discussion, so I sent\nout a \"how about doing it this way\" to make sure this topic will not\nbe forgotten. There is nothing more to it, and \"how about...\" is in\nno way final.\n\nThere obviously are pros and cons between introducing your new\nhelper to mark the ones that are allowed to catch SIGPIPE and\nchanging all occurrences of test_must_fail. I do not have a strong\nopinion yet, but it needs to be discussed and decided.\n"}]}