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

From: Jeff King <peff@peff.net>
Date: Apr 24, 2017, 01:43 UTC
Message-ID: <20170424014339.nuh7ixlqudfnftzp@sigill.intra.peff.net>
In-Reply-To: <xmqqk26a4q69.fsf@gitster.mtv.corp.google.com>

On Sun, Apr 23, 2017 at 05:14:54PM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> OK.  I am wondering why we do not do 
> 
> 	rm -fr "$TRASH_DIRECTORY"
> 
> and do this instead:
> 
> 	cd "$(dirname "$remove_trash")" &&
> 	rm -rf "$(basename "$remove_trash")"
> 
> in the original.  It feels somewhat unnatural.

I assumed the "cd" was there because some systems may be unhappy removing a directory which is our current working directory. That might just be superstition, though.

The use of "basename" in the second does seem superfluous, since the trash directory should be the full path (I suspect it wasn't in the early days, though).

Show 10 quoted lines

> So perhaps we can simplify and make it more robust by doing this?
> [...]
> +		if test -z "$debug"
> +		then
> +			test -d "$TRASH_DIRECTORY" ||
> +			error "Tests passed but trash directory already removed before test cleanup; aborting"
> +
> +			rm -fr "$TRASH_DIRECTORY" ||
> +			error "Tests passed but test cleanup failed; aborting"
> +		fi

That looks fine, assuming the answer to the "is the cwd important" question is "no".

-Peff