{"thread":{"id":"44446","subject":"[PATCH] t6026: ensure that long-running script really is","startedAt":"2016-11-09T13:51:45Z","lastAt":"2016-11-11T21:15:52Z","messageCount":9,"participants":["Johannes Schindelin","Junio C Hamano","Johannes Sixt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"305629","messageId":"16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de","threadId":"44446","inReplyTo":null,"subject":"[PATCH] t6026: ensure that long-running script really is","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-09T13:51:22Z","receivedAt":"2016-11-09T13:51:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When making sure that background tasks are cleaned up in 5babb5b\n(t6026-merge-attr: clean up background process at end of test case,\n2016-09-07), we considered to let the background task sleep longer, just\nto be certain that it will still be running when we want to kill it\nafter the test.\n\nSadly, the assumption appears not to hold true that the test case passes\nquickly enough to kill the background task within a second.\n\nSimply increase it to an hour. No system can be possibly slow enough to\nmake above-mentioned assumption incorrect.\n\nReported by Andreas Schwab.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\nPublished-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1\nFetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1\n\n t/t6026-merge-attr.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 7a6e33e..348d78b 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -183,16 +183,16 @@ test_expect_success 'up-to-date merge without common ancestor' '\n \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+\twrite_script sleep-an-hour.sh <<-\\EOF &&\n+\t\tsleep 3600 &\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+\t\t\"* merge=ours\" \"text merge=sleep-an-hour\" &&\n \ttest_config merge.ours.driver true &&\n-\ttest_config merge.sleep-one-second.driver ./sleep-one-second.sh &&\n+\ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n \tgit merge master\n '\n \n\nbase-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e\n-- \n2.10.1.583.g721a9e0\n"},{"id":"305744","messageId":"xmqqfumy51tk.fsf@gitster.mtv.corp.google.com","threadId":"44446","inReplyTo":"16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] t6026: ensure that long-running script really is","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-10T23:53:43Z","receivedAt":"2016-11-10T23:53:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> When making sure that background tasks are cleaned up in 5babb5b\n> (t6026-merge-attr: clean up background process at end of test case,\n> 2016-09-07), we considered to let the background task sleep longer, just\n> to be certain that it will still be running when we want to kill it\n> after the test.\n>\n> Sadly, the assumption appears not to hold true that the test case passes\n> quickly enough to kill the background task within a second.\n>\n> Simply increase it to an hour. No system can be possibly slow enough to\n> make above-mentioned assumption incorrect.\n>\n> Reported by Andreas Schwab.\n>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1\n\nOK, I think this is a much better option.  Assuming 3600 is long\nenough for everybody (and if not, we have a bigger problem ;-),\nit will ensure that the stray process will be around when we run the\n'git merge' test, and by not adding \"|| :\" after the kill, we check\nthat the stray process is still there, i.e. we tested what we wanted\nto test.\n\nWill revert the two patches that were queued previously and then\nqueue this one.\n\nThanks.\n\n>\n>  t/t6026-merge-attr.sh | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\n> index 7a6e33e..348d78b 100755\n> --- a/t/t6026-merge-attr.sh\n> +++ b/t/t6026-merge-attr.sh\n> @@ -183,16 +183,16 @@ test_expect_success 'up-to-date merge without common ancestor' '\n>  \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> +\twrite_script sleep-an-hour.sh <<-\\EOF &&\n> +\t\tsleep 3600 &\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> +\t\t\"* merge=ours\" \"text merge=sleep-an-hour\" &&\n>  \ttest_config merge.ours.driver true &&\n> -\ttest_config merge.sleep-one-second.driver ./sleep-one-second.sh &&\n> +\ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n>  \tgit merge master\n>  '\n>  \n>\n> base-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e\n"},{"id":"305753","messageId":"fbf517ad-7341-eb6d-ab38-4fe91410e57c@kdbg.org","threadId":"44446","inReplyTo":"xmqqfumy51tk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6026: ensure that long-running script really is","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-11T06:50:14Z","receivedAt":"2016-11-11T06:50:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.11.2016 um 00:53 schrieb Junio C Hamano:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n>> When making sure that background tasks are cleaned up in 5babb5b\n>> (t6026-merge-attr: clean up background process at end of test case,\n>> 2016-09-07), we considered to let the background task sleep longer, just\n>> to be certain that it will still be running when we want to kill it\n>> after the test.\n>>\n>> Sadly, the assumption appears not to hold true that the test case passes\n>> quickly enough to kill the background task within a second.\n>>\n>> Simply increase it to an hour. No system can be possibly slow enough to\n>> make above-mentioned assumption incorrect.\n>>\n>> Reported by Andreas Schwab.\n>>\n>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>> ---\n>> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1\n>> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1\n> \n> OK, I think this is a much better option.  Assuming 3600 is long\n> enough for everybody (and if not, we have a bigger problem ;-),\n> it will ensure that the stray process will be around when we run the\n> 'git merge' test, and by not adding \"|| :\" after the kill, we check\n> that the stray process is still there, i.e. we tested what we wanted\n> to test.\n> \n> Will revert the two patches that were queued previously and then\n> queue this one.\n\nShouldn't we then move the 'kill' out of test_when_finished and make\nit a proper condition of the test? Like this?\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 348d78b205..6ad8bd098a 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' '\n \t\tsleep 3600 &\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-an-hour\" &&\n \ttest_config merge.ours.driver true &&\n \ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n-\tgit merge master\n+\tgit merge master &&\n+\n+\t# We are testing that the custom merge driver does not block\n+\t# index.lock on Windows due to an inherited file handle.\n+\t# To ensure that this test checks this condition, the process\n+\t# must still be running at this point (and must have started\n+\t# in the first place), hence, this kill must not fail:\n+\tkill \"$(cat sleep.pid)\"\n '\n \n test_done\n\n"},{"id":"305758","messageId":"20161111084148.tgtsijn74z2pdylq@sigill.intra.peff.net","threadId":"44446","inReplyTo":"fbf517ad-7341-eb6d-ab38-4fe91410e57c@kdbg.org","subject":"Re: [PATCH] t6026: ensure that long-running script really is","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-11T08:41:48Z","receivedAt":"2016-11-11T08:41:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2016 at 07:50:14AM +0100, Johannes Sixt wrote:\n\n> Shouldn't we then move the 'kill' out of test_when_finished and make\n> it a proper condition of the test? Like this?\n> \n> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\n> index 348d78b205..6ad8bd098a 100755\n> --- a/t/t6026-merge-attr.sh\n> +++ b/t/t6026-merge-attr.sh\n> @@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' '\n>  \t\tsleep 3600 &\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-an-hour\" &&\n>  \ttest_config merge.ours.driver true &&\n>  \ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n> -\tgit merge master\n> +\tgit merge master &&\n> +\n> +\t# We are testing that the custom merge driver does not block\n> +\t# index.lock on Windows due to an inherited file handle.\n> +\t# To ensure that this test checks this condition, the process\n> +\t# must still be running at this point (and must have started\n> +\t# in the first place), hence, this kill must not fail:\n> +\tkill \"$(cat sleep.pid)\"\n>  '\n\nThat makes it more obvious that the return value of \"kill\" is important,\nwhich is good.\n\nBut the other thing the \"kill\" is doing is make sure we clean up after\nourselves, even if another part of the test fails. What happens if\n\"merge\" unexpectedly fails after starting the sleep process?\n\nI think the best compromise is a comment like the one you have here, but\naround the test_when_finished call (and possibly bumping the call down\nto right before \"git merge master\", which is where we expect the process\nto start).\n\n-Peff\n"},{"id":"305813","messageId":"6a421222-a138-5647-4965-8ede24d904b2@kdbg.org","threadId":"44446","inReplyTo":"20161111084148.tgtsijn74z2pdylq@sigill.intra.peff.net","subject":"[PATCH v2] t6026: ensure that long-running script really is","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-11T20:24:44Z","receivedAt":"2016-11-11T20:33:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"When making sure that background tasks are cleaned up in 5babb5b\n(t6026-merge-attr: clean up background process at end of test case,\n2016-09-07), we considered to let the background task sleep longer, just\nto be certain that it will still be running when we want to kill it\nafter the test.\n\nSadly, the assumption appears not to hold true that the test case passes\nquickly enough to kill the background task within a second.\n\nSimply increase it to an hour. No system can be possibly slow enough to\nmake above-mentioned assumption incorrect.\n\nReported by Andreas Schwab.  In-code comments by J6t.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\nAm 11.11.2016 um 09:41 schrieb Jeff King:\n> But the other thing the \"kill\" is doing is make sure we clean up after\n> ourselves, even if another part of the test fails.\n\nGood point. Here is an updated version.\n\nIf the pid file is not created, the kill command receives no\narguments. Here on Linux, it reports failure in this case.\nThis is good. I could have said\n\n   test_when_finished \"kill \\\"\\$(cat sleep.pid)\\\"\"\n\nbut that is a bit too much quoting for my taste when it is\nnot strictly necessary.\n\n t/t6026-merge-attr.sh | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 7a6e33e673..8f9b48a493 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -183,16 +183,24 @@ test_expect_success 'up-to-date merge without common ancestor' '\n \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+\twrite_script sleep-an-hour.sh <<-\\EOF &&\n+\t\tsleep 3600 &\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+\t\t\"* merge=ours\" \"text merge=sleep-an-hour\" &&\n \ttest_config merge.ours.driver true &&\n-\ttest_config merge.sleep-one-second.driver ./sleep-one-second.sh &&\n+\ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n+\n+\t# We are testing that the custom merge driver does not block\n+\t# index.lock on Windows due to an inherited file handle.\n+\t# To ensure that the backgrounded process ran sufficiently\n+\t# long (and has been started in the first place), we do not\n+\t# ignore the result of the kill command.\n+\t# By packaging the command in test_when_finished, we get both\n+\t# the correctness check and the clean-up.\n+\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n \tgit merge master\n '\n \n-- \n2.11.0.rc0.55.gd967357\n\n"},{"id":"305815","messageId":"xmqq60nt3fqc.fsf@gitster.mtv.corp.google.com","threadId":"44446","inReplyTo":"6a421222-a138-5647-4965-8ede24d904b2@kdbg.org","subject":"Re: [PATCH v2] t6026: ensure that long-running script really is","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-11T20:48:27Z","receivedAt":"2016-11-11T20:48:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Good point. Here is an updated version.\n\nUnfortunately, I already took the version before this one and\nstarted my integration cycle today.  I'll wiggle this in; it\nessentially is about adding a big comment to explain what is going\non and then moving the when-finished down below it, right?\n\nThanks.\n\n> If the pid file is not created, the kill command receives no\n> arguments. Here on Linux, it reports failure in this case.\n> This is good. I could have said\n>\n>    test_when_finished \"kill \\\"\\$(cat sleep.pid)\\\"\"\n>\n> but that is a bit too much quoting for my taste when it is\n> not strictly necessary.\n>\n>  t/t6026-merge-attr.sh | 18 +++++++++++++-----\n>  1 file changed, 13 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\n> index 7a6e33e673..8f9b48a493 100755\n> --- a/t/t6026-merge-attr.sh\n> +++ b/t/t6026-merge-attr.sh\n> @@ -183,16 +183,24 @@ test_expect_success 'up-to-date merge without common ancestor' '\n>  \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> +\twrite_script sleep-an-hour.sh <<-\\EOF &&\n> +\t\tsleep 3600 &\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> +\t\t\"* merge=ours\" \"text merge=sleep-an-hour\" &&\n>  \ttest_config merge.ours.driver true &&\n> -\ttest_config merge.sleep-one-second.driver ./sleep-one-second.sh &&\n> +\ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n> +\n> +\t# We are testing that the custom merge driver does not block\n> +\t# index.lock on Windows due to an inherited file handle.\n> +\t# To ensure that the backgrounded process ran sufficiently\n> +\t# long (and has been started in the first place), we do not\n> +\t# ignore the result of the kill command.\n> +\t# By packaging the command in test_when_finished, we get both\n> +\t# the correctness check and the clean-up.\n> +\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n>  \tgit merge master\n>  '\n"},{"id":"305817","messageId":"ba2b3f08-87b1-c954-2020-e5a4bece7f83@kdbg.org","threadId":"44446","inReplyTo":"xmqq60nt3fqc.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] t6026: ensure that long-running script really is","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-11T20:54:57Z","receivedAt":"2016-11-11T20:55:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.11.2016 um 21:48 schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>> Good point. Here is an updated version.\n>\n> Unfortunately, I already took the version before this one and\n> started my integration cycle today.  I'll wiggle this in; it\n> essentially is about adding a big comment to explain what is going\n> on and then moving the when-finished down below it, right?\n\nCorrect.\n\nThank you very much!\n-- Hannes\n\n"},{"id":"305819","messageId":"xmqqmvh5207k.fsf@gitster.mtv.corp.google.com","threadId":"44446","inReplyTo":"ba2b3f08-87b1-c954-2020-e5a4bece7f83@kdbg.org","subject":"Re: [PATCH v2] t6026: ensure that long-running script really is","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-11T21:09:03Z","receivedAt":"2016-11-11T21:09:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Am 11.11.2016 um 21:48 schrieb Junio C Hamano:\n>> Johannes Sixt <j6t@kdbg.org> writes:\n>>\n>>> Good point. Here is an updated version.\n>>\n>> Unfortunately, I already took the version before this one and\n>> started my integration cycle today.  I'll wiggle this in; it\n>> essentially is about adding a big comment to explain what is going\n>> on and then moving the when-finished down below it, right?\n>\n> Correct.\n\nThanks.  It has become like so:\n\n-- >8 --\nFrom: Johannes Sixt <j6t@kdbg.org>\nDate: Fri, 11 Nov 2016 21:24:44 +0100\nSubject: [PATCH] t6026: clarify the point of \"kill $(cat sleep.pid)\"\n\nWe lengthened the time the leftover process sleeps in the previous\ncommit to make sure it will be there while 'git merge' runs and\nfinishes.  It therefore needs to be killed before leaving the test.\nAnd it needs to be killed even when 'git merge' fails, so it has to\nbe triggered via test_when_finished mechanism.\n\nExplain all that in a large comment, and move the use site of\ntest_when_finished to immediately before 'git merge' invocation,\nwhere the process is spawned.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/t6026-merge-attr.sh | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\nindex 348d78b205..8f9b48a493 100755\n--- a/t/t6026-merge-attr.sh\n+++ b/t/t6026-merge-attr.sh\n@@ -187,12 +187,20 @@ test_expect_success 'custom merge does not lock index' '\n \t\tsleep 3600 &\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-an-hour\" &&\n \ttest_config merge.ours.driver true &&\n \ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n+\n+\t# We are testing that the custom merge driver does not block\n+\t# index.lock on Windows due to an inherited file handle.\n+\t# To ensure that the backgrounded process ran sufficiently\n+\t# long (and has been started in the first place), we do not\n+\t# ignore the result of the kill command.\n+\t# By packaging the command in test_when_finished, we get both\n+\t# the correctness check and the clean-up.\n+\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n \tgit merge master\n '\n \n-- \n2.11.0-rc0-149-gea84a1ce98\n\n"},{"id":"305820","messageId":"c4cdd21e-0cef-8255-75f2-90c84f271e02@kdbg.org","threadId":"44446","inReplyTo":"xmqqmvh5207k.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] t6026: ensure that long-running script really is","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-11-11T21:15:32Z","receivedAt":"2016-11-11T21:15:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.11.2016 um 22:09 schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>> Am 11.11.2016 um 21:48 schrieb Junio C Hamano:\n>>> Johannes Sixt <j6t@kdbg.org> writes:\n>>>\n>>>> Good point. Here is an updated version.\n>>>\n>>> Unfortunately, I already took the version before this one and\n>>> started my integration cycle today.  I'll wiggle this in; it\n>>> essentially is about adding a big comment to explain what is going\n>>> on and then moving the when-finished down below it, right?\n>>\n>> Correct.\n>\n> Thanks.  It has become like so:\n>\n> -- >8 --\n> From: Johannes Sixt <j6t@kdbg.org>\n> Date: Fri, 11 Nov 2016 21:24:44 +0100\n> Subject: [PATCH] t6026: clarify the point of \"kill $(cat sleep.pid)\"\n>\n> We lengthened the time the leftover process sleeps in the previous\n> commit to make sure it will be there while 'git merge' runs and\n> finishes.  It therefore needs to be killed before leaving the test.\n> And it needs to be killed even when 'git merge' fails, so it has to\n> be triggered via test_when_finished mechanism.\n>\n> Explain all that in a large comment, and move the use site of\n> test_when_finished to immediately before 'git merge' invocation,\n> where the process is spawned.\n>\n> Signed-off-by: Johannes Sixt <j6t@kdbg.org>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nPerfect, thank you very much.\n\n> ---\n>  t/t6026-merge-attr.sh | 10 +++++++++-\n>  1 file changed, 9 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh\n> index 348d78b205..8f9b48a493 100755\n> --- a/t/t6026-merge-attr.sh\n> +++ b/t/t6026-merge-attr.sh\n> @@ -187,12 +187,20 @@ test_expect_success 'custom merge does not lock index' '\n>  \t\tsleep 3600 &\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-an-hour\" &&\n>  \ttest_config merge.ours.driver true &&\n>  \ttest_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&\n> +\n> +\t# We are testing that the custom merge driver does not block\n> +\t# index.lock on Windows due to an inherited file handle.\n> +\t# To ensure that the backgrounded process ran sufficiently\n> +\t# long (and has been started in the first place), we do not\n> +\t# ignore the result of the kill command.\n> +\t# By packaging the command in test_when_finished, we get both\n> +\t# the correctness check and the clean-up.\n> +\ttest_when_finished \"kill \\$(cat sleep.pid)\" &&\n>  \tgit merge master\n>  '\n>\n>\n\n"}]}