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

[Derrick Stolee <stolee@xxxxxxxxx>]

On 2/8/2022 5:18 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.

>>  void git_config_set_multivar(const char *key, const char *value,
>>  			     const char *value_pattern, unsigned flags)
>>  {
>> -	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
>> +	git_config_set_multivar_in_file(git_path("config"),
>> +					key, value, value_pattern,
>>  					flags);
>>  }
> 
> Is this an unrelated "morally no-op" change?
> 
> It might have value to make caller more explicit by reducing the use
> of "I give NULL, you use 'config' for me", but that doesn't sound
> related to the addition of set_per_worktree_config_gently() helper.

Here, you're right. This one should have followed the same pattern
of having the "git_*" equivalent call the "repo_*" method, but
instead I incorrectly inlined some of the code.

The proper body should be

void git_config_set_multivar(const char *key, const char *value,
			     const char *value_pattern, unsigned flags)
{
	repo_config_set_multivar_gently(the_repository, key, value,
					value_pattern, flags);
}

to follow convention.

Thanks,
-Stolee