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

From: Junio C Hamano <[email-masked]>
Date: Mon, 12 Apr 2021 14:22:09 -0700

Junio C Hamano <[email-masked]> writes:

> Ævar Arnfjörð Bjarmason  <[email-masked]> writes:
>
>> There's no point in creating a repository or directory only to decide
>> right afterwards that we're skipping all the tests.
>>
>> So let's partially revert 06478dab4c (test-lib: retire $remove_trash
>> variable, 2017-04-23) and move the decision about whether to skip all
>> tests earlier.
>>
>> I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf
>> under --debug at the end of the test, 2017-04-24) for a bug we don't
>> want to re-introduce.
>>
>> While I'm at it let's move the HOME assignment to just before
>> test_create_repo, it could be lower, but it seems better to set it
>> before calling anything in test-lib-functions.sh
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
>> ---
>
> I am not clear what the reasoning behind this change is.  Is it
> correct to say that the idea flows like the following?
>
>  0. If we do the $GIT_SKIP_TESTS check early, specifically, before
>     we create the trash directory to run the tests in, we do not
>     have to create and then remove it.
>
>  1. If we naïvely do the above, test_done called in the "ah, the
>     entirety of test is skipped" case will try to see if the trash
>     directory exists and complain, so we need to add more code in
>     that test_done codepath to avoid complaints.
>
>  2. As a part of that "more code", we mark the fact that we created
>     a new repository with $remove_trash variable.
>
> Assuming that the above flow of thought is what is behind this
> patch, I think the patch is mostly sensible.

By the way, I did wonder if the call to test_done is even needed, or
if it is even ready to be called (in other words, the machineries it
tries to finalize like junit and HARNESS_ACTIVE are even initialized
yet).  I verified that calling test_done this early is fine with
respect to the call it makes to finalize_junit_xml, though.

> remove_trash needs to be cleared at the beginning, perhaps where
> store_arg_to and opt_required_arg are cleared, though, to protect
> us from a stray environment variable.
>
> Thanks.