From 8c1868b980bc7ac2a720f67dd9c682058d18c64f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 23 May 2019 19:43:56 +0200 Subject: [PATCH 1/2] fill_stat_cache_info(): prepare for an fsmonitor fix We will need to pass down the `struct index_state` to `mark_fsmonitor_valid()` for an upcoming bug fix, and this here function calls that there function, so we need to extend the signature of `fill_stat_cache_info()` first. Signed-off-by: Johannes Schindelin --- apply.c | 2 +- builtin/update-index.c | 2 +- cache.h | 2 +- entry.c | 2 +- read-cache.c | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/apply.c b/apply.c index 892ede5a318f75..ec585e466b53cd 100644 --- a/apply.c +++ b/apply.c @@ -4306,7 +4306,7 @@ static int add_index_file(struct apply_state *state, "created file '%s'"), path); } - fill_stat_cache_info(ce, &st); + fill_stat_cache_info(state->repo->index, ce, &st); } if (write_object_file(buf, size, blob_type, &ce->oid) < 0) { discard_cache_entry(ce); diff --git a/builtin/update-index.c b/builtin/update-index.c index 02ace602b9ae23..a9df27da042e7b 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -280,7 +280,7 @@ static int add_one_path(const struct cache_entry *old, const char *path, int len memcpy(ce->name, path, len); ce->ce_flags = create_ce_flags(0); ce->ce_namelen = len; - fill_stat_cache_info(ce, st); + fill_stat_cache_info(&the_index, ce, st); ce->ce_mode = ce_mode_from_stat(old, st->st_mode); if (index_path(&the_index, &ce->oid, path, st, diff --git a/cache.h b/cache.h index 27fe635f622c49..58727b92ac7aa0 100644 --- a/cache.h +++ b/cache.h @@ -822,7 +822,7 @@ extern int match_stat_data(const struct stat_data *sd, struct stat *st); extern int match_stat_data_racy(const struct index_state *istate, const struct stat_data *sd, struct stat *st); -extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st); +extern void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st); #define REFRESH_REALLY 0x0001 /* ignore_valid */ #define REFRESH_UNMERGED 0x0002 /* allow unmerged */ diff --git a/entry.c b/entry.c index 6fd72b30c87686..6b6d7a4aa3faad 100644 --- a/entry.c +++ b/entry.c @@ -373,7 +373,7 @@ static int write_entry(struct cache_entry *ce, if (lstat(ce->name, &st) < 0) return error_errno("unable to stat just-written file %s", ce->name); - fill_stat_cache_info(ce, &st); + fill_stat_cache_info(state->istate, ce, &st); ce->ce_flags |= CE_UPDATE_IN_BASE; mark_fsmonitor_invalid(state->istate, ce); state->istate->cache_changed |= CE_ENTRY_CHANGED; diff --git a/read-cache.c b/read-cache.c index 0e0c93edc9be5a..eb8fdec8a67358 100644 --- a/read-cache.c +++ b/read-cache.c @@ -194,7 +194,7 @@ int match_stat_data(const struct stat_data *sd, struct stat *st) * cache, ie the parts that aren't tracked by GIT, and only used * to validate the cache. */ -void fill_stat_cache_info(struct cache_entry *ce, struct stat *st) +void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st) { fill_stat_data(&ce->ce_stat_data, st); @@ -718,7 +718,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, memcpy(ce->name, path, namelen); ce->ce_namelen = namelen; if (!intent_only) - fill_stat_cache_info(ce, st); + fill_stat_cache_info(istate, ce, st); else ce->ce_flags |= CE_INTENT_TO_ADD; @@ -1437,7 +1437,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate, updated = make_empty_cache_entry(istate, ce_namelen(ce)); copy_cache_entry(updated, ce); memcpy(updated->name, ce->name, ce->ce_namelen + 1); - fill_stat_cache_info(updated, &st); + fill_stat_cache_info(istate, updated, &st); /* * If ignore_valid is not set, we should leave CE_VALID bit * alone. Otherwise, paths marked with --no-assume-unchanged From 11a2e57b608fed9abf725c45fd422e1ad5023cac Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 23 May 2019 19:47:00 +0200 Subject: [PATCH 2/2] mark_fsmonitor_valid(): mark the index as changed if needed Without this bug fix, t7519's four "status doesn't detect unreported modifications" test cases would fail occasionally (and, oddly enough, *a lot* more frequently on Windows). The reason is that these test cases intentionally use the side effect of `git status` to re-write the index if any updates were detected: they first clean the worktree, run `git status` to update the index as well as show the output to the casual reader, then make the worktree dirty again and expect no changes to reported if running with a mocked fsmonitor hook. The problem with this strategy was that the index was written during said `git status` on the clean worktree for the *wrong* reason: not because the index was marked as changed (it wasn't), but because the recorded mtimes were racy with the index' own mtime. As the mtime granularity on Windows is 100 nanoseconds (see e.g. https://docs.microsoft.com/en-us/windows/desktop/SysInfo/file-times), the mtimes of the files are often enough *not* racy with the index', so that that `git status` call currently does not always update the index (including the fsmonitor extension), causing the test case to fail. The obvious fix: if we change *any* index entry's `CE_FSMONITOR_VALID` flag, we should also mark the index as changed. That will cause the index to be written upon `git status`, *including* an updated fsmonitor extension. Side note: Even though the reader might think that the t7519 issue should be *much* more prevalent on Linux, given that the ext4 filesystem (that seems to be used by every Linux distribution) stores mtimes in nanosecond precision. However, ext4 uses `current_kernel_time()` (see https://unix.stackexchange.com/questions/11599#comment762968_11599; it is *amazingly* hard to find any proper source of information about such ext4 questions) whose accuracy seems to depend on many factors but is safely worse than the 100-nanosecond granularity of NTFS (again, it is *horribly* hard to find anything remotely authoritative about this question). So it seems that the racy index condition that hid the bug fixed by this patch simply is a lot more likely on Linux than on Windows. But not impossible ;-) Signed-off-by: Johannes Schindelin --- diff-lib.c | 2 +- fsmonitor.h | 5 +++-- preload-index.c | 2 +- read-cache.c | 4 ++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/diff-lib.c b/diff-lib.c index 23c8d351b3aa59..8bd77fafcfd7f7 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -232,7 +232,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option) if (!changed && !dirty_submodule) { ce_mark_uptodate(ce); - mark_fsmonitor_valid(ce); + mark_fsmonitor_valid(istate, ce); if (!revs->diffopt.flags.find_copies_harder) continue; } diff --git a/fsmonitor.h b/fsmonitor.h index 01017c43aa68be..466bb7b92682f8 100644 --- a/fsmonitor.h +++ b/fsmonitor.h @@ -49,9 +49,10 @@ extern void refresh_fsmonitor(struct index_state *istate); * called any time the cache entry has been updated to reflect the * current state of the file on disk. */ -static inline void mark_fsmonitor_valid(struct cache_entry *ce) +static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce) { - if (core_fsmonitor) { + if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) { + istate->cache_changed = 1; ce->ce_flags |= CE_FSMONITOR_VALID; trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name); } diff --git a/preload-index.c b/preload-index.c index e73600ee7841a2..ed6eaa47388af8 100644 --- a/preload-index.c +++ b/preload-index.c @@ -78,7 +78,7 @@ static void *preload_thread(void *_data) if (ie_match_stat(index, ce, &st, CE_MATCH_RACY_IS_DIRTY|CE_MATCH_IGNORE_FSMONITOR)) continue; ce_mark_uptodate(ce); - mark_fsmonitor_valid(ce); + mark_fsmonitor_valid(index, ce); } while (--nr > 0); if (p->progress) { struct progress_data *pd = p->progress; diff --git a/read-cache.c b/read-cache.c index eb8fdec8a67358..842857792f9c91 100644 --- a/read-cache.c +++ b/read-cache.c @@ -203,7 +203,7 @@ void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, st if (S_ISREG(st->st_mode)) { ce_mark_uptodate(ce); - mark_fsmonitor_valid(ce); + mark_fsmonitor_valid(istate, ce); } } @@ -1422,7 +1422,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate, */ if (!S_ISGITLINK(ce->ce_mode)) { ce_mark_uptodate(ce); - mark_fsmonitor_valid(ce); + mark_fsmonitor_valid(istate, ce); } return ce; }