Re: [PATCH] t6026: ensure that long-running script really is

From: Jeff King <peff@peff.net>
Date: Nov 11, 2016, 08:41 UTC
Message-ID: <20161111084148.tgtsijn74z2pdylq@sigill.intra.peff.net>
In-Reply-To: <fbf517ad-7341-eb6d-ab38-4fe91410e57c@kdbg.org>

On Fri, Nov 11, 2016 at 07:50:14AM +0100, Johannes Sixt wrote:

Show 27 quoted lines

> Shouldn't we then move the 'kill' out of test_when_finished and make
> it a proper condition of the test? Like this?
> 
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 348d78b205..6ad8bd098a 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' '
>  		sleep 3600 &
>  		echo $! >sleep.pid
>  	EOF
> -	test_when_finished "kill \$(cat sleep.pid)" &&
>  
>  	test_write_lines >.gitattributes \
>  		"* merge=ours" "text merge=sleep-an-hour" &&
>  	test_config merge.ours.driver true &&
>  	test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
> -	git merge master
> +	git merge master &&
> +
> +	# We are testing that the custom merge driver does not block
> +	# index.lock on Windows due to an inherited file handle.
> +	# To ensure that this test checks this condition, the process
> +	# must still be running at this point (and must have started
> +	# in the first place), hence, this kill must not fail:
> +	kill "$(cat sleep.pid)"
>  '

That makes it more obvious that the return value of "kill" is important, which is good.

But the other thing the "kill" is doing is make sure we clean up after ourselves, even if another part of the test fails. What happens if "merge" unexpectedly fails after starting the sleep process?

I think the best compromise is a comment like the one you have here, but around the test_when_finished call (and possibly bumping the call down to right before "git merge master", which is where we expect the process to start).

-Peff