{"thread":{"id":"44010","subject":"[PATCH] t6026-merge-attr: wait for process to release trash directory","startedAt":"2016-09-05T19:03:55Z","lastAt":"2016-09-09T09:58:37Z","messageCount":11,"participants":["Johannes Sixt","Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"301109","messageId":"3fd38b71-26bd-bdb3-fe5c-e7038abf51e0@kdbg.org","threadId":"44010","inReplyTo":null,"subject":"[PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-09-05T19:03:48Z","receivedAt":"2016-09-05T19:03:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The process spawned in the hook uses the test's trash directory as CWD.\nAs long as it is alive, the directory cannot be removed on Windows.\nAlthough the test succeeds, the 'test_done' that follows produces an\nerror message and leaves the trash directory around. Insert a delay to\ngive the hook time to go away.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n t/t6026-merge-attr.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex dd8f88d..2c5b138 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -191,7 +191,10 @@ test_expect_success 'custom merge does not lock index' '\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+\t# the trash directory cannot be removed on Windows until the hook\n+\t# does not occupy it for its CWD anymore; wait for it to quit:\n+\tsleep 1\n '\n \n test_done\n-- \n2.10.0.85.gea34e30\n\n"},{"id":"301173","messageId":"alpine.DEB.2.20.1609060909420.129229@virtualbox","threadId":"44010","inReplyTo":"3fd38b71-26bd-bdb3-fe5c-e7038abf51e0@kdbg.org","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T07:25:05Z","receivedAt":"2016-09-06T07:25:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 5 Sep 2016, Johannes Sixt wrote:\n\n> The process spawned in the hook uses the test's trash directory as CWD.\n> As long as it is alive, the directory cannot be removed on Windows.\n> Although the test succeeds, the 'test_done' that follows produces an\n> error message and leaves the trash directory around. Insert a delay to\n> give the hook time to go away.\n\nMaybe we should write a pid file in the sleep command instead, and kill it\nin the end? Something like this, maybe?\n\n-- snipsnap --\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex dd8f88d..2e2beb5 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -184,8 +184,10 @@ test_expect_success 'up-to-date merge without common\nancestor' '\n test_expect_success 'custom merge does not lock index' '\n \tgit reset --hard anchor &&\n \twrite_script sleep-one-second.sh <<-\\EOF &&\n-\t\tsleep 1 &\n+\t\tsleep 10 &\n+\t\techo $! >sleep.pid\n \tEOF\n+\ttest_when_finished \"kill -9 \\$(cat sleep.pid)\" &&\n \n \ttest_write_lines >.gitattributes \\\n \t\t\"* merge=ours\" \"text merge=sleep-one-second\" &&\n\n"},{"id":"301174","messageId":"20160906072707.goysliw32xgwmlol@sigill.intra.peff.net","threadId":"44010","inReplyTo":"3fd38b71-26bd-bdb3-fe5c-e7038abf51e0@kdbg.org","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-06T07:27:07Z","receivedAt":"2016-09-06T07:27:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 05, 2016 at 09:03:48PM +0200, Johannes Sixt wrote:\n\n> The process spawned in the hook uses the test's trash directory as CWD.\n> As long as it is alive, the directory cannot be removed on Windows.\n> Although the test succeeds, the 'test_done' that follows produces an\n> error message and leaves the trash directory around. Insert a delay to\n> give the hook time to go away.\n\nUgh. I'd love it if we could avoid inserting a sleep, which wastes time\nin the optimistic case and is insufficient in the pessimistic one.\n\nThe fact that the hook is already using sleep is even nastier, as it\nthat's a potential race on a loaded system.\n\nCan we do some signaling with fifos to tell the hook when it is safe to\nexit? Then we would just need to `wait` for its parent process.\n\n-Peff\n"},{"id":"301239","messageId":"1953cc16-7577-16dd-fe7e-f557e7a74f58@kdbg.org","threadId":"44010","inReplyTo":"alpine.DEB.2.20.1609060909420.129229@virtualbox","subject":"[PATCH v2] t6026-merge-attr: clean up background process at end of test case","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-09-07T06:10:41Z","receivedAt":"2016-09-07T06:10:51Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"The process spawned in the hook uses the test's trash directory as CWD.\nAs long as it is alive, the directory cannot be removed on Windows.\nAlthough the test succeeds, the 'test_done' that follows produces an\nerror message and leaves the trash directory around. Kill the process\nbefore the test case advances.\n\nHelped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nAm 06.09.2016 um 09:25 schrieb Johannes Schindelin:\n> Maybe we should write a pid file in the sleep command instead, and kill it\n> in the end? Something like this, maybe?\n\nYes, that is much better, thank you!\n\nI did not extend the sleep time because it requires to change the file name\nin the same patch.\n\nI did experiment with 'while sleep 1; do :; done &' to spin indefinitely,\nbut the loop did not terminate in time in my tests on Windows for some\nunknown reason (most likely because the killed process does not exit with\na non-zero code -- remember, Windows is not POSIX, particularly, when it\ncomes to signal handling).\n\nt/t6026-merge-attr.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex dd8f88d..7a6e33e 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -185,7 +185,9 @@ test_expect_success 'custom merge does not lock index' '\n \tgit reset --hard anchor &&\n \twrite_script sleep-one-second.sh <<-\\EOF &&\n \t\tsleep 1 &\n+\t\techo $! >sleep.pid\n \tEOF\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.0.85.gea34e30\n\n"},{"id":"301257","messageId":"alpine.DEB.2.20.1609071334340.129229@virtualbox","threadId":"44010","inReplyTo":"1953cc16-7577-16dd-fe7e-f557e7a74f58@kdbg.org","subject":"Re: [PATCH v2] t6026-merge-attr: clean up background process at end of test case","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-07T11:35:09Z","receivedAt":"2016-09-07T11:35:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Wed, 7 Sep 2016, Johannes Sixt wrote:\n\n> I did not extend the sleep time because it requires to change the file name\n> in the same patch.\n\nYeah, I was just concerned that maybe we would take longer than that\nsecond to finish the test. But I guess I am just too paranoid here.\n\nThanks for the patch!\nDscho\n"},{"id":"301310","messageId":"xmqq4m5rmu4i.fsf@gitster.mtv.corp.google.com","threadId":"44010","inReplyTo":"20160906072707.goysliw32xgwmlol@sigill.intra.peff.net","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T18:39:57Z","receivedAt":"2016-09-07T18:40:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Sep 05, 2016 at 09:03:48PM +0200, Johannes Sixt wrote:\n>\n>> The process spawned in the hook uses the test's trash directory as CWD.\n>> As long as it is alive, the directory cannot be removed on Windows.\n>> Although the test succeeds, the 'test_done' that follows produces an\n>> error message and leaves the trash directory around. Insert a delay to\n>> give the hook time to go away.\n>\n> Ugh. I'd love it if we could avoid inserting a sleep, which wastes time\n> in the optimistic case and is insufficient in the pessimistic one.\n>\n> The fact that the hook is already using sleep is even nastier, as it\n> that's a potential race on a loaded system.\n>\n> Can we do some signaling with fifos to tell the hook when it is safe to\n> exit? Then we would just need to `wait` for its parent process.\n\nIs fifo safe on Windows, though?\n\nWith v2 that explicitly kills, I guess we can make the sleep longer\nwithout slowing down in the optimistic case?\n"},{"id":"301316","messageId":"20160907190004.dw3p6fxkdaubwuvu@sigill.intra.peff.net","threadId":"44010","inReplyTo":"xmqq4m5rmu4i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-07T19:00:04Z","receivedAt":"2016-09-07T19:00:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:\n\n> > Can we do some signaling with fifos to tell the hook when it is safe to\n> > exit? Then we would just need to `wait` for its parent process.\n> \n> Is fifo safe on Windows, though?\n\nNo clue. We seem to use mkfifo unconditionally in lib-daemon, but\nperhaps people do not run that test on Windows. Other invocations seem\nto be protected by the PIPE prerequisite. But...\n\n> With v2 that explicitly kills, I guess we can make the sleep longer\n> without slowing down in the optimistic case?\n\nYeah, I think the v2 one is non-racy (I thought at first we might race\nwith the \"echo\", but it should be synchronous; the hook will not exit\nuntil we have written the pid file, and git will not exit until the hook\nis done running).\n\nI agree that the sleep could be made longer, to make the test less racy.\nHowever, the racy failure mode is that it might pass while testing\nnothing (i.e., the sleep ends anyway before the hook returns), so I\ndon't think it's a high priority.\n\n-Peff\n"},{"id":"301320","messageId":"xmqqa8fjlbwb.fsf@gitster.mtv.corp.google.com","threadId":"44010","inReplyTo":"20160907190004.dw3p6fxkdaubwuvu@sigill.intra.peff.net","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T19:59:00Z","receivedAt":"2016-09-07T19:59:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I agree that the sleep could be made longer, to make the test less racy.\n> However, the racy failure mode is that it might pass while testing\n> nothing (i.e., the sleep ends anyway before the hook returns), so I\n> don't think it's a high priority.\n\nI do not think it is necessary, either.  It was just me wondering\naloud, nothing more.\n"},{"id":"301390","messageId":"alpine.DEB.2.20.1609081002490.129229@virtualbox","threadId":"44010","inReplyTo":"20160907190004.dw3p6fxkdaubwuvu@sigill.intra.peff.net","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-08T08:05:58Z","receivedAt":"2016-09-08T08:06:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Junio,\n\nOn Wed, 7 Sep 2016, Jeff King wrote:\n\n> On Wed, Sep 07, 2016 at 11:39:57AM -0700, Junio C Hamano wrote:\n> \n> > > Can we do some signaling with fifos to tell the hook when it is safe to\n> > > exit? Then we would just need to `wait` for its parent process.\n> > \n> > Is fifo safe on Windows, though?\n> \n> No clue. We seem to use mkfifo unconditionally in lib-daemon, but\n> perhaps people do not run that test on Windows. Other invocations seem\n> to be protected by the PIPE prerequisite. But...\n\nAFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has\nto say about the matter:\n\n\ttest_lazy_prereq PIPE '\n\t\t# test whether the filesystem supports FIFOs\n\t\tcase $(uname -s) in\n\t\tCYGWIN*|MINGW*)\n\t\t\tfalse\n\t\t\t;;\n\t\t*)\n\t\t\trm -f testfifo && mkfifo testfifo\n\t\t\t;;\n\t\tesac\n\t'\n\nSo there you go.\n\nThe reason it is disabled is that Cygwin/MSYS2 do have a concept of a\nFIFO. But `git.exe` won't be able to access such a FIFO because it is\nemulated by the POSIX emulation layer, which Git cannot access.\n\n> > With v2 that explicitly kills, I guess we can make the sleep longer\n> > without slowing down in the optimistic case?\n> \n> Yeah, I think the v2 one is non-racy (I thought at first we might race\n> with the \"echo\", but it should be synchronous; the hook will not exit\n> until we have written the pid file, and git will not exit until the hook\n> is done running).\n\nPlease note that Hannes and I discussed this (as I originally suggested to\nincrease it to 10 seconds, and Hannes rightfully pointed out that we would\nhave to change the script name, too, as it says sleep-one-second.sh, and\nthat would have made the patch less readable) and we came to the same\nconclusion: it's not necessary.\n\nCiao,\nDscho\n"},{"id":"301395","messageId":"20160908082713.sgzp2evbvzuthdb4@sigill.intra.peff.net","threadId":"44010","inReplyTo":"alpine.DEB.2.20.1609081002490.129229@virtualbox","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-08T08:27:13Z","receivedAt":"2016-09-08T08:27:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 08, 2016 at 10:05:58AM +0200, Johannes Schindelin wrote:\n\n> > > Is fifo safe on Windows, though?\n> > \n> > No clue. We seem to use mkfifo unconditionally in lib-daemon, but\n> > perhaps people do not run that test on Windows. Other invocations seem\n> > to be protected by the PIPE prerequisite. But...\n> \n> AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has\n> to say about the matter:\n> \n> \ttest_lazy_prereq PIPE '\n> \t\t# test whether the filesystem supports FIFOs\n> \t\tcase $(uname -s) in\n> \t\tCYGWIN*|MINGW*)\n> \t\t\tfalse\n> \t\t\t;;\n> \t\t*)\n> \t\t\trm -f testfifo && mkfifo testfifo\n> \t\t\t;;\n> \t\tesac\n> \t'\n> \n> So there you go.\n> \n> The reason it is disabled is that Cygwin/MSYS2 do have a concept of a\n> FIFO. But `git.exe` won't be able to access such a FIFO because it is\n> emulated by the POSIX emulation layer, which Git cannot access.\n\nRegarding my \"unconditionally\" above: coincidentally, I happened to be\nlooking in lib-git-daemon.sh about an hour ago and noticed that we do\nindeed check \"test_have_prereq PIPE\" (just not near the mkfifo, of\ncourse, because we are not in a test block).\n\nIt seems to have been added by a \"Johannes Schindelin\". Any relation?\n\nSo yeah. It definitely would be a bad idea to use a fifo in a test that\nis already Windows-specific.\n\n-Peff\n"},{"id":"301468","messageId":"alpine.DEB.2.20.1609091158060.129229@virtualbox","threadId":"44010","inReplyTo":"20160908082713.sgzp2evbvzuthdb4@sigill.intra.peff.net","subject":"Re: [PATCH] t6026-merge-attr: wait for process to release trash directory","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-09T09:58:21Z","receivedAt":"2016-09-09T09:58:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 8 Sep 2016, Jeff King wrote:\n\n> On Thu, Sep 08, 2016 at 10:05:58AM +0200, Johannes Schindelin wrote:\n> \n> > > > Is fifo safe on Windows, though?\n> > > \n> > > No clue. We seem to use mkfifo unconditionally in lib-daemon, but\n> > > perhaps people do not run that test on Windows. Other invocations seem\n> > > to be protected by the PIPE prerequisite. But...\n> > \n> > AFAICT we do not use mkfifo on Windows. Let's see what t/test-lib.sh has\n> > to say about the matter:\n> > \n> > \ttest_lazy_prereq PIPE '\n> > \t\t# test whether the filesystem supports FIFOs\n> > \t\tcase $(uname -s) in\n> > \t\tCYGWIN*|MINGW*)\n> > \t\t\tfalse\n> > \t\t\t;;\n> > \t\t*)\n> > \t\t\trm -f testfifo && mkfifo testfifo\n> > \t\t\t;;\n> > \t\tesac\n> > \t'\n> > \n> > So there you go.\n> > \n> > The reason it is disabled is that Cygwin/MSYS2 do have a concept of a\n> > FIFO. But `git.exe` won't be able to access such a FIFO because it is\n> > emulated by the POSIX emulation layer, which Git cannot access.\n> \n> Regarding my \"unconditionally\" above: coincidentally, I happened to be\n> looking in lib-git-daemon.sh about an hour ago and noticed that we do\n> indeed check \"test_have_prereq PIPE\" (just not near the mkfifo, of\n> course, because we are not in a test block).\n> \n> It seems to have been added by a \"Johannes Schindelin\". Any relation?\n\nMaybe ;-)\nDscho\n"}]}