{"thread":{"id":"51168","subject":"[PATCH 0/2] Fix racy fsmonitor","startedAt":"2019-05-24T12:23:49Z","lastAt":"2019-05-24T12:23:52Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"376184","messageId":"pull.223.git.gitgitgadget@gmail.com","threadId":"51168","inReplyTo":null,"subject":"[PATCH 0/2] Fix racy fsmonitor","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-24T12:23:46Z","receivedAt":"2019-05-24T12:23:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The t7519-status-fsmonitor.sh tests became a lot more flaky with the recent\nfsmonitor fix (js/fsmonitor-refresh-after-discarding-index). That fix,\nhowever, did not introduce the flakiness, but it just made it much more\nlikely to be hit. And it seemed to be hit only on Windows.\n\nThe reason, though, is that the fsmonitor feature failed to mark the\nin-memory index as changed, i.e. in need of writing, and it was the \nhas_racy_timestamp() test that hid this bug in most cases (although a lot\nless on Windows, where the files' mtimes are actually a lot more accurate\nthan on Linux).\n\nThis fixes https://github.com/gitgitgadget/git/issues/197\n\nJohannes Schindelin (2):\n  fill_stat_cache_info(): prepare for an fsmonitor fix\n  mark_fsmonitor_valid(): mark the index as changed if needed\n\n apply.c                |  2 +-\n builtin/update-index.c |  2 +-\n cache.h                |  2 +-\n diff-lib.c             |  2 +-\n entry.c                |  2 +-\n fsmonitor.h            |  5 +++--\n preload-index.c        |  2 +-\n read-cache.c           | 10 +++++-----\n 8 files changed, 14 insertions(+), 13 deletions(-)\n\n\nbase-commit: aa25c82427ae70aebf3b8f970f2afd54e9a2a8c6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-223%2Fdscho%2Ffix-racy-fsmonitor-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-223/dscho/fix-racy-fsmonitor-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/223\n-- \ngitgitgadget\n"},{"id":"376185","messageId":"4f988adc1879cdf5d772b805c4b4d5b3f1a5612b.1558700625.git.gitgitgadget@gmail.com","threadId":"51168","inReplyTo":"pull.223.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] mark_fsmonitor_valid(): mark the index as changed if needed","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-24T12:23:48Z","receivedAt":"2019-05-24T12:23:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWithout this bug fix, t7519's four \"status doesn't detect unreported\nmodifications\" test cases would fail occasionally (and, oddly enough,\n*a lot* more frequently on Windows).\n\nThe reason is that these test cases intentionally use the side effect of\n`git status` to re-write the index if any updates were detected: they\nfirst clean the worktree, run `git status` to update the index as well\nas show the output to the casual reader, then make the worktree dirty\nagain and expect no changes to reported if running with a mocked\nfsmonitor hook.\n\nThe problem with this strategy was that the index was written during\nsaid `git status` on the clean worktree for the *wrong* reason: not\nbecause the index was marked as changed (it wasn't), but because the\nrecorded mtimes were racy with the index' own mtime.\n\nAs the mtime granularity on Windows is 100 nanoseconds (see e.g.\nhttps://docs.microsoft.com/en-us/windows/desktop/SysInfo/file-times),\nthe mtimes of the files are often enough *not* racy with the index', so\nthat that `git status` call currently does not always update the index\n(including the fsmonitor extension), causing the test case to fail.\n\nThe obvious fix: if we change *any* index entry's `CE_FSMONITOR_VALID`\nflag, we should also mark the index as changed. That will cause the\nindex to be written upon `git status`, *including* an updated fsmonitor\nextension.\n\nSide note: Even though the reader might think that the t7519 issue\nshould be *much* more prevalent on Linux, given that the ext4 filesystem\n(that seems to be used by every Linux distribution) stores mtimes in\nnanosecond precision. However, ext4 uses `current_kernel_time()` (see\nhttps://unix.stackexchange.com/questions/11599#comment762968_11599; it\nis *amazingly* hard to find any proper source of information about such\next4 questions) whose accuracy seems to depend on many factors but is\nsafely worse than the 100-nanosecond granularity of NTFS (again, it is\n*horribly* hard to find anything remotely authoritative about this\nquestion). So it seems that the racy index condition that hid the bug\nfixed by this patch simply is a lot more likely on Linux than on\nWindows. But not impossible ;-)\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n diff-lib.c      | 2 +-\n fsmonitor.h     | 5 +++--\n preload-index.c | 2 +-\n read-cache.c    | 4 ++--\n 4 files changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a838c219ec..61812f48c2 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -232,7 +232,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \t\tif (!changed && !dirty_submodule) {\n \t\t\tce_mark_uptodate(ce);\n-\t\t\tmark_fsmonitor_valid(ce);\n+\t\t\tmark_fsmonitor_valid(istate, ce);\n \t\t\tif (!revs->diffopt.flags.find_copies_harder)\n \t\t\t\tcontinue;\n \t\t}\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex 8489fa3244..739318ab6d 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -49,9 +49,10 @@ void refresh_fsmonitor(struct index_state *istate);\n  * called any time the cache entry has been updated to reflect the\n  * current state of the file on disk.\n  */\n-static inline void mark_fsmonitor_valid(struct cache_entry *ce)\n+static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)\n {\n-\tif (core_fsmonitor) {\n+\tif (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {\n+\t\tistate->cache_changed = 1;\n \t\tce->ce_flags |= CE_FSMONITOR_VALID;\n \t\ttrace_printf_key(&trace_fsmonitor, \"mark_fsmonitor_clean '%s'\", ce->name);\n \t}\ndiff --git a/preload-index.c b/preload-index.c\nindex e73600ee78..ed6eaa4738 100644\n--- a/preload-index.c\n+++ b/preload-index.c\n@@ -78,7 +78,7 @@ static void *preload_thread(void *_data)\n \t\tif (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY|CE_MATCH_IGNORE_FSMONITOR))\n \t\t\tcontinue;\n \t\tce_mark_uptodate(ce);\n-\t\tmark_fsmonitor_valid(ce);\n+\t\tmark_fsmonitor_valid(index, ce);\n \t} while (--nr > 0);\n \tif (p->progress) {\n \t\tstruct progress_data *pd = p->progress;\ndiff --git a/read-cache.c b/read-cache.c\nindex d3b43ae3ba..32816763bd 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -204,7 +204,7 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st\n \n \tif (S_ISREG(st->st_mode)) {\n \t\tce_mark_uptodate(ce);\n-\t\tmark_fsmonitor_valid(ce);\n+\t\tmark_fsmonitor_valid(istate, ce);\n \t}\n }\n \n@@ -1432,7 +1432,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \t\t\t */\n \t\t\tif (!S_ISGITLINK(ce->ce_mode)) {\n \t\t\t\tce_mark_uptodate(ce);\n-\t\t\t\tmark_fsmonitor_valid(ce);\n+\t\t\t\tmark_fsmonitor_valid(istate, ce);\n \t\t\t}\n \t\t\treturn ce;\n \t\t}\n-- \ngitgitgadget\n"},{"id":"376186","messageId":"232eb7a33bbb4863a1ae69000186f7338cdcaa63.1558700625.git.gitgitgadget@gmail.com","threadId":"51168","inReplyTo":"pull.223.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] fill_stat_cache_info(): prepare for an fsmonitor fix","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-24T12:23:47Z","receivedAt":"2019-05-24T12:23:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWe will need to pass down the `struct index_state` to\n`mark_fsmonitor_valid()` for an upcoming bug fix, and this here function\ncalls that there function, so we need to extend the signature of\n`fill_stat_cache_info()` first.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n apply.c                | 2 +-\n builtin/update-index.c | 2 +-\n cache.h                | 2 +-\n entry.c                | 2 +-\n read-cache.c           | 6 +++---\n 5 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex f15afa9f6a..4992eca416 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4310,7 +4310,7 @@ static int add_index_file(struct apply_state *state,\n \t\t\t\t\t\t     \"created file '%s'\"),\n \t\t\t\t\t\t   path);\n \t\t\t}\n-\t\t\tfill_stat_cache_info(ce, &st);\n+\t\t\tfill_stat_cache_info(state->repo->index, ce, &st);\n \t\t}\n \t\tif (write_object_file(buf, size, blob_type, &ce->oid) < 0) {\n \t\t\tdiscard_cache_entry(ce);\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 27db0928bf..3f8cc6ccb4 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -280,7 +280,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len\n \tmemcpy(ce->name, path, len);\n \tce->ce_flags = create_ce_flags(0);\n \tce->ce_namelen = len;\n-\tfill_stat_cache_info(ce, st);\n+\tfill_stat_cache_info(&the_index, ce, st);\n \tce->ce_mode = ce_mode_from_stat(old, st->st_mode);\n \n \tif (index_path(&the_index, &ce->oid, path, st,\ndiff --git a/cache.h b/cache.h\nindex b4bb2e2c11..bf20337ef4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -826,7 +826,7 @@ int match_stat_data(const struct stat_data *sd, struct stat *st);\n int match_stat_data_racy(const struct index_state *istate,\n \t\t\t const struct stat_data *sd, struct stat *st);\n \n-void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n+void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st);\n \n #define REFRESH_REALLY\t\t0x0001\t/* ignore_valid */\n #define REFRESH_UNMERGED\t0x0002\t/* allow unmerged */\ndiff --git a/entry.c b/entry.c\nindex 0e4f2f2910..53380bb614 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -373,7 +373,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\tif (lstat(ce->name, &st) < 0)\n \t\t\t\treturn error_errno(\"unable to stat just-written file %s\",\n \t\t\t\t\t\t   ce->name);\n-\t\tfill_stat_cache_info(ce, &st);\n+\t\tfill_stat_cache_info(state->istate, ce, &st);\n \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\tmark_fsmonitor_invalid(state->istate, ce);\n \t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\ndiff --git a/read-cache.c b/read-cache.c\nindex 22e7b9944e..d3b43ae3ba 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -195,7 +195,7 @@ int match_stat_data(const struct stat_data *sd, struct stat *st)\n  * cache, ie the parts that aren't tracked by GIT, and only used\n  * to validate the cache.\n  */\n-void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n+void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st)\n {\n \tfill_stat_data(&ce->ce_stat_data, st);\n \n@@ -728,7 +728,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,\n \tmemcpy(ce->name, path, namelen);\n \tce->ce_namelen = namelen;\n \tif (!intent_only)\n-\t\tfill_stat_cache_info(ce, st);\n+\t\tfill_stat_cache_info(istate, ce, st);\n \telse\n \t\tce->ce_flags |= CE_INTENT_TO_ADD;\n \n@@ -1447,7 +1447,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,\n \tupdated = make_empty_cache_entry(istate, ce_namelen(ce));\n \tcopy_cache_entry(updated, ce);\n \tmemcpy(updated->name, ce->name, ce->ce_namelen + 1);\n-\tfill_stat_cache_info(updated, &st);\n+\tfill_stat_cache_info(istate, updated, &st);\n \t/*\n \t * If ignore_valid is not set, we should leave CE_VALID bit\n \t * alone.  Otherwise, paths marked with --no-assume-unchanged\n-- \ngitgitgadget\n\n"}]}