Junio C Hamano <gitster@xxxxxxxxx> writes: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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 <avarab@xxxxxxxxx> >> --- > > 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.
