{"thread":{"id":"44442","subject":"[PATCH] t6026-merge-attr: don't fail if sleep exits early","startedAt":"2016-11-08T17:03:11Z","lastAt":"2016-11-10T23:04:07Z","messageCount":12,"participants":["Andreas Schwab","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305562","messageId":"mvmtwbhdhvb.fsf@hawking.suse.de","threadId":"44442","inReplyTo":null,"subject":"[PATCH] t6026-merge-attr: don't fail if sleep exits early","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2016-11-08T17:03:04Z","receivedAt":"2016-11-08T17:03:11Z","isPatch":true,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Commit 5babb5bdb3 (\"t6026-merge-attr: clean up background process at end\nof test case\") added a kill command to clean up after the test, but this\ncan fail if the sleep command exits before the cleanup is executed.\nIgnore the error from the kill command.\n\nSigned-off-by: Andreas Schwab \n---\nThe failure can be simulated by adding a sleep after the last command to\ndelay the cleanup.\n---\n t/t6026-merge-attr.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 7a6e33e673..2672b15aa3 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -187,7 +187,7 @@ test_expect_success 'custom merge does not lock index' '\n \t\tsleep 1 &\n \t\techo $! >sleep.pid\n \tEOF\n-\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n+\ttest_when_finished \"kill \\$(cat sleep.pid) || :\" &&\n \n \ttest_write_lines >.gitattributes \\\n \t\t\"* merge=ours\" \"text merge=sleep-one-second\" &&\n-- \n2.10.2\n\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"305567","messageId":"20161108200543.7ivo3xoafdl4uw6h@sigill.intra.peff.net","threadId":"44442","inReplyTo":"mvmtwbhdhvb.fsf@hawking.suse.de","subject":"Re: [PATCH] t6026-merge-attr: don't fail if sleep exits early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-08T20:05:44Z","receivedAt":"2016-11-08T20:05:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 08, 2016 at 06:03:04PM +0100, Andreas Schwab wrote:\n\n> Commit 5babb5bdb3 (\"t6026-merge-attr: clean up background process at end\n> of test case\") added a kill command to clean up after the test, but this\n> can fail if the sleep command exits before the cleanup is executed.\n> Ignore the error from the kill command.\n> \n> Signed-off-by: Andreas Schwab \n> ---\n> The failure can be simulated by adding a sleep after the last command to\n> delay the cleanup.\n\nThanks for the reproduction hint. I sometimes run the test suite through\na \"stress\" script that sees if a test script racily fails under load,\nbut I wasn't able to trigger it here (I guess a full second is just too\nlong even under high load). But the extra sleep makes it obvious.\n\nLooks like the original is in v2.10.1, so this should probably go to\nmaint, as well as the upcoming v2.11-rc1.\n\n-Peff\n"},{"id":"305628","messageId":"alpine.DEB.2.20.1611091437280.72596@virtualbox","threadId":"44442","inReplyTo":"20161108200543.7ivo3xoafdl4uw6h@sigill.intra.peff.net","subject":"Re: [PATCH] t6026-merge-attr: don't fail if sleep exits early","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-09T13:47:13Z","receivedAt":"2016-11-09T13:47:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi \n\nOn Tue, 8 Nov 2016, Jeff King wrote:\n\n> On Tue, Nov 08, 2016 at 06:03:04PM +0100, Andreas Schwab wrote:\n> \n> > Commit 5babb5bdb3 (\"t6026-merge-attr: clean up background process at end\n> > of test case\") added a kill command to clean up after the test, but this\n> > can fail if the sleep command exits before the cleanup is executed.\n> > Ignore the error from the kill command.\n> > \n> > Signed-off-by: Andreas Schwab \n> > ---\n> > The failure can be simulated by adding a sleep after the last command to\n> > delay the cleanup.\n> \n> Thanks for the reproduction hint. I sometimes run the test suite through\n> a \"stress\" script that sees if a test script racily fails under load,\n> but I wasn't able to trigger it here (I guess a full second is just too\n> long even under high load). But the extra sleep makes it obvious.\n> \n> Looks like the original is in v2.10.1, so this should probably go to\n> maint, as well as the upcoming v2.11-rc1.\n\nThe reason why we do not ignore kill errors is that we want to make sure\nthat the script *actually ran*. Otherwise, the thing we need to test here\ndoes not necessarily get tested.\n\nSo I would rather go with the patch Hannes hinted at when he said that he\ndid not want to change the file name (as it would have made the diff less\nobvious): increase the number of seconds.\n\nWill send out a superseding patch in a minute,\nDscho\n"},{"id":"305633","messageId":"mvmzil8btzb.fsf@hawking.suse.de","threadId":"44442","inReplyTo":"alpine.DEB.2.20.1611091437280.72596@virtualbox","subject":"Re: [PATCH] t6026-merge-attr: don't fail if sleep exits early","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2016-11-09T14:36:40Z","receivedAt":"2016-11-09T14:36:46Z","isPatch":true,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 09 2016, Johannes Schindelin wrote:\n\n> The reason why we do not ignore kill errors is that we want to make sure\n> that the script *actually ran*. Otherwise, the thing we need to test here\n> does not necessarily get tested.\n\nThat can be tested by looking for the pid file.\n\nAndreas.\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"305635","messageId":"20161109153128.aqm2lgdntdlycnaq@sigill.intra.peff.net","threadId":"44442","inReplyTo":"mvmzil8btzb.fsf@hawking.suse.de","subject":"Re: [PATCH] t6026-merge-attr: don't fail if sleep exits early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-09T15:31:28Z","receivedAt":"2016-11-09T15:31:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 09, 2016 at 03:36:40PM +0100, Andreas Schwab wrote:\n\n> On Nov 09 2016, Johannes Schindelin wrote:\n> \n> > The reason why we do not ignore kill errors is that we want to make sure\n> > that the script *actually ran*. Otherwise, the thing we need to test here\n> > does not necessarily get tested.\n> \n> That can be tested by looking for the pid file.\n\nI agree that makes the intent a lot more obvious. Having a necessary\ncondition of the test stuffed into a test_when_finished block seems\ncounter-intuitive.\n\n-Peff\n"},{"id":"305669","messageId":"mvm8tsrbusp.fsf_-_@hawking.suse.de","threadId":"44442","inReplyTo":"20161109153128.aqm2lgdntdlycnaq@sigill.intra.peff.net","subject":"[PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Andreas Schwab","fromEmail":"schwab@suse.de","sentAt":"2016-11-10T08:31:18Z","receivedAt":"2016-11-10T08:31:24Z","isPatch":true,"sender":{"key":"schwab@suse.de","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Commit 5babb5bdb3 (\"t6026-merge-attr: clean up background process at end\nof test case\") added a kill command to clean up after the test, but this\ncan fail if the sleep command exits before the cleanup is executed.\nIgnore the error from the kill command.\n\nExplicitly check for the existence of the pid file to test that the merge\ndriver was actually called.\n\nSigned-off-by: Andreas Schwab \n---\n t/t6026-merge-attr.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 7a6e33e673..03d13d00b5 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -187,13 +187,14 @@ test_expect_success 'custom merge does not lock index' '\n \t\tsleep 1 &\n \t\techo $! >sleep.pid\n \tEOF\n-\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n+\ttest_when_finished \"kill \\$(cat sleep.pid) || :\" &&\n \n \ttest_write_lines >.gitattributes \\\n \t\t\"* merge=ours\" \"text merge=sleep-one-second\" &&\n \ttest_config merge.ours.driver true &&\n \ttest_config merge.sleep-one-second.driver ./sleep-one-second.sh &&\n-\tgit merge master\n+\tgit merge master &&\n+\ttest -f sleep.pid\n '\n \n test_done\n-- \n2.10.2\n\n\n-- \nAndreas Schwab, SUSE Labs, schwab@suse.de\nGPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7\n\"And now for something completely different.\"\n"},{"id":"305688","messageId":"xmqqbmxn6t11.fsf@gitster.mtv.corp.google.com","threadId":"44442","inReplyTo":"mvm8tsrbusp.fsf_-_@hawking.suse.de","subject":"Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-10T19:20:42Z","receivedAt":"2016-11-10T19:21:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab writes:\n\n> Commit 5babb5bdb3 (\"t6026-merge-attr: clean up background process at end\n> of test case\") added a kill command to clean up after the test, but this\n> can fail if the sleep command exits before the cleanup is executed.\n> Ignore the error from the kill command.\n>\n> Explicitly check for the existence of the pid file to test that the merge\n> driver was actually called.\n>\n> Signed-off-by: Andreas Schwab \n> ---\n\nOK. sleep.pid is a reasonable easy-to-access side effect we can\nobserve to make sure that the sleep-one-second merge driver was\nindeed invoked, which was missing from the earlier round.\n\nThanks, will apply.\n\n> t/t6026-merge-attr.sh | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\n> index 7a6e33e673..03d13d00b5 100755\n> --- a/t/t6026-merge-attr.sh\n> +++ b/t/t6026-merge-attr.sh\n> @@ -187,13 +187,14 @@ test_expect_success 'custom merge does not lock index' '\n> \t\tsleep 1 &\n> \t\techo $! >sleep.pid\n> \tEOF\n> -\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n> +\ttest_when_finished \"kill \\$(cat sleep.pid) || :\" &&\n> \n> \ttest_write_lines >.gitattributes \\\n> \t\t\"* merge=ours\" \"text merge=sleep-one-second\" &&\n> \ttest_config merge.ours.driver true &&\n> \ttest_config merge.sleep-one-second.driver ./sleep-one-second.sh &&\n> -\tgit merge master\n> +\tgit merge master &&\n> +\ttest -f sleep.pid\n> '\n> \n> test_done\n> -- \n> 2.10.2\n"},{"id":"305733","messageId":"alpine.DEB.2.20.1611102254340.24684@virtualbox","threadId":"44442","inReplyTo":"xmqqbmxn6t11.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-10T21:55:35Z","receivedAt":"2016-11-10T21:55:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi all,\n\nOn Thu, 10 Nov 2016, Junio C Hamano wrote:\n\n> Andreas Schwab writes:\n> \n> > Commit 5babb5bdb3 (\"t6026-merge-attr: clean up background process at end\n> > of test case\") added a kill command to clean up after the test, but this\n> > can fail if the sleep command exits before the cleanup is executed.\n> > Ignore the error from the kill command.\n> >\n> > Explicitly check for the existence of the pid file to test that the merge\n> > driver was actually called.\n> >\n> > Signed-off-by: Andreas Schwab \n> > ---\n> \n> OK. sleep.pid is a reasonable easy-to-access side effect we can\n> observe to make sure that the sleep-one-second merge driver was\n> indeed invoked, which was missing from the earlier round.\n\nNo, this is incorrect. The condition that we need to know applies is that\nthe script is still running, and blocking if the bug reappears.\n\nIt is not enough to test that the script *started* running. If it exits\ntoo early, the problematic condition is never tested, and the test is\nuseless.\n\nCiao,\nJohannes\n"},{"id":"305735","messageId":"xmqq60nv55o3.fsf@gitster.mtv.corp.google.com","threadId":"44442","inReplyTo":"alpine.DEB.2.20.1611102254340.24684@virtualbox","subject":"Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-10T22:30:36Z","receivedAt":"2016-11-10T22:30:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> OK. sleep.pid is a reasonable easy-to-access side effect we can\n>> observe to make sure that the sleep-one-second merge driver was\n>> indeed invoked, which was missing from the earlier round.\n>\n> No, this is incorrect. The condition that we need to know applies is that\n> the script is still running, and blocking if the bug reappears.\n\nOK, I see what you are saying, and I see a few things wrong in here:\n\n * First, the test is titled in a misleading way. In the context of\n a patch that was titled ad65f7e3b7 (\"t6026-merge-attr: child\n processes must not inherit index.lock handles\", 2016-08-18), it\n might have been clear enough to say \"does not lock index\", but\n the sleeping is to make sure that we would notice if the fd to\n the index.lock leaked to the child process by mistake, and the\n way to do so is that the child arranges the leaked fd to be kept\n open after it exits (by spawning \"sleep\"). The test was never\n about \"does not lock index\" (the driver does not take any lock by\n itself in the first place).\n\n * There are three possible outcome from this test:\n\n - 'git merge' fails.\n\n This is expected to happen only on Windows and if the code gets\n broken and starts leaking the fd.\n\n - 'git merge' finishes correctly, the sleep is still running when\n test_when_finished goes to cull it.\n\n In this case, we KNOW there wasn't any fd leak IF we are on\n Windows where a leaked FD would not allow 'git merge' to\n succeed. But on other platforms, fd leak that may cause\n trouble for Windows friends will not be caught.\n\n - 'git merge' finishes correctly, the sleep is no longer\n running because the machine was heavily loaded; a workaround is\n to tolerate failure of culling it.\n\n In this case, we cannot tell anything from the test. Even if\n the fd was leaked, 'git merge' may have succeeded even on\n Windows.\n\nAs everybody knows there is no appropriate timeout value that is\ngood for everybody. I wonder if we can replace the sleep 1 with\nsomething like\n\n\t( while sleep 3600; do :; done ) &\n\nso that leaked fd will be kept even in any heavily loaded\nenvironment instead?\n\n"},{"id":"305736","messageId":"20161110223522.4b35ojaz5nhk4sll@sigill.intra.peff.net","threadId":"44442","inReplyTo":"xmqq60nv55o3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-10T22:35:22Z","receivedAt":"2016-11-10T22:35:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2016 at 02:30:36PM -0800, Junio C Hamano wrote:\n\n> As everybody knows there is no appropriate timeout value that is\n> good for everybody. I wonder if we can replace the sleep 1 with\n> something like\n> \n> \t( while sleep 3600; do :; done ) &\n> \n> so that leaked fd will be kept even in any heavily loaded\n> environment instead?\n\nI think you may have missed:\n\n http://public-inbox.org/git/16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de/\n\nwhich does roughly that. It does not loop, but I suspect 3600 is plenty\nin practice.\n\nI do think the test would be a lot more obvious if it confirmed at the\nend of the test that the process was still running, as opposed to\nrelying on test_when_finished to check it.\n\n-Peff\n"},{"id":"305737","messageId":"xmqq1syj54j9.fsf@gitster.mtv.corp.google.com","threadId":"44442","inReplyTo":"20161110223522.4b35ojaz5nhk4sll@sigill.intra.peff.net","subject":"Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-10T22:55:06Z","receivedAt":"2016-11-10T22:55:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do think the test would be a lot more obvious if it confirmed at the\n> end of the test that the process was still running, as opposed to\n> relying on test_when_finished to check it.\n\nI agree that \"check that the process is still running\" is a wrong\nthing to do in the first place. What would it mean if the process\nis no longer running? It is a timing-dependent bug in the test;\nafter all we failed to produce the condition that could trigger a\nbug that we are guarding against. And \"let's do '|| :'\" is sweeping\nthe bug (not in the code we are testing, but in the test that will\nfail to notice a bug we are preparing against) under the rug. So I\nagree with Dscho that we should do that first.\n\nIf we ensure that the process is still running, then such a check is\na good belt-and-suspenders way to catch a breakage in the mechanism\nwe choose to ensure it. So probably we can require that the kill in\nthe \"when finished\" part to actually send a signal to a process that\nis still running.\n\nIs there an equivalent to pause(2) available to shell scripts? I\nreally hate a single \"sleep 3600\" or anything with a magic number.\n"},{"id":"305738","messageId":"20161110230356.3is53qysegv637hf@sigill.intra.peff.net","threadId":"44442","inReplyTo":"xmqq1syj54j9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-10T23:03:56Z","receivedAt":"2016-11-10T23:04:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2016 at 02:55:06PM -0800, Junio C Hamano wrote:\n\n> If we ensure that the process is still running, then such a check is\n> a good belt-and-suspenders way to catch a breakage in the mechanism\n> we choose to ensure it. So probably we can require that the kill in\n> the \"when finished\" part to actually send a signal to a process that\n> is still running.\n> \n> Is there an equivalent to pause(2) available to shell scripts? I\n> really hate a single \"sleep 3600\" or anything with a magic number.\n\nI think it is usually spelled \"read