Re: [PATCH v4 2/5] worktree: create init_worktree_config()

[Elijah Newren <newren@gmail.com>]

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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@microsoft.com>
> ---
>  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.