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

[Brandon Williams <bmwill@google.com>]

On 06/12, Jonathan Nieder wrote:
> Hi again,
> 
> Brandon Williams wrote:
> > On 06/12, Jonathan Nieder wrote:
> 
> >> Alternatively, could this patch rename git_config_with_options?  That
> >> way any other patch in flight that calls git_config_with_options would
> >> conflict with this patch, giving us an opportunity to make sure it
> >> also sets git_dir.  As another nice side benefit it would make it easy
> >> for someone reading the patch to verify it didn't miss any callers.
> [...]
> > And I don't know if I agree with renaming a function just to rename it.
> 
> I forgot to say: another way to accomplish the same thing can be to
> reorder the function's arguments.  The relevant thing is to make code
> that calls the function without being aware of the new requirements
> fail to compile.
> 
> [...]
> >> Brandon Williams wrote:
> 
> >>>  	opts.respect_includes = 1;
> >>> +	if (have_git_dir())
> >>> +		opts.git_dir = get_git_common_dir();
> >>
> >> curious: Why get_git_common_dir() instead of get_git_dir()?
> >
> > Needs to be commondir since the config is stored in the common git
> > directory and not a per worktree git directory.
> 
> *puzzled* Why wasn't this needed before, then?  The rest of the patch
> should result in no functional change, but this part seems different.

there is no functional change, this is what always happened.
git_path("config") will replace gitdir with commondir under the hood.

> 
> Please add some explanation to the commit message.
> 
> Thanks,
> Jonathan

-- 
Brandon Williams