On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
>
> Upgrading a repository to use extensions.worktreeConfig is non-trivial.
> There are several steps involved, including moving some config settings
> from the common config file to the main worktree's config.worktree file.
> The previous change updated the documentation with all of these details.
>
> Commands such as 'git sparse-checkout set' upgrade the repository to use
> extensions.worktreeConfig without following these steps, causing some
> user pain in some special cases.
>
> Create a helper method, init_worktree_config(), that will be used in a
> later change to fix this behavior within 'git sparse-checkout set'. The
> method is carefully documented in worktree.h.
I was curious why you were only fixing `set` and not `init`, but I
looked ahead and it appears you are fixing both, since both use
set_config(). And I can see leaving out the mention of `init` since
it's deprecated. Anyway, it's all good here, I'm basically just
thinking out loud...
> Note that we do _not_ upgrade the repository format version to 1 during
> this process. The worktree config extension must be considered by Git
> and third-party tools even if core.repositoryFormatVersion is 0 for
> historical reasons documented in 11664196ac ("Revert
> "check_repository_format_gently(): refuse extensions for old
> repositories"", 2020-07-15). This is a special case for this extension,
> and newer extensions (such as extensions.objectFormat) still need to
> upgrade the repository format version.
>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> ---
> worktree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> worktree.h | 19 +++++++++++++++
> 2 files changed, 89 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 6f598dcfcdf..dc4ead4c8fb 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -5,6 +5,7 @@
> #include "worktree.h"
> #include "dir.h"
> #include "wt-status.h"
> +#include "config.h"
>
> void free_worktrees(struct worktree **worktrees)
> {
> @@ -826,3 +827,72 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
> *wtpath = path;
> return 0;
> }
> +
> +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;
> +}
> +
> +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 = xstrfmt("%s/config", r->commondir);
> + char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
> +
> + /*
> + * If the extension is already enabled, then we can skip the
> + * upgrade process.
> + */
> + if (repository_format_worktree_config)
> + return 0;
> + if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
> + return error(_("failed to set extensions.worktreeConfig setting"));
> +
> + 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 base 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.
> + */
> + 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 base worktree is located
> + * somewhere different than the parent of the common Git dir.
> + * Relocate that value to avoid breaking all worktrees with this
> + * upgrade to worktree config.
> + */
> + if (!git_configset_get_string_tmp(&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..5ea5fcc3647 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -183,4 +183,23 @@ 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 or core.bare is
> + * set to true, then those values are moved to the main worktree's
> + * config.worktree file.
This is a bit ambiguous. If core.worktree is set and core.bare is
false, are both moved or only one? I'm afraid folks won't understand
that it's just one from this description.
> + *
> + * 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
> --
Other than the ambiguity in worktree.h, this patch looks solid.
