Remove stale comments

Mikayla Maki and max created

Implement status bubbling query with sum tree traversals

co-authored-by: max <max@zed.dev>

Change summary

crates/collab/src/tests/integration_tests.rs            |  90 +--
crates/collab/src/tests/randomized_integration_tests.rs |   7 
crates/fs/src/fs.rs                                     |   6 
crates/project/src/worktree.rs                          | 216 +++++++++-
crates/rpc/src/proto.rs                                 |   5 
5 files changed, 228 insertions(+), 96 deletions(-)

Detailed changes

crates/collab/src/tests/integration_tests.rs 🔗

@@ -2415,14 +2415,10 @@ async fn test_git_diff_base_change(
     "
     .unindent();
 
-    client_a
-        .fs
-        .as_fake()
-        .set_index_for_repo(
-            Path::new("/dir/.git"),
-            &[(Path::new("a.txt"), diff_base.clone())],
-        )
-        .await;
+    client_a.fs.as_fake().set_index_for_repo(
+        Path::new("/dir/.git"),
+        &[(Path::new("a.txt"), diff_base.clone())],
+    );
 
     // Create the buffer
     let buffer_local_a = project_local
@@ -2464,14 +2460,10 @@ async fn test_git_diff_base_change(
         );
     });
 
-    client_a
-        .fs
-        .as_fake()
-        .set_index_for_repo(
-            Path::new("/dir/.git"),
-            &[(Path::new("a.txt"), new_diff_base.clone())],
-        )
-        .await;
+    client_a.fs.as_fake().set_index_for_repo(
+        Path::new("/dir/.git"),
+        &[(Path::new("a.txt"), new_diff_base.clone())],
+    );
 
     // Wait for buffer_local_a to receive it
     deterministic.run_until_parked();
@@ -2513,14 +2505,10 @@ async fn test_git_diff_base_change(
     "
     .unindent();
 
-    client_a
-        .fs
-        .as_fake()
-        .set_index_for_repo(
-            Path::new("/dir/sub/.git"),
-            &[(Path::new("b.txt"), diff_base.clone())],
-        )
-        .await;
+    client_a.fs.as_fake().set_index_for_repo(
+        Path::new("/dir/sub/.git"),
+        &[(Path::new("b.txt"), diff_base.clone())],
+    );
 
     // Create the buffer
     let buffer_local_b = project_local
@@ -2562,14 +2550,10 @@ async fn test_git_diff_base_change(
         );
     });
 
-    client_a
-        .fs
-        .as_fake()
-        .set_index_for_repo(
-            Path::new("/dir/sub/.git"),
-            &[(Path::new("b.txt"), new_diff_base.clone())],
-        )
-        .await;
+    client_a.fs.as_fake().set_index_for_repo(
+        Path::new("/dir/sub/.git"),
+        &[(Path::new("b.txt"), new_diff_base.clone())],
+    );
 
     // Wait for buffer_local_b to receive it
     deterministic.run_until_parked();
@@ -2646,8 +2630,7 @@ async fn test_git_branch_name(
     client_a
         .fs
         .as_fake()
-        .set_branch_name(Path::new("/dir/.git"), Some("branch-1"))
-        .await;
+        .set_branch_name(Path::new("/dir/.git"), Some("branch-1"));
 
     // Wait for it to catch up to the new branch
     deterministic.run_until_parked();
@@ -2673,8 +2656,7 @@ async fn test_git_branch_name(
     client_a
         .fs
         .as_fake()
-        .set_branch_name(Path::new("/dir/.git"), Some("branch-2"))
-        .await;
+        .set_branch_name(Path::new("/dir/.git"), Some("branch-2"));
 
     // Wait for buffer_local_a to receive it
     deterministic.run_until_parked();
@@ -2726,17 +2708,13 @@ async fn test_git_status_sync(
     const A_TXT: &'static str = "a.txt";
     const B_TXT: &'static str = "b.txt";
 
-    client_a
-        .fs
-        .as_fake()
-        .set_status_for_repo(
-            Path::new("/dir/.git"),
-            &[
-                (&Path::new(A_TXT), GitFileStatus::Added),
-                (&Path::new(B_TXT), GitFileStatus::Added),
-            ],
-        )
-        .await;
+    client_a.fs.as_fake().set_status_for_repo(
+        Path::new("/dir/.git"),
+        &[
+            (&Path::new(A_TXT), GitFileStatus::Added),
+            (&Path::new(B_TXT), GitFileStatus::Added),
+        ],
+    );
 
     let (project_local, _worktree_id) = client_a.build_local_project("/dir", cx_a).await;
     let project_id = active_call_a
@@ -2776,17 +2754,13 @@ async fn test_git_status_sync(
         assert_status(&Path::new(B_TXT), Some(GitFileStatus::Added), project, cx);
     });
 
-    client_a
-        .fs
-        .as_fake()
-        .set_status_for_repo(
-            Path::new("/dir/.git"),
-            &[
-                (&Path::new(A_TXT), GitFileStatus::Modified),
-                (&Path::new(B_TXT), GitFileStatus::Modified),
-            ],
-        )
-        .await;
+    client_a.fs.as_fake().set_status_for_repo(
+        Path::new("/dir/.git"),
+        &[
+            (&Path::new(A_TXT), GitFileStatus::Modified),
+            (&Path::new(B_TXT), GitFileStatus::Modified),
+        ],
+    );
 
     // Wait for buffer_local_a to receive it
     deterministic.run_until_parked();

crates/collab/src/tests/randomized_integration_tests.rs 🔗

@@ -789,7 +789,7 @@ async fn apply_client_operation(
                 if client.fs.metadata(&dot_git_dir).await?.is_none() {
                     client.fs.create_dir(&dot_git_dir).await?;
                 }
-                client.fs.set_index_for_repo(&dot_git_dir, &contents).await;
+                client.fs.set_index_for_repo(&dot_git_dir, &contents);
             }
             GitOperation::WriteGitBranch {
                 repo_path,
@@ -810,7 +810,7 @@ async fn apply_client_operation(
                 if client.fs.metadata(&dot_git_dir).await?.is_none() {
                     client.fs.create_dir(&dot_git_dir).await?;
                 }
-                client.fs.set_branch_name(&dot_git_dir, new_branch).await;
+                client.fs.set_branch_name(&dot_git_dir, new_branch);
             }
             GitOperation::WriteGitStatuses {
                 repo_path,
@@ -840,8 +840,7 @@ async fn apply_client_operation(
 
                 client
                     .fs
-                    .set_status_for_repo(&dot_git_dir, statuses.as_slice())
-                    .await;
+                    .set_status_for_repo(&dot_git_dir, statuses.as_slice());
             }
         },
     }

crates/fs/src/fs.rs 🔗

@@ -639,11 +639,11 @@ impl FakeFs {
         }
     }
 
-    pub async fn set_branch_name(&self, dot_git: &Path, branch: Option<impl Into<String>>) {
+    pub fn set_branch_name(&self, dot_git: &Path, branch: Option<impl Into<String>>) {
         self.with_git_state(dot_git, |state| state.branch_name = branch.map(Into::into))
     }
 
-    pub async fn set_index_for_repo(&self, dot_git: &Path, head_state: &[(&Path, String)]) {
+    pub fn set_index_for_repo(&self, dot_git: &Path, head_state: &[(&Path, String)]) {
         self.with_git_state(dot_git, |state| {
             state.index_contents.clear();
             state.index_contents.extend(
@@ -654,7 +654,7 @@ impl FakeFs {
         });
     }
 
-    pub async fn set_status_for_repo(&self, dot_git: &Path, statuses: &[(&Path, GitFileStatus)]) {
+    pub fn set_status_for_repo(&self, dot_git: &Path, statuses: &[(&Path, GitFileStatus)]) {
         self.with_git_state(dot_git, |state| {
             state.worktree_statuses.clear();
             state.worktree_statuses.extend(

crates/project/src/worktree.rs 🔗

@@ -36,7 +36,6 @@ use postage::{
     prelude::{Sink as _, Stream as _},
     watch,
 };
-use sha2::digest::typenum::private::IsLessPrivate;
 use smol::channel::{self, Sender};
 use std::{
     any::Any,
@@ -46,7 +45,7 @@ use std::{
     fmt,
     future::Future,
     mem,
-    ops::{Deref, DerefMut},
+    ops::{AddAssign, Deref, DerefMut, Sub},
     path::{Path, PathBuf},
     pin::Pin,
     sync::{
@@ -1455,7 +1454,7 @@ impl Snapshot {
     fn delete_entry(&mut self, entry_id: ProjectEntryId) -> Option<Arc<Path>> {
         let removed_entry = self.entries_by_id.remove(&entry_id, &())?;
         self.entries_by_path = {
-            let mut cursor = self.entries_by_path.cursor();
+            let mut cursor = self.entries_by_path.cursor::<TraversalProgress>();
             let mut new_entries_by_path =
                 cursor.slice(&TraversalTarget::Path(&removed_entry.path), Bias::Left, &());
             while let Some(entry) = cursor.item() {
@@ -1671,16 +1670,60 @@ impl Snapshot {
         })
     }
 
-    pub fn statuses_for_directories(&self, paths: &[&Path]) -> Vec<GitFileStatus> {
-        todo!();
-        // ["/a/b", "a/b/c", "a/b/d", "j"]
+    pub fn statuses_for_paths(&self, paths: &[&Path]) -> Vec<Option<GitFileStatus>> {
+        let mut cursor = self
+            .entries_by_path
+            .cursor::<(TraversalProgress, GitStatuses)>();
+        let mut paths = paths.iter().peekable();
+        let mut path_stack = Vec::<(&Path, usize, GitStatuses)>::new();
+        let mut result = Vec::new();
+
+        loop {
+            let next_path = paths.peek();
+            let containing_path = path_stack.last();
+
+            let mut entry_to_finish = None;
+            let mut path_to_start = None;
+            match (containing_path, next_path) {
+                (Some((containing_path, _, _)), Some(next_path)) => {
+                    if next_path.starts_with(containing_path) {
+                        path_to_start = paths.next();
+                    } else {
+                        entry_to_finish = path_stack.pop();
+                    }
+                }
+                (None, Some(_)) => path_to_start = paths.next(),
+                (Some(_), None) => entry_to_finish = path_stack.pop(),
+                (None, None) => break,
+            }
+
+            if let Some((containing_path, result_ix, prev_statuses)) = entry_to_finish {
+                cursor.seek_forward(
+                    &TraversalTarget::PathSuccessor(containing_path),
+                    Bias::Left,
+                    &(),
+                );
+
+                let statuses = cursor.start().1 - prev_statuses;
 
-        // Path stack:
-        // If path has descendents following it, push to stack: ["a/b"]
-        // Figure out a/b/c
-        // Figure out a/b/d
-        // Once no more descendants, pop the stack:
-        // Figure out a/b
+                result[result_ix] = if statuses.conflict > 0 {
+                    Some(GitFileStatus::Conflict)
+                } else if statuses.modified > 0 {
+                    Some(GitFileStatus::Modified)
+                } else if statuses.added > 0 {
+                    Some(GitFileStatus::Added)
+                } else {
+                    None
+                };
+            }
+            if let Some(path_to_start) = path_to_start {
+                cursor.seek_forward(&TraversalTarget::Path(path_to_start), Bias::Left, &());
+                path_stack.push((path_to_start, result.len(), cursor.start().1));
+                result.push(None);
+            }
+        }
+
+        return result;
     }
 
     pub fn paths(&self) -> impl Iterator<Item = &Arc<Path>> {
@@ -1954,8 +1997,6 @@ impl LocalSnapshot {
                 },
             );
 
-            self.scan_statuses(repo_lock.deref(), &work_directory, &work_directory.0);
-
             drop(repo_lock);
 
             self.git_repositories.insert(
@@ -2492,12 +2533,23 @@ impl sum_tree::Item for Entry {
             visible_file_count = 0;
         }
 
+        let mut statuses = GitStatuses::default();
+        match self.git_status {
+            Some(status) => match status {
+                GitFileStatus::Added => statuses.added = 1,
+                GitFileStatus::Modified => statuses.modified = 1,
+                GitFileStatus::Conflict => statuses.conflict = 1,
+            },
+            None => {}
+        }
+
         EntrySummary {
             max_path: self.path.clone(),
             count: 1,
             visible_count,
             file_count,
             visible_file_count,
+            statuses,
         }
     }
 }
@@ -2517,9 +2569,7 @@ pub struct EntrySummary {
     visible_count: usize,
     file_count: usize,
     visible_file_count: usize,
-    // git_modified_count: usize,
-    // git_added_count: usize,
-    // git_conflict_count: usize,
+    statuses: GitStatuses,
 }
 
 impl Default for EntrySummary {
@@ -2530,6 +2580,7 @@ impl Default for EntrySummary {
             visible_count: 0,
             file_count: 0,
             visible_file_count: 0,
+            statuses: Default::default(),
         }
     }
 }
@@ -2543,6 +2594,7 @@ impl sum_tree::Summary for EntrySummary {
         self.visible_count += rhs.visible_count;
         self.file_count += rhs.file_count;
         self.visible_file_count += rhs.visible_file_count;
+        self.statuses += rhs.statuses;
     }
 }
 
@@ -2761,7 +2813,7 @@ impl BackgroundScanner {
 
             if let Some(paths) = paths {
                 for path in paths {
-                    self.reload_repo_for_file_path(&path, &mut *snapshot, self.fs.as_ref());
+                    self.reload_git_status(&path, &mut *snapshot, self.fs.as_ref());
                 }
             }
 
@@ -3131,7 +3183,7 @@ impl BackgroundScanner {
         Some(())
     }
 
-    fn reload_repo_for_file_path(
+    fn reload_git_status(
         &self,
         path: &Path,
         snapshot: &mut LocalSnapshot,
@@ -3158,12 +3210,6 @@ impl BackgroundScanner {
 
                 (*entry_id, repo.repo_ptr.to_owned())
             };
-            /*
-            1. Populate dir, initializes the git repo
-            2. Sometimes, we get a file event inside the .git repo, before it's initializaed
-            In both cases, we should end up with an initialized repo and a full status scan
-
-            */
 
             let work_dir = snapshot
                 .entry_for_id(entry_id)
@@ -3575,6 +3621,39 @@ impl<'a> Default for TraversalProgress<'a> {
     }
 }
 
+#[derive(Clone, Debug, Default, Copy)]
+struct GitStatuses {
+    added: usize,
+    modified: usize,
+    conflict: usize,
+}
+
+impl AddAssign for GitStatuses {
+    fn add_assign(&mut self, rhs: Self) {
+        self.added += rhs.added;
+        self.modified += rhs.modified;
+        self.conflict += rhs.conflict;
+    }
+}
+
+impl Sub for GitStatuses {
+    type Output = GitStatuses;
+
+    fn sub(self, rhs: Self) -> Self::Output {
+        GitStatuses {
+            added: self.added - rhs.added,
+            modified: self.modified - rhs.modified,
+            conflict: self.conflict - rhs.conflict,
+        }
+    }
+}
+
+impl<'a> sum_tree::Dimension<'a, EntrySummary> for GitStatuses {
+    fn add_summary(&mut self, summary: &'a EntrySummary, _: &()) {
+        *self += summary.statuses
+    }
+}
+
 pub struct Traversal<'a> {
     cursor: sum_tree::Cursor<'a, Entry, TraversalProgress<'a>>,
     include_ignored: bool,
@@ -3676,6 +3755,14 @@ impl<'a, 'b> SeekTarget<'a, EntrySummary, TraversalProgress<'a>> for TraversalTa
     }
 }
 
+impl<'a, 'b> SeekTarget<'a, EntrySummary, (TraversalProgress<'a>, GitStatuses)>
+    for TraversalTarget<'b>
+{
+    fn cmp(&self, cursor_location: &(TraversalProgress<'a>, GitStatuses), _: &()) -> Ordering {
+        self.cmp(&cursor_location.0, &())
+    }
+}
+
 struct ChildEntriesIter<'a> {
     parent_path: &'a Path,
     traversal: Traversal<'a>,
@@ -4962,7 +5049,6 @@ mod tests {
             });
         }
 
-        // TODO: Stream statuses UPDATE THIS TO CHECK BUBBLIBG BEHAVIOR
         #[gpui::test]
         async fn test_git_status(deterministic: Arc<Deterministic>, cx: &mut TestAppContext) {
             const IGNORE_RULE: &'static str = "**/target";
@@ -5154,6 +5240,84 @@ mod tests {
             });
         }
 
+        #[gpui::test]
+        async fn test_statuses_for_paths(cx: &mut TestAppContext) {
+            let fs = FakeFs::new(cx.background());
+            fs.insert_tree(
+                "/root",
+                json!({
+                    ".git": {},
+                    "a": {
+                        "b": {
+                            "c1.txt": "",
+                            "c2.txt": "",
+                        },
+                        "d": {
+                            "e1.txt": "",
+                            "e2.txt": "",
+                            "e3.txt": "",
+                        }
+                    },
+                    "f": {
+                        "no-status.txt": ""
+                    },
+                    "g": {
+                        "h1.txt": "",
+                        "h2.txt": ""
+                    },
+
+                }),
+            )
+            .await;
+
+            let http_client = FakeHttpClient::with_404_response();
+            let client = cx.read(|cx| Client::new(http_client, cx));
+            let tree = Worktree::local(
+                client,
+                Path::new("/root"),
+                true,
+                fs.clone(),
+                Default::default(),
+                &mut cx.to_async(),
+            )
+            .await
+            .unwrap();
+
+            cx.foreground().run_until_parked();
+
+            fs.set_status_for_repo(
+                &Path::new("/root/.git"),
+                &[
+                    (Path::new("a/b/c1.txt"), GitFileStatus::Added),
+                    (Path::new("a/d/e2.txt"), GitFileStatus::Modified),
+                    (Path::new("g/h2.txt"), GitFileStatus::Conflict),
+                ],
+            );
+
+            cx.foreground().run_until_parked();
+
+            let snapshot = tree.read_with(cx, |tree, _| tree.snapshot());
+
+            assert_eq!(
+                snapshot.statuses_for_paths(&[
+                    Path::new(""),
+                    Path::new("a"),
+                    Path::new("a/b"),
+                    Path::new("a/d"),
+                    Path::new("f"),
+                    Path::new("g"),
+                ]),
+                &[
+                    Some(GitFileStatus::Conflict),
+                    Some(GitFileStatus::Modified),
+                    Some(GitFileStatus::Added),
+                    Some(GitFileStatus::Modified),
+                    None,
+                    Some(GitFileStatus::Conflict),
+                ]
+            )
+        }
+
         #[track_caller]
         fn git_init(path: &Path) -> git2::Repository {
             git2::Repository::init(path).expect("Failed to initialize git repository")

crates/rpc/src/proto.rs 🔗

@@ -497,11 +497,6 @@ pub fn split_worktree_update(
         .map(|repo| (repo.work_directory_id, repo))
         .collect::<HashMap<_, _>>();
 
-    // Maintain a list of inflight repositories
-    // Every time we send a repository's work directory, stick it in the list of in-flight repositories
-    // Every go of the loop, drain each in-flight repository's statuses
-    // Until we have no more data
-
     iter::from_fn(move || {
         if done_files {
             return None;