Re: [PATCH] test-lib: abort when can't remove trash directory

From: Jeff King <peff@peff.net>
Date: Apr 24, 2017, 07:52 UTC
Message-ID: <20170424075241.ybbq2cpvbmwtdwz7@sigill.intra.peff.net>
In-Reply-To: <xmqq60hu1mhq.fsf@gitster.mtv.corp.google.com>

On Sun, Apr 23, 2017 at 09:02:41PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> >> That looks fine, assuming the answer to the "is the cwd important"
> >> question is "no".
> >
> > And I do think the answer would be "yes", unfortunately.  There are
> > systems that do not even allow a file to be removed while it is
> > open, so...
> 
> In addition to "some platforms may not want cwd removed", this
> directory would be where test_at_end_hook_ will be running in, so
> we'd better be consistent with the current behaviour.

Good point. There's only one caller, but it does care about being in that directory.

Show 27 quoted lines

> Second try that hopefully is much less damaging
> 
>  t/test-lib.sh | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index cb0766b9ee..4e8f511870 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -760,12 +760,15 @@ test_done () {
>  			say "1..$test_count$skip_all"
>  		fi
>  
> -		test -d "$remove_trash" ||
> -		error "Tests passed but trash directory already removed before test cleanup; aborting"
> +		if test -z "$debug"
> +		then
> +			test -d "$TRASH_DIRECTORY" ||
> +			error "Tests passed but trash directory already removed before test cleanup; aborting"
>  
> -		cd "$(dirname "$remove_trash")" &&
> -		rm -rf "$(basename "$remove_trash")" ||
> -		error "Tests passed but test cleanup failed; aborting"
> +			cd "$(dirname "$TRASH_DIRECTORY")" &&
> +			rm -fr "$TRASH_DIRECTORY" ||
> +			error "Tests passed but test cleanup failed; aborting"
> +		fi

Yeah, that looks good to me.

-Peff