git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, vdye@github.com,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: [PATCH] maintenance: make unregister idempotent
Date: Tue, 20 Sep 2022 01:02:11 +0000	[thread overview]
Message-ID: <pull.1358.git.1663635732095.gitgitgadget@gmail.com> (raw)

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

             reply	other threads:[~2022-09-20  1:02 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20  1:02 Derrick Stolee via GitGitGadget [this message]
2022-09-21 17:19 ` [PATCH] maintenance: make unregister idempotent Junio C Hamano
2022-09-22 12:37   ` Derrick Stolee
2022-09-22 19:31     ` Junio C Hamano
2022-09-22 19:46       ` Derrick Stolee
2022-09-22 20:44         ` Junio C Hamano
2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-23 13:08     ` SZEDER Gábor
2022-09-26 13:32       ` Derrick Stolee
2022-09-26 15:39         ` Ævar Arnfjörð Bjarmason
2022-09-26 17:25           ` Derrick Stolee
2022-09-26 19:17             ` Junio C Hamano
2022-09-22 13:37   ` [PATCH v2 2/2] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48   ` [PATCH v3 0/3] scalar: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48     ` [PATCH v3 1/3] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-26 19:23       ` Junio C Hamano
2022-09-26 20:49         ` Derrick Stolee
2022-09-26 21:55       ` Junio C Hamano
2022-09-27 11:38         ` Derrick Stolee
2022-09-27 11:54           ` Derrick Stolee
2022-09-27 13:36             ` Ævar Arnfjörð Bjarmason
2022-09-27 13:55               ` Derrick Stolee
2022-09-26 18:48     ` [PATCH v3 2/3] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48     ` [PATCH v3 3/3] gc: replace config subprocesses with API calls Derrick Stolee via GitGitGadget
2022-09-26 19:27       ` Junio C Hamano
2022-09-27 13:56     ` [PATCH v4 0/4] scalar: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-27 13:56       ` [PATCH v4 1/4] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-27 13:56       ` [PATCH v4 2/4] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-27 13:57       ` [PATCH v4 3/4] gc: replace config subprocesses with API calls Derrick Stolee via GitGitGadget
2022-09-27 13:57       ` [PATCH v4 4/4] string-list: document iterator behavior on NULL input Derrick Stolee via GitGitGadget
2022-09-27 16:39         ` Junio C Hamano
2022-09-27 16:31       ` [PATCH v4 0/4] scalar: make unregister idempotent Junio C Hamano
2022-09-27 16:54         ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.1358.git.1663635732095.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).