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

[Brandon Williams <bmwill@google.com>]

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

-- 
Brandon Williams