Re: [PATCH v6 3/6] config: add repo_config_set_worktree_gently()

[Derrick Stolee <stolee@gmail.com>]

On 2/9/2022 12:49 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 2/8/2022 5:18 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> ...
>>>> @@ -3181,14 +3196,28 @@ void git_config_set_multivar_in_file(const char *config_filename,
>>>>  int git_config_set_multivar_gently(const char *key, const char *value,
>>>>  				   const char *value_pattern, unsigned flags)
>>>>  {
>>>> -	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
>>>> -						      flags);
>>>> +	return repo_config_set_multivar_gently(the_repository, key, value,
>>>> +					       value_pattern, flags);
>>>> +}
>>>
>>> Is this an unrelated "morally no-op" change?
>>
>> This one is to match the pattern of how "git_*" methods should
>> depend on their "repo_*" counterparts (with "the_repository" inserted
>> properly). So, it's part of the standard process for creating these
>> "repo_*" variants.
> 
> If only one of repo_config_set_multivar_gently() and
> git_config_set_multivar_gently() existed and we were completing the
> pair, then I would understand the explanation, but the title says
> that it is adding repo_config_set_worktree_gently(), which is not,
> and that is where the "unrelated" comes from.
> 
> It needs to be a separate preparatory step to add
> repo_config_set_multivar_gently() before we add
> repo_config_set_worktree_gently(), perhaps?

True, they could be split. The reason to create the _multivar_
version is for the case that worktree config is not specified,
so that is the only caller at the moment.

> A bit higher level question is if the public part of "config-set"
> API functions should gain an "easy" (in the sense of curl_easy_* set
> of API functions) API to allow the callers to say "I do not care to
> find out if per-worktree configuration is in use, or this particular
> variable is meant to be per-worktree, just set it to this value".
> 
> On this question, I am of two minds.  As certain variables (like
> core.sparseCheckout) should always be per-worktree just like certain
> refs (like HEAD) should always be per-worktree, I can understand the
> viewpoint that the callers _ought_ to know and explicitly say that
> they want to get/set in the per-worktree configuration file, but at
> the same time, I would think the callers should not have to care.
> So, I dunno.

This is an interesting idea. This would require creating a list
of "should be per-worktree" config keys that are checked within
the different *_config_set_* methods.

The biggest technical hurdle is that the multivar versions might
need to send a subset of the given config into the worktree
config and the rest to the common config.

Let's see how this progresses in the future, and whether we
have more config that we believe _must_ be stored on a per-
worktree basis. At that point, we can see whether the current
"distributed responsibility" model is too cumbersome.

Thanks,
-Stolee