On Sat, Oct 09, 2021 at 03:31:02PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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
'--debug' runs the tests as usual, including any 'test_when_finished'
commands, so I don't see how it would be affected. And indeed running
'./t0004-unwritable.sh --run=3 --debug' repeatedly doesn't have any
issues with removing the trash directory of the previous run.
> 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@xxxxxxxxx>
> ---
> 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..."
Is this message really necessary? I don't think it is, and there was
no similar message in the previous hunk around that 'sleep 5' either.
> + 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
>
