Re: [PATCH v6 2/6] worktree: create init_worktree_config()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2022, 22:09 UTC
Message-ID: <xmqq1r0dc8om.fsf@gitster.g>
In-Reply-To: <5d0cc242d92c68bf239f9e17eab9c80ec6b2d469.1644269583.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

> +static int move_config_setting(const char *key, const char *value,
> +			       const char *from_file, const char *to_file)
> +{
> +	if (git_config_set_in_file_gently(to_file, key, value))
> +		return error(_("unable to set %s in '%s'"), key, to_file);
> +	if (git_config_set_in_file_gently(from_file, key, NULL))
> +		return error(_("unable to unset %s in '%s'"), key, from_file);
> +	return 0;
> +}

Interesting.

The verb "move" in its name made me expect a "get (and remove) whatever value(s) defined out of the old file, and set them identically in the new file" sequence, but that is not what is done here. "set to this new single value in the new file and unset from the old one".

I can see the need to say "move it only when its value is X", so having the caller to extract the value before deciding to call the function (hence not "moving from old") does make sense, but then the function is misnamed---it is not "moving", it is doing something else.

Show 15 quoted lines

> +int init_worktree_config(struct repository *r)
> +{
> +	int res = 0;
> +	int bare = 0;
> +	struct config_set cs = { { 0 } };
> +	const char *core_worktree;
> +	char *common_config_file;
> +	char *main_worktree_file;
> +
> +	/*
> +	 * If the extension is already enabled, then we can skip the
> +	 * upgrade process.
> +	 */
> +	if (repository_format_worktree_config)
> +		return 0;

OK.

> +	if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
> +		return error(_("failed to set extensions.worktreeConfig setting"));

OK.

Show 12 quoted lines

> +	common_config_file = xstrfmt("%s/config", r->commondir);
> +	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> +
> +	git_configset_init(&cs);
> +	git_configset_add_file(&cs, common_config_file);
> +
> +	/*
> +	 * If core.bare is true in the common config file, then we need to
> +	 * move it to the main worktree's config file or it will break all
> +	 * worktrees. If it is false, then leave it in place because it
> +	 * _could_ be negating a global core.bare=true.
> +	 */

Is the assumption that the secondary worktrees are never bare, but the primary one could be (iow, adding worktrees to a bare repository would leave the original bare repository as the primary "worktree" that does not have "working tree")? I am trying to see what downsides it tries to avoid by not moving the core.bare==false setting. Shouldn't core.bare be set to false when "worktree add" creates a new one anyway, if the secondaries are never bare?

Show 6 quoted lines

> +	if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) {
> +		if ((res = move_config_setting("core.bare", "true",
> +					       common_config_file,
> +					       main_worktree_file)))
> +			goto cleanup;
> +	}

> +	/*
> +	 * If core.worktree is set, then the main worktree is located
> +	 * somewhere different than the parent of the common Git dir.

OK. We do not want to share the working tree for the primary worktree among secondary worktrees. For the primary, common and uncommon are the same, so it may not matter, but mention of "common Git dir" here may confuse readers? Unless overridden by the config, the parent of the git dir is the root of the working tree, no?

> +	 * Relocate that value to avoid breaking all worktrees with this
> +	 * upgrade to worktree config.
> +	 */

And if it is not set, then working tree of each worktree is the parent of the per-worktree Git dir, so they will automatically become separate, which makes sense.

Show 49 quoted lines

> +	if (!git_configset_get_value(&cs, "core.worktree", &core_worktree)) {
> +		if ((res = move_config_setting("core.worktree", core_worktree,
> +					       common_config_file,
> +					       main_worktree_file)))
> +			goto cleanup;
> +	}
> +
> +	/*
> +	 * Ensure that we use worktree config for the remaining lifetime
> +	 * of the current process.
> +	 */
> +	repository_format_worktree_config = 1;
> +
> +cleanup:
> +	git_configset_clear(&cs);
> +	free(common_config_file);
> +	free(main_worktree_file);
> +	return res;
> +}
> diff --git a/worktree.h b/worktree.h
> index 9e06fcbdf3d..e9e839926b0 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -183,4 +183,25 @@ void strbuf_worktree_ref(const struct worktree *wt,
>  			 struct strbuf *sb,
>  			 const char *refname);
>  
> +/**
> + * Enable worktree config for the first time. This will make the following
> + * adjustments:
> + *
> + * 1. Add extensions.worktreeConfig=true in the common config file.
> + *
> + * 2. If the common config file has a core.worktree value, then that value
> + *    is moved to the main worktree's config.worktree file.
> + *
> + * 3. If the common config file has a core.bare enabled, then that value
> + *    is moved to the main worktree's config.worktree file.
> + *
> + * If extensions.worktreeConfig is already true, then this method
> + * terminates early without any of the above steps. The existing config
> + * arrangement is assumed to be intentional.
> + *
> + * Returns 0 on success. Reports an error message and returns non-zero
> + * if any of these steps fail.
> + */
> +int init_worktree_config(struct repository *r);
> +
>  #endif