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

From: Jeff King <peff@peff.net>
Date: Jun 14, 2017, 05:58 UTC
Message-ID: <20170614055824.p2ueyiyfsdf6inve@sigill.intra.peff.net>
In-Reply-To: <20170613182606.GO154599@google.com>

On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:

Show 30 quoted lines

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

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

-Peff