From 5c91ebf1fe9f8716a945a669b2e9ebeb83cb6fbe Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Wed, 4 Mar 2026 19:54:23 +0100 Subject: [PATCH] git: Move diff num stat calculation to repository snapshot layer (#50645) Follow up on: https://github.com/zed-industries/zed/pull/49519 This PR reworks how Zed calculates diff num stats by moving the calculation to the `RepositorySnapshot` layer, instead of the `GitPanel`. This has a couple of benefits: 1. Snapshot recalculations are already set up to recompute on file system changes and only update the affected files. This means that diff stats don't need to manage their own subscription or states anymore like they did in the original PR. 2. We're able to further separate the data layer from the UI. Before, the git panel owned all the subscriptions and tasks that refreshed the diff stat, now the repository does, which is more inline with the code base. 3. Integration tests are cleaner because `FakeRepository` can handle all the data and calculations of diff stat and make it accessible to more tests in the codebase. Because a lot of tests wouldn't initialize the git panel when they used the git repository. 4. This made implementing remote/collab support for this feature streamline. Remote clients wouldn't get the same buffer events as local clients, so they wouldn't know that the diff stat state has been updated and invalidate their data. 5. File system changes that happened outside of Zed now trigger the diff stat refresh because we're using the `RepositorySnapshot`. I added some integration tests as well to make sure collab support is working this time. Finally, adding the initial diff calculation to `compute_snapshot` didn't affect performance for me when checking against chromium's diff with HEAD~1000. So this should be a safe change to make. I decided to add diff stats on the status entry struct because it made updating changed paths and the collab database much simpler than having two separate SumTrees. Also whenever the UI got a file's status it would check its diff stat as well, so this change makes that code more streamlined as well. Before you mark this PR as ready for review, make sure that you have: - [x] Added a solid test coverage and/or screenshots from doing manual testing. - [x] Done a self-review taking into account security and performance aspects. Release Notes: - N/A --- Cargo.lock | 1 + .../20221109000000_test_schema.sql | 2 + .../migrations/20251208000000_test_schema.sql | 2 + crates/collab/src/db.rs | 2 + crates/collab/src/db/queries/projects.rs | 149 +----------- crates/collab/src/db/queries/rooms.rs | 2 +- .../db/tables/project_repository_statuses.rs | 2 + crates/collab/tests/integration/git_tests.rs | 223 +++++++++++++++++- crates/fs/src/fake_git_repo.rs | 199 +++++++--------- crates/git/src/repository.rs | 63 ++--- crates/git/src/status.rs | 52 ++-- crates/git_ui/src/git_panel.rs | 153 ++++-------- crates/lsp/Cargo.toml | 4 +- crates/lsp/src/lsp.rs | 10 +- crates/project/src/git_store.rs | 196 +++++++-------- .../tests/integration/project_tests.rs | 39 ++- crates/proto/proto/git.proto | 25 +- crates/proto/proto/zed.proto | 5 +- crates/proto/src/proto.rs | 4 - .../remote_server/src/remote_editing_tests.rs | 124 ---------- 20 files changed, 562 insertions(+), 695 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d1b0a39869a44af1295235214836d446c509c360..c4ec49e50c3aa75e5e470414da470301e6f77e04 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10025,6 +10025,7 @@ dependencies = [ "ctor", "futures 0.3.31", "gpui", + "gpui_util", "log", "lsp-types", "parking_lot", diff --git a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql index 71e39fb595656e0dcdc53d97705b87a216ceb0f3..3e4b5c2ce211f68ef7e12895b542db5e6e3ea47c 100644 --- a/crates/collab/migrations.sqlite/20221109000000_test_schema.sql +++ b/crates/collab/migrations.sqlite/20221109000000_test_schema.sql @@ -122,6 +122,8 @@ CREATE TABLE "project_repository_statuses" ( "status_kind" INT4 NOT NULL, "first_status" INT4 NULL, "second_status" INT4 NULL, + "lines_added" INT4 NULL, + "lines_deleted" INT4 NULL, "scan_id" INT8 NOT NULL, "is_deleted" BOOL NOT NULL, PRIMARY KEY (project_id, repository_id, repo_path) diff --git a/crates/collab/migrations/20251208000000_test_schema.sql b/crates/collab/migrations/20251208000000_test_schema.sql index 493be3823e25a433d4a6a27a21c508f218dc68d1..0f4e4f2d2e3925ea1e4d2b964c5e4f159f393b4f 100644 --- a/crates/collab/migrations/20251208000000_test_schema.sql +++ b/crates/collab/migrations/20251208000000_test_schema.sql @@ -315,6 +315,8 @@ CREATE TABLE public.project_repository_statuses ( status_kind integer NOT NULL, first_status integer, second_status integer, + lines_added integer, + lines_deleted integer, scan_id bigint NOT NULL, is_deleted boolean NOT NULL ); diff --git a/crates/collab/src/db.rs b/crates/collab/src/db.rs index 57fb0df86495dc2013e7cd780c2e62e57298bd11..d8803c253f5feef8ef5e040f3ea112abcc688f52 100644 --- a/crates/collab/src/db.rs +++ b/crates/collab/src/db.rs @@ -732,6 +732,8 @@ fn db_status_to_proto( status: Some(proto::GitFileStatus { variant: Some(variant), }), + diff_stat_added: entry.lines_added.map(|v| v as u32), + diff_stat_deleted: entry.lines_deleted.map(|v| v as u32), }) } diff --git a/crates/collab/src/db/queries/projects.rs b/crates/collab/src/db/queries/projects.rs index fa3f99e1483e8a5d8410378493556b189eff78f1..24cf639a715aa9b88da80375b389debaea0c4295 100644 --- a/crates/collab/src/db/queries/projects.rs +++ b/crates/collab/src/db/queries/projects.rs @@ -334,147 +334,6 @@ impl Database { .await?; } - // Backward-compatibility for old Zed clients. - // - // Remove this block when Zed 1.80 stable has been out for a week. - { - if !update.updated_repositories.is_empty() { - project_repository::Entity::insert_many( - update.updated_repositories.iter().map(|repository| { - project_repository::ActiveModel { - project_id: ActiveValue::set(project_id), - legacy_worktree_id: ActiveValue::set(Some(worktree_id)), - id: ActiveValue::set(repository.repository_id as i64), - scan_id: ActiveValue::set(update.scan_id as i64), - is_deleted: ActiveValue::set(false), - branch_summary: ActiveValue::Set( - repository - .branch_summary - .as_ref() - .map(|summary| serde_json::to_string(summary).unwrap()), - ), - current_merge_conflicts: ActiveValue::Set(Some( - serde_json::to_string(&repository.current_merge_conflicts) - .unwrap(), - )), - // Old clients do not use abs path, entry ids, head_commit_details, or merge_message. - abs_path: ActiveValue::set(String::new()), - entry_ids: ActiveValue::set("[]".into()), - head_commit_details: ActiveValue::set(None), - merge_message: ActiveValue::set(None), - remote_upstream_url: ActiveValue::set(None), - remote_origin_url: ActiveValue::set(None), - } - }), - ) - .on_conflict( - OnConflict::columns([ - project_repository::Column::ProjectId, - project_repository::Column::Id, - ]) - .update_columns([ - project_repository::Column::ScanId, - project_repository::Column::BranchSummary, - project_repository::Column::CurrentMergeConflicts, - ]) - .to_owned(), - ) - .exec(&*tx) - .await?; - - let has_any_statuses = update - .updated_repositories - .iter() - .any(|repository| !repository.updated_statuses.is_empty()); - - if has_any_statuses { - project_repository_statuses::Entity::insert_many( - update.updated_repositories.iter().flat_map( - |repository: &proto::RepositoryEntry| { - repository.updated_statuses.iter().map(|status_entry| { - let (repo_path, status_kind, first_status, second_status) = - proto_status_to_db(status_entry.clone()); - project_repository_statuses::ActiveModel { - project_id: ActiveValue::set(project_id), - repository_id: ActiveValue::set( - repository.repository_id as i64, - ), - scan_id: ActiveValue::set(update.scan_id as i64), - is_deleted: ActiveValue::set(false), - repo_path: ActiveValue::set(repo_path), - status: ActiveValue::set(0), - status_kind: ActiveValue::set(status_kind), - first_status: ActiveValue::set(first_status), - second_status: ActiveValue::set(second_status), - } - }) - }, - ), - ) - .on_conflict( - OnConflict::columns([ - project_repository_statuses::Column::ProjectId, - project_repository_statuses::Column::RepositoryId, - project_repository_statuses::Column::RepoPath, - ]) - .update_columns([ - project_repository_statuses::Column::ScanId, - project_repository_statuses::Column::StatusKind, - project_repository_statuses::Column::FirstStatus, - project_repository_statuses::Column::SecondStatus, - ]) - .to_owned(), - ) - .exec(&*tx) - .await?; - } - - for repo in &update.updated_repositories { - if !repo.removed_statuses.is_empty() { - project_repository_statuses::Entity::update_many() - .filter( - project_repository_statuses::Column::ProjectId - .eq(project_id) - .and( - project_repository_statuses::Column::RepositoryId - .eq(repo.repository_id), - ) - .and( - project_repository_statuses::Column::RepoPath - .is_in(repo.removed_statuses.iter()), - ), - ) - .set(project_repository_statuses::ActiveModel { - is_deleted: ActiveValue::Set(true), - scan_id: ActiveValue::Set(update.scan_id as i64), - ..Default::default() - }) - .exec(&*tx) - .await?; - } - } - } - - if !update.removed_repositories.is_empty() { - project_repository::Entity::update_many() - .filter( - project_repository::Column::ProjectId - .eq(project_id) - .and(project_repository::Column::LegacyWorktreeId.eq(worktree_id)) - .and(project_repository::Column::Id.is_in( - update.removed_repositories.iter().map(|id| *id as i64), - )), - ) - .set(project_repository::ActiveModel { - is_deleted: ActiveValue::Set(true), - scan_id: ActiveValue::Set(update.scan_id as i64), - ..Default::default() - }) - .exec(&*tx) - .await?; - } - } - let connection_ids = self.project_guest_connection_ids(project_id, &tx).await?; Ok(connection_ids) }) @@ -552,6 +411,12 @@ impl Database { status_kind: ActiveValue::set(status_kind), first_status: ActiveValue::set(first_status), second_status: ActiveValue::set(second_status), + lines_added: ActiveValue::set( + status_entry.diff_stat_added.map(|v| v as i32), + ), + lines_deleted: ActiveValue::set( + status_entry.diff_stat_deleted.map(|v| v as i32), + ), } }), ) @@ -566,6 +431,8 @@ impl Database { project_repository_statuses::Column::StatusKind, project_repository_statuses::Column::FirstStatus, project_repository_statuses::Column::SecondStatus, + project_repository_statuses::Column::LinesAdded, + project_repository_statuses::Column::LinesDeleted, ]) .to_owned(), ) diff --git a/crates/collab/src/db/queries/rooms.rs b/crates/collab/src/db/queries/rooms.rs index 7c007a570a0cb25c5302495d7342882eec0e1942..b4cbd83167b227542d8de1022b7e2cf49f5a7645 100644 --- a/crates/collab/src/db/queries/rooms.rs +++ b/crates/collab/src/db/queries/rooms.rs @@ -738,7 +738,7 @@ impl Database { while let Some(db_status) = db_statuses.next().await { let db_status: project_repository_statuses::Model = db_status?; if db_status.is_deleted { - removed_statuses.push(db_status.repo_path); + removed_statuses.push(db_status.repo_path.clone()); } else { updated_statuses.push(db_status_to_proto(db_status)?); } diff --git a/crates/collab/src/db/tables/project_repository_statuses.rs b/crates/collab/src/db/tables/project_repository_statuses.rs index 7bb903d45085467a3285a58f8afdd7a29339731a..8160d8a03c2a3b4dd0db7675489eeafcef020a9a 100644 --- a/crates/collab/src/db/tables/project_repository_statuses.rs +++ b/crates/collab/src/db/tables/project_repository_statuses.rs @@ -17,6 +17,8 @@ pub struct Model { pub first_status: Option, /// For unmerged entries, this is the `second_head` status. For tracked entries, this is the `worktree_status`. pub second_status: Option, + pub lines_added: Option, + pub lines_deleted: Option, pub scan_id: i64, pub is_deleted: bool, } diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index 6e50e41bade5f5dfdf124f5a6d659e81fc2ce0f6..dccc99a07769e66a3eb318a8201d8e14a29ef4f2 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -1,16 +1,40 @@ use std::path::{Path, PathBuf}; use call::ActiveCall; -use git::status::{FileStatus, StatusCode, TrackedStatus}; -use git_ui::project_diff::ProjectDiff; +use collections::HashMap; +use git::{ + repository::RepoPath, + status::{DiffStat, FileStatus, StatusCode, TrackedStatus}, +}; +use git_ui::{git_panel::GitPanel, project_diff::ProjectDiff}; use gpui::{AppContext as _, BackgroundExecutor, TestAppContext, VisualTestContext}; use project::ProjectPath; use serde_json::json; + use util::{path, rel_path::rel_path}; use workspace::{MultiWorkspace, Workspace}; use crate::TestServer; +fn collect_diff_stats( + panel: &gpui::Entity, + cx: &C, +) -> HashMap { + panel.read_with(cx, |panel, cx| { + let Some(repo) = panel.active_repository() else { + return HashMap::default(); + }; + let snapshot = repo.read(cx).snapshot(); + let mut stats = HashMap::default(); + for entry in snapshot.statuses_by_path.iter() { + if let Some(diff_stat) = entry.diff_stat { + stats.insert(entry.repo_path.clone(), diff_stat); + } + } + stats + }) +} + #[gpui::test] async fn test_project_diff(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext) { let mut server = TestServer::start(cx_a.background_executor.clone()).await; @@ -279,3 +303,198 @@ async fn test_remote_git_worktrees( ); assert_eq!(bugfix_worktree.sha.as_ref(), "fake-sha"); } + +#[gpui::test] +async fn test_diff_stat_sync_between_host_and_downstream_client( + cx_a: &mut TestAppContext, + cx_b: &mut TestAppContext, + cx_c: &mut TestAppContext, +) { + let mut server = TestServer::start(cx_a.background_executor.clone()).await; + let client_a = server.create_client(cx_a, "user_a").await; + let client_b = server.create_client(cx_b, "user_b").await; + let client_c = server.create_client(cx_c, "user_c").await; + + server + .create_room(&mut [(&client_a, cx_a), (&client_b, cx_b), (&client_c, cx_c)]) + .await; + + let fs = client_a.fs(); + fs.insert_tree( + path!("/code"), + json!({ + "project1": { + ".git": {}, + "src": { + "lib.rs": "line1\nline2\nline3\n", + "new_file.rs": "added1\nadded2\n", + }, + "README.md": "# project 1", + } + }), + ) + .await; + + let dot_git = Path::new(path!("/code/project1/.git")); + fs.set_head_for_repo( + dot_git, + &[ + ("src/lib.rs", "line1\nold_line2\n".into()), + ("src/deleted.rs", "was_here\n".into()), + ], + "deadbeef", + ); + fs.set_index_for_repo( + dot_git, + &[ + ("src/lib.rs", "line1\nold_line2\nline3\nline4\n".into()), + ("src/staged_only.rs", "x\ny\n".into()), + ("src/new_file.rs", "added1\nadded2\n".into()), + ("README.md", "# project 1".into()), + ], + ); + + let (project_a, worktree_id) = client_a + .build_local_project(path!("/code/project1"), cx_a) + .await; + let active_call_a = cx_a.read(ActiveCall::global); + let project_id = active_call_a + .update(cx_a, |call, cx| call.share_project(project_a.clone(), cx)) + .await + .unwrap(); + let project_b = client_b.join_remote_project(project_id, cx_b).await; + let _project_c = client_c.join_remote_project(project_id, cx_c).await; + cx_a.run_until_parked(); + + let (workspace_a, cx_a) = client_a.build_workspace(&project_a, cx_a); + let (workspace_b, cx_b) = client_b.build_workspace(&project_b, cx_b); + + let panel_a = workspace_a.update_in(cx_a, GitPanel::new_test); + workspace_a.update_in(cx_a, |workspace, window, cx| { + workspace.add_panel(panel_a.clone(), window, cx); + }); + + let panel_b = workspace_b.update_in(cx_b, GitPanel::new_test); + workspace_b.update_in(cx_b, |workspace, window, cx| { + workspace.add_panel(panel_b.clone(), window, cx); + }); + + cx_a.run_until_parked(); + + let stats_a = collect_diff_stats(&panel_a, cx_a); + let stats_b = collect_diff_stats(&panel_b, cx_b); + + let mut expected: HashMap = HashMap::default(); + expected.insert( + RepoPath::new("src/lib.rs").unwrap(), + DiffStat { + added: 3, + deleted: 2, + }, + ); + expected.insert( + RepoPath::new("src/deleted.rs").unwrap(), + DiffStat { + added: 0, + deleted: 1, + }, + ); + expected.insert( + RepoPath::new("src/new_file.rs").unwrap(), + DiffStat { + added: 2, + deleted: 0, + }, + ); + expected.insert( + RepoPath::new("README.md").unwrap(), + DiffStat { + added: 1, + deleted: 0, + }, + ); + assert_eq!(stats_a, expected, "host diff stats should match expected"); + assert_eq!(stats_a, stats_b, "host and remote should agree"); + + let buffer_a = project_a + .update(cx_a, |p, cx| { + p.open_buffer((worktree_id, rel_path("src/lib.rs")), cx) + }) + .await + .unwrap(); + + let _buffer_b = project_b + .update(cx_b, |p, cx| { + p.open_buffer((worktree_id, rel_path("src/lib.rs")), cx) + }) + .await + .unwrap(); + cx_a.run_until_parked(); + + buffer_a.update(cx_a, |buf, cx| { + buf.edit([(buf.len()..buf.len(), "line4\n")], None, cx); + }); + project_a + .update(cx_a, |project, cx| { + project.save_buffer(buffer_a.clone(), cx) + }) + .await + .unwrap(); + cx_a.run_until_parked(); + + let stats_a = collect_diff_stats(&panel_a, cx_a); + let stats_b = collect_diff_stats(&panel_b, cx_b); + + let mut expected_after_edit = expected.clone(); + expected_after_edit.insert( + RepoPath::new("src/lib.rs").unwrap(), + DiffStat { + added: 4, + deleted: 2, + }, + ); + assert_eq!( + stats_a, expected_after_edit, + "host diff stats should reflect the edit" + ); + assert_eq!( + stats_b, expected_after_edit, + "remote diff stats should reflect the host's edit" + ); + + let active_call_b = cx_b.read(ActiveCall::global); + active_call_b + .update(cx_b, |call, cx| call.hang_up(cx)) + .await + .unwrap(); + cx_a.run_until_parked(); + + let user_id_b = client_b.current_user_id(cx_b).to_proto(); + active_call_a + .update(cx_a, |call, cx| call.invite(user_id_b, None, cx)) + .await + .unwrap(); + cx_b.run_until_parked(); + let active_call_b = cx_b.read(ActiveCall::global); + active_call_b + .update(cx_b, |call, cx| call.accept_incoming(cx)) + .await + .unwrap(); + cx_a.run_until_parked(); + + let project_b = client_b.join_remote_project(project_id, cx_b).await; + cx_a.run_until_parked(); + + let (workspace_b, cx_b) = client_b.build_workspace(&project_b, cx_b); + let panel_b = workspace_b.update_in(cx_b, GitPanel::new_test); + workspace_b.update_in(cx_b, |workspace, window, cx| { + workspace.add_panel(panel_b.clone(), window, cx); + }); + cx_b.run_until_parked(); + + let stats_b = collect_diff_stats(&panel_b, cx_b); + assert_eq!( + stats_b, expected_after_edit, + "remote diff stats should be restored from the database after rejoining the call" + ); +} diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 06ebea9157f97a0323297cd3ae142c4b306fe4ef..85489b6057cd8214ee512fb477428c93cdb32219 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -795,8 +795,8 @@ impl GitRepository for FakeGitRepository { fn diff_stat( &self, - diff_type: git::repository::DiffType, - ) -> BoxFuture<'_, Result>> { + path_prefixes: &[RepoPath], + ) -> BoxFuture<'_, Result> { fn count_lines(s: &str) -> u32 { if s.is_empty() { 0 @@ -805,122 +805,95 @@ impl GitRepository for FakeGitRepository { } } - match diff_type { - git::repository::DiffType::HeadToIndex => self - .with_state_async(false, |state| { - let mut result = HashMap::default(); - let all_paths: HashSet<&RepoPath> = state - .head_contents - .keys() - .chain(state.index_contents.keys()) - .collect(); - for path in all_paths { - let head = state.head_contents.get(path); - let index = state.index_contents.get(path); - match (head, index) { - (Some(old), Some(new)) if old != new => { - result.insert( - path.clone(), - git::status::DiffStat { - added: count_lines(new), - deleted: count_lines(old), - }, - ); - } - (Some(old), None) => { - result.insert( - path.clone(), - git::status::DiffStat { - added: 0, - deleted: count_lines(old), - }, - ); - } - (None, Some(new)) => { - result.insert( - path.clone(), - git::status::DiffStat { - added: count_lines(new), - deleted: 0, - }, - ); - } - _ => {} - } - } - Ok(result) - }) - .boxed(), - git::repository::DiffType::HeadToWorktree => { - let workdir_path = self.dot_git_path.parent().unwrap().to_path_buf(); - let worktree_files: HashMap = self + fn matches_prefixes(path: &RepoPath, prefixes: &[RepoPath]) -> bool { + if prefixes.is_empty() { + return true; + } + prefixes.iter().any(|prefix| { + let prefix_str = prefix.as_unix_str(); + if prefix_str == "." { + return true; + } + path == prefix || path.starts_with(&prefix) + }) + } + + let path_prefixes = path_prefixes.to_vec(); + + let workdir_path = self.dot_git_path.parent().unwrap().to_path_buf(); + let worktree_files: HashMap = self + .fs + .files() + .iter() + .filter_map(|path| { + let repo_path = path.strip_prefix(&workdir_path).ok()?; + if repo_path.starts_with(".git") { + return None; + } + let content = self .fs - .files() - .iter() - .filter_map(|path| { - let repo_path = path.strip_prefix(&workdir_path).ok()?; - if repo_path.starts_with(".git") { - return None; - } - let content = self - .fs - .read_file_sync(path) - .ok() - .and_then(|bytes| String::from_utf8(bytes).ok())?; - let repo_path = RelPath::new(repo_path, PathStyle::local()).ok()?; - Some((RepoPath::from_rel_path(&repo_path), content)) - }) - .collect(); + .read_file_sync(path) + .ok() + .and_then(|bytes| String::from_utf8(bytes).ok())?; + let repo_path = RelPath::new(repo_path, PathStyle::local()).ok()?; + Some((RepoPath::from_rel_path(&repo_path), content)) + }) + .collect(); - self.with_state_async(false, move |state| { - let mut result = HashMap::default(); - let all_paths: HashSet<&RepoPath> = state - .head_contents + self.with_state_async(false, move |state| { + let mut entries = Vec::new(); + let all_paths: HashSet<&RepoPath> = state + .head_contents + .keys() + .chain( + worktree_files .keys() - .chain(worktree_files.keys()) - .collect(); - for path in all_paths { - let head = state.head_contents.get(path); - let worktree = worktree_files.get(path); - match (head, worktree) { - (Some(old), Some(new)) if old != new => { - result.insert( - path.clone(), - git::status::DiffStat { - added: count_lines(new), - deleted: count_lines(old), - }, - ); - } - (Some(old), None) => { - result.insert( - path.clone(), - git::status::DiffStat { - added: 0, - deleted: count_lines(old), - }, - ); - } - (None, Some(new)) => { - result.insert( - path.clone(), - git::status::DiffStat { - added: count_lines(new), - deleted: 0, - }, - ); - } - _ => {} - } + .filter(|p| state.index_contents.contains_key(*p)), + ) + .collect(); + for path in all_paths { + if !matches_prefixes(path, &path_prefixes) { + continue; + } + let head = state.head_contents.get(path); + let worktree = worktree_files.get(path); + match (head, worktree) { + (Some(old), Some(new)) if old != new => { + entries.push(( + path.clone(), + git::status::DiffStat { + added: count_lines(new), + deleted: count_lines(old), + }, + )); } - Ok(result) - }) - .boxed() - } - git::repository::DiffType::MergeBase { .. } => { - future::ready(Ok(HashMap::default())).boxed() + (Some(old), None) => { + entries.push(( + path.clone(), + git::status::DiffStat { + added: 0, + deleted: count_lines(old), + }, + )); + } + (None, Some(new)) => { + entries.push(( + path.clone(), + git::status::DiffStat { + added: count_lines(new), + deleted: 0, + }, + )); + } + _ => {} + } } - } + entries.sort_by(|(a, _), (b, _)| a.cmp(b)); + Ok(git::status::GitDiffStat { + entries: entries.into(), + }) + }) + .boxed() } fn checkpoint(&self) -> BoxFuture<'static, Result> { diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index f5a856325cc80071f2c8ef500e7b07aa24035f59..c36c70935522836eeea4a83a889109dc807604c8 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -924,8 +924,8 @@ pub trait GitRepository: Send + Sync { fn diff_stat( &self, - diff: DiffType, - ) -> BoxFuture<'_, Result>>; + path_prefixes: &[RepoPath], + ) -> BoxFuture<'_, Result>; /// Creates a checkpoint for the repository. fn checkpoint(&self) -> BoxFuture<'static, Result>; @@ -1997,42 +1997,30 @@ impl GitRepository for RealGitRepository { fn diff_stat( &self, - diff: DiffType, - ) -> BoxFuture<'_, Result>> { + path_prefixes: &[RepoPath], + ) -> BoxFuture<'_, Result> { + let path_prefixes = path_prefixes.to_vec(); let git_binary = self.git_binary(); + self.executor .spawn(async move { - let git = git_binary?; - let output = match diff { - DiffType::HeadToIndex => { - git.build_command(["diff", "--numstat", "--staged"]) - .output() - .await? - } - DiffType::HeadToWorktree => { - git.build_command(["diff", "--numstat"]).output().await? - } - DiffType::MergeBase { base_ref } => { - git.build_command([ - "diff", - "--numstat", - "--merge-base", - base_ref.as_ref(), - "HEAD", - ]) - .output() - .await? - } - }; - - anyhow::ensure!( - output.status.success(), - "Failed to run git diff --numstat:\n{}", - String::from_utf8_lossy(&output.stderr) - ); - Ok(crate::status::parse_numstat(&String::from_utf8_lossy( - &output.stdout, - ))) + let git_binary = git_binary?; + let mut args: Vec = vec![ + "diff".into(), + "--numstat".into(), + "--no-renames".into(), + "HEAD".into(), + ]; + if !path_prefixes.is_empty() { + args.push("--".into()); + args.extend( + path_prefixes + .iter() + .map(|p| p.as_std_path().to_string_lossy().into_owned()), + ); + } + let output = git_binary.run(&args).await?; + Ok(crate::status::parse_numstat(&output)) }) .boxed() } @@ -2942,11 +2930,6 @@ fn git_status_args(path_prefixes: &[RepoPath]) -> Vec { OsString::from("--no-renames"), OsString::from("-z"), ]; - args.extend( - path_prefixes - .iter() - .map(|path_prefix| path_prefix.as_std_path().into()), - ); args.extend(path_prefixes.iter().map(|path_prefix| { if path_prefix.is_empty() { Path::new(".").into() diff --git a/crates/git/src/status.rs b/crates/git/src/status.rs index b20919e7ecf4748d0035a003ed5eadebae752dd7..e8b5caec505f7bf65cb4f5cd7d789207ccd8784f 100644 --- a/crates/git/src/status.rs +++ b/crates/git/src/status.rs @@ -586,13 +586,18 @@ pub struct DiffStat { pub deleted: u32, } +#[derive(Clone, Debug)] +pub struct GitDiffStat { + pub entries: Arc<[(RepoPath, DiffStat)]>, +} + /// Parses the output of `git diff --numstat` where output looks like: /// /// ```text /// 24 12 dir/file.txt /// ``` -pub fn parse_numstat(output: &str) -> HashMap { - let mut stats = HashMap::default(); +pub fn parse_numstat(output: &str) -> GitDiffStat { + let mut entries = Vec::new(); for line in output.lines() { let line = line.trim(); if line.is_empty() { @@ -613,10 +618,14 @@ pub fn parse_numstat(output: &str) -> HashMap { let Ok(path) = RepoPath::new(path_str) else { continue; }; - let stat = DiffStat { added, deleted }; - stats.insert(path, stat); + entries.push((path, DiffStat { added, deleted })); + } + entries.sort_by(|(a, _), (b, _)| a.cmp(b)); + entries.dedup_by(|(a, _), (b, _)| a == b); + + GitDiffStat { + entries: entries.into(), } - stats } #[cfg(test)] @@ -629,20 +638,25 @@ mod tests { use super::{DiffStat, parse_numstat}; + fn lookup<'a>(entries: &'a [(RepoPath, DiffStat)], path: &str) -> Option<&'a DiffStat> { + let path = RepoPath::new(path).unwrap(); + entries.iter().find(|(p, _)| p == &path).map(|(_, s)| s) + } + #[test] fn test_parse_numstat_normal() { let input = "10\t5\tsrc/main.rs\n3\t1\tREADME.md\n"; let result = parse_numstat(input); - assert_eq!(result.len(), 2); + assert_eq!(result.entries.len(), 2); assert_eq!( - result.get(&RepoPath::new("src/main.rs").unwrap()), + lookup(&result.entries, "src/main.rs"), Some(&DiffStat { added: 10, deleted: 5 }) ); assert_eq!( - result.get(&RepoPath::new("README.md").unwrap()), + lookup(&result.entries, "README.md"), Some(&DiffStat { added: 3, deleted: 1 @@ -655,10 +669,10 @@ mod tests { // git diff --numstat outputs "-\t-\tpath" for binary files let input = "-\t-\timage.png\n5\t2\tsrc/lib.rs\n"; let result = parse_numstat(input); - assert_eq!(result.len(), 1); - assert!(!result.contains_key(&RepoPath::new("image.png").unwrap())); + assert_eq!(result.entries.len(), 1); + assert!(lookup(&result.entries, "image.png").is_none()); assert_eq!( - result.get(&RepoPath::new("src/lib.rs").unwrap()), + lookup(&result.entries, "src/lib.rs"), Some(&DiffStat { added: 5, deleted: 2 @@ -668,18 +682,18 @@ mod tests { #[test] fn test_parse_numstat_empty_input() { - assert!(parse_numstat("").is_empty()); - assert!(parse_numstat("\n\n").is_empty()); - assert!(parse_numstat(" \n \n").is_empty()); + assert!(parse_numstat("").entries.is_empty()); + assert!(parse_numstat("\n\n").entries.is_empty()); + assert!(parse_numstat(" \n \n").entries.is_empty()); } #[test] fn test_parse_numstat_malformed_lines_skipped() { let input = "not_a_number\t5\tfile.rs\n10\t5\tvalid.rs\n"; let result = parse_numstat(input); - assert_eq!(result.len(), 1); + assert_eq!(result.entries.len(), 1); assert_eq!( - result.get(&RepoPath::new("valid.rs").unwrap()), + lookup(&result.entries, "valid.rs"), Some(&DiffStat { added: 10, deleted: 5 @@ -692,9 +706,9 @@ mod tests { // Lines with fewer than 3 tab-separated fields are skipped let input = "10\t5\n7\t3\tok.rs\n"; let result = parse_numstat(input); - assert_eq!(result.len(), 1); + assert_eq!(result.entries.len(), 1); assert_eq!( - result.get(&RepoPath::new("ok.rs").unwrap()), + lookup(&result.entries, "ok.rs"), Some(&DiffStat { added: 7, deleted: 3 @@ -707,7 +721,7 @@ mod tests { let input = "0\t0\tunchanged_but_present.rs\n"; let result = parse_numstat(input); assert_eq!( - result.get(&RepoPath::new("unchanged_but_present.rs").unwrap()), + lookup(&result.entries, "unchanged_but_present.rs"), Some(&DiffStat { added: 0, deleted: 0 diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index 1fabc387247e3f0889749463e3aabd89ef0bff42..61d94b68a118525bd9b67217a929ce7462696dc7 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -41,7 +41,7 @@ use gpui::{ WeakEntity, actions, anchored, deferred, point, size, uniform_list, }; use itertools::Itertools; -use language::{Buffer, BufferEvent, File}; +use language::{Buffer, File}; use language_model::{ ConfiguredModel, LanguageModelRegistry, LanguageModelRequest, LanguageModelRequestMessage, Role, }; @@ -51,7 +51,6 @@ use notifications::status_toast::{StatusToast, ToastIcon}; use panel::{PanelHeader, panel_button, panel_filled_button, panel_icon_button}; use project::{ Fs, Project, ProjectPath, - buffer_store::BufferStoreEvent, git_store::{GitStoreEvent, Repository, RepositoryEvent, RepositoryId, pending_op}, project_settings::{GitPathStyle, ProjectSettings}, }; @@ -533,6 +532,7 @@ pub struct GitStatusEntry { pub(crate) repo_path: RepoPath, pub(crate) status: FileStatus, pub(crate) staging: StageStatus, + pub(crate) diff_stat: Option, } impl GitStatusEntry { @@ -653,8 +653,7 @@ pub struct GitPanel { local_committer_task: Option>, bulk_staging: Option, stash_entries: GitStash, - diff_stats: HashMap, - diff_stats_task: Task<()>, + _settings_subscription: Subscription, } @@ -723,18 +722,14 @@ impl GitPanel { if tree_view != was_tree_view { this.view_mode = GitPanelViewMode::from_settings(cx); } + + let mut update_entries = false; if sort_by_path != was_sort_by_path || tree_view != was_tree_view { this.bulk_staging.take(); - this.update_visible_entries(window, cx); + update_entries = true; } - if diff_stats != was_diff_stats { - if diff_stats { - this.fetch_diff_stats(cx); - } else { - this.diff_stats.clear(); - this.diff_stats_task = Task::ready(()); - cx.notify(); - } + if (diff_stats != was_diff_stats) || update_entries { + this.update_visible_entries(window, cx); } was_sort_by_path = sort_by_path; was_tree_view = tree_view; @@ -791,33 +786,6 @@ impl GitPanel { ) .detach(); - let buffer_store = project.read(cx).buffer_store().clone(); - - for buffer in project.read(cx).opened_buffers(cx) { - cx.subscribe(&buffer, |this, _buffer, event, cx| { - if matches!(event, BufferEvent::Saved) { - if GitPanelSettings::get_global(cx).diff_stats { - this.fetch_diff_stats(cx); - } - } - }) - .detach(); - } - - cx.subscribe(&buffer_store, |_this, _store, event, cx| { - if let BufferStoreEvent::BufferAdded(buffer) = event { - cx.subscribe(buffer, |this, _buffer, event, cx| { - if matches!(event, BufferEvent::Saved) { - if GitPanelSettings::get_global(cx).diff_stats { - this.fetch_diff_stats(cx); - } - } - }) - .detach(); - } - }) - .detach(); - let mut this = Self { active_repository, commit_editor, @@ -858,8 +826,6 @@ impl GitPanel { entry_count: 0, bulk_staging: None, stash_entries: Default::default(), - diff_stats: HashMap::default(), - diff_stats_task: Task::ready(()), _settings_subscription, }; @@ -3575,6 +3541,7 @@ impl GitPanel { repo_path: entry.repo_path.clone(), status: entry.status, staging, + diff_stat: entry.diff_stat, }; if staging.has_staged() { @@ -3611,6 +3578,7 @@ impl GitPanel { repo_path: ops.repo_path.clone(), status: status.status, staging: StageStatus::Staged, + diff_stat: status.diff_stat, }); } } @@ -3743,60 +3711,9 @@ impl GitPanel { editor.set_placeholder_text(&placeholder_text, window, cx) }); - if GitPanelSettings::get_global(cx).diff_stats { - self.fetch_diff_stats(cx); - } - cx.notify(); } - fn fetch_diff_stats(&mut self, cx: &mut Context) { - let Some(repo) = self.active_repository.clone() else { - self.diff_stats.clear(); - return; - }; - - let unstaged_rx = repo.update(cx, |repo, cx| repo.diff_stat(DiffType::HeadToWorktree, cx)); - let staged_rx = repo.update(cx, |repo, cx| repo.diff_stat(DiffType::HeadToIndex, cx)); - - self.diff_stats_task = cx.spawn(async move |this, cx| { - let (unstaged_result, staged_result) = - futures::future::join(unstaged_rx, staged_rx).await; - - let mut combined = match unstaged_result { - Ok(Ok(stats)) => stats, - Ok(Err(err)) => { - log::warn!("Failed to fetch unstaged diff stats: {err:?}"); - HashMap::default() - } - Err(_) => HashMap::default(), - }; - - let staged = match staged_result { - Ok(Ok(stats)) => Some(stats), - Ok(Err(err)) => { - log::warn!("Failed to fetch staged diff stats: {err:?}"); - None - } - Err(_) => None, - }; - - if let Some(staged) = staged { - for (path, stat) in staged { - let entry = combined.entry(path).or_default(); - entry.added += stat.added; - entry.deleted += stat.deleted; - } - } - - this.update(cx, |this, cx| { - this.diff_stats = combined; - cx.notify(); - }) - .ok(); - }); - } - fn header_state(&self, header_type: Section) -> ToggleState { let (staged_count, count) = match header_type { Section::New => (self.new_staged_count, self.new_count), @@ -5227,17 +5144,14 @@ impl GitPanel { .active(|s| s.bg(active_bg)) .child(name_row) .when(GitPanelSettings::get_global(cx).diff_stats, |el| { - el.when_some( - self.diff_stats.get(&entry.repo_path).copied(), - move |this, stat| { - let id = format!("diff-stat-{}", id_for_diff_stat); - this.child(ui::DiffStat::new( - id, - stat.added as usize, - stat.deleted as usize, - )) - }, - ) + el.when_some(entry.diff_stat, move |this, stat| { + let id = format!("diff-stat-{}", id_for_diff_stat); + this.child(ui::DiffStat::new( + id, + stat.added as usize, + stat.deleted as usize, + )) + }) }) .child( div() @@ -5629,6 +5543,21 @@ impl GitPanel { } } +#[cfg(any(test, feature = "test-support"))] +impl GitPanel { + pub fn new_test( + workspace: &mut Workspace, + window: &mut Window, + cx: &mut Context, + ) -> Entity { + Self::new(workspace, window, cx) + } + + pub fn active_repository(&self) -> Option<&Entity> { + self.active_repository.as_ref() + } +} + impl Render for GitPanel { fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { let project = self.project.read(cx); @@ -6606,11 +6535,19 @@ mod tests { repo_path: repo_path("crates/gpui/gpui.rs"), status: StatusCode::Modified.worktree(), staging: StageStatus::Unstaged, + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), }), GitListEntry::Status(GitStatusEntry { repo_path: repo_path("crates/util/util.rs"), status: StatusCode::Modified.worktree(), staging: StageStatus::Unstaged, + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), },), ], ); @@ -6631,11 +6568,19 @@ mod tests { repo_path: repo_path("crates/gpui/gpui.rs"), status: StatusCode::Modified.worktree(), staging: StageStatus::Unstaged, + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), }), GitListEntry::Status(GitStatusEntry { repo_path: repo_path("crates/util/util.rs"), status: StatusCode::Modified.worktree(), staging: StageStatus::Unstaged, + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), },), ], ); diff --git a/crates/lsp/Cargo.toml b/crates/lsp/Cargo.toml index 9533ddb600b18213de4d6e50599c62aa182b9b8a..2c48575a648a9eba12b16ce8edb2cf959d7cc8b3 100644 --- a/crates/lsp/Cargo.toml +++ b/crates/lsp/Cargo.toml @@ -13,12 +13,13 @@ path = "src/lsp.rs" doctest = false [features] -test-support = ["async-pipe"] +test-support = ["async-pipe", "gpui_util"] [dependencies] anyhow.workspace = true async-pipe = { workspace = true, optional = true } collections.workspace = true +gpui_util = { workspace = true, optional = true } futures.workspace = true gpui.workspace = true log.workspace = true @@ -34,6 +35,7 @@ release_channel.workspace = true [dev-dependencies] async-pipe.workspace = true +gpui_util.workspace = true ctor.workspace = true gpui = { workspace = true, features = ["test-support"] } semver.workspace = true diff --git a/crates/lsp/src/lsp.rs b/crates/lsp/src/lsp.rs index e552c21d701cefa8aa1f4b6e14e826892e3b25b6..2e2318065292ffdc2ac39b577afc7a264d36473d 100644 --- a/crates/lsp/src/lsp.rs +++ b/crates/lsp/src/lsp.rs @@ -1970,10 +1970,14 @@ impl FakeLanguageServer { let responded_tx = responded_tx.clone(); let executor = cx.background_executor().clone(); async move { + let _guard = gpui_util::defer({ + let responded_tx = responded_tx.clone(); + move || { + responded_tx.unbounded_send(()).ok(); + } + }); executor.simulate_random_delay().await; - let result = result.await; - responded_tx.unbounded_send(()).ok(); - result + result.await } }) .detach(); diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index b03c7d69ab05daf94254a9d47cb2ae23da3043d1..fdafea73fd0ca797616cc58fc9e4b6a3c2101224 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -24,7 +24,7 @@ use futures::{ mpsc, oneshot::{self, Canceled}, }, - future::{self, Shared}, + future::{self, BoxFuture, Shared}, stream::FuturesOrdered, }; use git::{ @@ -39,8 +39,8 @@ use git::{ }, stash::{GitStash, StashEntry}, status::{ - DiffTreeType, FileStatus, GitSummary, StatusCode, TrackedStatus, TreeDiff, TreeDiffStatus, - UnmergedStatus, UnmergedStatusCode, + self, DiffStat, DiffTreeType, FileStatus, GitSummary, StatusCode, TrackedStatus, TreeDiff, + TreeDiffStatus, UnmergedStatus, UnmergedStatusCode, }, }; use gpui::{ @@ -195,6 +195,7 @@ pub struct GitStoreCheckpoint { pub struct StatusEntry { pub repo_path: RepoPath, pub status: FileStatus, + pub diff_stat: Option, } impl StatusEntry { @@ -216,6 +217,8 @@ impl StatusEntry { repo_path: self.repo_path.to_proto(), simple_status, status: Some(status_to_proto(self.status)), + diff_stat_added: self.diff_stat.map(|ds| ds.added), + diff_stat_deleted: self.diff_stat.map(|ds| ds.deleted), } } } @@ -226,7 +229,15 @@ impl TryFrom for StatusEntry { fn try_from(value: proto::StatusEntry) -> Result { let repo_path = RepoPath::from_proto(&value.repo_path).context("invalid repo path")?; let status = status_from_proto(value.simple_status, value.status)?; - Ok(Self { repo_path, status }) + let diff_stat = match (value.diff_stat_added, value.diff_stat_deleted) { + (Some(added), Some(deleted)) => Some(DiffStat { added, deleted }), + _ => None, + }; + Ok(Self { + repo_path, + status, + diff_stat, + }) } } @@ -555,7 +566,6 @@ impl GitStore { client.add_entity_request_handler(Self::handle_askpass); client.add_entity_request_handler(Self::handle_check_for_pushed_commits); client.add_entity_request_handler(Self::handle_git_diff); - client.add_entity_request_handler(Self::handle_git_diff_stat); client.add_entity_request_handler(Self::handle_tree_diff); client.add_entity_request_handler(Self::handle_get_blob_content); client.add_entity_request_handler(Self::handle_open_unstaged_diff); @@ -2761,45 +2771,6 @@ impl GitStore { Ok(proto::GitDiffResponse { diff }) } - async fn handle_git_diff_stat( - this: Entity, - envelope: TypedEnvelope, - mut cx: AsyncApp, - ) -> Result { - let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); - let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; - let diff_type = match envelope.payload.diff_type() { - proto::git_diff_stat::DiffType::HeadToIndex => DiffType::HeadToIndex, - proto::git_diff_stat::DiffType::HeadToWorktree => DiffType::HeadToWorktree, - proto::git_diff_stat::DiffType::MergeBase => { - let base_ref = envelope - .payload - .merge_base_ref - .ok_or_else(|| anyhow!("merge_base_ref is required for MergeBase diff type"))?; - DiffType::MergeBase { - base_ref: base_ref.into(), - } - } - }; - - let stats = repository_handle - .update(&mut cx, |repository_handle, cx| { - repository_handle.diff_stat(diff_type, cx) - }) - .await??; - - let entries = stats - .into_iter() - .map(|(path, stat)| proto::GitDiffStatEntry { - path: path.to_proto(), - added: stat.added, - deleted: stat.deleted, - }) - .collect(); - - Ok(proto::GitDiffStatResponse { entries }) - } - async fn handle_tree_diff( this: Entity, request: TypedEnvelope, @@ -3623,7 +3594,9 @@ impl RepositorySnapshot { current_new_entry = new_statuses.next(); } Ordering::Equal => { - if new_entry.status != old_entry.status { + if new_entry.status != old_entry.status + || new_entry.diff_stat != old_entry.diff_stat + { updated_statuses.push(new_entry.to_proto()); } current_old_entry = old_statuses.next(); @@ -3693,6 +3666,12 @@ impl RepositorySnapshot { .cloned() } + pub fn diff_stat_for_path(&self, path: &RepoPath) -> Option { + self.statuses_by_path + .get(&PathKey(path.as_ref().clone()), ()) + .and_then(|entry| entry.diff_stat) + } + pub fn abs_path_to_repo_path(&self, abs_path: &Path) -> Option { Self::abs_path_to_repo_path_inner(&self.work_directory_abs_path, abs_path, self.path_style) } @@ -4193,6 +4172,10 @@ impl Repository { self.snapshot.status() } + pub fn diff_stat_for_path(&self, path: &RepoPath) -> Option { + self.snapshot.diff_stat_for_path(path) + } + pub fn cached_stash(&self) -> GitStash { self.snapshot.stash_entries.clone() } @@ -5884,63 +5867,6 @@ impl Repository { }) } - /// Fetches per-line diff statistics (additions/deletions) via `git diff --numstat`. - pub fn diff_stat( - &mut self, - diff_type: DiffType, - _cx: &App, - ) -> oneshot::Receiver< - Result>, - > { - let id = self.id; - self.send_job(None, move |repo, _cx| async move { - match repo { - RepositoryState::Local(LocalRepositoryState { backend, .. }) => { - backend.diff_stat(diff_type).await - } - RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { - let (proto_diff_type, merge_base_ref) = match &diff_type { - DiffType::HeadToIndex => { - (proto::git_diff_stat::DiffType::HeadToIndex.into(), None) - } - DiffType::HeadToWorktree => { - (proto::git_diff_stat::DiffType::HeadToWorktree.into(), None) - } - DiffType::MergeBase { base_ref } => ( - proto::git_diff_stat::DiffType::MergeBase.into(), - Some(base_ref.to_string()), - ), - }; - let response = client - .request(proto::GitDiffStat { - project_id: project_id.0, - repository_id: id.to_proto(), - diff_type: proto_diff_type, - merge_base_ref, - }) - .await?; - - let stats = response - .entries - .into_iter() - .filter_map(|entry| { - let path = RepoPath::from_proto(&entry.path).log_err()?; - Some(( - path, - git::status::DiffStat { - added: entry.added, - deleted: entry.deleted, - }, - )) - }) - .collect(); - - Ok(stats) - } - } - }) - } - pub fn create_branch( &mut self, branch_name: String, @@ -6165,6 +6091,7 @@ impl Repository { cx.emit(RepositoryEvent::StatusesChanged); } self.snapshot.statuses_by_path.edit(edits, ()); + if update.is_last_update { self.snapshot.scan_id = update.scan_id; } @@ -6479,22 +6406,43 @@ impl Repository { return Ok(()); } + let has_head = prev_snapshot.head_commit.is_some(); + let stash_entries = backend.stash_entries().await?; let changed_path_statuses = cx .background_spawn(async move { let mut changed_paths = changed_paths.into_iter().flatten().collect::>(); - let statuses = backend - .status(&changed_paths.iter().cloned().collect::>()) - .await?; + let changed_paths_vec = changed_paths.iter().cloned().collect::>(); + + let status_task = backend.status(&changed_paths_vec); + let diff_stat_future = if has_head { + backend.diff_stat(&changed_paths_vec) + } else { + future::ready(Ok(status::GitDiffStat { + entries: Arc::default(), + })) + .boxed() + }; + + let (statuses, diff_stats) = + futures::future::try_join(status_task, diff_stat_future).await?; + + let diff_stats: HashMap = + HashMap::from_iter(diff_stats.entries.into_iter().cloned()); + let mut changed_path_statuses = Vec::new(); let prev_statuses = prev_snapshot.statuses_by_path.clone(); let mut cursor = prev_statuses.cursor::(()); for (repo_path, status) in &*statuses.entries { + let current_diff_stat = diff_stats.get(repo_path).copied(); + changed_paths.remove(repo_path); if cursor.seek_forward(&PathTarget::Path(repo_path), Bias::Left) - && cursor.item().is_some_and(|entry| entry.status == *status) + && cursor.item().is_some_and(|entry| { + entry.status == *status && entry.diff_stat == current_diff_stat + }) { continue; } @@ -6502,6 +6450,7 @@ impl Repository { changed_path_statuses.push(Edit::Insert(StatusEntry { repo_path: repo_path.clone(), status: *status, + diff_stat: current_diff_stat, })); } let mut cursor = prev_statuses.cursor::(()); @@ -6859,11 +6808,31 @@ async fn compute_snapshot( let mut events = Vec::new(); let branches = backend.branches().await?; let branch = branches.into_iter().find(|branch| branch.is_head); - let statuses = backend - .status(&[RepoPath::from_rel_path( + + // Useful when branch is None in detached head state + let head_commit = match backend.head_sha().await { + Some(head_sha) => backend.show(head_sha).await.log_err(), + None => None, + }; + + let diff_stat_future: BoxFuture<'_, Result> = if head_commit.is_some() { + backend.diff_stat(&[]) + } else { + future::ready(Ok(status::GitDiffStat { + entries: Arc::default(), + })) + .boxed() + }; + let (statuses, diff_stats) = futures::future::try_join( + backend.status(&[RepoPath::from_rel_path( &RelPath::new(".".as_ref(), PathStyle::local()).unwrap(), - )]) - .await?; + )]), + diff_stat_future, + ) + .await?; + + let diff_stat_map: HashMap<&RepoPath, DiffStat> = + diff_stats.entries.iter().map(|(p, s)| (p, *s)).collect(); let stash_entries = backend.stash_entries().await?; let mut conflicted_paths = Vec::new(); let statuses_by_path = SumTree::from_iter( @@ -6874,6 +6843,7 @@ async fn compute_snapshot( StatusEntry { repo_path: repo_path.clone(), status: *status, + diff_stat: diff_stat_map.get(repo_path).copied(), } }), (), @@ -6886,12 +6856,6 @@ async fn compute_snapshot( events.push(RepositoryEvent::StatusesChanged) } - // Useful when branch is None in detached head state - let head_commit = match backend.head_sha().await { - Some(head_sha) => backend.show(head_sha).await.log_err(), - None => None, - }; - if branch != prev_snapshot.branch || head_commit != prev_snapshot.head_commit { events.push(RepositoryEvent::BranchChanged); } diff --git a/crates/project/tests/integration/project_tests.rs b/crates/project/tests/integration/project_tests.rs index d597377910a2a837e456ac4384b06c333887dfb3..d86b969e61ed173ee314cde6f584f2dbab6859f9 100644 --- a/crates/project/tests/integration/project_tests.rs +++ b/crates/project/tests/integration/project_tests.rs @@ -31,7 +31,7 @@ use futures::{StreamExt, future}; use git::{ GitHostingProviderRegistry, repository::{RepoPath, repo_path}, - status::{FileStatus, StatusCode, TrackedStatus}, + status::{DiffStat, FileStatus, StatusCode, TrackedStatus}, }; use git2::RepositoryInitOptions; use gpui::{ @@ -9253,14 +9253,23 @@ async fn test_git_repository_status(cx: &mut gpui::TestAppContext) { StatusEntry { repo_path: repo_path("a.txt"), status: StatusCode::Modified.worktree(), + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), }, StatusEntry { repo_path: repo_path("b.txt"), status: FileStatus::Untracked, + diff_stat: None, }, StatusEntry { repo_path: repo_path("d.txt"), status: StatusCode::Deleted.worktree(), + diff_stat: Some(DiffStat { + added: 0, + deleted: 1, + }), }, ] ); @@ -9282,18 +9291,31 @@ async fn test_git_repository_status(cx: &mut gpui::TestAppContext) { StatusEntry { repo_path: repo_path("a.txt"), status: StatusCode::Modified.worktree(), + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), }, StatusEntry { repo_path: repo_path("b.txt"), status: FileStatus::Untracked, + diff_stat: None, }, StatusEntry { repo_path: repo_path("c.txt"), status: StatusCode::Modified.worktree(), + diff_stat: Some(DiffStat { + added: 1, + deleted: 1, + }), }, StatusEntry { repo_path: repo_path("d.txt"), status: StatusCode::Deleted.worktree(), + diff_stat: Some(DiffStat { + added: 0, + deleted: 1, + }), }, ] ); @@ -9327,6 +9349,10 @@ async fn test_git_repository_status(cx: &mut gpui::TestAppContext) { [StatusEntry { repo_path: repo_path("a.txt"), status: StatusCode::Deleted.worktree(), + diff_stat: Some(DiffStat { + added: 0, + deleted: 1, + }), }] ); }); @@ -9391,6 +9417,7 @@ async fn test_git_status_postprocessing(cx: &mut gpui::TestAppContext) { worktree_status: StatusCode::Added } .into(), + diff_stat: None, }] ) }); @@ -9593,6 +9620,10 @@ async fn test_repository_pending_ops_staging( worktree_status: StatusCode::Unmodified } .into(), + diff_stat: Some(DiffStat { + added: 1, + deleted: 0, + }), }] ); }); @@ -9699,6 +9730,10 @@ async fn test_repository_pending_ops_long_running_staging( worktree_status: StatusCode::Unmodified } .into(), + diff_stat: Some(DiffStat { + added: 1, + deleted: 0, + }), }] ); }); @@ -9823,10 +9858,12 @@ async fn test_repository_pending_ops_stage_all( StatusEntry { repo_path: repo_path("a.txt"), status: FileStatus::Untracked, + diff_stat: None, }, StatusEntry { repo_path: repo_path("b.txt"), status: FileStatus::Untracked, + diff_stat: None, }, ] ); diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index 6cb3acfcd878c8f970c4e99789939424a3835709..736abcdaa49f62d72582750a8a28ea785baee282 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -229,29 +229,6 @@ message GitDiffResponse { string diff = 1; } -message GitDiffStat { - uint64 project_id = 1; - uint64 repository_id = 2; - DiffType diff_type = 3; - optional string merge_base_ref = 4; - - enum DiffType { - HEAD_TO_WORKTREE = 0; - HEAD_TO_INDEX = 1; - MERGE_BASE = 2; - } -} - -message GitDiffStatResponse { - repeated GitDiffStatEntry entries = 1; -} - -message GitDiffStatEntry { - string path = 1; - uint32 added = 2; - uint32 deleted = 3; -} - message GitInit { uint64 project_id = 1; string abs_path = 2; @@ -360,6 +337,8 @@ message StatusEntry { // Can be removed once collab's min version is >=0.171.0. GitStatus simple_status = 2; GitFileStatus status = 3; + optional uint32 diff_stat_added = 4; + optional uint32 diff_stat_deleted = 5; } message StashEntry { diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index d6139f5342d153221d13917e26565a4c0eb5a707..c129b6eff26404b66b38439c29f0b83289b37172 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -474,9 +474,7 @@ message Envelope { SpawnKernel spawn_kernel = 426; SpawnKernelResponse spawn_kernel_response = 427; - KillKernel kill_kernel = 428; - GitDiffStat git_diff_stat = 429; - GitDiffStatResponse git_diff_stat_response = 430; // current max + KillKernel kill_kernel = 428; // current max } reserved 87 to 88; @@ -501,6 +499,7 @@ message Envelope { reserved 280 to 281; reserved 332 to 333; reserved 394 to 396; + reserved 429 to 430; } message Hello { diff --git a/crates/proto/src/proto.rs b/crates/proto/src/proto.rs index 3d30551557000c305a82b328828b566c9d78f75e..dd0a77beb29345021563b21bafd261d02b87e1ab 100644 --- a/crates/proto/src/proto.rs +++ b/crates/proto/src/proto.rs @@ -322,8 +322,6 @@ messages!( (CheckForPushedCommitsResponse, Background), (GitDiff, Background), (GitDiffResponse, Background), - (GitDiffStat, Background), - (GitDiffStatResponse, Background), (GitInit, Background), (GetDebugAdapterBinary, Background), (DebugAdapterBinary, Background), @@ -541,7 +539,6 @@ request_messages!( (GitRenameBranch, Ack), (CheckForPushedCommits, CheckForPushedCommitsResponse), (GitDiff, GitDiffResponse), - (GitDiffStat, GitDiffStatResponse), (GitInit, Ack), (ToggleBreakpoint, Ack), (GetDebugAdapterBinary, DebugAdapterBinary), @@ -730,7 +727,6 @@ entity_messages!( GitRemoveRemote, CheckForPushedCommits, GitDiff, - GitDiffStat, GitInit, BreakpointsForFile, ToggleBreakpoint, diff --git a/crates/remote_server/src/remote_editing_tests.rs b/crates/remote_server/src/remote_editing_tests.rs index 9b9fe9948ace530d7e55d2843952ca5c9efb3749..7f9953c8a4e746d9586b663330badb38149cfb64 100644 --- a/crates/remote_server/src/remote_editing_tests.rs +++ b/crates/remote_server/src/remote_editing_tests.rs @@ -6,7 +6,6 @@ use agent::{AgentTool, ReadFileTool, ReadFileToolInput, ToolCallEventStream, Too use client::{Client, UserStore}; use clock::FakeSystemClock; use collections::{HashMap, HashSet}; -use git::repository::DiffType; use language_model::LanguageModelToolResultContent; use extension::ExtensionHostProxy; @@ -1917,129 +1916,6 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA assert_eq!(server_branch.name(), "totally-new-branch"); } -#[gpui::test] -async fn test_remote_git_diff_stat(cx: &mut TestAppContext, server_cx: &mut TestAppContext) { - let fs = FakeFs::new(server_cx.executor()); - fs.insert_tree( - path!("/code"), - json!({ - "project1": { - ".git": {}, - "src": { - "lib.rs": "line1\nline2\nline3\n", - "new_file.rs": "added1\nadded2\n", - }, - "README.md": "# project 1", - }, - }), - ) - .await; - - let dot_git = Path::new(path!("/code/project1/.git")); - - // HEAD: lib.rs (2 lines), deleted.rs (1 line) - fs.set_head_for_repo( - dot_git, - &[ - ("src/lib.rs", "line1\nold_line2\n".into()), - ("src/deleted.rs", "was_here\n".into()), - ], - "deadbeef", - ); - // Index: lib.rs modified (4 lines), staged_only.rs new (2 lines) - fs.set_index_for_repo( - dot_git, - &[ - ("src/lib.rs", "line1\nold_line2\nline3\nline4\n".into()), - ("src/staged_only.rs", "x\ny\n".into()), - ], - ); - - let (project, _headless) = init_test(&fs, cx, server_cx).await; - let (_worktree, _) = project - .update(cx, |project, cx| { - project.find_or_create_worktree(path!("/code/project1"), true, cx) - }) - .await - .unwrap(); - cx.run_until_parked(); - - let repo_path = |s: &str| git::repository::RepoPath::new(s).unwrap(); - - let repository = project.update(cx, |project, cx| project.active_repository(cx).unwrap()); - - // --- HeadToWorktree --- - let stats = cx - .update(|cx| repository.update(cx, |repo, cx| repo.diff_stat(DiffType::HeadToWorktree, cx))) - .await - .unwrap() - .unwrap(); - - // src/lib.rs: worktree 3 lines vs HEAD 2 lines - let stat = stats.get(&repo_path("src/lib.rs")).expect("src/lib.rs"); - assert_eq!((stat.added, stat.deleted), (3, 2)); - - // src/new_file.rs: only in worktree (2 lines) - let stat = stats - .get(&repo_path("src/new_file.rs")) - .expect("src/new_file.rs"); - assert_eq!((stat.added, stat.deleted), (2, 0)); - - // src/deleted.rs: only in HEAD (1 line) - let stat = stats - .get(&repo_path("src/deleted.rs")) - .expect("src/deleted.rs"); - assert_eq!((stat.added, stat.deleted), (0, 1)); - - // README.md: only in worktree (1 line) - let stat = stats.get(&repo_path("README.md")).expect("README.md"); - assert_eq!((stat.added, stat.deleted), (1, 0)); - - // --- HeadToIndex --- - let stats = cx - .update(|cx| repository.update(cx, |repo, cx| repo.diff_stat(DiffType::HeadToIndex, cx))) - .await - .unwrap() - .unwrap(); - - // src/lib.rs: index 4 lines vs HEAD 2 lines - let stat = stats.get(&repo_path("src/lib.rs")).expect("src/lib.rs"); - assert_eq!((stat.added, stat.deleted), (4, 2)); - - // src/staged_only.rs: only in index (2 lines) - let stat = stats - .get(&repo_path("src/staged_only.rs")) - .expect("src/staged_only.rs"); - assert_eq!((stat.added, stat.deleted), (2, 0)); - - // src/deleted.rs: in HEAD but not in index - let stat = stats - .get(&repo_path("src/deleted.rs")) - .expect("src/deleted.rs"); - assert_eq!((stat.added, stat.deleted), (0, 1)); - - // --- MergeBase (not implemented in FakeGitRepository) --- - let stats = cx - .update(|cx| { - repository.update(cx, |repo, cx| { - repo.diff_stat( - DiffType::MergeBase { - base_ref: "main".into(), - }, - cx, - ) - }) - }) - .await - .unwrap() - .unwrap(); - - assert!( - stats.is_empty(), - "MergeBase diff_stat should return empty from FakeGitRepository" - ); -} - #[gpui::test] async fn test_remote_agent_fs_tool_calls(cx: &mut TestAppContext, server_cx: &mut TestAppContext) { let fs = FakeFs::new(server_cx.executor());