Re: [PATCH v1 2/3] config: introduce repo_config_set* functions

From: Glen Choo <chooglen@google.com>
Date: Nov 12, 2021, 00:45 UTC
Message-ID: <kl6lv90ytd4v.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <xmqq7ddel9te.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> 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.

Show 5 quoted lines

> 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.

Show 9 quoted lines

>  - 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).