Re: [PATCH v2 4/5] config: add repo_config_set_worktree_gently()
From: Eric Sunshine <hidden>
Date: 2021-12-22 01:11:53
On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget [off-list ref] wrote:
The previous change added upgrade_to_worktree_config() to assist creating a worktree-specific config for the first time. However, this requires every config writer to care about that upgrade before writing to the worktree-specific config. In addition, callers need to know how to generate the name of the config.worktree file and pass it to the config API. To assist, create a new repo_config_set_worktree_gently() method in the config API that handles the upgrade_to_worktree_config() method in addition to assigning the value in the worktree-specific config. This will be consumed by an upcoming change.
I still feel very uncomfortable about this function conflating two very different concerns (upgrading the repository to per-worktree config, and the simple setting of a config variable). Since I've already explained my discomfort and suggested alternatives several times during this discussion (most recently at [1]), I don't have all that much more to say. However, I do have a few comments... First, I would have no problem if this function "did the right thing" where "the right thing" means correctly choosing between .git/config and .git/config.worktree depending upon whether or not `extensions.worktreeConfig` is set, and only that. It should not perform any sort of repository upgrade on its own. Doing it this way should satisfy your major concern of relieving callers from having to figure out the correct configuration file name. Second, if you absolutely must have this function, as implemented, as part of the public API (despite my protests), please give it a name which is sufficiently different from the other "config setter" functions so that people won't be confused into thinking it's just another "setter" without realizing that calling it has repository-wide consequences. Third, I don't have an objection if you want to make this function private (static) to builtin/sparse-checkout.c, thus omitting it from the public API. This way you can have its convenience where you want it, and we can delay finishing this discussion until such time that it becomes apparent that other modules want its convenience, as well, if that ever comes about. [1]: https://lore.kernel.org/git/CAPig+cRombN-8g0t7Hs9qQypJoY41gK3+kvypH4D0G6EB4JgbQ@mail.gmail.com/ (local)
quoted hunk ↗ jump to hunk
Signed-off-by: Derrick Stolee <redacted> ---diff --git a/config.c b/config.c index 9c9eef16018..81f3a689c11 100644 --- a/config.c +++ b/config.c@@ -21,6 +21,7 @@ #include "dir.h" #include "color.h" #include "refs.h" +#include "worktree.h" struct config_source { struct config_source *prev;@@ -2880,6 +2881,15 @@ int git_config_set_gently(const char *key, const char *value) return git_config_set_multivar_gently(key, value, NULL, 0); } +int repo_config_set_worktree_gently(struct repository *r, + const char *key, const char *value) +{ + return upgrade_to_worktree_config(r) || + git_config_set_multivar_in_file_gently( + repo_git_path(r, "config.worktree"), + key, value, NULL, 0); +} + void git_config_set(const char *key, const char *value) { repo_config_set(the_repository, key, value);diff --git a/config.h b/config.h index 5531fc018e3..b05c51b3528 100644 --- a/config.h +++ b/config.h@@ -253,6 +253,13 @@ void git_config_set_in_file(const char *, const char *, const char *); int git_config_set_gently(const char *, const char *); +/** + * Write a config value into the config.worktree file for the current + * worktree. This will initialize extensions.worktreeConfig if necessary, + * which might trigger some changes to the root repository's config file. + */ +int repo_config_set_worktree_gently(struct repository *, const char *, const char *); + /** * write config values to `.git/config`, takes a key/value pair as parameter. */ --