{"thread":{"id":"52215","subject":"[PATCH 0/1] fsmonitor: skip sanity check if the index is split","startedAt":"2019-11-08T07:09:25Z","lastAt":"2019-11-15T05:05:02Z","messageCount":14,"participants":["Utsav Shah via GitGitGadget","Junio C Hamano","Kevin Willford","Utsav Shah","William Baker","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"385760","messageId":"pull.458.git.1573196960.gitgitgadget@gmail.com","threadId":"52215","inReplyTo":null,"subject":"[PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-08T07:09:19Z","receivedAt":"2019-11-08T07:09:25Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"The checks added in 3444ec2 to ensure that the fsmonitor_dirty bitmap does\nnot have more bits than the index do not play well with the split index.\n\ngit update-index --fsmonitor --split-index calls write_locked_index which\ncalls write_shared_index as well as write_split_index. The first call fills\nup the fsmonitor_dirty bitmap, and the second modifies the index such that\nistate->cache_nr is zero and this assert is hit.\n\nThe test written does reproduce the error, but only flakily. There is\nlimited difference with GIT_TEST_FSMONITOR=fsmonitor-all or\nGIT_TEST_FSMONITOR=fsmonitor-watchman, so the flakiness might come from\nsomewhere else, which I haven't tracked down.\n\nThe test also requires checkout of a new branch, and checking out back to\nmaster. It's clear that the index gets into some poor state through these\noperations, and there is a deeper bug somewhere.\n\nAt the very least, this patch mitigates an over-eager check for split index\nusers while maintaining good invariants for the standard case. Also, I\nhaven't been able to reproduce this with \"standard\" user commands, like\nstatus/checkout/stash, so the blast radius seems limited.\n\nHelped-by: Kevin Willford kewillf@microsoft.com [kewillf@microsoft.com]\nHelped-by: Junio C Hamano gitster@pobox.com [gitster@pobox.com]\nSigned-off-by: Utsav Shah utsav@dropbox.com [utsav@dropbox.com]\n\nUtsav Shah (1):\n fsmonitor: skip sanity check if the index is split\n\n fsmonitor.c | 8 ++++----\n t/t7519-status-fsmonitor.sh | 23 +++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-458%2FUtsav2%2Fsplit-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-458/Utsav2/split-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/458\n-- \ngitgitgadget\n"},{"id":"385761","messageId":"8d69ba362261690e58b3879c33ac01c8888dc473.1573196960.git.gitgitgadget@gmail.com","threadId":"52215","inReplyTo":"pull.458.git.1573196960.gitgitgadget@gmail.com","subject":"[PATCH 1/1] fsmonitor: skip sanity check if the index is split","fromName":"Utsav Shah via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-08T07:09:20Z","receivedAt":"2019-11-08T07:09:27Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"From: Utsav Shah \n\nThe checks added in 3444ec2eb2 (\"fsmonitor: don't fill bitmap with\nentries to be removed\", 2019-10-11), to ensure that the\nfsmonitor_dirty bitmap does not have more bits than the index\ndo not play well with the split index.\n\ngit update-index --fsmonitor --split-index calls write_locked_index\nwhich calls write_shared_index as well as write_split_index.\nThe first call fills up the fsmonitor_dirty bitmap,\nand the second modifies the index such that istate->cache_nr is zero and\nthis assert is hit.\n\nThe test written does reproduce the error, but only flakily. There is\nlimited difference with GIT_TEST_FSMONITOR=fsmonitor-all or\nGIT_TEST_FSMONITOR=fsmonitor-watchman, so the flakiness might come from\nsomewhere else, which I haven't tracked down.\n\nThe test also requires checkout of a new branch, and checking out back\nto master. It's clear that the index gets into some poor state through\nthese operations, and there is a deeper bug somewhere.\n\nAt the very least, this patch mitigates an over-eager check for split\nindex users while maintaining good invariants for the standard case.\nAlso, I haven't been able to reproduce this with \"standard\" user\ncommands, like status/checkout/stash, so the blast radius seems limited.\n\nHelped-by: Kevin Willford \nHelped-by: Junio C Hamano \nSigned-off-by: Utsav Shah \n---\n fsmonitor.c | 8 ++++----\n t/t7519-status-fsmonitor.sh | 23 +++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1f4aa1b150..01cba22b38 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -16,7 +16,7 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n \tstruct index_state *istate = (struct index_state *)is;\n \tstruct cache_entry *ce;\n \n-\tif (pos >= istate->cache_nr)\n+\tif (!istate->split_index && pos >= istate->cache_nr)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n \t\t (uintmax_t)pos, istate->cache_nr);\n \n@@ -55,7 +55,7 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \n@@ -83,7 +83,7 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \n@@ -252,7 +252,7 @@ void tweak_fsmonitor(struct index_state *istate)\n \t\t\t}\n \n \t\t\t/* Mark all previously saved entries as dirty */\n-\t\t\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\t\t\tif (!istate->split_index && istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n \t\t\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t\t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex d8df990972..b5029eff3e 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -371,4 +371,27 @@ test_expect_success 'status succeeds after staging/unstaging ' '\n \t)\n '\n \n+# Git will only split indices if we have a bunch of files created,\n+# so that prep work of creating a few hundred files is required.\n+# Note that this test doesn't fail determinstically without\n+# its corresponding bugfix.\n+test_expect_success 'update-index succeeds after staging with split index' '\n+\ttest_create_repo fsmonitor-stage-split &&\n+\t(\n+\t\tcd fsmonitor-stage-split &&\n+\t\ttest_commit initial &&\n+\t\tfiles=$(test_seq 1 100) &&\n+\t\techo \"hello world\" > file &&\n+\t\ttouch $files &&\n+\t\tgit add -A &&\n+\t\tgit commit -m \"next\" &&\n+\t\tgit config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-watchman\" &&\n+\t\techo \"hello world\" > file &&\n+\t\tgit checkout -b new-branch &&\n+\t\tgit checkout master &&\n+\t\techo hello >> file &&\n+\t\tgit update-index --split-index --untracked-cache --fsmonitor\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"385857","messageId":"xmqqh83bjig6.fsf@gitster-ct.c.googlers.com","threadId":"52215","inReplyTo":"pull.458.git.1573196960.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-11T01:43:53Z","receivedAt":"2019-11-11T01:43:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Utsav Shah via GitGitGadget\" writes:\n\n> At the very least, this patch mitigates an over-eager check for split index\n> users while maintaining good invariants for the standard case.\n\nOK, it sounds more like this \"it does not make any sense to compare\nthe position in the fsmonitor bitmap (which covers the entire thing)\nwith the position in just a split part of the index (which covers\nonly the delta over the base index)\"? If that is the case, it means\nthat the \"check\" is even worse than being \"over-eager\"---it simply\nis not correct.\n\nThanks, will queue.\n"},{"id":"385858","messageId":"xmqqa793jhne.fsf@gitster-ct.c.googlers.com","threadId":"52215","inReplyTo":"xmqqh83bjig6.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-11T02:01:09Z","receivedAt":"2019-11-11T02:01:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Utsav Shah via GitGitGadget\" writes:\n>\n>> At the very least, this patch mitigates an over-eager check for split index\n>> users while maintaining good invariants for the standard case.\n>\n> OK, it sounds more like this \"it does not make any sense to compare\n> the position in the fsmonitor bitmap (which covers the entire thing)\n> with the position in just a split part of the index (which covers\n> only the delta over the base index)\"? If that is the case, it means\n> that the \"check\" is even worse than being \"over-eager\"---it simply\n> is not correct.\n\nHaving said all that, I wonder if we are doing the right thing with\nor without 3444ec2e (\"fsmonitor: don't fill bitmap with entries to\nbe removed\", 2019-10-11) in the split-index mode in the first place.\n\nThe fact that your \"loosen the check and allow 'pos' that identifies\na tracked path used by the fsmonitor bitmap to be larger than the\nsize of the istate->cache[]\" patch under discussion is needed is\nthat 'pos' may sometimes be larger than isate->cache[] no? Then\nwhat happens in this hunk, for example?\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 231e83a94d..1f4aa1b150 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n static void fsmonitor_ewah_callback(size_t pos, void *is)\n {\n \tstruct index_state *istate = (struct index_state *)is;\n-\tstruct cache_entry *ce = istate->cache[pos];\n+\tstruct cache_entry *ce;\n \n+\tif (pos >= istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %u)\",\n+\t\t (uintmax_t)pos, istate->cache_nr);\n+\n+\tce = istate->cache[pos];\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n\nThe istate->cache[] is a dynamic array whose size is managed via the\nusual ALLOC_GROW() using istate->cache_nr and istate->cache_alloc,\nwhether the split-index feature is in use. When your patch makes a\ndifference, then, doesn't the access to istate->cache[] pick up a\nrandom garbage and then flip the bit?\n\nPuzzled... In any case, \"check is worse than over-eager, it simply\nis wrong\" I wrote in the message I am responding to is totally\nincorrect, it seems. It smells like lifting the check would just\nhide the underlying problem under the rug?\n\n"},{"id":"385914","messageId":"BN6PR21MB07869E8D1DCAF189C4E472A891740@BN6PR21MB0786.namprd21.prod.outlook.com","threadId":"52215","inReplyTo":"xmqqa793jhne.fsf@gitster-ct.c.googlers.com","subject":"RE: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Kevin Willford","fromEmail":"kevin.willford@microsoft.com","sentAt":"2019-11-11T16:55:11Z","receivedAt":"2019-11-11T16:55:55Z","isPatch":true,"sender":{"key":"kevin.willford@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/40003493?v=4"},"body":"> From: git-owner@vger.kernel.org On Behalf\n> Of Junio C Hamano\n> Sent: Sunday, November 10, 2019 7:01 PM\n> \n> Junio C Hamano writes:\n> \n> > \"Utsav Shah via GitGitGadget\" writes:\n> >\n> >> At the very least, this patch mitigates an over-eager check for split\n> >> index users while maintaining good invariants for the standard case.\n> >\n> > OK, it sounds more like this \"it does not make any sense to compare\n> > the position in the fsmonitor bitmap (which covers the entire thing)\n> > with the position in just a split part of the index (which covers only\n> > the delta over the base index)\"? If that is the case, it means that\n> > the \"check\" is even worse than being \"over-eager\"---it simply is not\n> > correct.\n> \n> Having said all that, I wonder if we are doing the right thing with or without\n> 3444ec2e (\"fsmonitor: don't fill bitmap with entries to be removed\", 2019-10-\n> 11) in the split-index mode in the first place.\n> \n> The fact that your \"loosen the check and allow 'pos' that identifies a tracked\n> path used by the fsmonitor bitmap to be larger than the size of the istate-\n> >cache[]\" patch under discussion is needed is that 'pos' may sometimes be\n> larger than isate->cache[] no? Then what happens in this hunk, for example?\n> \n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 231e83a94d..1f4aa1b150 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor =\n> TRACE_KEY_INIT(FSMONITOR); static void fsmonitor_ewah_callback(size_t\n> pos, void *is) {\n> \tstruct index_state *istate = (struct index_state *)is;\n> -\tstruct cache_entry *ce = istate->cache[pos];\n> +\tstruct cache_entry *ce;\n> \n> +\tif (pos >= istate->cache_nr)\n> +\t\tBUG(\"fsmonitor_dirty has more entries than the index\n> (%\"PRIuMAX\" >= %u)\",\n> +\t\t (uintmax_t)pos, istate->cache_nr);\n> +\n> +\tce = istate->cache[pos];\n> \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n> \n> The istate->cache[] is a dynamic array whose size is managed via the usual\n> ALLOC_GROW() using istate->cache_nr and istate->cache_alloc, whether the\n> split-index feature is in use. When your patch makes a difference, then,\n> doesn't the access to istate->cache[] pick up a random garbage and then flip\n> the bit?\n> \n> Puzzled... In any case, \"check is worse than over-eager, it simply is wrong\" I\n> wrote in the message I am responding to is totally incorrect, it seems. It\n> smells like lifting the check would just hide the underlying problem under the\n> rug?\n\nI agree. The only 2 places that excluding the split-index make sense are in\nread_fsmonitor_extension and write_fsmonitor_extension because the\nindex_state that is being passing into those methods could be the delta index\nin which case the number of entries for the fsmonitor bitmap would almost\nalways be more and cause the BUG to be hit which it should not be.\n\nThe reason it is not needed and should not be in the other 2 places is they\nare ran from tweak_fsmonitor which is ran at post_read_index_from which\nis after the base and delta indexes have been loaded into the indes_state and\nthe index_state will have all the entries and if the fsmonitor bitmap is bigger\nthan the number of entries then the BUG should be hit. \n\nKevin\n"},{"id":"385915","messageId":"CABhpXbdyfxF3xVrjHUDzN7inu6O+CVVxG1wc9shhy33XiDoNPA@mail.gmail.com","threadId":"52215","inReplyTo":"BN6PR21MB07869E8D1DCAF189C4E472A891740@BN6PR21MB0786.namprd21.prod.outlook.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Utsav Shah","fromEmail":"ukshah2@illinois.edu","sentAt":"2019-11-11T17:25:41Z","receivedAt":"2019-11-11T17:25:55Z","isPatch":true,"sender":{"key":"ukshah2@illinois.edu","avatar":null},"body":"On Mon, Nov 11, 2019 at 8:55 AM Kevin Willford\n wrote:\n>\n> > From: git-owner@vger.kernel.org On Behalf\n> > Of Junio C Hamano\n> > Sent: Sunday, November 10, 2019 7:01 PM\n> >\n> > Junio C Hamano writes:\n> >\n> > > \"Utsav Shah via GitGitGadget\" writes:\n> > >\n> > >> At the very least, this patch mitigates an over-eager check for split\n> > >> index users while maintaining good invariants for the standard case.\n> > >\n> > > OK, it sounds more like this \"it does not make any sense to compare\n> > > the position in the fsmonitor bitmap (which covers the entire thing)\n> > > with the position in just a split part of the index (which covers only\n> > > the delta over the base index)\"? If that is the case, it means that\n> > > the \"check\" is even worse than being \"over-eager\"---it simply is not\n> > > correct.\n> >\n> > Having said all that, I wonder if we are doing the right thing with or without\n> > 3444ec2e (\"fsmonitor: don't fill bitmap with entries to be removed\", 2019-10-\n> > 11) in the split-index mode in the first place.\n> >\n> > The fact that your \"loosen the check and allow 'pos' that identifies a tracked\n> > path used by the fsmonitor bitmap to be larger than the size of the istate-\n> > >cache[]\" patch under discussion is needed is that 'pos' may sometimes be\n> > larger than isate->cache[] no? Then what happens in this hunk, for example?\n> >\n> > diff --git a/fsmonitor.c b/fsmonitor.c\n> > index 231e83a94d..1f4aa1b150 100644\n> > --- a/fsmonitor.c\n> > +++ b/fsmonitor.c\n> > @@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor =\n> > TRACE_KEY_INIT(FSMONITOR); static void fsmonitor_ewah_callback(size_t\n> > pos, void *is) {\n> > struct index_state *istate = (struct index_state *)is;\n> > - struct cache_entry *ce = istate->cache[pos];\n> > + struct cache_entry *ce;\n> >\n> > + if (pos >= istate->cache_nr)\n> > + BUG(\"fsmonitor_dirty has more entries than the index\n> > (%\"PRIuMAX\" >= %u)\",\n> > + (uintmax_t)pos, istate->cache_nr);\n> > +\n> > + ce = istate->cache[pos];\n> > ce->ce_flags &= ~CE_FSMONITOR_VALID;\n> >\n> > The istate->cache[] is a dynamic array whose size is managed via the usual\n> > ALLOC_GROW() using istate->cache_nr and istate->cache_alloc, whether the\n> > split-index feature is in use. When your patch makes a difference, then,\n> > doesn't the access to istate->cache[] pick up a random garbage and then flip\n> > the bit?\n> >\n> > Puzzled... In any case, \"check is worse than over-eager, it simply is wrong\" I\n> > wrote in the message I am responding to is totally incorrect, it seems. It\n> > smells like lifting the check would just hide the underlying problem under the\n> > rug?\n>\n> I agree. The only 2 places that excluding the split-index make sense are in\n> read_fsmonitor_extension and write_fsmonitor_extension because the\n> index_state that is being passing into those methods could be the delta index\n> in which case the number of entries for the fsmonitor bitmap would almost\n> always be more and cause the BUG to be hit which it should not be.\n>\n> The reason it is not needed and should not be in the other 2 places is they\n> are ran from tweak_fsmonitor which is ran at post_read_index_from which\n> is after the base and delta indexes have been loaded into the indes_state and\n> the index_state will have all the entries and if the fsmonitor bitmap is bigger\n> than the number of entries then the BUG should be hit.\n\nThanks. What exactly is the delta index? Is it the \"split\" index, vs\nthe shared indices? I was surprised to see cache_nr being zero. My\nunderstanding was that cache and cache_nr would always be the\nmaterialized version of the entire index, which is clearly incorrect.\n\n>\n> Kevin\n"},{"id":"385917","messageId":"ba3736b8-9f44-5438-36c3-213d8a3d621e@gmail.com","threadId":"52215","inReplyTo":"BN6PR21MB07869E8D1DCAF189C4E472A891740@BN6PR21MB0786.namprd21.prod.outlook.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"William Baker","fromEmail":"williamtbakeremail@gmail.com","sentAt":"2019-11-11T17:30:56Z","receivedAt":"2019-11-11T17:31:09Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"\nOn 11/11/19 8:55 AM, Kevin Willford wrote:\n>>\n>> The istate->cache[] is a dynamic array whose size is managed via the usual\n>> ALLOC_GROW() using istate->cache_nr and istate->cache_alloc, whether the\n>> split-index feature is in use. When your patch makes a difference, then,\n>> doesn't the access to istate->cache[] pick up a random garbage and then flip\n>> the bit?\n>>\n>> Puzzled... In any case, \"check is worse than over-eager, it simply is wrong\" I\n>> wrote in the message I am responding to is totally incorrect, it seems. It\n>> smells like lifting the check would just hide the underlying problem under the\n>> rug?\n> \n> I agree. The only 2 places that excluding the split-index make sense are in\n> read_fsmonitor_extension and write_fsmonitor_extension because the\n> index_state that is being passing into those methods could be the delta index\n> in which case the number of entries for the fsmonitor bitmap would almost\n> always be more and cause the BUG to be hit which it should not be.\n> \n> The reason it is not needed and should not be in the other 2 places is they\n> are ran from tweak_fsmonitor which is ran at post_read_index_from which\n> is after the base and delta indexes have been loaded into the indes_state and\n> the index_state will have all the entries and if the fsmonitor bitmap is bigger\n> than the number of entries then the BUG should be hit. \n> \n\nI agree. While working on the 3444ec2e patch I missed that read_fsmonitor_extension\nand write_fsmonitor_extension could be called with the delta index rather than the\nfull index.\n\nI think it makes sense to leave the check in the other two places.\n\nThanks,\nWilliam\n\n\n"},{"id":"385918","messageId":"BN6PR21MB078648200A11238D0FD992FC91740@BN6PR21MB0786.namprd21.prod.outlook.com","threadId":"52215","inReplyTo":"CABhpXbdyfxF3xVrjHUDzN7inu6O+CVVxG1wc9shhy33XiDoNPA@mail.gmail.com","subject":"RE: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Kevin Willford","fromEmail":"kevin.willford@microsoft.com","sentAt":"2019-11-11T18:21:19Z","receivedAt":"2019-11-11T18:21:24Z","isPatch":true,"sender":{"key":"kevin.willford@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/40003493?v=4"},"body":"> From: Utsav Shah \n> Sent: Monday, November 11, 2019 10:26 AM\n> \n> On Mon, Nov 11, 2019 at 8:55 AM Kevin Willford\n> wrote:\n> >\n> > > From: git-owner@vger.kernel.org On\n> > > Behalf Of Junio C Hamano\n> > > Sent: Sunday, November 10, 2019 7:01 PM\n> > >\n> > > Junio C Hamano writes:\n> > >\n> > > > \"Utsav Shah via GitGitGadget\" writes:\n> > > >\n> > > >> At the very least, this patch mitigates an over-eager check for\n> > > >> split index users while maintaining good invariants for the standard\n> case.\n> > > >\n> > > > OK, it sounds more like this \"it does not make any sense to\n> > > > compare the position in the fsmonitor bitmap (which covers the\n> > > > entire thing) with the position in just a split part of the index\n> > > > (which covers only the delta over the base index)\"? If that is\n> > > > the case, it means that the \"check\" is even worse than being\n> > > > \"over-eager\"---it simply is not correct.\n> > >\n> > > Having said all that, I wonder if we are doing the right thing with\n> > > or without 3444ec2e (\"fsmonitor: don't fill bitmap with entries to\n> > > be removed\", 2019-10-\n> > > 11) in the split-index mode in the first place.\n> > >\n> > > The fact that your \"loosen the check and allow 'pos' that identifies\n> > > a tracked path used by the fsmonitor bitmap to be larger than the\n> > > size of the istate-\n> > > >cache[]\" patch under discussion is needed is that 'pos' may\n> > > >sometimes be\n> > > larger than isate->cache[] no? Then what happens in this hunk, for\n> example?\n> > >\n> > > diff --git a/fsmonitor.c b/fsmonitor.c index 231e83a94d..1f4aa1b150\n> > > 100644\n> > > --- a/fsmonitor.c\n> > > +++ b/fsmonitor.c\n> > > @@ -14,8 +14,13 @@ struct trace_key trace_fsmonitor =\n> > > TRACE_KEY_INIT(FSMONITOR); static void\n> > > fsmonitor_ewah_callback(size_t pos, void *is) {\n> > > struct index_state *istate = (struct index_state *)is;\n> > > - struct cache_entry *ce = istate->cache[pos];\n> > > + struct cache_entry *ce;\n> > >\n> > > + if (pos >= istate->cache_nr)\n> > > + BUG(\"fsmonitor_dirty has more entries than the index\n> > > (%\"PRIuMAX\" >= %u)\",\n> > > + (uintmax_t)pos, istate->cache_nr);\n> > > +\n> > > + ce = istate->cache[pos];\n> > > ce->ce_flags &= ~CE_FSMONITOR_VALID;\n> > >\n> > > The istate->cache[] is a dynamic array whose size is managed via the\n> > > usual\n> > > ALLOC_GROW() using istate->cache_nr and istate->cache_alloc, whether\n> > > the split-index feature is in use. When your patch makes a\n> > > difference, then, doesn't the access to istate->cache[] pick up a\n> > > random garbage and then flip the bit?\n> > >\n> > > Puzzled... In any case, \"check is worse than over-eager, it simply\n> > > is wrong\" I wrote in the message I am responding to is totally\n> > > incorrect, it seems. It smells like lifting the check would just\n> > > hide the underlying problem under the rug?\n> >\n> > I agree. The only 2 places that excluding the split-index make sense\n> > are in read_fsmonitor_extension and write_fsmonitor_extension because\n> > the index_state that is being passing into those methods could be the\n> > delta index in which case the number of entries for the fsmonitor\n> > bitmap would almost always be more and cause the BUG to be hit which it\n> should not be.\n> >\n> > The reason it is not needed and should not be in the other 2 places is\n> > they are ran from tweak_fsmonitor which is ran at post_read_index_from\n> > which is after the base and delta indexes have been loaded into the\n> > indes_state and the index_state will have all the entries and if the\n> > fsmonitor bitmap is bigger than the number of entries then the BUG should\n> be hit.\n> \n> Thanks. What exactly is the delta index? Is it the \"split\" index, vs the shared\n> indices?\n\nYes the delta is the same as the split index mentioned here\nhttps://git-scm.com/docs/git-update-index#_split_index.\n\n> I was surprised to see cache_nr being zero. My understanding was\n> that cache and cache_nr would always be the materialized version of the\n> entire index, which is clearly incorrect.\n\nMost of the time that is correct but if you look in read_index_from, the\nindex is loaded with the call to\n\nret = do_read_index(istate, path, 0);\n\nThis will read the index extensions so read_fsmonitor_extension will be\ncalled and the cache will only have the entries from the split/delta index.\n\nThe base/shared index isn't loaded and in the cache until later when\nmerge_base_index(istate); is called which is right before the call to\npost_read_index_from where tweak_fsmonitor will get called from.\n"},{"id":"386005","messageId":"20191112111809.GN4348@szeder.dev","threadId":"52215","inReplyTo":"8d69ba362261690e58b3879c33ac01c8888dc473.1573196960.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fsmonitor: skip sanity check if the index is split","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-12T11:18:09Z","receivedAt":"2019-11-12T11:18:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Nov 08, 2019 at 07:09:20AM +0000, Utsav Shah via GitGitGadget wrote:\n> The checks added in 3444ec2eb2 (\"fsmonitor: don't fill bitmap with\n> entries to be removed\", 2019-10-11), to ensure that the\n> fsmonitor_dirty bitmap does not have more bits than the index\n> do not play well with the split index.\n> \n> git update-index --fsmonitor --split-index calls write_locked_index\n> which calls write_shared_index as well as write_split_index.\n> The first call fills up the fsmonitor_dirty bitmap,\n> and the second modifies the index such that istate->cache_nr is zero and\n> this assert is hit.\n\nJust to make sure that we are on the same page, is this the one?\n\n BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (102 > 1)\n\n> The test written does reproduce the error, but only flakily. There is\n> limited difference with GIT_TEST_FSMONITOR=fsmonitor-all or\n> GIT_TEST_FSMONITOR=fsmonitor-watchman, so the flakiness might come from\n> somewhere else, which I haven't tracked down.\n> \n> The test also requires checkout of a new branch, and checking out back\n> to master.\n\nIt doesn't; see below.\n\n> It's clear that the index gets into some poor state through\n> these operations, and there is a deeper bug somewhere.\n> \n> At the very least, this patch mitigates an over-eager check for split\n> index users while maintaining good invariants for the standard case.\n> Also, I haven't been able to reproduce this with \"standard\" user\n> commands, like status/checkout/stash, so the blast radius seems limited.\n> \n> Helped-by: Kevin Willford \n> Helped-by: Junio C Hamano \n> Signed-off-by: Utsav Shah \n> ---\n\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index d8df990972..b5029eff3e 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -371,4 +371,27 @@ test_expect_success 'status succeeds after staging/unstaging ' '\n> \t)\n> '\n> \n> +# Git will only split indices if we have a bunch of files created,\n> +# so that prep work of creating a few hundred files is required.\n\n'git update-index --split-index' splits the index no matter what; it\neven splits an empty index.\n\n> +# Note that this test doesn't fail determinstically without\n> +# its corresponding bugfix.\n> +test_expect_success 'update-index succeeds after staging with split index' '\n> +\ttest_create_repo fsmonitor-stage-split &&\n> +\t(\n> +\t\tcd fsmonitor-stage-split &&\n> +\t\ttest_commit initial &&\n> +\t\tfiles=$(test_seq 1 100) &&\n> +\t\techo \"hello world\" > file &&\n> +\t\ttouch $files &&\n> +\t\tgit add -A &&\n> +\t\tgit commit -m \"next\" &&\n> +\t\tgit config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-watchman\" &&\n> +\t\techo \"hello world\" > file &&\n> +\t\tgit checkout -b new-branch &&\n> +\t\tgit checkout master &&\n> +\t\techo hello >> file &&\n> +\t\tgit update-index --split-index --untracked-cache --fsmonitor\n> +\t)\n> +'\n\nI could reproduce the failure with '-r 30 --stress' relatively easily\n[1], but with those options I could shave off a lot from this test:\n\n test_create_repo fsmonitor-stage-split &&\n (\n cd fsmonitor-stage-split &&\n >tracked &&\n git add tracked &&\n git config core.fsmonitor\n \"$TEST_DIRECTORY/t7519/fsmonitor-watchman\" &&\n >untracked &&\n git update-index --split-index --untracked-cache --fsmonitor\n )\n\nand could still trigger the failure:\n\n + git update-index --split-index --untracked-cache --fsmonitor\n open2: exec of watchman -j failed at /home/szeder/src/git/t/t7519/fsmonitor-watchman line 47.\n BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (1 > 0)\n\n\n[1] There is a quite expensive lazy prereq evaluation outside of\n 'test_expect_*' that I disabled it with the following for\n speedier stress testing:\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 997d5fb349..103520415d 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -50,8 +50,7 @@ write_integration_script () {\n }\n \n test_lazy_prereq UNTRACKED_CACHE '\n-\t{ git update-index --test-untracked-cache; ret=$?; } &&\n-\ttest $ret -ne 1\n+\ttrue\n '\n \n test_expect_success 'setup' '\n"},{"id":"386047","messageId":"CAPYzU3PKrL=tz13_VV2uCkX04UyaC4GvD9GPvtnHUeu7UKU4cg@mail.gmail.com","threadId":"52215","inReplyTo":"20191112111809.GN4348@szeder.dev","subject":"Re: [PATCH 1/1] fsmonitor: skip sanity check if the index is split","fromName":"Utsav Shah","fromEmail":"utsav@dropbox.com","sentAt":"2019-11-12T21:08:12Z","receivedAt":"2019-11-12T21:08:27Z","isPatch":true,"sender":{"key":"utsav@dropbox.com","avatar":"https://avatars.githubusercontent.com/u/49925333?v=4"},"body":"On Tue, Nov 12, 2019 at 3:18 AM SZEDER Gábor wrote:\n>\n> On Fri, Nov 08, 2019 at 07:09:20AM +0000, Utsav Shah via GitGitGadget wrote:\n> > The checks added in 3444ec2eb2 (\"fsmonitor: don't fill bitmap with\n> > entries to be removed\", 2019-10-11), to ensure that the\n> > fsmonitor_dirty bitmap does not have more bits than the index\n> > do not play well with the split index.\n> >\n> > git update-index --fsmonitor --split-index calls write_locked_index\n> > which calls write_shared_index as well as write_split_index.\n> > The first call fills up the fsmonitor_dirty bitmap,\n> > and the second modifies the index such that istate->cache_nr is zero and\n> > this assert is hit.\n>\n> Just to make sure that we are on the same page, is this the one?\n>\n> BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (102 > 1)\n>\n\nYes, that's the one.\n\n> > The test written does reproduce the error, but only flakily. There is\n> > limited difference with GIT_TEST_FSMONITOR=fsmonitor-all or\n> > GIT_TEST_FSMONITOR=fsmonitor-watchman, so the flakiness might come from\n> > somewhere else, which I haven't tracked down.\n> >\n> > The test also requires checkout of a new branch, and checking out back\n> > to master.\n>\n> It doesn't; see below.\n>\n> > It's clear that the index gets into some poor state through\n> > these operations, and there is a deeper bug somewhere.\n> >\n> > At the very least, this patch mitigates an over-eager check for split\n> > index users while maintaining good invariants for the standard case.\n> > Also, I haven't been able to reproduce this with \"standard\" user\n> > commands, like status/checkout/stash, so the blast radius seems limited.\n> >\n> > Helped-by: Kevin Willford \n> > Helped-by: Junio C Hamano \n> > Signed-off-by: Utsav Shah \n> > ---\n>\n> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> > index d8df990972..b5029eff3e 100755\n> > --- a/t/t7519-status-fsmonitor.sh\n> > +++ b/t/t7519-status-fsmonitor.sh\n> > @@ -371,4 +371,27 @@ test_expect_success 'status succeeds after staging/unstaging ' '\n> > )\n> > '\n> >\n> > +# Git will only split indices if we have a bunch of files created,\n> > +# so that prep work of creating a few hundred files is required.\n>\n> 'git update-index --split-index' splits the index no matter what; it\n> even splits an empty index.\n>\n> > +# Note that this test doesn't fail determinstically without\n> > +# its corresponding bugfix.\n> > +test_expect_success 'update-index succeeds after staging with split index' '\n> > + test_create_repo fsmonitor-stage-split &&\n> > + (\n> > + cd fsmonitor-stage-split &&\n> > + test_commit initial &&\n> > + files=$(test_seq 1 100) &&\n> > + echo \"hello world\" > file &&\n> > + touch $files &&\n> > + git add -A &&\n> > + git commit -m \"next\" &&\n> > + git config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-watchman\" &&\n> > + echo \"hello world\" > file &&\n> > + git checkout -b new-branch &&\n> > + git checkout master &&\n> > + echo hello >> file &&\n> > + git update-index --split-index --untracked-cache --fsmonitor\n> > + )\n> > +'\n>\n> I could reproduce the failure with '-r 30 --stress' relatively easily\n> [1], but with those options I could shave off a lot from this test:\n>\n> test_create_repo fsmonitor-stage-split &&\n> (\n> cd fsmonitor-stage-split &&\n> >tracked &&\n> git add tracked &&\n> git config core.fsmonitor\n> \"$TEST_DIRECTORY/t7519/fsmonitor-watchman\" &&\n> >untracked &&\n> git update-index --split-index --untracked-cache --fsmonitor\n> )\n>\n> and could still trigger the failure:\n>\n> + git update-index --split-index --untracked-cache --fsmonitor\n> open2: exec of watchman -j failed at /home/szeder/src/git/t/t7519/fsmonitor-watchman line 47.\n> BUG: fsmonitor.c:88: fsmonitor_dirty has more entries than the index (1 > 0)\n>\n>\n> [1] There is a quite expensive lazy prereq evaluation outside of\n> 'test_expect_*' that I disabled it with the following for\n> speedier stress testing:\n>\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index 997d5fb349..103520415d 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -50,8 +50,7 @@ write_integration_script () {\n> }\n>\n> test_lazy_prereq UNTRACKED_CACHE '\n> - { git update-index --test-untracked-cache; ret=$?; } &&\n> - test $ret -ne 1\n> + true\n> '\n>\n> test_expect_success 'setup' '\n"},{"id":"386079","messageId":"xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com","threadId":"52215","inReplyTo":"BN6PR21MB07869E8D1DCAF189C4E472A891740@BN6PR21MB0786.namprd21.prod.outlook.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T01:30:05Z","receivedAt":"2019-11-13T01:30:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kevin Willford writes:\n\n> I agree. The only 2 places that excluding the split-index make sense are in\n> read_fsmonitor_extension and write_fsmonitor_extension because the\n> index_state that is being passing into those methods could be the delta index\n> in which case the number of entries for the fsmonitor bitmap would almost\n> always be more and cause the BUG to be hit which it should not be.\n\nThanks. Here is what I came up with to tie the loose ends of this\nthread.\n\n-- >8 --\nFrom: Junio C Hamano \nSubject: [PATCH] fsmonitor: do not compare bitmap size with size of split index\n\n3444ec2e (\"fsmonitor: don't fill bitmap with entries to be removed\",\n2019-10-11) added a handful of sanity checks that make sure that a\nbit position in fsmonitor bitmap does not go beyond the end of the\nindex. As each bit in the bitmap corresponds to a path in the\nindex, this is the right check most of the time.\n\nExcept for the case when we are in the split-index mode and looking\nat a delta index that is to be overlayed on the base index but\nbefore the base index has actually been merged in, namely in read_\nand write_fsmonitor_extension(). In these codepaths, the entries in\nthe split/delta index is typically a small subset of the entire set\nof paths (otherwise why would we be using split-index?), so the\nbitmap used by the fsmonitor is almost always larger than the number\nof entries in the partial index, and the incorrect comparison would\ntrigger the BUG().\n\nReported-by: Utsav Shah \nHelped-by: Kevin Willford \nHelped-by: William Baker \nSigned-off-by: Junio C Hamano \n---\n fsmonitor.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 1f4aa1b150..0477500b39 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -55,7 +55,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \t}\n \tistate->fsmonitor_dirty = fsmonitor_dirty;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\tif (!istate->split_index &&\n+\t istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \n@@ -83,7 +84,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n \n-\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n+\tif (!istate->split_index &&\n+\t istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \n-- \n2.24.0-346-gee0de6d492\n\n"},{"id":"386159","messageId":"CABhpXbfn6zOMaVkg_a8xhqT5JFRXxxWkdi1SjHq38z_NbMqdaQ@mail.gmail.com","threadId":"52215","inReplyTo":"xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Utsav Shah","fromEmail":"ukshah2@illinois.edu","sentAt":"2019-11-14T02:55:16Z","receivedAt":"2019-11-14T02:55:30Z","isPatch":true,"sender":{"key":"ukshah2@illinois.edu","avatar":null},"body":"This looks good to me.\n\nOn Tue, Nov 12, 2019 at 5:30 PM Junio C Hamano wrote:\n>\n> Kevin Willford writes:\n>\n> > I agree. The only 2 places that excluding the split-index make sense are in\n> > read_fsmonitor_extension and write_fsmonitor_extension because the\n> > index_state that is being passing into those methods could be the delta index\n> > in which case the number of entries for the fsmonitor bitmap would almost\n> > always be more and cause the BUG to be hit which it should not be.\n>\n> Thanks. Here is what I came up with to tie the loose ends of this\n> thread.\n>\n> -- >8 --\n> From: Junio C Hamano \n> Subject: [PATCH] fsmonitor: do not compare bitmap size with size of split index\n>\n> 3444ec2e (\"fsmonitor: don't fill bitmap with entries to be removed\",\n> 2019-10-11) added a handful of sanity checks that make sure that a\n> bit position in fsmonitor bitmap does not go beyond the end of the\n> index. As each bit in the bitmap corresponds to a path in the\n> index, this is the right check most of the time.\n>\n> Except for the case when we are in the split-index mode and looking\n> at a delta index that is to be overlayed on the base index but\n> before the base index has actually been merged in, namely in read_\n> and write_fsmonitor_extension(). In these codepaths, the entries in\n> the split/delta index is typically a small subset of the entire set\n> of paths (otherwise why would we be using split-index?), so the\n> bitmap used by the fsmonitor is almost always larger than the number\n> of entries in the partial index, and the incorrect comparison would\n> trigger the BUG().\n>\n> Reported-by: Utsav Shah \n> Helped-by: Kevin Willford \n> Helped-by: William Baker \n> Signed-off-by: Junio C Hamano \n> ---\n> fsmonitor.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 1f4aa1b150..0477500b39 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -55,7 +55,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n> }\n> istate->fsmonitor_dirty = fsmonitor_dirty;\n>\n> - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> + if (!istate->split_index &&\n> + istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n>\n> @@ -83,7 +84,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> uint32_t ewah_size = 0;\n> int fixup = 0;\n>\n> - if (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> + if (!istate->split_index &&\n> + istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> BUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n>\n> --\n> 2.24.0-346-gee0de6d492\n>\n"},{"id":"386192","messageId":"beb5a2b4-f07c-c3fd-9e68-dbee85fba2ee@gmail.com","threadId":"52215","inReplyTo":"xmqqzhh0d0ma.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"William Baker","fromEmail":"williamtbakeremail@gmail.com","sentAt":"2019-11-14T16:41:25Z","receivedAt":"2019-11-14T16:41:29Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"\nOn 11/12/19 5:30 PM, Junio C Hamano wrote:\n> Thanks. Here is what I came up with to tie the loose ends of this\n> thread.\n> \n> -- >8 --\n> From: Junio C Hamano \n> Subject: [PATCH] fsmonitor: do not compare bitmap size with size of split index\n> \n> 3444ec2e (\"fsmonitor: don't fill bitmap with entries to be removed\",\n> 2019-10-11) added a handful of sanity checks that make sure that a\n> bit position in fsmonitor bitmap does not go beyond the end of the\n> index. As each bit in the bitmap corresponds to a path in the\n> index, this is the right check most of the time.\n> \n> Except for the case when we are in the split-index mode and looking\n> at a delta index that is to be overlayed on the base index but\n> before the base index has actually been merged in, namely in read_\n> and write_fsmonitor_extension(). In these codepaths, the entries in\n> the split/delta index is typically a small subset of the entire set\n> of paths (otherwise why would we be using split-index?), so the\n> bitmap used by the fsmonitor is almost always larger than the number\n> of entries in the partial index, and the incorrect comparison would\n> trigger the BUG().\n> \n> Reported-by: Utsav Shah \n> Helped-by: Kevin Willford \n> Helped-by: William Baker \n> Signed-off-by: Junio C Hamano \n> ---\n> fsmonitor.c | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 1f4aa1b150..0477500b39 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -55,7 +55,8 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n> \t}\n> \tistate->fsmonitor_dirty = fsmonitor_dirty;\n> \n> -\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> +\tif (!istate->split_index &&\n> +\t istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> \n> @@ -83,7 +84,8 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> \tuint32_t ewah_size = 0;\n> \tint fixup = 0;\n> \n> -\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> +\tif (!istate->split_index &&\n> +\t istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n> \n> \n\nThis looks good to me.\n\nThanks,\nWilliam\n"},{"id":"386252","messageId":"xmqqh83591ca.fsf@gitster-ct.c.googlers.com","threadId":"52215","inReplyTo":"beb5a2b4-f07c-c3fd-9e68-dbee85fba2ee@gmail.com","subject":"Re: [PATCH 0/1] fsmonitor: skip sanity check if the index is split","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-15T05:04:53Z","receivedAt":"2019-11-15T05:05:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Baker writes:\n\n> On 11/12/19 5:30 PM, Junio C Hamano wrote:\n>> Thanks. Here is what I came up with to tie the loose ends of this\n>> thread.\n>> ...\n>\n> This looks good to me.\n\nThanks.\n"}]}