Jeff King <[email protected]> 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_
