Brandon Williams wrote:
> On 06/13, Jonathan Nieder wrote:
>> Brandon Williams wrote:
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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.
What is the next step, then? You can find the notion ridiculous but
it's how this project has worked in my experience (and how other
projects with similar patch-based workflows work).
I also don't really understand why it is so bad to have to care about
API compatibility when it is so simple to do --- change the function
signature or change the function name. That's all it takes.
>> - 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.
There is, because of the gitdir: behavior change.
Jonathan