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
next 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).