[PATCH] t7063: fix breakage with split index

[PATCH] t7063: fix breakage with split index

[Thomas Gummerer]

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.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

Hi,

This breakage is both in the current master and next.  I'm not entirely
sure this is the best way to solve the issue, but unfortunately I don't
have any more time to look into this.

 t/t7063-status-untracked-cache.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index bd4806c..2f958c7 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -8,6 +8,8 @@ avoid_racy() {
 	sleep 1
 }
 
+unset GIT_TEST_SPLIT_INDEX
+
 # It's fine if git update-index returns an error code other than one,
 # it'll be caught in the first test.
 test_lazy_prereq UNTRACKED_CACHE '
-- 
2.4.0.184.g8e1974e

Re: [PATCH] t7063: fix breakage with split index

[Junio C Hamano]

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?

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?

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.

Thanks.

>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>
> Hi,
>
> This breakage is both in the current master and next.  I'm not entirely
> sure this is the best way to solve the issue, but unfortunately I don't
> have any more time to look into this.
>
>  t/t7063-status-untracked-cache.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
> index bd4806c..2f958c7 100755
> --- a/t/t7063-status-untracked-cache.sh
> +++ b/t/t7063-status-untracked-cache.sh
> @@ -8,6 +8,8 @@ avoid_racy() {
>  	sleep 1
>  }
>  
> +unset GIT_TEST_SPLIT_INDEX
> +
>  # It's fine if git update-index returns an error code other than one,
>  # it'll be caught in the first test.
>  test_lazy_prereq UNTRACKED_CACHE '

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

[Nguyễn Thái Ngọc Duy]

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(-)

diff --git a/read-cache.c b/read-cache.c
index 723d48d..309ccc7 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -999,7 +999,8 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	}
 	pos = -pos-1;
 
-	untracked_cache_add_to_index(istate, ce->name);
+	if (!(option & ADD_CACHE_KEEP_CACHE_TREE))
+		untracked_cache_add_to_index(istate, ce->name);
 
 	/*
 	 * Inserting a merged entry ("stage 0") into the index
-- 
2.3.0.rc1.137.g477eb31

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

That may not be a bad idea, indeed.

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

I do hope that this can be made for everybody, though.  Any project
will get larger, not smaller, over time and these two (I am not sure
what you refer to as 'last part', though) are meant to support
projects with larger working trees better.  After all, that is why I
merged the untracked-cache series reasonably early to 'master' in
this cycle to give us time to shake out little issues like this one.
I think we killed two so far since it has been merged to 'master',
so the plan is working rather nicely ;-).

Thanks.