Subject: [PATCH] t6500: don't run detached auto gc at the end of the test script

From: SZEDER Gábor <[email-masked]>
Date: Mon, 10 Apr 2017 14:59:11 +0200

The last test in 't6500-gc', 'background auto gc does not run if
gc.log is present and recent but does if it is old', added in
a831c06a2 (gc: ignore old gc.log files, 2017-02-10), may sporadically
trigger an error message from the test harness:

  rm: cannot remove 'trash directory.t6500-gc/.git/objects': Directory not empty

The test in question ends with executing an auto gc in the backround,
which occasionally takes so long that it's still running when
'test_done' is about to remove the trash directory.  This 'rm -rf
$trash' in the foreground might race with the detached auto gc to
create and delete files and directories, and gc might (re-)create a
path that 'rm' already visited and removed, triggering the above error
when 'rm' attempts to remove its parent directory.

Commit bb05510e5 (t5510: run auto-gc in the foreground, 2016-05-01)
fixed the same problem in a different test script by simply
disallowing background gc.  Unfortunately, what worked there is not
applicable here, because the purpose of this test is to check the
behavior of a detached auto gc.

Move the detached auto gc execution further away from the end of the
test script by splitting the test in two: run the 'background auto gc
runs if an old gc.log is present' part first, leaving only the
aborting auto gc executions in the last test.  Run the first test's
detached auto gc in a dedicated sub-repository to prevent it from
interfering with the subseqent test's gc.  Also add a comment at the
end of the test script to warn developers of future tests about this
issue.

While this change doesn't completely eliminate the possibility of
this race, it significantly and seemingly sufficiently reduces its
probability.  Running t6500 in a loop while my box was under heavy CPU
and I/O load usually triggered the error under 15 seconds, but with
this patch it run overnight without incident.

(Alternatively, we could check the presence of the gc.pid file after
starting the detached auto gc, and wait while it's there.  However,
this would create a different race: the auto gc process creates the
pid file after it goes to the background, so our check in the
foreground might racily look for the pid file before it's even
created, and thus would not wait for the background gc to finish.
Furthermore, this would open up the possibility of the test hanging if
something were to go wrong with the gc process and the pid file were
not removed.)

Signed-off-by: SZEDER Gábor <[email-masked]>
---
 t/t6500-gc.sh | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index 08de2e8ab..2dc202c9b 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -67,7 +67,27 @@ test_expect_success 'auto gc with too many loose objects does not attempt to cre
 	test_line_count = 2 new # There is one new pack and its .idx
 '
 
-test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '
+test_expect_success 'background auto gc runs if an old gc.log is present' '
+	# The detached auto gc process can run long enough in the
+	# background to racily interfere with the gc execution in the
+	# subsequent test, hence the dedicated repository.
+	test_create_repo other &&
+	(
+		cd other &&
+		test_commit foo &&
+		git repack &&
+		test_commit bar &&
+		git repack &&
+		git config gc.autopacklimit 1 &&
+		git config gc.autodetach true &&
+		git config gc.logexpiry 2.days &&
+		echo fleem >.git/gc.log &&
+		test-chmtime =-345600 .git/gc.log &&	# 4 days
+		git gc --auto
+	)
+'
+
+test_expect_success 'background auto gc does not run if gc.log is present' '
 	test_commit foo &&
 	test_commit bar &&
 	git repack &&
@@ -77,10 +97,12 @@ test_expect_success 'background auto gc does not run if gc.log is present and re
 	test_must_fail git gc --auto 2>err &&
 	test_i18ngrep "^error:" err &&
 	test_config gc.logexpiry 5.days &&
-	test-chmtime =-345600 .git/gc.log &&
-	test_must_fail git gc --auto &&
-	test_config gc.logexpiry 2.days &&
-	git gc --auto
+	test-chmtime =-345600 .git/gc.log &&	# 4 days
+	test_must_fail git gc --auto
 '
 
+# DO NOT leave a detached auto gc process running near the end of the
+# test script: it can run long enough in the background to racily
+# interfere with the cleanup in 'test_done'.
+
 test_done
-- 
2.12.2.613.g9c5b79913