Re: [PATCH] test-lib.sh: try to re-chmod & retry on failed trash removal

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2021, 22:14 UTC
Message-ID: <xmqqee8seded.fsf@gitster.g>
In-Reply-To: <patch-1.1-d7e88a77fef-20211009T133043Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 16 quoted lines

> 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.

OK, the first thing these scripts do is to create $TRASH_DIRECTORY for their own use, and that is what fails (hence "Cannot prepare").

And making sure we try harder to prepare would be a good thing. Given that understanding of the motivation...

> @@ -1210,10 +1210,10 @@ test_done () {

...it is puzzling why we are touching the code that happens _after_ a test is run. If our philosophy is to leave the mess after a test fails so that the debugging person can take a look without contaminating the forensics data, but blindly and forcibly clean up before starting a new round, reusing the "do more than usual to remove" logic meant to be used for the latter here feels counter productive and also a bit counter intuitive.

Show 12 quoted lines

>  			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

Show 7 quoted lines

> +# 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

If a test lost searchable bit from directories, "u+wx" may be necessary to clean fully, no?

Show 5 quoted lines

> +		rm -rf "$dir"
> +	fi
> +	! test -d "$dir"
> +}
> +

On the other hand...

Show 9 quoted lines

>  # 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" || {

... this one does make sense and matches what the proposed log message sold us the change as.

>  	GIT_EXIT_OK=t
>  	echo >&5 "FATAL: Cannot prepare test area"
>  	exit 1

Thanks.