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

From: Jeff King <peff@peff.net>
Date: Jun 13, 2017, 07:08 UTC
Message-ID: <20170613070813.v3vthqlqsga7djgz@sigill.intra.peff.net>
In-Reply-To: <20170613061627.GJ154599@google.com>

On Mon, Jun 12, 2017 at 11:16:27PM -0700, Brandon Williams wrote:

Show 14 quoted lines

> > > *puzzled* Why wasn't this needed before, then?  The rest of the patch
> > > should result in no functional change, but this part seems different.
> > 
> > Now I'm puzzled, too. The original that got filled in lazily by the
> > config functions was always get_git_dir(). I can buy the argument that
> > this was a bug (I'm not familiar enough with worktree to say one way or
> > the other), but if it's a fix it should definitely go into another
> > patch.
> 
> Well actually... in do_git_config_sequence 'git_path("config")' is
> called which will convert gitdir to commondir under the hood.  you can't
> use vanilla gitdir because the config isn't stored in a worktree's
> gitdir but rather in the commondir as the config is shared by all
> worktrees.

Sorry, I missed the fact that there were two sites changed on the first read.

> So maybe we actually need to add a field to the 'config_options' struct
> of 'commondir' such that the commondir can be used to load the actual
> config file and 'gitdir' can be used to handle the 'IncludeIf' stuff.

On reflection, I suspect that probably is the case. If you have a workdir in ~/foo, you probably want to match IncludeIf against that instead of wherever the common dir happens to be.

-Peff