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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Stefan Beller" <sbeller@google.com>,
	"Johannes Sixt" <j6t@kdbg.org>,
	"David Turner" <dturner@twosigma.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] test-lib: abort when can't remove trash directory
Date: Sun, 23 Apr 2017 21:43:40 -0400	[thread overview]
Message-ID: <20170424014339.nuh7ixlqudfnftzp@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqk26a4q69.fsf@gitster.mtv.corp.google.com>

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

> 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).

> 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