Fix ordering of multibuffer excerpts (#39476)

Cole Miller and Lukas Wirth created 1 month ago

The ordering of path-based excerpts in multibuffers regressed with
#38744, because we changed the `path` field of `PathKey` to be a string
(from `std::path::Path`) and used the derived `Ord` implementation,
which doesn't agree with the path-based order of worktree traversals.
This PR fixes that by using `RelPath` for `PathKey`. Instead of using
`File::full_path`, which can be absolute, we always use `File::path` and
distinguish different worktrees using their ID.

Release Notes:

- N/A

---------

Co-authored-by: Lukas Wirth <[e-mail]>

Change summary

crates/collab/src/tests/git_tests.rs          | 12 ++++++++-
crates/editor/src/editor_tests.rs             |  9 ++-----
crates/git_ui/src/commit_view.rs              |  8 +++---
crates/git_ui/src/project_diff.rs             | 21 +++++++++--------
crates/multi_buffer/src/multi_buffer.rs       | 25 ++++++++++++++------
crates/multi_buffer/src/multi_buffer_tests.rs |  9 ++++---
6 files changed, 50 insertions(+), 34 deletions(-)

Detailed changes

crates/collab/src/tests/git_tests.rs

@@ -84,7 +84,11 @@ async fn test_project_diff(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext)
     diff.update(cx_b, |diff, cx| {
         assert_eq!(
             diff.excerpt_paths(cx),
-            vec!["changed.txt", "deleted.txt", "created.txt"]
+            vec![
+                rel_path("changed.txt").into_arc(),
+                rel_path("deleted.txt").into_arc(),
+                rel_path("created.txt").into_arc()
+            ]
         );
     });
 
@@ -121,7 +125,11 @@ async fn test_project_diff(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext)
     diff.update(cx_b, |diff, cx| {
         assert_eq!(
             diff.excerpt_paths(cx),
-            vec!["deleted.txt", "unchanged.txt", "created.txt"]
+            vec![
+                rel_path("deleted.txt").into_arc(),
+                rel_path("unchanged.txt").into_arc(),
+                rel_path("created.txt").into_arc()
+            ]
         );
     });
 }

crates/editor/src/editor_tests.rs

@@ -16464,7 +16464,7 @@ async fn test_following_with_multiple_excerpts(cx: &mut TestAppContext) {
     leader.update(cx, |leader, cx| {
         leader.buffer.update(cx, |multibuffer, cx| {
             multibuffer.set_excerpts_for_path(
-                PathKey::namespaced(1, "b.txt".into()),
+                PathKey::namespaced(1, rel_path("b.txt").into_arc()),
                 buffer_1.clone(),
                 vec![
                     Point::row_range(0..3),
@@ -16475,7 +16475,7 @@ async fn test_following_with_multiple_excerpts(cx: &mut TestAppContext) {
                 cx,
             );
             multibuffer.set_excerpts_for_path(
-                PathKey::namespaced(1, "a.txt".into()),
+                PathKey::namespaced(1, rel_path("a.txt").into_arc()),
                 buffer_2.clone(),
                 vec![Point::row_range(0..6), Point::row_range(8..12)],
                 0,
@@ -20974,10 +20974,7 @@ async fn test_display_diff_hunks(cx: &mut TestAppContext) {
         for buffer in &buffers {
             let snapshot = buffer.read(cx).snapshot();
             multibuffer.set_excerpts_for_path(
-                PathKey::namespaced(
-                    0,
-                    buffer.read(cx).file().unwrap().path().as_unix_str().into(),
-                ),
+                PathKey::namespaced(0, buffer.read(cx).file().unwrap().path().clone()),
                 buffer.clone(),
                 vec![text::Anchor::MIN.to_point(&snapshot)..text::Anchor::MAX.to_point(&snapshot)],
                 2,

crates/git_ui/src/commit_view.rs

@@ -43,8 +43,8 @@ struct CommitMetadataFile {
     worktree_id: WorktreeId,
 }
 
-const COMMIT_METADATA_NAMESPACE: u32 = 0;
-const FILE_NAMESPACE: u32 = 1;
+const COMMIT_METADATA_NAMESPACE: u64 = 0;
+const FILE_NAMESPACE: u64 = 1;
 
 impl CommitView {
     pub fn open(
@@ -145,7 +145,7 @@ impl CommitView {
             });
             multibuffer.update(cx, |multibuffer, cx| {
                 multibuffer.set_excerpts_for_path(
-                    PathKey::namespaced(COMMIT_METADATA_NAMESPACE, file.title.as_unix_str().into()),
+                    PathKey::namespaced(COMMIT_METADATA_NAMESPACE, file.title.clone()),
                     buffer.clone(),
                     vec![Point::zero()..buffer.read(cx).max_point()],
                     0,
@@ -193,7 +193,7 @@ impl CommitView {
                             .collect::<Vec<_>>();
                         let path = snapshot.file().unwrap().path().clone();
                         let _is_newly_added = multibuffer.set_excerpts_for_path(
-                            PathKey::namespaced(FILE_NAMESPACE, path.as_unix_str().into()),
+                            PathKey::namespaced(FILE_NAMESPACE, path),
                             buffer,
                             diff_hunk_ranges,
                             multibuffer_context_lines(cx),

crates/git_ui/src/project_diff.rs

@@ -73,9 +73,9 @@ struct DiffBuffer {
     file_status: FileStatus,
 }
 
-const CONFLICT_NAMESPACE: u32 = 1;
-const TRACKED_NAMESPACE: u32 = 2;
-const NEW_NAMESPACE: u32 = 3;
+const CONFLICT_NAMESPACE: u64 = 1;
+const TRACKED_NAMESPACE: u64 = 2;
+const NEW_NAMESPACE: u64 = 3;
 
 impl ProjectDiff {
     pub(crate) fn register(workspace: &mut Workspace, cx: &mut Context<Workspace>) {
@@ -243,7 +243,7 @@ impl ProjectDiff {
             TRACKED_NAMESPACE
         };
 
-        let path_key = PathKey::namespaced(namespace, entry.repo_path.as_unix_str().into());
+        let path_key = PathKey::namespaced(namespace, entry.repo_path.0);
 
         self.move_to_path(path_key, window, cx)
     }
@@ -397,7 +397,7 @@ impl ProjectDiff {
                 } else {
                     TRACKED_NAMESPACE
                 };
-                let path_key = PathKey::namespaced(namespace, entry.repo_path.as_unix_str().into());
+                let path_key = PathKey::namespaced(namespace, entry.repo_path.0.clone());
 
                 previous_paths.remove(&path_key);
                 let load_buffer = self
@@ -531,11 +531,12 @@ impl ProjectDiff {
     }
 
     #[cfg(any(test, feature = "test-support"))]
-    pub fn excerpt_paths(&self, cx: &App) -> Vec<String> {
+    pub fn excerpt_paths(&self, cx: &App) -> Vec<std::sync::Arc<util::rel_path::RelPath>> {
         self.multibuffer
             .read(cx)
             .excerpt_paths()
-            .map(|key| key.path().to_string())
+            .map(|key| key.path())
+            .cloned()
             .collect()
     }
 }
@@ -1361,7 +1362,7 @@ mod tests {
     use settings::SettingsStore;
     use std::path::Path;
     use unindent::Unindent as _;
-    use util::path;
+    use util::{path, rel_path::rel_path};
 
     use super::*;
 
@@ -1467,7 +1468,7 @@ mod tests {
 
         let editor = cx.update_window_entity(&diff, |diff, window, cx| {
             diff.move_to_path(
-                PathKey::namespaced(TRACKED_NAMESPACE, "foo".into()),
+                PathKey::namespaced(TRACKED_NAMESPACE, rel_path("foo").into_arc()),
                 window,
                 cx,
             );
@@ -1488,7 +1489,7 @@ mod tests {
 
         let editor = cx.update_window_entity(&diff, |diff, window, cx| {
             diff.move_to_path(
-                PathKey::namespaced(TRACKED_NAMESPACE, "bar".into()),
+                PathKey::namespaced(TRACKED_NAMESPACE, rel_path("bar").into_arc()),
                 window,
                 cx,
             );

crates/multi_buffer/src/multi_buffer.rs

@@ -49,7 +49,7 @@ use text::{
     subscription::{Subscription, Topic},
 };
 use theme::SyntaxTheme;
-use util::post_inc;
+use util::{post_inc, rel_path::RelPath};
 
 const NEWLINES: &[u8] = &[b'\n'; u8::MAX as usize];
 
@@ -161,24 +161,33 @@ impl MultiBufferDiffHunk {
 
 #[derive(PartialEq, Eq, Ord, PartialOrd, Clone, Hash, Debug)]
 pub struct PathKey {
-    namespace: u32,
-    path: Arc<str>,
+    namespace: Option<u64>,
+    path: Arc<RelPath>,
 }
 
 impl PathKey {
-    pub fn namespaced(namespace: u32, path: Arc<str>) -> Self {
-        Self { namespace, path }
+    pub fn namespaced(namespace: u64, path: Arc<RelPath>) -> Self {
+        Self {
+            namespace: Some(namespace),
+            path,
+        }
     }
 
     pub fn for_buffer(buffer: &Entity<Buffer>, cx: &App) -> Self {
         if let Some(file) = buffer.read(cx).file() {
-            Self::namespaced(1, file.full_path(cx).to_string_lossy().into_owned().into())
+            Self::namespaced(file.worktree_id(cx).to_proto(), file.path().clone())
         } else {
-            Self::namespaced(0, buffer.entity_id().to_string().into())
+            Self {
+                namespace: None,
+                path: RelPath::unix(&buffer.entity_id().to_string())
+                    .unwrap()
+                    .into_arc(),
+            }
         }
     }
 
-    pub fn path(&self) -> &Arc<str> {
+    #[cfg(any(test, feature = "test-support"))]
+    pub fn path(&self) -> &Arc<RelPath> {
         &self.path
     }
 }

crates/multi_buffer/src/multi_buffer_tests.rs

@@ -8,6 +8,7 @@ use rand::prelude::*;
 use settings::SettingsStore;
 use std::env;
 use util::RandomCharIter;
+use util::rel_path::rel_path;
 use util::test::sample_text;
 
 #[ctor::ctor]
@@ -1524,7 +1525,7 @@ fn test_set_excerpts_for_buffer_ordering(cx: &mut TestAppContext) {
             cx,
         )
     });
-    let path1: PathKey = PathKey::namespaced(0, "/".into());
+    let path1: PathKey = PathKey::namespaced(0, rel_path("root").into_arc());
 
     let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));
     multibuffer.update(cx, |multibuffer, cx| {
@@ -1619,7 +1620,7 @@ fn test_set_excerpts_for_buffer(cx: &mut TestAppContext) {
             cx,
         )
     });
-    let path1: PathKey = PathKey::namespaced(0, "/".into());
+    let path1: PathKey = PathKey::namespaced(0, rel_path("root").into_arc());
     let buf2 = cx.new(|cx| {
         Buffer::local(
             indoc! {
@@ -1638,7 +1639,7 @@ fn test_set_excerpts_for_buffer(cx: &mut TestAppContext) {
             cx,
         )
     });
-    let path2 = PathKey::namespaced(1, "/".into());
+    let path2 = PathKey::namespaced(1, rel_path("root").into_arc());
 
     let multibuffer = cx.new(|_| MultiBuffer::new(Capability::ReadWrite));
     multibuffer.update(cx, |multibuffer, cx| {
@@ -1815,7 +1816,7 @@ fn test_set_excerpts_for_buffer_rename(cx: &mut TestAppContext) {
             cx,
         )
     });
-    let path: PathKey = PathKey::namespaced(0, "/".into());
+    let path: PathKey = PathKey::namespaced(0, rel_path("root").into_arc());
     let buf2 = cx.new(|cx| {
         Buffer::local(
             indoc! {