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