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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2017, 02:58 UTC
Message-ID: <xmqqwpaa1pgb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170424014339.nuh7ixlqudfnftzp@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

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

Ahh, OK, that makes sense.  I forgot about that.

Show 17 quoted lines

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

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