On Thu, Oct 11, 2018 at 07:14:31AM +0900, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > I get why you're doing it: your topic here only cares about removing > > index dependencies, so you did the minimal thing to move that forward. > > > > But if you think about what this function is doing, it is quite clearly > > dependent on the whole repository, since the userdiff config we're > > looking up may come from repo config. > > In the case of userdiff that is pretty much limited to read-only > operation, I fully agree, but in more general cases, we would need > to pass both the repository and an in-core index separately, I would > say. Imagine doing a partial commit, where we construct a separate > istate that is not the "repo's index" and use that to write out a > tree object to be wrapped in a new commit, and update the current > branch ref. Yeah, agreed. I was actually puzzled at first why userdiff needs to know about the index at all. But the answer is that the attr code may read .gitattributes out of the index. It's _possible_ somebody would want to do that with an index besides the normal repo one, but I find it somewhat unlikely. I think my instinct there is based on it being "read-only", as you said. One thing that confused me even more is that diff_options now has a "struct repository" field in it. I get how that saves passing it around, but I also wonder if it may run into similar issues at some point. I'm perfectly willing to punt on it until it actually comes up in practice, though. -Peff
