Re: [PATCH v2 3/5] worktree: add upgrade_to_worktree_config()

2 messages, 2 authors, 2021-12-28 · open the first message on its own page

Re: [PATCH v2 3/5] worktree: add upgrade_to_worktree_config()

From: Junio C Hamano <hidden>
Date: 2021-12-22 05:43:50

"Derrick Stolee via GitGitGadget" [off-list ref] writes:
+int upgrade_to_worktree_config(struct repository *r)
Not a suggestion to rename the function, but does it mean "we have
been using a single configuration for all worktrees attached to the
repository, but we are switching to use per-worktree configuration
file"?  I am wondering if the phrase "worktree_config" is clear
enough for our future developers.
+{
+	int res;
+	int bare = 0;
+	struct config_set cs = { 0 };
+	char *base_config_file = xstrfmt("%s/config", r->commondir);
+	char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, base_config_file);
+
+	/*
+	 * If the base repository is bare, then we need to move core.bare=true
+	 * out of the base config file and into the base repository's
+	 * config.worktree file.
+	 */
+	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
+		if ((res = git_config_set_in_file_gently(base_worktree_file,
+							"core.bare", "true"))) {
+			error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
+			goto cleanup;
+		}
Hmph, I would have expected to see

		if (git_config_set_in_file_gently(...)) {
			res = error(_("..."));
                        goto cleanup;
		}

instead (obviously with "res" initialized to "0" to assume all will
be well at the beginning).

More importantly, are we prepared to see the repository 'r' that
already uses per-worktree config layout and refrain from causing any
damage to it, or are we all perfect developers that any caller that
calls this on repository 'r' that does not need to be upgraded is a
BUG()?

Is "core.bare" the only thing that needs special treatment?  Will it
stay to be, or will we see more configuration variables that need
special casing like this?
quoted hunk
+	if (upgrade_repository_format(r, 1) < 0) {
+		res = error(_("unable to upgrade repository format to enable worktreeConfig"));
+		goto cleanup;
+	}
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
+		error(_("failed to set extensions.worktreeConfig setting"));
+		goto cleanup;
+	}
+
+cleanup:
+	git_configset_clear(&cs);
+	free(base_config_file);
+	free(base_worktree_file);
+	trace2_printf("returning %d", res);
+	return res;
+}
diff --git a/worktree.h b/worktree.h
index 8b7c408132d..170b6b1e1f5 100644
--- a/worktree.h
+++ b/worktree.h
@@ -182,4 +182,16 @@ void strbuf_worktree_ref(const struct worktree *wt,
 			 struct strbuf *sb,
 			 const char *refname);
 
+/**
+ * Upgrade the config of the current repository and its base (if different
+ * from this repository) to use worktree-config. This might adjust config
+ * in both repositories, including:
+ *
+ * 1. Upgrading the repository format version to 1.
+ * 2. Adding extensions.worktreeConfig to the base config file.
+ * 3. Moving core.bare=true from the base config file to the base
+ *    repository's config.worktree file.
+ */
+int upgrade_to_worktree_config(struct repository *r);
+
 #endif

Re: [PATCH v2 3/5] worktree: add upgrade_to_worktree_config()

From: Derrick Stolee <hidden>
Date: 2021-12-28 15:13:29

On 12/22/2021 12:38 AM, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
quoted
+int upgrade_to_worktree_config(struct repository *r)
Not a suggestion to rename the function, but does it mean "we have
been using a single configuration for all worktrees attached to the
repository, but we are switching to use per-worktree configuration
file"?  I am wondering if the phrase "worktree_config" is clear
enough for our future developers.
quoted
+{
+	int res;
+	int bare = 0;
+	struct config_set cs = { 0 };
+	char *base_config_file = xstrfmt("%s/config", r->commondir);
+	char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, base_config_file);
+
+	/*
+	 * If the base repository is bare, then we need to move core.bare=true
+	 * out of the base config file and into the base repository's
+	 * config.worktree file.
+	 */
+	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
+		if ((res = git_config_set_in_file_gently(base_worktree_file,
+							"core.bare", "true"))) {
+			error(_("unable to set core.bare=true in '%s'"), base_worktree_file);
+			goto cleanup;
+		}
Hmph, I would have expected to see

		if (git_config_set_in_file_gently(...)) {
			res = error(_("..."));
                        goto cleanup;
		}

instead (obviously with "res" initialized to "0" to assume all will
be well at the beginning).
Deep down in the git_config_set_... stack, it returns helpful error
codes that I thought would be good to communicate upwards. cmd_config()
uses these error codes, too, but that's a more obvious place where the
user is expecting the error code to be related to the config operation.
 
If this communication is not helpful, then I will use the pattern you
suggest. I will assume this is the case unless told otherwise.
More importantly, are we prepared to see the repository 'r' that
already uses per-worktree config layout and refrain from causing any
damage to it, or are we all perfect developers that any caller that
calls this on repository 'r' that does not need to be upgraded is a
BUG()?
I don't think we should add burden to the callers to check that the
repo is not using worktree config. Thinking back to your rename idea
this could be "ensure_using_worktree_config()" to make it clear that
we will use the worktree config after the method, whether or not we
were using it beforehand.

I think this current implementation is non-damaging if someone calls
it after already using worktree config. The change being that a user
can go and add core.bare=false to the common config file and break all
worktrees again, and the current implementation will help heal that.
It's probably better to let the user have that ability to mess things
up and not try to fix something so broken.

Thanks,
-Stolee
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help