From a2d58068a7f52f3a81b2e2a4a22bf02a19f084d9 Mon Sep 17 00:00:00 2001 From: Mikayla Maki Date: Mon, 5 Jun 2023 17:30:12 -0700 Subject: [PATCH] Improve test generation and implement status propogation co-authored-by: max --- crates/collab/src/tests/integration_tests.rs | 19 ++- .../src/tests/randomized_integration_tests.rs | 18 ++- crates/fs/src/fs.rs | 40 ++++- crates/project/src/worktree.rs | 138 +++++++++++------- crates/project_panel/src/project_panel.rs | 15 +- 5 files changed, 154 insertions(+), 76 deletions(-) diff --git a/crates/collab/src/tests/integration_tests.rs b/crates/collab/src/tests/integration_tests.rs index f8504c1905..889f5bf77d 100644 --- a/crates/collab/src/tests/integration_tests.rs +++ b/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(); diff --git a/crates/collab/src/tests/randomized_integration_tests.rs b/crates/collab/src/tests/randomized_integration_tests.rs index b1fd05f01e..7a46d4c3f6 100644 --- a/crates/collab/src/tests/randomized_integration_tests.rs +++ b/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::>(); + let git_operation = self.rng.gen::(); + GitOperation::WriteGitStatuses { repo_path, statuses, + git_operation, } } _ => unreachable!(), diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 77878b26a2..e8b309242d 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -619,7 +619,7 @@ impl FakeFs { .boxed() } - pub fn with_git_state(&self, dot_git: &Path, f: F) + pub fn with_git_state(&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>) { - 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 diff --git a/crates/project/src/worktree.rs b/crates/project/src/worktree.rs index d808c41350..27dc890ec0 100644 --- a/crates/project/src/worktree.rs +++ b/crates/project/src/worktree.rs @@ -1670,46 +1670,42 @@ impl Snapshot { }) } - pub fn statuses_for_paths<'a>( - &self, - paths: impl IntoIterator, - ) -> Vec> { + /// 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 next_entry = result.get(result_ix); + let containing_entry = entry_stack.last().map(|(ix, _)| &result[*ix]); - 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 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> { @@ -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)], + ) { + let mut entries = expected_statuses + .iter() + .map(|(path, _)| snapshot.entry_for_path(path).unwrap().clone()) + .collect::>(); + snapshot.propagate_git_statuses(&mut entries); + assert_eq!( + entries + .iter() + .map(|e| (e.path.as_ref(), e.git_status)) + .collect::>(), + expected_statuses + ); + } } #[track_caller] diff --git a/crates/project_panel/src/project_panel.rs b/crates/project_panel/src/project_panel.rs index 3d7d32cef4..9563d54be8 100644 --- a/crates/project_panel/src/project_panel.rs +++ b/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