{"thread":{"id":"58460","subject":"[PATCH] maintenance: make unregister idempotent","startedAt":"2022-09-20T01:02:18Z","lastAt":"2022-09-27T16:55:27Z","messageCount":34,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Derrick Stolee","SZEDER Gábor","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463251","messageId":"pull.1358.git.1663635732095.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":null,"subject":"[PATCH] maintenance: make unregister idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-20T01:02:11Z","receivedAt":"2022-09-20T01:02:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance unregister' subcommand has a step that removes the\ncurrent repository from the multi-valued maitenance.repo config key.\nThis fails if the repository is not listed in that key. This makes\nrunning 'git maintenance unregister' twice result in a failure in the\nsecond instance.\n\nMake this task idempotent, since the end result is the same in both\ncases: maintenance will no longer run on this repository.\n\nSigned-off-by: Derrick Stolee \n---\n maintenance: make unregister idempotent\n \n I noticed this while we were updating the microsoft/git fork to include\n v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent\n before, but instead some change in 'scalar unregister' led to it relying\n on the return code of 'git maintenance unregister'. Our functional tests\n expect 'scalar unregister' to be idempotent, and I think that's a good\n pattern for 'git maintenance unregister', so I'm fixing it at that\n layer.\n \n Despite finding this during the 2.38.0-rc0 integration, this isn't\n critical to the release.\n \n Perhaps an argument could be made that \"failure means it wasn't\n registered before\", but I think that isn't terribly helpful.\n \n Our functional tests are running the unregister subcommand to disable\n maintenance in order to run tests on the object store (such as running\n maintenance commands in the foreground and checking the object store\n afterwards). This is a form of automation using 'unregister' as a check\n that maintenance will not run at the same time, and it doesn't care if\n maintenance was already disabled. I can imagine other scripting\n scenarios wanting that kind of guarantee.\n \n Thanks, -Stolee\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1358\n\n builtin/gc.c | 24 +++++++++++++++++++-----\n t/t7900-maintenance.sh | 5 ++++-\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 0accc024067..787e9c702b2 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1528,9 +1528,13 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \tstruct option options[] = {\n \t\tOPT_END(),\n \t};\n-\tint rc;\n+\tconst char *key = \"maintenance.repo\";\n+\tint rc = 0;\n \tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n+\tint found = 0;\n+\tstruct string_list_item *item;\n+\tconst struct string_list *list = git_config_get_value_multi(key);\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_unregister_usage, 0);\n@@ -1538,11 +1542,21 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tusage_with_options(builtin_maintenance_unregister_usage,\n \t\t\t\t options);\n \n-\tconfig_unset.git_cmd = 1;\n-\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n+\tfor_each_string_list_item(item, list) {\n+\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\tfound = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (found) {\n+\t\tconfig_unset.git_cmd = 1;\n+\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n+\t\t\t \"--fixed-value\", key, maintpath, NULL);\n+\n+\t\trc = run_command(&config_unset);\n+\t}\n \n-\trc = run_command(&config_unset);\n \tfree(maintpath);\n \treturn rc;\n }\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 2724a44fe3e..3747e4a14f8 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -493,7 +493,10 @@ test_expect_success 'register and unregister' '\n \n \tgit maintenance unregister &&\n \tgit config --global --get-all maintenance.repo >actual &&\n-\ttest_cmp before actual\n+\ttest_cmp before actual &&\n+\n+\t# Expect unregister to be idempotent.\n+\tgit maintenance unregister\n '\n \n test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n\nbase-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df\n-- \ngitgitgadget\n"},{"id":"463377","messageId":"xmqqpmfo4pc7.fsf@gitster.g","threadId":"58460","inReplyTo":"pull.1358.git.1663635732095.gitgitgadget@gmail.com","subject":"Re: [PATCH] maintenance: make unregister idempotent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-21T17:19:52Z","receivedAt":"2022-09-21T17:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The 'git maintenance unregister' subcommand has a step that removes the\n> current repository from the multi-valued maitenance.repo config key.\n> This fails if the repository is not listed in that key. This makes\n> running 'git maintenance unregister' twice result in a failure in the\n> second instance.\n>\n> Make this task idempotent, since the end result is the same in both\n> cases: maintenance will no longer run on this repository.\n\nI am not sure if this is a good idea. What is the ultimate reason\nwhy we want to allow running it blindly without knowing if it is\nnecessary? Is it because there is no easy way to tell if unregister\nis needed in the first place?\n\nIf this were \"unregister X\" that takes a parameter that tells what\nto unregister, I would be vehemently opposed to the idea, because\nI'd expect people make typos and want to be reminded of them when\nthey happen, but ...\n\n> \tgit maintenance unregister &&\n> \tgit config --global --get-all maintenance.repo >actual &&\n> -\ttest_cmp before actual\n> +\ttest_cmp before actual &&\n> +\n> +\t# Expect unregister to be idempotent.\n> +\tgit maintenance unregister\n> '\n\n... given that the user does not have any control over what to\nunregister from the command line (it is decided implicitly by where\nthe user is), I am halfway OK with it.\n\nA user with two repositories may still be upset after running\n\"unregister\" in the one they did not mean to, and not getting told\nabout the mistake, though, e.g.\n\n $ ls *.git\n a.git b.git\n $ cd a.git\n $ git maintenance unregister\n $ cd b.git\n ... this would give you an error message ...\n $ git maintenance unregister\n ... this is silent with the change ...\n\nSurely, the second \"cd\" should be to ../b.git instead of b.git and\nsurely the user should have noticed that \"cd\" gave them an error,\nbut the unregister that complains would be an extra chance for them\nto notice the mistake when they wanted to unregister the maintenance\ntasks from all their repositories.\n\nI wonder if something like the \"--force\" option, i.e.\n\n $ >cruft\n $ rm curft; echo $?\n rm: cannot remove 'curft': No such file or directory\n 1\n $ rm cruft; echo $?\n 0\n $ rm cruft; echo $?\n rm: cannot remove 'cruft': No such file or directory\n 1\n $ rm -f cruft; echo $?\n 0\n\nwhich gives us both typo detection while supporting blind removal,\nis a usable workaround? I dunno.\n\nBut as I said, I am halfway OK with the change. I just think it is\na bit unfriendly to the users.\n\nThanks.\n"},{"id":"463439","messageId":"bc57439a-bddc-6c1a-a51d-11498d17c206@github.com","threadId":"58460","inReplyTo":"xmqqpmfo4pc7.fsf@gitster.g","subject":"Re: [PATCH] maintenance: make unregister idempotent","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-22T12:37:47Z","receivedAt":"2022-09-22T12:38:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/21/2022 1:19 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> From: Derrick Stolee \n>>\n>> The 'git maintenance unregister' subcommand has a step that removes the\n>> current repository from the multi-valued maitenance.repo config key.\n>> This fails if the repository is not listed in that key. This makes\n>> running 'git maintenance unregister' twice result in a failure in the\n>> second instance.\n>>\n>> Make this task idempotent, since the end result is the same in both\n>> cases: maintenance will no longer run on this repository.\n> \n> I am not sure if this is a good idea. What is the ultimate reason\n> why we want to allow running it blindly without knowing if it is\n> necessary? Is it because there is no easy way to tell if unregister\n> is needed in the first place?\n\nWe want to leave the internal details of what it means to be\nregistered as hidden to the user. They could look for the repo in\nthe global config, but that seems like a hassle when they just\nwant to make sure they are not currently registered. \n\n>> +\t# Expect unregister to be idempotent.\n>> +\tgit maintenance unregister\n>> '\n> \n> ... given that the user does not have any control over what to\n> unregister from the command line (it is decided implicitly by where\n> the user is), I am halfway OK with it.\n> \n> A user with two repositories may still be upset after running\n> \"unregister\" in the one they did not mean to, and not getting told\n> about the mistake, though, e.g.\n...\n> I wonder if something like the \"--force\" option, \n\nI like the --force option. I'll add it in v2 and then update Scalar\nto use that option.\n\nThanks,\n-Stolee\n"},{"id":"463441","messageId":"f5d8d6e490102202147c965a4d50c4f3b02d4a5d.1663853837.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v2.git.1663853837.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] scalar: make 'unregister' idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T13:37:17Z","receivedAt":"2022-09-22T13:38:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'scalar unregister' command removes a repository from the list of\nregistered Scalar repositories and removes it from the list of\nrepositories registered for background maintenance. If the repository\nwas not already registered for background maintenance, then the command\nfails, even if the repository was still registered as a Scalar\nrepository.\n\nAfter using 'scalar clone' or 'scalar register', the repository would be\nenrolled in background maintenance since those commands run 'git\nmaintenance start'. If the user runs 'git maintenance unregister' on\nthat repository, then it is still in the list of repositories which get\nnew config updates from 'scalar reconfigure'. The 'scalar unregister'\ncommand would fail since 'git maintenance unregister' would fail.\n\nFurther, the add_or_remove_enlistment() method in scalar.c already has\nthis idempotent nature built in as an expectation since it returns zero\nwhen the scalar.repo list already has the proper containment of the\nrepository.\n\nThe previous change added the 'git maintenance unregister --force'\noption, so use it within 'scalar unregister' to make it idempotent.\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 5 ++++-\n t/t9210-scalar.sh | 5 ++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex c5c1ce68919..6de9c0ee523 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure)\n \n static int toggle_maintenance(int enable)\n {\n-\treturn run_git(\"maintenance\", enable ? \"start\" : \"unregister\", NULL);\n+\treturn run_git(\"maintenance\",\n+\t\t enable ? \"start\" : \"unregister\",\n+\t\t enable ? NULL : \"--force\",\n+\t\t NULL);\n }\n \n static int add_or_remove_enlistment(int add)\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 14ca575a214..be51a8bb7a4 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' '\n \ttest_must_fail git config --get --global --fixed-value \\\n \t\tmaintenance.repo \"$(pwd)/vanish/src\" &&\n \tscalar list >scalar.repos &&\n-\t! grep -F \"$(pwd)/vanish/src\" scalar.repos\n+\t! grep -F \"$(pwd)/vanish/src\" scalar.repos &&\n+\n+\t# scalar unregister should be idempotent\n+\tscalar unregister vanish\n '\n \n test_expect_success 'set up repository to clone' '\n-- \ngitgitgadget\n"},{"id":"463442","messageId":"pull.1358.v2.git.1663853837.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.git.1663635732095.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] scalar: make unregister idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T13:37:15Z","receivedAt":"2022-09-22T13:38:26Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I noticed this while we were updating the microsoft/git fork to include\nv2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent\nbefore, but instead some change in 'scalar unregister' led to it relying on\nthe return code of 'git maintenance unregister'. Our functional tests expect\n'scalar unregister' to be idempotent, and I think that's a good pattern for\n'git maintenance unregister', so I'm fixing it at that layer.\n\nDespite finding this during the 2.38.0-rc0 integration, this isn't critical\nto the release.\n\nPerhaps an argument could be made that \"failure means it wasn't registered\nbefore\", but I think that isn't terribly helpful.\n\nOur functional tests are running the unregister subcommand to disable\nmaintenance in order to run tests on the object store (such as running\nmaintenance commands in the foreground and checking the object store\nafterwards). This is a form of automation using 'unregister' as a check that\nmaintenance will not run at the same time, and it doesn't care if\nmaintenance was already disabled. I can imagine other scripting scenarios\nwanting that kind of guarantee.\n\n\nUpdates in v2\n=============\n\n * This is now a two-patch series.\n * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the\n usage for 'git maintenance unregister' removed its translation markers.\n * Instead of making git maintenance unregister idempotent, add a --force\n option for those who do not want to require that the repository is\n already registered.\n * Make scalar unregister idempotent, with reasons argued in patch 2.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n maintenance: add 'unregister --force'\n scalar: make 'unregister' idempotent\n\n Documentation/git-maintenance.txt | 6 +++++-\n builtin/gc.c | 31 +++++++++++++++++++++++++------\n scalar.c | 5 ++++-\n t/t7900-maintenance.sh | 6 +++++-\n t/t9210-scalar.sh | 5 ++++-\n 5 files changed, 43 insertions(+), 10 deletions(-)\n\n\nbase-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1358\n\nRange-diff vs v1:\n\n 1: f028208a3ab ! 1: 69c74f52eef maintenance: make unregister idempotent\n @@ Metadata\n Author: Derrick Stolee \n \n ## Commit message ##\n - maintenance: make unregister idempotent\n + maintenance: add 'unregister --force'\n \n The 'git maintenance unregister' subcommand has a step that removes the\n current repository from the multi-valued maitenance.repo config key.\n @@ Commit message\n running 'git maintenance unregister' twice result in a failure in the\n second instance.\n \n - Make this task idempotent, since the end result is the same in both\n - cases: maintenance will no longer run on this repository.\n + This failure exit code is helpful, but its message is not. Add a new\n + die() message that explicitly calls out the failure due to the\n + repository not being registered.\n +\n + In some cases, users may want to run 'git maintenance unregister' just\n + to make sure that background jobs will not start on this repository, but\n + they do not want to check to see if it is registered first. Add a new\n + '--force' option that will siltently succeed if the repository is not\n + already registered.\n \n Signed-off-by: Derrick Stolee \n \n + ## Documentation/git-maintenance.txt ##\n +@@ Documentation/git-maintenance.txt: SYNOPSIS\n + [verse]\n + 'git maintenance' run []\n + 'git maintenance' start [--scheduler=]\n +-'git maintenance' (stop|register|unregister)\n ++'git maintenance' (stop|register|unregister) []\n + \n + \n + DESCRIPTION\n +@@ Documentation/git-maintenance.txt: unregister::\n + \tRemove the current repository from background maintenance. This\n + \tonly removes the repository from the configured list. It does not\n + \tstop the background maintenance processes from running.\n +++\n ++The `unregister` subcommand will report an error if the current repository\n ++is not already registered. Use the `--force` option to return success even\n ++when the current repository is not registered.\n + \n + TASKS\n + -----\n +\n ## builtin/gc.c ##\n -@@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, const char *prefi\n +@@ builtin/gc.c: done:\n + }\n + \n + static char const * const builtin_maintenance_unregister_usage[] = {\n +-\t\"git maintenance unregister\",\n ++\t\"git maintenance unregister [--force]\",\n + \tNULL\n + };\n + \n + static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n + {\n ++\tint force = 0;\n \tstruct option options[] = {\n ++\t\tOPT_BOOL(0, \"force\", &force,\n ++\t\t\t N_(\"return success even if repository was not registered\")),\n \t\tOPT_END(),\n \t};\n -\tint rc;\n @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con\n +\t\t\t \"--fixed-value\", key, maintpath, NULL);\n +\n +\t\trc = run_command(&config_unset);\n ++\t} else if (!force) {\n ++\t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n +\t}\n \n -\trc = run_command(&config_unset);\n @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '\n -\ttest_cmp before actual\n +\ttest_cmp before actual &&\n +\n -+\t# Expect unregister to be idempotent.\n -+\tgit maintenance unregister\n ++\ttest_must_fail git maintenance unregister 2>err &&\n ++\tgrep \"is not registered\" err &&\n ++\tgit maintenance unregister --force\n '\n \n test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n -: ----------- > 2: f5d8d6e4901 scalar: make 'unregister' idempotent\n\n-- \ngitgitgadget\n"},{"id":"463443","messageId":"69c74f52eefd906c38494759a02e137e4d7c01d8.1663853837.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v2.git.1663853837.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] maintenance: add 'unregister --force'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-22T13:37:16Z","receivedAt":"2022-09-22T13:38:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance unregister' subcommand has a step that removes the\ncurrent repository from the multi-valued maitenance.repo config key.\nThis fails if the repository is not listed in that key. This makes\nrunning 'git maintenance unregister' twice result in a failure in the\nsecond instance.\n\nThis failure exit code is helpful, but its message is not. Add a new\ndie() message that explicitly calls out the failure due to the\nrepository not being registered.\n\nIn some cases, users may want to run 'git maintenance unregister' just\nto make sure that background jobs will not start on this repository, but\nthey do not want to check to see if it is registered first. Add a new\n'--force' option that will siltently succeed if the repository is not\nalready registered.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/git-maintenance.txt | 6 +++++-\n builtin/gc.c | 31 +++++++++++++++++++++++++------\n t/t7900-maintenance.sh | 6 +++++-\n 3 files changed, 35 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex 9c630efe19c..bb888690e4d 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git maintenance' run []\n 'git maintenance' start [--scheduler=]\n-'git maintenance' (stop|register|unregister)\n+'git maintenance' (stop|register|unregister) []\n \n \n DESCRIPTION\n@@ -79,6 +79,10 @@ unregister::\n \tRemove the current repository from background maintenance. This\n \tonly removes the repository from the configured list. It does not\n \tstop the background maintenance processes from running.\n++\n+The `unregister` subcommand will report an error if the current repository\n+is not already registered. Use the `--force` option to return success even\n+when the current repository is not registered.\n \n TASKS\n -----\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2753bd15a5e..3189b4ba2b1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1519,18 +1519,25 @@ done:\n }\n \n static char const * const builtin_maintenance_unregister_usage[] = {\n-\t\"git maintenance unregister\",\n+\t\"git maintenance unregister [--force]\",\n \tNULL\n };\n \n static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n {\n+\tint force = 0;\n \tstruct option options[] = {\n+\t\tOPT_BOOL(0, \"force\", &force,\n+\t\t\t N_(\"return success even if repository was not registered\")),\n \t\tOPT_END(),\n \t};\n-\tint rc;\n+\tconst char *key = \"maintenance.repo\";\n+\tint rc = 0;\n \tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n+\tint found = 0;\n+\tstruct string_list_item *item;\n+\tconst struct string_list *list = git_config_get_value_multi(key);\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_unregister_usage, 0);\n@@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tusage_with_options(builtin_maintenance_unregister_usage,\n \t\t\t\t options);\n \n-\tconfig_unset.git_cmd = 1;\n-\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n+\tfor_each_string_list_item(item, list) {\n+\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\tfound = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (found) {\n+\t\tconfig_unset.git_cmd = 1;\n+\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n+\t\t\t \"--fixed-value\", key, maintpath, NULL);\n+\n+\t\trc = run_command(&config_unset);\n+\t} else if (!force) {\n+\t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n+\t}\n \n-\trc = run_command(&config_unset);\n \tfree(maintpath);\n \treturn rc;\n }\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 2724a44fe3e..cfe3236567a 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -493,7 +493,11 @@ test_expect_success 'register and unregister' '\n \n \tgit maintenance unregister &&\n \tgit config --global --get-all maintenance.repo >actual &&\n-\ttest_cmp before actual\n+\ttest_cmp before actual &&\n+\n+\ttest_must_fail git maintenance unregister 2>err &&\n+\tgrep \"is not registered\" err &&\n+\tgit maintenance unregister --force\n '\n \n test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n-- \ngitgitgadget\n\n"},{"id":"463467","messageId":"xmqqtu4zxl25.fsf@gitster.g","threadId":"58460","inReplyTo":"bc57439a-bddc-6c1a-a51d-11498d17c206@github.com","subject":"Re: [PATCH] maintenance: make unregister idempotent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-22T19:31:46Z","receivedAt":"2022-09-22T19:31:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> I am not sure if this is a good idea. What is the ultimate reason\n>> why we want to allow running it blindly without knowing if it is\n>> necessary? Is it because there is no easy way to tell if unregister\n>> is needed in the first place?\n>\n> We want to leave the internal details of what it means to be\n> registered as hidden to the user. They could look for the repo in\n> the global config, but that seems like a hassle when they just\n> want to make sure they are not currently registered. \n\nOK, so there is no published officially sanctioned way to ask \"is\nthis repository under maintenance's control and cron jobs run in\nit?\" or \"give me the list of such repositories\". \n\nThen I can see why you want to allow users to blindly run\n\"unregister\", with or without \"--force\".\n\nBut doesn't it point at a more fundamental problem? \n\nIs there a reason why we want to hide the list of repositories\n(enlistments?) from the users?\n\n\n"},{"id":"463469","messageId":"748f4e1f-f2a3-872c-2ffd-643841c454dd@github.com","threadId":"58460","inReplyTo":"xmqqtu4zxl25.fsf@gitster.g","subject":"Re: [PATCH] maintenance: make unregister idempotent","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-22T19:46:41Z","receivedAt":"2022-09-22T19:46:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/22/2022 3:31 PM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>>> I am not sure if this is a good idea. What is the ultimate reason\n>>> why we want to allow running it blindly without knowing if it is\n>>> necessary? Is it because there is no easy way to tell if unregister\n>>> is needed in the first place?\n>>\n>> We want to leave the internal details of what it means to be\n>> registered as hidden to the user. They could look for the repo in\n>> the global config, but that seems like a hassle when they just\n>> want to make sure they are not currently registered. \n> \n> OK, so there is no published officially sanctioned way to ask \"is\n> this repository under maintenance's control and cron jobs run in\n> it?\" or \"give me the list of such repositories\". \n> \n> Then I can see why you want to allow users to blindly run\n> \"unregister\", with or without \"--force\".\n> \n> But doesn't it point at a more fundamental problem? \n> \n> Is there a reason why we want to hide the list of repositories\n> (enlistments?) from the users?\n\nI don't think we want to hide it, but we've never needed to present\nthe list in a canonical way before. It's been sufficient to let\nusers run 'git config --global --get-all maintenance.repo', assuming\nthey know that config key is the important one.\n\nAdding a 'git maintenance list-registered' or something would solve\nthat problem, but I'm not sure it is a super high priority.\n\nThanks,\n-Stolee\n"},{"id":"463475","messageId":"xmqqh70zxhpp.fsf@gitster.g","threadId":"58460","inReplyTo":"748f4e1f-f2a3-872c-2ffd-643841c454dd@github.com","subject":"Re: [PATCH] maintenance: make unregister idempotent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-22T20:44:02Z","receivedAt":"2022-09-22T20:44:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Adding a 'git maintenance list-registered' or something would solve\n> that problem, but I'm not sure it is a super high priority.\n\nIs that \"git maintenance\" or \"scalar\" that list-registered would\nhang below?\n\nIn any case, I tend to think the word \"idempotent\" is used as a\nrough synonym to \"sloppy\", but unregistering from the automatic\nmaintenance (but still known as part of enlistment?) would probably\nbe a rare event that would not be a huge deal if the user failed to\ndo so without getting reminded, so I would not oppose to the step\n[2/2] of the updated series.\n\nThanks, queued.\n"},{"id":"463510","messageId":"20220923130832.GA1761@szeder.dev","threadId":"58460","inReplyTo":"69c74f52eefd906c38494759a02e137e4d7c01d8.1663853837.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] maintenance: add 'unregister --force'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-23T13:08:32Z","receivedAt":"2022-09-23T13:08:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n> static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n> {\n> +\tint force = 0;\n> \tstruct option options[] = {\n> +\t\tOPT_BOOL(0, \"force\", &force,\n> +\t\t\t N_(\"return success even if repository was not registered\")),\n\nThis could be shortened a bit by using OPT__FORCE() instead of\nOPT_BOOL(). OTOH, please make it a bit longer, and declare the option\nwith the PARSE_OPT_NOCOMPLETE flag to hide it from completion:\n'--force' is usually used to enable something potentially dangerous,\nand therefore should be used with care, so our completion script in\ngeneral doesn't offer it, giving the users more opportunity to think\nthings through while typing it out. Though, arguably in this\nparticular case it seems there is not much danger to be afraid of.\n\n\n> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n> \t\t\t\t options);\n> \n> -\tconfig_unset.git_cmd = 1;\n> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n> +\tfor_each_string_list_item(item, list) {\n> +\t\tif (!strcmp(maintpath, item->string)) {\n> +\t\t\tfound = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (found) {\n> +\t\tconfig_unset.git_cmd = 1;\n> +\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n> +\t\t\t \"--fixed-value\", key, maintpath, NULL);\n> +\n> +\t\trc = run_command(&config_unset);\n\nIt seems a bit heavy-handed to fork() an extra git process just to\nunset a config variable. Wouldn't a properly parametrized\ngit_config_set_multivar_in_file_gently() call be sufficient? Though\nTBH I don't offhand know what those parameters should be, in\nparticular whether there is a convenient way to find out the path of\nthe global config file in order to pass it to this function.\n\n(Not an issue with this patch, as the context shows that this has been\ndone with the extra process before; it just caught my eye.)\n\n"},{"id":"463627","messageId":"fc6739c3-2f7c-5dab-e4a2-8243deafde50@github.com","threadId":"58460","inReplyTo":"20220923130832.GA1761@szeder.dev","subject":"Re: [PATCH v2 1/2] maintenance: add 'unregister --force'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-26T13:32:14Z","receivedAt":"2022-09-26T15:02:25Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/23/2022 9:08 AM, SZEDER Gábor wrote:\n> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n>> {\n>> +\tint force = 0;\n>> \tstruct option options[] = {\n>> +\t\tOPT_BOOL(0, \"force\", &force,\n>> +\t\t\t N_(\"return success even if repository was not registered\")),\n> \n> This could be shortened a bit by using OPT__FORCE() instead of\n> OPT_BOOL(). OTOH, please make it a bit longer, and declare the option\n> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:\n\nLooks like I can do both like this:\n\n\t\tOPT__FORCE(&force,\n\t\t\t N_(\"return success even if repository was not registered\"),\n\t\t\t PARSE_OPT_NOCOMPLETE),\n\n>> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n>> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n>> \t\t\t\t options);\n>> \n>> -\tconfig_unset.git_cmd = 1;\n>> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n>> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n>> +\tfor_each_string_list_item(item, list) {\n>> +\t\tif (!strcmp(maintpath, item->string)) {\n>> +\t\t\tfound = 1;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tif (found) {\n>> +\t\tconfig_unset.git_cmd = 1;\n>> +\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n>> +\t\t\t \"--fixed-value\", key, maintpath, NULL);\n>> +\n>> +\t\trc = run_command(&config_unset);\n> \n> It seems a bit heavy-handed to fork() an extra git process just to\n> unset a config variable. Wouldn't a properly parametrized\n> git_config_set_multivar_in_file_gently() call be sufficient? Though\n> TBH I don't offhand know what those parameters should be, in\n> particular whether there is a convenient way to find out the path of\n> the global config file in order to pass it to this function.\n> \n> (Not an issue with this patch, as the context shows that this has been\n> done with the extra process before; it just caught my eye.)\n\nThanks. I'll look into it.\n\n-Stolee\n"},{"id":"463639","messageId":"220926.86mtamyvo7.gmgdl@evledraar.gmail.com","threadId":"58460","inReplyTo":"fc6739c3-2f7c-5dab-e4a2-8243deafde50@github.com","subject":"Re: [PATCH v2 1/2] maintenance: add 'unregister --force'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-26T15:39:50Z","receivedAt":"2022-09-26T16:59:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 26 2022, Derrick Stolee wrote:\n\n> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:\n>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>> static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n>>> {\n>>> +\tint force = 0;\n>>> \tstruct option options[] = {\n>>> +\t\tOPT_BOOL(0, \"force\", &force,\n>>> +\t\t\t N_(\"return success even if repository was not registered\")),\n>> \n>> This could be shortened a bit by using OPT__FORCE() instead of\n>> OPT_BOOL(). OTOH, please make it a bit longer, and declare the option\n>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:\n>\n> Looks like I can do both like this:\n>\n> \t\tOPT__FORCE(&force,\n> \t\t\t N_(\"return success even if repository was not registered\"),\n> \t\t\t PARSE_OPT_NOCOMPLETE),\n\nI don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it\nfor most of --force, but in some non-destructive cases (e.g. \"add\") we\ndon't.\n\nThis seems to be such a case, we'll destroy no data or do anything\nirrecoverable. It's really just a\n--do-not-be-so-anal-about-your-exit-code option.\n\nI'm guessing that you wanted to be able to error check this more\nstrictly in some cases. For \"git remote\" I post-hoc filled in this\nuse-case by exiting with a code of 2 on missing remotes on e.g. \"git\nremote remove\", see: 9144ba4cf52 (remote: add meaningful exit code on\nmissing/existing, 2020-10-27).\n\nIn this case we would return exit code 5 for this in most case before,\nas we wouldn't be able to find the key, wouldn't we? I.e. the return\nvalue from \"git config\". Seems so:\n\t\n\t$ GIT_TRACE=1 git maintenance unregister; echo $?\n\t17:48:59.984529 exec-cmd.c:90 trace: resolved executable path from procfs: /home/avar/local/bin/git\n\t17:48:59.984566 exec-cmd.c:237 trace: resolved executable dir: /home/avar/local/bin\n\t17:48:59.986795 git.c:509 trace: built-in: git maintenance unregister\n\t17:48:59.986817 run-command.c:654 trace: run_command: git config --global --unset --fixed-value maintenance.repo /home/avar/g/git\n\t17:48:59.987532 exec-cmd.c:90 trace: resolved executable path from procfs: /home/avar/local/bin/git\n\t17:48:59.987561 exec-cmd.c:237 trace: resolved executable dir: /home/avar/local/bin\n\t17:48:59.988733 git.c:509 trace: built-in: git config --global --unset --fixed-value maintenance.repo /home/avar/g/git\n\t5\n\nMaybe we want to just define an exit code here too? I think doing so is\na better interface, the user can then pipe the STDERR to /dev/null\nthemselves (or equivalent).\n\nAside from anything else, I think this would be much clearer if it were\nsplit up so that:\n\n * We first test what we do now without --force, which is clearly\n untested and undocumented (you are adding tests for it here\n while-at-it)\n\n * This commit, which adds a --force.\n\nAlso:\n\n> @@ -1538,11 +1545,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n> \t\t\t\t options);\n> \n> -\tconfig_unset.git_cmd = 1;\n> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n> +\tfor_each_string_list_item(item, list) {\n> +\t\tif (!strcmp(maintpath, item->string)) {\n> +\t\t\tfound = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nThis code now has a race condition it didn't before. Before we just did\na \"git config --unset\" which would have locked the config file, so if we\ndidn't have a key we'd return 5.\n\n> +\tif (found) {\n\nBut here we looked for the key *earlier*, so in that window we could\nhave raced and had the key again, so ....\n\n> +\t\tconfig_unset.git_cmd = 1;\n> +\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n> +\t\t\t \"--fixed-value\", key, maintpath, NULL);\n> +\n> +\t\trc = run_command(&config_unset);\n> +\t} else if (!force) {\n\n...found would not be true, and if you you didn't have --force...\n\n> +\t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n> +\t}\n> \n> -\trc = run_command(&config_unset);\n\n...this removal would cause us to still have the key in the end, no? I.e.:\n\n 1. We check if the key is there\n 2. Another process LOCKS config\n 3. Another process SETS the key\n 4. Another process UNLOCKS config\n 5. We act with the assumption that the key isn't set\n\nMaybe it's not racy, or it doesn't matter.\n"},{"id":"463646","messageId":"22a767da-5ae0-5a8f-d5f4-f2c5cc217be4@github.com","threadId":"58460","inReplyTo":"220926.86mtamyvo7.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/2] maintenance: add 'unregister --force'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-26T17:25:51Z","receivedAt":"2022-09-26T17:52:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/26/2022 11:39 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Sep 26 2022, Derrick Stolee wrote:\n> \n>> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:\n>>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>>> static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n>>>> {\n>>>> +\tint force = 0;\n>>>> \tstruct option options[] = {\n>>>> +\t\tOPT_BOOL(0, \"force\", &force,\n>>>> +\t\t\t N_(\"return success even if repository was not registered\")),\n>>>\n>>> This could be shortened a bit by using OPT__FORCE() instead of\n>>> OPT_BOOL(). OTOH, please make it a bit longer, and declare the option\n>>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:\n>>\n>> Looks like I can do both like this:\n>>\n>> \t\tOPT__FORCE(&force,\n>> \t\t\t N_(\"return success even if repository was not registered\"),\n>> \t\t\t PARSE_OPT_NOCOMPLETE),\n> \n> I don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it\n> for most of --force, but in some non-destructive cases (e.g. \"add\") we\n> don't.\n> \n> This seems to be such a case, we'll destroy no data or do anything\n> irrecoverable. It's really just a\n> --do-not-be-so-anal-about-your-exit-code option.\n\nI agree, so I wasn't completely sold on PARSE_OPT_NOCOMPLETE. I'll use\nyour vote to not add that option.\n\n> I'm guessing that you wanted to be able to error check this more\n> strictly in some cases. For \"git remote\" I post-hoc filled in this\n> use-case by exiting with a code of 2 on missing remotes on e.g. \"git\n> remote remove\", see: 9144ba4cf52 (remote: add meaningful exit code on\n> missing/existing, 2020-10-27).\n\nGenerally, I'm not terribly interested in the exit code value other\nthan 0, 128, and being three categories. I definitely\ndon't want to create a strict list of exit code modes here.\n\n> In this case we would return exit code 5 for this in most case before,\n> as we wouldn't be able to find the key, wouldn't we? I.e. the return\n> value from \"git config\".\n\nWe definitely inherited an exit code from 'git config' here, but\nI should probably convert it into a die() message to make it clearer.\n\n> This code now has a race condition it didn't before. Before we just did\n> a \"git config --unset\" which would have locked the config file, so if we\n> didn't have a key we'd return 5.\n> \n>> +\tif (found) {\n> \n> But here we looked for the key *earlier*, so in that window we could\n> have raced and had the key again, so ....\n> \n>> +\t\tconfig_unset.git_cmd = 1;\n>> +\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n>> +\t\t\t \"--fixed-value\", key, maintpath, NULL);\n>> +\n>> +\t\trc = run_command(&config_unset);\n>> +\t} else if (!force) {\n> \n> ...found would not be true, and if you you didn't have --force...\n> \n>> +\t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n>> +\t}\n>> \n>> -\trc = run_command(&config_unset);\n> \n> ...this removal would cause us to still have the key in the end, no? I.e.:\n> \n> 1. We check if the key is there\n> 2. Another process LOCKS config\n> 3. Another process SETS the key\n> 4. Another process UNLOCKS config> 5. We act with the assumption that the key isn't set\n\nI think this list of events is fine, since we check the value and then\ndo not try to modify state if it isn't there.\n\nInstead if we had this scenario:\n\n 1. We check and see that it _is_there.\n 2. Another process modifies config to remove the value.\n 3. We try to remove the value and fail.\n\nIn this case, the --force option would still fail even though the end\nresult is the same. We could ignore a \"not found\" case during a --force\noption to avoid failing due to such concurrency.\n \n> Maybe it's not racy, or it doesn't matter.\nGenerally, I think in this case it doesn't matter, but we can be a bit\nmore careful about handling races.\n\nThanks,\n-Stolee\n"},{"id":"463658","messageId":"pull.1358.v3.git.1664218087.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v2.git.1663853837.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] scalar: make unregister idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T18:48:03Z","receivedAt":"2022-09-26T18:48:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I noticed this while we were updating the microsoft/git fork to include\nv2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent\nbefore, but instead some change in 'scalar unregister' led to it relying on\nthe return code of 'git maintenance unregister'. Our functional tests expect\n'scalar unregister' to be idempotent, and I think that's a good pattern for\n'git maintenance unregister', so I'm fixing it at that layer.\n\nDespite finding this during the 2.38.0-rc0 integration, this isn't critical\nto the release.\n\nPerhaps an argument could be made that \"failure means it wasn't registered\nbefore\", but I think that isn't terribly helpful.\n\nOur functional tests are running the unregister subcommand to disable\nmaintenance in order to run tests on the object store (such as running\nmaintenance commands in the foreground and checking the object store\nafterwards). This is a form of automation using 'unregister' as a check that\nmaintenance will not run at the same time, and it doesn't care if\nmaintenance was already disabled. I can imagine other scripting scenarios\nwanting that kind of guarantee.\n\n\nUpdates in v3\n=============\n\n * The --force option now uses OPT_FORCE and is hidden from autocomplete.\n * A new commit is added that removes the use of Git subprocesses in favor\n of git_config_set_multivar_in_file_gently().\n\n\nUpdates in v2\n=============\n\n * This is now a two-patch series.\n * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the\n usage for 'git maintenance unregister' removed its translation markers.\n * Instead of making git maintenance unregister idempotent, add a --force\n option for those who do not want to require that the repository is\n already registered.\n * Make scalar unregister idempotent, with reasons argued in patch 2.\n\nThanks, -Stolee\n\nDerrick Stolee (3):\n maintenance: add 'unregister --force'\n scalar: make 'unregister' idempotent\n gc: replace config subprocesses with API calls\n\n Documentation/git-maintenance.txt | 6 +-\n builtin/gc.c | 95 +++++++++++++++++++++----------\n scalar.c | 5 +-\n t/t7900-maintenance.sh | 6 +-\n t/t9210-scalar.sh | 5 +-\n 5 files changed, 82 insertions(+), 35 deletions(-)\n\n\nbase-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1358\n\nRange-diff vs v2:\n\n 1: 69c74f52eef ! 1: 8a8bffaec89 maintenance: add 'unregister --force'\n @@ builtin/gc.c: done:\n {\n +\tint force = 0;\n \tstruct option options[] = {\n -+\t\tOPT_BOOL(0, \"force\", &force,\n -+\t\t\t N_(\"return success even if repository was not registered\")),\n ++\t\tOPT__FORCE(&force,\n ++\t\t\t N_(\"return success even if repository was not registered\"),\n ++\t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_END(),\n \t};\n -\tint rc;\n 2: f5d8d6e4901 = 2: 06d5ef3fc57 scalar: make 'unregister' idempotent\n -: ----------- > 3: 260d7bee36e gc: replace config subprocesses with API calls\n\n-- \ngitgitgadget\n"},{"id":"463659","messageId":"8a8bffaec89e55da0c5bcac2f04331e0d4e69790.1664218087.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v3.git.1664218087.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T18:48:04Z","receivedAt":"2022-09-26T18:48:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance unregister' subcommand has a step that removes the\ncurrent repository from the multi-valued maitenance.repo config key.\nThis fails if the repository is not listed in that key. This makes\nrunning 'git maintenance unregister' twice result in a failure in the\nsecond instance.\n\nThis failure exit code is helpful, but its message is not. Add a new\ndie() message that explicitly calls out the failure due to the\nrepository not being registered.\n\nIn some cases, users may want to run 'git maintenance unregister' just\nto make sure that background jobs will not start on this repository, but\nthey do not want to check to see if it is registered first. Add a new\n'--force' option that will siltently succeed if the repository is not\nalready registered.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/git-maintenance.txt | 6 +++++-\n builtin/gc.c | 32 +++++++++++++++++++++++++------\n t/t7900-maintenance.sh | 6 +++++-\n 3 files changed, 36 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex 9c630efe19c..bb888690e4d 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git maintenance' run []\n 'git maintenance' start [--scheduler=]\n-'git maintenance' (stop|register|unregister)\n+'git maintenance' (stop|register|unregister) []\n \n \n DESCRIPTION\n@@ -79,6 +79,10 @@ unregister::\n \tRemove the current repository from background maintenance. This\n \tonly removes the repository from the configured list. It does not\n \tstop the background maintenance processes from running.\n++\n+The `unregister` subcommand will report an error if the current repository\n+is not already registered. Use the `--force` option to return success even\n+when the current repository is not registered.\n \n TASKS\n -----\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2753bd15a5e..8d154586272 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1519,18 +1519,26 @@ done:\n }\n \n static char const * const builtin_maintenance_unregister_usage[] = {\n-\t\"git maintenance unregister\",\n+\t\"git maintenance unregister [--force]\",\n \tNULL\n };\n \n static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n {\n+\tint force = 0;\n \tstruct option options[] = {\n+\t\tOPT__FORCE(&force,\n+\t\t\t N_(\"return success even if repository was not registered\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_END(),\n \t};\n-\tint rc;\n+\tconst char *key = \"maintenance.repo\";\n+\tint rc = 0;\n \tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n+\tint found = 0;\n+\tstruct string_list_item *item;\n+\tconst struct string_list *list = git_config_get_value_multi(key);\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_unregister_usage, 0);\n@@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tusage_with_options(builtin_maintenance_unregister_usage,\n \t\t\t\t options);\n \n-\tconfig_unset.git_cmd = 1;\n-\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n+\tfor_each_string_list_item(item, list) {\n+\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\tfound = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (found) {\n+\t\tconfig_unset.git_cmd = 1;\n+\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n+\t\t\t \"--fixed-value\", key, maintpath, NULL);\n+\n+\t\trc = run_command(&config_unset);\n+\t} else if (!force) {\n+\t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n+\t}\n \n-\trc = run_command(&config_unset);\n \tfree(maintpath);\n \treturn rc;\n }\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 2724a44fe3e..cfe3236567a 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -493,7 +493,11 @@ test_expect_success 'register and unregister' '\n \n \tgit maintenance unregister &&\n \tgit config --global --get-all maintenance.repo >actual &&\n-\ttest_cmp before actual\n+\ttest_cmp before actual &&\n+\n+\ttest_must_fail git maintenance unregister 2>err &&\n+\tgrep \"is not registered\" err &&\n+\tgit maintenance unregister --force\n '\n \n test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n-- \ngitgitgadget\n\n"},{"id":"463660","messageId":"06d5ef3fc57e1961adb541274f6f9b99ab058b66.1664218087.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v3.git.1664218087.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] scalar: make 'unregister' idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T18:48:05Z","receivedAt":"2022-09-26T18:48:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'scalar unregister' command removes a repository from the list of\nregistered Scalar repositories and removes it from the list of\nrepositories registered for background maintenance. If the repository\nwas not already registered for background maintenance, then the command\nfails, even if the repository was still registered as a Scalar\nrepository.\n\nAfter using 'scalar clone' or 'scalar register', the repository would be\nenrolled in background maintenance since those commands run 'git\nmaintenance start'. If the user runs 'git maintenance unregister' on\nthat repository, then it is still in the list of repositories which get\nnew config updates from 'scalar reconfigure'. The 'scalar unregister'\ncommand would fail since 'git maintenance unregister' would fail.\n\nFurther, the add_or_remove_enlistment() method in scalar.c already has\nthis idempotent nature built in as an expectation since it returns zero\nwhen the scalar.repo list already has the proper containment of the\nrepository.\n\nThe previous change added the 'git maintenance unregister --force'\noption, so use it within 'scalar unregister' to make it idempotent.\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 5 ++++-\n t/t9210-scalar.sh | 5 ++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex c5c1ce68919..6de9c0ee523 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure)\n \n static int toggle_maintenance(int enable)\n {\n-\treturn run_git(\"maintenance\", enable ? \"start\" : \"unregister\", NULL);\n+\treturn run_git(\"maintenance\",\n+\t\t enable ? \"start\" : \"unregister\",\n+\t\t enable ? NULL : \"--force\",\n+\t\t NULL);\n }\n \n static int add_or_remove_enlistment(int add)\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 14ca575a214..be51a8bb7a4 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' '\n \ttest_must_fail git config --get --global --fixed-value \\\n \t\tmaintenance.repo \"$(pwd)/vanish/src\" &&\n \tscalar list >scalar.repos &&\n-\t! grep -F \"$(pwd)/vanish/src\" scalar.repos\n+\t! grep -F \"$(pwd)/vanish/src\" scalar.repos &&\n+\n+\t# scalar unregister should be idempotent\n+\tscalar unregister vanish\n '\n \n test_expect_success 'set up repository to clone' '\n-- \ngitgitgadget\n\n"},{"id":"463661","messageId":"260d7bee36e1af2f6a6a8791d293402e97a502e6.1664218087.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v3.git.1664218087.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] gc: replace config subprocesses with API calls","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T18:48:06Z","receivedAt":"2022-09-26T18:48:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance [un]register' commands set or unset the multi-\nvalued maintenance.repo config key with the absolute path of the current\nrepository. These are set in the global config file.\n\nInstead of calling a subcommand and creating a new process, create the\nproper API calls to git_config_set_multivar_in_file_gently(). It\nrequires loading the filename for the global config file (and erroring\nout if now $HOME value is set). We also need to be careful about using\nCONFIG_REGEX_NONE when adding the value and using\nCONFIG_FLAGS_FIXED_VALUE when removing the value. In both cases, we\ncheck that the value already exists (this check already existed for\n'unregister').\n\nAlso, remove the transparent translation of the error code from the\nconfig API to the exit code of 'git maintenance'. Instead, use die() to\nrecover from failures at that level. In the case of 'unregister\n--force', allow the CONFIG_NOTHING_SET error code to be a success. This\nallows a possible race where another process removes the config value.\nThe end result is that the config value is not set anymore, so we can\ntreat this as a success.\n\nReported-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 75 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 44 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8d154586272..4c59235950d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1470,11 +1470,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_END(),\n \t};\n-\tint rc;\n+\tint found = 0;\n+\tconst char *key = \"maintenance.repo\";\n \tchar *config_value;\n-\tstruct child_process config_set = CHILD_PROCESS_INIT;\n-\tstruct child_process config_get = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n+\tstruct string_list_item *item;\n+\tconst struct string_list *list;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_register_usage, 0);\n@@ -1491,31 +1492,35 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \telse\n \t\tgit_config_set(\"maintenance.strategy\", \"incremental\");\n \n-\tconfig_get.git_cmd = 1;\n-\tstrvec_pushl(&config_get.args, \"config\", \"--global\", \"--get\",\n-\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n-\tconfig_get.out = -1;\n-\n-\tif (start_command(&config_get)) {\n-\t\trc = error(_(\"failed to run 'git config'\"));\n-\t\tgoto done;\n+\tlist = git_config_get_value_multi(key);\n+\tif (list) {\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\t\tfound = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n \n-\t/* We already have this value in our config! */\n-\tif (!finish_command(&config_get)) {\n-\t\trc = 0;\n-\t\tgoto done;\n+\tif (!found) {\n+\t\tint rc;\n+\t\tchar *user_config, *xdg_config;\n+\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tif (!user_config)\n+\t\t\tdie(_(\"$HOME not set\"));\n+\t\trc = git_config_set_multivar_in_file_gently(\n+\t\t\tuser_config, \"maintenance.repo\", maintpath,\n+\t\t\tCONFIG_REGEX_NONE, 0);\n+\t\tfree(user_config);\n+\t\tfree(xdg_config);\n+\n+\t\tif (rc)\n+\t\t\tdie(_(\"unable to add '%s' value of '%s'\"),\n+\t\t\t key, maintpath);\n \t}\n \n-\tconfig_set.git_cmd = 1;\n-\tstrvec_pushl(&config_set.args, \"config\", \"--add\", \"--global\", \"maintenance.repo\",\n-\t\t maintpath, NULL);\n-\n-\trc = run_command(&config_set);\n-\n-done:\n \tfree(maintpath);\n-\treturn rc;\n+\treturn 0;\n }\n \n static char const * const builtin_maintenance_unregister_usage[] = {\n@@ -1533,8 +1538,6 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tOPT_END(),\n \t};\n \tconst char *key = \"maintenance.repo\";\n-\tint rc = 0;\n-\tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n \tint found = 0;\n \tstruct string_list_item *item;\n@@ -1554,17 +1557,27 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t}\n \n \tif (found) {\n-\t\tconfig_unset.git_cmd = 1;\n-\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t\t \"--fixed-value\", key, maintpath, NULL);\n-\n-\t\trc = run_command(&config_unset);\n+\t\tint rc;\n+\t\tchar *user_config, *xdg_config;\n+\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tif (!user_config)\n+\t\t\tdie(_(\"$HOME not set\"));\n+\t\trc = git_config_set_multivar_in_file_gently(\n+\t\t\tuser_config, key, NULL, maintpath,\n+\t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n+\t\tfree(user_config);\n+\t\tfree(xdg_config);\n+\n+\t\tif (rc &&\n+\t\t (!force || rc == CONFIG_NOTHING_SET))\n+\t\t\tdie(_(\"unable to unset '%s' value of '%s'\"),\n+\t\t\t key, maintpath);\n \t} else if (!force) {\n \t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n \t}\n \n \tfree(maintpath);\n-\treturn rc;\n+\treturn 0;\n }\n \n static const char *get_frequency(enum schedule_priority schedule)\n-- \ngitgitgadget\n"},{"id":"463669","messageId":"xmqqbkr2ndxv.fsf@gitster.g","threadId":"58460","inReplyTo":"22a767da-5ae0-5a8f-d5f4-f2c5cc217be4@github.com","subject":"Re: [PATCH v2 1/2] maintenance: add 'unregister --force'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T19:17:00Z","receivedAt":"2022-09-26T19:17:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 9/26/2022 11:39 AM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Mon, Sep 26 2022, Derrick Stolee wrote:\n>> \n>>> On 9/23/2022 9:08 AM, SZEDER Gábor wrote:\n>>>> On Thu, Sep 22, 2022 at 01:37:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>>>> static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n>>>>> {\n>>>>> +\tint force = 0;\n>>>>> \tstruct option options[] = {\n>>>>> +\t\tOPT_BOOL(0, \"force\", &force,\n>>>>> +\t\t\t N_(\"return success even if repository was not registered\")),\n>>>>\n>>>> This could be shortened a bit by using OPT__FORCE() instead of\n>>>> OPT_BOOL(). OTOH, please make it a bit longer, and declare the option\n>>>> with the PARSE_OPT_NOCOMPLETE flag to hide it from completion:\n>>>\n>>> Looks like I can do both like this:\n>>>\n>>> \t\tOPT__FORCE(&force,\n>>> \t\t\t N_(\"return success even if repository was not registered\"),\n>>> \t\t\t PARSE_OPT_NOCOMPLETE),\n>> \n>> I don't think PARSE_OPT_NOCOMPLETE is appropriate here. Yes we use it\n>> for most of --force, but in some non-destructive cases (e.g. \"add\") we\n>> don't.\n>> \n>> This seems to be such a case, we'll destroy no data or do anything\n>> irrecoverable. It's really just a\n>> --do-not-be-so-anal-about-your-exit-code option.\n>\n> I agree, so I wasn't completely sold on PARSE_OPT_NOCOMPLETE. I'll use\n> your vote to not add that option.\n\nI am perfectly OK with that. Given that \"git reset --hard\" is not\ngiven nocomplete option, I do not see much point in \"destructive\nones are not completed\" guideline in practice anyway. After all,\n\"add --force\" would be destructively removing the object name\nrecorded for the path previously.\n\nThanks for carefully thinking the UI remifications through.\n"},{"id":"463671","messageId":"xmqq7d1qndn7.fsf@gitster.g","threadId":"58460","inReplyTo":"8a8bffaec89e55da0c5bcac2f04331e0d4e69790.1664218087.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T19:23:24Z","receivedAt":"2022-09-26T19:23:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> [verse]\n> 'git maintenance' run []\n> 'git maintenance' start [--scheduler=]\n> -'git maintenance' (stop|register|unregister)\n> +'git maintenance' (stop|register|unregister) []\n\nAn unrelated tangent, but should register complain when given in a\nrepository that is already registered as well? Just being curious.\n"},{"id":"463672","messageId":"xmqq35cendfq.fsf@gitster.g","threadId":"58460","inReplyTo":"260d7bee36e1af2f6a6a8791d293402e97a502e6.1664218087.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/3] gc: replace config subprocesses with API calls","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T19:27:53Z","receivedAt":"2022-09-26T19:28:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The 'git maintenance [un]register' commands set or unset the multi-\n> valued maintenance.repo config key with the absolute path of the current\n> repository. These are set in the global config file.\n\nOK. This step is new but it looks reasonable. \n\nEmbarrassingly sadly, we open, rewrite, and close the configuration\nfile for each of these \"proper API calls\", so the IO load is not\nreduced, even though we do not have to spawn extra processes ;-)\n\nAll three patches queued. Thanks.\n"},{"id":"463675","messageId":"03f26dc6-85c7-1ce9-67d6-b4325e9e6d52@github.com","threadId":"58460","inReplyTo":"xmqq7d1qndn7.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-26T20:49:50Z","receivedAt":"2022-09-26T20:49:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/26/2022 3:23 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> @@ -11,7 +11,7 @@ SYNOPSIS\n>> [verse]\n>> 'git maintenance' run []\n>> 'git maintenance' start [--scheduler=]\n>> -'git maintenance' (stop|register|unregister)\n>> +'git maintenance' (stop|register|unregister) []\n> \n> An unrelated tangent, but should register complain when given in a\n> repository that is already registered as well? Just being curious.\n\nLet's leave that as a #leftoverbits and perhaps as something to\nconsider next to something like 'git maintenance list' to list the\ncurrently-registered repositories.\n\nThanks,\n-Stolee\n"},{"id":"463676","messageId":"xmqqa66lby1w.fsf@gitster.g","threadId":"58460","inReplyTo":"8a8bffaec89e55da0c5bcac2f04331e0d4e69790.1664218087.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-26T21:55:39Z","receivedAt":"2022-09-26T21:55:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n> \t\t\t\t options);\n> \n> -\tconfig_unset.git_cmd = 1;\n> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n> +\tfor_each_string_list_item(item, list) {\n> +\t\tif (!strcmp(maintpath, item->string)) {\n> +\t\t\tfound = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nJust out of curiosity, I ran this in a fresh repository and got a\nsegfault. An attached patch obviously fixes it, but I am wondering\nif a better \"fix\" is to teach for_each_string_list_item() that it is\nperfectly reasonable to see a NULL passed to it as the list, which\nis a mere special case that the caller has a string list with 0\nitems on it.\n\nThoughts?\n\nThanks.\n\n\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 4c59235950..67f41fad4b 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1549,6 +1549,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tusage_with_options(builtin_maintenance_unregister_usage,\n \t\t\t\t options);\n \n+\tif (list)\n \tfor_each_string_list_item(item, list) {\n \t\tif (!strcmp(maintpath, item->string)) {\n \t\t\tfound = 1;\n-- \n2.38.0-rc1-123-g02867222b8\n\n"},{"id":"463706","messageId":"b1e6f118-046c-42f8-01a8-2543e792685a@github.com","threadId":"58460","inReplyTo":"xmqqa66lby1w.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-27T11:38:17Z","receivedAt":"2022-09-27T11:38:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/26/2022 5:55 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n>> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n>> \t\t\t\t options);\n>> \n>> -\tconfig_unset.git_cmd = 1;\n>> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n>> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n>> +\tfor_each_string_list_item(item, list) {\n>> +\t\tif (!strcmp(maintpath, item->string)) {\n>> +\t\t\tfound = 1;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n> \n> Just out of curiosity, I ran this in a fresh repository and got a\n> segfault.\n\nYikes! Thanks for catching. I think there was another instance in\nthe 'register' code that I caught by tests, but I appreciate you\ncatching this one.\n\n> An attached patch obviously fixes it, but I am wondering\n> if a better \"fix\" is to teach for_each_string_list_item() that it is\n> perfectly reasonable to see a NULL passed to it as the list, which\n> is a mere special case that the caller has a string list with 0\n> items on it.\n> \n> Thoughts?\n\nI agree that for_each_string_list_item() could handle NULL lists\nbetter, especially because it looks like a method and hides some\ndetails. Plus, wrapping the for-ish loop with an if statement is\nparticularly ugly.\n\nI think this might be more confusing that\ngit_configset_get_value_multi() can return a NULL list instead of\nan empty list. It boils down to this diff:\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..39cc0534170 100644\n--- a/config.c\n+++ b/config.c\n@@ -2416,8 +2416,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n \n const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n {\n+\tstatic struct string_list empty_list = STRING_LIST_INIT_NODUP;\n \tstruct config_set_element *e = configset_find_element(cs, key);\n-\treturn e ? &e->value_list : NULL;\n+\treturn e ? &e->value_list : &empty_list;\n }\n \n int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n\nHowever, this change causes a lot of cascading failures that\nare probably not worth tracking down.\n\nI'll get a patch put together that changes the behavior of\nfor_each_string_list_item() and adds the missing 'unregister' test\nso we can avoid this problem.\n\nThanks,\n-Stolee\n"},{"id":"463709","messageId":"63aa68fb-c215-a79f-e3c6-1c1a489220e2@github.com","threadId":"58460","inReplyTo":"b1e6f118-046c-42f8-01a8-2543e792685a@github.com","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-27T11:54:38Z","receivedAt":"2022-09-27T11:54:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/2022 7:38 AM, Derrick Stolee wrote:\n> On 9/26/2022 5:55 PM, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>>\n>>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n>>> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n>>> \t\t\t\t options);\n>>> \n>>> -\tconfig_unset.git_cmd = 1;\n>>> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n>>> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n>>> +\tfor_each_string_list_item(item, list) {\n>>> +\t\tif (!strcmp(maintpath, item->string)) {\n>>> +\t\t\tfound = 1;\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t}\n>>\n>> Just out of curiosity, I ran this in a fresh repository and got a\n>> segfault.\n> \n> Yikes! Thanks for catching. I think there was another instance in\n> the 'register' code that I caught by tests, but I appreciate you\n> catching this one.\n> \n>> An attached patch obviously fixes it, but I am wondering\n>> if a better \"fix\" is to teach for_each_string_list_item() that it is\n>> perfectly reasonable to see a NULL passed to it as the list, which\n>> is a mere special case that the caller has a string list with 0\n>> items on it.\n>>\n>> Thoughts?\n> \n> I agree that for_each_string_list_item() could handle NULL lists\n> better, especially because it looks like a method and hides some\n> details. Plus, wrapping the for-ish loop with an if statement is\n> particularly ugly.\n...\n> I'll get a patch put together that changes the behavior of\n> for_each_string_list_item() and adds the missing 'unregister' test\n> so we can avoid this problem.\n\nOf course, there is a reason why we don't check for NULL here,\nand it's because -Werror=address complains when we use a non-pointer\nvalue in the macro:\n\nstring-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]\n 146 | for (item = (list) ? (list)->items : NULL; \\\n |\n\nI tried searching for a way to suppress this error in a particular\ncase like this (perhaps using something like an attribute?), but I\ncouldn't find anything.\n\nThanks,\n-Stolee\n"},{"id":"463711","messageId":"220927.86a66lylk5.gmgdl@evledraar.gmail.com","threadId":"58460","inReplyTo":"63aa68fb-c215-a79f-e3c6-1c1a489220e2@github.com","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T13:36:14Z","receivedAt":"2022-09-27T13:49:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Derrick Stolee wrote:\n\n> On 9/27/2022 7:38 AM, Derrick Stolee wrote:\n>> On 9/26/2022 5:55 PM, Junio C Hamano wrote:\n>>> \"Derrick Stolee via GitGitGadget\" writes:\n>>>\n>>>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n>>>> \t\tusage_with_options(builtin_maintenance_unregister_usage,\n>>>> \t\t\t\t options);\n>>>> \n>>>> -\tconfig_unset.git_cmd = 1;\n>>>> -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n>>>> -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n>>>> +\tfor_each_string_list_item(item, list) {\n>>>> +\t\tif (!strcmp(maintpath, item->string)) {\n>>>> +\t\t\tfound = 1;\n>>>> +\t\t\tbreak;\n>>>> +\t\t}\n>>>> +\t}\n>>>\n>>> Just out of curiosity, I ran this in a fresh repository and got a\n>>> segfault.\n>> \n>> Yikes! Thanks for catching. I think there was another instance in\n>> the 'register' code that I caught by tests, but I appreciate you\n>> catching this one.\n>> \n>>> An attached patch obviously fixes it, but I am wondering\n>>> if a better \"fix\" is to teach for_each_string_list_item() that it is\n>>> perfectly reasonable to see a NULL passed to it as the list, which\n>>> is a mere special case that the caller has a string list with 0\n>>> items on it.\n>>>\n>>> Thoughts?\n>> \n>> I agree that for_each_string_list_item() could handle NULL lists\n>> better, especially because it looks like a method and hides some\n>> details. Plus, wrapping the for-ish loop with an if statement is\n>> particularly ugly.\n> ...\n>> I'll get a patch put together that changes the behavior of\n>> for_each_string_list_item() and adds the missing 'unregister' test\n>> so we can avoid this problem.\n>\n> Of course, there is a reason why we don't check for NULL here,\n> and it's because -Werror=address complains when we use a non-pointer\n> value in the macro:\n>\n> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]\n> 146 | for (item = (list) ? (list)->items : NULL; \\\n> |\n>\n> I tried searching for a way to suppress this error in a particular\n> case like this (perhaps using something like an attribute?), but I\n> couldn't find anything.\n\nWe discussed this exact issue just a few months ago, see:\nhttps://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/\n\nIn general I don't think we should be teaching\nfor_each_string_list_item() to handle NULL.\n\nInstead most callers that need to deal with a \"NULL\" list should\nprobably just use a list that's never NULL. See:\nhttps://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/\n\nIn this case however it seems perfectly reasonable to return a valid\npointer or NULL, and the function documents as much:\n\t\n\t/**\n\t * Finds and returns the value list, sorted in order of increasing priority\n\t * for the configuration variable `key`. When the configuration variable\n\t * `key` is not found, returns NULL. The caller should not free or modify\n\t * the returned pointer, as it is owned by the cache.\n\t */\n\tconst struct string_list *git_config_get_value_multi(const char *key);\n\nYou also have code in 3/3 that uses that API in the correct way, I think\njust adjusting this callsite in 1/3 would be the right move here.\n\nThis also gives the reader & compiler more information to e.g. eliminate\ndead code. You're calling maintpath() unconditionally, but if you have\nno config & the user provided --force we'll never end up using it, so we\ncan avoid allocating it in the first place.\n\n"},{"id":"463714","messageId":"30b43cee-e7fb-067f-8a84-ff1fef5444db@github.com","threadId":"58460","inReplyTo":"220927.86a66lylk5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 1/3] maintenance: add 'unregister --force'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-27T13:55:50Z","receivedAt":"2022-09-27T13:56:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/2022 9:36 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Sep 27 2022, Derrick Stolee wrote:\n>> Of course, there is a reason why we don't check for NULL here,\n>> and it's because -Werror=address complains when we use a non-pointer\n>> value in the macro:\n>>\n>> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]\n>> 146 | for (item = (list) ? (list)->items : NULL; \\\n>> |\n>>\n>> I tried searching for a way to suppress this error in a particular\n>> case like this (perhaps using something like an attribute?), but I\n>> couldn't find anything.\n> \n> We discussed this exact issue just a few months ago, see:\n> https://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/\n\nThanks for finding this thread. I knew it was vaguely familiar.\n \n> In general I don't think we should be teaching\n> for_each_string_list_item() to handle NULL.\n> \n> Instead most callers that need to deal with a \"NULL\" list should\n> probably just use a list that's never NULL. See:\n> https://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/\n> \n> In this case however it seems perfectly reasonable to return a valid\n> pointer or NULL, and the function documents as much:\n> \t\n> \t/**\n> \t * Finds and returns the value list, sorted in order of increasing priority\n> \t * for the configuration variable `key`. When the configuration variable\n> \t * `key` is not found, returns NULL. The caller should not free or modify\n> \t * the returned pointer, as it is owned by the cache.\n> \t */\n> \tconst struct string_list *git_config_get_value_multi(const char *key);\n\nIt documents that it will never return an empty list, and instead will\nreturn NULL. There are several places that check that condition explicitly.\nConverting them is not terribly hard, though, and I'll send an RFC soon\nthat performs that conversion.\n\n> This also gives the reader & compiler more information to e.g. eliminate\n> dead code. You're calling maintpath() unconditionally, but if you have\n> no config & the user provided --force we'll never end up using it, so we\n> can avoid allocating it in the first place.\n\nWhile you're correct that we could avoid that allocation, it makes the\ncode look terrible and hard to reason about, so I won't make that change.\n\nThanks,\n-Stolee\n \n"},{"id":"463715","messageId":"pull.1358.v4.git.1664287021.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v3.git.1664218087.gitgitgadget@gmail.com","subject":"[PATCH v4 0/4] scalar: make unregister idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T13:56:57Z","receivedAt":"2022-09-27T13:57:08Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I noticed this while we were updating the microsoft/git fork to include\nv2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent\nbefore, but instead some change in 'scalar unregister' led to it relying on\nthe return code of 'git maintenance unregister'. Our functional tests expect\n'scalar unregister' to be idempotent, and I think that's a good pattern for\n'git maintenance unregister', so I'm fixing it at that layer.\n\nDespite finding this during the 2.38.0-rc0 integration, this isn't critical\nto the release.\n\nPerhaps an argument could be made that \"failure means it wasn't registered\nbefore\", but I think that isn't terribly helpful.\n\nOur functional tests are running the unregister subcommand to disable\nmaintenance in order to run tests on the object store (such as running\nmaintenance commands in the foreground and checking the object store\nafterwards). This is a form of automation using 'unregister' as a check that\nmaintenance will not run at the same time, and it doesn't care if\nmaintenance was already disabled. I can imagine other scripting scenarios\nwanting that kind of guarantee.\n\n\nUpdates in v4\n=============\n\n * The previous version would segfault if 'git maintenance unregister' was\n called with an empty 'maintenance.repo' config list. This scenario is\n fixed and tested.\n * Part of the issue is that for_each_string_list_item() cannot handle a\n NULL value. The macro can't be made smarter without failing\n -Werror=address issues, so for now I added a patch that adds a warning to\n its doc comment.\n\n\nUpdates in v3\n=============\n\n * The --force option now uses OPT_FORCE and is hidden from autocomplete.\n * A new commit is added that removes the use of Git subprocesses in favor\n of git_config_set_multivar_in_file_gently().\n\n\nUpdates in v2\n=============\n\n * This is now a two-patch series.\n * I rebased onto v2.38.0-rc1 for two reasons: Scalar is now merged, and the\n usage for 'git maintenance unregister' removed its translation markers.\n * Instead of making git maintenance unregister idempotent, add a --force\n option for those who do not want to require that the repository is\n already registered.\n * Make scalar unregister idempotent, with reasons argued in patch 2.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n maintenance: add 'unregister --force'\n scalar: make 'unregister' idempotent\n gc: replace config subprocesses with API calls\n string-list: document iterator behavior on NULL input\n\n Documentation/git-maintenance.txt | 6 +-\n builtin/gc.c | 98 +++++++++++++++++++++----------\n scalar.c | 5 +-\n string-list.h | 7 ++-\n t/t7900-maintenance.sh | 11 +++-\n t/t9210-scalar.sh | 5 +-\n 6 files changed, 96 insertions(+), 36 deletions(-)\n\n\nbase-commit: 1b3d6e17fe83eb6f79ffbac2f2c61bbf1eaef5f8\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1358\n\nRange-diff vs v3:\n\n 1: 8a8bffaec89 ! 1: c3301e21109 maintenance: add 'unregister --force'\n @@ Commit message\n '--force' option that will siltently succeed if the repository is not\n already registered.\n \n + Also add an extra test of 'git maintenance unregister' at a point where\n + there are no registered repositories. This should fail without --force.\n +\n Signed-off-by: Derrick Stolee \n \n ## Documentation/git-maintenance.txt ##\n @@ builtin/gc.c: done:\n \tchar *maintpath = get_maintpath();\n +\tint found = 0;\n +\tstruct string_list_item *item;\n -+\tconst struct string_list *list = git_config_get_value_multi(key);\n ++\tconst struct string_list *list;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_unregister_usage, 0);\n @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con\n -\tconfig_unset.git_cmd = 1;\n -\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n -\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n -+\tfor_each_string_list_item(item, list) {\n -+\t\tif (!strcmp(maintpath, item->string)) {\n -+\t\t\tfound = 1;\n -+\t\t\tbreak;\n ++\tlist = git_config_get_value_multi(key);\n ++\tif (list) {\n ++\t\tfor_each_string_list_item(item, list) {\n ++\t\t\tif (!strcmp(maintpath, item->string)) {\n ++\t\t\t\tfound = 1;\n ++\t\t\t\tbreak;\n ++\t\t\t}\n +\t\t}\n +\t}\n +\n @@ builtin/gc.c: static int maintenance_unregister(int argc, const char **argv, con\n }\n \n ## t/t7900-maintenance.sh ##\n +@@ t/t7900-maintenance.sh: test_expect_success 'maintenance.strategy inheritance' '\n + \n + test_expect_success 'register and unregister' '\n + \ttest_when_finished git config --global --unset-all maintenance.repo &&\n ++\n ++\ttest_must_fail git maintenance unregister 2>err &&\n ++\tgrep \"is not registered\" err &&\n ++\tgit maintenance unregister --force &&\n ++\n + \tgit config --global --add maintenance.repo /existing1 &&\n + \tgit config --global --add maintenance.repo /existing2 &&\n + \tgit config --global --get-all maintenance.repo >before &&\n @@ t/t7900-maintenance.sh: test_expect_success 'register and unregister' '\n \n \tgit maintenance unregister &&\n 2: 06d5ef3fc57 = 2: a768c326c0f scalar: make 'unregister' idempotent\n 3: 260d7bee36e = 3: 5aa9cc1d6b9 gc: replace config subprocesses with API calls\n -: ----------- > 4: 73a262cdca4 string-list: document iterator behavior on NULL input\n\n-- \ngitgitgadget\n"},{"id":"463716","messageId":"c3301e21109b3f730d601ea4f21a49b9e7319cdf.1664287021.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v4.git.1664287021.gitgitgadget@gmail.com","subject":"[PATCH v4 1/4] maintenance: add 'unregister --force'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T13:56:58Z","receivedAt":"2022-09-27T13:57:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance unregister' subcommand has a step that removes the\ncurrent repository from the multi-valued maitenance.repo config key.\nThis fails if the repository is not listed in that key. This makes\nrunning 'git maintenance unregister' twice result in a failure in the\nsecond instance.\n\nThis failure exit code is helpful, but its message is not. Add a new\ndie() message that explicitly calls out the failure due to the\nrepository not being registered.\n\nIn some cases, users may want to run 'git maintenance unregister' just\nto make sure that background jobs will not start on this repository, but\nthey do not want to check to see if it is registered first. Add a new\n'--force' option that will siltently succeed if the repository is not\nalready registered.\n\nAlso add an extra test of 'git maintenance unregister' at a point where\nthere are no registered repositories. This should fail without --force.\n\nSigned-off-by: Derrick Stolee \n---\n Documentation/git-maintenance.txt | 6 +++++-\n builtin/gc.c | 35 +++++++++++++++++++++++++------\n t/t7900-maintenance.sh | 11 +++++++++-\n 3 files changed, 44 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt\nindex 9c630efe19c..bb888690e4d 100644\n--- a/Documentation/git-maintenance.txt\n+++ b/Documentation/git-maintenance.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git maintenance' run []\n 'git maintenance' start [--scheduler=]\n-'git maintenance' (stop|register|unregister)\n+'git maintenance' (stop|register|unregister) []\n \n \n DESCRIPTION\n@@ -79,6 +79,10 @@ unregister::\n \tRemove the current repository from background maintenance. This\n \tonly removes the repository from the configured list. It does not\n \tstop the background maintenance processes from running.\n++\n+The `unregister` subcommand will report an error if the current repository\n+is not already registered. Use the `--force` option to return success even\n+when the current repository is not registered.\n \n TASKS\n -----\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 2753bd15a5e..dc0ba9e3648 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1519,18 +1519,26 @@ done:\n }\n \n static char const * const builtin_maintenance_unregister_usage[] = {\n-\t\"git maintenance unregister\",\n+\t\"git maintenance unregister [--force]\",\n \tNULL\n };\n \n static int maintenance_unregister(int argc, const char **argv, const char *prefix)\n {\n+\tint force = 0;\n \tstruct option options[] = {\n+\t\tOPT__FORCE(&force,\n+\t\t\t N_(\"return success even if repository was not registered\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_END(),\n \t};\n-\tint rc;\n+\tconst char *key = \"maintenance.repo\";\n+\tint rc = 0;\n \tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n+\tint found = 0;\n+\tstruct string_list_item *item;\n+\tconst struct string_list *list;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_unregister_usage, 0);\n@@ -1538,11 +1546,26 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tusage_with_options(builtin_maintenance_unregister_usage,\n \t\t\t\t options);\n \n-\tconfig_unset.git_cmd = 1;\n-\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n+\tlist = git_config_get_value_multi(key);\n+\tif (list) {\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\t\tfound = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\tif (found) {\n+\t\tconfig_unset.git_cmd = 1;\n+\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n+\t\t\t \"--fixed-value\", key, maintpath, NULL);\n+\n+\t\trc = run_command(&config_unset);\n+\t} else if (!force) {\n+\t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n+\t}\n \n-\trc = run_command(&config_unset);\n \tfree(maintpath);\n \treturn rc;\n }\ndiff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh\nindex 2724a44fe3e..96bdd420456 100755\n--- a/t/t7900-maintenance.sh\n+++ b/t/t7900-maintenance.sh\n@@ -480,6 +480,11 @@ test_expect_success 'maintenance.strategy inheritance' '\n \n test_expect_success 'register and unregister' '\n \ttest_when_finished git config --global --unset-all maintenance.repo &&\n+\n+\ttest_must_fail git maintenance unregister 2>err &&\n+\tgrep \"is not registered\" err &&\n+\tgit maintenance unregister --force &&\n+\n \tgit config --global --add maintenance.repo /existing1 &&\n \tgit config --global --add maintenance.repo /existing2 &&\n \tgit config --global --get-all maintenance.repo >before &&\n@@ -493,7 +498,11 @@ test_expect_success 'register and unregister' '\n \n \tgit maintenance unregister &&\n \tgit config --global --get-all maintenance.repo >actual &&\n-\ttest_cmp before actual\n+\ttest_cmp before actual &&\n+\n+\ttest_must_fail git maintenance unregister 2>err &&\n+\tgrep \"is not registered\" err &&\n+\tgit maintenance unregister --force\n '\n \n test_expect_success !MINGW 'register and unregister with regex metacharacters' '\n-- \ngitgitgadget\n\n"},{"id":"463717","messageId":"a768c326c0fc749c40369461865688b5dd3f25af.1664287021.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v4.git.1664287021.gitgitgadget@gmail.com","subject":"[PATCH v4 2/4] scalar: make 'unregister' idempotent","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T13:56:59Z","receivedAt":"2022-09-27T13:57:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'scalar unregister' command removes a repository from the list of\nregistered Scalar repositories and removes it from the list of\nrepositories registered for background maintenance. If the repository\nwas not already registered for background maintenance, then the command\nfails, even if the repository was still registered as a Scalar\nrepository.\n\nAfter using 'scalar clone' or 'scalar register', the repository would be\nenrolled in background maintenance since those commands run 'git\nmaintenance start'. If the user runs 'git maintenance unregister' on\nthat repository, then it is still in the list of repositories which get\nnew config updates from 'scalar reconfigure'. The 'scalar unregister'\ncommand would fail since 'git maintenance unregister' would fail.\n\nFurther, the add_or_remove_enlistment() method in scalar.c already has\nthis idempotent nature built in as an expectation since it returns zero\nwhen the scalar.repo list already has the proper containment of the\nrepository.\n\nThe previous change added the 'git maintenance unregister --force'\noption, so use it within 'scalar unregister' to make it idempotent.\n\nSigned-off-by: Derrick Stolee \n---\n scalar.c | 5 ++++-\n t/t9210-scalar.sh | 5 ++++-\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/scalar.c b/scalar.c\nindex c5c1ce68919..6de9c0ee523 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure)\n \n static int toggle_maintenance(int enable)\n {\n-\treturn run_git(\"maintenance\", enable ? \"start\" : \"unregister\", NULL);\n+\treturn run_git(\"maintenance\",\n+\t\t enable ? \"start\" : \"unregister\",\n+\t\t enable ? NULL : \"--force\",\n+\t\t NULL);\n }\n \n static int add_or_remove_enlistment(int add)\ndiff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh\nindex 14ca575a214..be51a8bb7a4 100755\n--- a/t/t9210-scalar.sh\n+++ b/t/t9210-scalar.sh\n@@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' '\n \ttest_must_fail git config --get --global --fixed-value \\\n \t\tmaintenance.repo \"$(pwd)/vanish/src\" &&\n \tscalar list >scalar.repos &&\n-\t! grep -F \"$(pwd)/vanish/src\" scalar.repos\n+\t! grep -F \"$(pwd)/vanish/src\" scalar.repos &&\n+\n+\t# scalar unregister should be idempotent\n+\tscalar unregister vanish\n '\n \n test_expect_success 'set up repository to clone' '\n-- \ngitgitgadget\n\n"},{"id":"463718","messageId":"5aa9cc1d6b93b5ad3d66ac01a4518a91ced39bcb.1664287021.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v4.git.1664287021.gitgitgadget@gmail.com","subject":"[PATCH v4 3/4] gc: replace config subprocesses with API calls","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T13:57:00Z","receivedAt":"2022-09-27T13:57:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe 'git maintenance [un]register' commands set or unset the multi-\nvalued maintenance.repo config key with the absolute path of the current\nrepository. These are set in the global config file.\n\nInstead of calling a subcommand and creating a new process, create the\nproper API calls to git_config_set_multivar_in_file_gently(). It\nrequires loading the filename for the global config file (and erroring\nout if now $HOME value is set). We also need to be careful about using\nCONFIG_REGEX_NONE when adding the value and using\nCONFIG_FLAGS_FIXED_VALUE when removing the value. In both cases, we\ncheck that the value already exists (this check already existed for\n'unregister').\n\nAlso, remove the transparent translation of the error code from the\nconfig API to the exit code of 'git maintenance'. Instead, use die() to\nrecover from failures at that level. In the case of 'unregister\n--force', allow the CONFIG_NOTHING_SET error code to be a success. This\nallows a possible race where another process removes the config value.\nThe end result is that the config value is not set anymore, so we can\ntreat this as a success.\n\nReported-by: SZEDER Gábor \nSigned-off-by: Derrick Stolee \n---\n builtin/gc.c | 75 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 44 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex dc0ba9e3648..7a585f0b71d 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1470,11 +1470,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \tstruct option options[] = {\n \t\tOPT_END(),\n \t};\n-\tint rc;\n+\tint found = 0;\n+\tconst char *key = \"maintenance.repo\";\n \tchar *config_value;\n-\tstruct child_process config_set = CHILD_PROCESS_INIT;\n-\tstruct child_process config_get = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n+\tstruct string_list_item *item;\n+\tconst struct string_list *list;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_maintenance_register_usage, 0);\n@@ -1491,31 +1492,35 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \telse\n \t\tgit_config_set(\"maintenance.strategy\", \"incremental\");\n \n-\tconfig_get.git_cmd = 1;\n-\tstrvec_pushl(&config_get.args, \"config\", \"--global\", \"--get\",\n-\t\t \"--fixed-value\", \"maintenance.repo\", maintpath, NULL);\n-\tconfig_get.out = -1;\n-\n-\tif (start_command(&config_get)) {\n-\t\trc = error(_(\"failed to run 'git config'\"));\n-\t\tgoto done;\n+\tlist = git_config_get_value_multi(key);\n+\tif (list) {\n+\t\tfor_each_string_list_item(item, list) {\n+\t\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\t\tfound = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n \t}\n \n-\t/* We already have this value in our config! */\n-\tif (!finish_command(&config_get)) {\n-\t\trc = 0;\n-\t\tgoto done;\n+\tif (!found) {\n+\t\tint rc;\n+\t\tchar *user_config, *xdg_config;\n+\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tif (!user_config)\n+\t\t\tdie(_(\"$HOME not set\"));\n+\t\trc = git_config_set_multivar_in_file_gently(\n+\t\t\tuser_config, \"maintenance.repo\", maintpath,\n+\t\t\tCONFIG_REGEX_NONE, 0);\n+\t\tfree(user_config);\n+\t\tfree(xdg_config);\n+\n+\t\tif (rc)\n+\t\t\tdie(_(\"unable to add '%s' value of '%s'\"),\n+\t\t\t key, maintpath);\n \t}\n \n-\tconfig_set.git_cmd = 1;\n-\tstrvec_pushl(&config_set.args, \"config\", \"--add\", \"--global\", \"maintenance.repo\",\n-\t\t maintpath, NULL);\n-\n-\trc = run_command(&config_set);\n-\n-done:\n \tfree(maintpath);\n-\treturn rc;\n+\treturn 0;\n }\n \n static char const * const builtin_maintenance_unregister_usage[] = {\n@@ -1533,8 +1538,6 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tOPT_END(),\n \t};\n \tconst char *key = \"maintenance.repo\";\n-\tint rc = 0;\n-\tstruct child_process config_unset = CHILD_PROCESS_INIT;\n \tchar *maintpath = get_maintpath();\n \tint found = 0;\n \tstruct string_list_item *item;\n@@ -1557,17 +1560,27 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t}\n \n \tif (found) {\n-\t\tconfig_unset.git_cmd = 1;\n-\t\tstrvec_pushl(&config_unset.args, \"config\", \"--global\", \"--unset\",\n-\t\t\t \"--fixed-value\", key, maintpath, NULL);\n-\n-\t\trc = run_command(&config_unset);\n+\t\tint rc;\n+\t\tchar *user_config, *xdg_config;\n+\t\tgit_global_config(&user_config, &xdg_config);\n+\t\tif (!user_config)\n+\t\t\tdie(_(\"$HOME not set\"));\n+\t\trc = git_config_set_multivar_in_file_gently(\n+\t\t\tuser_config, key, NULL, maintpath,\n+\t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n+\t\tfree(user_config);\n+\t\tfree(xdg_config);\n+\n+\t\tif (rc &&\n+\t\t (!force || rc == CONFIG_NOTHING_SET))\n+\t\t\tdie(_(\"unable to unset '%s' value of '%s'\"),\n+\t\t\t key, maintpath);\n \t} else if (!force) {\n \t\tdie(_(\"repository '%s' is not registered\"), maintpath);\n \t}\n \n \tfree(maintpath);\n-\treturn rc;\n+\treturn 0;\n }\n \n static const char *get_frequency(enum schedule_priority schedule)\n-- \ngitgitgadget\n\n"},{"id":"463719","messageId":"73a262cdca46a45aeeda6f47ea3357aaeb937e7b.1664287021.git.gitgitgadget@gmail.com","threadId":"58460","inReplyTo":"pull.1358.v4.git.1664287021.gitgitgadget@gmail.com","subject":"[PATCH v4 4/4] string-list: document iterator behavior on NULL input","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T13:57:01Z","receivedAt":"2022-09-27T13:57:29Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe for_each_string_list_item() macro takes a string_list and\nautomatically constructs a for loop to iterate over its contents. This\nmacro will segfault if the list is non-NULL.\n\nWe cannot change the macro to be careful around NULL values because\nthere are many callers that use the address of a local variable, which\nwill never be NULL and will cause compile errors with -Werror=address.\n\nFor now, leave a documentation comment to try to avoid mistakes in the\nfuture where a caller does not check for a NULL list.\n\nSigned-off-by: Derrick Stolee \n---\n string-list.h | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/string-list.h b/string-list.h\nindex d5a744e1438..c7b0d5d0008 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n int for_each_string_list(struct string_list *list,\n \t\t\t string_list_each_func_t func, void *cb_data);\n \n-/** Iterate over each item, as a macro. */\n+/**\n+ * Iterate over each item, as a macro.\n+ *\n+ * Be sure that 'list' is non-NULL. The macro cannot perform NULL\n+ * checks due to -Werror=address errors.\n+ */\n #define for_each_string_list_item(item,list) \\\n \tfor (item = (list)->items; \\\n \t item && item < (list)->items + (list)->nr; \\\n-- \ngitgitgadget\n"},{"id":"463736","messageId":"xmqqtu4s7p9q.fsf@gitster.g","threadId":"58460","inReplyTo":"pull.1358.v4.git.1664287021.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/4] scalar: make unregister idempotent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T16:31:13Z","receivedAt":"2022-09-27T16:31:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> I noticed this while we were updating the microsoft/git fork to include\n> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent\n> before, but instead some change in 'scalar unregister' led to it relying on\n> the return code of 'git maintenance unregister'. Our functional tests expect\n> 'scalar unregister' to be idempotent, and I think that's a good pattern for\n> 'git maintenance unregister', so I'm fixing it at that layer.\n>\n> Despite finding this during the 2.38.0-rc0 integration, this isn't critical\n> to the release.\n>\n> Perhaps an argument could be made that \"failure means it wasn't registered\n> before\", but I think that isn't terribly helpful.\n\nI happen _not_ to share the idea that \"unregister is expected to be\nidempotent\" is a good pattern at all, and it is a better pattern to\nmake failure mean that the object specified to be acted upon did not\neven exist (cf. \"rm no-such-file\"). But that aside, does what the\nabove paragraphs mention still true for this round, namely, you are\n\"fixing\" it at that (i.e. \"maintenance unregister\") layer?\n\nThe cover letter does not become part of the permanent history, so\nit is not a huge deal as long as the reviewers know what they are\nlooking at ;-). Just leaving a note in case somebody who missed the\niterations up to v3 is now interested in the topic.\n\nThanks for a quick iteration.\n"},{"id":"463738","messageId":"xmqqmtak7ov7.fsf@gitster.g","threadId":"58460","inReplyTo":"73a262cdca46a45aeeda6f47ea3357aaeb937e7b.1664287021.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 4/4] string-list: document iterator behavior on NULL input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T16:39:56Z","receivedAt":"2022-09-27T16:40:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The for_each_string_list_item() macro takes a string_list and\n> automatically constructs a for loop to iterate over its contents. This\n> macro will segfault if the list is non-NULL.\n>\n> We cannot change the macro to be careful around NULL values because\n> there are many callers that use the address of a local variable, which\n> will never be NULL and will cause compile errors with -Werror=address.\n>\n> For now, leave a documentation comment to try to avoid mistakes in the\n> future where a caller does not check for a NULL list.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> string-list.h | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n\nFor exactly the -Werror=address reason, any other way that checks\nlist for NULL-ness cannot be done here (other than with Peff's\ninline helper that returns the first item or NULL), which is a bit\nof shame but this is totally outside the topic, and an additional\ncomment is a good thing to have here.\n\nThanks. All four patches look reasonable. Will queue (but after I\nmake sure I can tag and push out -rc2 today).\n\n\n> diff --git a/string-list.h b/string-list.h\n> index d5a744e1438..c7b0d5d0008 100644\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c\n> int for_each_string_list(struct string_list *list,\n> \t\t\t string_list_each_func_t func, void *cb_data);\n> \n> -/** Iterate over each item, as a macro. */\n> +/**\n> + * Iterate over each item, as a macro.\n> + *\n> + * Be sure that 'list' is non-NULL. The macro cannot perform NULL\n> + * checks due to -Werror=address errors.\n> + */\n> #define for_each_string_list_item(item,list) \\\n> \tfor (item = (list)->items; \\\n> \t item && item < (list)->items + (list)->nr; \\\n"},{"id":"463744","messageId":"9ccd46f1-4577-3075-db1c-a260fcf247ad@github.com","threadId":"58460","inReplyTo":"xmqqtu4s7p9q.fsf@gitster.g","subject":"Re: [PATCH v4 0/4] scalar: make unregister idempotent","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-27T16:54:08Z","receivedAt":"2022-09-27T16:55:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/2022 12:31 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> I noticed this while we were updating the microsoft/git fork to include\n>> v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent\n>> before, but instead some change in 'scalar unregister' led to it relying on\n>> the return code of 'git maintenance unregister'. Our functional tests expect\n>> 'scalar unregister' to be idempotent, and I think that's a good pattern for\n>> 'git maintenance unregister', so I'm fixing it at that layer.\n>>\n>> Despite finding this during the 2.38.0-rc0 integration, this isn't critical\n>> to the release.\n>>\n>> Perhaps an argument could be made that \"failure means it wasn't registered\n>> before\", but I think that isn't terribly helpful.\n> \n> I happen _not_ to share the idea that \"unregister is expected to be\n> idempotent\" is a good pattern at all, and it is a better pattern to\n> make failure mean that the object specified to be acted upon did not\n> even exist (cf. \"rm no-such-file\"). But that aside, does what the\n> above paragraphs mention still true for this round, namely, you are\n> \"fixing\" it at that (i.e. \"maintenance unregister\") layer?\n\nSorry, I forgot to update the cover letter when updating the title.\n\"git maintenance unregister\" will fail if the repository is not already\nregistered (unless --force is given). Now, \"scalar unregister\" _is_\nidempotent (it uses \"git maintenance unregister --force\").\n \n> The cover letter does not become part of the permanent history, so\n> it is not a huge deal as long as the reviewers know what they are\n> looking at ;-). Just leaving a note in case somebody who missed the\n> iterations up to v3 is now interested in the topic.\n> \n> Thanks for a quick iteration.\n\nThank you for your very careful review!\n\n-Stolee\n"}]}