Re: [PATCH 0/2] config includes, take 2

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 6, 2012, 09:53 UTC
Message-ID: <4F2FA330.7020803@alum.mit.edu>
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>

On 02/06/2012 07:27 AM, Jeff King wrote:

Show 11 quoted lines

>   3. perform cycle and duplicate detection on included files
> [...]
> 
> We first read the global config, which sets the value to "global", then
> includes foo, which overwrites it to "foo". Then we read the repo
> config, which sets the value to "repo", and then does _not_ actually
> read foo. Because git config is read linearly and later values tend to
> overwrite earlier ones, we would want to suppress the _first_ instance
> of a file, not the second (or really, the final if it is included many
> times). But that is impossible to do without generating a complete graph
> of includes and then pruning the early ones.

ISTM that the main goal was to prevent infinite recursion, not avoid a little bit of redundant reading. If that is the goal, all you have to record is the "include stack" context that caused the currently-being-included file to be read and make sure that the currently-being-included file didn't appear earlier on the stack. The fact that the same file was included earlier (but not as part of the same include chain) needn't be considered an error.

Show 6 quoted lines

> [...]
> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.

I agree that it is more sensible to error out than to ignore a recursive include.

It might nevertheless be useful to have an "include call stack" available for generating output for errors that occur when parsing config files; something like:

Error when parsing /home/me/bogusconfigfile line 75
    which was included from /home/me/okconfigfile line 13
    which was included from /home/me/.gitconfig line 22

or

Error: /home/me/bogusconfigfile included recursively by
/home/me/bogusfile2 line 75
    which was included from /home/me/bogusconfigfile line 13
    which was included from /home/me/.gitconfig line 22

OTOH such error stack dumps could be generated when unwinding the C call stack without the need for a separate "include call stack".

However, one could even imagine a command like

    $ git config --where-defined user.email
    user.email defined by /home/me/myfile2 line 75
        which was included from /home/me/myfile1 line 13
        which was included from /home/me/.gitconfig line 22
    user.email redefined by /home/me/project/.git/companyconfig line 8
        which was included from /home/me/project/.git/config line 20

This usage could not be implemented using the C stack, because the C stack cannot be unwound multiple times.

But these are all just wild ideas. I doubt that people's config files will become so complicated that this much infrastructure is needed.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/