Re: [PATCH v3 6/6] Use the early config machinery to expand aliases

[Brandon Williams <bmwill@google.com>]

On 06/14, Johannes Schindelin wrote:
> Hi Peff & Brandon,
> 
> On Wed, 14 Jun 2017, Jeff King wrote:
> 
> > On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:
> > 
> > > So because I've been looking at the config machinery lately, I've
> > > noticed a lot of issues with how things are handled with respect to
> > > gitdir vs commondir.  Essentially the config resides at commondir/config
> > > always, and only at gitdir/config when not working with a worktree.
> > > Because of this, your patches point out a bug in how early config is
> > > handled.  I'll illustrate this using aliases.
> > > 
> > > Before this series (because aliases are read using the standard config
> > > machinery):
> > > 
> > >   > git init main
> > >   > git -C main config alias.test '!echo hello'
> > >   > git -C main test
> > >     hello
> > >   > git -C main worktree add ../worktree
> > >   > git -C worktree test
> > >     hello
> > > 
> > > After this series (using read_early_config()):
> > > 
> > >   > git init main
> > >   > git -C main config alias.test '!echo hello'
> > >   > git -C main test
> > >     hello
> > >   > git -C main worktree add ../worktree
> > >   > git -C worktree test
> > >     git: 'test' is not a git command. See 'git --help'.
> > > 
> > > The issue is that read_early_config passes the gitdir and not the
> > > commondir when reading the config.
> > 
> > Good catch.
> 
> Oh wow.
> 
> > > The solution would be to add a 'commondir' field to the config_options
> > > struct and populate that before reading the config.  I'm planning on
> > > fixing this in v2 of my config cleanup series which I'll hopefully have
> > > finished by the end of the day.
> > 
> > I think that read_early_config() really meant to set the commondir, as
> > it was always about actual config-file lookup. So it was sort-of buggy
> > already, though I suspect it was pretty hard to trigger.
> > 
> > But I agree that since include_by_gitdir now reads the same struct
> > field, swapping it out fixes the config-reading at the cost of breaking
> > that function. And we really need to pass both in.
> > 
> > I'm not sure whether we should care about this for Dscho's series or
> > not. I think his series _does_ make the problem easier to trigger. But
> > it's a minor enough bug that I think I'd be OK with letting your
> > solution proceed independently.
> 
> Brandon, how hard would it be to build on top of my series? I ask because
> I have to take care of some other things and would not have the time to
> adjust my patch series on top of yours anytime soon.

It should be pretty easy to just rebase ontop of your series.  I'll make
sure to do that before sending out the next revision of mine.

-- 
Brandon Williams