From b7a4393f2904ffaa739459db15e5f51a4b58879b Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Mon, 28 Jun 2021 19:05:38 -0600 Subject: [PATCH] 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 | 16 ++++- zed/src/editor/buffer.rs | 83 +++++++++++++++---------- zed/src/workspace.rs | 68 +++++++++++++++----- zed/src/worktree.rs | 131 ++++++++++++++++++++++----------------- 4 files changed, 190 insertions(+), 108 deletions(-) diff --git a/zed/src/editor.rs b/zed/src/editor.rs index a2dd1c15da..bbeeeb647e 100644 --- a/zed/src/editor.rs +++ b/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, cx: &mut ViewContext) -> Task> { - self.buffer.update(cx, |b, cx| b.save(new_file, cx)) + fn save(&mut self, cx: &mut ViewContext) -> Result>> { + self.buffer.update(cx, |b, cx| b.save(cx)) + } + + fn save_as( + &mut self, + worktree: &ModelHandle, + path: &Path, + cx: &mut ViewContext, + ) -> Task> { + self.buffer + .update(cx, |b, cx| b.save_as(worktree, path, cx)) } fn is_dirty(&self, cx: &AppContext) -> bool { diff --git a/zed/src/editor/buffer.rs b/zed/src/editor/buffer.rs index 2f1d463f7b..e601989396 100644 --- a/zed/src/editor/buffer.rs +++ b/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( - &mut self, - new_file: Option, - cx: &mut ModelContext, - ) -> Task> { + pub fn save(&mut self, cx: &mut ModelContext) -> Result>> { + let file = self + .file + .as_ref() + .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 { - if let Some(file) = new_file.as_ref().or(file.as_ref()) { - let result = cx.update(|cx| file.save(text, cx)).await; - if result.is_ok() { - handle.update(&mut cx, |me, cx| me.did_save(version, new_file, cx)); - } - result - } else { - Ok(()) - } + Ok(cx.spawn(|this, mut cx| async move { + save.await?; + this.update(&mut cx, |this, cx| { + this.did_save(version, cx).unwrap(); + }); + Ok(()) + })) + } + + pub fn save_as( + &mut self, + worktree: &ModelHandle, + path: impl Into>, + cx: &mut ModelContext, + ) -> Task> { + 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( - &mut self, - version: time::Global, - new_file: Option, - cx: &mut ModelContext, - ) { - 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 { + fn did_save(&mut self, version: time::Global, cx: &mut ModelContext) -> Result<()> { + if let Some(file) = self.file.as_ref() { 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, cx: &mut ModelContext) { @@ -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(), None, cx); + buffer.did_save(buffer.version(), cx).unwrap(); }); // after saving, the buffer is not dirty, and emits a saved event. diff --git a/zed/src/workspace.rs b/zed/src/workspace.rs index 15f7dba23d..9b74023ef1 100644 --- a/zed/src/workspace.rs +++ b/zed/src/workspace.rs @@ -127,7 +127,13 @@ pub trait ItemView: View { fn has_conflict(&self, _: &AppContext) -> bool { false } - fn save(&mut self, _: Option, _: &mut ViewContext) -> Task>; + fn save(&mut self, cx: &mut ViewContext) -> Result>>; + fn save_as( + &mut self, + worktree: &ModelHandle, + path: &Path, + cx: &mut ViewContext, + ) -> Task>; 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, cx: &mut MutableAppContext) -> Task>; + fn save(&self, cx: &mut MutableAppContext) -> Result>>; + fn save_as( + &self, + worktree: &ModelHandle, + path: &Path, + cx: &mut MutableAppContext, + ) -> Task>; } impl ItemHandle for ModelHandle { @@ -236,8 +248,17 @@ impl ItemViewHandle for ViewHandle { }) } - fn save(&self, file: Option, cx: &mut MutableAppContext) -> Task> { - self.update(cx, |item, cx| item.save(file, cx)) + fn save(&self, cx: &mut MutableAppContext) -> Result>> { + self.update(cx, |item, cx| item.save(cx)) + } + + fn save_as( + &self, + worktree: &ModelHandle, + path: &Path, + cx: &mut MutableAppContext, + ) -> Task> { + 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, + ) -> (ModelHandle, 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) -> 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| { - if let Some(path) = path { + cx.prompt_for_new_path(&start_abs_path, move |abs_path, cx| { + if let Some(abs_path) = abs_path { cx.spawn(|mut cx| async move { - let result = async move { - let file = - handle.update(&mut cx, |me, cx| me.file_for_path(&path, cx)); - cx.update(|cx| item.save(Some(file), cx)).await - } - .await; + let result = handle + .update(&mut cx, |me, cx| { + let (worktree, path) = me.worktree_for_abs_path(&abs_path, cx); + item.save_as(&worktree, &path, cx.as_mut()) + }) + .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); } }) diff --git a/zed/src/worktree.rs b/zed/src/worktree.rs index 82e1723370..1d8702d0e7 100644 --- a/zed/src/worktree.rs +++ b/zed/src/worktree.rs @@ -203,11 +203,11 @@ impl Worktree { pub fn save( &self, path: &Path, - content: Rope, + text: Rope, cx: &mut ModelContext, ) -> impl Future> { 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| { - this.as_local().unwrap().load(&path, cx.as_ref()) - }) + .update(&mut cx, |this, cx| this.as_local().unwrap().load(&path, cx)) .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 - .read_with(&cx, |this, cx| { + let new_text = handle + .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> { + fn load(&self, path: &Path, cx: &mut ModelContext) -> Task> { 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(); - file.read_to_string(&mut contents)?; + let mut text = String::new(); + 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, + path: impl Into>, + content: Rope, + cx: &mut ModelContext, + ) -> Task> { + 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>, - content: Rope, + text: Rope, cx: &mut ModelContext, ) -> Task> { 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| { - let worktree = worktree.as_local_mut().unwrap(); - worktree.poll_snapshot(cx); + worktree.update(&mut cx, |this, _| { + let this = this.as_local_mut().unwrap(); + this.snapshot = this.background_snapshot.lock().clone(); }); Ok(()) }) @@ -731,9 +762,10 @@ impl Snapshot { } pub fn paths(&self) -> impl Iterator> { + 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()); - if let Some(renamed_entry_id) = self.removed_entry_ids.remove(&entry.inode) { - entry.id = renamed_entry_id; - } + self.reuse_entry_id(&mut entry); 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) { - entry.id = renamed_entry_id; - } + self.reuse_entry_id(&mut entry); 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, 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( - &self, - content: Rope, - cx: &mut MutableAppContext, - ) -> impl Future> { + pub fn save(&self, text: Rope, cx: &mut MutableAppContext) -> impl Future> { 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, path: Arc, 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(); - assert_eq!( - new_contents, - buffer.read_with(&cx, |buffer, _| buffer.text()) - ); + let new_text = fs::read_to_string(dir.path().join(path)).unwrap(); + assert_eq!(new_text, 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(); - assert_eq!( - new_contents, - buffer.read_with(&cx, |buffer, _| buffer.text()) - ); + let new_text = fs::read_to_string(file_path).unwrap(); + assert_eq!(new_text, 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;