{"thread":{"id":"51965","subject":"[PATCH 0/1] fsmonitor: don't fill bitmap with entries to be removed","startedAt":"2019-10-03T19:49:58Z","lastAt":"2019-10-15T19:07:44Z","messageCount":17,"participants":["William Baker via GitGitGadget","Johannes Schindelin","Junio C Hamano","William Baker","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"383344","messageId":"pull.372.git.gitgitgadget@gmail.com","threadId":"51965","inReplyTo":null,"subject":"[PATCH 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-03T19:49:55Z","receivedAt":"2019-10-03T19:49:58Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"This patch series fixes a segfault that I encountered while testing\nfsmonitor. Under some circumstances, the fsmonitor extension was being\nwritten with too many bits, and subsequent git commands would segfault when\ntrying to apply the bits to the index.\n\nAs part of these changes I've added some BUG checks that would have helped\ncatch this problem sooner. Special thanks to Dscho for pointing me in the\nright direction and suggesting a test that can reproduce the issue.\n\nThanks, William\n\nWilliam Baker (1):\n  fsmonitor: don't fill bitmap with entries to be removed\n\n fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n t/t7519-status-fsmonitor.sh | 12 ++++++++++++\n t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n 3 files changed, 60 insertions(+), 5 deletions(-)\n create mode 100755 t/t7519/fsmonitor-env\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-372/wilbaker/fix_git_fsmonitor_crash-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/372\n-- \ngitgitgadget\n"},{"id":"383345","messageId":"ce9bf4237e69fcaf2b3e8b50bb88ff61c3b0f710.1570132194.git.gitgitgadget@gmail.com","threadId":"51965","inReplyTo":"pull.372.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-03T19:49:56Z","receivedAt":"2019-10-03T19:50:02Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"From: William Baker <William.Baker@microsoft.com>\n\nWhile doing some testing with fsmonitor enabled I found\nthat git commands would segfault after staging and\nunstaging an untracked file.  Looking at the crash it\nappeared that fsmonitor_ewah_callback was attempting to\nadjust bits beyond the bounds of the index cache.\n\nDigging into how this could happen it became clear that\nthe fsmonitor extension must have been written with\nmore bits than there were entries in the index.  The\nroot cause ended up being that fill_fsmonitor_bitmap was\npopulating fsmonitor_dirty with bits for all entries in\nthe index, even those that had been marked for removal.\n\nTo solve this problem fill_fsmonitor_bitmap has been\nupdated to skip entries with the the CE_REMOVE flag set.\nWith this change the bits written for the fsmonitor\nextension will be consistent with the index entries\nwritten by do_write_index.  Additionally, BUG checks\nhave been added to detect if the number of bits in\nfsmonitor_dirty should ever exceed the number of\nentries in the index again.\n\nAnother option that was considered was moving the call\nto fill_fsmonitor_bitmap closer to where the index is\nwritten (and where the fsmonitor extension itself is\nwritten).  However, that did not work as the\nfsmonitor_dirty bitmap must be filled before the index\nis split during writing.\n\nSigned-off-by: William Baker <William.Baker@microsoft.com>\n---\n fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n t/t7519-status-fsmonitor.sh | 12 ++++++++++++\n t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n 3 files changed, 60 insertions(+), 5 deletions(-)\n create mode 100755 t/t7519/fsmonitor-env\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 231e83a94d..fa0b96d84e 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+\t\n+\tif (pos >= istate->cache_nr)\n+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)pos, (uintmax_t)istate->cache_nr);\n \n+\tce = istate->cache[pos];\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n }\n \n@@ -50,17 +55,24 @@ 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+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n+\n \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n }\n \n void fill_fsmonitor_bitmap(struct index_state *istate)\n {\n-\tunsigned int i;\n+\tunsigned int i, skipped = 0;\n \tistate->fsmonitor_dirty = ewah_new();\n-\tfor (i = 0; i < istate->cache_nr; i++)\n-\t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n-\t\t\tewah_set(istate->fsmonitor_dirty, i);\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tif (istate->cache[i]->ce_flags & CE_REMOVE)\n+\t\t\tskipped++;\n+\t\telse if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n+\t\t\tewah_set(istate->fsmonitor_dirty, i - skipped);\n+\t}\n }\n \n void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n@@ -71,6 +83,10 @@ 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+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n+\n \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n \n@@ -236,6 +252,9 @@ 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\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n+\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)istate->cache_nr);\n \t\t\tewah_each_bit(istate->fsmonitor_dirty, fsmonitor_ewah_callback, istate);\n \n \t\t\t/* Now mark the untracked cache for fsmonitor usage */\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 81a375fa0f..85df54f07c 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -354,4 +354,16 @@ test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_cmp expect actual\n '\n \n+# Use test files that start with 'z' so that the entries being added\n+# and removed appear at the end of the index.\n+test_expect_success 'status succeeds after staging/unstaging ' '\n+\ttest_commit initial &&\n+\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n+\ttouch $removed &&\n+\tgit add $removed &&\n+\ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n+\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n+\tFSMONITOR_LIST=\"$removed\" git status\n+'\n+\n test_done\ndiff --git a/t/t7519/fsmonitor-env b/t/t7519/fsmonitor-env\nnew file mode 100755\nindex 0000000000..8f1f7ab164\n--- /dev/null\n+++ b/t/t7519/fsmonitor-env\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# An test hook script to integrate with git to test fsmonitor.\n+#\n+# The hook is passed a version (currently 1) and a time in nanoseconds\n+# formatted as a string and outputs to stdout all files that have been\n+# modified since the given time. Paths must be relative to the root of\n+# the working tree and separated by a single NUL.\n+#\n+#echo \"$0 $*\" >&2\n+\n+if test \"$#\" -ne 2\n+then\n+\techo \"$0: exactly 2 arguments expected\" >&2\n+\texit 2\n+fi\n+\n+if test \"$1\" != 1\n+then\n+\techo \"Unsupported core.fsmonitor hook version.\" >&2\n+\texit 1\n+fi\n+\n+printf '%s\\n' $FSMONITOR_LIST\n-- \ngitgitgadget\n"},{"id":"383362","messageId":"nycvar.QRO.7.76.6.1910032306380.46@tvgsbejvaqbjf.bet","threadId":"51965","inReplyTo":"pull.372.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-03T21:08:24Z","receivedAt":"2019-10-03T21:08:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi all,\n\nOn Thu, 3 Oct 2019, William Baker via GitGitGadget wrote:\n\n> This patch series fixes a segfault that I encountered while testing\n> fsmonitor. Under some circumstances, the fsmonitor extension was being\n> written with too many bits, and subsequent git commands would segfault when\n> trying to apply the bits to the index.\n>\n> As part of these changes I've added some BUG checks that would have helped\n> catch this problem sooner. Special thanks to Dscho for pointing me in the\n> right direction and suggesting a test that can reproduce the issue.\n>\n> Thanks, William\n\nPlease note that I was involved with the development of this patch,\nreviewed a couple of iterations internally and am implictly okay with\nthis first public iteration.\n\nCiao,\nDscho\n\n>\n> William Baker (1):\n>   fsmonitor: don't fill bitmap with entries to be removed\n>\n>  fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n>  t/t7519-status-fsmonitor.sh | 12 ++++++++++++\n>  t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n>  3 files changed, 60 insertions(+), 5 deletions(-)\n>  create mode 100755 t/t7519/fsmonitor-env\n>\n>\n> base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-372/wilbaker/fix_git_fsmonitor_crash-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/372\n> --\n> gitgitgadget\n>\n"},{"id":"383379","messageId":"xmqqd0fda07a.fsf@gitster-ct.c.googlers.com","threadId":"51965","inReplyTo":"nycvar.QRO.7.76.6.1910032306380.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T23:17:13Z","receivedAt":"2019-10-03T23:17:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi all,\n>\n> On Thu, 3 Oct 2019, William Baker via GitGitGadget wrote:\n>\n>> This patch series fixes a segfault that I encountered while testing\n>> fsmonitor. Under some circumstances, the fsmonitor extension was being\n>> written with too many bits, and subsequent git commands would segfault when\n>> trying to apply the bits to the index.\n>>\n>> As part of these changes I've added some BUG checks that would have helped\n>> catch this problem sooner. Special thanks to Dscho for pointing me in the\n>> right direction and suggesting a test that can reproduce the issue.\n>>\n>> Thanks, William\n>\n> Please note that I was involved with the development of this patch,\n> reviewed a couple of iterations internally and am implictly okay with\n> this first public iteration.\n\nIOW, \"Reviewed-by: dscho\".  That's good to hear.\n\nTHanks.\n"},{"id":"383385","messageId":"xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com","threadId":"51965","inReplyTo":"ce9bf4237e69fcaf2b3e8b50bb88ff61c3b0f710.1570132194.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T23:36:34Z","receivedAt":"2019-10-03T23:36:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"William Baker via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>  create mode 100755 t/t7519/fsmonitor-env\n> ...\n> +\tif (pos >= istate->cache_nr)\n> +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n> +\t\t    (uintmax_t)pos, (uintmax_t)istate->cache_nr);\n\nThis is how we show size_t values without using \"%z\" that we avoid,\nbut are \"pos\" and 'cache_nr\" size_t or ssize_t?  I thought they are\nplain boring unsigned, so shouldn't we use the plain boring \"%u\"\nwithout casting?\n\nThe same comment applies to other uses of uintmax_t cast in this\npatch.\n\n>  void fill_fsmonitor_bitmap(struct index_state *istate)\n>  {\n> -\tunsigned int i;\n> +\tunsigned int i, skipped = 0;\n>  \tistate->fsmonitor_dirty = ewah_new();\n> -\tfor (i = 0; i < istate->cache_nr; i++)\n> -\t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n> -\t\t\tewah_set(istate->fsmonitor_dirty, i);\n> +\tfor (i = 0; i < istate->cache_nr; i++) {\n> +\t\tif (istate->cache[i]->ce_flags & CE_REMOVE)\n> +\t\t\tskipped++;\n> +\t\telse if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n> +\t\t\tewah_set(istate->fsmonitor_dirty, i - skipped);\n> +\t}\n>  }\n\nMatches the explanation in the proposed log message pretty well.\nGood job.\n\n> @@ -354,4 +354,16 @@ test_expect_success 'discard_index() also discards fsmonitor info' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +# Use test files that start with 'z' so that the entries being added\n> +# and removed appear at the end of the index.\n\nIn other words, future developers are warned against adding entries\nto and leaving them in the index that sort later than z100 in new\ntests they add before this point.  Is the above wording clear enough\nto tell them that, I wonder?\n\n> +test_expect_success 'status succeeds after staging/unstaging ' '\n> +\ttest_commit initial &&\n> +\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n\nThanks.\n"},{"id":"383597","messageId":"0de8e44f-53b8-e530-6e75-f696ffa889ce@gmail.com","threadId":"51965","inReplyTo":"xmqq7e5l9zb1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker","fromEmail":"williamtbakeremail@gmail.com","sentAt":"2019-10-07T18:10:05Z","receivedAt":"2019-10-07T18:10:10Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"On 10/3/19 4:36 PM, Junio C Hamano wrote:\n\n>> +\tif (pos >= istate->cache_nr)\n>> +\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n>> +\t\t    (uintmax_t)pos, (uintmax_t)istate->cache_nr);\n> \n> This is how we show size_t values without using \"%z\" that we avoid,\n> but are \"pos\" and 'cache_nr\" size_t or ssize_t?  I thought they are\n> plain boring unsigned, so shouldn't we use the plain boring \"%u\"\n> without casting?\n> \n> The same comment applies to other uses of uintmax_t cast in this\n> patch.\n> \n\nThanks for catching this.  I will update these BUGs in the next\npatch to avoid casting.\n\n>> +# Use test files that start with 'z' so that the entries being added\n>> +# and removed appear at the end of the index.\n> \n> In other words, future developers are warned against adding entries\n> to and leaving them in the index that sort later than z100 in new\n> tests they add before this point.  Is the above wording clear enough\n> to tell them that, I wonder?\n> \n\nYou're understanding is correct, and I agree this comment could be\nclearer.  I will fix this up in v2.\n\nThanks for the feedback!\nWilliam\n"},{"id":"383766","messageId":"08741d986c2b51828f115ab50f178d62e9982978.1570654810.git.gitgitgadget@gmail.com","threadId":"51965","inReplyTo":"pull.372.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-09T21:00:12Z","receivedAt":"2019-10-09T21:00:18Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"From: William Baker <William.Baker@microsoft.com>\n\nWhile doing some testing with fsmonitor enabled I found\nthat git commands would segfault after staging and\nunstaging an untracked file.  Looking at the crash it\nappeared that fsmonitor_ewah_callback was attempting to\nadjust bits beyond the bounds of the index cache.\n\nDigging into how this could happen it became clear that\nthe fsmonitor extension must have been written with\nmore bits than there were entries in the index.  The\nroot cause ended up being that fill_fsmonitor_bitmap was\npopulating fsmonitor_dirty with bits for all entries in\nthe index, even those that had been marked for removal.\n\nTo solve this problem fill_fsmonitor_bitmap has been\nupdated to skip entries with the the CE_REMOVE flag set.\nWith this change the bits written for the fsmonitor\nextension will be consistent with the index entries\nwritten by do_write_index.  Additionally, BUG checks\nhave been added to detect if the number of bits in\nfsmonitor_dirty should ever exceed the number of\nentries in the index again.\n\nAnother option that was considered was moving the call\nto fill_fsmonitor_bitmap closer to where the index is\nwritten (and where the fsmonitor extension itself is\nwritten).  However, that did not work as the\nfsmonitor_dirty bitmap must be filled before the index\nis split during writing.\n\nSigned-off-by: William Baker <William.Baker@microsoft.com>\n---\n fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n t/t7519-status-fsmonitor.sh | 13 +++++++++++++\n t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n 3 files changed, 61 insertions(+), 5 deletions(-)\n create mode 100755 t/t7519/fsmonitor-env\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 231e83a94d..7e27839278 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+\t\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 }\n \n@@ -50,17 +55,24 @@ 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+\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 \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n }\n \n void fill_fsmonitor_bitmap(struct index_state *istate)\n {\n-\tunsigned int i;\n+\tunsigned int i, skipped = 0;\n \tistate->fsmonitor_dirty = ewah_new();\n-\tfor (i = 0; i < istate->cache_nr; i++)\n-\t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n-\t\t\tewah_set(istate->fsmonitor_dirty, i);\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tif (istate->cache[i]->ce_flags & CE_REMOVE)\n+\t\t\tskipped++;\n+\t\telse if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n+\t\t\tewah_set(istate->fsmonitor_dirty, i - skipped);\n+\t}\n }\n \n void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n@@ -71,6 +83,10 @@ 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+\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 \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n \n@@ -236,6 +252,9 @@ 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\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);\n \n \t\t\t/* Now mark the untracked cache for fsmonitor usage */\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 81a375fa0f..87042470ab 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -354,4 +354,17 @@ test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_cmp expect actual\n '\n \n+# This test covers staging/unstaging files that appear at the end of the index.\n+# Test files with names beginning with 'z' are used under the assumption that\n+# earlier tests do not add/leave index entries that sort below them. \n+test_expect_success 'status succeeds after staging/unstaging ' '\n+\ttest_commit initial &&\n+\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n+\ttouch $removed &&\n+\tgit add $removed &&\n+\ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n+\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n+\tFSMONITOR_LIST=\"$removed\" git status\n+'\n+\n test_done\ndiff --git a/t/t7519/fsmonitor-env b/t/t7519/fsmonitor-env\nnew file mode 100755\nindex 0000000000..8f1f7ab164\n--- /dev/null\n+++ b/t/t7519/fsmonitor-env\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# An test hook script to integrate with git to test fsmonitor.\n+#\n+# The hook is passed a version (currently 1) and a time in nanoseconds\n+# formatted as a string and outputs to stdout all files that have been\n+# modified since the given time. Paths must be relative to the root of\n+# the working tree and separated by a single NUL.\n+#\n+#echo \"$0 $*\" >&2\n+\n+if test \"$#\" -ne 2\n+then\n+\techo \"$0: exactly 2 arguments expected\" >&2\n+\texit 2\n+fi\n+\n+if test \"$1\" != 1\n+then\n+\techo \"Unsupported core.fsmonitor hook version.\" >&2\n+\texit 1\n+fi\n+\n+printf '%s\\n' $FSMONITOR_LIST\n-- \ngitgitgadget\n"},{"id":"383767","messageId":"pull.372.v2.git.gitgitgadget@gmail.com","threadId":"51965","inReplyTo":"pull.372.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-09T21:00:11Z","receivedAt":"2019-10-09T21:00:19Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"This is the second iteration of changes to fix the segfault that I\nencountered while testing fsmonitor. This iteration includes the following\nupdates for feedback I received on v1:\n\n * Use %u instead of %\"PRIuMAX\" for unsigned ints in BUG format strings\n * Updated the new test's comment to be more descriptive\n\nThanks,\n\nWilliam\n\nWilliam Baker (1):\n  fsmonitor: don't fill bitmap with entries to be removed\n\n fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n t/t7519-status-fsmonitor.sh | 13 +++++++++++++\n t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n 3 files changed, 61 insertions(+), 5 deletions(-)\n create mode 100755 t/t7519/fsmonitor-env\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-372/wilbaker/fix_git_fsmonitor_crash-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/372\n\nRange-diff vs v1:\n\n 1:  ce9bf4237e ! 1:  08741d986c fsmonitor: don't fill bitmap with entries to be removed\n     @@ -44,8 +44,8 @@\n      +\tstruct cache_entry *ce;\n      +\t\n      +\tif (pos >= istate->cache_nr)\n     -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n     -+\t\t    (uintmax_t)pos, (uintmax_t)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     @@ -56,8 +56,8 @@\n       \tistate->fsmonitor_dirty = fsmonitor_dirty;\n       \n      +\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n     -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n     -+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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       \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n       \treturn 0;\n     @@ -85,8 +85,8 @@\n       \tint fixup = 0;\n       \n      +\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n     -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n     -+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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       \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n       \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n     @@ -96,8 +96,8 @@\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\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n     -+\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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);\n       \n       \t\t\t/* Now mark the untracked cache for fsmonitor usage */\n     @@ -109,8 +109,9 @@\n       \ttest_cmp expect actual\n       '\n       \n     -+# Use test files that start with 'z' so that the entries being added\n     -+# and removed appear at the end of the index.\n     ++# This test covers staging/unstaging files that appear at the end of the index.\n     ++# Test files with names beginning with 'z' are used under the assumption that\n     ++# earlier tests do not add/leave index entries that sort below them. \n      +test_expect_success 'status succeeds after staging/unstaging ' '\n      +\ttest_commit initial &&\n      +\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n\n-- \ngitgitgadget\n"},{"id":"383776","messageId":"xmqq1rvll5w8.fsf@gitster-ct.c.googlers.com","threadId":"51965","inReplyTo":"pull.372.v2.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-10T01:56:55Z","receivedAt":"2019-10-10T01:59:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"William Baker via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> This is the second iteration of changes to fix the segfault that I\n> encountered while testing fsmonitor. This iteration includes the following\n> updates for feedback I received on v1:\n>\n>  * Use %u instead of %\"PRIuMAX\" for unsigned ints in BUG format strings\n>  * Updated the new test's comment to be more descriptive\n>\n> Thanks,\n>\n> William\n>\n> William Baker (1):\n>   fsmonitor: don't fill bitmap with entries to be removed\n\nThanks.  Dscho, is this still Reviewed-by: you?\n\n>\n>  fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n>  t/t7519-status-fsmonitor.sh | 13 +++++++++++++\n>  t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n>  3 files changed, 61 insertions(+), 5 deletions(-)\n>  create mode 100755 t/t7519/fsmonitor-env\n>\n>\n> base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-372/wilbaker/fix_git_fsmonitor_crash-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/372\n>\n> Range-diff vs v1:\n>\n>  1:  ce9bf4237e ! 1:  08741d986c fsmonitor: don't fill bitmap with entries to be removed\n>      @@ -44,8 +44,8 @@\n>       +\tstruct cache_entry *ce;\n>       +\t\n>       +\tif (pos >= istate->cache_nr)\n>      -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n>      -+\t\t    (uintmax_t)pos, (uintmax_t)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>      @@ -56,8 +56,8 @@\n>        \tistate->fsmonitor_dirty = fsmonitor_dirty;\n>        \n>       +\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n>      -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n>      -+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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>        \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n>        \treturn 0;\n>      @@ -85,8 +85,8 @@\n>        \tint fixup = 0;\n>        \n>       +\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n>      -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n>      -+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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>        \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n>        \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n>      @@ -96,8 +96,8 @@\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\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n>      -+\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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);\n>        \n>        \t\t\t/* Now mark the untracked cache for fsmonitor usage */\n>      @@ -109,8 +109,9 @@\n>        \ttest_cmp expect actual\n>        '\n>        \n>      -+# Use test files that start with 'z' so that the entries being added\n>      -+# and removed appear at the end of the index.\n>      ++# This test covers staging/unstaging files that appear at the end of the index.\n>      ++# Test files with names beginning with 'z' are used under the assumption that\n>      ++# earlier tests do not add/leave index entries that sort below them. \n>       +test_expect_success 'status succeeds after staging/unstaging ' '\n>       +\ttest_commit initial &&\n>       +\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n"},{"id":"383788","messageId":"20191010110732.GJ29845@szeder.dev","threadId":"51965","inReplyTo":"08741d986c2b51828f115ab50f178d62e9982978.1570654810.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-10T11:07:32Z","receivedAt":"2019-10-10T11:07:38Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Oct 09, 2019 at 02:00:12PM -0700, William Baker via GitGitGadget wrote:\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index 81a375fa0f..87042470ab 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -354,4 +354,17 @@ test_expect_success 'discard_index() also discards fsmonitor info' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +# This test covers staging/unstaging files that appear at the end of the index.\n> +# Test files with names beginning with 'z' are used under the assumption that\n> +# earlier tests do not add/leave index entries that sort below them. \n> +test_expect_success 'status succeeds after staging/unstaging ' '\n> +\ttest_commit initial &&\n\nThis is confusing: this is the 29th test case in this script and it\ncreates an \"initial\" commit?!\n\nThe first \"setup\" test case has already created an initial commit, so\nthis should rather be called \"second\".\n\nOTOH, none of the later commands in this test case seem to have\nanything to do with this second commit, and indeed the test case works\neven without it (i.e. 'git status' still segfaults without the fix and\nthen succeeds with the fix applied), so instead of updating its\nmessage perhaps it could simply be removed.\n\n> +\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n> +\ttouch $removed &&\n> +\tgit add $removed &&\n> +\ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n> +\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n> +\tFSMONITOR_LIST=\"$removed\" git status\n> +'\n> +\n>  test_done\n"},{"id":"383789","messageId":"20191010112204.GK29845@szeder.dev","threadId":"51965","inReplyTo":"20191010110732.GJ29845@szeder.dev","subject":"Re: [PATCH v2 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-10-10T11:22:04Z","receivedAt":"2019-10-10T11:22:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Oct 10, 2019 at 01:07:32PM +0200, SZEDER Gábor wrote:\n> On Wed, Oct 09, 2019 at 02:00:12PM -0700, William Baker via GitGitGadget wrote:\n> > diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> > index 81a375fa0f..87042470ab 100755\n> > --- a/t/t7519-status-fsmonitor.sh\n> > +++ b/t/t7519-status-fsmonitor.sh\n> > @@ -354,4 +354,17 @@ test_expect_success 'discard_index() also discards fsmonitor info' '\n> >  \ttest_cmp expect actual\n> >  '\n> >  \n> > +# This test covers staging/unstaging files that appear at the end of the index.\n> > +# Test files with names beginning with 'z' are used under the assumption that\n> > +# earlier tests do not add/leave index entries that sort below them. \n\nI just read through Junio's comments on the first version of this\npatch, in particular his remarks about this comment.\n\nIf this new test case below were run in a dedicated repository, then\nthis comment wouldn't be necessary, and all my comments below about\nthat not-really-initial commit would be moot, too.\n\n> > +test_expect_success 'status succeeds after staging/unstaging ' '\n> > +\ttest_commit initial &&\n> \n> This is confusing: this is the 29th test case in this script and it\n> creates an \"initial\" commit?!\n> \n> The first \"setup\" test case has already created an initial commit, so\n> this should rather be called \"second\".\n> \n> OTOH, none of the later commands in this test case seem to have\n> anything to do with this second commit, and indeed the test case works\n> even without it (i.e. 'git status' still segfaults without the fix and\n> then succeeds with the fix applied), so instead of updating its\n> message perhaps it could simply be removed.\n\n> \n> > +\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n> > +\ttouch $removed &&\n> > +\tgit add $removed &&\n> > +\ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n> > +\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n> > +\tFSMONITOR_LIST=\"$removed\" git status\n> > +'\n> > +\n> >  test_done\n"},{"id":"383797","messageId":"nycvar.QRO.7.76.6.1910101401310.46@tvgsbejvaqbjf.bet","threadId":"51965","inReplyTo":"xmqq1rvll5w8.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-10T12:02:46Z","receivedAt":"2019-10-10T12:03:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 10 Oct 2019, Junio C Hamano wrote:\n\n> \"William Baker via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > This is the second iteration of changes to fix the segfault that I\n> > encountered while testing fsmonitor. This iteration includes the following\n> > updates for feedback I received on v1:\n> >\n> >  * Use %u instead of %\"PRIuMAX\" for unsigned ints in BUG format strings\n> >  * Updated the new test's comment to be more descriptive\n> >\n> > Thanks,\n> >\n> > William\n> >\n> > William Baker (1):\n> >   fsmonitor: don't fill bitmap with entries to be removed\n>\n> Thanks.  Dscho, is this still Reviewed-by: you?\n\nYes!\n\nThanks,\nDscho\n\nP.S.: I guess we could even go and make this a `Co-Authored-by:` because\nthe test case is actually by me, at least originally... ;-)\n\n>\n> >\n> >  fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n> >  t/t7519-status-fsmonitor.sh | 13 +++++++++++++\n> >  t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n> >  3 files changed, 61 insertions(+), 5 deletions(-)\n> >  create mode 100755 t/t7519/fsmonitor-env\n> >\n> >\n> > base-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v2\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-372/wilbaker/fix_git_fsmonitor_crash-v2\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/372\n> >\n> > Range-diff vs v1:\n> >\n> >  1:  ce9bf4237e ! 1:  08741d986c fsmonitor: don't fill bitmap with entries to be removed\n> >      @@ -44,8 +44,8 @@\n> >       +\tstruct cache_entry *ce;\n> >       +\n> >       +\tif (pos >= istate->cache_nr)\n> >      -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" >= %\"PRIuMAX\")\",\n> >      -+\t\t    (uintmax_t)pos, (uintmax_t)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> >      @@ -56,8 +56,8 @@\n> >        \tistate->fsmonitor_dirty = fsmonitor_dirty;\n> >\n> >       +\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> >      -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> >      -+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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> >        \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n> >        \treturn 0;\n> >      @@ -85,8 +85,8 @@\n> >        \tint fixup = 0;\n> >\n> >       +\tif (istate->fsmonitor_dirty->bit_size > istate->cache_nr)\n> >      -+\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> >      -+\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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> >        \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n> >        \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n> >      @@ -96,8 +96,8 @@\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\t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %\"PRIuMAX\")\",\n> >      -+\t\t\t\t    (uintmax_t)istate->fsmonitor_dirty->bit_size, (uintmax_t)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);\n> >\n> >        \t\t\t/* Now mark the untracked cache for fsmonitor usage */\n> >      @@ -109,8 +109,9 @@\n> >        \ttest_cmp expect actual\n> >        '\n> >\n> >      -+# Use test files that start with 'z' so that the entries being added\n> >      -+# and removed appear at the end of the index.\n> >      ++# This test covers staging/unstaging files that appear at the end of the index.\n> >      ++# Test files with names beginning with 'z' are used under the assumption that\n> >      ++# earlier tests do not add/leave index entries that sort below them.\n> >       +test_expect_success 'status succeeds after staging/unstaging ' '\n> >       +\ttest_commit initial &&\n> >       +\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n>\n"},{"id":"383903","messageId":"9c012ff1-182b-d71b-cd50-9c476f26748c@gmail.com","threadId":"51965","inReplyTo":"20191010112204.GK29845@szeder.dev","subject":"Re: [PATCH v2 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker","fromEmail":"williamtbakeremail@gmail.com","sentAt":"2019-10-11T16:38:41Z","receivedAt":"2019-10-11T16:38:47Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"On 10/10/19 4:22 AM, SZEDER Gábor wrote:\n>>> +# This test covers staging/unstaging files that appear at the end of the index.\n>>> +# Test files with names beginning with 'z' are used under the assumption that\n>>> +# earlier tests do not add/leave index entries that sort below them. \n> \n> I just read through Junio's comments on the first version of this\n> patch, in particular his remarks about this comment.\n> \n> If this new test case below were run in a dedicated repository, then\n> this comment wouldn't be necessary, and all my comments below about\n> that not-really-initial commit would be moot, too.\n> \n\nThanks for this suggestion!  I will submit a v3 version of the patch\nwith an update to the test script.\n\n- William\n\n"},{"id":"383927","messageId":"840972e08b2178e89b2c3ed77eb20c91ead894ad.1570824681.git.gitgitgadget@gmail.com","threadId":"51965","inReplyTo":"pull.372.v3.git.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-11T20:11:23Z","receivedAt":"2019-10-11T20:11:29Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"From: William Baker <William.Baker@microsoft.com>\n\nWhile doing some testing with fsmonitor enabled I found\nthat git commands would segfault after staging and\nunstaging an untracked file.  Looking at the crash it\nappeared that fsmonitor_ewah_callback was attempting to\nadjust bits beyond the bounds of the index cache.\n\nDigging into how this could happen it became clear that\nthe fsmonitor extension must have been written with\nmore bits than there were entries in the index.  The\nroot cause ended up being that fill_fsmonitor_bitmap was\npopulating fsmonitor_dirty with bits for all entries in\nthe index, even those that had been marked for removal.\n\nTo solve this problem fill_fsmonitor_bitmap has been\nupdated to skip entries with the the CE_REMOVE flag set.\nWith this change the bits written for the fsmonitor\nextension will be consistent with the index entries\nwritten by do_write_index.  Additionally, BUG checks\nhave been added to detect if the number of bits in\nfsmonitor_dirty should ever exceed the number of\nentries in the index again.\n\nAnother option that was considered was moving the call\nto fill_fsmonitor_bitmap closer to where the index is\nwritten (and where the fsmonitor extension itself is\nwritten).  However, that did not work as the\nfsmonitor_dirty bitmap must be filled before the index\nis split during writing.\n\nSigned-off-by: William Baker <William.Baker@microsoft.com>\n---\n fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n t/t7519-status-fsmonitor.sh | 17 +++++++++++++++++\n t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n 3 files changed, 65 insertions(+), 5 deletions(-)\n create mode 100755 t/t7519/fsmonitor-env\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 231e83a94d..7e27839278 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+\t\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 }\n \n@@ -50,17 +55,24 @@ 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+\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 \ttrace_printf_key(&trace_fsmonitor, \"read fsmonitor extension successful\");\n \treturn 0;\n }\n \n void fill_fsmonitor_bitmap(struct index_state *istate)\n {\n-\tunsigned int i;\n+\tunsigned int i, skipped = 0;\n \tistate->fsmonitor_dirty = ewah_new();\n-\tfor (i = 0; i < istate->cache_nr; i++)\n-\t\tif (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n-\t\t\tewah_set(istate->fsmonitor_dirty, i);\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tif (istate->cache[i]->ce_flags & CE_REMOVE)\n+\t\t\tskipped++;\n+\t\telse if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))\n+\t\t\tewah_set(istate->fsmonitor_dirty, i - skipped);\n+\t}\n }\n \n void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n@@ -71,6 +83,10 @@ 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+\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 \tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n \n@@ -236,6 +252,9 @@ 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\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);\n \n \t\t\t/* Now mark the untracked cache for fsmonitor usage */\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 81a375fa0f..af3f9951fa 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -354,4 +354,21 @@ test_expect_success 'discard_index() also discards fsmonitor info' '\n \ttest_cmp expect actual\n '\n \n+# Test staging/unstaging files that appear at the end of the index.  Test\n+# file names begin with 'z' so that they are sorted to the end of the index. \n+test_expect_success 'status succeeds after staging/unstaging ' '\n+\ttest_create_repo fsmonitor-stage-unstage &&\n+\t(\n+\t\tcd fsmonitor-stage-unstage &&\n+\t\ttest_commit initial &&\n+\t\tgit update-index --fsmonitor &&\n+\t\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n+\t\ttouch $removed &&\n+\t\tgit add $removed &&\n+\t\tgit config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n+\t\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n+\t\tFSMONITOR_LIST=\"$removed\" git status\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t7519/fsmonitor-env b/t/t7519/fsmonitor-env\nnew file mode 100755\nindex 0000000000..8f1f7ab164\n--- /dev/null\n+++ b/t/t7519/fsmonitor-env\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+#\n+# An test hook script to integrate with git to test fsmonitor.\n+#\n+# The hook is passed a version (currently 1) and a time in nanoseconds\n+# formatted as a string and outputs to stdout all files that have been\n+# modified since the given time. Paths must be relative to the root of\n+# the working tree and separated by a single NUL.\n+#\n+#echo \"$0 $*\" >&2\n+\n+if test \"$#\" -ne 2\n+then\n+\techo \"$0: exactly 2 arguments expected\" >&2\n+\texit 2\n+fi\n+\n+if test \"$1\" != 1\n+then\n+\techo \"Unsupported core.fsmonitor hook version.\" >&2\n+\texit 1\n+fi\n+\n+printf '%s\\n' $FSMONITOR_LIST\n-- \ngitgitgadget\n"},{"id":"383928","messageId":"pull.372.v3.git.gitgitgadget@gmail.com","threadId":"51965","inReplyTo":"pull.372.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-11T20:11:22Z","receivedAt":"2019-10-11T20:11:32Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"This is the third iteration of changes to fix the segfault that I\nencountered while testing fsmonitor. This iteration includes the following\nupdates for feedback I received on v2:\n\n * Update the new test case to use its own dedicated test repository\n\nThis latest v3 series has been reviewed by Dscho.\n\nThanks, William\n\nWilliam Baker (1):\n  fsmonitor: don't fill bitmap with entries to be removed\n\n fsmonitor.c                 | 29 ++++++++++++++++++++++++-----\n t/t7519-status-fsmonitor.sh | 17 +++++++++++++++++\n t/t7519/fsmonitor-env       | 24 ++++++++++++++++++++++++\n 3 files changed, 65 insertions(+), 5 deletions(-)\n create mode 100755 t/t7519/fsmonitor-env\n\n\nbase-commit: 5fa0f5238b0cd46cfe7f6fa76c3f526ea98148d9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-372%2Fwilbaker%2Ffix_git_fsmonitor_crash-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-372/wilbaker/fix_git_fsmonitor_crash-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/372\n\nRange-diff vs v2:\n\n 1:  08741d986c ! 1:  840972e08b fsmonitor: don't fill bitmap with entries to be removed\n     @@ -109,17 +109,21 @@\n       \ttest_cmp expect actual\n       '\n       \n     -+# This test covers staging/unstaging files that appear at the end of the index.\n     -+# Test files with names beginning with 'z' are used under the assumption that\n     -+# earlier tests do not add/leave index entries that sort below them. \n     ++# Test staging/unstaging files that appear at the end of the index.  Test\n     ++# file names begin with 'z' so that they are sorted to the end of the index. \n      +test_expect_success 'status succeeds after staging/unstaging ' '\n     -+\ttest_commit initial &&\n     -+\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n     -+\ttouch $removed &&\n     -+\tgit add $removed &&\n     -+\ttest_config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n     -+\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n     -+\tFSMONITOR_LIST=\"$removed\" git status\n     ++\ttest_create_repo fsmonitor-stage-unstage &&\n     ++\t(\n     ++\t\tcd fsmonitor-stage-unstage &&\n     ++\t\ttest_commit initial &&\n     ++\t\tgit update-index --fsmonitor &&\n     ++\t\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n     ++\t\ttouch $removed &&\n     ++\t\tgit add $removed &&\n     ++\t\tgit config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n     ++\t\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n     ++\t\tFSMONITOR_LIST=\"$removed\" git status\n     ++\t)\n      +'\n      +\n       test_done\n\n-- \ngitgitgadget\n"},{"id":"383958","messageId":"xmqq1rvig3fb.fsf@gitster-ct.c.googlers.com","threadId":"51965","inReplyTo":"840972e08b2178e89b2c3ed77eb20c91ead894ad.1570824681.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-12T01:26:00Z","receivedAt":"2019-10-12T01:26:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"William Baker via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> +# Test staging/unstaging files that appear at the end of the index.  Test\n> +# file names begin with 'z' so that they are sorted to the end of the index. \n\nWell, the test is now done in a freshly created repository, so the\nz* files are the only thing you have in here---technically they are\nat the end of the index, but so they are at the beginning, too.\n\nWould it affect the effectiveness of the test that you do not have\nany other paths in the working tree or in the index, unlike the test\nin the previous rounds that did not use a newly created test\nrepository?  \n\nThis is not a rhetorical question, but purely asking. \"no, this\nstill tests what we want to test and shows breakage when the fix to\nthe code in the patch gets reverted\" is perfectly a good answer, but\nin that case, is \"the end of\" the most important trait of the\ncondition this test is checking?  Wouldn't the bug be exposed as\nlong as we remove sufficiently large number of entries (like\n\"removing more paths than the paths still in the index at the end\"\nor something like that)?\n\nThanks.\n\n> +test_expect_success 'status succeeds after staging/unstaging ' '\n> +\ttest_create_repo fsmonitor-stage-unstage &&\n> +\t(\n> +\t\tcd fsmonitor-stage-unstage &&\n> +\t\ttest_commit initial &&\n> +\t\tgit update-index --fsmonitor &&\n> +\t\tremoved=$(test_seq 1 100 | sed \"s/^/z/\") &&\n> +\t\ttouch $removed &&\n> +\t\tgit add $removed &&\n> +\t\tgit config core.fsmonitor \"$TEST_DIRECTORY/t7519/fsmonitor-env\" &&\n> +\t\tFSMONITOR_LIST=\"$removed\" git restore -S $removed &&\n> +\t\tFSMONITOR_LIST=\"$removed\" git status\n> +\t)\n> +'\n> +\n>  test_done\n> diff --git a/t/t7519/fsmonitor-env b/t/t7519/fsmonitor-env\n> new file mode 100755\n> index 0000000000..8f1f7ab164\n> --- /dev/null\n> +++ b/t/t7519/fsmonitor-env\n> @@ -0,0 +1,24 @@\n> +#!/bin/sh\n> +#\n> +# An test hook script to integrate with git to test fsmonitor.\n> +#\n> +# The hook is passed a version (currently 1) and a time in nanoseconds\n> +# formatted as a string and outputs to stdout all files that have been\n> +# modified since the given time. Paths must be relative to the root of\n> +# the working tree and separated by a single NUL.\n> +#\n> +#echo \"$0 $*\" >&2\n> +\n> +if test \"$#\" -ne 2\n> +then\n> +\techo \"$0: exactly 2 arguments expected\" >&2\n> +\texit 2\n> +fi\n> +\n> +if test \"$1\" != 1\n> +then\n> +\techo \"Unsupported core.fsmonitor hook version.\" >&2\n> +\texit 1\n> +fi\n> +\n> +printf '%s\\n' $FSMONITOR_LIST\n"},{"id":"384116","messageId":"3d3b290c-bc15-4bd5-e0c0-1377c9ec3ff9@gmail.com","threadId":"51965","inReplyTo":"xmqq1rvig3fb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] fsmonitor: don't fill bitmap with entries to be removed","fromName":"William Baker","fromEmail":"williamtbakeremail@gmail.com","sentAt":"2019-10-15T19:07:39Z","receivedAt":"2019-10-15T19:07:44Z","isPatch":true,"sender":{"key":"williamtbakeremail@gmail.com","avatar":null},"body":"On 10/11/19 6:26 PM, Junio C Hamano wrote:\n> \"William Baker via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> +# Test staging/unstaging files that appear at the end of the index.  Test\n>> +# file names begin with 'z' so that they are sorted to the end of the index. \n> \n> Well, the test is now done in a freshly created repository, so the\n> z* files are the only thing you have in here---technically they are\n> at the end of the index, but so they are at the beginning, too.\n> \n\nThere is one other file in the index created by 'test_commit', however,\nthe point still stands that there are almost no other entries in the\nindex now that the test is using its own repository.\n\n> Would it affect the effectiveness of the test that you do not have\n> any other paths in the working tree or in the index, unlike the test\n> in the previous rounds that did not use a newly created test\n> repository?  \n\nThe test still validates the scenario that we're concerned about,\nnamely that the new index that's written has less entries than\nthe index of the last entry in the old index that's is not flagged\nwith CE_FSMONITOR_VALID but is flagged for removal (CE_REMOVE).\n\n> This is not a rhetorical question, but purely asking. \"no, this\n> still tests what we want to test and shows breakage when the fix to\n> the code in the patch gets reverted\" is perfectly a good answer, but\n> in that case, is \"the end of\" the most important trait of the\n> condition this test is checking?  Wouldn't the bug be exposed as\n> long as we remove sufficiently large number of entries (like\n> \"removing more paths than the paths still in the index at the end\"\n> or something like that)?\n\nThis is exactly right.  The most important trait is that the last\nentry flagged with CE_REMOVE does not have CE_FSMONITOR_VALID set\nand has an index >= the number of entries in the new index being\nwritten.\n\nI will send out a patch on top of 'wb/fsmonitor-bitmap-fix' with\nan update to the comment for this test.\n\nThanks,\nWilliam\n"}]}