diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index 791e0f61117d0e84c86f1849af079a664206fb4b..ad6aa7ec3c6f9a129495e437bd379b023ec7cf61 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -8,15 +8,15 @@ use git::{ use git_ui::{commit_tooltip::CommitAvatar, commit_view::CommitView}; use gpui::{ AnyElement, App, Bounds, ClickEvent, ClipboardItem, Context, Corner, DefiniteLength, - DragMoveEvent, ElementId, Entity, EventEmitter, FocusHandle, Focusable, FontWeight, Hsla, - InteractiveElement, ParentElement, PathBuilder, Pixels, Point, Render, ScrollStrategy, + DragMoveEvent, ElementId, Empty, Entity, EventEmitter, FocusHandle, Focusable, FontWeight, + Hsla, InteractiveElement, ParentElement, PathBuilder, Pixels, Point, Render, ScrollStrategy, ScrollWheelEvent, SharedString, Styled, Subscription, Task, WeakEntity, Window, actions, anchored, deferred, point, px, }; use menu::{SelectNext, SelectPrevious}; use project::{ Project, - git_store::{CommitDataState, GitStoreEvent, Repository, RepositoryEvent}, + git_store::{CommitDataState, GitStoreEvent, Repository, RepositoryEvent, RepositoryId}, }; use settings::Settings; use smallvec::{SmallVec, smallvec}; @@ -541,6 +541,8 @@ impl GraphData { color_idx: commit_color.0 as usize, })); } + + self.max_commit_count = AllCommitCount::Loaded(self.commits.len()); } } @@ -645,8 +647,8 @@ pub struct GitGraph { log_order: LogOrder, selected_commit_diff: Option, _commit_diff_task: Option>, - _load_task: Option>, commit_details_split_state: Entity, + selected_repo_id: Option, } impl GitGraph { @@ -672,36 +674,38 @@ impl GitGraph { let git_store = project.read(cx).git_store().clone(); let accent_colors = cx.theme().accents(); - let mut graph = GraphData::new(accent_colors_count(accent_colors)); + let graph = GraphData::new(accent_colors_count(accent_colors)); let log_source = LogSource::default(); let log_order = LogOrder::default(); cx.subscribe(&git_store, |this, _, event, cx| match event { - GitStoreEvent::RepositoryUpdated(_, repo_event, is_active) => { - if *is_active { - if let Some(repository) = this.project.read(cx).active_repository(cx) { + GitStoreEvent::RepositoryUpdated(updated_repo_id, repo_event, _) => { + if this + .selected_repo_id + .as_ref() + .is_some_and(|repo_id| repo_id == updated_repo_id) + { + if let Some(repository) = this.get_selected_repository(cx) { this.on_repository_event(repository, repo_event, cx); } } } - GitStoreEvent::ActiveRepositoryChanged(_) => { - this.graph_data.clear(); - cx.notify(); + GitStoreEvent::ActiveRepositoryChanged(changed_repo_id) => { + // todo(git_graph): Make this selectable from UI so we don't have to always use active repository + if this.selected_repo_id != *changed_repo_id { + this.selected_repo_id = *changed_repo_id; + this.graph_data.clear(); + cx.notify(); + } } _ => {} }) .detach(); - if let Some(repository) = project.read(cx).active_repository(cx) { - repository.update(cx, |repository, cx| { - // This won't overlap with loading commits from the repository because - // we either have all commits or commits loaded in chunks and loading commits - // from the repository event is always adding the last chunk of commits. - let (commits, _) = - repository.graph_data(log_source.clone(), log_order, 0..usize::MAX, cx); - graph.add_commits(commits); - }); - } + let active_repository = project + .read(cx) + .active_repository(cx) + .map(|repo| repo.read(cx).id); let table_interaction_state = cx.new(|cx| TableInteractionState::new(cx)); let table_column_widths = cx.new(|cx| TableColumnWidths::new(4, cx)); @@ -715,17 +719,16 @@ impl GitGraph { state.scroll_handle.0.borrow_mut().last_item_size = None; }); row_height = new_row_height; + cx.notify(); } - cx.notify(); }) .detach(); - GitGraph { + let mut this = GitGraph { focus_handle, project, workspace, graph_data: graph, - _load_task: None, _commit_diff_task: None, context_menu: None, row_height, @@ -738,7 +741,11 @@ impl GitGraph { log_source, log_order, commit_details_split_state: cx.new(|_cx| SplitState::new()), - } + selected_repo_id: active_repository, + }; + + this.fetch_initial_graph_data(cx); + this } fn on_repository_event( @@ -748,29 +755,52 @@ impl GitGraph { cx: &mut Context, ) { match event { - RepositoryEvent::GitGraphCountUpdated(_, commit_count) => { + RepositoryEvent::GitGraphCountUpdated((order, source), commit_count) => { + if order != &self.log_order || source != &self.log_source { + return; + } + let old_count = self.graph_data.commits.len(); repository.update(cx, |repository, cx| { - let (commits, _) = repository.graph_data( - self.log_source.clone(), - self.log_order, - old_count..*commit_count, - cx, - ); + let (commits, _) = + repository.graph_data(source.clone(), *order, old_count..*commit_count, cx); self.graph_data.add_commits(commits); }); - - self.graph_data.max_commit_count = AllCommitCount::Loaded(*commit_count); - } - RepositoryEvent::BranchChanged => { - self.graph_data.clear(); cx.notify(); } + RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { + // Only invalidate if we scanned atleast once, + // meaning we are not inside the initial repo loading state + // NOTE: this fixes an loading performance regression + if repository.read(cx).scan_id > 1 { + self.graph_data.clear(); + cx.notify(); + } + } _ => {} } + } - cx.notify(); + fn fetch_initial_graph_data(&mut self, cx: &mut App) { + if let Some(repository) = self.get_selected_repository(cx) { + repository.update(cx, |repository, cx| { + let (commits, _) = repository.graph_data( + self.log_source.clone(), + self.log_order, + 0..usize::MAX, + cx, + ); + self.graph_data.add_commits(commits); + }); + } + } + + fn get_selected_repository(&self, cx: &App) -> Option> { + let project = self.project.read(cx); + self.selected_repo_id + .as_ref() + .and_then(|repo_id| project.repositories(cx).get(&repo_id).cloned()) } fn render_badge(&self, name: &SharedString, accent_color: gpui::Hsla) -> impl IntoElement { @@ -799,9 +829,7 @@ impl GitGraph { _window: &mut Window, cx: &mut Context, ) -> Vec> { - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); + let repository = self.get_selected_repository(cx); let row_height = self.row_height; @@ -943,11 +971,8 @@ impl GitGraph { }; let sha = commit.data.sha.to_string(); - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); - let Some(repository) = repository else { + let Some(repository) = self.get_selected_repository(cx) else { return; }; @@ -984,11 +1009,7 @@ impl GitGraph { return; }; - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); - - let Some(repository) = repository else { + let Some(repository) = self.get_selected_repository(cx) else { return; }; @@ -1034,19 +1055,15 @@ impl GitGraph { cx: &mut Context, ) -> impl IntoElement { let Some(selected_idx) = self.selected_entry_idx else { - return div().into_any_element(); + return Empty.into_any_element(); }; let Some(commit_entry) = self.graph_data.commits.get(selected_idx) else { - return div().into_any_element(); + return Empty.into_any_element(); }; - let repository = self - .project - .read_with(cx, |project, cx| project.active_repository(cx)); - - let Some(repository) = repository else { - return div().into_any_element(); + let Some(repository) = self.get_selected_repository(cx) else { + return Empty.into_any_element(); }; let data = repository.update(cx, |repository, cx| { @@ -1634,24 +1651,28 @@ impl Render for GitGraph { let (commit_count, is_loading) = match self.graph_data.max_commit_count { AllCommitCount::Loaded(count) => (count, true), AllCommitCount::NotLoaded => { - let is_loading = self.project.update(cx, |project, cx| { - if let Some(repository) = project.active_repository(cx) { + let (commit_count, is_loading) = + if let Some(repository) = self.get_selected_repository(cx) { repository.update(cx, |repository, cx| { // Start loading the graph data if we haven't started already - repository - .graph_data(self.log_source.clone(), self.log_order, 0..0, cx) - .1 + let (commits, is_loading) = repository.graph_data( + self.log_source.clone(), + self.log_order, + 0..usize::MAX, + cx, + ); + self.graph_data.add_commits(&commits); + (commits.len(), is_loading) }) } else { - false - } - }) && self.graph_data.commits.is_empty(); + (0, false) + }; - (self.graph_data.commits.len(), is_loading) + (commit_count, is_loading) } }; - let content = if self.graph_data.commits.is_empty() { + let content = if commit_count == 0 { let message = if is_loading { "Loading" } else { @@ -1685,6 +1706,7 @@ impl Render for GitGraph { div() .p_2() .border_b_1() + .whitespace_nowrap() .border_color(cx.theme().colors().border) .child(Label::new("Graph").color(Color::Muted)), ) @@ -1945,12 +1967,14 @@ mod tests { use git::repository::InitialGraphCommitData; use gpui::TestAppContext; use project::Project; + use project::git_store::{GitStoreEvent, RepositoryEvent}; use rand::prelude::*; use serde_json::json; use settings::SettingsStore; use smallvec::{SmallVec, smallvec}; use std::path::Path; - use std::sync::Arc; + use std::sync::{Arc, Mutex}; + use workspace::MultiWorkspace; fn init_test(cx: &mut TestAppContext) { cx.update(|cx| { @@ -1959,6 +1983,14 @@ mod tests { }); } + fn init_test_with_theme(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + theme::init(theme::LoadThemes::JustBase, cx); + }); + } + /// Generates a random commit DAG suitable for testing git graph rendering. /// /// The commits are ordered newest-first (like git log output), so: @@ -2526,7 +2558,7 @@ mod tests { // The full integration test has less iterations because it's significantly slower // than the random commit test - #[gpui::test(iterations = 5)] + #[gpui::test(iterations = 10)] async fn test_git_graph_random_integration(mut rng: StdRng, cx: &mut TestAppContext) { init_test(cx); @@ -2591,4 +2623,189 @@ mod tests { ); } } + + #[gpui::test] + async fn test_initial_graph_data_not_cleared_on_initial_loading(cx: &mut TestAppContext) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + Path::new("/project"), + json!({ + ".git": {}, + "file.txt": "content", + }), + ) + .await; + + let mut rng = StdRng::seed_from_u64(42); + let commits = generate_random_commit_dag(&mut rng, 10, false); + fs.set_graph_commits(Path::new("/project/.git"), commits.clone()); + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + let observed_repository_events = Arc::new(Mutex::new(Vec::new())); + project.update(cx, |project, cx| { + let observed_repository_events = observed_repository_events.clone(); + cx.subscribe(project.git_store(), move |_, _, event, _| { + if let GitStoreEvent::RepositoryUpdated(_, repository_event, true) = event { + observed_repository_events + .lock() + .expect("repository event mutex should be available") + .push(repository_event.clone()); + } + }) + .detach(); + }); + + let repository = project.read_with(cx, |project, cx| { + project + .active_repository(cx) + .expect("should have a repository") + }); + + repository.update(cx, |repo, cx| { + repo.graph_data( + crate::LogSource::default(), + crate::LogOrder::default(), + 0..usize::MAX, + cx, + ); + }); + + project + .update(cx, |project, cx| project.git_scans_complete(cx)) + .await; + cx.run_until_parked(); + + let observed_repository_events = observed_repository_events + .lock() + .expect("repository event mutex should be available"); + assert!( + observed_repository_events + .iter() + .any(|event| matches!(event, RepositoryEvent::BranchChanged)), + "initial repository scan should emit BranchChanged" + ); + assert!( + observed_repository_events + .iter() + .any(|event| matches!(event, RepositoryEvent::MergeHeadsChanged)), + "initial repository scan should emit MergeHeadsChanged" + ); + + let graph_data_key = (crate::LogOrder::default(), crate::LogSource::default()); + let commit_count_after = repository.read_with(cx, |repo, _| { + repo.initial_graph_data + .get(&graph_data_key) + .map(|(_, data)| data.len()) + .unwrap_or(0) + }); + assert_eq!( + commits.len(), + commit_count_after, + "initial_graph_data should remain populated after events emitted by initial repository scan" + ); + } + + #[gpui::test] + async fn test_graph_data_repopulated_from_cache_after_repo_switch(cx: &mut TestAppContext) { + init_test_with_theme(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + Path::new("/project_a"), + json!({ + ".git": {}, + "file.txt": "content", + }), + ) + .await; + fs.insert_tree( + Path::new("/project_b"), + json!({ + ".git": {}, + "other.txt": "content", + }), + ) + .await; + + let mut rng = StdRng::seed_from_u64(42); + let commits = generate_random_commit_dag(&mut rng, 10, false); + fs.set_graph_commits(Path::new("/project_a/.git"), commits.clone()); + + let project = Project::test( + fs.clone(), + [Path::new("/project_a"), Path::new("/project_b")], + cx, + ) + .await; + cx.run_until_parked(); + + let (first_repository, second_repository) = project.read_with(cx, |project, cx| { + let mut first_repository = None; + let mut second_repository = None; + + for repository in project.repositories(cx).values() { + let work_directory_abs_path = &repository.read(cx).work_directory_abs_path; + if work_directory_abs_path.as_ref() == Path::new("/project_a") { + first_repository = Some(repository.clone()); + } else if work_directory_abs_path.as_ref() == Path::new("/project_b") { + second_repository = Some(repository.clone()); + } + } + + ( + first_repository.expect("should have repository for /project_a"), + second_repository.expect("should have repository for /project_b"), + ) + }); + first_repository.update(cx, |repository, cx| repository.set_as_active_repository(cx)); + cx.run_until_parked(); + + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + + let workspace_weak = + multi_workspace.read_with(&*cx, |multi, _| multi.workspace().downgrade()); + let git_graph = cx.new_window_entity(|window, cx| { + GitGraph::new(project.clone(), workspace_weak, window, cx) + }); + cx.run_until_parked(); + + // Verify initial graph data is loaded + let initial_commit_count = + git_graph.read_with(&*cx, |graph, _| graph.graph_data.commits.len()); + assert!( + initial_commit_count > 0, + "graph data should have been loaded, got 0 commits" + ); + + second_repository.update(&mut *cx, |repository, cx| { + repository.set_as_active_repository(cx) + }); + cx.run_until_parked(); + + let commit_count_after_clear = + git_graph.read_with(&*cx, |graph, _| graph.graph_data.commits.len()); + assert_eq!( + commit_count_after_clear, 0, + "graph_data should be cleared after switching away" + ); + + first_repository.update(&mut *cx, |repository, cx| { + repository.set_as_active_repository(cx) + }); + + git_graph.update_in(&mut *cx, |this, window, cx| { + this.render(window, cx); + }); + cx.run_until_parked(); + + let commit_count_after_switch_back = + git_graph.read_with(&*cx, |graph, _| graph.graph_data.commits.len()); + assert_eq!( + initial_commit_count, commit_count_after_switch_back, + "graph_data should be repopulated from cache after switching back to the same repo" + ); + } } diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index b9d6f72d532609775ccc8b8d426cf278a390cdc2..1272a689b908413fff5eef71cf5e0e98fd72429b 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -3743,9 +3743,11 @@ impl Repository { }) .shared(); - cx.subscribe_self(|this, event: &RepositoryEvent, _| match event { + cx.subscribe_self(move |this, event: &RepositoryEvent, _| match event { RepositoryEvent::BranchChanged | RepositoryEvent::MergeHeadsChanged => { - this.initial_graph_data.clear(); + if this.scan_id > 1 { + this.initial_graph_data.clear(); + } } _ => {} })