Re: [PATCH v1 2/3] config: introduce repo_config_set* functions
flat view
From: Junio C Hamano <hidden>
Date: 2021-11-11 20:24:50
Glen Choo [off-list ref] writes:
We have git_config_set() that sets a config value for the_repository's config file, and repo_config_get* that reads config values from a struct repository. Thus, it seems reasonable to have to have repo_git_config_set() that can set values for a config file of a struct repository. Implement repo_config_set() and repo_config_set_gently(), which take struct repository. However, unlike other instances where struct repository is added to a repo_* function, this patch does not change the implementations of git_config_set() or git_config_set_gently(); those functions use the_repository much deeper in their call chain through git_pathdup(). Mark this inconsistency as NEEDSWORK.
Being able to only affect "config" in the_repository->gitdir is less flexible than being able to affect "config" in repo->gitdir for any repository is good. Do we need a similar thing for repo->commondir as well?
quoted hunk
+/* + * Sets a config value in a repository. + */ +int repo_config_set_gently(struct repository *r, const char *key, + const char *value) +{ + int ret; + char *path; + + path = repo_git_path(r, "config"); + ret = git_config_set_in_file_gently(path, key, value); + free(path); + return ret; +} + +void repo_config_set(struct repository *r, const char *key, const char *value) +{ + char *path; + + path = repo_git_path(r, "config"); + git_config_set_in_file(path, key, value); + free(path); +}
Many questions:
- What do these do for an existing key? Add another value?
Replace existing one? If the latter, what do we plan to do with
multi-valued keys?
- Don't we need an interface to remove, rename, etc.?
- If we call repo_config_set(repo, "key", "value") and then call
repo_git_config_string(repo, "key", &value) on the same
repository, do we read the value back or do we give a stale
value?
- A change like this should make existing config_set() that only
works on the_repository into a thin wrapper, e.g.
void git_config_set(const char *keyu, const char **value)
{
repo_config_set(the_repository, key, value);
}
But that is not happening here. What prevents us from doing so?
Thanks.