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, 18:10 UTC
Message-ID: <CABPp-BGn8mz5tPrVk6fX3JKcaSuz4_XQSsSmvEMT_H5i4mF2Lg@mail.gmail.com>
In-Reply-To: <20180606170243.4169-12-pclouds@gmail.com>

On Wed, Jun 6, 2018 at 10:02 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

Show 12 quoted lines

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

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