Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

From: Duy Nguyen <pclouds@gmail.com>
Date: Nov 7, 2016, 10:08 UTC
Message-ID: <CACsJy8DjXrOgB-_-t47uSdCQFg9s_o+Oj9NBmAhDFZ3aYvjBgg@mail.gmail.com>
In-Reply-To: <CAP8UFD3hNEU_UeVizU6SVJTt4hqJPag9XWqZOM3FKCGJZXOthg@mail.gmail.com>

On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 24 quoted lines

> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>>> +void remove_split_index(struct index_state *istate)
>>> +{
>>> +       if (istate->split_index) {
>>> +               /*
>>> +                * can't discard_split_index(&the_index); because that
>>> +                * will destroy split_index->base->cache[], which may
>>> +                * be shared with the_index.cache[]. So yeah we're
>>> +                * leaking a bit here.
>>
>> In the context of update-index, this is a one-time thing and leaking
>> is tolerable. But because it becomes a library function now, this leak
>> can become more serious, I think.
>>
>> The only other (indirect) caller is read_index_from() so probably not
>> bad most of the time (we read at the beginning of a command only).
>> sequencer.c may discard and re-read the index many times though,
>> leaking could be visible there.
>
> So is it enough to check if split_index->base->cache[] is shared with
> the_index.cache[] and then decide if discard_split_index(&the_index)
> should be called?

It's likely shared though. We could un-share cache[] by duplicating index entries in the_index.cache[] if they point back to split_index->base (we know what entries are shared by examining the "index" field). Once we do that, we can discard_split_index() unconditionally. There's another place that has similar leak: move_cache_to_base_index(), which could receive the same treatment.

-- 
Duy