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

From: Brandon Williams <bmwill@google.com>
Date: Jun 13, 2017, 21:38 UTC
Message-ID: <20170613213815.GP154599@google.com>
In-Reply-To: <20170613210806.GJ133952@aiede.mtv.corp.google.com>

On 06/13, Jonathan Nieder wrote:

Show 20 quoted lines

> 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