Re: [PATCH v4 17/23] read-cache.c: remove an implicit dependency on the_index

From: Duy Nguyen <pclouds@gmail.com>
Date: Jun 9, 2018, 18:45 UTC
Message-ID: <CACsJy8DoU_qkghnNWKPy9+z=Lktg6jSbBFk6xEUFwAUos4Gu5A@mail.gmail.com>
In-Reply-To: <CABPp-BGn8mz5tPrVk6fX3JKcaSuz4_XQSsSmvEMT_H5i4mF2Lg@mail.gmail.com>

On Sat, Jun 9, 2018 at 8:10 PM Elijah Newren <newren@gmail.com> wrote:

Show 19 quoted lines

>
> On Wed, Jun 6, 2018 at 10:02 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index b404ebac7c..4f054d6dbb 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -315,7 +315,7 @@ static int add_cacheinfo(struct merge_options *o,
> >         struct cache_entry *ce;
> >         int ret;
> >
> > -       ce = make_cache_entry(mode, oid ? oid->hash : null_sha1, path, stage, 0);
> > +       ce = make_index_entry(&the_index, mode, oid ? oid->hash : null_sha1, path, stage, 0);
> >         if (!ce)
> >                 return err(o, _("add_cacheinfo failed for path '%s'; merge aborting."), path);
>
> There's also a refresh_cache_entry() call about ten lines after this;
> since you converted all other make_cache_entry() and
> refresh_cache_entry() calls in this patch, I'm curious if that one was
> left out for a reason or was just an oversight.

Ah I didn't mean to convert or kill refresh_cache_entry(), not outside read-cache.c. I rely on NO_THE_INDEX_COMPATIBILITY_MACROS to catch *cache* functions and if we set it in this file, we're going to have a lot more work to do and plenty of the_index will show up.

Show 5 quoted lines

> There are also a lot of add_cache_entry() calls in this function.  I'm
> guessing we should either convert all of those too, or just change
> back this particular make_index_entry to make_cache_entry() as it was;
> it seems weird to have a mix of explicit the_index and implicit
> the_index usages in the same function.

Yes some files still have the mix of the_index and *cache*(). This one and apply.c come to mind. There's more work to do to kill all the_index outside builtin/

Show 5 quoted lines

> If we convert them all,
> perhaps we should consider having merge_options store the index we're
> working on?  If you want to punt this until later or leave it for me
> while I make all my ongoing merge-recursive changes, that's fine.
> Just thought I'd point it out.

Right you're updating merge-recursive.c, I'd love it if you could define NO_THE_INDEX_COMPATIBILITY_MACROS here. Yes merge_options sounds like a good place to tell merge-recursive where to get a struct index_state.

-- 
Duy