Re: [PATCH 3/4] config: add repo_config_set_worktree_gently()

On 12/21/2021 12:53 AM, Eric Sunshine wrote:

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

When adding config values to the worktree config, we might enable the
extensions.worktreeConfig setting and create the config.worktree file
for the first time. When the base repository is bare, this creates a
change of behavior for determining if the worktree is bare or not. A
worktree off of a bare repository is assumed to be non-bare when
extensions.worktreeConfig is disabled. When extensions.worktreeConfig is
enabled but config.worktree is empty, the worktree is considered bare
because the base repo's core.bare=true setting is used.

To avoid issues like this, create a helper that initializes all the
right settings in the correct order. A caller will be added in the next
change.

As discussed already in [1], [2], and [3], the solution implemented by
this patch is undesirable, and I gave an outline in [4] about how I
think the new utility function ought to be implemented instead, so I
won't say anything further about that here. However, I do still have
one or two review comments to make about the general approach taken by
patch. See below...

[1]: https://lore.kernel.org/git/CAPig+cQPUe9REf+wgVNjyak_nk3V361h-48rTFgk6TGC7vJgOA@mail.gmail.com/ (local)
[2]: https://lore.kernel.org/git/CAPig+cTVzMtiHzkJq7VRg4Xa3xhrq7KKCdK5OSDY6bvwKu_ynA@mail.gmail.com/ (local)
[3]: https://lore.kernel.org/git/6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com/ (local)
[4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/ (local)

quoted

Signed-off-by: Derrick Stolee <redacted>
---

diff --git a/config.c b/config.c

@@ -2880,6 +2880,33 @@ int git_config_set_gently(const char *key, const char *value)
+int repo_config_set_worktree_gently(struct repository *r,
+                                   const char *key, const char *value)
+{
+       int res;
+       const char *config_filename = repo_git_path(r, "config.worktree");
+
+       /*
+        * Ensure that core.bare reflects the current worktree, since the
+        * logic for is_bare_repository() changes if extensions.worktreeConfig
+        * is disabled.
+        */
+       if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
+                                                         r->worktree ? "false" : "true",
+                                                         NULL, 0))) {
+               error(_("unable to set core.bare setting in worktree config"));
+               return res;
+       }
+       if (upgrade_repository_format(r, 1) < 0)
+               return error(_("unable to upgrade repository format to enable worktreeConfig"));
+       if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
+               error(_("failed to set extensions.worktreeConfig setting"));
+               return res;
+       }
+
+       return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
+}

diff --git a/config.h b/config.h

@@ -253,6 +253,12 @@ void git_config_set_in_file(const char *, const char *, const char *);
+/**
+ * Write a config value into the config.worktree file for the current
+ * worktree. This will initialize extensions.worktreeConfig if necessary.
+ */
+int repo_config_set_worktree_gently(struct repository *, const char *, const char *);

I understand your desire to make this "setter" function as transparent
and simple for clients as possible, however, I think it does too much
by conflating two very distinct operations (one which changes the
nature of the repository itself, and one which simply sets a config
variable), and is far too magical. It doesn't help that the function
name gives no indication of just how magical it is, and it is easy to
imagine people calling this function thinking that it's just a simple
"config setter" like all the other similarly-named functions, without
realizing the impact it may have on the repository overall (i.e.
upgrading to version 1 and changing to per-worktree config).

I would feel much more comfortable for the new utility function to
have a single-purpose: namely, to upgrade the repository to a
per-worktree configuration mode (if not already upgraded) as outlined
in [4]. That's it. It shouldn't do anything other than that. This way,
callers which need per-worktree configuration must call the new
function explicitly to obtain the desired behavior, rather than
getting per-worktree configuration as a magical and implicit
side-effect of calling what looks like a plain old "config setter".
This should make it easier to reason about. Taking this approach also
means that you don't need to introduce a special-purpose "config
setter" just for worktrees; instead, you'd use the normal existing
"config setter" functions. For instance, if the new utility function
is named enable_per_worktree_config(), then `git sparse-checkout init`
might do something like this:

I understand your desire to separate these concerns, and maybe there
is value in having another method that _just_ does the "upgrade to
worktree config". However, if we don't also create this helper method
for setting worktree-specific config, then we are going to hit this
issue again.
 

    enable_per_worktree_config(r);
    config_path = repo_git_path(r, "config.worktree")
    git_config_set_in_file_gently(config_path, "core.sparseCheckout", ...);
    git_config_set_in_file_gently(config_path, "core.sparseCheckoutCone", ...);

(This, of course, assumes that repo_git_path() latches the changes
made by enable_per_worktree_config() so that it "does the right
thing", but it seems that existing code in `git sparse-checkout init`
is already expecting it to do so, so perhaps it does work that way.)

If we expect every caller that assigns config to the worktree to follow
this sequence of events, then we should encapsulate that in a method so
developers can discover it and call it instead of needing to write these
lines over again. Just repeating the literal "config.worktree" in
multiple places is enough justification for making a helper, let alone
these more subtle issues around bare repos and non-bare worktrees.

The helper method will need clear documentation to say "this will upgrade
the repository format and add extensions.worktreeConfig" so those new
consumers are aware of the full functionality.

Thanks,
-Stolee