On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxxxxx/ [2]: https://lore.kernel.org/git/CAPig+cTVzMtiHzkJq7VRg4Xa3xhrq7KKCdK5OSDY6bvwKu_ynA@xxxxxxxxxxxxxx/ [3]: https://lore.kernel.org/git/6d72a020-ded7-6ef2-825c-ce6421194b26@xxxxxxxxx/ [4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@xxxxxxxxxxxxxx/ > Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx> > --- > 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: 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.)
