threads / patch / 61223

patchdo not set GIT_TEST_MAINT_SCHEDULER where it does not matter

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

## tl;dr

5 messages between Mar 29, 2024 and Mar 31, 2024. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Junio C Hamano· Mar 29, 2024, 00:51 UTC · lore

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * t7900 (maintenance) uses many tests that does one-shot export of
   the variable, and one test that sets the value to its safe
   "failure" value and exports it at the end.

   t9210 (scaler) sets up a safe fake scheduler and exports it
   before doing any of its tests.

   Nobody else that includes t/test-lib.sh mentions this variable.

 t/test-lib.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Show changes to diff +3 −3

diff --git c/t/test-lib.sh w/t/test-lib.sh
index c8af8dab79..48345864f4 100644
--- 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
 
 # Does this platform support `git fsmonitor--daemon`
 #

Eric Sunshine· Mar 29, 2024, 02:43 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 28, 2024 at 8:51 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 9 quoted lines

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

Show 13 quoted lines

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

Junio C Hamano· Mar 29, 2024, 05:38 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 7 quoted lines

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

Yeah, if the variable were exported. So I am OK with a fix in the opposite direction.

Eric Sunshine· Mar 29, 2024, 22:27 UTC · re: Junio C Hamano · lore

[PATCH] test-lib: fix non-functioning GIT_TEST_MAINT_SCHEDULER fallback

From: Eric Sunshine <sunshine@sunshineco.com>

When environment variable GIT_TEST_MAINT_SCHEDULER is set, `git maintenance` invokes the command specified as the variable's value rather than invoking the actual underlying platform-specific scheduler management command. By setting GIT_TEST_MAINT_SCHEDULER to some suitable value, test authors can therefore validate behavior of "destructive" `git maintenance` commands without having to worry about clobbering the user's own local scheduler configuration.

In order to protect an absent-minded test author from forgetting to set GIT_TEST_MAINT_SCHEDULER in the local test script (and thus clobbering his or her own scheduler configuration), t/test-lib.sh assigns an "immediately error-out" value to GIT_TEST_MAINT_SCHEDULER by default which should ensure that the problem will be caught and reported before any damage can be done to the configuration of the person running the tests.

Unfortunately, however, t/test-lib.sh neglects to export GIT_TEST_MAINT_SCHEDULER, which renders the default "error-out" assignment worthles. Fix this by exporting the variable as originally intended.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-of-by: Eric Sunshine <sunshine@sunshineco.com>
---

This is a replacement for Junio's [1]. That attempt made it easier for a test author to shoot him or herself in the foot. This replacement patch instead fixes the foot-shooting guard.

[1]: https://lore.kernel.org/git/xmqqmsqhsvwk.fsf@gitster.g/

 t/test-lib.sh | 1 +
 1 file changed, 1 insertion(+)

Show changes to t/test-lib.sh +1 −0

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c8af8dab79..79d3e0e7d9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1962,6 +1962,7 @@ test_lazy_prereq DEFAULT_REPO_FORMAT '
 # Tests that verify the scheduler integration must set this locally
 # to avoid errors.
 GIT_TEST_MAINT_SCHEDULER="none:exit 1"
+export GIT_TEST_MAINT_SCHEDULER
 
 # Does this platform support `git fsmonitor--daemon`
 #

-- 
2.44.0

Eric Sunshine· Mar 31, 2024, 06:51 UTC · re: Eric Sunshine · lore

Re: [PATCH] test-lib: fix non-functioning GIT_TEST_MAINT_SCHEDULER fallback

On Fri, Mar 29, 2024 at 6:27 PM Eric Sunshine <ericsunshine@charter.net> wrote:

Show 20 quoted lines

> When environment variable GIT_TEST_MAINT_SCHEDULER is set, `git
> maintenance` invokes the command specified as the variable's value
> rather than invoking the actual underlying platform-specific scheduler
> management command. By setting GIT_TEST_MAINT_SCHEDULER to some suitable
> value, test authors can therefore validate behavior of "destructive"
> `git maintenance` commands without having to worry about clobbering the
> user's own local scheduler configuration.
>
> In order to protect an absent-minded test author from forgetting to set
> GIT_TEST_MAINT_SCHEDULER in the local test script (and thus clobbering
> his or her own scheduler configuration), t/test-lib.sh assigns an
> "immediately error-out" value to GIT_TEST_MAINT_SCHEDULER by default
> which should ensure that the problem will be caught and reported before
> any damage can be done to the configuration of the person running the
> tests.
>
> Unfortunately, however, t/test-lib.sh neglects to export
> GIT_TEST_MAINT_SCHEDULER, which renders the default "error-out"
> assignment worthles. Fix this by exporting the variable as originally
> intended.

s/worthles/worthless/

(I won't reroll just for this minor typo.)