RE: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test script

[David Turner <David.Turner@twosigma.com>]

Thanks for fixing this!

> -----Original Message-----
> From: SZEDER Gábor [mailto:szeder.dev@gmail.com]
> Sent: Thursday, April 13, 2017 6:32 AM
> To: Junio C Hamano <gitster@pobox.com>
> Cc: Jeff King <peff@peff.net>; Johannes Sixt <j6t@kdbg.org>; David Turner
> <David.Turner@twosigma.com>; git@vger.kernel.org; SZEDER Gábor
> <szeder.dev@gmail.com>
> Subject: [PATCHv2.1] t6500: wait for detached auto gc at the end of the test
> script
> 
> 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 message 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.
> 
> Make sure that the test doesn't continue before the gc is finished in the
> background with a clever bit of shell trickery:
> 
>   - Open fd 9 in the shell, to be inherited by the background gc
>     process, because our daemonize() only closes the standard fds 0,
>     1 and 2.
>   - Duplicate this fd 9 to stdout.
>   - Read 'git gc's stdout, and thus fd 9, through a command
>     substitution.  We don't actually care about gc's output, but this
>     construct has two useful properties:
>   - This read blocks until stdout or fd 9 are open.  While stdout is
>     closed after the main gc process creates the background process
>     and exits, fd 9 remains open until the backround process exits.
>   - The variable assignment from the command substitution gets its
>     exit status from the command executed within the command
>     substitution, i.e. a failing main gc process will cause the test
>     to fail.
> 
> Note, that this fd trickery doesn't work on Windows, because due to MSYS
> limitations the git process only inherits the standard fds 0, 1 and 2 from the shell.
> Luckily, it doesn't matter in this case, because on Windows daemonize() is
> basically a noop, thus 'git gc --auto' always runs in the foreground.
> 
> And since we can now continue the test reliably after the detached gc finished,
> check that there is only a single packfile left at the end, i.e. that the detached gc
> actually did what it was supposed to do.
> Also add a comment at the end of the test script to warn developers of future
> tests about this issue of long running detached gc processes.
> 
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> 
> Updated subject line, but otherwise the same as v2.
> 
>  t/t6500-gc.sh | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 08de2e8ab..cc7acd101 100755
> --- a/t/t6500-gc.sh
> +++ b/t/t6500-gc.sh
> @@ -67,6 +67,16 @@ 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  '
> 
> +run_and_wait_for_auto_gc () {
> +	# We read stdout from gc for the side effect of waiting until the
> +	# background gc process exits, closing its fd 9.  Furthermore, the
> +	# variable assignment from a command substitution preserves the
> +	# exit status of the main gc process.
> +	# Note: this fd trickery doesn't work on Windows, but there is no
> +	# need to, because on Win the auto gc always runs in the foreground.
> +	doesnt_matter=$(git gc --auto 9>&1)
> +}
> +
>  test_expect_success 'background auto gc does not run if gc.log is present and
> recent but does if it is old' '
>  	test_commit foo &&
>  	test_commit bar &&
> @@ -80,7 +90,13 @@ test_expect_success 'background auto gc does not run if
> gc.log is present and re
>  	test-chmtime =-345600 .git/gc.log &&
>  	test_must_fail git gc --auto &&
>  	test_config gc.logexpiry 2.days &&
> -	git gc --auto
> +	run_and_wait_for_auto_gc &&
> +	ls .git/objects/pack/pack-*.pack >packs &&
> +	test_line_count = 1 packs
>  '
> 
> +# 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