git@vger.kernel.org mailing list mirror (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,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <derrickstolee@github.com>
Subject: [PATCH v3 3/3] gc: replace config subprocesses with API calls
Date: Mon, 26 Sep 2022 18:48:06 +0000	[thread overview]
Message-ID: <260d7bee36e1af2f6a6a8791d293402e97a502e6.1664218087.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1358.v3.git.1664218087.gitgitgadget@gmail.com>

From: Derrick Stolee <derrickstolee@github.com>

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 <szeder.dev@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 builtin/gc.c | 75 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 31 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 8d154586272..4c59235950d 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;
@@ -1554,17 +1557,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);
-
-		rc = run_command(&config_unset);
+		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);
+
+		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)
-- 
gitgitgadget

  parent reply	other threads:[~2022-09-26 18:48 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20  1:02 [PATCH] maintenance: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-21 17:19 ` 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     ` Derrick Stolee via GitGitGadget [this message]
2022-09-26 19:27       ` [PATCH v3 3/3] gc: replace config subprocesses with API calls 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=260d7bee36e1af2f6a6a8791d293402e97a502e6.1664218087.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.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).