Improve test generation and implement status propogation

Mikayla Maki and max created

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

Change summary

crates/collab/src/tests/integration_tests.rs            |  19 
crates/collab/src/tests/randomized_integration_tests.rs |  18 +
crates/fs/src/fs.rs                                     |  40 ++
crates/project/src/worktree.rs                          | 140 +++++++---
crates/project_panel/src/project_panel.rs               |  15 
5 files changed, 155 insertions(+), 77 deletions(-)

Detailed changes

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

@@ -2708,7 +2708,7 @@ 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(
+    client_a.fs.as_fake().set_status_for_repo_via_git_operation(
         Path::new("/dir/.git"),
         &[
             (&Path::new(A_TXT), GitFileStatus::Added),
@@ -2754,13 +2754,16 @@ 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),
-        ],
-    );
+    client_a
+        .fs
+        .as_fake()
+        .set_status_for_repo_via_working_copy_change(
+            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 🔗

@@ -815,6 +815,7 @@ async fn apply_client_operation(
             GitOperation::WriteGitStatuses {
                 repo_path,
                 statuses,
+                git_operation,
             } => {
                 if !client.fs.directories().contains(&repo_path) {
                     return Err(TestError::Inapplicable);
@@ -838,9 +839,16 @@ async fn apply_client_operation(
                     client.fs.create_dir(&dot_git_dir).await?;
                 }
 
-                client
-                    .fs
-                    .set_status_for_repo(&dot_git_dir, statuses.as_slice());
+                if git_operation {
+                    client
+                        .fs
+                        .set_status_for_repo_via_git_operation(&dot_git_dir, statuses.as_slice());
+                } else {
+                    client.fs.set_status_for_repo_via_working_copy_change(
+                        &dot_git_dir,
+                        statuses.as_slice(),
+                    );
+                }
             }
         },
     }
@@ -1229,6 +1237,7 @@ enum GitOperation {
     WriteGitStatuses {
         repo_path: PathBuf,
         statuses: Vec<(PathBuf, GitFileStatus)>,
+        git_operation: bool,
     },
 }
 
@@ -1854,9 +1863,12 @@ impl TestPlan {
                     })
                     .collect::<Vec<_>>();
 
+                let git_operation = self.rng.gen::<bool>();
+
                 GitOperation::WriteGitStatuses {
                     repo_path,
                     statuses,
+                    git_operation,
                 }
             }
             _ => unreachable!(),

crates/fs/src/fs.rs 🔗

@@ -619,7 +619,7 @@ impl FakeFs {
         .boxed()
     }
 
-    pub fn with_git_state<F>(&self, dot_git: &Path, f: F)
+    pub fn with_git_state<F>(&self, dot_git: &Path, emit_git_event: bool, f: F)
     where
         F: FnOnce(&mut FakeGitRepositoryState),
     {
@@ -633,18 +633,22 @@ impl FakeFs {
 
             f(&mut repo_state);
 
-            state.emit_event([dot_git]);
+            if emit_git_event {
+                state.emit_event([dot_git]);
+            }
         } else {
             panic!("not a directory");
         }
     }
 
     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))
+        self.with_git_state(dot_git, true, |state| {
+            state.branch_name = branch.map(Into::into)
+        })
     }
 
     pub fn set_index_for_repo(&self, dot_git: &Path, head_state: &[(&Path, String)]) {
-        self.with_git_state(dot_git, |state| {
+        self.with_git_state(dot_git, true, |state| {
             state.index_contents.clear();
             state.index_contents.extend(
                 head_state
@@ -654,8 +658,32 @@ impl FakeFs {
         });
     }
 
-    pub fn set_status_for_repo(&self, dot_git: &Path, statuses: &[(&Path, GitFileStatus)]) {
-        self.with_git_state(dot_git, |state| {
+    pub fn set_status_for_repo_via_working_copy_change(
+        &self,
+        dot_git: &Path,
+        statuses: &[(&Path, GitFileStatus)],
+    ) {
+        self.with_git_state(dot_git, false, |state| {
+            state.worktree_statuses.clear();
+            state.worktree_statuses.extend(
+                statuses
+                    .iter()
+                    .map(|(path, content)| ((**path).into(), content.clone())),
+            );
+        });
+        self.state.lock().emit_event(
+            statuses
+                .iter()
+                .map(|(path, _)| dot_git.parent().unwrap().join(path)),
+        );
+    }
+
+    pub fn set_status_for_repo_via_git_operation(
+        &self,
+        dot_git: &Path,
+        statuses: &[(&Path, GitFileStatus)],
+    ) {
+        self.with_git_state(dot_git, true, |state| {
             state.worktree_statuses.clear();
             state.worktree_statuses.extend(
                 statuses

crates/project/src/worktree.rs 🔗

@@ -1670,46 +1670,42 @@ impl Snapshot {
         })
     }
 
-    pub fn statuses_for_paths<'a>(
-        &self,
-        paths: impl IntoIterator<Item = &'a Path>,
-    ) -> Vec<Option<GitFileStatus>> {
+    /// Update the `git_status` of the given entries such that files'
+    /// statuses bubble up to their ancestor directories.
+    pub fn propagate_git_statuses(&self, result: &mut [Entry]) {
         let mut cursor = self
             .entries_by_path
             .cursor::<(TraversalProgress, GitStatuses)>();
-        let mut paths = paths.into_iter().peekable();
-        let mut path_stack = Vec::<(&Path, usize, GitStatuses)>::new();
-        let mut result = Vec::new();
+        let mut entry_stack = Vec::<(usize, GitStatuses)>::new();
 
+        let mut result_ix = 0;
         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();
+            let next_entry = result.get(result_ix);
+            let containing_entry = entry_stack.last().map(|(ix, _)| &result[*ix]);
+
+            let entry_to_finish = match (containing_entry, next_entry) {
+                (Some(_), None) => entry_stack.pop(),
+                (Some(containing_entry), Some(next_path)) => {
+                    if !next_path.path.starts_with(&containing_entry.path) {
+                        entry_stack.pop()
                     } else {
-                        entry_to_finish = path_stack.pop();
+                        None
                     }
                 }
-                (None, Some(_)) => path_to_start = paths.next(),
-                (Some(_), None) => entry_to_finish = path_stack.pop(),
+                (None, Some(_)) => None,
                 (None, None) => break,
-            }
+            };
 
-            if let Some((containing_path, result_ix, prev_statuses)) = entry_to_finish {
+            if let Some((entry_ix, prev_statuses)) = entry_to_finish {
                 cursor.seek_forward(
-                    &TraversalTarget::PathSuccessor(containing_path),
+                    &TraversalTarget::PathSuccessor(&result[entry_ix].path),
                     Bias::Left,
                     &(),
                 );
 
                 let statuses = cursor.start().1 - prev_statuses;
 
-                result[result_ix] = if statuses.conflict > 0 {
+                result[entry_ix].git_status = if statuses.conflict > 0 {
                     Some(GitFileStatus::Conflict)
                 } else if statuses.modified > 0 {
                     Some(GitFileStatus::Modified)
@@ -1718,15 +1714,18 @@ impl Snapshot {
                 } 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);
+            } else {
+                if result[result_ix].is_dir() {
+                    cursor.seek_forward(
+                        &TraversalTarget::Path(&result[result_ix].path),
+                        Bias::Left,
+                        &(),
+                    );
+                    entry_stack.push((result_ix, cursor.start().1));
+                }
+                result_ix += 1;
             }
         }
-
-        return result;
     }
 
     pub fn paths(&self) -> impl Iterator<Item = &Arc<Path>> {
@@ -5309,7 +5308,7 @@ mod tests {
         }
 
         #[gpui::test]
-        async fn test_statuses_for_paths(cx: &mut TestAppContext) {
+        async fn test_propagate_git_statuses(cx: &mut TestAppContext) {
             let fs = FakeFs::new(cx.background());
             fs.insert_tree(
                 "/root",
@@ -5338,7 +5337,7 @@ mod tests {
             )
             .await;
 
-            fs.set_status_for_repo(
+            fs.set_status_for_repo_via_git_operation(
                 &Path::new("/root/.git"),
                 &[
                     (Path::new("a/b/c1.txt"), GitFileStatus::Added),
@@ -5361,27 +5360,68 @@ mod tests {
             .unwrap();
 
             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"),
-                ]),
+            check_propagated_statuses(
+                &snapshot,
                 &[
-                    Some(GitFileStatus::Conflict),
-                    Some(GitFileStatus::Modified),
-                    Some(GitFileStatus::Added),
-                    Some(GitFileStatus::Modified),
-                    None,
-                    Some(GitFileStatus::Conflict),
-                ]
-            )
+                    (Path::new(""), Some(GitFileStatus::Conflict)),
+                    (Path::new("a"), Some(GitFileStatus::Modified)),
+                    (Path::new("a/b"), Some(GitFileStatus::Added)),
+                    (Path::new("a/b/c1.txt"), Some(GitFileStatus::Added)),
+                    (Path::new("a/b/c2.txt"), None),
+                    (Path::new("a/d"), Some(GitFileStatus::Modified)),
+                    (Path::new("a/d/e2.txt"), Some(GitFileStatus::Modified)),
+                    (Path::new("f"), None),
+                    (Path::new("f/no-status.txt"), None),
+                    (Path::new("g"), Some(GitFileStatus::Conflict)),
+                    (Path::new("g/h2.txt"), Some(GitFileStatus::Conflict)),
+                ],
+            );
+
+            check_propagated_statuses(
+                &snapshot,
+                &[
+                    (Path::new("a/b"), Some(GitFileStatus::Added)),
+                    (Path::new("a/b/c1.txt"), Some(GitFileStatus::Added)),
+                    (Path::new("a/b/c2.txt"), None),
+                    (Path::new("a/d"), Some(GitFileStatus::Modified)),
+                    (Path::new("a/d/e1.txt"), None),
+                    (Path::new("a/d/e2.txt"), Some(GitFileStatus::Modified)),
+                    (Path::new("f"), None),
+                    (Path::new("f/no-status.txt"), None),
+                    (Path::new("g"), Some(GitFileStatus::Conflict)),
+                ],
+            );
+
+            check_propagated_statuses(
+                &snapshot,
+                &[
+                    (Path::new("a/b/c1.txt"), Some(GitFileStatus::Added)),
+                    (Path::new("a/b/c2.txt"), None),
+                    (Path::new("a/d/e1.txt"), None),
+                    (Path::new("a/d/e2.txt"), Some(GitFileStatus::Modified)),
+                    (Path::new("f/no-status.txt"), None),
+                ],
+            );
+
+            fn check_propagated_statuses(
+                snapshot: &Snapshot,
+                expected_statuses: &[(&Path, Option<GitFileStatus>)],
+            ) {
+                let mut entries = expected_statuses
+                    .iter()
+                    .map(|(path, _)| snapshot.entry_for_path(path).unwrap().clone())
+                    .collect::<Vec<_>>();
+                snapshot.propagate_git_statuses(&mut entries);
+                assert_eq!(
+                    entries
+                        .iter()
+                        .map(|e| (e.path.as_ref(), e.git_status))
+                        .collect::<Vec<_>>(),
+                    expected_statuses
+                );
+            }
         }
 
         #[track_caller]

crates/project_panel/src/project_panel.rs 🔗

@@ -1012,6 +1012,9 @@ impl ProjectPanel {
                 }
                 entry_iter.advance();
             }
+
+            snapshot.propagate_git_statuses(&mut visible_worktree_entries);
+
             visible_worktree_entries.sort_by(|entry_a, entry_b| {
                 let mut components_a = entry_a.path.components().peekable();
                 let mut components_b = entry_b.path.components().peekable();
@@ -1109,16 +1112,8 @@ impl ProjectPanel {
                     .unwrap_or(&[]);
 
                 let entry_range = range.start.saturating_sub(ix)..end_ix - ix;
-                let statuses = worktree.read(cx).statuses_for_paths(
-                    visible_worktree_entries[entry_range.clone()]
-                        .iter()
-                        .map(|entry| entry.path.as_ref()),
-                );
-                for (entry, status) in visible_worktree_entries[entry_range]
-                    .iter()
-                    .zip(statuses.into_iter())
-                {
-                    let status = git_status_setting.then(|| status).flatten();
+                for entry in visible_worktree_entries[entry_range].iter() {
+                    let status = git_status_setting.then(|| entry.git_status).flatten();
 
                     let mut details = EntryDetails {
                         filename: entry