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

On 12/21/2021 7:45 PM, Eric Sunshine wrote:

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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

Bare worktrees don't exist, that is correct. But if one existed it
would be a directory where you could operate as if it is a bare repo,
but it has its own HEAD different from the base repo's HEAD. Not sure
why one would want it.

I guess the question is: what happens if someone writes core.bare=true
into their worktree config? A question to resolve another day, perhaps.
 

quoted

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:

Good suggestions to add the necessary context here. Thanks.

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

quoted

Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---

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.

That would be good. I originally erred on the side of least complicated
but slower because this is not run very often.

quoted

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

quoted

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

'repo_config' is too generic, because I want the worktree config for
the "original" repo. I chose to call that the "base" repo and its
worktree config. Shared_config is a good name, though.

quoted

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

Yes, but specific to the original/root/bare repo. I'm open to preferences
here, but "repo" isn't specific enough.

quoted

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

It took me a long time to actually understand the purpose of
core.worktree, since it seems in conflict with the 'git worktree'
feature. Indeed, it is special-cased the same way core.bare is, so
this relocation is required.

quoted

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

This order can still cause some issues, specifically the worktree will
still think it is bare or the core.worktree value is incorrect, but that
is less serious than a broken base repo.

quoted

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

Is this leftover debugging or intentional?

Leftover debugging. Thanks for catching.

Thanks,
-Stolee