Use `flush_fs_events` more after performing synchronous fs mutations

Antonio Scandurra created 4 years ago

I am not sure I have caught all the examples of this, but in general
I think we always want to perform a `flush_fs_events` as opposed to
`next_scan_complete` when doing synchronous I/O. Indeed, the file
system may inform us about the events caused by the just-performed
I/O over multiple batches, and `next_scan_complete` may return
before seeing all of them.

Note that this also removes a few assertions which were ensuring
that, on start, a worktree's file handle wouldn't know its deleted
status, even if the file didn't exist for sure on disk. However,
now that `file` is an async API, it's possible that by the time the
`FileHandle` is resolved, `Worktree` has already completed scanning.
We test a similar behavior further along in the test where those
assertions were removed, so it felt okay to proceed without them.

Change summary

zed/src/editor/buffer/mod.rs | 11 ++++-------
zed/src/worktree.rs          | 35 +++++++++--------------------------
2 files changed, 13 insertions(+), 33 deletions(-)

Detailed changes

zed/src/editor/buffer/mod.rs

@@ -3148,8 +3148,7 @@ mod tests {
             });
 
             fs::remove_file(dir.path().join("file2")).unwrap();
-            tree.update(&mut app, |tree, ctx| tree.next_scan_complete(ctx))
-                .await;
+            tree.flush_fs_events(&app).await;
             assert_eq!(
                 *events.borrow(),
                 &[Event::Dirtied, Event::FileHandleChanged]
@@ -3174,8 +3173,7 @@ mod tests {
             });
             events.borrow_mut().clear();
             fs::remove_file(dir.path().join("file3")).unwrap();
-            tree.update(&mut app, |tree, ctx| tree.next_scan_complete(ctx))
-                .await;
+            tree.flush_fs_events(&app).await;
             assert_eq!(*events.borrow(), &[Event::FileHandleChanged]);
             app.read(|ctx| assert!(buffer3.read(ctx).is_dirty()));
         });
@@ -3218,14 +3216,13 @@ mod tests {
 
         // Change the file on disk, adding two new lines of text, and removing
         // one line.
-        buffer.update(&mut app, |buffer, _| {
+        buffer.read_with(&app, |buffer, _| {
             assert!(!buffer.is_dirty());
             assert!(!buffer.has_conflict());
         });
-        tree.flush_fs_events(&app).await;
         let new_contents = "AAAA\naaa\nBB\nbbbbb\n";
-
         fs::write(&abs_path, new_contents).unwrap();
+        tree.flush_fs_events(&app).await;
 
         // Because the buffer was not modified, it is reloaded from disk. Its
         // contents are edited according to the diff between the old and new

zed/src/worktree.rs

@@ -229,23 +229,16 @@ impl Worktree {
                 writer.write(chunk.as_bytes())?;
             }
             writer.flush()?;
-            Self::update_file_handle(&file, &path, &handles)?;
+
+            if let Some(handle) = handles.lock().get(&*path).and_then(Weak::upgrade) {
+                let mut handle = handle.lock();
+                handle.mtime = file.metadata()?.modified()?;
+                handle.is_deleted = false;
+            }
+
             Ok(())
         })
     }
-
-    fn update_file_handle(
-        file: &fs::File,
-        path: &Path,
-        handles: &Mutex<HashMap<Arc<Path>, Weak<Mutex<FileHandleState>>>>,
-    ) -> Result<()> {
-        if let Some(handle) = handles.lock().get(path).and_then(Weak::upgrade) {
-            let mut handle = handle.lock();
-            handle.mtime = file.metadata()?.modified()?;
-            handle.is_deleted = false;
-        }
-        Ok(())
-    }
 }
 
 impl Entity for Worktree {
@@ -1571,14 +1564,6 @@ mod tests {
         let file5 = app.update(|ctx| tree.file("b/c/file5", ctx)).await;
         let non_existent_file = app.update(|ctx| tree.file("a/file_x", ctx)).await;
 
-        // The worktree hasn't scanned the directories containing these paths,
-        // so it can't determine that the paths are deleted.
-        assert!(!file2.is_deleted());
-        assert!(!file3.is_deleted());
-        assert!(!file4.is_deleted());
-        assert!(!file5.is_deleted());
-        assert!(!non_existent_file.is_deleted());
-
         // After scanning, the worktree knows which files exist and which don't.
         app.read(|ctx| tree.read(ctx).scan_complete()).await;
         assert!(!file2.is_deleted());
@@ -1592,8 +1577,7 @@ mod tests {
         std::fs::remove_file(dir.path().join("b/c/file5")).unwrap();
         std::fs::rename(dir.path().join("b/c"), dir.path().join("d")).unwrap();
         std::fs::rename(dir.path().join("a/file2"), dir.path().join("a/file2.new")).unwrap();
-        tree.update(&mut app, |tree, ctx| tree.next_scan_complete(ctx))
-            .await;
+        tree.flush_fs_events(&app).await;
 
         app.read(|ctx| {
             assert_eq!(
@@ -1653,8 +1637,7 @@ mod tests {
 
         fs::write(dir.path().join("tracked-dir/tracked-file2"), "").unwrap();
         fs::write(dir.path().join("ignored-dir/ignored-file2"), "").unwrap();
-        tree.update(&mut app, |tree, ctx| tree.next_scan_complete(ctx))
-            .await;
+        tree.flush_fs_events(&app).await;
         app.read(|ctx| {
             let tree = tree.read(ctx);
             let dot_git = tree.entry_for_path(".git").unwrap();