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

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