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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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.

Ciao,
Dscho