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

[Derrick Stolee <stolee@gmail.com>]

On 12/21/2021 12:53 AM, Eric Sunshine wrote:
> On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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/
> [2]: https://lore.kernel.org/git/CAPig+cTVzMtiHzkJq7VRg4Xa3xhrq7KKCdK5OSDY6bvwKu_ynA@mail.gmail.com/
> [3]: https://lore.kernel.org/git/6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com/
> [4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/
> 
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> 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