On Thu, Jan 26, 2012 at 12:42:28PM -0800, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > And no, I didn't do any cycle detection. We could either do: > > > > 1. Record some canonical name for each source we look at (probably > > realpath() for files, and the sha1 for refs), and don't descend > > into already-seen sources. > > > > 2. Simply provide a maximum depth, and don't include beyond it. > > > > The latter is much simpler to implement, but I think the former is a > > little nicer for the user. > > Another thing I wondered after reading this patch was that it will be a > rather common "mistake" to include the same file twice, one in ~/.gitconfig > and then another in project specific .git/config, or more likely, people > start using useful ones in ~their/.gitconfig, and then the site administrator > by popular demand adding the same include in /etc/gitconfig to retroactively > cause the same file included twice for them. > > Your first alternative solution should solve this case nicely as well, I > would think. Agreed. I'll implement that, then. There's a slight complication with when to clear the "I have seen this" mark for each source. If you are interested only in breaking cycles, then obviously you can just clear the marks as you pop the stack. But if you want to stop repeated inclusion down different branches of the include tree, you need to keep the marks until you're done (e.g., the same file referenced by .git/config and ~/.gitconfig). But you do still need to clear them, because we repeatedly call git_config with different callback functions, and we need to re-parse each time. I think it should be sufficient to make the marks live through git_config(), and get cleared after that (so all of .git/config, ~/.gitconfig, and /etc/gitconfig will only load a given include once, but another call to git_config will load it again). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html