{"thread":{"id":"45649","subject":"[PATCH] t6500: don't run detached auto gc at the end of the test script","startedAt":"2017-04-10T12:59:26Z","lastAt":"2017-04-25T06:31:56Z","messageCount":41,"participants":["SZEDER Gábor","Jeff King","Johannes Sixt","David Turner","Stefan Beller","Junio C Hamano","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316486","messageId":"20170410125911.6800-1-szeder.dev@gmail.com","threadId":"45649","inReplyTo":null,"subject":"[PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-10T12:59:11Z","receivedAt":"2017-04-10T12:59:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The last test in 't6500-gc', 'background auto gc does not run if\ngc.log is present and recent but does if it is old', added in\na831c06a2 (gc: ignore old gc.log files, 2017-02-10), may sporadically\ntrigger an error message from the test harness:\n\n rm: cannot remove 'trash directory.t6500-gc/.git/objects': Directory not empty\n\nThe test in question ends with executing an auto gc in the backround,\nwhich occasionally takes so long that it's still running when\n'test_done' is about to remove the trash directory. This 'rm -rf\n$trash' in the foreground might race with the detached auto gc to\ncreate and delete files and directories, and gc might (re-)create a\npath that 'rm' already visited and removed, triggering the above error\nwhen 'rm' attempts to remove its parent directory.\n\nCommit bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01)\nfixed the same problem in a different test script by simply\ndisallowing background gc. Unfortunately, what worked there is not\napplicable here, because the purpose of this test is to check the\nbehavior of a detached auto gc.\n\nMove the detached auto gc execution further away from the end of the\ntest script by splitting the test in two: run the 'background auto gc\nruns if an old gc.log is present' part first, leaving only the\naborting auto gc executions in the last test. Run the first test's\ndetached auto gc in a dedicated sub-repository to prevent it from\ninterfering with the subseqent test's gc. Also add a comment at the\nend of the test script to warn developers of future tests about this\nissue.\n\nWhile this change doesn't completely eliminate the possibility of\nthis race, it significantly and seemingly sufficiently reduces its\nprobability. Running t6500 in a loop while my box was under heavy CPU\nand I/O load usually triggered the error under 15 seconds, but with\nthis patch it run overnight without incident.\n\n(Alternatively, we could check the presence of the gc.pid file after\nstarting the detached auto gc, and wait while it's there. However,\nthis would create a different race: the auto gc process creates the\npid file after it goes to the background, so our check in the\nforeground might racily look for the pid file before it's even\ncreated, and thus would not wait for the background gc to finish.\nFurthermore, this would open up the possibility of the test hanging if\nsomething were to go wrong with the gc process and the pid file were\nnot removed.)\n\nSigned-off-by: SZEDER Gábor \n---\n t/t6500-gc.sh | 32 +++++++++++++++++++++++++++-----\n 1 file changed, 27 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 08de2e8ab..2dc202c9b 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -67,7 +67,27 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre\n \ttest_line_count = 2 new # There is one new pack and its .idx\n '\n \n-test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n+test_expect_success 'background auto gc runs if an old gc.log is present' '\n+\t# The detached auto gc process can run long enough in the\n+\t# background to racily interfere with the gc execution in the\n+\t# subsequent test, hence the dedicated repository.\n+\ttest_create_repo other &&\n+\t(\n+\t\tcd other &&\n+\t\ttest_commit foo &&\n+\t\tgit repack &&\n+\t\ttest_commit bar &&\n+\t\tgit repack &&\n+\t\tgit config gc.autopacklimit 1 &&\n+\t\tgit config gc.autodetach true &&\n+\t\tgit config gc.logexpiry 2.days &&\n+\t\techo fleem >.git/gc.log &&\n+\t\ttest-chmtime =-345600 .git/gc.log &&\t# 4 days\n+\t\tgit gc --auto\n+\t)\n+'\n+\n+test_expect_success 'background auto gc does not run if gc.log is present' '\n \ttest_commit foo &&\n \ttest_commit bar &&\n \tgit repack &&\n@@ -77,10 +97,12 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest_must_fail git gc --auto 2>err &&\n \ttest_i18ngrep \"^error:\" err &&\n \ttest_config gc.logexpiry 5.days &&\n-\ttest-chmtime =-345600 .git/gc.log &&\n-\ttest_must_fail git gc --auto &&\n-\ttest_config gc.logexpiry 2.days &&\n-\tgit gc --auto\n+\ttest-chmtime =-345600 .git/gc.log &&\t# 4 days\n+\ttest_must_fail git gc --auto\n '\n \n+# DO NOT leave a detached auto gc process running near the end of the\n+# test script: it can run long enough in the background to racily\n+# interfere with the cleanup in 'test_done'.\n+\n test_done\n-- \n2.12.2.613.g9c5b79913\n\n"},{"id":"316490","messageId":"20170410135837.2ukgksfxdlcfqldy@sigill.intra.peff.net","threadId":"45649","inReplyTo":"20170410125911.6800-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T13:58:37Z","receivedAt":"2017-04-10T13:58:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2017 at 02:59:11PM +0200, SZEDER Gábor wrote:\n\n> While this change doesn't completely eliminate the possibility of\n> this race, it significantly and seemingly sufficiently reduces its\n> probability. Running t6500 in a loop while my box was under heavy CPU\n> and I/O load usually triggered the error under 15 seconds, but with\n> this patch it run overnight without incident.\n> \n> (Alternatively, we could check the presence of the gc.pid file after\n> starting the detached auto gc, and wait while it's there. However,\n> this would create a different race: the auto gc process creates the\n> pid file after it goes to the background, so our check in the\n> foreground might racily look for the pid file before it's even\n> created, and thus would not wait for the background gc to finish.\n> Furthermore, this would open up the possibility of the test hanging if\n> something were to go wrong with the gc process and the pid file were\n> not removed.)\n\nCould we just leave open a pipe descriptor that the child inherits, and\nwait for it to close?\n\nSomething like:\n\n git gc --auto 9>&1 | read\n\nshould wait until the background gc process finishes. It depends on our\ndaemonize() not closing descriptors beyond 0/1/2, but that is certainly\nthe case now.\n\nIt also loses the exit status of the main \"git gc\", but that can be\nfixed with shell hackery:\n\n code=$(sh -c 'git gc --auto; echo $?' 9>&1)\n test \"$code\" = 0\n\n-Peff\n"},{"id":"316496","messageId":"CAM0VKj=4Utapk9iFasChkPSdkWxB5WiHtpZGPUYKMC5LKrnGXw@mail.gmail.com","threadId":"45649","inReplyTo":"20170410135837.2ukgksfxdlcfqldy@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-10T16:31:54Z","receivedAt":"2017-04-10T16:32:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 10, 2017 at 3:58 PM, Jeff King wrote:\n> On Mon, Apr 10, 2017 at 02:59:11PM +0200, SZEDER Gábor wrote:\n>\n>> While this change doesn't completely eliminate the possibility of\n>> this race, it significantly and seemingly sufficiently reduces its\n>> probability. Running t6500 in a loop while my box was under heavy CPU\n>> and I/O load usually triggered the error under 15 seconds, but with\n>> this patch it run overnight without incident.\n>>\n>> (Alternatively, we could check the presence of the gc.pid file after\n>> starting the detached auto gc, and wait while it's there. However,\n>> this would create a different race: the auto gc process creates the\n>> pid file after it goes to the background, so our check in the\n>> foreground might racily look for the pid file before it's even\n>> created, and thus would not wait for the background gc to finish.\n>> Furthermore, this would open up the possibility of the test hanging if\n>> something were to go wrong with the gc process and the pid file were\n>> not removed.)\n>\n> Could we just leave open a pipe descriptor that the child inherits, and\n> wait for it to close?\n>\n> Something like:\n>\n> git gc --auto 9>&1 | read\n>\n> should wait until the background gc process finishes. It depends on our\n> daemonize() not closing descriptors beyond 0/1/2, but that is certainly\n> the case now.\n>\n> It also loses the exit status of the main \"git gc\", but that can be\n> fixed with shell hackery:\n>\n> code=$(sh -c 'git gc --auto; echo $?' 9>&1)\n> test \"$code\" = 0\n\nIndeed this seems to work, and luckily we don't need that much\nhackery. When there is a single variable assignment and the expansion\nof a command substitution is assigned to the variable, then the exit\nstatus is that of the command inside the command substitution, i.e.\n\n $ v=$(false) ; echo $?\n 1\n\nThis means we can write this simply as:\n\n doesnt_matter=$(git gc --auto 9>&1)\n\nIt's still hackery :)\n\nOTOH, this makes it possible to continue the test reliably after the\ngc finished in the background, so we could also check that there is\nonly a single pack file left, i.e. that the detached gc did what it\nwas supposed to do.\n"},{"id":"316498","messageId":"20170410163557.gn3mlcalfhhncbtt@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKj=4Utapk9iFasChkPSdkWxB5WiHtpZGPUYKMC5LKrnGXw@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T16:35:58Z","receivedAt":"2017-04-10T16:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2017 at 06:31:54PM +0200, SZEDER Gábor wrote:\n\n> Indeed this seems to work, and luckily we don't need that much\n> hackery. When there is a single variable assignment and the expansion\n> of a command substitution is assigned to the variable, then the exit\n> status is that of the command inside the command substitution, i.e.\n> \n> $ v=$(false) ; echo $?\n> 1\n> \n> This means we can write this simply as:\n> \n> doesnt_matter=$(git gc --auto 9>&1)\n> \n> It's still hackery :)\n\nHeh. Yeah, I would call that _more_ hackery in that it's much more\nclever. But it is shorter. :)\n\nI think as long as the trickery is documented that's OK (and calling it\ndoesnt_matter and explaining in the commit message is fine by me;\nhopefully that name would induce somebody to look in the history).\n\n> OTOH, this makes it possible to continue the test reliably after the\n> gc finished in the background, so we could also check that there is\n> only a single pack file left, i.e. that the detached gc did what it\n> was supposed to do.\n\nYes, I think the test can and should check the after-gc state.\n\n-Peff\n"},{"id":"316499","messageId":"CAM0VKjkdqjbkqOF6ucymtuBAAgBVJQ6SGe4Ep2gqVBtNp=s_CA@mail.gmail.com","threadId":"45649","inReplyTo":"20170410163557.gn3mlcalfhhncbtt@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-10T16:56:30Z","receivedAt":"2017-04-10T16:56:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 10, 2017 at 6:35 PM, Jeff King wrote:\n> On Mon, Apr 10, 2017 at 06:31:54PM +0200, SZEDER Gábor wrote:\n\n>> This means we can write this simply as:\n>>\n>> doesnt_matter=$(git gc --auto 9>&1)\n>>\n>> It's still hackery :)\n>\n> Heh. Yeah, I would call that _more_ hackery in that it's much more\n> clever. But it is shorter. :)\n>\n> I think as long as the trickery is documented that's OK (and calling it\n> doesnt_matter and explaining in the commit message is fine by me;\n> hopefully that name would induce somebody to look in the history).\n\nFor the sake of self documentation and potential future users, I will\nput it into a helper function like run_and_wait_for_detached_auto_gc()\nor something. Jury is still out on the proper function name (it would\nbe a shame if the funcname were longer than the command ;), but time\nis up for today...\n"},{"id":"316501","messageId":"20170410170154.qwzaolflrvsduwzd@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjkdqjbkqOF6ucymtuBAAgBVJQ6SGe4Ep2gqVBtNp=s_CA@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T17:01:54Z","receivedAt":"2017-04-10T17:02:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 10, 2017 at 06:56:30PM +0200, SZEDER Gábor wrote:\n\n> On Mon, Apr 10, 2017 at 6:35 PM, Jeff King wrote:\n> > On Mon, Apr 10, 2017 at 06:31:54PM +0200, SZEDER Gábor wrote:\n> \n> >> This means we can write this simply as:\n> >>\n> >> doesnt_matter=$(git gc --auto 9>&1)\n> >>\n> >> It's still hackery :)\n> >\n> > Heh. Yeah, I would call that _more_ hackery in that it's much more\n> > clever. But it is shorter. :)\n> >\n> > I think as long as the trickery is documented that's OK (and calling it\n> > doesnt_matter and explaining in the commit message is fine by me;\n> > hopefully that name would induce somebody to look in the history).\n> \n> For the sake of self documentation and potential future users, I will\n> put it into a helper function like run_and_wait_for_detached_auto_gc()\n> or something. Jury is still out on the proper function name (it would\n> be a shame if the funcname were longer than the command ;), but time\n> is up for today...\n\nI wonder if you could make it a general test-lib function, like:\n\n run_and_wait () {\n\t# we read stdout from the child only for the side effect\n\t# of waiting until all child sub-processes exit, closing their\n\t# fd 9.\n\tdoes_not_matter=$(\"$@\" 9>&1)\n }\n\nYou could make it even more general by doing an 'eval' in the middle,\nbut that is probably getting too insane. :)\n\nI don't know if there are other spots that would benefit from this. Most\nof the other racy stuff like this that I recall working on actually\nwanted to run two co-processes (so background something, then keep\nrunning the test, then ask the backgrounded task to die and wait for it\nto finish).\n\n-Peff\n"},{"id":"316649","messageId":"426f08b9-79c8-0c5b-e07e-4dd6a49243e9@kdbg.org","threadId":"45649","inReplyTo":"20170410170154.qwzaolflrvsduwzd@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-04-11T21:32:31Z","receivedAt":"2017-04-11T21:32:41Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.04.2017 um 19:01 schrieb Jeff King:\n> I wonder if you could make it a general test-lib function, like:\n>\n> run_and_wait () {\n> \t# we read stdout from the child only for the side effect\n> \t# of waiting until all child sub-processes exit, closing their\n> \t# fd 9.\n> \tdoes_not_matter=$(\"$@\" 9>&1)\n\nI'm afraid this won't work on Windows when the invoked command is git. \nFD inheritance between MSYS (bash) and non-MSYS programs (git) is only \nimplemented for FDs 0,1,2. That's a deficiency of MSYS, and I don't \nthink that was improved in MSYS2.\n\n> }\n\n-- Hannes\n\n"},{"id":"316660","messageId":"CAM0VKjmuCjLBFAErXP06wu0+-7SdWVMaVu_g25Q4V-uQz8T1uA@mail.gmail.com","threadId":"45649","inReplyTo":"426f08b9-79c8-0c5b-e07e-4dd6a49243e9@kdbg.org","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-12T00:27:05Z","receivedAt":"2017-04-12T00:27:11Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Apr 11, 2017 at 11:32 PM, Johannes Sixt wrote:\n> Am 10.04.2017 um 19:01 schrieb Jeff King:\n>>\n>> I wonder if you could make it a general test-lib function, like:\n>>\n>> run_and_wait () {\n>> # we read stdout from the child only for the side effect\n>> # of waiting until all child sub-processes exit, closing their\n>> # fd 9.\n>> does_not_matter=$(\"$@\" 9>&1)\n>\n>\n> I'm afraid this won't work on Windows when the invoked command is git. FD\n> inheritance between MSYS (bash) and non-MSYS programs (git) is only\n> implemented for FDs 0,1,2. That's a deficiency of MSYS, and I don't think\n> that was improved in MSYS2.\n\nOh, that's a pity, I was almost ready with v2...\n\nUnfortunately, this makes the general helper function unworkable, of\ncourse. Though in this particular case it wouldn't matter, because on\nWindows daemonize() is basically a noop and 'git gc --auto' remains in\nthe foreground anyway.\n\nI think we should stick with my initial patch, then.\n"},{"id":"316664","messageId":"20170412005011.46tr4mgsxk7chgfz@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjmuCjLBFAErXP06wu0+-7SdWVMaVu_g25Q4V-uQz8T1uA@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-12T00:50:11Z","receivedAt":"2017-04-12T00:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 12, 2017 at 02:27:05AM +0200, SZEDER Gábor wrote:\n\n> >> I wonder if you could make it a general test-lib function, like:\n> >>\n> >> run_and_wait () {\n> >> # we read stdout from the child only for the side effect\n> >> # of waiting until all child sub-processes exit, closing their\n> >> # fd 9.\n> >> does_not_matter=$(\"$@\" 9>&1)\n> >\n> >\n> > I'm afraid this won't work on Windows when the invoked command is git. FD\n> > inheritance between MSYS (bash) and non-MSYS programs (git) is only\n> > implemented for FDs 0,1,2. That's a deficiency of MSYS, and I don't think\n> > that was improved in MSYS2.\n> \n> Oh, that's a pity, I was almost ready with v2...\n> \n> Unfortunately, this makes the general helper function unworkable, of\n> course. Though in this particular case it wouldn't matter, because on\n> Windows daemonize() is basically a noop and 'git gc --auto' remains in\n> the foreground anyway.\n\nThat makes it tempting to use in this scenario as a one-off with a\ncomment.\n\n> I think we should stick with my initial patch, then.\n\nI'm not entirely opposed, but my understanding was that it didn't\nactually fix the race, it just made it a bit bigger. Which is sort of\nunsatisfying.\n\nI couldn't get the original to show a failure, though, even under heavy\nload. So maybe widening the race is enough.\n\n-Peff\n"},{"id":"316704","messageId":"CAM0VKjkuoCotm8FB5qdTQsC0SaJiwBA9dXmdLJdSs8NwLdY8fQ@mail.gmail.com","threadId":"45649","inReplyTo":"20170412005011.46tr4mgsxk7chgfz@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-12T22:03:20Z","receivedAt":"2017-04-12T22:03:26Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Apr 12, 2017 at 2:50 AM, Jeff King wrote:\n> On Wed, Apr 12, 2017 at 02:27:05AM +0200, SZEDER Gábor wrote:\n>\n>> >> I wonder if you could make it a general test-lib function, like:\n>> >>\n>> >> run_and_wait () {\n>> >> # we read stdout from the child only for the side effect\n>> >> # of waiting until all child sub-processes exit, closing their\n>> >> # fd 9.\n>> >> does_not_matter=$(\"$@\" 9>&1)\n>> >\n>> >\n>> > I'm afraid this won't work on Windows when the invoked command is git. FD\n>> > inheritance between MSYS (bash) and non-MSYS programs (git) is only\n>> > implemented for FDs 0,1,2. That's a deficiency of MSYS, and I don't think\n>> > that was improved in MSYS2.\n>>\n>> Oh, that's a pity, I was almost ready with v2...\n>>\n>> Unfortunately, this makes the general helper function unworkable, of\n>> course. Though in this particular case it wouldn't matter, because on\n>> Windows daemonize() is basically a noop and 'git gc --auto' remains in\n>> the foreground anyway.\n>\n> That makes it tempting to use in this scenario as a one-off with a\n> comment.\n\nOk, I'll send it out in a minute, and Junio can then take his pick.\n\n>> I think we should stick with my initial patch, then.\n>\n> I'm not entirely opposed, but my understanding was that it didn't\n> actually fix the race, it just made it a bit bigger. Which is sort of\n> unsatisfying.\n>\n> I couldn't get the original to show a failure, though, even under heavy\n> load. So maybe widening the race is enough.\n\nJust to be clear: it's only an occasionally appearing error message.\nThere is no failure in the sense of test failure, because 'rm -rf\n$trash' erroring out during housekeeping does not fail the test suite.\n"},{"id":"316706","messageId":"20170412220717.7357-1-szeder.dev@gmail.com","threadId":"45649","inReplyTo":"CAM0VKjkuoCotm8FB5qdTQsC0SaJiwBA9dXmdLJdSs8NwLdY8fQ@mail.gmail.com","subject":"[PATCHv2] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-12T22:07:17Z","receivedAt":"2017-04-12T22:07:36Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The last test in 't6500-gc', 'background auto gc does not run if\ngc.log is present and recent but does if it is old', added in\na831c06a2 (gc: ignore old gc.log files, 2017-02-10), may sporadically\ntrigger an error message from the test harness:\n\n rm: cannot remove 'trash directory.t6500-gc/.git/objects': Directory not empty\n\nThe test in question ends with executing an auto gc in the backround,\nwhich occasionally takes so long that it's still running when\n'test_done' is about to remove the trash directory. This 'rm -rf\n$trash' in the foreground might race with the detached auto gc to\ncreate and delete files and directories, and gc might (re-)create a\npath that 'rm' already visited and removed, triggering the above error\nmessage when 'rm' attempts to remove its parent directory.\n\nCommit bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01)\nfixed the same problem in a different test script by simply\ndisallowing background gc. Unfortunately, what worked there is not\napplicable here, because the purpose of this test is to check the\nbehavior of a detached auto gc.\n\nMake sure that the test doesn't continue before the gc is finished in\nthe background with a clever bit of shell trickery:\n\n - Open fd 9 in the shell, to be inherited by the background gc\n process, because our daemonize() only closes the standard fds 0,\n 1 and 2.\n - Duplicate this fd 9 to stdout.\n - Read 'git gc's stdout, and thus fd 9, through a command\n substitution. We don't actually care about gc's output, but this\n construct has two useful properties:\n - This read blocks until stdout or fd 9 are open. While stdout is\n closed after the main gc process creates the background process\n and exits, fd 9 remains open until the backround process exits.\n - The variable assignment from the command substitution gets its\n exit status from the command executed within the command\n substitution, i.e. a failing main gc process will cause the test\n to fail.\n\nNote, that this fd trickery doesn't work on Windows, because due to\nMSYS limitations the git process only inherits the standard fds 0, 1\nand 2 from the shell. Luckily, it doesn't matter in this case,\nbecause on Windows daemonize() is basically a noop, thus 'git gc\n--auto' always runs in the foreground.\n\nAnd since we can now continue the test reliably after the detached gc\nfinished, check that there is only a single packfile left at the end,\ni.e. that the detached gc actually did what it was supposed to do.\nAlso add a comment at the end of the test script to warn developers of\nfuture tests about this issue of long running detached gc processes.\n\nHelped-by: Jeff King \nHelped-by: Johannes Sixt \nSigned-off-by: SZEDER Gábor \n---\n t/t6500-gc.sh | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 08de2e8ab..cc7acd101 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -67,6 +67,16 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre\n \ttest_line_count = 2 new # There is one new pack and its .idx\n '\n \n+run_and_wait_for_auto_gc () {\n+\t# We read stdout from gc for the side effect of waiting until the\n+\t# background gc process exits, closing its fd 9. Furthermore, the\n+\t# variable assignment from a command substitution preserves the\n+\t# exit status of the main gc process.\n+\t# Note: this fd trickery doesn't work on Windows, but there is no\n+\t# need to, because on Win the auto gc always runs in the foreground.\n+\tdoesnt_matter=$(git gc --auto 9>&1)\n+}\n+\n test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n \ttest_commit foo &&\n \ttest_commit bar &&\n@@ -80,7 +90,13 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest-chmtime =-345600 .git/gc.log &&\n \ttest_must_fail git gc --auto &&\n \ttest_config gc.logexpiry 2.days &&\n-\tgit gc --auto\n+\trun_and_wait_for_auto_gc &&\n+\tls .git/objects/pack/pack-*.pack >packs &&\n+\ttest_line_count = 1 packs\n '\n \n+# DO NOT leave a detached auto gc process running near the end of the\n+# test script: it can run long enough in the background to racily\n+# interfere with the cleanup in 'test_done'.\n+\n test_done\n-- \n2.12.2.613.g9c5b79913\n\n"},{"id":"316718","messageId":"20170413103138.12717-1-szeder.dev@gmail.com","threadId":"45649","inReplyTo":"20170412220717.7357-1-szeder.dev@gmail.com","subject":"[PATCHv2.1] t6500: wait for detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-13T10:31:38Z","receivedAt":"2017-04-13T10:31:54Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The last test in 't6500-gc', 'background auto gc does not run if\ngc.log is present and recent but does if it is old', added in\na831c06a2 (gc: ignore old gc.log files, 2017-02-10), may sporadically\ntrigger an error message from the test harness:\n\n rm: cannot remove 'trash directory.t6500-gc/.git/objects': Directory not empty\n\nThe test in question ends with executing an auto gc in the backround,\nwhich occasionally takes so long that it's still running when\n'test_done' is about to remove the trash directory. This 'rm -rf\n$trash' in the foreground might race with the detached auto gc to\ncreate and delete files and directories, and gc might (re-)create a\npath that 'rm' already visited and removed, triggering the above error\nmessage when 'rm' attempts to remove its parent directory.\n\nCommit bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01)\nfixed the same problem in a different test script by simply\ndisallowing background gc. Unfortunately, what worked there is not\napplicable here, because the purpose of this test is to check the\nbehavior of a detached auto gc.\n\nMake sure that the test doesn't continue before the gc is finished in\nthe background with a clever bit of shell trickery:\n\n - Open fd 9 in the shell, to be inherited by the background gc\n process, because our daemonize() only closes the standard fds 0,\n 1 and 2.\n - Duplicate this fd 9 to stdout.\n - Read 'git gc's stdout, and thus fd 9, through a command\n substitution. We don't actually care about gc's output, but this\n construct has two useful properties:\n - This read blocks until stdout or fd 9 are open. While stdout is\n closed after the main gc process creates the background process\n and exits, fd 9 remains open until the backround process exits.\n - The variable assignment from the command substitution gets its\n exit status from the command executed within the command\n substitution, i.e. a failing main gc process will cause the test\n to fail.\n\nNote, that this fd trickery doesn't work on Windows, because due to\nMSYS limitations the git process only inherits the standard fds 0, 1\nand 2 from the shell. Luckily, it doesn't matter in this case,\nbecause on Windows daemonize() is basically a noop, thus 'git gc\n--auto' always runs in the foreground.\n\nAnd since we can now continue the test reliably after the detached gc\nfinished, check that there is only a single packfile left at the end,\ni.e. that the detached gc actually did what it was supposed to do.\nAlso add a comment at the end of the test script to warn developers of\nfuture tests about this issue of long running detached gc processes.\n\nHelped-by: Jeff King \nHelped-by: Johannes Sixt \nSigned-off-by: SZEDER Gábor \n---\n\nUpdated subject line, but otherwise the same as v2.\n\n t/t6500-gc.sh | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 08de2e8ab..cc7acd101 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -67,6 +67,16 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre\n \ttest_line_count = 2 new # There is one new pack and its .idx\n '\n \n+run_and_wait_for_auto_gc () {\n+\t# We read stdout from gc for the side effect of waiting until the\n+\t# background gc process exits, closing its fd 9. Furthermore, the\n+\t# variable assignment from a command substitution preserves the\n+\t# exit status of the main gc process.\n+\t# Note: this fd trickery doesn't work on Windows, but there is no\n+\t# need to, because on Win the auto gc always runs in the foreground.\n+\tdoesnt_matter=$(git gc --auto 9>&1)\n+}\n+\n test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n \ttest_commit foo &&\n \ttest_commit bar &&\n@@ -80,7 +90,13 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest-chmtime =-345600 .git/gc.log &&\n \ttest_must_fail git gc --auto &&\n \ttest_config gc.logexpiry 2.days &&\n-\tgit gc --auto\n+\trun_and_wait_for_auto_gc &&\n+\tls .git/objects/pack/pack-*.pack >packs &&\n+\ttest_line_count = 1 packs\n '\n \n+# DO NOT leave a detached auto gc process running near the end of the\n+# test script: it can run long enough in the background to racily\n+# interfere with the cleanup in 'test_done'.\n+\n test_done\n-- \n2.12.2.613.g9c5b79913\n\n"},{"id":"316726","messageId":"af5e80be96d142ed9707edadf9ceae3f@exmbdft7.ad.twosigma.com","threadId":"45649","inReplyTo":"20170413103138.12717-1-szeder.dev@gmail.com","subject":"RE: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test script","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2017-04-13T16:06:16Z","receivedAt":"2017-04-13T16:06:25Z","isPatch":false,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"Thanks for fixing this!\n\n> -----Original Message-----\n> From: SZEDER Gábor [mailto:szeder.dev@gmail.com]\n> Sent: Thursday, April 13, 2017 6:32 AM\n> To: Junio C Hamano \n> Cc: Jeff King ; Johannes Sixt ; David Turner\n> ; git@vger.kernel.org; SZEDER Gábor\n> \n> Subject: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test\n> script\n> \n> The last test in 't6500-gc', 'background auto gc does not run if gc.log is present\n> and recent but does if it is old', added in\n> a831c06a2 (gc: ignore old gc.log files, 2017-02-10), may sporadically trigger an\n> error message from the test harness:\n> \n> rm: cannot remove 'trash directory.t6500-gc/.git/objects': Directory not empty\n> \n> The test in question ends with executing an auto gc in the backround, which\n> occasionally takes so long that it's still running when 'test_done' is about to\n> remove the trash directory. This 'rm -rf $trash' in the foreground might race\n> with the detached auto gc to create and delete files and directories, and gc\n> might (re-)create a path that 'rm' already visited and removed, triggering the\n> above error message when 'rm' attempts to remove its parent directory.\n> \n> Commit bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) fixed the\n> same problem in a different test script by simply disallowing background gc.\n> Unfortunately, what worked there is not applicable here, because the purpose\n> of this test is to check the behavior of a detached auto gc.\n> \n> Make sure that the test doesn't continue before the gc is finished in the\n> background with a clever bit of shell trickery:\n> \n> - Open fd 9 in the shell, to be inherited by the background gc\n> process, because our daemonize() only closes the standard fds 0,\n> 1 and 2.\n> - Duplicate this fd 9 to stdout.\n> - Read 'git gc's stdout, and thus fd 9, through a command\n> substitution. We don't actually care about gc's output, but this\n> construct has two useful properties:\n> - This read blocks until stdout or fd 9 are open. While stdout is\n> closed after the main gc process creates the background process\n> and exits, fd 9 remains open until the backround process exits.\n> - The variable assignment from the command substitution gets its\n> exit status from the command executed within the command\n> substitution, i.e. a failing main gc process will cause the test\n> to fail.\n> \n> Note, that this fd trickery doesn't work on Windows, because due to MSYS\n> limitations the git process only inherits the standard fds 0, 1 and 2 from the shell.\n> Luckily, it doesn't matter in this case, because on Windows daemonize() is\n> basically a noop, thus 'git gc --auto' always runs in the foreground.\n> \n> And since we can now continue the test reliably after the detached gc finished,\n> check that there is only a single packfile left at the end, i.e. that the detached gc\n> actually did what it was supposed to do.\n> Also add a comment at the end of the test script to warn developers of future\n> tests about this issue of long running detached gc processes.\n> \n> Helped-by: Jeff King \n> Helped-by: Johannes Sixt \n> Signed-off-by: SZEDER Gábor \n> ---\n> \n> Updated subject line, but otherwise the same as v2.\n> \n> t/t6500-gc.sh | 18 +++++++++++++++++-\n> 1 file changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 08de2e8ab..cc7acd101 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -67,6 +67,16 @@ test_expect_success 'auto gc with too many loose\n> objects does not attempt to cre\n> \ttest_line_count = 2 new # There is one new pack and its .idx '\n> \n> +run_and_wait_for_auto_gc () {\n> +\t# We read stdout from gc for the side effect of waiting until the\n> +\t# background gc process exits, closing its fd 9. Furthermore, the\n> +\t# variable assignment from a command substitution preserves the\n> +\t# exit status of the main gc process.\n> +\t# Note: this fd trickery doesn't work on Windows, but there is no\n> +\t# need to, because on Win the auto gc always runs in the foreground.\n> +\tdoesnt_matter=$(git gc --auto 9>&1)\n> +}\n> +\n> test_expect_success 'background auto gc does not run if gc.log is present and\n> recent but does if it is old' '\n> \ttest_commit foo &&\n> \ttest_commit bar &&\n> @@ -80,7 +90,13 @@ test_expect_success 'background auto gc does not run if\n> gc.log is present and re\n> \ttest-chmtime =-345600 .git/gc.log &&\n> \ttest_must_fail git gc --auto &&\n> \ttest_config gc.logexpiry 2.days &&\n> -\tgit gc --auto\n> +\trun_and_wait_for_auto_gc &&\n> +\tls .git/objects/pack/pack-*.pack >packs &&\n> +\ttest_line_count = 1 packs\n> '\n> \n> +# DO NOT leave a detached auto gc process running near the end of the #\n> +test script: it can run long enough in the background to racily #\n> +interfere with the cleanup in 'test_done'.\n> +\n> test_done\n> --\n> 2.12.2.613.g9c5b79913\n\n"},{"id":"316727","messageId":"20170413163701.33zjwrgnlfxjfwjo@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjkuoCotm8FB5qdTQsC0SaJiwBA9dXmdLJdSs8NwLdY8fQ@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-13T16:37:01Z","receivedAt":"2017-04-13T16:37:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 12:03:20AM +0200, SZEDER Gábor wrote:\n\n> > I couldn't get the original to show a failure, though, even under heavy\n> > load. So maybe widening the race is enough.\n> \n> Just to be clear: it's only an occasionally appearing error message.\n> There is no failure in the sense of test failure, because 'rm -rf\n> $trash' erroring out during housekeeping does not fail the test suite.\n\nAh, OK, that makes more sense. I can detect it reliably by just checking\n\n ! test -d $root/trash*\n\nin my stress-test after the test successfully completes.\n\n-Peff\n"},{"id":"316730","messageId":"20170413164451.quvt7xdzsm64lntp@sigill.intra.peff.net","threadId":"45649","inReplyTo":"20170413103138.12717-1-szeder.dev@gmail.com","subject":"Re: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-13T16:44:51Z","receivedAt":"2017-04-13T16:45:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 12:31:38PM +0200, SZEDER Gábor wrote:\n\n> Note, that this fd trickery doesn't work on Windows, because due to\n> MSYS limitations the git process only inherits the standard fds 0, 1\n> and 2 from the shell. Luckily, it doesn't matter in this case,\n> because on Windows daemonize() is basically a noop, thus 'git gc\n> --auto' always runs in the foreground.\n> \n> And since we can now continue the test reliably after the detached gc\n> finished, check that there is only a single packfile left at the end,\n> i.e. that the detached gc actually did what it was supposed to do.\n> Also add a comment at the end of the test script to warn developers of\n> future tests about this issue of long running detached gc processes.\n\nThe whole thing looks nicely explained, and I'm happy that we're able to\nreliably add this extra check at the end of the test.\n\nI did wonder what will happen if Windows learns to daemonize() the\nauto-gc. I don't think we'll get an immediate test failure, but this\ntest will become racy again. But this time we'll actually notice the\nracy failure, because the \"ls\" will report extra packs if it runs before\nthe background gc does. At which point we can revisit this.\n\nIt would be nice if there were a non-racy way to detect whether we\ndaemonized or not, and complain on Windows when we do. Then we'll be\nnotified immediately when daemonize() changes by the test failure,\nrather than waiting for a racy failure.\n\nI guess we could probably grep for the \"in the background\" message from\nthe parent gc. OTOH, maybe it is not even worth it. The racy version\nshould fail reasonably promptly, I think, and the comments you've left\nwould point any investigator in the right direction.\n\n-Peff\n"},{"id":"316737","messageId":"CAGZ79kY6TmEV38NkYJwkb5=Bu69tCjx6k+t6An5_52h8rEo5-Q@mail.gmail.com","threadId":"45649","inReplyTo":"20170413163701.33zjwrgnlfxjfwjo@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T17:55:08Z","receivedAt":"2017-04-13T17:55:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 9:37 AM, Jeff King wrote:\n> Ah, OK, that makes more sense. I can detect it reliably by just checking\n>\n> ! test -d $root/trash*\n>\n> in my stress-test after the test successfully completes.\n\nWould we want to report such an error from the test suite itself?\n(My line of thinking is that this is a similar issue to broken && chains,\nwhich we do report).\n\nStefan\n"},{"id":"316738","messageId":"20170413175701.5ogpe7qbflbkgljm@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAGZ79kY6TmEV38NkYJwkb5=Bu69tCjx6k+t6An5_52h8rEo5-Q@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-13T17:57:01Z","receivedAt":"2017-04-13T17:57:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 10:55:08AM -0700, Stefan Beller wrote:\n\n> On Thu, Apr 13, 2017 at 9:37 AM, Jeff King wrote:\n> > Ah, OK, that makes more sense. I can detect it reliably by just checking\n> >\n> > ! test -d $root/trash*\n> >\n> > in my stress-test after the test successfully completes.\n> \n> Would we want to report such an error from the test suite itself?\n> (My line of thinking is that this is a similar issue to broken && chains,\n> which we do report).\n\nYeah, I had a similar thought. I can't think of any reason why it would\ntrigger a false positive, as long as we account for test-failure and the\n--debug flag properly. I don't think we can just drop the \"-f\" from the\nfinal \"rm\", because it may be overriding funny permissions left by\ntests.\n\n-Peff\n"},{"id":"316742","messageId":"CAM0VKjkA3O9YB5QpT+hBwt13FtQXOwiW5Fh3goSbi42V8rTzjQ@mail.gmail.com","threadId":"45649","inReplyTo":"20170413164451.quvt7xdzsm64lntp@sigill.intra.peff.net","subject":"Re: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-13T18:08:12Z","receivedAt":"2017-04-13T18:08:25Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 13, 2017 at 6:44 PM, Jeff King wrote:\n> On Thu, Apr 13, 2017 at 12:31:38PM +0200, SZEDER Gábor wrote:\n\n> I did wonder what will happen if Windows learns to daemonize() the\n> auto-gc. I don't think we'll get an immediate test failure, but this\n> test will become racy again. But this time we'll actually notice the\n> racy failure, because the \"ls\" will report extra packs if it runs before\n> the background gc does. At which point we can revisit this.\n\nDscho said that it would take significant effort to make daemonize()\nwork on Windows, so I guess it will take a while before we'll have to\nrevisit this.\n\n> It would be nice if there were a non-racy way to detect whether we\n> daemonized or not, and complain on Windows when we do. Then we'll be\n> notified immediately when daemonize() changes by the test failure,\n> rather than waiting for a racy failure.\n>\n> I guess we could probably grep for the \"in the background\" message from\n> the parent gc. OTOH, maybe it is not even worth it.\n\nThat wouldn't work at the moment, because auto gc says that it will go\nto the background even on Windows.\n\nAs it is, auto gc first prints the \"Auto packing the repo...\" message,\nand calls daemonize() after that. Which message it prints, i.e. \"in\nbackground\" or not, depends solely on the value of the 'gc.autoDetach'\nconfig variable, which is true by default. The only platform-specific\nthing about all this is the #ifdef in daemonize(), but then it's\nalready too late, the misleading message has already been printed.\n\nSee the discussion the patch for the same issue in a different test\nscript (bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01)),\nincluding a patch at the end that prevents auto gc on Windows from\nlying about going to the background (which I, not being a Windows\nuser, didn't follow through).\n\n http://public-inbox.org/git/20160505171430.Horde.-GuvDpZBfS8VI1Zcfn4bJQI@webmail.informatik.kit.edu/T/#u\n\n> The racy version\n> should fail reasonably promptly, I think, and the comments you've left\n> would point any investigator in the right direction.\n"},{"id":"316744","messageId":"20170413181211.kxrw76fgm5fbrwav@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjkA3O9YB5QpT+hBwt13FtQXOwiW5Fh3goSbi42V8rTzjQ@mail.gmail.com","subject":"Re: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-13T18:12:12Z","receivedAt":"2017-04-13T18:12:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 08:08:12PM +0200, SZEDER Gábor wrote:\n\n> On Thu, Apr 13, 2017 at 6:44 PM, Jeff King wrote:\n> > On Thu, Apr 13, 2017 at 12:31:38PM +0200, SZEDER Gábor wrote:\n> \n> > I did wonder what will happen if Windows learns to daemonize() the\n> > auto-gc. I don't think we'll get an immediate test failure, but this\n> > test will become racy again. But this time we'll actually notice the\n> > racy failure, because the \"ls\" will report extra packs if it runs before\n> > the background gc does. At which point we can revisit this.\n> \n> Dscho said that it would take significant effort to make daemonize()\n> work on Windows, so I guess it will take a while before we'll have to\n> revisit this.\n\nYeah, that's what I figured. I mostly just didn't want to leave a\ntime-bomb for future developers.\n\n> > I guess we could probably grep for the \"in the background\" message from\n> > the parent gc. OTOH, maybe it is not even worth it.\n> \n> That wouldn't work at the moment, because auto gc says that it will go\n> to the background even on Windows.\n\nAh, OK. Let's not worry about it, then. I think the way your test is\nconstructed we should get a racy failure not long after the change, and\nyour comments would lead people to realize what is going on.\n\n-Peff\n"},{"id":"316761","messageId":"CAM0VKjmK0ib+sjf0sMPeiK7DrwHq1dJ58ZhDdp6HZsuQdb-eRw@mail.gmail.com","threadId":"45649","inReplyTo":"20170413175701.5ogpe7qbflbkgljm@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-13T19:03:26Z","receivedAt":"2017-04-13T19:03:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 13, 2017 at 7:57 PM, Jeff King wrote:\n> On Thu, Apr 13, 2017 at 10:55:08AM -0700, Stefan Beller wrote:\n>\n>> On Thu, Apr 13, 2017 at 9:37 AM, Jeff King wrote:\n>> > Ah, OK, that makes more sense. I can detect it reliably by just checking\n>> >\n>> > ! test -d $root/trash*\n>> >\n>> > in my stress-test after the test successfully completes.\n>>\n>> Would we want to report such an error from the test suite itself?\n>> (My line of thinking is that this is a similar issue to broken && chains,\n>> which we do report).\n\nA broken &&-chain can harm the test's correctness by hiding errors.\nThe failing 'rm -rf $trash' during housekeeping, after all the tests\nin the test script has been run and their results reported... not\nreally, I would think, though arguably it's a sign that something is\nfishy.\n\n> Yeah, I had a similar thought. I can't think of any reason why it would\n> trigger a false positive, as long as we account for test-failure and the\n> --debug flag properly. I don't think we can just drop the \"-f\" from the\n> final \"rm\", because it may be overriding funny permissions left by\n> tests.\n\nFWIW, I used the following rather simple change during stress-testing\nthese patches (barring white-space damage):\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 13b569682..d7fa15a69 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -763,7 +763,7 @@ test_done () {\n\n test -d \"$remove_trash\" &&\n cd \"$(dirname \"$remove_trash\")\" &&\n- rm -rf \"$(basename \"$remove_trash\")\"\n+ rm -rf \"$(basename \"$remove_trash\")\" || exit 1\n\n test_at_end_hook_\n"},{"id":"316768","messageId":"20170413191236.3usecfgkjujhblo6@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjmK0ib+sjf0sMPeiK7DrwHq1dJ58ZhDdp6HZsuQdb-eRw@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-13T19:12:36Z","receivedAt":"2017-04-13T19:12:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 09:03:26PM +0200, SZEDER Gábor wrote:\n\n> > Yeah, I had a similar thought. I can't think of any reason why it would\n> > trigger a false positive, as long as we account for test-failure and the\n> > --debug flag properly. I don't think we can just drop the \"-f\" from the\n> > final \"rm\", because it may be overriding funny permissions left by\n> > tests.\n> \n> FWIW, I used the following rather simple change during stress-testing\n> these patches (barring white-space damage):\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 13b569682..d7fa15a69 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -763,7 +763,7 @@ test_done () {\n> \n> test -d \"$remove_trash\" &&\n> cd \"$(dirname \"$remove_trash\")\" &&\n> - rm -rf \"$(basename \"$remove_trash\")\"\n> + rm -rf \"$(basename \"$remove_trash\")\" || exit 1\n\nOh, right. I thought \"-f\" would cause it to exit 0 even if some items\nfailed to be removed, but that only applies to ENOENT. So I think that\nis probably a good change. I agree it's not a true error, but just a\nsign of something fishy, but I don't think it hurts to have extra lint\nchecks.\n\nReplacing it the \"exit 1\" with a \"die\" that has a message is probably a\ngood idea, though.\n\n-Peff\n"},{"id":"316777","messageId":"CAM0VKjkVUPPsvpAu1WXeNg7sv1_V=Uxhhevf9fVkZvk9WERwwQ@mail.gmail.com","threadId":"45649","inReplyTo":"20170413191236.3usecfgkjujhblo6@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-13T19:35:08Z","receivedAt":"2017-04-13T19:35:15Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Apr 13, 2017 at 9:12 PM, Jeff King wrote:\n> On Thu, Apr 13, 2017 at 09:03:26PM +0200, SZEDER Gábor wrote:\n>\n>> > Yeah, I had a similar thought. I can't think of any reason why it would\n>> > trigger a false positive, as long as we account for test-failure and the\n>> > --debug flag properly. I don't think we can just drop the \"-f\" from the\n>> > final \"rm\", because it may be overriding funny permissions left by\n>> > tests.\n>>\n>> FWIW, I used the following rather simple change during stress-testing\n>> these patches (barring white-space damage):\n>>\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 13b569682..d7fa15a69 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -763,7 +763,7 @@ test_done () {\n>>\n>> test -d \"$remove_trash\" &&\n\nI'm not sure under what circumstances the trash dir could be missing at\nthis point...\n\n>> cd \"$(dirname \"$remove_trash\")\" &&\n>> - rm -rf \"$(basename \"$remove_trash\")\"\n>> + rm -rf \"$(basename \"$remove_trash\")\" || exit 1\n\n... but when it is already removed, then I think we should not exit\nwith error here.\nNothing that a pair of {} wouldn't handle.\n\n> Oh, right. I thought \"-f\" would cause it to exit 0 even if some items\n> failed to be removed, but that only applies to ENOENT. So I think that\n> is probably a good change. I agree it's not a true error, but just a\n> sign of something fishy, but I don't think it hurts to have extra lint\n> checks.\n>\n> Replacing it the \"exit 1\" with a \"die\" that has a message is probably a\n> good idea, though.\n\nIf we can't 'cd' or 'rm -rf', then they will tell us why they failed\nanyway, most likely including the name of the trash directory.\nDo we really need more error messages than that?\n"},{"id":"316850","messageId":"20170414200829.ahqignx3pkhbuepg@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjkVUPPsvpAu1WXeNg7sv1_V=Uxhhevf9fVkZvk9WERwwQ@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-14T20:08:30Z","receivedAt":"2017-04-14T20:08:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 13, 2017 at 09:35:08PM +0200, SZEDER Gábor wrote:\n\n> >> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> >> index 13b569682..d7fa15a69 100644\n> >> --- a/t/test-lib.sh\n> >> +++ b/t/test-lib.sh\n> >> @@ -763,7 +763,7 @@ test_done () {\n> >>\n> >> test -d \"$remove_trash\" &&\n> \n> I'm not sure under what circumstances the trash dir could be missing at\n> this point...\n\nYeah, I don't really see the point of that line. Our whole goal is to\nremove it anyway.\n\n> >> cd \"$(dirname \"$remove_trash\")\" &&\n> >> - rm -rf \"$(basename \"$remove_trash\")\"\n> >> + rm -rf \"$(basename \"$remove_trash\")\" || exit 1\n> \n> ... but when it is already removed, then I think we should not exit\n> with error here.\n> Nothing that a pair of {} wouldn't handle.\n\nI suppose so. It might be worth being picky just on the principle that\nif it _is_ gone that's unexpected and we'd prefer somebody notice and\nfigure out why.\n\n> > Replacing it the \"exit 1\" with a \"die\" that has a message is probably a\n> > good idea, though.\n> \n> If we can't 'cd' or 'rm -rf', then they will tell us why they failed\n> anyway, most likely including the name of the trash directory.\n> Do we really need more error messages than that?\n\nI just meant something like \"tests passed but test cleanup failed;\naborting\" so that the user has a better idea what is going on. But since\nit shouldn't happen, maybe that doesn't matter.\n\n-Peff\n"},{"id":"317352","messageId":"CAM0VKjm1m4v9vTpwFEejBuD3NuGm+kAdEV-_rzCXCz2G4m5NGw@mail.gmail.com","threadId":"45649","inReplyTo":"20170414200829.ahqignx3pkhbuepg@sigill.intra.peff.net","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-20T16:42:59Z","receivedAt":"2017-04-20T16:43:07Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 14, 2017 at 10:08 PM, Jeff King wrote:\n> On Thu, Apr 13, 2017 at 09:35:08PM +0200, SZEDER Gábor wrote:\n>\n>> >> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> >> index 13b569682..d7fa15a69 100644\n>> >> --- a/t/test-lib.sh\n>> >> +++ b/t/test-lib.sh\n>> >> @@ -763,7 +763,7 @@ test_done () {\n>> >>\n>> >> test -d \"$remove_trash\" &&\n>>\n>> I'm not sure under what circumstances the trash dir could be missing at\n>> this point...\n>\n> Yeah, I don't really see the point of that line. Our whole goal is to\n> remove it anyway.\n\nIt turns out it's necessary, see below.\n\n>> >> cd \"$(dirname \"$remove_trash\")\" &&\n>> >> - rm -rf \"$(basename \"$remove_trash\")\"\n>> >> + rm -rf \"$(basename \"$remove_trash\")\" || exit 1\n>>\n>> ... but when it is already removed, then I think we should not exit\n>> with error here.\n>> Nothing that a pair of {} wouldn't handle.\n>\n> I suppose so. It might be worth being picky just on the principle that\n> if it _is_ gone that's unexpected and we'd prefer somebody notice and\n> figure out why.\n\nOK, agreed.\n\nHowever, we do need the above 'test -d' for this to work, because 'rm\n-rf' doesn't consider non-existing paths as errors, so we wouldn't\nnotice that the trash directory is already gone.\n\n>> > Replacing it the \"exit 1\" with a \"die\" that has a message is probably a\n>> > good idea, though.\n\ntest-lib.sh has a special 'die', different from git-sh-setup.sh's\n'die'. I'll use 'error \"uh-oh\"' instead.\n\n>> If we can't 'cd' or 'rm -rf', then they will tell us why they failed\n>> anyway, most likely including the name of the trash directory.\n>> Do we really need more error messages than that?\n>\n> I just meant something like \"tests passed but test cleanup failed;\n> aborting\" so that the user has a better idea what is going on. But since\n> it shouldn't happen, maybe that doesn't matter.\n\nAnd we do need an error message, because 'test -d' would remain silent\nwhen the directory were already missing.\n"},{"id":"317353","messageId":"20170420164536.mcbmgfzhsrjqi24f@sigill.intra.peff.net","threadId":"45649","inReplyTo":"CAM0VKjm1m4v9vTpwFEejBuD3NuGm+kAdEV-_rzCXCz2G4m5NGw@mail.gmail.com","subject":"Re: [PATCH] t6500: don't run detached auto gc at the end of the test script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T16:45:36Z","receivedAt":"2017-04-20T16:45:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 06:42:59PM +0200, SZEDER Gábor wrote:\n\n> > I suppose so. It might be worth being picky just on the principle that\n> > if it _is_ gone that's unexpected and we'd prefer somebody notice and\n> > figure out why.\n> \n> OK, agreed.\n> \n> However, we do need the above 'test -d' for this to work, because 'rm\n> -rf' doesn't consider non-existing paths as errors, so we wouldn't\n> notice that the trash directory is already gone.\n\nRight, that makes sense.\n\n> >> > Replacing it the \"exit 1\" with a \"die\" that has a message is probably a\n> >> > good idea, though.\n> \n> test-lib.sh has a special 'die', different from git-sh-setup.sh's\n> 'die'. I'll use 'error \"uh-oh\"' instead.\n\nYeah, sorry, error() is the right function in this instance.\n\n-Peff\n"},{"id":"317354","messageId":"20170420165230.5951-1-szeder.dev@gmail.com","threadId":"45649","inReplyTo":"CAM0VKjm1m4v9vTpwFEejBuD3NuGm+kAdEV-_rzCXCz2G4m5NGw@mail.gmail.com","subject":"[PATCH] test-lib: abort when can't remove trash directory","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-20T16:52:30Z","receivedAt":"2017-04-20T16:53:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"We had two similar bugs in the tests sporadically triggering error\nmessages during the removal of the trash directory, see commits\nbb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and\nef09036cf (t6500: wait for detached auto gc at the end of the test\nscript, 2017-04-13). The test script succeeded nonetheless, because\nthese errors are ignored during housekeeping in 'test_done'.\n\nHowever, such an error is a sign that something is fishy in the test\nscript. Print an error message and abort the test script when the\ntrash directory can't be removed successfully or is already removed,\nbecause that's unexpected and we woud prefer somebody notice and\nfigure out why.\n\nSigned-off-by: SZEDER Gábor \n---\n\nNote, that the commit message references ef09036cf (t6500: wait for\ndetached auto gc at the end of the test script, 2017-04-13), which\nis still only in 'pu'.\n\n t/test-lib.sh | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 13b569682..e9e6f677d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -761,9 +761,12 @@ test_done () {\n \t\t\tsay \"1..$test_count$skip_all\"\n \t\tfi\n \n-\t\ttest -d \"$remove_trash\" &&\n+\t\ttest -d \"$remove_trash\" ||\n+\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n+\n \t\tcd \"$(dirname \"$remove_trash\")\" &&\n-\t\trm -rf \"$(basename \"$remove_trash\")\"\n+\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n+\t\terror \"Tests passed but test cleanup failed; aborting\"\n \n \t\ttest_at_end_hook_\n \n-- \n2.12.2.613.g9c5b79913\n\n"},{"id":"317365","messageId":"20170420190654.24ufwrhpllpdpmkk@sigill.intra.peff.net","threadId":"45649","inReplyTo":"20170420165230.5951-1-szeder.dev@gmail.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T19:06:54Z","receivedAt":"2017-04-20T19:07:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 06:52:30PM +0200, SZEDER Gábor wrote:\n\n> We had two similar bugs in the tests sporadically triggering error\n> messages during the removal of the trash directory, see commits\n> bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and\n> ef09036cf (t6500: wait for detached auto gc at the end of the test\n> script, 2017-04-13). The test script succeeded nonetheless, because\n> these errors are ignored during housekeeping in 'test_done'.\n> \n> However, such an error is a sign that something is fishy in the test\n> script. Print an error message and abort the test script when the\n> trash directory can't be removed successfully or is already removed,\n> because that's unexpected and we woud prefer somebody notice and\n> figure out why.\n> \n> Signed-off-by: SZEDER Gábor \n\nI think this is a good idea, and the patch itself looks good. Thanks.\n\n-Peff\n"},{"id":"317441","messageId":"xmqq7f2e8u14.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"20170420165230.5951-1-szeder.dev@gmail.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T00:48:55Z","receivedAt":"2017-04-21T00:49:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> We had two similar bugs in the tests sporadically triggering error\n> messages during the removal of the trash directory, see commits\n> bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and\n> ef09036cf (t6500: wait for detached auto gc at the end of the test\n> script, 2017-04-13). The test script succeeded nonetheless, because\n> these errors are ignored during housekeeping in 'test_done'.\n>\n> However, such an error is a sign that something is fishy in the test\n> script. Print an error message and abort the test script when the\n> trash directory can't be removed successfully or is already removed,\n> because that's unexpected and we woud prefer somebody notice and\n> figure out why.\n\nMakes sense to me, too.\n\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n>\n> Note, that the commit message references ef09036cf (t6500: wait for\n> detached auto gc at the end of the test script, 2017-04-13), which\n> is still only in 'pu'.\n\nI think that one is already part of 2.13-rc0 ;-)\n\n> t/test-lib.sh | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 13b569682..e9e6f677d 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -761,9 +761,12 @@ test_done () {\n> \t\t\tsay \"1..$test_count$skip_all\"\n> \t\tfi\n> \n> -\t\ttest -d \"$remove_trash\" &&\n> +\t\ttest -d \"$remove_trash\" ||\n> +\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n> +\n> \t\tcd \"$(dirname \"$remove_trash\")\" &&\n> -\t\trm -rf \"$(basename \"$remove_trash\")\"\n> +\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n> +\t\terror \"Tests passed but test cleanup failed; aborting\"\n> \n> \t\ttest_at_end_hook_\n"},{"id":"317514","messageId":"CAM0VKj=kaFseg9cs9SOyzJCVtpBXD8sq9mWWWYg=mAxu7uRB7g@mail.gmail.com","threadId":"45649","inReplyTo":"xmqq7f2e8u14.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2017-04-21T20:06:13Z","receivedAt":"2017-04-21T20:06:20Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 21, 2017 at 2:48 AM, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n>\n>> We had two similar bugs in the tests sporadically triggering error\n>> messages during the removal of the trash directory, see commits\n>> bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01) and\n>> ef09036cf (t6500: wait for detached auto gc at the end of the test\n>> script, 2017-04-13). The test script succeeded nonetheless, because\n>> these errors are ignored during housekeeping in 'test_done'.\n>>\n>> However, such an error is a sign that something is fishy in the test\n>> script. Print an error message and abort the test script when the\n>> trash directory can't be removed successfully or is already removed,\n>> because that's unexpected and we woud prefer somebody notice and\n\ns/woud/would/\n\n>> figure out why.\n\n\n> Note, that the commit message references ef09036cf (t6500: wait for\n>> detached auto gc at the end of the test script, 2017-04-13), which\n>> is still only in 'pu'.\n>\n> I think that one is already part of 2.13-rc0 ;-)\n\nYeah, I should have fetched before submitting.\n"},{"id":"317516","messageId":"20170421201527.wdtxhox3p4g35gex@sigill.intra.peff.net","threadId":"45649","inReplyTo":"20170420165230.5951-1-szeder.dev@gmail.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-21T20:15:28Z","receivedAt":"2017-04-21T20:15:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 20, 2017 at 06:52:30PM +0200, SZEDER Gábor wrote:\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 13b569682..e9e6f677d 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -761,9 +761,12 @@ test_done () {\n> \t\t\tsay \"1..$test_count$skip_all\"\n> \t\tfi\n> \n> -\t\ttest -d \"$remove_trash\" &&\n> +\t\ttest -d \"$remove_trash\" ||\n> +\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n\nI think I found out why this \"test -d\" was here in the first place:\n\n $ ./t0000-basic.sh --debug\n [...]\n # passed all 77 test(s)\n 1..77\n error: Tests passed but trash directory already removed before test cleanup; aborting\n\nWhen --debug is in use, we do not set $remove_trash. The original was\nrelying on 'test -d \"\"' to return false.\n\nI think this whole removal block should probably be moved inside a\nconditional like:\n\n if test -n \"$remove_trash\"\n then\n ...\n fi\n\nI also wonder if we should come up with a better name than\n$remove_trash. A script which unknowingly overwrites that variable would\nbe disastrous.\n\nPerhaps we should drop it entirely and just do:\n\n if test -z \"$debug\"\n then\n test -d \"$TRASH_DIRECTORY\" ||\n error \"Tests passed but...\"\n [and so forth...]\n fi\n\n-Peff\n"},{"id":"317621","messageId":"xmqqk26a4q69.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"20170421201527.wdtxhox3p4g35gex@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-24T00:14:54Z","receivedAt":"2017-04-24T00:15:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> -\t\ttest -d \"$remove_trash\" &&\n>> +\t\ttest -d \"$remove_trash\" ||\n>> +\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n>\n> I think I found out why this \"test -d\" was here in the first place:\n>\n> $ ./t0000-basic.sh --debug\n> [...]\n> # passed all 77 test(s)\n> 1..77\n> error: Tests passed but trash directory already removed before test cleanup; aborting\n>\n> When --debug is in use, we do not set $remove_trash. The original was\n> relying on 'test -d \"\"' to return false.\n>\n> I think this whole removal block should probably be moved inside a\n> conditional like:\n>\n> if test -n \"$remove_trash\"\n> then\n> ...\n> fi\n\nThanks for digging. Yes, checking for non-empty string is\ndefinitely better.\n\n> I also wonder if we should come up with a better name than\n> $remove_trash. A script which unknowingly overwrites that variable would\n> be disastrous.\n>\n> Perhaps we should drop it entirely and just do:\n>\n> if test -z \"$debug\"\n> then\n> test -d \"$TRASH_DIRECTORY\" ||\n> error \"Tests passed but...\"\n> [and so forth...]\n> fi\n\nOK. I am wondering why we do not do \n\n\trm -fr \"$TRASH_DIRECTORY\"\n\nand do this instead:\n\n\tcd \"$(dirname \"$remove_trash\")\" &&\n\trm -rf \"$(basename \"$remove_trash\")\"\n\nin the original. It feels somewhat unnatural.\n\n\t... goes and looks ...\n\nOf course, back when abc5d372 (\"Enable parallel tests\", 2008-08-08)\nwas writen, we didn't even have TRASH_DIRECTORY variable; because\nthe way the test-lib.sh ensured that the trash directory is prestine\nwas to first do a 'rm -fr \"$test\"' before the first test_create_repo,\nthe above makes sort of matches how that initial removal is done.\n\nSo perhaps we can simplify and make it more robust by doing this?\n\n t/test-lib.sh | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex cde7fc7fcf..f1ab8f33d9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -760,9 +760,14 @@ test_done () {\n \t\t\tsay \"1..$test_count$skip_all\"\n \t\tfi\n \n-\t\ttest -d \"$remove_trash\" &&\n-\t\tcd \"$(dirname \"$remove_trash\")\" &&\n-\t\trm -rf \"$(basename \"$remove_trash\")\"\n+\t\tif test -z \"$debug\"\n+\t\tthen\n+\t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n+\t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n+\n+\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n+\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n+\t\tfi\n \n \t\ttest_at_end_hook_\n \n"},{"id":"317627","messageId":"20170424014339.nuh7ixlqudfnftzp@sigill.intra.peff.net","threadId":"45649","inReplyTo":"xmqqk26a4q69.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-24T01:43:40Z","receivedAt":"2017-04-24T01:43:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 23, 2017 at 05:14:54PM -0700, Junio C Hamano wrote:\n\n> OK. I am wondering why we do not do \n> \n> \trm -fr \"$TRASH_DIRECTORY\"\n> \n> and do this instead:\n> \n> \tcd \"$(dirname \"$remove_trash\")\" &&\n> \trm -rf \"$(basename \"$remove_trash\")\"\n> \n> in the original. It feels somewhat unnatural.\n\nI assumed the \"cd\" was there because some systems may be unhappy\nremoving a directory which is our current working directory. That might\njust be superstition, though.\n\nThe use of \"basename\" in the second does seem superfluous, since the\ntrash directory should be the full path (I suspect it wasn't in the\nearly days, though).\n\n> So perhaps we can simplify and make it more robust by doing this?\n> [...]\n> +\t\tif test -z \"$debug\"\n> +\t\tthen\n> +\t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n> +\t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n> +\n> +\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n> +\t\tfi\n\nThat looks fine, assuming the answer to the \"is the cwd important\"\nquestion is \"no\".\n\n-Peff\n"},{"id":"317633","messageId":"xmqqwpaa1pgb.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"20170424014339.nuh7ixlqudfnftzp@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-24T02:58:44Z","receivedAt":"2017-04-24T02:58:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Apr 23, 2017 at 05:14:54PM -0700, Junio C Hamano wrote:\n>\n>> OK. I am wondering why we do not do \n>> \n>> \trm -fr \"$TRASH_DIRECTORY\"\n>> \n>> and do this instead:\n>> \n>> \tcd \"$(dirname \"$remove_trash\")\" &&\n>> \trm -rf \"$(basename \"$remove_trash\")\"\n>> \n>> in the original. It feels somewhat unnatural.\n>\n> I assumed the \"cd\" was there because some systems may be unhappy\n> removing a directory which is our current working directory. That might\n> just be superstition, though.\n\nAhh, OK, that makes sense. I forgot about that.\n\n> The use of \"basename\" in the second does seem superfluous, since the\n> trash directory should be the full path (I suspect it wasn't in the\n> early days, though).\n>\n>> So perhaps we can simplify and make it more robust by doing this?\n>> [...]\n>> +\t\tif test -z \"$debug\"\n>> +\t\tthen\n>> +\t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n>> +\t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n>> +\n>> +\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n>> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n>> +\t\tfi\n>\n> That looks fine, assuming the answer to the \"is the cwd important\"\n> question is \"no\".\n\nAnd I do think the answer would be \"yes\", unfortunately. There are\nsystems that do not even allow a file to be removed while it is\nopen, so...\n\n"},{"id":"317640","messageId":"xmqq60hu1mhq.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"xmqqwpaa1pgb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-24T04:02:41Z","receivedAt":"2017-04-24T04:04:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> That looks fine, assuming the answer to the \"is the cwd important\"\n>> question is \"no\".\n>\n> And I do think the answer would be \"yes\", unfortunately. There are\n> systems that do not even allow a file to be removed while it is\n> open, so...\n\nIn addition to \"some platforms may not want cwd removed\", this\ndirectory would be where test_at_end_hook_ will be running in, so\nwe'd better be consistent with the current behaviour.\n\nSecond try that hopefully is much less damaging\n\n t/test-lib.sh | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex cb0766b9ee..4e8f511870 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -760,12 +760,15 @@ test_done () {\n \t\t\tsay \"1..$test_count$skip_all\"\n \t\tfi\n \n-\t\ttest -d \"$remove_trash\" ||\n-\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n+\t\tif test -z \"$debug\"\n+\t\tthen\n+\t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n+\t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n \n-\t\tcd \"$(dirname \"$remove_trash\")\" &&\n-\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n-\t\terror \"Tests passed but test cleanup failed; aborting\"\n+\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n+\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n+\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n+\t\tfi\n \n \t\ttest_at_end_hook_\n \n\n\n\n"},{"id":"317648","messageId":"20170424075241.ybbq2cpvbmwtdwz7@sigill.intra.peff.net","threadId":"45649","inReplyTo":"xmqq60hu1mhq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-24T07:52:42Z","receivedAt":"2017-04-24T07:52:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 23, 2017 at 09:02:41PM -0700, Junio C Hamano wrote:\n\n> >> That looks fine, assuming the answer to the \"is the cwd important\"\n> >> question is \"no\".\n> >\n> > And I do think the answer would be \"yes\", unfortunately. There are\n> > systems that do not even allow a file to be removed while it is\n> > open, so...\n> \n> In addition to \"some platforms may not want cwd removed\", this\n> directory would be where test_at_end_hook_ will be running in, so\n> we'd better be consistent with the current behaviour.\n\nGood point. There's only one caller, but it does care about being in\nthat directory.\n\n> Second try that hopefully is much less damaging\n> \n> t/test-lib.sh | 13 ++++++++-----\n> 1 file changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index cb0766b9ee..4e8f511870 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -760,12 +760,15 @@ test_done () {\n> \t\t\tsay \"1..$test_count$skip_all\"\n> \t\tfi\n> \n> -\t\ttest -d \"$remove_trash\" ||\n> -\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n> +\t\tif test -z \"$debug\"\n> +\t\tthen\n> +\t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n> +\t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n> \n> -\t\tcd \"$(dirname \"$remove_trash\")\" &&\n> -\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n> -\t\terror \"Tests passed but test cleanup failed; aborting\"\n> +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n> +\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n> +\t\tfi\n\nYeah, that looks good to me.\n\n-Peff\n"},{"id":"317653","messageId":"18ee407c-db3b-16a2-4822-0ff302045b27@web.de","threadId":"45649","inReplyTo":"20170424075241.ybbq2cpvbmwtdwz7@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-04-24T09:39:26Z","receivedAt":"2017-04-24T09:39:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"[]\n>>\n>> -\t\tcd \"$(dirname \"$remove_trash\")\" &&\n>> -\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n>> -\t\terror \"Tests passed but test cleanup failed; aborting\"\n>> +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n>> +\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n>> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n>> +\t\tfi\n>\n> Yeah, that looks good to me.\n\nDoes it ?\nChecking the error code of \"rm -f\" doesn't work as expected from the script:\nrm -rf DoesNotExist ; echo $?\n0\n\nI think it should be\n\n>> +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n>> +\t\t\trm -r \"$TRASH_DIRECTORY\" ||\n>> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n\n\n\n\n\n"},{"id":"317654","messageId":"20170424094654.eucylyv7k2l332jw@sigill.intra.peff.net","threadId":"45649","inReplyTo":"18ee407c-db3b-16a2-4822-0ff302045b27@web.de","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-24T09:46:54Z","receivedAt":"2017-04-24T09:47:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2017 at 11:39:26AM +0200, Torsten Bögershausen wrote:\n\n> []\n> > > \n> > > -\t\tcd \"$(dirname \"$remove_trash\")\" &&\n> > > -\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n> > > -\t\terror \"Tests passed but test cleanup failed; aborting\"\n> > > +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n> > > +\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n> > > +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n> > > +\t\tfi\n> > \n> > Yeah, that looks good to me.\n> \n> Does it ?\n> Checking the error code of \"rm -f\" doesn't work as expected from the script:\n> rm -rf DoesNotExist ; echo $?\n> 0\n\nENOENT is treated specially by \"rm -f\". We cover that case explicitly\nwith the \"test -d\" above (in the bit you didn't quote), and then rely on\n\"rm\" to report other errors. Like:\n\n $ rm -rf /etc/passwd ; echo $?\n rm: cannot remove '/etc/passwd': Permission denied\n 1\n\n> I think it should be\n> \n> > > +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n> > > +\t\t\trm -r \"$TRASH_DIRECTORY\" ||\n> > > +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n\nI think we need the \"-f\" to overcome rm's tendency to prompt in some\ncases:\n\n $ echo content >foo\n $ chmod 0 foo\n $ rm foo\n rm: remove write-protected regular file 'foo'? ^C\n $ rm -f foo\n [no output; it just works]\n\n-Peff\n"},{"id":"317757","messageId":"xmqq4lxdxlp0.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"18ee407c-db3b-16a2-4822-0ff302045b27@web.de","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-25T02:31:07Z","receivedAt":"2017-04-25T02:31:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> []\n>>>\n>>> -\t\tcd \"$(dirname \"$remove_trash\")\" &&\n>>> -\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n>>> -\t\terror \"Tests passed but test cleanup failed; aborting\"\n>>> +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n>>> +\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n>>> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n>>> +\t\tfi\n>>\n>> Yeah, that looks good to me.\n>\n> Does it ?\n> Checking the error code of \"rm -f\" doesn't work as expected from the script:\n> rm -rf DoesNotExist ; echo $?\n> 0\n>\n> I think it should be\n>\n>>> +\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n>>> +\t\t\trm -r \"$TRASH_DIRECTORY\" ||\n>>> +\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n\nWhat Peff told you in his response is all correct, but besides, you\ncan see the patch and realize that the original has been using \"rm\n-rf\" for ages.\n\nThis change is about not using $remove_trash variable, as it felt\nbrittle and error prone than detecting $debug case and removing\n$TRASH_DIRECTORY. So in that sense, I shouldn't have even rolled\nthe removal of basename into it.\n\nHaving said that, because people care about the number of subprocess\ninvocations, I am tempted to suggest doing\n\n\tcd \"$TRASH_DIRECTORY/..\" &&\n\trm -fr \"$TRASH_DIRECTORY\" ||\n\terror ...\n\nwhich should reduce another process ;-)\n"},{"id":"317782","messageId":"xmqqmvb5uin8.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"20170424075241.ybbq2cpvbmwtdwz7@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-25T06:05:15Z","receivedAt":"2017-04-25T06:05:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Good point. There's only one caller, but it does care about being in\n> that directory.\n>\n>> Second try that hopefully is much less damaging\n\nI've been carrying it as a SQUASH??? patch, but I think it is better\nto split it as a separate pach, as removal of $remove_trash is an\noptional thing. The main thing, i.e. SZEDER's \"abort when trash is\nalready gone or when we cannot remove\" _can_ be (and is) correctly\ndone with his pach alone.\n\nSo here is what I queued on top.\n\n-- >8 --\nSubject: [PATCH] test-lib: retire $remove_trash variable\n\nThe convention \"$remove_trash is set to the trash directory that is\nused during the test, so that it will be removed at the end, but\nunder --debug option we set the varilable to empty string to\npreserve the directory\" made sense back when it was introduced, as\nthere was no $TRASH_DIRECTORY variable. These days, since no tests\nlooks at the variable, it is obscure and even risks that by mistake\nthe variable gets used for something else (e.g. remove_trash=yes)\nand cause us misbehave.\n\nRewrite the clean-up sequence in test_done helper to explicitly\ncheck the $debug condition and remove the trash directory using\nthe $TRASH_DIRECTORY variable.\n\nNote that \"go to the directory one level above the trash and then\nremove it\" is kept and this is deliverate; test_at_end_hook_ will\nkeep running from the expected location, and also some platforms may\nnot like a directory that is serving as the $cwd of a still-active\nprocess removed.\n\nWe _might_ want to rewrite\n\n cd \"$(dirname \"$TRASH_DIRECTORY\")\"\n\nfurther to\n\n cd \"$TRASH_DIRECTORY/..\"\n\nto lose one extra process, but let's leave it to a later patch.\n\nSigned-off-by: Junio C Hamano \n---\n t/test-lib.sh | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex cb0766b9ee..976566047d 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -760,12 +760,15 @@ test_done () {\n \t\t\tsay \"1..$test_count$skip_all\"\n \t\tfi\n \n-\t\ttest -d \"$remove_trash\" ||\n-\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n+\t\tif test -z \"$debug\"\n+\t\tthen\n+\t\t\ttest -d \"$TRASH_DIRECTORY\" ||\n+\t\t\terror \"Tests passed but trash directory already removed before test cleanup; aborting\"\n \n-\t\tcd \"$(dirname \"$remove_trash\")\" &&\n-\t\trm -rf \"$(basename \"$remove_trash\")\" ||\n-\t\terror \"Tests passed but test cleanup failed; aborting\"\n+\t\t\tcd \"$(dirname \"$TRASH_DIRECTORY\")\" &&\n+\t\t\trm -fr \"$TRASH_DIRECTORY\" ||\n+\t\t\terror \"Tests passed but test cleanup failed; aborting\"\n+\t\tfi\n \n \t\ttest_at_end_hook_\n \n@@ -921,7 +924,6 @@ case \"$TRASH_DIRECTORY\" in\n /*) ;; # absolute path is good\n *) TRASH_DIRECTORY=\"$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY\" ;;\n esac\n-test ! -z \"$debug\" || remove_trash=$TRASH_DIRECTORY\n rm -fr \"$TRASH_DIRECTORY\" || {\n \tGIT_EXIT_OK=t\n \techo >&5 \"FATAL: Cannot prepare test area\"\n-- \n2.13.0-rc0-308-g931de5db53\n\n"},{"id":"317783","messageId":"20170425060728.bprvsmiazho6ceyt@sigill.intra.peff.net","threadId":"45649","inReplyTo":"xmqqmvb5uin8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-25T06:07:28Z","receivedAt":"2017-04-25T06:07:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 24, 2017 at 11:05:15PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Good point. There's only one caller, but it does care about being in\n> > that directory.\n> >\n> >> Second try that hopefully is much less damaging\n> \n> I've been carrying it as a SQUASH??? patch, but I think it is better\n> to split it as a separate pach, as removal of $remove_trash is an\n> optional thing. The main thing, i.e. SZEDER's \"abort when trash is\n> already gone or when we cannot remove\" _can_ be (and is) correctly\n> done with his pach alone.\n\nSort of. It still has the bug that it dies with error() when \"--debug\"\nis used. But that is relatively minor, and as long as yours gets applied\non top I do not mind the momentary breakage.\n\n> So here is what I queued on top.\n> \n> -- >8 --\n> Subject: [PATCH] test-lib: retire $remove_trash variable\n\nYeah, this looks fine (though it could mention that it is fixing the\n\"test -d\" bug).\n\n-Peff\n"},{"id":"317785","messageId":"xmqqefwhuhfa.fsf@gitster.mtv.corp.google.com","threadId":"45649","inReplyTo":"20170425060728.bprvsmiazho6ceyt@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: abort when can't remove trash directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-25T06:31:37Z","receivedAt":"2017-04-25T06:31:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Sort of. It still has the bug that it dies with error() when \"--debug\"\n> is used.\n\nAh, I forgot about that one. Let me squash it in without the\nremove_trash change then, perhaps, but not today.\n\n"}]}