On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen <pclouds@xxxxxxxxx> wrote:
>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
>> <christian.couder@xxxxxxxxx> 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