| Message ID | 20211111171643.13805-3-chooglen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make create_branch() accept any repository | expand |
Glen Choo <chooglen@google.com> 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? > +/* > + * 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.
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> 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? git_config_set() can only affect repo->gitdir, so from the perspective of "what should a repo_* variant of git_config_set() do", then no, we do not need a similar thing. As far as I can tell, config.c only works with the gitdir, not the commondir. I cannot comment on whether we will ever need to set config values in the commondir. > 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? For an existing key, this should replace the first instance found. This eventually calls of git_config_set_multivar_in_file_gently() with value_pattern=NULL, flags = 0. According to the comments: * if flags contains the CONFIG_FLAGS_MULTI_REPLACE flag, all matching * key/values are removed before a single new pair is written. If the * flag is not present, then replace only the first match. We pass flags=0, so only the first instance is replaced. * - [value_pattern] as a string. It will disregard key/value pairs where value * does not match. We pass value_pattern=NULL, so we consider all instances. > - Don't we need an interface to remove, rename, etc.? This function supports 'remove' by passing value=NULL. By rename, I believe you're referring to renaming a config section, e.g. a repo_* version of git_config_copy_or_rename_section_in_file()? I think this is warranted if we perform a full plumbing of struct repository through config.c. But I think it would be prudent to do this plumbing piecemeal - perhaps starting with "set", and then proceeding to the other operations. > - 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? We read the correct value if repo == the_repository but we do not if r != the_repository. Thanks for spotting this bug. I believe your concern comes from the fact that struct repository caches config values in repo->config and thus we are not guaranteed to read the value back from the file. Following this train of thought, we can see that git_config_set_multivar_in_file_gently() clears the cache for the_repository, by calling git_config_clear(). Because this is hardcoded to the_repository, git_config_set_multivar_in_file_gently() cannot be safely called from repo_config_set() and my implementation is buggy. > - 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? I think it is _possible_, as long as we plumb struct repository through the call chain all the way to git_config_set_multivar_in_file_gently(). I didn't do so because I thought that I had an implementation of repo_config_set() without introducing a major overhaul of config.c. Because it is an alternative implementation, I decided not to replace the existing one. However, as noted above, this alternative implementation is wrong because git_config_set_multivar_in_file_gently() makes use of the_repository (i.e. I didn't read the function carefully enough). I will attempt this plumbing, which will allow us to make git_config_set() a thin wrapper. However, if this turns out to be too difficult, I will implement branch --recurse-submodules with child processes and leave this for a future clean up (as hinted at in the cover letter).
Glen Choo <chooglen@google.com> writes: >> - 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? > > I think it is _possible_, as long as we plumb struct repository through > the call chain all the way to git_config_set_multivar_in_file_gently(). > > [...] > > I will attempt this plumbing, which will allow us to make > git_config_set() a thin wrapper. However, if this turns out to be too > difficult, I will implement branch --recurse-submodules with child > processes and leave this for a future clean up (as hinted at in the > cover letter). I believe that this plumbing is already doable but it would take an extensive overhaul of config.c as well as some cleanup of path.c. This is more work than I am willing to take on right now, but I'll revisit this at some point.
diff --git a/config.c b/config.c index cd51efe99a..8dd00b8a13 100644 --- a/config.c +++ b/config.c @@ -2869,6 +2869,30 @@ void git_config_set_in_file(const char *config_filename, git_config_set_multivar_in_file(config_filename, key, value, NULL, 0); } +/* + * 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); +} + int git_config_set_gently(const char *key, const char *value) { return git_config_set_multivar_gently(key, value, NULL, 0); diff --git a/config.h b/config.h index 69d733824a..4a6919b984 100644 --- a/config.h +++ b/config.h @@ -253,6 +253,17 @@ void git_config_set_in_file(const char *, const char *, const char *); int git_config_set_gently(const char *, const char *); +/* + * Write config values to a repo's config file. + * + * NEEDSWORK: These are non-the_repository equivalents of + * git_config_set*, but have a completely separate implementation. In + * the ideal case, we git_config_set* should just use the repo_* + * equivalents, just like most other repo_* functions. + */ +int repo_config_set_gently(struct repository *, const char *, const char *); +void repo_config_set(struct repository *, const char *, const char *); + /** * write config values to `.git/config`, takes a key/value pair as parameter. */
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. Signed-off-by: Glen Choo <chooglen@google.com> --- config.c | 24 ++++++++++++++++++++++++ config.h | 11 +++++++++++ 2 files changed, 35 insertions(+)