On Wed, Oct 10, 2018 at 4:51 PM Jeff King <peff@xxxxxxxx> wrote: > > On Fri, Sep 21, 2018 at 05:57:33PM +0200, Nguyễn Thái Ngọc Duy wrote: > > > diff --git a/diff.c b/diff.c > > index 140d0e86df..5256b9eabc 100644 > > --- a/diff.c > > +++ b/diff.c > > @@ -2093,23 +2093,25 @@ static void diff_words_flush(struct emit_callback *ecbdata) > > } > > } > > > > -static void diff_filespec_load_driver(struct diff_filespec *one) > > +static void diff_filespec_load_driver(struct diff_filespec *one, > > + struct index_state *istate) > > I hadn't looked at this topic until today, when I tried merging next > with some of my other local bits and ran into conflicts. I found the > idea of passing index_state here, rather than the whole "struct > repository", a bit curious. > > 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. > > So I think in the long run this probably should take a repository > argument, and use r->index instead of a separate istate argument. That > said, I'm not totally opposed to taking this incremental step and > letting somebody later sort out the config portions. I wonder if it > would be worth calling that out in the commit message to help that later > person. But I guess it is a bit late if this is already in next. Maybe. When I made this series, I tried to reduce the access scope as much as possible. If it turns out it needs more, we can always turn "struct index_state *" into "struct repository *" later, -- Duy
