From 1ebe6b029703e4bebf413ec052918a81390797ac Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 27 Sep 2022 13:56:58 +0000 Subject: [PATCH 1/4] maintenance: add 'unregister --force' The 'git maintenance unregister' subcommand has a step that removes the current repository from the multi-valued maitenance.repo config key. This fails if the repository is not listed in that key. This makes running 'git maintenance unregister' twice result in a failure in the second instance. This failure exit code is helpful, but its message is not. Add a new die() message that explicitly calls out the failure due to the repository not being registered. In some cases, users may want to run 'git maintenance unregister' just to make sure that background jobs will not start on this repository, but they do not want to check to see if it is registered first. Add a new '--force' option that will siltently succeed if the repository is not already registered. Also add an extra test of 'git maintenance unregister' at a point where there are no registered repositories. This should fail without --force. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/git-maintenance.txt | 6 +++++- builtin/gc.c | 35 +++++++++++++++++++++++++------ t/t7900-maintenance.sh | 11 +++++++++- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/Documentation/git-maintenance.txt b/Documentation/git-maintenance.txt index 9c630efe19..bb888690e4 100644 --- a/Documentation/git-maintenance.txt +++ b/Documentation/git-maintenance.txt @@ -11,7 +11,7 @@ SYNOPSIS [verse] 'git maintenance' run [] 'git maintenance' start [--scheduler=] -'git maintenance' (stop|register|unregister) +'git maintenance' (stop|register|unregister) [] DESCRIPTION @@ -79,6 +79,10 @@ unregister:: Remove the current repository from background maintenance. This only removes the repository from the configured list. It does not stop the background maintenance processes from running. ++ +The `unregister` subcommand will report an error if the current repository +is not already registered. Use the `--force` option to return success even +when the current repository is not registered. TASKS ----- diff --git a/builtin/gc.c b/builtin/gc.c index 2753bd15a5..dc0ba9e364 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1519,18 +1519,26 @@ done: } static char const * const builtin_maintenance_unregister_usage[] = { - "git maintenance unregister", + "git maintenance unregister [--force]", NULL }; static int maintenance_unregister(int argc, const char **argv, const char *prefix) { + int force = 0; struct option options[] = { + OPT__FORCE(&force, + N_("return success even if repository was not registered"), + PARSE_OPT_NOCOMPLETE), OPT_END(), }; - int rc; + const char *key = "maintenance.repo"; + int rc = 0; struct child_process config_unset = CHILD_PROCESS_INIT; char *maintpath = get_maintpath(); + int found = 0; + struct string_list_item *item; + const struct string_list *list; argc = parse_options(argc, argv, prefix, options, builtin_maintenance_unregister_usage, 0); @@ -1538,11 +1546,26 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi usage_with_options(builtin_maintenance_unregister_usage, options); - config_unset.git_cmd = 1; - strvec_pushl(&config_unset.args, "config", "--global", "--unset", - "--fixed-value", "maintenance.repo", maintpath, NULL); + list = git_config_get_value_multi(key); + if (list) { + for_each_string_list_item(item, list) { + if (!strcmp(maintpath, item->string)) { + found = 1; + break; + } + } + } + + if (found) { + config_unset.git_cmd = 1; + strvec_pushl(&config_unset.args, "config", "--global", "--unset", + "--fixed-value", key, maintpath, NULL); + + rc = run_command(&config_unset); + } else if (!force) { + die(_("repository '%s' is not registered"), maintpath); + } - rc = run_command(&config_unset); free(maintpath); return rc; } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 2724a44fe3..96bdd42045 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -480,6 +480,11 @@ test_expect_success 'maintenance.strategy inheritance' ' test_expect_success 'register and unregister' ' test_when_finished git config --global --unset-all maintenance.repo && + + test_must_fail git maintenance unregister 2>err && + grep "is not registered" err && + git maintenance unregister --force && + git config --global --add maintenance.repo /existing1 && git config --global --add maintenance.repo /existing2 && git config --global --get-all maintenance.repo >before && @@ -493,7 +498,11 @@ test_expect_success 'register and unregister' ' git maintenance unregister && git config --global --get-all maintenance.repo >actual && - test_cmp before actual + test_cmp before actual && + + test_must_fail git maintenance unregister 2>err && + grep "is not registered" err && + git maintenance unregister --force ' test_expect_success !MINGW 'register and unregister with regex metacharacters' ' From d871b6c6c634689134a92d068bfb066d77d8429d Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 27 Sep 2022 13:56:59 +0000 Subject: [PATCH 2/4] scalar: make 'unregister' idempotent The 'scalar unregister' command removes a repository from the list of registered Scalar repositories and removes it from the list of repositories registered for background maintenance. If the repository was not already registered for background maintenance, then the command fails, even if the repository was still registered as a Scalar repository. After using 'scalar clone' or 'scalar register', the repository would be enrolled in background maintenance since those commands run 'git maintenance start'. If the user runs 'git maintenance unregister' on that repository, then it is still in the list of repositories which get new config updates from 'scalar reconfigure'. The 'scalar unregister' command would fail since 'git maintenance unregister' would fail. Further, the add_or_remove_enlistment() method in scalar.c already has this idempotent nature built in as an expectation since it returns zero when the scalar.repo list already has the proper containment of the repository. The previous change added the 'git maintenance unregister --force' option, so use it within 'scalar unregister' to make it idempotent. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- scalar.c | 5 ++++- t/t9210-scalar.sh | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/scalar.c b/scalar.c index c5c1ce6891..6de9c0ee52 100644 --- a/scalar.c +++ b/scalar.c @@ -207,7 +207,10 @@ static int set_recommended_config(int reconfigure) static int toggle_maintenance(int enable) { - return run_git("maintenance", enable ? "start" : "unregister", NULL); + return run_git("maintenance", + enable ? "start" : "unregister", + enable ? NULL : "--force", + NULL); } static int add_or_remove_enlistment(int add) diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh index 14ca575a21..be51a8bb7a 100755 --- a/t/t9210-scalar.sh +++ b/t/t9210-scalar.sh @@ -116,7 +116,10 @@ test_expect_success 'scalar unregister' ' test_must_fail git config --get --global --fixed-value \ maintenance.repo "$(pwd)/vanish/src" && scalar list >scalar.repos && - ! grep -F "$(pwd)/vanish/src" scalar.repos + ! grep -F "$(pwd)/vanish/src" scalar.repos && + + # scalar unregister should be idempotent + scalar unregister vanish ' test_expect_success 'set up repository to clone' ' From 50a044f1e407ef217cb5d057af08e9e87daddc78 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 27 Sep 2022 13:57:00 +0000 Subject: [PATCH 3/4] gc: replace config subprocesses with API calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'git maintenance [un]register' commands set or unset the multi- valued maintenance.repo config key with the absolute path of the current repository. These are set in the global config file. Instead of calling a subcommand and creating a new process, create the proper API calls to git_config_set_multivar_in_file_gently(). It requires loading the filename for the global config file (and erroring out if now $HOME value is set). We also need to be careful about using CONFIG_REGEX_NONE when adding the value and using CONFIG_FLAGS_FIXED_VALUE when removing the value. In both cases, we check that the value already exists (this check already existed for 'unregister'). Also, remove the transparent translation of the error code from the config API to the exit code of 'git maintenance'. Instead, use die() to recover from failures at that level. In the case of 'unregister --force', allow the CONFIG_NOTHING_SET error code to be a success. This allows a possible race where another process removes the config value. The end result is that the config value is not set anymore, so we can treat this as a success. Reported-by: SZEDER Gábor Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/gc.c | 73 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index dc0ba9e364..7a585f0b71 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1470,11 +1470,12 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) struct option options[] = { OPT_END(), }; - int rc; + int found = 0; + const char *key = "maintenance.repo"; char *config_value; - struct child_process config_set = CHILD_PROCESS_INIT; - struct child_process config_get = CHILD_PROCESS_INIT; char *maintpath = get_maintpath(); + struct string_list_item *item; + const struct string_list *list; argc = parse_options(argc, argv, prefix, options, builtin_maintenance_register_usage, 0); @@ -1491,31 +1492,35 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) else git_config_set("maintenance.strategy", "incremental"); - config_get.git_cmd = 1; - strvec_pushl(&config_get.args, "config", "--global", "--get", - "--fixed-value", "maintenance.repo", maintpath, NULL); - config_get.out = -1; - - if (start_command(&config_get)) { - rc = error(_("failed to run 'git config'")); - goto done; + list = git_config_get_value_multi(key); + if (list) { + for_each_string_list_item(item, list) { + if (!strcmp(maintpath, item->string)) { + found = 1; + break; + } + } } - /* We already have this value in our config! */ - if (!finish_command(&config_get)) { - rc = 0; - goto done; + if (!found) { + int rc; + char *user_config, *xdg_config; + git_global_config(&user_config, &xdg_config); + if (!user_config) + die(_("$HOME not set")); + rc = git_config_set_multivar_in_file_gently( + user_config, "maintenance.repo", maintpath, + CONFIG_REGEX_NONE, 0); + free(user_config); + free(xdg_config); + + if (rc) + die(_("unable to add '%s' value of '%s'"), + key, maintpath); } - config_set.git_cmd = 1; - strvec_pushl(&config_set.args, "config", "--add", "--global", "maintenance.repo", - maintpath, NULL); - - rc = run_command(&config_set); - -done: free(maintpath); - return rc; + return 0; } static char const * const builtin_maintenance_unregister_usage[] = { @@ -1533,8 +1538,6 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi OPT_END(), }; const char *key = "maintenance.repo"; - int rc = 0; - struct child_process config_unset = CHILD_PROCESS_INIT; char *maintpath = get_maintpath(); int found = 0; struct string_list_item *item; @@ -1557,17 +1560,27 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi } if (found) { - config_unset.git_cmd = 1; - strvec_pushl(&config_unset.args, "config", "--global", "--unset", - "--fixed-value", key, maintpath, NULL); + int rc; + char *user_config, *xdg_config; + git_global_config(&user_config, &xdg_config); + if (!user_config) + die(_("$HOME not set")); + rc = git_config_set_multivar_in_file_gently( + user_config, key, NULL, maintpath, + CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); + free(user_config); + free(xdg_config); - rc = run_command(&config_unset); + if (rc && + (!force || rc == CONFIG_NOTHING_SET)) + die(_("unable to unset '%s' value of '%s'"), + key, maintpath); } else if (!force) { die(_("repository '%s' is not registered"), maintpath); } free(maintpath); - return rc; + return 0; } static const char *get_frequency(enum schedule_priority schedule) From d151f0cce7fca1fc156a9ea1dc98c59e1be512c9 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 27 Sep 2022 13:57:01 +0000 Subject: [PATCH 4/4] string-list: document iterator behavior on NULL input The for_each_string_list_item() macro takes a string_list and automatically constructs a for loop to iterate over its contents. This macro will segfault if the list is non-NULL. We cannot change the macro to be careful around NULL values because there are many callers that use the address of a local variable, which will never be NULL and will cause compile errors with -Werror=address. For now, leave a documentation comment to try to avoid mistakes in the future where a caller does not check for a NULL list. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- string-list.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/string-list.h b/string-list.h index d5a744e143..c7b0d5d000 100644 --- a/string-list.h +++ b/string-list.h @@ -141,7 +141,12 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c int for_each_string_list(struct string_list *list, string_list_each_func_t func, void *cb_data); -/** Iterate over each item, as a macro. */ +/** + * Iterate over each item, as a macro. + * + * Be sure that 'list' is non-NULL. The macro cannot perform NULL + * checks due to -Werror=address errors. + */ #define for_each_string_list_item(item,list) \ for (item = (list)->items; \ item && item < (list)->items + (list)->nr; \