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

[Junio C Hamano <gitster@pobox.com>]

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

> +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.

> +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.

> +	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?

> +	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.

> +	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