Re: [PATCH] do not set GIT_TEST_MAINT_SCHEDULER where it does not matter

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Mar 28, 2024 at 8:51 PM Junio C Hamano <gitster@pobox.com> wrote:
> 31345d55 (maintenance: extract platform-specific scheduling,
> 2020-11-24) added code to t/test-lib.sh for everybody to set
> GIT_TEST_MAINT_SCHEDULER to a "safe" value and instructed the test
> writers to set the variable locally when their test wants to check
> the scheduler integration.
>
> But it did so without "export GIT_TEST_MAINT_SCHEDULER", so the
> setting does not seem to have any effect anyway.  Instead of setting
> it to a "safe" value, just unset it.

I agree that the missing `export` makes this a do-nothing assignment.
In fact, that problem traces back to the original 2fec604f8d
(maintenance: add start/stop subcommands, 2020-09-11).

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git c/t/test-lib.sh w/t/test-lib.sh
> @@ -1959,9 +1959,9 @@ test_lazy_prereq DEFAULT_REPO_FORMAT '
>  # Ensure that no test accidentally triggers a Git command
>  # that runs the actual maintenance scheduler, affecting a user's
>  # system permanently.
> -# Tests that verify the scheduler integration must set this locally
> -# to avoid errors.
> -GIT_TEST_MAINT_SCHEDULER="none:exit 1"
> +# Tests that verify the scheduler integration must set and
> +# export this variable locally.
> +sane_unset GIT_TEST_MAINT_SCHEDULER

Clearly the idea was to protect the scheduler-configuration of the
person running the test in the event that the test author forgot to
set GIT_TEST_MAINT_SCHEDULER to one of the legitimate "testing values"
before invoking a "destructive" command, such as `git maintenance
start`. By defaulting to `none:exit 1`, the problem would be caught
and reported before any damage could be done to the configuration of
the person running the tests.

So, I'm somewhat skeptical of the new direction of simply unsetting
GIT_TEST_MAINT_SCHEDULER since that outright removes the intended
protection. I'd have expected this problem to be addressed by
exporting GIT_TEST_MAINT_SCHEDULER, not by making it easier for an
absent-minded test author to break his or her own configuration.

Having said that, it you do want to go the route of eliminating the
(intended) protection altogether, I have a couple additional
observations:

First, this change requires a corresponding update to the lead-in
comment ("Ensure that no test accidentally triggers a Git command that
runs the actual maintenance scheduler, affecting a user's system
permanently.") since it renders that comment incorrect.

Second, it seems very unlikely that GIT_TEST_MAINT_SCHEDULER will be
set in the user's environment anyhow before running the tests, so
unsetting it here seems unnecessary and pointless. Instead, a cleaner
approach would be to simply remove the entire hunk in t/test-lib.sh
dealing with GIT_TEST_MAINT_SCHEDULER, including the comment.