git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal
Date: Sat,  9 Oct 2021 15:31:02 +0200	[thread overview]
Message-ID: <patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com> (raw)

This fixes a regression in [1] where t0004-unwritable.sh was made to
use "test_when_finished" for cleanup, we wouldn't re-chmod the
".git/objects" on failure, leading to a persistent error when running
the test.

This can be demonstrated as e.g. (output snipped for less verbosity):

    $ ./t0004-unwritable.sh --run=3 --immediate
    ok 1 # skip setup (--run)
    ok 2 # skip write-tree should notice unwritable repository (--run)
    not ok 3 - commit should notice unwritable repository
    [...]
    $ ./t0004-unwritable.sh --run=3 --immediate
    rm: cannot remove '[...]/trash directory.t0004-unwritable/.git/objects/info': Permission denied
    FATAL: Cannot prepare test area
    [...]

I.e. if any of its tests failed, and the tests were being run under
"--debug"[2] or "--immediate"[3] (which was introduced after [1]) we
wouldn't re-chmod the object directory. We'd then fail on the next run
since the test setup couldn't remove the trash files.

Instead of some version of reverting [1] let's make the test-lib.sh
resilient to this edge-case, it will happen due to [1], but also
e.g. if the relevant "test-lib.sh" process is kill -9'd during the
test run. We should try harder to recover in this case. If we fail to
remove the test directory let's retry after (re-)chmod-ing it.

This doesn't need to be guarded by something that's equivalent to
"POSIXPERM" since if we don't support "chmod" we were about to fail
anyway. Let's also discard any error output from (a possibly
nonexisting) "chmod", we'll fail on the subsequent "rm -rf"
anyway.

The lack of &&-chaining between the "chmod" and "rm -rf" is
intentional, if we fail the first "rm -rf", can't chmod, but then
succeed the second time around that's what we were hoping for. We just
want to nuke the directory, not carry forward every possible error
code or error message.

1. dbda967684d (t0004 (unwritable files): simplify error handling,
   2010-09-06)
2. 5a4a088add3 (test-lib: do not remove trash_directory if called with
   --debug, 2008-08-21)
3. b586744a864 (test: skip clean-up when running under --immediate
   mode, 2011-06-27)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index aa1ad8180ed..706ce0cdeb9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1210,10 +1210,10 @@ test_done () {
 			error "Tests passed but trash directory already removed before test cleanup; aborting"
 
 			cd "$TRASH_DIRECTORY/.." &&
-			rm -fr "$TRASH_DIRECTORY" || {
+			remove_trash_directory "$TRASH_DIRECTORY" || {
 				# try again in a bit
 				sleep 5;
-				rm -fr "$TRASH_DIRECTORY"
+				remove_trash_directory "$TRASH_DIRECTORY"
 			} ||
 			error "Tests passed but test cleanup failed; aborting"
 		fi
@@ -1370,6 +1370,18 @@ then
 	exit 1
 fi
 
+# Try really hard to clean up our mess
+remove_trash_directory() {
+	dir="$1"
+	if ! rm -rf "$dir"
+	then
+		say_color info >&3 "Failed to remove trash directory, trying to re-chmod it first..."
+		chmod -R u+w "$dir" 2>/dev/null
+		rm -rf "$dir"
+	fi
+	! test -d "$dir"
+}
+
 # Are we running this test at all?
 remove_trash=
 this_test=${0##*/}
@@ -1388,7 +1400,7 @@ GNUPGHOME="$HOME/gnupg-home-not-used"
 export HOME GNUPGHOME USER_HOME
 
 # Test repository
-rm -fr "$TRASH_DIRECTORY" || {
+remove_trash_directory "$TRASH_DIRECTORY" || {
 	GIT_EXIT_OK=t
 	echo >&5 "FATAL: Cannot prepare test area"
 	exit 1
-- 
2.33.0.1492.gcc3b81fc59a


             reply	other threads:[~2021-10-09 13:31 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-09 13:31 Ævar Arnfjörð Bjarmason [this message]
2021-10-10 21:36 ` [PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal SZEDER Gábor
2021-10-10 22:14 ` Junio C Hamano
2021-10-11  4:34   ` Junio C Hamano
2021-10-11  1:41 ` [PATCH v2] " Ævar Arnfjörð Bjarmason
2021-10-12 23:03   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).