Subject: Re: [PATCH v3 02/12] test-lib: bring $remove_trash out of retirement

From: Junio C Hamano <[email-masked]>
Date: Tue, 20 Apr 2021 15:28:31 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> We carry around the "$remove_trash" variable because if the directory
> is unexpectedly gone at test_done time we'll hit the error about it
> being unexpectedly gone added in df4c0d1a792 (test-lib: abort when
> can't remove trash directory, 2017-04-20).

Makes sense, but then remove_trash should be initialized to a
non-empty string at the beginning, to avoid it leaking from end-user
environment, just like the way store_arg_to and opt_required_arg are
protected?

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 3dec266221..7522faf39f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1169,7 +1169,7 @@ test_done () {
>  			esac
>  		fi
>  
> -		if test -z "$debug"
> +		if test -z "$debug" && test -n "$remove_trash"
>  		then
>  			test -d "$TRASH_DIRECTORY" ||
>  			error "Tests passed but trash directory already removed before test cleanup; aborting"
> @@ -1334,6 +1334,21 @@ then
>  	exit 1
>  fi
>  
> +# Are we running this test at all?
> +this_test=${0##*/}
> +this_test=${this_test%%-*}
> +if match_pattern_list "$this_test" $GIT_SKIP_TESTS
> +then
> +	say_color info >&3 "skipping test $this_test altogether"
> +	skip_all="skip all tests in $this_test"
> +	test_done
> +fi
> +
> +# Last-minute variable setup
> +HOME="$TRASH_DIRECTORY"
> +GNUPGHOME="$HOME/gnupg-home-not-used"
> +export HOME GNUPGHOME
> +
>  # Test repository
>  rm -fr "$TRASH_DIRECTORY" || {
>  	GIT_EXIT_OK=t
> @@ -1341,10 +1356,7 @@ rm -fr "$TRASH_DIRECTORY" || {
>  	exit 1
>  }
>  
> -HOME="$TRASH_DIRECTORY"
> -GNUPGHOME="$HOME/gnupg-home-not-used"
> -export HOME GNUPGHOME
> -
> +remove_trash=t
>  if test -z "$TEST_NO_CREATE_REPO"
>  then
>  	test_create_repo "$TRASH_DIRECTORY"
> @@ -1356,15 +1368,6 @@ fi
>  # in subprocesses like git equals our $PWD (for pathname comparisons).
>  cd -P "$TRASH_DIRECTORY" || exit 1
>  
> -this_test=${0##*/}
> -this_test=${this_test%%-*}
> -if match_pattern_list "$this_test" $GIT_SKIP_TESTS
> -then
> -	say_color info >&3 "skipping test $this_test altogether"
> -	skip_all="skip all tests in $this_test"
> -	test_done
> -fi
> -
>  if test -n "$write_junit_xml"
>  then
>  	junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"