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

[Jeff King <peff@peff.net>]

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

> 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