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

From: Elijah Newren <newren@gmail.com>
Date: Jun 9, 2018, 19:48 UTC
Message-ID: <CABPp-BGhS1_4dh=sMb56qaunQXkX=hu4foPV49Np-P8eZeCR5w@mail.gmail.com>
In-Reply-To: <CACsJy8DoU_qkghnNWKPy9+z=Lktg6jSbBFk6xEUFwAUos4Gu5A@mail.gmail.com>

On Sat, Jun 9, 2018 at 11:45 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 35 quoted lines

> On Sat, Jun 9, 2018 at 8:10 PM Elijah Newren <newren@gmail.com> wrote:
>>
>> 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.
>
>> 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/

Yeah, there's also low-level common files like sha1-file.c or ll-merge.c which now have &the_index in them (or more references to it). Clearly those need to be propagated up, but as you say, there's more work to do. Your patch series just moves things in the right direction.

Show 10 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.

Sure, I can tackle that.