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

On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder
[off-list ref] wrote:

On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen [off-list ref] wrote:

quoted

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
[off-list ref] wrote:

quoted

+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