From d99f5fe83e4d4bec572b0ea9f6284cb9a285d1a6 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 18 Nov 2024 10:30:08 -0800 Subject: [PATCH] Add `File.disk_state` enum to clarify filesystem states (#20776) Motivation for this is to make things more understandable while figuring out #20775. This is intended to be a refactoring that does not affect behavior, but there are a few tricky spots: * Previously `File.mtime()` (now `File.disk_state().mtime()`) would return last known modification time for deleted files. Looking at uses, I believe this will not affect anything. If there are behavior changes here I believe they would be improvements. * `BufferEvent::DirtyChanged` is now only emitted if dirtiness actually changed, rather than if it may have changed. This should only be an efficiency improvement. Release Notes: - N/A Co-authored-by: Mikayla Maki --- Cargo.toml | 2 +- .../random_project_collaboration_tests.rs | 7 +- crates/copilot/src/copilot.rs | 10 +- crates/editor/src/items.rs | 5 +- crates/language/src/buffer.rs | 98 +++++++++++-------- crates/multi_buffer/src/multi_buffer.rs | 10 +- crates/project/src/buffer_store.rs | 37 +++---- crates/project/src/image_store.rs | 38 +++---- crates/project/src/project_tests.rs | 30 ++++-- crates/worktree/src/worktree.rs | 58 ++++++----- 10 files changed, 161 insertions(+), 134 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 14f43f4e82573..ef5569f72b344 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -388,7 +388,7 @@ pet-core = { git = "https://github.com/microsoft/python-environment-tools.git", pet-poetry = { git = "https://github.com/microsoft/python-environment-tools.git", rev = "ffcbf3f28c46633abd5448a52b1f396c322e0d6c" } pet-reporter = { git = "https://github.com/microsoft/python-environment-tools.git", rev = "ffcbf3f28c46633abd5448a52b1f396c322e0d6c" } postage = { version = "0.5", features = ["futures-traits"] } -pretty_assertions = "1.3.0" +pretty_assertions = { version = "1.3.0", features = ["unstable"] } profiling = "1" prost = "0.9" prost-build = "0.9" diff --git a/crates/collab/src/tests/random_project_collaboration_tests.rs b/crates/collab/src/tests/random_project_collaboration_tests.rs index 47f6a38073175..66a9d0680479a 100644 --- a/crates/collab/src/tests/random_project_collaboration_tests.rs +++ b/crates/collab/src/tests/random_project_collaboration_tests.rs @@ -1323,11 +1323,8 @@ impl RandomizedTest for ProjectCollaborationTest { match (host_file, guest_file) { (Some(host_file), Some(guest_file)) => { assert_eq!(guest_file.path(), host_file.path()); - assert_eq!(guest_file.is_deleted(), host_file.is_deleted()); - assert_eq!( - guest_file.mtime(), - host_file.mtime(), - "guest {} mtime does not match host {} for path {:?} in project {}", + assert_eq!(guest_file.disk_state(), host_file.disk_state(), + "guest {} disk_state does not match host {} for path {:?} in project {}", guest_user_id, host_user_id, guest_file.path(), diff --git a/crates/copilot/src/copilot.rs b/crates/copilot/src/copilot.rs index a9cf406860a48..b654df1d6ee75 100644 --- a/crates/copilot/src/copilot.rs +++ b/crates/copilot/src/copilot.rs @@ -1229,8 +1229,10 @@ mod tests { Some(self) } - fn mtime(&self) -> Option { - unimplemented!() + fn disk_state(&self) -> language::DiskState { + language::DiskState::Present { + mtime: std::time::UNIX_EPOCH, + } } fn path(&self) -> &Arc { @@ -1245,10 +1247,6 @@ mod tests { unimplemented!() } - fn is_deleted(&self) -> bool { - unimplemented!() - } - fn as_any(&self) -> &dyn std::any::Any { unimplemented!() } diff --git a/crates/editor/src/items.rs b/crates/editor/src/items.rs index fb26a5654ba68..d3914f6772ddc 100644 --- a/crates/editor/src/items.rs +++ b/crates/editor/src/items.rs @@ -16,7 +16,8 @@ use gpui::{ VisualContext, WeakView, WindowContext, }; use language::{ - proto::serialize_anchor as serialize_text_anchor, Bias, Buffer, CharKind, Point, SelectionGoal, + proto::serialize_anchor as serialize_text_anchor, Bias, Buffer, CharKind, DiskState, Point, + SelectionGoal, }; use lsp::DiagnosticSeverity; use multi_buffer::AnchorRangeExt; @@ -641,7 +642,7 @@ impl Item for Editor { .read(cx) .as_singleton() .and_then(|buffer| buffer.read(cx).file()) - .map_or(false, |file| file.is_deleted() && file.is_created()); + .map_or(false, |file| file.disk_state() == DiskState::Deleted); h_flex() .gap_2() diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 77fc53705fd36..571e444d7c06b 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -104,6 +104,7 @@ pub struct Buffer { text: TextBuffer, diff_base: Option, git_diff: git::diff::BufferDiff, + /// Filesystem state, `None` when there is no path. file: Option>, /// The mtime of the file when this buffer was last loaded from /// or saved to disk. @@ -371,8 +372,9 @@ pub trait File: Send + Sync { self.as_local().is_some() } - /// Returns the file's mtime. - fn mtime(&self) -> Option; + /// Returns whether the file is new, exists in storage, or has been deleted. Includes metadata + /// only available in some states, such as modification time. + fn disk_state(&self) -> DiskState; /// Returns the path of this file relative to the worktree's root directory. fn path(&self) -> &Arc; @@ -390,14 +392,6 @@ pub trait File: Send + Sync { /// This is needed for looking up project-specific settings. fn worktree_id(&self, cx: &AppContext) -> WorktreeId; - /// Returns whether the file has been deleted. - fn is_deleted(&self) -> bool; - - /// Returns whether the file existed on disk at one point - fn is_created(&self) -> bool { - self.mtime().is_some() - } - /// Converts this file into an [`Any`] trait object. fn as_any(&self) -> &dyn Any; @@ -408,6 +402,34 @@ pub trait File: Send + Sync { fn is_private(&self) -> bool; } +/// The file's storage status - whether it's stored (`Present`), and if so when it was last +/// modified. In the case where the file is not stored, it can be either `New` or `Deleted`. In the +/// UI these two states are distinguished. For example, the buffer tab does not display a deletion +/// indicator for new files. +#[derive(Clone, Copy, Debug, PartialEq)] +pub enum DiskState { + /// File created in Zed that has not been saved. + New, + /// File present on the filesystem. + Present { + /// Last known mtime (modification time). + mtime: SystemTime, + }, + /// Deleted file that was previously present. + Deleted, +} + +impl DiskState { + /// Returns the file's last known modification time on disk. + pub fn mtime(self) -> Option { + match self { + DiskState::New => None, + DiskState::Present { mtime } => Some(mtime), + DiskState::Deleted => None, + } + } +} + /// The file associated with a buffer, in the case where the file is on the local disk. pub trait LocalFile: File { /// Returns the absolute path of this file @@ -750,7 +772,7 @@ impl Buffer { file: Option>, capability: Capability, ) -> Self { - let saved_mtime = file.as_ref().and_then(|file| file.mtime()); + let saved_mtime = file.as_ref().and_then(|file| file.disk_state().mtime()); let snapshot = buffer.snapshot(); let git_diff = git::diff::BufferDiff::new(&snapshot); let syntax_map = Mutex::new(SyntaxMap::new(&snapshot)); @@ -1014,7 +1036,7 @@ impl Buffer { self.reload_task = Some(cx.spawn(|this, mut cx| async move { let Some((new_mtime, new_text)) = this.update(&mut cx, |this, cx| { let file = this.file.as_ref()?.as_local()?; - Some((file.mtime(), file.load(cx))) + Some((file.disk_state().mtime(), file.load(cx))) })? else { return Ok(()); @@ -1070,6 +1092,7 @@ impl Buffer { /// Updates the [`File`] backing this buffer. This should be called when /// the file has changed or has been deleted. pub fn file_updated(&mut self, new_file: Arc, cx: &mut ModelContext) { + let was_dirty = self.is_dirty(); let mut file_changed = false; if let Some(old_file) = self.file.as_ref() { @@ -1077,21 +1100,12 @@ impl Buffer { file_changed = true; } - if new_file.is_deleted() { - if !old_file.is_deleted() { - file_changed = true; - if !self.is_dirty() { - cx.emit(BufferEvent::DirtyChanged); - } - } - } else { - let new_mtime = new_file.mtime(); - if new_mtime != old_file.mtime() { - file_changed = true; - - if !self.is_dirty() { - cx.emit(BufferEvent::ReloadNeeded); - } + let old_state = old_file.disk_state(); + let new_state = new_file.disk_state(); + if old_state != new_state { + file_changed = true; + if !was_dirty && matches!(new_state, DiskState::Present { .. }) { + cx.emit(BufferEvent::ReloadNeeded) } } } else { @@ -1101,6 +1115,9 @@ impl Buffer { self.file = Some(new_file); if file_changed { self.non_text_state_update_count += 1; + if was_dirty != self.is_dirty() { + cx.emit(BufferEvent::DirtyChanged); + } cx.emit(BufferEvent::FileHandleChanged); cx.notify(); } @@ -1742,15 +1759,10 @@ impl Buffer { pub fn is_dirty(&self) -> bool { self.capability != Capability::ReadOnly && (self.has_conflict - || self.has_unsaved_edits() - || self - .file - .as_ref() - .map_or(false, |file| file.is_deleted() || !file.is_created())) - } - - pub fn is_deleted(&self) -> bool { - self.file.as_ref().map_or(false, |file| file.is_deleted()) + || self.file.as_ref().map_or(false, |file| { + matches!(file.disk_state(), DiskState::New | DiskState::Deleted) + }) + || self.has_unsaved_edits()) } /// Checks if the buffer and its file have both changed since the buffer @@ -1762,7 +1774,13 @@ impl Buffer { let Some(file) = self.file.as_ref() else { return false; }; - file.is_deleted() || (file.mtime() > self.saved_mtime && self.has_unsaved_edits()) + match file.disk_state() { + DiskState::New | DiskState::Deleted => true, + DiskState::Present { mtime } => match self.saved_mtime { + Some(saved_mtime) => mtime > saved_mtime && self.has_unsaved_edits(), + None => true, + }, + } } /// Gets a [`Subscription`] that tracks all of the changes to the buffer's text. @@ -4403,7 +4421,7 @@ impl File for TestFile { None } - fn mtime(&self) -> Option { + fn disk_state(&self) -> DiskState { unimplemented!() } @@ -4415,10 +4433,6 @@ impl File for TestFile { WorktreeId::from_usize(0) } - fn is_deleted(&self) -> bool { - unimplemented!() - } - fn as_any(&self) -> &dyn std::any::Any { unimplemented!() } diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 5d111d81cec4d..b6ba702b4ec83 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -10,9 +10,9 @@ use itertools::Itertools; use language::{ language_settings::{language_settings, LanguageSettings}, AutoindentMode, Buffer, BufferChunks, BufferRow, BufferSnapshot, Capability, CharClassifier, - CharKind, Chunk, CursorShape, DiagnosticEntry, File, IndentGuide, IndentSize, Language, - LanguageScope, OffsetRangeExt, OffsetUtf16, Outline, OutlineItem, Point, PointUtf16, Selection, - TextDimension, ToOffset as _, ToOffsetUtf16 as _, ToPoint as _, ToPointUtf16 as _, + CharKind, Chunk, CursorShape, DiagnosticEntry, DiskState, File, IndentGuide, IndentSize, + Language, LanguageScope, OffsetRangeExt, OffsetUtf16, Outline, OutlineItem, Point, PointUtf16, + Selection, TextDimension, ToOffset as _, ToOffsetUtf16 as _, ToPoint as _, ToPointUtf16 as _, TransactionId, Unclipped, }; use smallvec::SmallVec; @@ -2035,7 +2035,9 @@ impl MultiBuffer { edited |= buffer_edited; non_text_state_updated |= buffer_non_text_state_updated; is_dirty |= buffer.is_dirty(); - has_deleted_file |= buffer.file().map_or(false, |file| file.is_deleted()); + has_deleted_file |= buffer + .file() + .map_or(false, |file| file.disk_state() == DiskState::Deleted); has_conflict |= buffer.has_conflict(); } if edited { diff --git a/crates/project/src/buffer_store.rs b/crates/project/src/buffer_store.rs index 5d8fb3ab391d1..634d87dca2174 100644 --- a/crates/project/src/buffer_store.rs +++ b/crates/project/src/buffer_store.rs @@ -20,7 +20,7 @@ use language::{ deserialize_line_ending, deserialize_version, serialize_line_ending, serialize_version, split_operations, }, - Buffer, BufferEvent, Capability, File as _, Language, Operation, + Buffer, BufferEvent, Capability, DiskState, File as _, Language, Operation, }; use rpc::{proto, AnyProtoClient, ErrorExt as _, TypedEnvelope}; use smol::channel::Receiver; @@ -434,7 +434,10 @@ impl LocalBufferStore { let line_ending = buffer.line_ending(); let version = buffer.version(); let buffer_id = buffer.remote_id(); - if buffer.file().is_some_and(|file| !file.is_created()) { + if buffer + .file() + .is_some_and(|file| file.disk_state() == DiskState::New) + { has_changed_file = true; } @@ -444,7 +447,7 @@ impl LocalBufferStore { cx.spawn(move |this, mut cx| async move { let new_file = save.await?; - let mtime = new_file.mtime; + let mtime = new_file.disk_state().mtime(); this.update(&mut cx, |this, cx| { if let Some((downstream_client, project_id)) = this.downstream_client(cx) { if has_changed_file { @@ -658,37 +661,30 @@ impl LocalBufferStore { return None; } - let new_file = if let Some(entry) = old_file + let snapshot_entry = old_file .entry_id .and_then(|entry_id| snapshot.entry_for_id(entry_id)) - { - File { - is_local: true, - entry_id: Some(entry.id), - mtime: entry.mtime, - path: entry.path.clone(), - worktree: worktree.clone(), - is_deleted: false, - is_private: entry.is_private, - } - } else if let Some(entry) = snapshot.entry_for_path(old_file.path.as_ref()) { + .or_else(|| snapshot.entry_for_path(old_file.path.as_ref())); + + let new_file = if let Some(entry) = snapshot_entry { File { + disk_state: match entry.mtime { + Some(mtime) => DiskState::Present { mtime }, + None => old_file.disk_state, + }, is_local: true, entry_id: Some(entry.id), - mtime: entry.mtime, path: entry.path.clone(), worktree: worktree.clone(), - is_deleted: false, is_private: entry.is_private, } } else { File { + disk_state: DiskState::Deleted, is_local: true, entry_id: old_file.entry_id, path: old_file.path.clone(), - mtime: old_file.mtime, worktree: worktree.clone(), - is_deleted: true, is_private: old_file.is_private, } }; @@ -867,10 +863,9 @@ impl BufferStoreImpl for Model { Some(Arc::new(File { worktree, path, - mtime: None, + disk_state: DiskState::New, entry_id: None, is_local: true, - is_deleted: false, is_private: false, })), Capability::ReadWrite, diff --git a/crates/project/src/image_store.rs b/crates/project/src/image_store.rs index b3425e7fade6e..9f794d5248c8b 100644 --- a/crates/project/src/image_store.rs +++ b/crates/project/src/image_store.rs @@ -9,7 +9,7 @@ use gpui::{ hash, prelude::*, AppContext, EventEmitter, Img, Model, ModelContext, Subscription, Task, WeakModel, }; -use language::File; +use language::{DiskState, File}; use rpc::{AnyProtoClient, ErrorExt as _}; use std::ffi::OsStr; use std::num::NonZeroU64; @@ -74,11 +74,12 @@ impl ImageItem { file_changed = true; } - if !new_file.is_deleted() { - let new_mtime = new_file.mtime(); - if new_mtime != old_file.mtime() { - file_changed = true; - cx.emit(ImageItemEvent::ReloadNeeded); + let old_state = old_file.disk_state(); + let new_state = new_file.disk_state(); + if old_state != new_state { + file_changed = true; + if matches!(new_state, DiskState::Present { .. }) { + cx.emit(ImageItemEvent::ReloadNeeded) } } @@ -503,37 +504,30 @@ impl LocalImageStore { return; } - let new_file = if let Some(entry) = old_file + let snapshot_entry = old_file .entry_id .and_then(|entry_id| snapshot.entry_for_id(entry_id)) - { - worktree::File { - is_local: true, - entry_id: Some(entry.id), - mtime: entry.mtime, - path: entry.path.clone(), - worktree: worktree.clone(), - is_deleted: false, - is_private: entry.is_private, - } - } else if let Some(entry) = snapshot.entry_for_path(old_file.path.as_ref()) { + .or_else(|| snapshot.entry_for_path(old_file.path.as_ref())); + + let new_file = if let Some(entry) = snapshot_entry { worktree::File { + disk_state: match entry.mtime { + Some(mtime) => DiskState::Present { mtime }, + None => old_file.disk_state, + }, is_local: true, entry_id: Some(entry.id), - mtime: entry.mtime, path: entry.path.clone(), worktree: worktree.clone(), - is_deleted: false, is_private: entry.is_private, } } else { worktree::File { + disk_state: DiskState::Deleted, is_local: true, entry_id: old_file.entry_id, path: old_file.path.clone(), - mtime: old_file.mtime, worktree: worktree.clone(), - is_deleted: true, is_private: old_file.is_private, } }; diff --git a/crates/project/src/project_tests.rs b/crates/project/src/project_tests.rs index 490d2e67b5ccc..ab00d62d6cd10 100644 --- a/crates/project/src/project_tests.rs +++ b/crates/project/src/project_tests.rs @@ -5,12 +5,12 @@ use gpui::{AppContext, SemanticVersion, UpdateGlobal}; use http_client::Url; use language::{ language_settings::{language_settings, AllLanguageSettings, LanguageSettingsContent}, - tree_sitter_rust, tree_sitter_typescript, Diagnostic, DiagnosticSet, FakeLspAdapter, + tree_sitter_rust, tree_sitter_typescript, Diagnostic, DiagnosticSet, DiskState, FakeLspAdapter, LanguageConfig, LanguageMatcher, LanguageName, LineEnding, OffsetRangeExt, Point, ToPoint, }; use lsp::{DiagnosticSeverity, NumberOrString}; use parking_lot::Mutex; -use pretty_assertions::assert_eq; +use pretty_assertions::{assert_eq, assert_matches}; use serde_json::json; #[cfg(not(windows))] use std::os; @@ -3239,10 +3239,22 @@ async fn test_rescan_and_remote_updates(cx: &mut gpui::TestAppContext) { Path::new("b/c/file5") ); - assert!(!buffer2.read(cx).file().unwrap().is_deleted()); - assert!(!buffer3.read(cx).file().unwrap().is_deleted()); - assert!(!buffer4.read(cx).file().unwrap().is_deleted()); - assert!(buffer5.read(cx).file().unwrap().is_deleted()); + assert_matches!( + buffer2.read(cx).file().unwrap().disk_state(), + DiskState::Present { .. } + ); + assert_matches!( + buffer3.read(cx).file().unwrap().disk_state(), + DiskState::Present { .. } + ); + assert_matches!( + buffer4.read(cx).file().unwrap().disk_state(), + DiskState::Present { .. } + ); + assert_eq!( + buffer5.read(cx).file().unwrap().disk_state(), + DiskState::Deleted + ); }); // Update the remote worktree. Check that it becomes consistent with the @@ -3416,7 +3428,11 @@ async fn test_buffer_is_dirty(cx: &mut gpui::TestAppContext) { ] ); events.lock().clear(); - buffer.did_save(buffer.version(), buffer.file().unwrap().mtime(), cx); + buffer.did_save( + buffer.version(), + buffer.file().unwrap().disk_state().mtime(), + cx, + ); }); // after saving, the buffer is not dirty, and emits a saved event. diff --git a/crates/worktree/src/worktree.rs b/crates/worktree/src/worktree.rs index a3e290e6d65de..5bd064b534c1b 100644 --- a/crates/worktree/src/worktree.rs +++ b/crates/worktree/src/worktree.rs @@ -29,6 +29,7 @@ use gpui::{ Task, }; use ignore::IgnoreStack; +use language::DiskState; use parking_lot::Mutex; use paths::local_settings_folder_relative_path; use postage::{ @@ -1313,9 +1314,10 @@ impl LocalWorktree { entry_id: None, worktree, path, - mtime: Some(metadata.mtime), + disk_state: DiskState::Present { + mtime: metadata.mtime, + }, is_local: true, - is_deleted: false, is_private, }) } @@ -1374,9 +1376,10 @@ impl LocalWorktree { entry_id: None, worktree, path, - mtime: Some(metadata.mtime), + disk_state: DiskState::Present { + mtime: metadata.mtime, + }, is_local: true, - is_deleted: false, is_private, }) } @@ -1512,10 +1515,11 @@ impl LocalWorktree { Ok(Arc::new(File { worktree, path, - mtime: Some(metadata.mtime), + disk_state: DiskState::Present { + mtime: metadata.mtime, + }, entry_id: None, is_local: true, - is_deleted: false, is_private, })) } @@ -3178,10 +3182,9 @@ impl fmt::Debug for Snapshot { pub struct File { pub worktree: Model, pub path: Arc, - pub mtime: Option, + pub disk_state: DiskState, pub entry_id: Option, pub is_local: bool, - pub is_deleted: bool, pub is_private: bool, } @@ -3194,8 +3197,8 @@ impl language::File for File { } } - fn mtime(&self) -> Option { - self.mtime + fn disk_state(&self) -> DiskState { + self.disk_state } fn path(&self) -> &Arc { @@ -3238,10 +3241,6 @@ impl language::File for File { self.worktree.read(cx).id() } - fn is_deleted(&self) -> bool { - self.is_deleted - } - fn as_any(&self) -> &dyn Any { self } @@ -3251,8 +3250,8 @@ impl language::File for File { worktree_id: self.worktree.read(cx).id().to_proto(), entry_id: self.entry_id.map(|id| id.to_proto()), path: self.path.to_string_lossy().into(), - mtime: self.mtime.map(|time| time.into()), - is_deleted: self.is_deleted, + mtime: self.disk_state.mtime().map(|time| time.into()), + is_deleted: self.disk_state == DiskState::Deleted, } } @@ -3293,10 +3292,13 @@ impl File { Arc::new(Self { worktree, path: entry.path.clone(), - mtime: entry.mtime, + disk_state: if let Some(mtime) = entry.mtime { + DiskState::Present { mtime } + } else { + DiskState::New + }, entry_id: Some(entry.id), is_local: true, - is_deleted: false, is_private: entry.is_private, }) } @@ -3316,13 +3318,22 @@ impl File { return Err(anyhow!("worktree id does not match file")); } + let disk_state = if proto.is_deleted { + DiskState::Deleted + } else { + if let Some(mtime) = proto.mtime.map(&Into::into) { + DiskState::Present { mtime } + } else { + DiskState::New + } + }; + Ok(Self { worktree, path: Path::new(&proto.path).into(), - mtime: proto.mtime.map(|time| time.into()), + disk_state, entry_id: proto.entry_id.map(ProjectEntryId::from_proto), is_local: false, - is_deleted: proto.is_deleted, is_private: false, }) } @@ -3336,10 +3347,9 @@ impl File { } pub fn project_entry_id(&self, _: &AppContext) -> Option { - if self.is_deleted { - None - } else { - self.entry_id + match self.disk_state { + DiskState::Deleted => None, + _ => self.entry_id, } } }