Re: [PATCH v2 0/2] Config conditional include

From: Jeff King <peff@peff.net>
Date: Jun 28, 2016, 21:03 UTC
Message-ID: <20160628210309.GC21002@sigill.intra.peff.net>
In-Reply-To: <vpqk2h9qbp8.fsf@anie.imag.fr>

On Tue, Jun 28, 2016 at 10:51:15PM +0200, Matthieu Moy wrote:

Show 23 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Tue, Jun 28, 2016 at 07:26:39PM +0200, Nguyễn Thái Ngọc Duy wrote:
> >
> >> There's a surprise about core.ignorecase. We are matching paths, so we
> >> should match case-insensitively if core.ignorecase tells us so. And it
> >> gets a bit tricky if core.ignorecase is defined in the same config
> >> file. I don't think we have ever told the user that keys are processed
> >> from top down. We do now.
> >
> > Hrm. I'm not excited about introducing ordering issues into the config
> > parsing.
> 
> There's already at least one case of ordering-sensitive variables, that
> we encountered when writting the config cache during Tanay Abhra's GSoC:
> diff.<driver>.funcname Vs diff.<driver>.xfuncname. Git applies the "last
> one wins" policy, which is the normal rule for a single-valued variable,
> but in this case, a "funcname" definition can override an "xfuncname"
> def. To preserve this behavior we had to introduce ordering in the
> cache, but to me this was a design mistake to rely on order.
> 
> In short: we already have one, but I'm not excited either about
> introducing new ones.

I still see funcname versus xfuncname as fundamentally a "last one wins" scenario; it's just that the two options are sort-of synonyms. But we are still talking about the same linear-ish parsing scheme, and I think it just makes the implementation a little more complicated.

I'm much more worried about something that impacts how we parse the config, and is set up in a possibly unrelated config-parsing sequence. So whether ignorecase will work depends on more variables:

 - are we doing our config parse before or after somebody has called
   git_config() at the start of a program?

 - if before (or during), does our callback call git_default_core_config()?

 - if so, did core.ignorecase appear before our include? (Almost
   certainly not, if our include is in ~/.gitconfig, because we parse
   from least-specific to most-specific).

So here it is not the implementation that is complicated, but the user-facing behavior. It's very difficult to predict when your include will kick in, and there is a good chance it will behave differently for different programs.

In general I think the best bet here is to lazy-load such values from the config-cache (so we _know_ that we got a complete parse before we look at the value). But that creates a recursion problem when we try to lazy-load from inside the config parser itself.

-Peff