{"thread":{"id":"39538","subject":"[PATCH] t7063: fix breakage with split index","startedAt":"2015-06-05T23:31:33Z","lastAt":"2015-06-08T16:49:02Z","messageCount":4,"participants":["Thomas Gummerer","Junio C Hamano","Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"263075","messageId":"1433547093-6616-1-git-send-email-t.gummerer@gmail.com","threadId":"39538","inReplyTo":null,"subject":"[PATCH] t7063: fix breakage with split index","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-06-05T23:31:33Z","receivedAt":"2015-06-05T23:31:33Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When running the test suite with GIT_TEST_SPLIT_INDEX set, tests 17-18\nin t7063 fail. Unset GIT_TEST_SPLIT_INDEX at the beginning of the test,\nin order to fix it.\n\nSigned-off-by: Thomas Gummerer \n---\n\nHi,\n\nThis breakage is both in the current master and next. I'm not entirely\nsure this is the best way to solve the issue, but unfortunately I don't\nhave any more time to look into this.\n\n t/t7063-status-untracked-cache.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex bd4806c..2f958c7 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -8,6 +8,8 @@ avoid_racy() {\n \tsleep 1\n }\n \n+unset GIT_TEST_SPLIT_INDEX\n+\n # It's fine if git update-index returns an error code other than one,\n # it'll be caught in the first test.\n test_lazy_prereq UNTRACKED_CACHE '\n-- \n2.4.0.184.g8e1974e\n"},{"id":"263137","messageId":"xmqq1thodop8.fsf@gitster.dls.corp.google.com","threadId":"39538","inReplyTo":"1433547093-6616-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH] t7063: fix breakage with split index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-07T06:20:35Z","receivedAt":"2015-06-07T06:20:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> When running the test suite with GIT_TEST_SPLIT_INDEX set, tests 17-18\n> in t7063 fail. Unset GIT_TEST_SPLIT_INDEX at the beginning of the test,\n> in order to fix it.\n\nThat is not fixing but sweeping the problem under the rug, is it?\n\nDuy, untracked-cache is a fairly new toy and I wouldn't be surprised\nif it has un-thought-out interaction with split-index which is also\na fairly new exotic toy. As both are from you, can you take a look\nat it?\n\nWe may want to make it easier to run tests with TEST-SPLIT-INDEX, if\nwe envision that the feature will bring us sufficient benefit and we\nwould eventually want to encourage its use to more people. As it\nstands now, only people who are curious enough opt into trying it\nout by exporting the environment, which would be done by a tiny\nminority of developers and users.\n\nThanks.\n\n>\n> Signed-off-by: Thomas Gummerer \n> ---\n>\n> Hi,\n>\n> This breakage is both in the current master and next. I'm not entirely\n> sure this is the best way to solve the issue, but unfortunately I don't\n> have any more time to look into this.\n>\n> t/t7063-status-untracked-cache.sh | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\n> index bd4806c..2f958c7 100755\n> --- a/t/t7063-status-untracked-cache.sh\n> +++ b/t/t7063-status-untracked-cache.sh\n> @@ -8,6 +8,8 @@ avoid_racy() {\n> \tsleep 1\n> }\n> \n> +unset GIT_TEST_SPLIT_INDEX\n> +\n> # It's fine if git update-index returns an error code other than one,\n> # it'll be caught in the first test.\n> test_lazy_prereq UNTRACKED_CACHE '\n"},{"id":"263146","messageId":"1433673652-27720-1-git-send-email-pclouds@gmail.com","threadId":"39538","inReplyTo":"xmqq1thodop8.fsf@gitster.dls.corp.google.com","subject":"[PATCH] read-cache: fix untracked cache invalidation when split-index is used","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2015-06-07T10:40:52Z","receivedAt":"2015-06-07T10:40:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Before this change, t7063.17 fails. The actual action though happens at\nt7063.16 where the entry \"two\" is added back to index after being\nremoved in the .13. Here we expect a directory invalidate at .16 and\nnone at .17 where untracked cache is refreshed. But things do not go as\nexpected when GIT_TEST_SPLIT_INDEX is set.\n\nThe different behavior that happens at .16 when split index is used: the\nentry \"two\", when deleted at .13, is simply marked \"deleted\". When .16\nexecutes, the entry resurfaces from the version in base index. This\nhappens in merge_base_index() where add_index_entry() is called to add\n\"two\" back from the base index.\n\nThis is where the bug comes from. The add_index_entry() is called with\nADD_CACHE_KEEP_CACHE_TREE flag because this version of \"two\" is not new,\nit does not break either cache-tree or untracked cache. The code should\ncheck this flag and not invalidate untracked cache. This causes a second\ninvalidation violates test expectation. The fix is obvious.\n\nNoticed-by: Thomas Gummerer \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n PS. perhaps I should rename ADD_CACHE_KEEP_CACHE_TREE to\n ADD_CACHE_KEEP_CACHE, or add a new flag .._KEEP_UNTRACKED_CACHE to\n avoid confusion.\n\n On Sun, Jun 7, 2015 at 1:20 PM, Junio C Hamano wrote:\n > Thomas Gummerer writes:\n >\n >> When running the test suite with GIT_TEST_SPLIT_INDEX set, tests 17-18\n >> in t7063 fail.  Unset GIT_TEST_SPLIT_INDEX at the beginning of the test,\n >> in order to fix it.\n >\n > That is not fixing but sweeping the problem under the rug, is it?\n\n Indeed.\n\n > Duy, untracked-cache is a fairly new toy and I wouldn't be surprised\n > if it has un-thought-out interaction with split-index which is also\n > a fairly new exotic toy.  As both are from you, can you take a look\n > at it?\n\n Still a bit slow to address, but I marked Thomas' mail for\n investigation when it came.\n\n > We may want to make it easier to run tests with TEST-SPLIT-INDEX, if\n > we envision that the feature will bring us sufficient benefit and we\n > would eventually want to encourage its use to more people.  As it\n > stands now, only people who are curious enough opt into trying it\n > out by exporting the environment, which would be done by a tiny\n > minority of developers and users.\n\n Untracked cache, split index and the last part (*) all aim at a\n smaller user audience with large work trees. They are not really what\n a common user needs, but I hope they do have users.\n \n We could make the test suite run twice, a normal run and a\n test-split-index run. But I'm not sure if it really justifies\n doubling test time. We will have to deal with this situation when/if\n pack-v4 is merged because we would want to exercise both v3 and v4 as\n much as possible.\n\n (*) the last part should keep index read time small enough even if\n the index is very large and avoid lstat() at refresh time with\n the help from watchman.\n\n read-cache.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 723d48d..309ccc7 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -999,7 +999,8 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n \t}\n \tpos = -pos-1;\n \n-\tuntracked_cache_add_to_index(istate, ce->name);\n+\tif (!(option & ADD_CACHE_KEEP_CACHE_TREE))\n+\t\tuntracked_cache_add_to_index(istate, ce->name);\n \n \t/*\n \t * Inserting a merged entry (\"stage 0\") into the index\n-- \n2.3.0.rc1.137.g477eb31\n"},{"id":"263250","messageId":"xmqq7frecfi9.fsf@gitster.dls.corp.google.com","threadId":"39538","inReplyTo":"1433673652-27720-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] read-cache: fix untracked cache invalidation when split-index is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-08T16:49:02Z","receivedAt":"2015-06-08T16:49:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Before this change, t7063.17 fails. The actual action though happens at\n> t7063.16 where the entry \"two\" is added back to index after being\n> removed in the .13. Here we expect a directory invalidate at .16 and\n> none at .17 where untracked cache is refreshed. But things do not go as\n> expected when GIT_TEST_SPLIT_INDEX is set.\n>\n> The different behavior that happens at .16 when split index is used: the\n> entry \"two\", when deleted at .13, is simply marked \"deleted\". When .16\n> executes, the entry resurfaces from the version in base index. This\n> happens in merge_base_index() where add_index_entry() is called to add\n> \"two\" back from the base index.\n>\n> This is where the bug comes from. The add_index_entry() is called with\n> ADD_CACHE_KEEP_CACHE_TREE flag because this version of \"two\" is not new,\n> it does not break either cache-tree or untracked cache. The code should\n> check this flag and not invalidate untracked cache. This causes a second\n> invalidation violates test expectation. The fix is obvious.\n>\n> Noticed-by: Thomas Gummerer \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> PS. perhaps I should rename ADD_CACHE_KEEP_CACHE_TREE to\n> ADD_CACHE_KEEP_CACHE, or add a new flag .._KEEP_UNTRACKED_CACHE to\n> avoid confusion.\n\nThat may not be a bad idea, indeed.\n\n> Untracked cache, split index and the last part (*) all aim at a\n> smaller user audience with large work trees. They are not really what\n> a common user needs, but I hope they do have users.\n\nI do hope that this can be made for everybody, though. Any project\nwill get larger, not smaller, over time and these two (I am not sure\nwhat you refer to as 'last part', though) are meant to support\nprojects with larger working trees better. After all, that is why I\nmerged the untracked-cache series reasonably early to 'master' in\nthis cycle to give us time to shake out little issues like this one.\nI think we killed two so far since it has been merged to 'master',\nso the plan is working rather nicely ;-).\n\nThanks.\n"}]}