Re: [PATCH v2 4/6] config: don't implicitly use gitdir

>>> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
>>> not set up) added a 'git_dir' field to the config_options struct.  Let's
>>> use this option field explicitly all the time instead of occasionally
>>> falling back to calling 'git_pathdup("config")' to get the path to the
>>> local repository configuration.  This allows 'do_git_config_sequence()'
>>> to not implicitly rely on global repository state.
>>>
>>> Signed-off-by: Brandon Williams <bmwill@google.com>
>>> ---
>>>  builtin/config.c | 2 ++
>>>  config.c         | 6 ++----
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> The same comments as before still apply:
>>
>> - this changes API to make opts->git_dir mandatory, which is error prone
>>   and easily avoidable, e.g. by making git_dir an argument to
>>   git_config_with_options
>
> I still don't agree with this.  I have looked at all callers and ensured
> that 'git_dir' will be set when appropriate in the 'config_options'
> struct.  I find the notion ridiculous that I would need to change a
> function's name or arguments every time the internals of the function
> are adjusted or when an options struct obtains a new field.  Plus, there
> is already an aptly named parameter of type 'config_options' with which
> to hold options for the config machinery.  This struct is also added to
> in a later patch to include commondir so that the gitdir vs commondir
> issue can be resolved.

>> - the commit message doesn't say anything about to git dir vs common dir
>>   change.  It needs to, or even better, the switch to use common dir
>>   instead of git dir can happen as a separate patch.
>
> There really isn't any switching in this patch.  One of the following
> patches in this series addresses this problem in more detail though.