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?
+/*
+ * 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 [off-list ref] writes:
Glen Choo [off-list ref] writes:
quoted
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 [off-list ref] writes:
quoted
- 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.