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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 25, 2017, 06:05 UTC
Message-ID: <xmqqmvb5uin8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170424075241.ybbq2cpvbmwtdwz7@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> Good point. There's only one caller, but it does care about being in
> that directory.
>
>> Second try that hopefully is much less damaging

I've been carrying it as a SQUASH??? patch, but I think it is better to split it as a separate pach, as removal of $remove_trash is an optional thing. The main thing, i.e. SZEDER's "abort when trash is already gone or when we cannot remove" _can_ be (and is) correctly done with his pach alone.

So here is what I queued on top.

-- >8 --
Subject: [PATCH] test-lib: retire $remove_trash variable

The convention "$remove_trash is set to the trash directory that is used during the test, so that it will be removed at the end, but under --debug option we set the varilable to empty string to preserve the directory" made sense back when it was introduced, as there was no $TRASH_DIRECTORY variable. These days, since no tests looks at the variable, it is obscure and even risks that by mistake the variable gets used for something else (e.g. remove_trash=yes) and cause us misbehave.

Rewrite the clean-up sequence in test_done helper to explicitly check the $debug condition and remove the trash directory using the $TRASH_DIRECTORY variable.

Note that "go to the directory one level above the trash and then remove it" is kept and this is deliverate; test_at_end_hook_ will keep running from the expected location, and also some platforms may not like a directory that is serving as the $cwd of a still-active process removed.

We _might_ want to rewrite

    cd "$(dirname "$TRASH_DIRECTORY")"

further to

    cd "$TRASH_DIRECTORY/.."

to lose one extra process, but let's leave it to a later patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index cb0766b9ee..976566047d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -760,12 +760,15 @@ test_done () {
 			say "1..$test_count$skip_all"
 		fi
 
-		test -d "$remove_trash" ||
-		error "Tests passed but trash directory already removed before test cleanup; aborting"
+		if test -z "$debug"
+		then
+			test -d "$TRASH_DIRECTORY" ||
+			error "Tests passed but trash directory already removed before test cleanup; aborting"
 
-		cd "$(dirname "$remove_trash")" &&
-		rm -rf "$(basename "$remove_trash")" ||
-		error "Tests passed but test cleanup failed; aborting"
+			cd "$(dirname "$TRASH_DIRECTORY")" &&
+			rm -fr "$TRASH_DIRECTORY" ||
+			error "Tests passed but test cleanup failed; aborting"
+		fi
 
 		test_at_end_hook_
 
@@ -921,7 +924,6 @@ case "$TRASH_DIRECTORY" in
 /*) ;; # absolute path is good
  *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
 esac
-test ! -z "$debug" || remove_trash=$TRASH_DIRECTORY
 rm -fr "$TRASH_DIRECTORY" || {
 	GIT_EXIT_OK=t
 	echo >&5 "FATAL: Cannot prepare test area"

-- 
2.13.0-rc0-308-g931de5db53