{"thread":{"id":"61223","subject":"[PATCH] do not set GIT_TEST_MAINT_SCHEDULER where it does not matter","startedAt":"2024-03-29T00:51:10Z","lastAt":"2024-03-31T06:51:28Z","messageCount":5,"participants":["Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491797","messageId":"xmqqmsqhsvwk.fsf@gitster.g","threadId":"61223","inReplyTo":null,"subject":"[PATCH] do not set GIT_TEST_MAINT_SCHEDULER where it does not matter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T00:51:07Z","receivedAt":"2024-03-29T00:51:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"31345d55 (maintenance: extract platform-specific scheduling,\n2020-11-24) added code to t/test-lib.sh for everybody to set\nGIT_TEST_MAINT_SCHEDULER to a \"safe\" value and instructed the test\nwriters to set the variable locally when their test wants to check\nthe scheduler integration.\n\nBut it did so without \"export GIT_TEST_MAINT_SCHEDULER\", so the\nsetting does not seem to have any effect anyway. Instead of setting\nit to a \"safe\" value, just unset it.\n\nSigned-off-by: Junio C Hamano \n---\n\n * t7900 (maintenance) uses many tests that does one-shot export of\n the variable, and one test that sets the value to its safe\n \"failure\" value and exports it at the end.\n\n t9210 (scaler) sets up a safe fake scheduler and exports it\n before doing any of its tests.\n\n Nobody else that includes t/test-lib.sh mentions this variable.\n\n t/test-lib.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git c/t/test-lib.sh w/t/test-lib.sh\nindex c8af8dab79..48345864f4 100644\n--- c/t/test-lib.sh\n+++ w/t/test-lib.sh\n@@ -1959,9 +1959,9 @@ test_lazy_prereq DEFAULT_REPO_FORMAT '\n # Ensure that no test accidentally triggers a Git command\n # that runs the actual maintenance scheduler, affecting a user's\n # system permanently.\n-# Tests that verify the scheduler integration must set this locally\n-# to avoid errors.\n-GIT_TEST_MAINT_SCHEDULER=\"none:exit 1\"\n+# Tests that verify the scheduler integration must set and\n+# export this variable locally.\n+sane_unset GIT_TEST_MAINT_SCHEDULER\n \n # Does this platform support `git fsmonitor--daemon`\n #\n"},{"id":"491799","messageId":"CAPig+cStHRX-wZKwdcO33wCjd4UU3MO-rVisyOFZ1vPbGaN51Q@mail.gmail.com","threadId":"61223","inReplyTo":"xmqqmsqhsvwk.fsf@gitster.g","subject":"Re: [PATCH] do not set GIT_TEST_MAINT_SCHEDULER where it does not matter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-29T02:43:48Z","receivedAt":"2024-03-29T02:44:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 28, 2024 at 8:51 PM Junio C Hamano wrote:\n> 31345d55 (maintenance: extract platform-specific scheduling,\n> 2020-11-24) added code to t/test-lib.sh for everybody to set\n> GIT_TEST_MAINT_SCHEDULER to a \"safe\" value and instructed the test\n> writers to set the variable locally when their test wants to check\n> the scheduler integration.\n>\n> But it did so without \"export GIT_TEST_MAINT_SCHEDULER\", so the\n> setting does not seem to have any effect anyway. Instead of setting\n> it to a \"safe\" value, just unset it.\n\nI agree that the missing `export` makes this a do-nothing assignment.\nIn fact, that problem traces back to the original 2fec604f8d\n(maintenance: add start/stop subcommands, 2020-09-11).\n\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git c/t/test-lib.sh w/t/test-lib.sh\n> @@ -1959,9 +1959,9 @@ test_lazy_prereq DEFAULT_REPO_FORMAT '\n> # Ensure that no test accidentally triggers a Git command\n> # that runs the actual maintenance scheduler, affecting a user's\n> # system permanently.\n> -# Tests that verify the scheduler integration must set this locally\n> -# to avoid errors.\n> -GIT_TEST_MAINT_SCHEDULER=\"none:exit 1\"\n> +# Tests that verify the scheduler integration must set and\n> +# export this variable locally.\n> +sane_unset GIT_TEST_MAINT_SCHEDULER\n\nClearly the idea was to protect the scheduler-configuration of the\nperson running the test in the event that the test author forgot to\nset GIT_TEST_MAINT_SCHEDULER to one of the legitimate \"testing values\"\nbefore invoking a \"destructive\" command, such as `git maintenance\nstart`. By defaulting to `none:exit 1`, the problem would be caught\nand reported before any damage could be done to the configuration of\nthe person running the tests.\n\nSo, I'm somewhat skeptical of the new direction of simply unsetting\nGIT_TEST_MAINT_SCHEDULER since that outright removes the intended\nprotection. I'd have expected this problem to be addressed by\nexporting GIT_TEST_MAINT_SCHEDULER, not by making it easier for an\nabsent-minded test author to break his or her own configuration.\n\nHaving said that, it you do want to go the route of eliminating the\n(intended) protection altogether, I have a couple additional\nobservations:\n\nFirst, this change requires a corresponding update to the lead-in\ncomment (\"Ensure that no test accidentally triggers a Git command that\nruns the actual maintenance scheduler, affecting a user's system\npermanently.\") since it renders that comment incorrect.\n\nSecond, it seems very unlikely that GIT_TEST_MAINT_SCHEDULER will be\nset in the user's environment anyhow before running the tests, so\nunsetting it here seems unnecessary and pointless. Instead, a cleaner\napproach would be to simply remove the entire hunk in t/test-lib.sh\ndealing with GIT_TEST_MAINT_SCHEDULER, including the comment.\n"},{"id":"491814","messageId":"xmqqsf09vbqf.fsf@gitster.g","threadId":"61223","inReplyTo":"CAPig+cStHRX-wZKwdcO33wCjd4UU3MO-rVisyOFZ1vPbGaN51Q@mail.gmail.com","subject":"Re: [PATCH] do not set GIT_TEST_MAINT_SCHEDULER where it does not matter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T05:38:32Z","receivedAt":"2024-03-29T05:38:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Clearly the idea was to protect the scheduler-configuration of the\n> person running the test in the event that the test author forgot to\n> set GIT_TEST_MAINT_SCHEDULER to one of the legitimate \"testing values\"\n> before invoking a \"destructive\" command, such as `git maintenance\n> start`. By defaulting to `none:exit 1`, the problem would be caught\n> and reported before any damage could be done to the configuration of\n> the person running the tests.\n\nYeah, if the variable were exported. So I am OK with a fix in the\nopposite direction.\n\n"},{"id":"491863","messageId":"20240329222703.9343-1-ericsunshine@charter.net","threadId":"61223","inReplyTo":"xmqqmsqhsvwk.fsf@gitster.g","subject":"[PATCH] test-lib: fix non-functioning GIT_TEST_MAINT_SCHEDULER fallback","fromName":"Eric Sunshine","fromEmail":"ericsunshine@charter.net","sentAt":"2024-03-29T22:27:03Z","receivedAt":"2024-03-29T22:29:28Z","isPatch":true,"sender":{"key":"ericsunshine@charter.net","avatar":null},"body":"From: Eric Sunshine \n\nWhen environment variable GIT_TEST_MAINT_SCHEDULER is set, `git\nmaintenance` invokes the command specified as the variable's value\nrather than invoking the actual underlying platform-specific scheduler\nmanagement command. By setting GIT_TEST_MAINT_SCHEDULER to some suitable\nvalue, test authors can therefore validate behavior of \"destructive\"\n`git maintenance` commands without having to worry about clobbering the\nuser's own local scheduler configuration.\n\nIn order to protect an absent-minded test author from forgetting to set\nGIT_TEST_MAINT_SCHEDULER in the local test script (and thus clobbering\nhis or her own scheduler configuration), t/test-lib.sh assigns an\n\"immediately error-out\" value to GIT_TEST_MAINT_SCHEDULER by default\nwhich should ensure that the problem will be caught and reported before\nany damage can be done to the configuration of the person running the\ntests.\n\nUnfortunately, however, t/test-lib.sh neglects to export\nGIT_TEST_MAINT_SCHEDULER, which renders the default \"error-out\"\nassignment worthles. Fix this by exporting the variable as originally\nintended.\n\nReported-by: Junio C Hamano \nSigned-of-by: Eric Sunshine \n---\n\nThis is a replacement for Junio's [1]. That attempt made it easier for a\ntest author to shoot him or herself in the foot. This replacement patch\ninstead fixes the foot-shooting guard.\n\n[1]: https://lore.kernel.org/git/xmqqmsqhsvwk.fsf@gitster.g/\n\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c8af8dab79..79d3e0e7d9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1962,6 +1962,7 @@ test_lazy_prereq DEFAULT_REPO_FORMAT '\n # Tests that verify the scheduler integration must set this locally\n # to avoid errors.\n GIT_TEST_MAINT_SCHEDULER=\"none:exit 1\"\n+export GIT_TEST_MAINT_SCHEDULER\n \n # Does this platform support `git fsmonitor--daemon`\n #\n-- \n2.44.0\n\n"},{"id":"491918","messageId":"CAPig+cQApMC_UEgee06e=jbu9VoNHQT10hCU1OAQtpn1W7Fqmw@mail.gmail.com","threadId":"61223","inReplyTo":"20240329222703.9343-1-ericsunshine@charter.net","subject":"Re: [PATCH] test-lib: fix non-functioning GIT_TEST_MAINT_SCHEDULER fallback","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-31T06:51:16Z","receivedAt":"2024-03-31T06:51:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 29, 2024 at 6:27 PM Eric Sunshine wrote:\n> When environment variable GIT_TEST_MAINT_SCHEDULER is set, `git\n> maintenance` invokes the command specified as the variable's value\n> rather than invoking the actual underlying platform-specific scheduler\n> management command. By setting GIT_TEST_MAINT_SCHEDULER to some suitable\n> value, test authors can therefore validate behavior of \"destructive\"\n> `git maintenance` commands without having to worry about clobbering the\n> user's own local scheduler configuration.\n>\n> In order to protect an absent-minded test author from forgetting to set\n> GIT_TEST_MAINT_SCHEDULER in the local test script (and thus clobbering\n> his or her own scheduler configuration), t/test-lib.sh assigns an\n> \"immediately error-out\" value to GIT_TEST_MAINT_SCHEDULER by default\n> which should ensure that the problem will be caught and reported before\n> any damage can be done to the configuration of the person running the\n> tests.\n>\n> Unfortunately, however, t/test-lib.sh neglects to export\n> GIT_TEST_MAINT_SCHEDULER, which renders the default \"error-out\"\n> assignment worthles. Fix this by exporting the variable as originally\n> intended.\n\ns/worthles/worthless/\n\n(I won't reroll just for this minor typo.)\n"}]}