Merge #805

805: don't distinguish Create and Write events in VFS r=pnkfelix a=matklad

r? @pnkfelix

Co-authored-by: Aleksey Kladov <[email protected]>

2 files changed, 42 insertions, 38 deletions

diff --git a/crates/ra_vfs/src/io.rs b/crates/ra_vfs/src/io.rs
index ff5ae3a19..dc0b84d5a 100644
--- a/crates/ra_vfs/src/io.rs
+++ b/crates/ra_vfs/src/io.rs
@@ -17,14 +17,28 @@ pub(crate) enum Task {
     AddRoot { root: VfsRoot, config: Arc<RootConfig> },
 }
 
+/// `TaskResult` transfers files read on the IO thread to the VFS on the main
+/// thread.
 #[derive(Debug)]
 pub enum TaskResult {
+    /// Emitted when we've recursively scanned a source root during the initial
+    /// load.
     BulkLoadRoot { root: VfsRoot, files: Vec<(RelativePathBuf, String)> },
-    AddSingleFile { root: VfsRoot, path: RelativePathBuf, text: String },
-    ChangeSingleFile { root: VfsRoot, path: RelativePathBuf, text: String },
-    RemoveSingleFile { root: VfsRoot, path: RelativePathBuf },
+    /// Emitted when we've noticed that a single file has changed.
+    ///
+    /// Note that this by design does not distinguish between
+    /// create/delete/write events, and instead specifies the *current* state of
+    /// the file. The idea is to guarantee that in the quiescent state the sum
+    /// of all results equals to the current state of the file system, while
+    /// allowing to skip intermediate events in non-quiescent states.
+    SingleFile { root: VfsRoot, path: RelativePathBuf, text: Option<String> },
 }
 
+/// The kind of raw notification we've received from the notify library.
+///
+/// Note that these are not necessary 100% precise (for example we might receive
+/// `Create` instead of `Write`, see #734), but we try do distinguish `Create`s
+/// to implement recursive watching of directories.
 #[derive(Debug)]
 enum ChangeKind {
     Create,
@@ -45,7 +59,7 @@ impl Worker {
         // explained by the following concerns:
         //    * we need to burn a thread translating from notify's mpsc to
         //      crossbeam_channel.
-        //    * we want to read all files from a single thread, to gurantee that
+        //    * we want to read all files from a single thread, to guarantee that
         //      we always get fresher versions and never go back in time.
         //    * we want to tear down everything neatly during shutdown.
         let (worker, worker_handle) = thread_worker::spawn(
@@ -63,7 +77,7 @@ impl Worker {
                 let mut watcher = notify::watcher(notify_sender, WATCHER_DELAY)
                     .map_err(|e| log::error!("failed to spawn notify {}", e))
                     .ok();
-                // Start a silly thread to tranform between two channels
+                // Start a silly thread to transform between two channels
                 let thread = thread::spawn(move || {
                     notify_receiver
                         .into_iter()
@@ -98,7 +112,7 @@ impl Worker {
                 }
                 // Stopped the watcher
                 drop(watcher.take());
-                // Drain pending events: we are not inrerested in them anyways!
+                // Drain pending events: we are not interested in them anyways!
                 watcher_receiver.into_iter().for_each(|_| ());
 
                 let res = thread.join();
@@ -199,23 +213,16 @@ fn handle_change(
             }
             paths
                 .into_iter()
-                .filter_map(|rel_path| {
+                .try_for_each(|rel_path| {
                     let abs_path = rel_path.to_path(&config.root);
-                    let text = read_to_string(&abs_path)?;
-                    Some((rel_path, text))
-                })
-                .try_for_each(|(path, text)| {
-                    sender.send(TaskResult::AddSingleFile { root, path, text })
+                    let text = read_to_string(&abs_path);
+                    sender.send(TaskResult::SingleFile { root, path: rel_path, text })
                 })
                 .unwrap()
         }
-        ChangeKind::Write => {
-            if let Some(text) = read_to_string(&path) {
-                sender.send(TaskResult::ChangeSingleFile { root, path: rel_path, text }).unwrap();
-            }
-        }
-        ChangeKind::Remove => {
-            sender.send(TaskResult::RemoveSingleFile { root, path: rel_path }).unwrap()
+        ChangeKind::Write | ChangeKind::Remove => {
+            let text = read_to_string(&path);
+            sender.send(TaskResult::SingleFile { root, path: rel_path, text }).unwrap();
         }
     }
 }
diff --git a/crates/ra_vfs/src/lib.rs b/crates/ra_vfs/src/lib.rs
index 3805be570..5d98d905c 100644
--- a/crates/ra_vfs/src/lib.rs
+++ b/crates/ra_vfs/src/lib.rs
@@ -37,8 +37,8 @@ impl_arena_id!(VfsRoot);
 
 /// Describes the contents of a single source root.
 ///
-/// `RootConfig` can be thought of as a glob pattern like `src/**.rs` whihc
-/// specifes the source root or as a function whihc takes a `PathBuf` and
+/// `RootConfig` can be thought of as a glob pattern like `src/**.rs` which
+/// specifies the source root or as a function which takes a `PathBuf` and
 /// returns `true` iff path belongs to the source root
 pub(crate) struct RootConfig {
     root: PathBuf,
@@ -60,7 +60,7 @@ impl RootConfig {
     fn new(root: PathBuf, excluded_dirs: Vec<PathBuf>) -> RootConfig {
         RootConfig { root, excluded_dirs }
     }
-    /// Cheks if root contains a path and returns a root-relative path.
+    /// Checks if root contains a path and returns a root-relative path.
     pub(crate) fn contains(&self, path: &Path) -> Option<RelativePathBuf> {
         // First, check excluded dirs
         if self.excluded_dirs.iter().any(|it| path.starts_with(it)) {
@@ -210,7 +210,7 @@ impl Vfs {
         match task {
             TaskResult::BulkLoadRoot { root, files } => {
                 let mut cur_files = Vec::new();
-                // While we were scanning the root in the backgound, a file might have
+                // While we were scanning the root in the background, a file might have
                 // been open in the editor, so we need to account for that.
                 let exising = self.root2files[root]
                     .iter()
@@ -230,21 +230,18 @@ impl Vfs {
                 let change = VfsChange::AddRoot { root, files: cur_files };
                 self.pending_changes.push(change);
             }
-            TaskResult::AddSingleFile { root, path, text } => {
-                if self.find_file(root, &path).is_none() {
-                    self.do_add_file(root, path, text, false);
-                }
-            }
-            TaskResult::ChangeSingleFile { root, path, text } => {
-                if let Some(file) = self.find_file(root, &path) {
-                    self.do_change_file(file, text, false);
-                } else {
-                    self.do_add_file(root, path, text, false);
-                }
-            }
-            TaskResult::RemoveSingleFile { root, path } => {
-                if let Some(file) = self.find_file(root, &path) {
-                    self.do_remove_file(root, path, file, false);
+            TaskResult::SingleFile { root, path, text } => {
+                match (self.find_file(root, &path), text) {
+                    (Some(file), None) => {
+                        self.do_remove_file(root, path, file, false);
+                    }
+                    (None, Some(text)) => {
+                        self.do_add_file(root, path, text, false);
+                    }
+                    (Some(file), Some(text)) => {
+                        self.do_change_file(file, text, false);
+                    }
+                    (None, None) => (),
                 }
             }
         }