[PATCH] read-cache: fix untracked cache invalidation when split-index is used

Before this change, t7063.17 fails. The actual action though happens at t7063.16 where the entry "two" is added back to index after being removed in the .13. Here we expect a directory invalidate at .16 and none at .17 where untracked cache is refreshed. But things do not go as expected when GIT_TEST_SPLIT_INDEX is set.

The different behavior that happens at .16 when split index is used: the entry "two", when deleted at .13, is simply marked "deleted". When .16 executes, the entry resurfaces from the version in base index. This happens in merge_base_index() where add_index_entry() is called to add "two" back from the base index.

This is where the bug comes from. The add_index_entry() is called with ADD_CACHE_KEEP_CACHE_TREE flag because this version of "two" is not new, it does not break either cache-tree or untracked cache. The code should check this flag and not invalidate untracked cache. This causes a second invalidation violates test expectation. The fix is obvious.

Noticed-by: Thomas Gummerer <t.gummerer@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 PS. perhaps I should rename ADD_CACHE_KEEP_CACHE_TREE to
 ADD_CACHE_KEEP_CACHE, or add a new flag .._KEEP_UNTRACKED_CACHE to
 avoid confusion.

 On Sun, Jun 7, 2015 at 1:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
 > Thomas Gummerer <t.gummerer@gmail.com> writes:
 >
 >> When running the test suite with GIT_TEST_SPLIT_INDEX set, tests 17-18
 >> in t7063 fail.  Unset GIT_TEST_SPLIT_INDEX at the beginning of the test,
 >> in order to fix it.
 >
 > That is not fixing but sweeping the problem under the rug, is it?

 Indeed.

 > Duy, untracked-cache is a fairly new toy and I wouldn't be surprised
 > if it has un-thought-out interaction with split-index which is also
 > a fairly new exotic toy.  As both are from you, can you take a look
 > at it?

 Still a bit slow to address, but I marked Thomas' mail for
 investigation when it came.

 > We may want to make it easier to run tests with TEST-SPLIT-INDEX, if
 > we envision that the feature will bring us sufficient benefit and we
 > would eventually want to encourage its use to more people.  As it
 > stands now, only people who are curious enough opt into trying it
 > out by exporting the environment, which would be done by a tiny
 > minority of developers and users.

 Untracked cache, split index and the last part (*) all aim at a
 smaller user audience with large work trees. They are not really what
 a common user needs, but I hope they do have users.
 
 We could make the test suite run twice, a normal run and a
 test-split-index run. But I'm not sure if it really justifies
 doubling test time. We will have to deal with this situation when/if
 pack-v4 is merged because we would want to exercise both v3 and v4 as
 much as possible.

 (*) the last part should keep index read time small enough even if
     the index is very large and avoid lstat() at refresh time with
     the help from watchman.

 read-cache.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)