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

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

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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?

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