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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

>> -		test -d "$remove_trash" &&
>> +		test -d "$remove_trash" ||
>> +		error "Tests passed but trash directory already removed before test cleanup; aborting"
>
> I think I found out why this "test -d" was here in the first place:
>
>   $ ./t0000-basic.sh --debug
>   [...]
>   # passed all 77 test(s)
>   1..77
>   error: Tests passed but trash directory already removed before test cleanup; aborting
>
> When --debug is in use, we do not set $remove_trash. The original was
> relying on 'test -d ""' to return false.
>
> I think this whole removal block should probably be moved inside a
> conditional like:
>
>   if test -n "$remove_trash"
>   then
>      ...
>   fi

Thanks for digging.  Yes, checking for non-empty string is
definitely better.

> I also wonder if we should come up with a better name than
> $remove_trash. A script which unknowingly overwrites that variable would
> be disastrous.
>
> Perhaps we should drop it entirely and just do:
>
>   if test -z "$debug"
>   then
>      test -d "$TRASH_DIRECTORY" ||
>      error "Tests passed but..."
>   [and so forth...]
>   fi

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.

	... goes and looks ...

Of course, back when abc5d372 ("Enable parallel tests", 2008-08-08)
was writen, we didn't even have TRASH_DIRECTORY variable; because
the way the test-lib.sh ensured that the trash directory is prestine
was to first do a 'rm -fr "$test"' before the first test_create_repo,
the above makes sort of matches how that initial removal is done.

So perhaps we can simplify and make it more robust by doing this?

 t/test-lib.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cde7fc7fcf..f1ab8f33d9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -760,9 +760,14 @@ test_done () {
 			say "1..$test_count$skip_all"
 		fi
 
-		test -d "$remove_trash" &&
-		cd "$(dirname "$remove_trash")" &&
-		rm -rf "$(basename "$remove_trash")"
+		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
 
 		test_at_end_hook_