[PATCH] maintenance: make unregister idempotent

From: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Date: Sep 20, 2022, 01:02 UTC
Message-ID: <pull.1358.git.1663635732095.gitgitgadget@gmail.com>

From: Derrick Stolee <derrickstolee@github.com>

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.

Make this task idempotent, since the end result is the same in both cases: maintenance will no longer run on this repository.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
    maintenance: make unregister idempotent
    
    I noticed this while we were updating the microsoft/git fork to include
    v2.38.0-rc0. I don't think 'git maintenance unregister' was idempotent
    before, but instead some change in 'scalar unregister' led to it relying
    on the return code of 'git maintenance unregister'. Our functional tests
    expect 'scalar unregister' to be idempotent, and I think that's a good
    pattern for 'git maintenance unregister', so I'm fixing it at that
    layer.
    
    Despite finding this during the 2.38.0-rc0 integration, this isn't
    critical to the release.
    
    Perhaps an argument could be made that "failure means it wasn't
    registered before", but I think that isn't terribly helpful.
    
    Our functional tests are running the unregister subcommand to disable
    maintenance in order to run tests on the object store (such as running
    maintenance commands in the foreground and checking the object store
    afterwards). This is a form of automation using 'unregister' as a check
    that maintenance will not run at the same time, and it doesn't care if
    maintenance was already disabled. I can imagine other scripting
    scenarios wanting that kind of guarantee.
    
    Thanks, -Stolee

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1358%2Fderrickstolee%2Fmaintenance-unregister-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1358/derrickstolee/maintenance-unregister-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1358

 builtin/gc.c           | 24 +++++++++++++++++++-----
 t/t7900-maintenance.sh |  5 ++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 0accc024067..787e9c702b2 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1528,9 +1528,13 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 	struct option options[] = {
 		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 = git_config_get_value_multi(key);
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_maintenance_unregister_usage, 0);
@@ -1538,11 +1542,21 @@ 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);
+	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);
+	}
 
-	rc = run_command(&config_unset);
 	free(maintpath);
 	return rc;
 }
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index 2724a44fe3e..3747e4a14f8 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -493,7 +493,10 @@ 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 &&
+
+	# Expect unregister to be idempotent.
+	git maintenance unregister
 '
 
 test_expect_success !MINGW 'register and unregister with regex metacharacters' '

base-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df

-- 
gitgitgadget