Re: [PATCH v5 17/23] userdiff.c: remove implicit dependency on the_index

[Duy Nguyen <pclouds@gmail.com>]

On Wed, Oct 10, 2018 at 4:51 PM Jeff King <peff@peff.net> 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