Merge pull request #1870 from zed-industries/fix-remote-abs-paths

Antonio Scandurra created

Fix bug where absolute paths of worktrees were not being stored on the server

Change summary

crates/collab/src/integration_tests.rs |  7 ++
crates/collab/src/rpc.rs               |  6 
crates/collab/src/rpc/store.rs         | 16 ++++
crates/editor/src/items.rs             | 86 ++++++++++++++++++++++++++-
crates/project/src/worktree.rs         |  8 +-
5 files changed, 109 insertions(+), 14 deletions(-)

Detailed changes

crates/collab/src/integration_tests.rs 🔗

@@ -5579,6 +5579,13 @@ async fn test_random_collaboration(
                                 guest_client.username,
                                 id
                             );
+                            assert_eq!(
+                                guest_snapshot.abs_path(),
+                                host_snapshot.abs_path(),
+                                "{} has different abs path than the host for worktree {}",
+                                guest_client.username,
+                                id
+                            );
                             assert_eq!(
                                 guest_snapshot.entries(false).collect::<Vec<_>>(),
                                 host_snapshot.entries(false).collect::<Vec<_>>(),

crates/collab/src/rpc.rs 🔗

@@ -42,7 +42,6 @@ use std::{
     marker::PhantomData,
     net::SocketAddr,
     ops::{Deref, DerefMut},
-    os::unix::prelude::OsStrExt,
     rc::Rc,
     sync::{
         atomic::{AtomicBool, Ordering::SeqCst},
@@ -945,7 +944,7 @@ impl Server {
                 id: *id,
                 root_name: worktree.root_name.clone(),
                 visible: worktree.visible,
-                abs_path: worktree.abs_path.as_os_str().as_bytes().to_vec(),
+                abs_path: worktree.abs_path.clone(),
             })
             .collect::<Vec<_>>();
 
@@ -996,7 +995,7 @@ impl Server {
             let message = proto::UpdateWorktree {
                 project_id: project_id.to_proto(),
                 worktree_id: *worktree_id,
-                abs_path: worktree.abs_path.as_os_str().as_bytes().to_vec(),
+                abs_path: worktree.abs_path.clone(),
                 root_name: worktree.root_name.clone(),
                 updated_entries: worktree.entries.values().cloned().collect(),
                 removed_entries: Default::default(),
@@ -1105,6 +1104,7 @@ impl Server {
             project_id,
             worktree_id,
             &request.payload.root_name,
+            &request.payload.abs_path,
             &request.payload.removed_entries,
             &request.payload.updated_entries,
             request.payload.scan_id,

crates/collab/src/rpc/store.rs 🔗

@@ -61,7 +61,7 @@ pub struct Collaborator {
 
 #[derive(Default, Serialize)]
 pub struct Worktree {
-    pub abs_path: PathBuf,
+    pub abs_path: Vec<u8>,
     pub root_name: String,
     pub visible: bool,
     #[serde(skip)]
@@ -711,7 +711,11 @@ impl Store {
                             Worktree {
                                 root_name: worktree.root_name,
                                 visible: worktree.visible,
-                                ..Default::default()
+                                abs_path: worktree.abs_path.clone(),
+                                entries: Default::default(),
+                                diagnostic_summaries: Default::default(),
+                                scan_id: Default::default(),
+                                is_complete: Default::default(),
                             },
                         )
                     })
@@ -790,7 +794,11 @@ impl Store {
                         Worktree {
                             root_name: worktree.root_name.clone(),
                             visible: worktree.visible,
-                            ..Default::default()
+                            abs_path: worktree.abs_path.clone(),
+                            entries: Default::default(),
+                            diagnostic_summaries: Default::default(),
+                            scan_id: Default::default(),
+                            is_complete: false,
                         },
                     );
                 }
@@ -942,6 +950,7 @@ impl Store {
         project_id: ProjectId,
         worktree_id: u64,
         worktree_root_name: &str,
+        worktree_abs_path: &[u8],
         removed_entries: &[u64],
         updated_entries: &[proto::Entry],
         scan_id: u64,
@@ -952,6 +961,7 @@ impl Store {
         let connection_ids = project.connection_ids();
         let mut worktree = project.worktrees.entry(worktree_id).or_default();
         worktree.root_name = worktree_root_name.to_string();
+        worktree.abs_path = worktree_abs_path.to_vec();
 
         for entry_id in removed_entries {
             worktree.entries.remove(entry_id);

crates/editor/src/items.rs 🔗

@@ -819,11 +819,20 @@ impl StatusItemView for CursorPosition {
 
 fn path_for_buffer<'a>(
     buffer: &ModelHandle<MultiBuffer>,
-    mut height: usize,
+    height: usize,
     include_filename: bool,
     cx: &'a AppContext,
 ) -> Option<Cow<'a, Path>> {
     let file = buffer.read(cx).as_singleton()?.read(cx).file()?;
+    path_for_file(file, height, include_filename, cx)
+}
+
+fn path_for_file<'a>(
+    file: &'a dyn language::File,
+    mut height: usize,
+    include_filename: bool,
+    cx: &'a AppContext,
+) -> Option<Cow<'a, Path>> {
     // Ensure we always render at least the filename.
     height += 1;
 
@@ -845,13 +854,82 @@ fn path_for_buffer<'a>(
         if include_filename {
             Some(full_path.into())
         } else {
-            Some(full_path.parent().unwrap().to_path_buf().into())
+            Some(full_path.parent()?.to_path_buf().into())
         }
     } else {
-        let mut path = file.path().strip_prefix(prefix).unwrap();
+        let mut path = file.path().strip_prefix(prefix).ok()?;
         if !include_filename {
-            path = path.parent().unwrap();
+            path = path.parent()?;
         }
         Some(path.into())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use gpui::MutableAppContext;
+    use std::{
+        path::{Path, PathBuf},
+        sync::Arc,
+    };
+
+    #[gpui::test]
+    fn test_path_for_file(cx: &mut MutableAppContext) {
+        let file = TestFile {
+            path: Path::new("").into(),
+            full_path: PathBuf::from(""),
+        };
+        assert_eq!(path_for_file(&file, 0, false, cx), None);
+    }
+
+    struct TestFile {
+        path: Arc<Path>,
+        full_path: PathBuf,
+    }
+
+    impl language::File for TestFile {
+        fn path(&self) -> &Arc<Path> {
+            &self.path
+        }
+
+        fn full_path(&self, _: &gpui::AppContext) -> PathBuf {
+            self.full_path.clone()
+        }
+
+        fn as_local(&self) -> Option<&dyn language::LocalFile> {
+            todo!()
+        }
+
+        fn mtime(&self) -> std::time::SystemTime {
+            todo!()
+        }
+
+        fn file_name<'a>(&'a self, _: &'a gpui::AppContext) -> &'a std::ffi::OsStr {
+            todo!()
+        }
+
+        fn is_deleted(&self) -> bool {
+            todo!()
+        }
+
+        fn save(
+            &self,
+            _: u64,
+            _: language::Rope,
+            _: clock::Global,
+            _: project::LineEnding,
+            _: &mut MutableAppContext,
+        ) -> gpui::Task<anyhow::Result<(clock::Global, String, std::time::SystemTime)>> {
+            todo!()
+        }
+
+        fn as_any(&self) -> &dyn std::any::Any {
+            todo!()
+        }
+
+        fn to_proto(&self) -> rpc::proto::File {
+            todo!()
+        }
+    }
+}

crates/project/src/worktree.rs 🔗

@@ -1179,6 +1179,10 @@ impl Snapshot {
         self.id
     }
 
+    pub fn abs_path(&self) -> &Arc<Path> {
+        &self.abs_path
+    }
+
     pub fn contains_entry(&self, entry_id: ProjectEntryId) -> bool {
         self.entries_by_id.get(&entry_id, &()).is_some()
     }
@@ -1370,10 +1374,6 @@ impl Snapshot {
 }
 
 impl LocalSnapshot {
-    pub fn abs_path(&self) -> &Arc<Path> {
-        &self.abs_path
-    }
-
     pub fn extension_counts(&self) -> &HashMap<OsString, usize> {
         &self.extension_counts
     }