Skip to content

Do query the fsmonitor again after the index has been discarded #2127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
May 9, 2019
Merged

Do query the fsmonitor again after the index has been discarded #2127

merged 2 commits into from
May 9, 2019

Conversation

dscho
Copy link
Member

@dscho dscho commented Mar 14, 2019

@dscho dscho requested a review from benpeart March 15, 2019 12:18
benpeart
benpeart approved these changes Mar 15, 2019
View reviewed changes
Copy link

@benpeart benpeart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for debugging and fixing this. The new regression test and bug fix both look good.

benpeart
benpeart previously requested changes Mar 15, 2019
View reviewed changes
Copy link

@benpeart benpeart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't debugged into it but the fact that the fsmonitor test failed in the test run indicated something isn't right.

@dscho
Copy link
Member Author

dscho commented Mar 15, 2019

Indeed, it is not even a flaky test, I re-ran it twice, and it still produced the same issue. Locally, no problem though!

So I guess I will (ab-)use this PR to bisect.

@dscho
Copy link
Member Author

dscho commented Mar 16, 2019

I haven't debugged into it but the fact that the fsmonitor test failed in the test run indicated something isn't right.

So, I re-ran it with --stress --stress-limit=50 locally, after downloading the artifacts (and editing the bin-wrappers/*), and it did not reproduce.

Then I finally broke down and triggered yet another re-run, and it "fixed" it.

So it is a flaky test, after all, and seems to be pretty hard to trigger. Which makes it likely to be some racy timestamp issue: since the marker file is written, the fsmonitor hook is called. It's just that the information from the fsmonitor apparently does not prevent the full refresh, which means that the timestamps probably match.

However, the fact that the fsmonitor feature did not yield a false positive here makes me less worried. We still should have a closer look at this at some stage, but it is not like the fsmonitor incorrectly prevented a correct status update. That would be more critical.

@dscho dscho mentioned this pull request Mar 16, 2019
Closed
@dscho dscho requested a review from benpeart March 17, 2019 19:10
@dscho dscho mentioned this pull request Mar 17, 2019
Closed
@dscho dscho added this to the v2.21.0(2) milestone May 8, 2019
@dscho dscho dismissed benpeart’s stale review May 9, 2019 18:58

We took this to the Git mailing list and decided that there are no more concerns.

dscho added 2 commits May 9, 2019 21:04
@dscho
fsmonitor: demonstrate that it is not refreshed after discard_index()
328b4a2 
This one is tricky.

When `core.fsmonitor` is set, a `refresh_index()` will not perform a
full scan of files that might be modified, but will query the fsmonitor
and refresh only the ones that have been actually touched.

Due to implementation details, the fsmonitor is queried in
`refresh_cache_ent()`, but of course it only has to be queried once, so
we set a flag when we did that. But when the index was discarded, we did
not re-set that flag.

So far, this is only covered by our test suite when running with
GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all, and only due to the way the
built-in stash interacts with the recursive merge machinery.

Let's introduce a straight-forward regression test for this.

We simply extend the "read & discard index" loop in `test-tool
read-cache` to optionally refresh the index, report on a given file's
status, and then modify that file. Due to the bug described above, only
the first refresh will actually query the fsmonitor; subsequent loop
iterations will not.

This problem was reported by Ævar Arnfjörð Bjarmason.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
fsmonitor: force a refresh after the index was discarded
912cfd3 
With this change, the `index_state` struct becomes the new home for the
flag that says whether the fsmonitor hook has been run, i.e. it is now
per-index.

It also gets re-set when the index is discarded, fixing the bug
demonstrated by the "test_expect_failure" test added in the preceding
commit. In that case fsmonitor-enabled Git would miss updates under
certain circumstances, see that preceding commit for details.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the fix-fsmonitor branch from a64452a to 912cfd3 Compare May 9, 2019 19:09
@dscho
Copy link
Member Author

dscho commented May 9, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dscho dscho merged commit 81a060a into git-for-windows:master May 9, 2019
@dscho dscho deleted the fix-fsmonitor branch May 9, 2019 21:04
git-for-windows-ci pushed a commit that referenced this pull request May 9, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
daa8bbe 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 10, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
162f1fc 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 10, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
dbac74e 
Do query the fsmonitor again after the index has been discarded
git-for-windows-ci pushed a commit that referenced this pull request May 10, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
54eee7b 
Do query the fsmonitor again after the index has been discarded
git-for-windows-ci pushed a commit that referenced this pull request May 10, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
b3945c2 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 13, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
99f4b07 
Do query the fsmonitor again after the index has been discarded
dscho added a commit to dscho/git that referenced this pull request May 13, 2019
@dscho
Merge pull request git-for-windows#2127 from dscho/fix-fsmonitor
5cdba46 
Do query the fsmonitor again after the index has been discarded
dscho added a commit to dscho/git that referenced this pull request May 13, 2019
@dscho
Merge pull request git-for-windows#2127 from dscho/fix-fsmonitor
5df6868 
Do query the fsmonitor again after the index has been discarded
git-for-windows-ci pushed a commit that referenced this pull request May 14, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
5697469 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 14, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
ba8818a 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 14, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
c91483e 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 21, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
a541156 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 22, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
a746510 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request May 25, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
53d90c8 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request Jun 4, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
9531fc3 
Do query the fsmonitor again after the index has been discarded
dscho added a commit that referenced this pull request Jun 8, 2019
@dscho
Merge pull request #2127 from dscho/fix-fsmonitor
76576b2 
Do query the fsmonitor again after the index has been discarded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpeart benpeart Awaiting requested review from benpeart

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.22.0
Development

Successfully merging this pull request may close these issues.
2 participants
@dscho @benpeart