Re: [PATCH] test-lib: abort when can't remove trash directory
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 24, 2017, 00:14 UTC
- Message-ID
- <xmqqk26a4q69.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170421201527.wdtxhox3p4g35gex@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
>> - 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.
Show 12 quoted lines
> 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_