Redesign Worktree save API and make test_rescan_simple pass

This commit does too much. The first goal was to change our approach to saving new buffers so that we don't need to construct a File for an entry that doesn't exist. Rather than doing that, we call `Worktree::save_buffer_as` with the buffer handle, the path, and the contents. This then saves the buffer and returns a handle to a `File` that references an entry that actually exists. I needed to do this so that we can store an entry id on `File`.

In the process, I noticed intermittent test failures on `test_rescan_simple`, so I made some changes required to fix those related to our reuse of existing ids. Our previous approach of removing a path when inserting a new entry was broken, because of the recursive nature of `remove_path`. Instead, I simply recycle the id of an existing worktree entry with the same path if one is present, then allow it to be replaced.

zed/src/editor.rs

@@ -7,7 +7,7 @@ use crate::{
     settings::{Settings, StyleId},
     util::{post_inc, Bias},
     workspace,
-    worktree::File,
+    worktree::{File, Worktree},
 };
 use anyhow::Result;
 pub use buffer::*;
@@ -2506,8 +2506,18 @@ impl workspace::ItemView for Editor {
         Some(clone)
     }
 
-    fn save(&mut self, new_file: Option<File>, cx: &mut ViewContext<Self>) -> Task<Result<()>> {
+    fn save(&mut self, cx: &mut ViewContext<Self>) -> Result<Task<Result<()>>> {
-        self.buffer.update(cx, |b, cx| b.save(new_file, cx))
+        self.buffer.update(cx, |b, cx| b.save(cx))
+    }
+
+    fn save_as(
+        &mut self,
+        worktree: &ModelHandle<Worktree>,
+        path: &Path,
+        cx: &mut ViewContext<Self>,
+    ) -> Task<Result<()>> {
+        self.buffer
+            .update(cx, |b, cx| b.save_as(worktree, path, cx))
     }
 
     fn is_dirty(&self, cx: &AppContext) -> bool {

zed/src/editor/buffer.rs

@@ -20,10 +20,10 @@ use crate::{
     sum_tree::{self, FilterCursor, SumTree},
     time::{self, ReplicaId},
     util::Bias,
-    worktree::File,
+    worktree::{File, Worktree},
 };
 use anyhow::{anyhow, Result};
-use gpui::{AppContext, Entity, ModelContext, Task};
+use gpui::{AppContext, Entity, ModelContext, ModelHandle, Task};
 use lazy_static::lazy_static;
 use std::{
     cell::RefCell,
@@ -561,44 +561,60 @@ impl Buffer {
         self.file.as_ref()
     }
 
-    pub fn save(
+    pub fn save(&mut self, cx: &mut ModelContext<Self>) -> Result<Task<Result<()>>> {
-        &mut self,
+        let file = self
-        new_file: Option<File>,
+            .file
-        cx: &mut ModelContext<Self>,
+            .as_ref()
-    ) -> Task<Result<()>> {
+            .ok_or_else(|| anyhow!("buffer has no file"))?;
+
         let text = self.visible_text.clone();
         let version = self.version.clone();
-        let file = self.file.clone();
+        let save = file.save(text, cx.as_mut());
 
-        cx.spawn(|handle, mut cx| async move {
+        Ok(cx.spawn(|this, mut cx| async move {
-            if let Some(file) = new_file.as_ref().or(file.as_ref()) {
+            save.await?;
-                let result = cx.update(|cx| file.save(text, cx)).await;
+            this.update(&mut cx, |this, cx| {
-                if result.is_ok() {
+                this.did_save(version, cx).unwrap();
-                    handle.update(&mut cx, |me, cx| me.did_save(version, new_file, cx));
+            });
-                }
+            Ok(())
-                result
+        }))
-            } else {
+    }
-                Ok(())
+
-            }
+    pub fn save_as(
+        &mut self,
+        worktree: &ModelHandle<Worktree>,
+        path: impl Into<Arc<Path>>,
+        cx: &mut ModelContext<Self>,
+    ) -> Task<Result<()>> {
+        let handle = cx.handle();
+        let text = self.visible_text.clone();
+        let version = self.version.clone();
+        let save_as = worktree.update(cx, |worktree, cx| {
+            worktree
+                .as_local_mut()
+                .unwrap()
+                .save_buffer_as(handle, path, text, cx)
+        });
+
+        cx.spawn(|this, mut cx| async move {
+            save_as.await.map(|new_file| {
+                this.update(&mut cx, |this, cx| {
+                    this.file = Some(new_file);
+                    this.did_save(version, cx).unwrap();
+                });
+            })
         })
     }
 
-    fn did_save(
+    fn did_save(&mut self, version: time::Global, cx: &mut ModelContext<Self>) -> Result<()> {
-        &mut self,
+        if let Some(file) = self.file.as_ref() {
-        version: time::Global,
-        new_file: Option<File>,
-        cx: &mut ModelContext<Self>,
-    ) {
-        if let Some(new_file) = new_file {
-            let buffer = cx.handle();
-            new_file.buffer_added(buffer, cx.as_mut());
-            self.file = Some(new_file);
-        }
-        if let Some(file) = &self.file {
             self.saved_mtime = file.mtime(cx.as_ref());
+            self.saved_version = version;
+            cx.emit(Event::Saved);
+            Ok(())
+        } else {
+            Err(anyhow!("buffer has no file"))
         }
-        self.saved_version = version;
-        cx.emit(Event::Saved);
     }
 
     pub fn file_was_moved(&mut self, new_path: Arc<Path>, cx: &mut ModelContext<Self>) {
@@ -3051,8 +3067,7 @@ mod tests {
             assert!(buffer.is_dirty(cx.as_ref()));
             assert_eq!(*events.borrow(), &[Event::Edited, Event::Dirtied]);
             events.borrow_mut().clear();
-
+            buffer.did_save(buffer.version(), cx).unwrap();
-            buffer.did_save(buffer.version(), None, cx);
         });
 
         // after saving, the buffer is not dirty, and emits a saved event.

zed/src/workspace.rs

@@ -127,7 +127,13 @@ pub trait ItemView: View {
     fn has_conflict(&self, _: &AppContext) -> bool {
         false
     }
-    fn save(&mut self, _: Option<File>, _: &mut ViewContext<Self>) -> Task<anyhow::Result<()>>;
+    fn save(&mut self, cx: &mut ViewContext<Self>) -> Result<Task<Result<()>>>;
+    fn save_as(
+        &mut self,
+        worktree: &ModelHandle<Worktree>,
+        path: &Path,
+        cx: &mut ViewContext<Self>,
+    ) -> Task<anyhow::Result<()>>;
     fn should_activate_item_on_event(_: &Self::Event) -> bool {
         false
     }
@@ -162,7 +168,13 @@ pub trait ItemViewHandle: Send + Sync {
     fn to_any(&self) -> AnyViewHandle;
     fn is_dirty(&self, cx: &AppContext) -> bool;
     fn has_conflict(&self, cx: &AppContext) -> bool;
-    fn save(&self, file: Option<File>, cx: &mut MutableAppContext) -> Task<anyhow::Result<()>>;
+    fn save(&self, cx: &mut MutableAppContext) -> Result<Task<Result<()>>>;
+    fn save_as(
+        &self,
+        worktree: &ModelHandle<Worktree>,
+        path: &Path,
+        cx: &mut MutableAppContext,
+    ) -> Task<anyhow::Result<()>>;
 }
 
 impl<T: Item> ItemHandle for ModelHandle<T> {
@@ -236,8 +248,17 @@ impl<T: ItemView> ItemViewHandle for ViewHandle<T> {
         })
     }
 
-    fn save(&self, file: Option<File>, cx: &mut MutableAppContext) -> Task<anyhow::Result<()>> {
+    fn save(&self, cx: &mut MutableAppContext) -> Result<Task<Result<()>>> {
-        self.update(cx, |item, cx| item.save(file, cx))
+        self.update(cx, |item, cx| item.save(cx))
+    }
+
+    fn save_as(
+        &self,
+        worktree: &ModelHandle<Worktree>,
+        path: &Path,
+        cx: &mut MutableAppContext,
+    ) -> Task<anyhow::Result<()>> {
+        self.update(cx, |item, cx| item.save_as(worktree, path, cx))
     }
 
     fn is_dirty(&self, cx: &AppContext) -> bool {
@@ -388,6 +409,23 @@ impl Workspace {
         }
     }
 
+    fn worktree_for_abs_path(
+        &mut self,
+        abs_path: &Path,
+        cx: &mut ViewContext<Self>,
+    ) -> (ModelHandle<Worktree>, PathBuf) {
+        for tree in self.worktrees.iter() {
+            if let Some(path) = tree
+                .read(cx)
+                .as_local()
+                .and_then(|tree| abs_path.strip_prefix(&tree.abs_path()).ok())
+            {
+                return (tree.clone(), path.to_path_buf());
+            }
+        }
+        (self.add_worktree(abs_path, cx), PathBuf::new())
+    }
+
     fn file_for_path(&mut self, abs_path: &Path, cx: &mut ViewContext<Self>) -> File {
         for tree in self.worktrees.iter() {
             if let Some(relative_path) = tree
@@ -559,19 +597,19 @@ impl Workspace {
             let handle = cx.handle();
             if item.entry_id(cx.as_ref()).is_none() {
                 let worktree = self.worktrees.iter().next();
-                let start_path = worktree
+                let start_abs_path = worktree
                     .and_then(|w| w.read(cx).as_local())
                     .map_or(Path::new(""), |w| w.abs_path())
                     .to_path_buf();
-                cx.prompt_for_new_path(&start_path, move |path, cx| {
+                cx.prompt_for_new_path(&start_abs_path, move |abs_path, cx| {
-                    if let Some(path) = path {
+                    if let Some(abs_path) = abs_path {
                         cx.spawn(|mut cx| async move {
-                            let result = async move {
+                            let result = handle
-                                let file =
+                                .update(&mut cx, |me, cx| {
-                                    handle.update(&mut cx, |me, cx| me.file_for_path(&path, cx));
+                                    let (worktree, path) = me.worktree_for_abs_path(&abs_path, cx);
-                                cx.update(|cx| item.save(Some(file), cx)).await
+                                    item.save_as(&worktree, &path, cx.as_mut())
-                            }
+                                })
-                            .await;
+                                .await;
                             if let Err(error) = result {
                                 error!("failed to save item: {:?}, ", error);
                             }
@@ -590,7 +628,7 @@ impl Workspace {
                     move |answer, cx| {
                         if answer == 0 {
                             cx.spawn(|mut cx| async move {
-                                if let Err(error) = cx.update(|cx| item.save(None, cx)).await {
+                                if let Err(error) = cx.update(|cx| item.save(cx)).unwrap().await {
                                     error!("failed to save item: {:?}, ", error);
                                 }
                             })
@@ -600,7 +638,7 @@ impl Workspace {
                 );
             } else {
                 cx.spawn(|_, mut cx| async move {
-                    if let Err(error) = cx.update(|cx| item.save(None, cx)).await {
+                    if let Err(error) = cx.update(|cx| item.save(cx)).unwrap().await {
                         error!("failed to save item: {:?}, ", error);
                     }
                 })

zed/src/worktree.rs

@@ -203,11 +203,11 @@ impl Worktree {
     pub fn save(
         &self,
         path: &Path,
-        content: Rope,
+        text: Rope,
         cx: &mut ModelContext<Self>,
     ) -> impl Future<Output = Result<()>> {
         match self {
-            Worktree::Local(worktree) => worktree.save(path, content, cx),
+            Worktree::Local(worktree) => worktree.save(path, text, cx),
             Worktree::Remote(_) => todo!(),
         }
     }
@@ -329,9 +329,7 @@ impl LocalWorktree {
                 Ok(existing_buffer)
             } else {
                 let contents = this
-                    .read_with(&cx, |this, cx| {
+                    .update(&mut cx, |this, cx| this.as_local().unwrap().load(&path, cx))
-                        this.as_local().unwrap().load(&path, cx.as_ref())
-                    })
                     .await?;
                 let language = language_registry.select_language(&path).cloned();
                 let file = File::new(handle, path.into());
@@ -390,8 +388,8 @@ impl LocalWorktree {
 
                         if diff.modified.contains(&path) {
                             cx.spawn(|buffer, mut cx| async move {
-                                let new_contents = handle
+                                let new_text = handle
-                                    .read_with(&cx, |this, cx| {
+                                    .update(&mut cx, |this, cx| {
                                         let this = this.as_local().unwrap();
                                         this.load(&path, cx)
                                     })
@@ -402,7 +400,7 @@ impl LocalWorktree {
                                 });
                                 if let Some(mtime) = mtime {
                                     buffer.update(&mut cx, |buffer, cx| {
-                                        buffer.file_was_modified(new_contents, mtime, cx)
+                                        buffer.file_was_modified(new_text, mtime, cx)
                                     });
                                 }
                                 Result::<_, anyhow::Error>::Ok(())
@@ -465,39 +463,72 @@ impl LocalWorktree {
         }
     }
 
-    fn load(&self, path: &Path, cx: &AppContext) -> Task<Result<String>> {
+    fn load(&self, path: &Path, cx: &mut ModelContext<Worktree>) -> Task<Result<String>> {
         let path = Arc::from(path);
         let abs_path = self.absolutize(&path);
         let background_snapshot = self.background_snapshot.clone();
 
-        cx.background().spawn(async move {
+        let load = cx.background().spawn(async move {
             let mut file = fs::File::open(&abs_path)?;
-            let mut contents = String::new();
+            let mut text = String::new();
-            file.read_to_string(&mut contents)?;
+            file.read_to_string(&mut text)?;
 
             // Eagerly populate the snapshot with an updated entry for the loaded file
             refresh_entry(&background_snapshot, path, &abs_path)?;
 
-            Result::<_, anyhow::Error>::Ok(contents)
+            Result::<_, anyhow::Error>::Ok(text)
+        });
+
+        cx.spawn(|this, mut cx| async move {
+            let text = load.await?;
+            this.update(&mut cx, |this, _| {
+                let this = this.as_local_mut().unwrap();
+                this.snapshot = this.background_snapshot.lock().clone();
+            });
+
+            Ok(text)
+        })
+    }
+
+    pub fn save_buffer_as(
+        &self,
+        buffer: ModelHandle<Buffer>,
+        path: impl Into<Arc<Path>>,
+        content: Rope,
+        cx: &mut ModelContext<Worktree>,
+    ) -> Task<Result<File>> {
+        let handle = cx.handle();
+        let path = path.into();
+        let save = self.save(path.clone(), content, cx);
+
+        cx.spawn(|this, mut cx| async move {
+            save.await?;
+            this.update(&mut cx, |this, _| {
+                if let Some(this) = this.as_local_mut() {
+                    this.open_buffers.insert(buffer.id(), buffer.downgrade());
+                }
+            });
+            Ok(File::new(handle, path))
         })
     }
 
     pub fn save(
         &self,
         path: impl Into<Arc<Path>>,
-        content: Rope,
+        text: Rope,
         cx: &mut ModelContext<Worktree>,
     ) -> Task<Result<()>> {
         let path = path.into();
         let abs_path = self.absolutize(&path);
         let background_snapshot = self.background_snapshot.clone();
+
         let save = {
             let path = path.clone();
             cx.background().spawn(async move {
-                let buffer_size = content.summary().bytes.min(10 * 1024);
+                let buffer_size = text.summary().bytes.min(10 * 1024);
                 let file = fs::File::create(&abs_path)?;
                 let mut writer = io::BufWriter::with_capacity(buffer_size, &file);
-                for chunk in content.chunks() {
+                for chunk in text.chunks() {
                     writer.write(chunk.as_bytes())?;
                 }
                 writer.flush()?;
@@ -511,9 +542,9 @@ impl LocalWorktree {
 
         cx.spawn(|worktree, mut cx| async move {
             save.await?;
-            worktree.update(&mut cx, |worktree, cx| {
+            worktree.update(&mut cx, |this, _| {
-                let worktree = worktree.as_local_mut().unwrap();
+                let this = this.as_local_mut().unwrap();
-                worktree.poll_snapshot(cx);
+                this.snapshot = this.background_snapshot.lock().clone();
             });
             Ok(())
         })
@@ -731,9 +762,10 @@ impl Snapshot {
     }
 
     pub fn paths(&self) -> impl Iterator<Item = &Arc<Path>> {
+        let empty_path = Path::new("");
         self.entries
             .cursor::<(), ()>()
-            .skip(1)
+            .filter(move |entry| entry.path.as_ref() != empty_path)
             .map(|entry| entry.path())
     }
 
@@ -799,10 +831,7 @@ impl Snapshot {
                 .insert(ignore_dir_path.into(), (Arc::new(ignore), self.scan_id));
         }
 
-        self.remove_path(entry.path());
+        self.reuse_entry_id(&mut entry);
-        if let Some(renamed_entry_id) = self.removed_entry_ids.remove(&entry.inode) {
-            entry.id = renamed_entry_id;
-        }
         self.entries.insert_or_replace(entry, &())
     }
 
@@ -830,14 +859,20 @@ impl Snapshot {
         edits.push(Edit::Insert(parent_entry));
 
         for mut entry in entries {
-            if let Some(renamed_entry_id) = self.removed_entry_ids.remove(&entry.inode) {
+            self.reuse_entry_id(&mut entry);
-                entry.id = renamed_entry_id;
-            }
             edits.push(Edit::Insert(entry));
         }
         self.entries.edit(edits, &());
     }
 
+    fn reuse_entry_id(&mut self, entry: &mut Entry) {
+        if let Some(removed_entry_id) = self.removed_entry_ids.remove(&entry.inode) {
+            entry.id = removed_entry_id;
+        } else if let Some(existing_entry) = self.entry_for_path(&entry.path) {
+            entry.id = existing_entry.id;
+        }
+    }
+
     fn remove_path(&mut self, path: &Path) {
         let mut new_entries;
         let removed_entry_ids;
@@ -980,16 +1015,6 @@ impl File {
         Self { worktree, path }
     }
 
-    pub fn buffer_added(&self, buffer: ModelHandle<Buffer>, cx: &mut MutableAppContext) {
-        self.worktree.update(cx, |worktree, _| {
-            if let Worktree::Local(worktree) = worktree {
-                worktree
-                    .open_buffers
-                    .insert(buffer.id(), buffer.downgrade());
-            }
-        })
-    }
-
     pub fn buffer_updated(&self, buffer_id: u64, operation: Operation, cx: &mut MutableAppContext) {
         self.worktree.update(cx, |worktree, cx| {
             if let Some((rpc, remote_id)) = match worktree {
@@ -1066,13 +1091,9 @@ impl File {
             .map_or(UNIX_EPOCH, |entry| entry.mtime)
     }
 
-    pub fn save(
+    pub fn save(&self, text: Rope, cx: &mut MutableAppContext) -> impl Future<Output = Result<()>> {
-        &self,
-        content: Rope,
-        cx: &mut MutableAppContext,
-    ) -> impl Future<Output = Result<()>> {
         self.worktree
-            .update(cx, |worktree, cx| worktree.save(&self.path(), content, cx))
+            .update(cx, |worktree, cx| worktree.save(&self.path(), text, cx))
     }
 
     pub fn worktree_id(&self) -> usize {
@@ -1217,7 +1238,7 @@ impl<'a> Ord for PathSearch<'a> {
                     a.cmp(b)
                 }
             }
-            _ => todo!("not sure we need the other two cases"),
+            _ => unreachable!("not sure we need the other two cases"),
         }
     }
 }
@@ -1684,7 +1705,7 @@ fn refresh_entry(snapshot: &Mutex<Snapshot>, path: Arc<Path>, abs_path: &Path) -
         root_char_bag = snapshot.root_char_bag;
         next_entry_id = snapshot.next_entry_id.clone();
     }
-    let entry = fs_entry_for_path(root_char_bag, &next_entry_id, path, &abs_path)?
+    let entry = fs_entry_for_path(root_char_bag, &next_entry_id, path, abs_path)?
         .ok_or_else(|| anyhow!("could not read saved file metadata"))?;
     snapshot.lock().insert_entry(entry);
     Ok(())
@@ -2119,11 +2140,8 @@ mod tests {
         .await
         .unwrap();
 
-        let new_contents = fs::read_to_string(dir.path().join(path)).unwrap();
+        let new_text = fs::read_to_string(dir.path().join(path)).unwrap();
-        assert_eq!(
+        assert_eq!(new_text, buffer.read_with(&cx, |buffer, _| buffer.text()));
-            new_contents,
-            buffer.read_with(&cx, |buffer, _| buffer.text())
-        );
     }
 
     #[gpui::test]
@@ -2149,11 +2167,8 @@ mod tests {
         .await
         .unwrap();
 
-        let new_contents = fs::read_to_string(file_path).unwrap();
+        let new_text = fs::read_to_string(file_path).unwrap();
-        assert_eq!(
+        assert_eq!(new_text, buffer.read_with(&cx, |buffer, _| buffer.text()));
-            new_contents,
-            buffer.read_with(&cx, |buffer, _| buffer.text())
-        );
     }
 
     #[gpui::test]
@@ -2183,7 +2198,11 @@ mod tests {
             async move { buffer.await.unwrap() }
         };
         let id_for_path = |path: &'static str, cx: &gpui::TestAppContext| {
-            tree.read_with(cx, |tree, _| tree.entry_for_path(path).unwrap().id)
+            tree.read_with(cx, |tree, _| {
+                tree.entry_for_path(path)
+                    .expect(&format!("no entry for path {}", path))
+                    .id
+            })
         };
 
         let buffer2 = buffer_for_path("a/file2", &mut cx).await;