{"thread":{"id":"59158","subject":"[PATCH 0/3] Allow scalar to succeed despite maintenance failures","startedAt":"2023-01-27T20:07:55Z","lastAt":"2023-01-30T19:26:23Z","messageCount":16,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Victoria Dye","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"471102","messageId":"pull.1473.git.1674849963.gitgitgadget@gmail.com","threadId":"59158","inReplyTo":null,"subject":"[PATCH 0/3] Allow scalar to succeed despite maintenance failures","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-27T20:06:00Z","receivedAt":"2023-01-27T20:07:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"At $DAYJOB we received a report of issues with scalar clone and scalar\nregister in a protected environment. Specifically, they couldn't run crontab\nor systemctl, which causes git maintenance start to fail. That failure was\npercolating through scalar clone and scalar register, even though the rest\nof their repository was properly set up.\n\nConvert these hard failures into soft warnings.\n\nThis change could probably be done in a single commit, but I wanted to\ndemonstrate the existing behavior before changing it. The first patch was\nalso a related necessary step that will be helpful for future scalar tests.\n\nThanks,\n\n * Stolee\n\nDerrick Stolee (3):\n  t: allow 'scalar' in test_must_fail\n  t921*: test scalar behavior starting maintenance\n  scalar: only warn when background maintenance fails\n\n scalar.c                | 2 +-\n t/t9210-scalar.sh       | 7 +++++++\n t/t9211-scalar-clone.sh | 6 ++++++\n t/test-lib-functions.sh | 2 +-\n 4 files changed, 15 insertions(+), 2 deletions(-)\n\n\nbase-commit: 5cc9858f1b470844dea5c5d3e936af183fdf2c68\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1473%2Fderrickstolee%2Fscalar-register-warn-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1473/derrickstolee/scalar-register-warn-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1473\n-- \ngitgitgadget\n"},{"id":"471103","messageId":"d75780e0567b5f765816ab7522afe550ebaa3521.1674849963.git.gitgitgadget@gmail.com","threadId":"59158","inReplyTo":"pull.1473.git.1674849963.gitgitgadget@gmail.com","subject":"[PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-27T20:06:03Z","receivedAt":"2023-01-27T20:07:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nA user reported issues with 'scalar clone' and 'scalar register' when\nworking in an environment that had locked down the ability to run\n'crontab' or 'systemctl' in that those commands registered as _failures_\ninstead of opportunistically reporting a success with just a warning\nabout background maintenance.\n\nAs a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a\nsuccessful background maintenance, but this is not a viable strategy for\nlong-term.\n\nUpdate 'scalar register' and 'scalar clone' to no longer fail by\nmodifying register_dir() to only warn when toggle_maintenance(1) fails.\n\nSince background maintenance is a \"nice to have\" and not a requirement\nfor a working repository, it is best to move this from hard error to\ngentle warning.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n scalar.c                | 2 +-\n t/t9210-scalar.sh       | 4 ++--\n t/t9211-scalar-clone.sh | 4 ++--\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex f25d5f1d0ef..ca19b95ce46 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -262,7 +262,7 @@ static int register_dir(void)\n \t\treturn error(_(\"could not set recommended config\"));\n \n \tif (toggle_maintenance(1))\n-\t\treturn error(_(\"could not turn on maintenance\"));\n+\t\twarning(_(\"could not turn on maintenance\"));\n \n \tif (have_fsmonitor_support() && start_fsmonitor_daemon()) {\n \t\treturn error(_(\"could not start the FSMonitor daemon\"));\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 13a4f6dbcf4..4432a30d10b 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '\n \ttest_cmp_config -C test/src true core.fsmonitor\n '\n \n-test_expect_success 'scalar register fails when background maintenance fails' '\n+test_expect_success 'scalar register warns when background maintenance fails' '\n \tgit init register-repo &&\n \tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n-\t\ttest_must_fail scalar register register-repo 2>err &&\n+\t\tscalar register register-repo 2>err &&\n \tgrep \"could not turn on maintenance\" err\n '\n \ndiff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\nindex a6156da29ac..872ad1c9c2b 100755\n--- a/t/t9211-scalar-clone.sh\n+++ b/t/t9211-scalar-clone.sh\n@@ -174,9 +174,9 @@ test_expect_success 'progress without tty' '\n \tcleanup_clone $enlistment\n '\n \n-test_expect_success 'scalar clone fails when background maintenance fails' '\n+test_expect_success 'scalar clone warns when background maintenance fails' '\n \tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n-\t\ttest_must_fail scalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n+\t\tscalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n \tgrep \"could not turn on maintenance\" err\n '\n \n-- \ngitgitgadget\n"},{"id":"471104","messageId":"f05325c862b618067c67e727c81dafdf4463e4fc.1674849963.git.gitgitgadget@gmail.com","threadId":"59158","inReplyTo":"pull.1473.git.1674849963.gitgitgadget@gmail.com","subject":"[PATCH 2/3] t921*: test scalar behavior starting maintenance","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-27T20:06:02Z","receivedAt":"2023-01-27T20:08:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nA user recently reported issues with 'scalar register' and 'scalar\nclone' in that they failed when the system had permissions locked down\nso both 'crontab' and 'systemctl' commands failed when trying to enable\nbackground maintenance.\n\nThis hard error is undesirable, but let's create tests that demonstrate\nthis behavior before modiying the behavior. We can use\nGIT_TEST_MAINT_SCHEDULER to guarantee failure and check the exit code\nand error message.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n t/t9210-scalar.sh       | 7 +++++++\n t/t9211-scalar-clone.sh | 6 ++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 25f500cf682..13a4f6dbcf4 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -104,6 +104,13 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '\n \ttest_cmp_config -C test/src true core.fsmonitor\n '\n \n+test_expect_success 'scalar register fails when background maintenance fails' '\n+\tgit init register-repo &&\n+\tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n+\t\ttest_must_fail scalar register register-repo 2>err &&\n+\tgrep \"could not turn on maintenance\" err\n+'\n+\n test_expect_success 'scalar unregister' '\n \tgit init vanish/src &&\n \tscalar register vanish/src &&\ndiff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\nindex 02d32fb6ede..a6156da29ac 100755\n--- a/t/t9211-scalar-clone.sh\n+++ b/t/t9211-scalar-clone.sh\n@@ -174,4 +174,10 @@ test_expect_success 'progress without tty' '\n \tcleanup_clone $enlistment\n '\n \n+test_expect_success 'scalar clone fails when background maintenance fails' '\n+\tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n+\t\ttest_must_fail scalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n+\tgrep \"could not turn on maintenance\" err\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"471105","messageId":"94630bc3bc7f4f236c28e791e0faba6590c735e8.1674849963.git.gitgitgadget@gmail.com","threadId":"59158","inReplyTo":"pull.1473.git.1674849963.gitgitgadget@gmail.com","subject":"[PATCH 1/3] t: allow 'scalar' in test_must_fail","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-27T20:06:01Z","receivedAt":"2023-01-27T20:08:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee <derrickstolee@github.com>\n\nThis will enable scalar tests to use the test_must_fail helper, when\nnecessary.\n\nSigned-off-by: Derrick Stolee <derrickstolee@github.com>\n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 79922227030..75b8ee95e7f 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1016,7 +1016,7 @@ test_must_fail_acceptable () {\n \tfi\n \n \tcase \"$1\" in\n-\tgit|__git*|test-tool|test_terminal)\n+\tgit|__git*|scalar|test-tool|test_terminal)\n \t\treturn 0\n \t\t;;\n \t*)\n-- \ngitgitgadget\n\n"},{"id":"471106","messageId":"xmqqleln90ka.fsf@gitster.g","threadId":"59158","inReplyTo":"d75780e0567b5f765816ab7522afe550ebaa3521.1674849963.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-27T20:36:53Z","receivedAt":"2023-01-27T20:36:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Derrick Stolee <derrickstolee@github.com>\n>\n> A user reported issues with 'scalar clone' and 'scalar register' when\n> working in an environment that had locked down the ability to run\n> 'crontab' or 'systemctl' in that those commands registered as _failures_\n> instead of opportunistically reporting a success with just a warning\n> about background maintenance.\n>\n> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a\n> successful background maintenance, but this is not a viable strategy for\n> long-term.\n>\n> Update 'scalar register' and 'scalar clone' to no longer fail by\n> modifying register_dir() to only warn when toggle_maintenance(1) fails.\n>\n> Since background maintenance is a \"nice to have\" and not a requirement\n> for a working repository, it is best to move this from hard error to\n> gentle warning.\n\nWasn't the main selling point of scalar that users do not have to\nworry about various minute details of configuration settings to\nmaintain their clone of projects on the larger side?  The \"maintain\ntheir clone\" certainly should include running periodic maintenance\ntasks without them having to worry about it.  It feels like this is\ncalling for an explicit \"disable periodic maintenance tasks in this\nrepository\" option to help these esoteric environments that disable\ncron-like system services, while keeping the default safer,\ni.e. fail loudly when the periodic maintenance tasks that the users\nexpect to happen cannot be enabled, or something.\n\nPerhaps I am not the primary audience, but hmph, I have a feeling\nthat this is not exactly going into a healthy direction.\n\nOther two steps that led to this step looked quite sensible, though.\n\nThanks.\n"},{"id":"471107","messageId":"65b1735b-7ea6-5f7c-e1d9-6c986c7beb1d@github.com","threadId":"59158","inReplyTo":"d75780e0567b5f765816ab7522afe550ebaa3521.1674849963.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-01-27T22:06:11Z","receivedAt":"2023-01-27T22:06:18Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee <derrickstolee@github.com>\n> \n> A user reported issues with 'scalar clone' and 'scalar register' when\n> working in an environment that had locked down the ability to run\n> 'crontab' or 'systemctl' in that those commands registered as _failures_\n> instead of opportunistically reporting a success with just a warning\n> about background maintenance.\n> \n> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a\n> successful background maintenance, but this is not a viable strategy for\n> long-term.\n> \n> Update 'scalar register' and 'scalar clone' to no longer fail by\n> modifying register_dir() to only warn when toggle_maintenance(1) fails.\n> \n> Since background maintenance is a \"nice to have\" and not a requirement\n> for a working repository, it is best to move this from hard error to\n> gentle warning.\n> \n> Signed-off-by: Derrick Stolee <derrickstolee@github.com>\n> ---\n>  scalar.c                | 2 +-\n>  t/t9210-scalar.sh       | 4 ++--\n>  t/t9211-scalar-clone.sh | 4 ++--\n>  3 files changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/scalar.c b/scalar.c\n> index f25d5f1d0ef..ca19b95ce46 100644\n> --- a/scalar.c\n> +++ b/scalar.c\n> @@ -262,7 +262,7 @@ static int register_dir(void)\n>  \t\treturn error(_(\"could not set recommended config\"));\n>  \n>  \tif (toggle_maintenance(1))\n> -\t\treturn error(_(\"could not turn on maintenance\"));\n> +\t\twarning(_(\"could not turn on maintenance\"));\n\nShould we do the same thing for 'unregister_dir()'? Unlike 'register_dir()',\nit doesn't break immediately (and finishes removing the enlistment), but it\nstill returns a nonzero error code from 'scalar unregister'.\n\n>  \n>  \tif (have_fsmonitor_support() && start_fsmonitor_daemon()) {\n>  \t\treturn error(_(\"could not start the FSMonitor daemon\"));\n> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\n> index 13a4f6dbcf4..4432a30d10b 100755\n> --- a/t/t9210-scalar.sh\n> +++ b/t/t9210-scalar.sh\n> @@ -104,10 +104,10 @@ test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '\n>  \ttest_cmp_config -C test/src true core.fsmonitor\n>  '\n>  \n> -test_expect_success 'scalar register fails when background maintenance fails' '\n> +test_expect_success 'scalar register warns when background maintenance fails' '\n>  \tgit init register-repo &&\n>  \tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n> -\t\ttest_must_fail scalar register register-repo 2>err &&\n> +\t\tscalar register register-repo 2>err &&\n>  \tgrep \"could not turn on maintenance\" err\n>  '\n>  \n> diff --git a/t/t9211-scalar-clone.sh b/t/t9211-scalar-clone.sh\n> index a6156da29ac..872ad1c9c2b 100755\n> --- a/t/t9211-scalar-clone.sh\n> +++ b/t/t9211-scalar-clone.sh\n> @@ -174,9 +174,9 @@ test_expect_success 'progress without tty' '\n>  \tcleanup_clone $enlistment\n>  '\n>  \n> -test_expect_success 'scalar clone fails when background maintenance fails' '\n> +test_expect_success 'scalar clone warns when background maintenance fails' '\n>  \tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n> -\t\ttest_must_fail scalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n> +\t\tscalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n>  \tgrep \"could not turn on maintenance\" err\n>  '\n\nSimilarly, it might be nice to show how 'scalar unregister' behaves when\nmaintenance fails in the tests.\n\n"},{"id":"471108","messageId":"7070bd26-3020-5a17-2f51-2df9ae00df8c@github.com","threadId":"59158","inReplyTo":"65b1735b-7ea6-5f7c-e1d9-6c986c7beb1d@github.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-27T22:14:43Z","receivedAt":"2023-01-27T22:14:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/27/2023 5:06 PM, Victoria Dye wrote:\n> Derrick Stolee via GitGitGadget wrote:\n>>  \tif (toggle_maintenance(1))\n>> -\t\treturn error(_(\"could not turn on maintenance\"));\n>> +\t\twarning(_(\"could not turn on maintenance\"));\n> \n> Should we do the same thing for 'unregister_dir()'? Unlike 'register_dir()',\n> it doesn't break immediately (and finishes removing the enlistment), but it\n> still returns a nonzero error code from 'scalar unregister'.\n\nThe interesting thing about unregister_dir() is that\ntoggle_maintenance(0) \"turns of maintenance\" by removing the\nmaintenance.repo config value pointing to this repository,\nnot by removing the maintenance schedule. Thus, we don't get\nthe failure in the same way.\n\n>> -test_expect_success 'scalar clone fails when background maintenance fails' '\n>> +test_expect_success 'scalar clone warns when background maintenance fails' '\n>>  \tGIT_TEST_MAINT_SCHEDULER=\"crontab:false,launchctl:false,schtasks:false\" \\\n>> -\t\ttest_must_fail scalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n>> +\t\tscalar clone \"file://$(pwd)/to-clone\" maint-fail 2>err &&\n>>  \tgrep \"could not turn on maintenance\" err\n>>  '\n> \n> Similarly, it might be nice to show how 'scalar unregister' behaves when\n> maintenance fails in the tests.\n \nAnd the way I found out was by making the same tests, but since\nthey actually never failed or listed a warning (for this reason)\nI left it out.\n\nThanks,\n-Stolee\n"},{"id":"471109","messageId":"4913381a-769f-aba0-c04d-559d103e8396@github.com","threadId":"59158","inReplyTo":"xmqqleln90ka.fsf@gitster.g","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-27T22:18:23Z","receivedAt":"2023-01-27T22:18:28Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/27/2023 3:36 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <derrickstolee@github.com>\n>>\n>> A user reported issues with 'scalar clone' and 'scalar register' when\n>> working in an environment that had locked down the ability to run\n>> 'crontab' or 'systemctl' in that those commands registered as _failures_\n>> instead of opportunistically reporting a success with just a warning\n>> about background maintenance.\n>>\n>> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a\n>> successful background maintenance, but this is not a viable strategy for\n>> long-term.\n>>\n>> Update 'scalar register' and 'scalar clone' to no longer fail by\n>> modifying register_dir() to only warn when toggle_maintenance(1) fails.\n>>\n>> Since background maintenance is a \"nice to have\" and not a requirement\n>> for a working repository, it is best to move this from hard error to\n>> gentle warning.\n> \n> Wasn't the main selling point of scalar that users do not have to\n> worry about various minute details of configuration settings to\n> maintain their clone of projects on the larger side?\n\nYes, and that includes things like partial clone, sparse-checkout,\nand FS Monitor which are independent of this maintenance change.\n\n>  The \"maintain\n> their clone\" certainly should include running periodic maintenance\n> tasks without them having to worry about it.  It feels like this is\n> calling for an explicit \"disable periodic maintenance tasks in this\n> repository\" option to help these esoteric environments that disable\n> cron-like system services, while keeping the default safer,\n> i.e. fail loudly when the periodic maintenance tasks that the users\n> expect to happen cannot be enabled, or something.\n> \n> Perhaps I am not the primary audience, but hmph, I have a feeling\n> that this is not exactly going into a healthy direction.\n\nHere, we are in an environment where background maintenance is\nunavailable in an unexpected way. If that feature is not available\nto the user, should they not get the benefits of the others?\n\nNot having background maintenance is definitely a downside, but it's\ndifferent from failing to connect to the server or being unable to\ncomplete setting up the working directory. The warning communicates\nthe issue, so users can realize the problem exists and work to resolve\nit in their own way.\n\nThanks,\n-Stolee\n"},{"id":"471110","messageId":"3ade6d9f-8477-40c2-d683-d629e863c6ab@github.com","threadId":"59158","inReplyTo":"xmqqleln90ka.fsf@gitster.g","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-01-27T22:18:35Z","receivedAt":"2023-01-27T22:18:44Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Derrick Stolee <derrickstolee@github.com>\n>>\n>> A user reported issues with 'scalar clone' and 'scalar register' when\n>> working in an environment that had locked down the ability to run\n>> 'crontab' or 'systemctl' in that those commands registered as _failures_\n>> instead of opportunistically reporting a success with just a warning\n>> about background maintenance.\n>>\n>> As a workaround, they can use GIT_TEST_MAINT_SCHEDULER to fake a\n>> successful background maintenance, but this is not a viable strategy for\n>> long-term.\n>>\n>> Update 'scalar register' and 'scalar clone' to no longer fail by\n>> modifying register_dir() to only warn when toggle_maintenance(1) fails.\n>>\n>> Since background maintenance is a \"nice to have\" and not a requirement\n>> for a working repository, it is best to move this from hard error to\n>> gentle warning.\n> \n> Wasn't the main selling point of scalar that users do not have to\n> worry about various minute details of configuration settings to\n> maintain their clone of projects on the larger side?  The \"maintain\n> their clone\" certainly should include running periodic maintenance\n> tasks without them having to worry about it.  It feels like this is\n> calling for an explicit \"disable periodic maintenance tasks in this\n> repository\" option to help these esoteric environments that disable\n> cron-like system services, while keeping the default safer,\n> i.e. fail loudly when the periodic maintenance tasks that the users\n> expect to happen cannot be enabled, or something.\n\nI see Stolee's approach as more in line with how FSMonitor is treated by\n'scalar', which is \"only turn it on if it's supported, but otherwise do\nnothing\" (the main difference here being that a warning is displayed if\nmaintenance can't be turned on). That still fits the stated goal of 'scalar'\n(\"configure all the niche large-repo settings for me when I\nclone/register\"), but it makes 'scalar' more forgiving of system\nconfigurations that don't support maintenance.\n\nThat said, this doesn't distinguish between \"maintenance couldn't be turned\non because the system doesn't support it\" and \"maintenance couldn't be\nturned on because of an internal error\". The latter might still be worth\nfailing on, so maybe something like this would be more palatable?\n\n--------8<--------8<--------8<--------\ndiff --git a/scalar.c b/scalar.c\nindex 6c52243cdf1..138780abe5f 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -252,6 +252,10 @@ static int stop_fsmonitor_daemon(void)\n \treturn 0;\n }\n \n+static int have_maintenance_support(void) {\n+\t/* check whether at least one scheduler is supported on the system */\n+}\n+\n static int register_dir(void)\n {\n \tif (add_or_remove_enlistment(1))\n@@ -260,7 +264,7 @@ static int register_dir(void)\n \tif (set_recommended_config(0))\n \t\treturn error(_(\"could not set recommended config\"));\n \n-\tif (toggle_maintenance(1))\n+\tif (have_maintenance_support() && toggle_maintenance(1))\n \t\treturn error(_(\"could not turn on maintenance\"));\n \n \tif (have_fsmonitor_support() && start_fsmonitor_daemon()) {\n\n-------->8-------->8-------->8--------\n\n> \n> Perhaps I am not the primary audience, but hmph, I have a feeling\n> that this is not exactly going into a healthy direction.\n\nI don't think this change would be the start of a larger pattern, since most\nScalar-related settings aren't system-dependent (only FSMonitor and\nmaintenance are, AFAIK). What would be worse, I think, is setting the\nmaintenance behavior with a new command option - I could very easily see\n*that* leading to a slippery slope of endless options to toggle 'scalar's\nbehaviors, completely defeating it's whole \"set everything up for me\"\npurpose.\n\n> \n> Other two steps that led to this step looked quite sensible, though.\n> \n> Thanks.\n\n"},{"id":"471111","messageId":"xmqq357v8poc.fsf@gitster.g","threadId":"59158","inReplyTo":"4913381a-769f-aba0-c04d-559d103e8396@github.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-28T00:32:03Z","receivedAt":"2023-01-28T00:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>>  The \"maintain\n>> their clone\" certainly should include running periodic maintenance\n>> tasks without them having to worry about it.  It feels like this is\n>> calling for an explicit \"disable periodic maintenance tasks in this\n>> repository\" option to help these esoteric environments that disable\n>> cron-like system services, while keeping the default safer,\n>> i.e. fail loudly when the periodic maintenance tasks that the users\n>> expect to happen cannot be enabled, or something.\n>> \n>> Perhaps I am not the primary audience, but hmph, I have a feeling\n>> that this is not exactly going into a healthy direction.\n>\n> Here, we are in an environment where background maintenance is\n> unavailable in an unexpected way. If that feature is not available\n> to the user, should they not get the benefits of the others?\n\nThat is not what I was saying.  I just have expected to see a way\nfor the user to give scalar an explicit \"I understand that periodic\nmaintenance does not happen in this repository\" consent, instead of\ndemoting an error detection for everybody to a warning that users\nwill just ignore.\n"},{"id":"471139","messageId":"b63611dc-a889-8900-403a-ec7c42a89705@github.com","threadId":"59158","inReplyTo":"xmqq357v8poc.fsf@gitster.g","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-30T13:44:23Z","receivedAt":"2023-01-30T13:44:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/27/2023 7:32 PM, Junio C Hamano wrote:\n> Derrick Stolee <derrickstolee@github.com> writes:\n> \n>>>  The \"maintain\n>>> their clone\" certainly should include running periodic maintenance\n>>> tasks without them having to worry about it.  It feels like this is\n>>> calling for an explicit \"disable periodic maintenance tasks in this\n>>> repository\" option to help these esoteric environments that disable\n>>> cron-like system services, while keeping the default safer,\n>>> i.e. fail loudly when the periodic maintenance tasks that the users\n>>> expect to happen cannot be enabled, or something.\n>>>\n>>> Perhaps I am not the primary audience, but hmph, I have a feeling\n>>> that this is not exactly going into a healthy direction.\n>>\n>> Here, we are in an environment where background maintenance is\n>> unavailable in an unexpected way. If that feature is not available\n>> to the user, should they not get the benefits of the others?\n> \n> That is not what I was saying.  I just have expected to see a way\n> for the user to give scalar an explicit \"I understand that periodic\n> maintenance does not happen in this repository\" consent, instead of\n> demoting an error detection for everybody to a warning that users\n> will just ignore.\n\nAh, so you'd prefer a --no-maintenance option for users who have\nthis problem instead of just a warning. I'll do that in v2.\n\nThis could be a good time for me to upstream the --no-src option\nwhile I'm messing with arguments in 'scalar clone'.\n\nThanks,\n-Stolee\n"},{"id":"471141","messageId":"xmqqk01458um.fsf@gitster.g","threadId":"59158","inReplyTo":"b63611dc-a889-8900-403a-ec7c42a89705@github.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T15:40:33Z","receivedAt":"2023-01-30T15:40:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <derrickstolee@github.com> writes:\n\n>>> Here, we are in an environment where background maintenance is\n>>> unavailable in an unexpected way. If that feature is not available\n>>> to the user, should they not get the benefits of the others?\n>> \n>> That is not what I was saying.  I just have expected to see a way\n>> for the user to give scalar an explicit \"I understand that periodic\n>> maintenance does not happen in this repository\" consent, instead of\n>> demoting an error detection for everybody to a warning that users\n>> will just ignore.\n>\n> Ah, so you'd prefer a --no-maintenance option for users who have\n> this problem instead of just a warning. I'll do that in v2.\n\nOr a repository-local configuration to declare \"no need to do the\nmaintenance stuff here\", probably?  The expected use case you gave\ndoes not match per-invocation command line option very well, right?\n\n> This could be a good time for me to upstream the --no-src option\n> while I'm messing with arguments in 'scalar clone'.\n\nOK.  Thanks.\n\n"},{"id":"471142","messageId":"db04e31d-681f-5809-f51a-37b9c734b45a@github.com","threadId":"59158","inReplyTo":"b63611dc-a889-8900-403a-ec7c42a89705@github.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-01-30T17:42:55Z","receivedAt":"2023-01-30T17:43:01Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 1/27/2023 7:32 PM, Junio C Hamano wrote:\n>> Derrick Stolee <derrickstolee@github.com> writes:\n>>\n>>>>  The \"maintain\n>>>> their clone\" certainly should include running periodic maintenance\n>>>> tasks without them having to worry about it.  It feels like this is\n>>>> calling for an explicit \"disable periodic maintenance tasks in this\n>>>> repository\" option to help these esoteric environments that disable\n>>>> cron-like system services, while keeping the default safer,\n>>>> i.e. fail loudly when the periodic maintenance tasks that the users\n>>>> expect to happen cannot be enabled, or something.\n>>>>\n>>>> Perhaps I am not the primary audience, but hmph, I have a feeling\n>>>> that this is not exactly going into a healthy direction.\n>>>\n>>> Here, we are in an environment where background maintenance is\n>>> unavailable in an unexpected way. If that feature is not available\n>>> to the user, should they not get the benefits of the others?\n>>\n>> That is not what I was saying.  I just have expected to see a way\n>> for the user to give scalar an explicit \"I understand that periodic\n>> maintenance does not happen in this repository\" consent, instead of\n>> demoting an error detection for everybody to a warning that users\n>> will just ignore.\n> \n> Ah, so you'd prefer a --no-maintenance option for users who have\n> this problem instead of just a warning. I'll do that in v2.\n\nI mentioned this earlier [1], but I want to reiterate that I really don't\nthink a dedicated '--no-maintenance' option is a good approach to this\nproblem. I understand wanting more active user acknowledgement that \"I\nunderstand that periodic maintenance does not happen in this repository\";\nwithout that, users may (rightfully) be confused when they find their\nscalar-cloned repository full of loose objects. But, in the use case you've\npresented (where no scheduler is available), the user would need to -\nsomewhat redundantly, I feel - acknowledge that for *every* repository they\nclone. \n\nI'm also still worried about cluttering scalar's UX with options that toggle\nuse of its internally-configured options and features. One of the big\nselling points for including scalar in the upstream project ([2], [3]) was\nits ability to \"intelligently\" configure all of the settings a user would\nneed to optimize a large repository *without* a user needing to know what\nany of those options are/what they mean. These settings are inherently\nsubject to change (due to use of experimental features); exposing a feature\ntoggle entrenches that setting permanently within scalar and makes a user\naware of implementation details that were intended to be hidden. At a high\nlevel, it pushes scalar towards simply being an \"opinionated\" 'git\nconfig'-configurator, which was a model I explicitly tried to move away from\nwhile upstreaming last year.  \n\nI still believe treating maintenance like FSMonitor - pre-determining\nwhether the feature is available and only enabling it if possible - is the\nmost consistent and user-friendly solution to the given problem within the\ncontext of scalar. But, if you feel that user acknowledgement is absolutely\ncritical, I'd strongly prefer a config setting like 'maintenance.enabled';\nit could be set globally (the appropriate scope in the case of a system that\nhas no scheduler), or with '-c' with Scalar clone if it really needs to be\nper-repo.\n\n[1] https://lore.kernel.org/git/3ade6d9f-8477-40c2-d683-d629e863c6ab@github.com/\n[2] https://lore.kernel.org/git/pull.1005.git.1630359290.gitgitgadget@gmail.com/\n[3] https://lore.kernel.org/git/pull.1275.git.1656521925.gitgitgadget@gmail.com/\n\n> \n> This could be a good time for me to upstream the --no-src option\n> while I'm messing with arguments in 'scalar clone'.\n\nFor what it's worth, my concerns about option clutter don't really apply to\n'--no-src' (cloning directly into a given directory, rather than\n'<directory>/src'). Unlike features like FSMonitor and maintenance, the\n'src/' directory is a user-facing Scalar design decision. It's also\nsomething that seems to exist primarily for backward-compatibility reasons\n(if I'm interpreting your earlier comments [4] correctly). This could be a\nstep on a deprecation path to make '--no-src' the default and remove the\nlegacy enlistment structure? At the very least, it's sufficiently outside\nscalar's \"configure for a large repo\" scope for me to not worry about it\nsetting a bad precedent.\n\n[4] https://lore.kernel.org/git/82716e5b-3522-68f5-7479-1b39811e0cb2@github.com/\n\n> \n> Thanks,\n> -Stolee\n\n"},{"id":"471147","messageId":"xmqqy1pj3l3k.fsf@gitster.g","threadId":"59158","inReplyTo":"db04e31d-681f-5809-f51a-37b9c734b45a@github.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-30T18:58:55Z","receivedAt":"2023-01-30T18:58:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye <vdye@github.com> writes:\n\n> I'm also still worried about cluttering scalar's UX with options that toggle\n> use of its internally-configured options and features. One of the big\n> selling points for including scalar in the upstream project ([2], [3]) was\n> its ability to \"intelligently\" configure all of the settings a user would\n> need to optimize a large repository *without* a user needing to know what\n> any of those options are/what they mean. These settings are inherently\n> subject to change (due to use of experimental features); exposing a feature\n> toggle entrenches that setting permanently within scalar and makes a user\n> aware of implementation details that were intended to be hidden. At a high\n> level, it pushes scalar towards simply being an \"opinionated\" 'git\n> config'-configurator, which was a model I explicitly tried to move away from\n> while upstreaming last year.  \n\nI personally do not think \"opinionated configurator\" is a bad model\nat all.  And \"this does not seem to work here, so let's silently\ndisable it, as the user does not want to hear about minute details\"\nis a valid opinion to have for such a tool.\n\nI too share the aversion to command line option for this one.\nDisabled periodic task support is most likely system-wide, and\npassing --no-whatever every time you touch a new repository on the\nsame system does not make much sense.\n\nThanks.\n"},{"id":"471149","messageId":"35796b79-dfa8-92c3-33e1-3313da10975a@github.com","threadId":"59158","inReplyTo":"xmqqy1pj3l3k.fsf@gitster.g","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-30T19:06:31Z","receivedAt":"2023-01-30T19:06:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/30/2023 1:58 PM, Junio C Hamano wrote:\n> Victoria Dye <vdye@github.com> writes:\n> \n>> I'm also still worried about cluttering scalar's UX with options that toggle\n>> use of its internally-configured options and features. One of the big\n>> selling points for including scalar in the upstream project ([2], [3]) was\n>> its ability to \"intelligently\" configure all of the settings a user would\n>> need to optimize a large repository *without* a user needing to know what\n>> any of those options are/what they mean. These settings are inherently\n>> subject to change (due to use of experimental features); exposing a feature\n>> toggle entrenches that setting permanently within scalar and makes a user\n>> aware of implementation details that were intended to be hidden. At a high\n>> level, it pushes scalar towards simply being an \"opinionated\" 'git\n>> config'-configurator, which was a model I explicitly tried to move away from\n>> while upstreaming last year.  \n> \n> I personally do not think \"opinionated configurator\" is a bad model\n> at all.  And \"this does not seem to work here, so let's silently\n> disable it, as the user does not want to hear about minute details\"\n> is a valid opinion to have for such a tool.\n> \n> I too share the aversion to command line option for this one.\n> Disabled periodic task support is most likely system-wide, and\n> passing --no-whatever every time you touch a new repository on the\n> same system does not make much sense.\n\nThanks, both. v2 will include --no-src, but not --no-maintenance.\n\n-Stolee\n"},{"id":"471151","messageId":"1958e192-3ffe-ac10-4097-6da5eba02f4f@github.com","threadId":"59158","inReplyTo":"3ade6d9f-8477-40c2-d683-d629e863c6ab@github.com","subject":"Re: [PATCH 3/3] scalar: only warn when background maintenance fails","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-30T19:25:55Z","receivedAt":"2023-01-30T19:26:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/27/2023 5:18 PM, Victoria Dye wrote:\n> Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\nThis reply almost got lost in the shuffle, but mostly because it\nwasn't super-relevant if we were going with the --no-maintenance\noption. It's relevant again, so I wanted to point something out.\n\n> I see Stolee's approach as more in line with how FSMonitor is treated by\n> 'scalar', which is \"only turn it on if it's supported, but otherwise do\n> nothing\" (the main difference here being that a warning is displayed if\n> maintenance can't be turned on). That still fits the stated goal of 'scalar'\n> (\"configure all the niche large-repo settings for me when I\n> clone/register\"), but it makes 'scalar' more forgiving of system\n> configurations that don't support maintenance.\n> \n> That said, this doesn't distinguish between \"maintenance couldn't be turned\n> on because the system doesn't support it\" and \"maintenance couldn't be\n> turned on because of an internal error\". The latter might still be worth\n> failing on, so maybe something like this would be more palatable?\n> \n> --------8<--------8<--------8<--------\n> diff --git a/scalar.c b/scalar.c\n> index 6c52243cdf1..138780abe5f 100644\n> --- a/scalar.c\n> +++ b/scalar.c\n> @@ -252,6 +252,10 @@ static int stop_fsmonitor_daemon(void)\n>  \treturn 0;\n>  }\n>  \n> +static int have_maintenance_support(void) {\n> +\t/* check whether at least one scheduler is supported on the system */\n> +}\n> +\n>  static int register_dir(void)\n>  {\n>  \tif (add_or_remove_enlistment(1))\n> @@ -260,7 +264,7 @@ static int register_dir(void)\n>  \tif (set_recommended_config(0))\n>  \t\treturn error(_(\"could not set recommended config\"));\n>  \n> -\tif (toggle_maintenance(1))\n> +\tif (have_maintenance_support() && toggle_maintenance(1))\n>  \t\treturn error(_(\"could not turn on maintenance\"));\n>  \n>  \tif (have_fsmonitor_support() && start_fsmonitor_daemon()) {\n\nThe tricky thing about this is that have_fsmonitor_support() is\nsomething we can detect at compile time, while have_maintenance_support()\nwould not (unless we start building for a new platform).\n\nThe case that brought this up is a platform that has both 'crontab'\nand 'systemctl' on the PATH, but when executing those commands an\nerror occurs due to permissions.\n\nSo, if we wanted to distinguish between permissions issues and/or\nother unrelated failures, we would need to be able to parse the\noutput of those commands. That seems fraught with potential errors,\nso it seems like we should _attempt_ to enable maintenance and warn\nwith whatever failure is presented.\n\nThe only thing I could think is that we could define a custom exit\ncode within 'git maintenance start' that means \"we were able to\nstart the scheduler process, but it failed\" to differentiate from\nother kinds of failures, such as failing to write to global config.\n\nThanks,\n-Stolee\n"}]}