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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Some features, such as the sparse-checkout builtin, require using the
> worktree config extension. It might seem simple to upgrade the
> repository format and add extensions.worktreeConfig, and that is what
> happens in the sparse-checkout builtin.
>
> Transitioning from one config file to multiple has some strange
> side-effects. In particular, if the base repository is bare and the
> worktree is not, Git knows to treat the worktree as non-bare as a
> special case when not using worktree config. Once worktree config is
> enabled, Git stops that special case since the core.bare setting could
> apply at the worktree config level. This opens the door for bare
> worktrees.

It would be a good idea to drop the final sentence since there is no
such thing as a bare worktree (either conceptually or practically),
and end the first sentence at "case": i.e. "... stops that special
case."

> To help resolve this transition, create upgrade_to_worktree_config() to
> navigate the intricacies of this operation. In particular, we need to
> look for core.bare=true within the base config file and move that
> setting into the core repository's config.worktree file.
>
> To gain access to the core repository's config and config.worktree file,
> we reference a repository struct's 'commondir' member. If the repository
> was a submodule instead of a worktree, then this still applies
> correctly.

I'm not sure how much this commit message is going to help someone who
did not participate in the discussion which led to this patch series.
I think the entire commit message could be written more concisely like
this:

    worktree: add helper to upgrade repository to per-worktree config

    Changing a repository to use per-worktree configuration is a
    somewhat involved manual process, as described in the
    `git-worktree` documentation, but it's easy to get wrong if the
    steps are not followed precisely, which could lead to odd
    anomalies such as Git thinking that a worktree is "bare" (which
    conceptually makes no sense). Therefore, add a utility function to
    automate the process of switching to per-worktree configuration
    for modules which require such configuration. (In the future, it
    may make sense to also expose this convenience as a `git-worktree`
    command to automate the process for end-users, as well.)

    To upgrade the repository to per-worktree configuration, it performs
    these steps:

    * enable `extensions.worktreeConfig` in .git/config

    * relocate `core.bare` from .git/config to .git/config.worktree
      (if key exists)

    * relocate `core.worktree` from .git/config to
      .git/config.worktree (if key exists)

    It also upgrades the repository format to 1 if necessary since
    that is a prerequisite of using `extensions`.

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -830,3 +831,49 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
> +int upgrade_to_worktree_config(struct repository *r)
> +{
> +       int res;
> +       int bare = 0;
> +       struct config_set cs = { 0 };

This function is doing a lot of unnecessary work if per-worktree
configuration is already enabled. The very first thing it should be
doing is checking whether or not it actually needs to do that work,
and short-circuit if it doesn't. I would think that simply checking
whether `extensions.worktreeConfig` is true and returning early if it
is would be sufficient.

> +       char *base_config_file = xstrfmt("%s/config", r->commondir);

If we look at path.c:strbuf_worktree_gitdir(), we see that this should
be using `r->gitdir`, not `r->commondir`.

> +       char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir);

Per path.c:strbuf_worktree_gitdir(), this use of `r->commondir` is
correct. Good.

Can we use more meaningful variable names? It's not at all clear what
"base" means in this context (I don't think it has any analog in Git
terminology). Perhaps name these `shared_config` and `repo_config`,
respectively.

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

Here, too, it's not clear what "base" means. I think you want to say
that it needs to "move `core.bare` from the shared config to the
repo-specific config".

> +       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;
> +               }
> +
> +               if ((res = git_config_set_in_file_gently(base_config_file,
> +                                                       "core.bare", NULL))) {
> +                       error(_("unable to unset core.bare=true in '%s'"), base_config_file);
> +                       goto cleanup;
> +               }
> +       }

This seems unnecessarily complicated and overly specific, thus
potentially confusing. The requirements laid out in git-worktree.txt
say only to move the configuration key from .git/config to
.git/config.worktree; it doesn't add any qualifiers about the value
being "true". And, indeed, we should not care about the value; it's
immaterial to this operation. Instead, we should just treat it
opaquely and relocate the key and value from .git/config (if it
exists) to .git/config.worktree without interpretation.

The error messages are too specific, as well, by mentioning "true".
They could instead be:

    unable to set `core.bare` in '%s'

and

    unable to remove `core.bare` from '%s'

However, there is a much more _severe_ problem with this
implementation: it is incomplete. As documented in git-worktree.txt
(and mentioned several times during this discussion), this utility
function _must_ relocate both `core.bare` _and_ `core.worktree` (if
they exist) from .git/config to .git/config.worktree. This
implementation neglects to relocate `core.worktree`, which can leave
things in quite a broken state (just as neglecting to relocate
`core.bare` can).

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

The order in which this function performs its operations can leave the
repository in a broken state if any of the steps fails. For instance,
if setting `extensions.worktreeConfig=true` fails _after_ you've
relocated `core.bare` (and `core.worktree`) to .git/config.worktree,
then those configuration values will no longer be "active" since the
config system won't consult .git/config.worktree without
`extensions.worktreeConfig` enabled.

To be resilient against this sort of problem, you should perform the
operations in this order:

(1) upgrade repository format to 1
(2) enable `extensions.worktreeConfig`
(3) relocate `core.bare` and `core.worktree`

> +cleanup:
> +       git_configset_clear(&cs);
> +       free(base_config_file);
> +       free(base_worktree_file);
> +       trace2_printf("returning %d", res);

Is this leftover debugging or intentional?

> +       return res;
> +}
> diff --git a/worktree.h b/worktree.h
> @@ -182,4 +182,16 @@ void strbuf_worktree_ref(const struct worktree *wt,
> +/**
> + * 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:

Here, too, it's not clear what "base" means. Moreover, this seems to
be talking about multiple repositories, but we're only dealing with a
single repository and zero or more worktrees, so it's not clear what
this is trying to say.

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

As mentioned above, it's unnecessary and perhaps confusing to focus
only on "true" here; we should be treating the value opaquely.

Also, if you're talking about the specific config settings which this
relocates, then `core.worktree` should be mentioned too, not just
`core.bare`.

> + */
> +int upgrade_to_worktree_config(struct repository *r);