From b1619767b44b18f5371b526c23428b39b5b066ac Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Tue, 21 Apr 2026 17:10:03 -0400 Subject: [PATCH 01/28] Start work on making the commit data handler support remote Co-authored-by: Remco Smits --- crates/project/src/git_store.rs | 134 ++++++++++++++++++++++++-------- 1 file changed, 102 insertions(+), 32 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index d35a13e7df3e55..e7ee2d55a95c2d 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -46,8 +46,8 @@ use git::{ }, }; use gpui::{ - App, AppContext, AsyncApp, Context, Entity, EventEmitter, SharedString, Subscription, Task, - WeakEntity, + App, AppContext, AsyncApp, BackgroundExecutor, Context, Entity, EventEmitter, SharedString, + Subscription, Task, WeakEntity, }; use language::{ Buffer, BufferEvent, Language, LanguageRegistry, @@ -75,7 +75,7 @@ use std::{ Arc, atomic::{self, AtomicU64}, }, - time::Instant, + time::{Duration, Instant}, }; use sum_tree::{Edit, SumTree, TreeMap}; use task::Shell; @@ -312,9 +312,14 @@ struct GraphCommitDataHandler { commit_data_request: smol::channel::Sender, } +/// Represents the handler of a git cat-file --batch process within Zed +/// It's used to lazily fetch commit data as needed (whatever a user is viewing) enum GraphCommitHandlerState { + /// The handler is starting up the process Starting, + /// The handler is open and processing requests Open(GraphCommitDataHandler), + /// The handler closed because it didn't receive any requests in the last 10s Closed, } @@ -5054,28 +5059,99 @@ impl Repository { let background_executor = cx.background_executor().clone(); cx.background_spawn(async move { - let backend = match state.await { - Ok(RepositoryState::Local(LocalRepositoryState { backend, .. })) => backend, - Ok(RepositoryState::Remote(_)) => { - log::error!("commit_data_reader not supported for remote repositories"); - return; + match state.await { + Ok(RepositoryState::Local(LocalRepositoryState { backend, .. })) => { + Self::local_commit_data_reader( + backend, + request_rx, + result_tx, + background_executor, + ) + .await; + } + Ok(RepositoryState::Remote(RemoteRepositoryState { project_id, client })) => { + Self::remote_commit_data_reader( + project_id, + client, + request_rx, + result_tx, + background_executor, + ) + .await; } Err(error) => { log::error!("failed to get repository state: {error}"); return; } }; + }) + .detach(); - let reader = match backend.commit_data_reader() { - Ok(reader) => reader, - Err(error) => { - log::error!("failed to create commit data reader: {error:?}"); - return; + self.graph_commit_data_handler = GraphCommitHandlerState::Open(GraphCommitDataHandler { + _task: foreground_task, + commit_data_request: request_tx_for_handler, + }); + } + + async fn local_commit_data_reader( + backend: Arc, + request_rx: smol::channel::Receiver, + result_tx: smol::channel::Sender<(Oid, GraphCommitData)>, + background_executor: BackgroundExecutor, + ) { + let reader = match backend.commit_data_reader() { + Ok(reader) => reader, + Err(error) => { + log::error!("failed to create commit data reader: {error:?}"); + return; + } + }; + + loop { + let timeout = background_executor.timer(std::time::Duration::from_secs(10)); + + futures::select_biased! { + sha = futures::FutureExt::fuse(request_rx.recv()) => { + let Ok(sha) = sha else { + break; + }; + + match reader.read(sha).await { + Ok(commit_data) => { + if result_tx.send((sha, commit_data)).await.is_err() { + break; + } + } + Err(error) => { + log::error!("failed to read commit data for {sha}: {error:?}"); + } + } } - }; + _ = futures::FutureExt::fuse(timeout) => { + break; + } + } + } + + drop(result_tx); + } + + async fn remote_commit_data_reader( + project_id: ProjectId, + client: AnyProtoClient, + request_rx: smol::channel::Receiver, + result_tx: smol::channel::Sender<(Oid, GraphCommitData)>, + background_executor: BackgroundExecutor, + ) { + loop { + let mut queued_shas = Vec::with_capacity(64); loop { - let timeout = background_executor.timer(std::time::Duration::from_secs(10)); + if queued_shas.len() >= 64 { + break; + } + + let timeout = background_executor.timer(std::time::Duration::from_millis(5)); futures::select_biased! { sha = futures::FutureExt::fuse(request_rx.recv()) => { @@ -5083,16 +5159,8 @@ impl Repository { break; }; - match reader.read(sha).await { - Ok(commit_data) => { - if result_tx.send((sha, commit_data)).await.is_err() { - break; - } - } - Err(error) => { - log::error!("failed to read commit data for {sha}: {error:?}"); - } - } + queued_shas.push(sha); + } _ = futures::FutureExt::fuse(timeout) => { break; @@ -5100,14 +5168,16 @@ impl Repository { } } - drop(result_tx); - }) - .detach(); + if queued_shas.is_empty() { + break; + } - self.graph_commit_data_handler = GraphCommitHandlerState::Open(GraphCommitDataHandler { - _task: foreground_task, - commit_data_request: request_tx_for_handler, - }); + let result = client.request(request).await; + result_tx.send(msg) + + background_executor.timer(Duration::from_millis(2)).await; + + } } fn buffer_store(&self, cx: &App) -> Option> { From 15379cd71ecb85943dd56c0377ce0bffcf03b665 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Tue, 21 Apr 2026 17:24:10 -0400 Subject: [PATCH 02/28] Handle most of remote side of commit data handler Co-authored-by: Remco Smits --- crates/project/src/git_store.rs | 59 +++++++++++++++++++++++++++++++-- crates/proto/proto/git.proto | 19 +++++++++++ crates/proto/proto/zed.proto | 4 ++- crates/proto/src/proto.rs | 4 +++ 4 files changed, 82 insertions(+), 4 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index e7ee2d55a95c2d..9f94affd4728c0 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -62,6 +62,7 @@ use rpc::{ }; use serde::Deserialize; use settings::{Settings, WorktreeId}; +use smallvec::SmallVec; use smol::future::yield_now; use std::{ cmp::Ordering, @@ -5056,6 +5057,7 @@ impl Repository { }); let request_tx_for_handler = request_tx; + let repository_id = self.id; let background_executor = cx.background_executor().clone(); cx.background_spawn(async move { @@ -5073,6 +5075,7 @@ impl Repository { Self::remote_commit_data_reader( project_id, client, + repository_id, request_rx, result_tx, background_executor, @@ -5139,6 +5142,7 @@ impl Repository { async fn remote_commit_data_reader( project_id: ProjectId, client: AnyProtoClient, + repository_id: RepositoryId, request_rx: smol::channel::Receiver, result_tx: smol::channel::Sender<(Oid, GraphCommitData)>, background_executor: BackgroundExecutor, @@ -5172,12 +5176,34 @@ impl Repository { break; } - let result = client.request(request).await; - result_tx.send(msg) + let result = client + .request(proto::GetGraphCommitData { + project_id: project_id.to_proto(), + repository_id: repository_id.to_proto(), + shas: queued_shas.into_iter().map(|oid| oid.to_string()).collect(), + }) + .await; - background_executor.timer(Duration::from_millis(2)).await; + if let Ok(commit_data) = result { + for commit in commit_data.commits { + let Ok(commit_data) = graph_commit_data_from_proto(commit) else { + continue; + }; + if result_tx + .send((commit_data.sha, commit_data)) + .await + .is_err() + { + return; + } + } + } + + background_executor.timer(Duration::from_millis(2)).await; } + + drop(result_tx); } fn buffer_store(&self, cx: &App) -> Option> { @@ -7693,6 +7719,33 @@ fn deserialize_blame_buffer_response( Some(Blame { entries, messages }) } +fn graph_commit_data_to_proto(commit: &GraphCommitData) -> proto::GraphCommitData { + proto::GraphCommitData { + sha: commit.sha.to_string(), + parents: commit.parents.iter().map(|p| p.to_string()).collect(), + author_name: commit.author_name.to_string(), + author_email: commit.author_email.to_string(), + commit_timestamp: commit.commit_timestamp, + subject: commit.subject.to_string(), + } +} + +fn graph_commit_data_from_proto(commit: proto::GraphCommitData) -> Result { + let sha = Oid::from_str(&commit.sha)?; + let mut parents = SmallVec::with_capacity(commit.parents.len()); + for parent in &commit.parents { + parents.push(Oid::from_str(parent)?); + } + Ok(GraphCommitData { + sha, + parents, + author_name: SharedString::from(commit.author_name), + author_email: SharedString::from(commit.author_email), + commit_timestamp: commit.commit_timestamp, + subject: SharedString::from(commit.subject), + }) +} + fn branch_to_proto(branch: &git::repository::Branch) -> proto::Branch { proto::Branch { is_head: branch.is_head, diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index 78f3fb2aea9dec..939b8c9dcbef5d 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -692,3 +692,22 @@ message RunGitHook { uint64 repository_id = 2; GitHook hook = 3; } + +message GetGraphCommitData { + uint64 project_id = 1; + uint64 repository_id = 2; + repeated string shas = 3; +} + +message GraphCommitData { + string sha = 1; + repeated string parents = 2; + string author_name = 3; + string author_email = 4; + int64 commit_timestamp = 5; + string subject = 6; +} + +message GetGraphCommitDataResponse { + repeated GraphCommitData commits = 1; +} diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index 1da7b968922633..5bd4050e6527be 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -481,7 +481,9 @@ message Envelope { GitEditRef git_edit_ref = 443; GitCreateArchiveCheckpoint git_create_archive_checkpoint = 444; GitCreateArchiveCheckpointResponse git_create_archive_checkpoint_response = 445; - GitRestoreArchiveCheckpoint git_restore_archive_checkpoint = 446; // current max + GitRestoreArchiveCheckpoint git_restore_archive_checkpoint = 446; + GetGraphCommitData get_graph_commit_data = 447; + GetGraphCommitDataResponse get_graph_commit_data_response = 448; // current max } reserved 87 to 88; diff --git a/crates/proto/src/proto.rs b/crates/proto/src/proto.rs index 83a559cb283306..d191562f8194ea 100644 --- a/crates/proto/src/proto.rs +++ b/crates/proto/src/proto.rs @@ -358,6 +358,8 @@ messages!( (GitGetHeadShaResponse, Background), (GitEditRef, Background), (GitRepairWorktrees, Background), + (GetGraphCommitData, Background), + (GetGraphCommitDataResponse, Background), (GitWorktreesResponse, Background), (GitCreateWorktree, Background), (GitRemoveWorktree, Background), @@ -573,6 +575,7 @@ request_messages!( (GitGetHeadSha, GitGetHeadShaResponse), (GitEditRef, Ack), (GitRepairWorktrees, Ack), + (GetGraphCommitData, GetGraphCommitDataResponse), (GitCreateWorktree, Ack), (GitRemoveWorktree, Ack), (GitRenameWorktree, Ack), @@ -767,6 +770,7 @@ entity_messages!( GitGetHeadSha, GitEditRef, GitRepairWorktrees, + GetGraphCommitData, GitCreateArchiveCheckpoint, GitRestoreArchiveCheckpoint, GitCreateWorktree, From 80ed0c2ba981749500cf33d2614b0d1915890492 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Tue, 21 Apr 2026 17:46:43 -0400 Subject: [PATCH 03/28] In progress work --- crates/project/src/git_store.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 9f94affd4728c0..f9377340dbfc97 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -601,6 +601,7 @@ impl GitStore { client.add_entity_request_handler(Self::handle_get_head_sha); client.add_entity_request_handler(Self::handle_edit_ref); client.add_entity_request_handler(Self::handle_repair_worktrees); + client.add_entity_request_handler(Self::handle_get_commit_data); } pub fn is_local(&self) -> bool { @@ -2531,6 +2532,23 @@ impl GitStore { Ok(proto::GitGetHeadShaResponse { sha: head_sha }) } + async fn handle_get_commit_data( + 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)?; + + repository_handle + .update(&mut cx, |repository, _| { + repository.fetch_commit_data(sha, cx) + }) + .await??; + + Ok(proto::Ack {}) + } + async fn handle_edit_ref( this: Entity, envelope: TypedEnvelope, From 2181e08b5d34018aa31f377e1e2516faaefb8ea3 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Tue, 21 Apr 2026 20:33:33 -0400 Subject: [PATCH 04/28] Clean up --- crates/git_graph/src/git_graph.rs | 10 +-- crates/project/src/git_store.rs | 103 ++++++++++++++++++++++-------- 2 files changed, 84 insertions(+), 29 deletions(-) diff --git a/crates/git_graph/src/git_graph.rs b/crates/git_graph/src/git_graph.rs index e0175db09f1ef9..2783415bd7cafc 100644 --- a/crates/git_graph/src/git_graph.rs +++ b/crates/git_graph/src/git_graph.rs @@ -1251,7 +1251,7 @@ impl GitGraph { .min(self.graph_data.commits.len().saturating_sub(1))] .iter() .for_each(|commit| { - repository.fetch_commit_data(commit.data.sha, cx); + repository.fetch_commit_data(commit.data.sha, false, cx); }); }); } @@ -1270,7 +1270,9 @@ impl GitGraph { }; let data = repository.update(cx, |repository, cx| { - repository.fetch_commit_data(commit.data.sha, cx).clone() + repository + .fetch_commit_data(commit.data.sha, false, cx) + .clone() }); let short_sha = commit.data.sha.display_short(); @@ -1817,7 +1819,7 @@ impl GitGraph { let data = repository.update(cx, |repository, cx| { repository - .fetch_commit_data(commit_entry.data.sha, cx) + .fetch_commit_data(commit_entry.data.sha, false, cx) .clone() }); @@ -1846,7 +1848,7 @@ impl GitGraph { Some(data.commit_timestamp), data.subject.clone(), ), - CommitDataState::Loading => ("Loading…".into(), "".into(), None, "Loading…".into()), + CommitDataState::Loading(_) => ("Loading…".into(), "".into(), None, "Loading…".into()), }; let date_string = commit_timestamp diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index f9377340dbfc97..e08c95e14db979 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -274,7 +274,7 @@ pub struct MergeDetails { #[derive(Clone)] pub enum CommitDataState { - Loading, + Loading(Option>>>), Loaded(Arc), } @@ -311,16 +311,16 @@ pub struct JobInfo { struct GraphCommitDataHandler { _task: Task<()>, commit_data_request: smol::channel::Sender, + completers: HashMap>>, } /// Represents the handler of a git cat-file --batch process within Zed /// It's used to lazily fetch commit data as needed (whatever a user is viewing) enum GraphCommitHandlerState { - /// The handler is starting up the process - Starting, /// The handler is open and processing requests Open(GraphCommitDataHandler), /// The handler closed because it didn't receive any requests in the last 10s + /// or hasn't been open before Closed, } @@ -2540,13 +2540,36 @@ impl GitStore { let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; - repository_handle - .update(&mut cx, |repository, _| { - repository.fetch_commit_data(sha, cx) - }) - .await??; + let shas: Vec = envelope + .payload + .shas + .iter() + .filter_map(|s| Oid::from_str(s).ok()) + .collect(); - Ok(proto::Ack {}) + let receivers = repository_handle.update(&mut cx, |repository, cx| { + shas.iter() + .filter_map(|&sha| match repository.fetch_commit_data(sha, true, cx) { + CommitDataState::Loading(Some(shared)) => Some(shared.clone()), + CommitDataState::Loaded(data) => { + let (tx, rx) = oneshot::channel(); + tx.send(data.clone()).ok(); + Some(rx.shared()) + } + _ => None, + }) + .collect::>() + }); + + let results = future::join_all(receivers).await; + + let commits = results + .into_iter() + .filter_map(|result| result.ok()) + .map(|data| graph_commit_data_to_proto(&data)) + .collect(); + + Ok(proto::GetGraphCommitDataResponse { commits }) } async fn handle_edit_ref( @@ -5022,30 +5045,48 @@ impl Repository { Ok(()) } - pub fn fetch_commit_data(&mut self, sha: Oid, cx: &mut Context) -> &CommitDataState { + pub fn fetch_commit_data( + &mut self, + sha: Oid, + get_waiter: bool, + cx: &mut Context, + ) -> &CommitDataState { if !self.commit_data.contains_key(&sha) { - match &self.graph_commit_data_handler { + match &mut self.graph_commit_data_handler { GraphCommitHandlerState::Open(handler) => { - if handler.commit_data_request.try_send(sha).is_ok() { - let old_value = self.commit_data.insert(sha, CommitDataState::Loading); - debug_assert!(old_value.is_none(), "We should never overwrite commit data"); + if get_waiter { + let (tx, rx) = oneshot::channel(); + handler.completers.insert(sha, tx); + self.commit_data + .insert(sha, CommitDataState::Loading(Some(rx.shared()))); + } else { + self.commit_data.insert(sha, CommitDataState::Loading(None)); } + + handler.commit_data_request.try_send(sha).ok(); } GraphCommitHandlerState::Closed => { - self.open_graph_commit_data_handler(cx); + let mut handler = self.open_graph_commit_data_handler(cx); + + if get_waiter { + let (tx, rx) = oneshot::channel(); + handler.completers.insert(sha, tx); + self.commit_data + .insert(sha, CommitDataState::Loading(Some(rx.shared()))); + } else { + self.commit_data.insert(sha, CommitDataState::Loading(None)); + } + + handler.commit_data_request.try_send(sha).ok(); + self.graph_commit_data_handler = GraphCommitHandlerState::Open(handler); } - GraphCommitHandlerState::Starting => {} } } - self.commit_data - .get(&sha) - .unwrap_or(&CommitDataState::Loading) + &self.commit_data[&sha] } - fn open_graph_commit_data_handler(&mut self, cx: &mut Context) { - self.graph_commit_data_handler = GraphCommitHandlerState::Starting; - + fn open_graph_commit_data_handler(&self, cx: &Context) -> GraphCommitDataHandler { let state = self.repository_state.clone(); let (result_tx, result_rx) = smol::channel::bounded::<(Oid, GraphCommitData)>(64); let (request_tx, request_rx) = smol::channel::unbounded::(); @@ -5053,14 +5094,25 @@ impl Repository { let foreground_task = cx.spawn(async move |this, cx| { while let Ok((sha, commit_data)) = result_rx.recv().await { let result = this.update(cx, |this, cx| { + let data = Arc::new(commit_data); let old_value = this .commit_data - .insert(sha, CommitDataState::Loaded(Arc::new(commit_data))); + .insert(sha, CommitDataState::Loaded(data.clone())); debug_assert!( !matches!(old_value, Some(CommitDataState::Loaded(_))), "We should never overwrite commit data" ); + if let GraphCommitHandlerState::Open(handler) = + &mut this.graph_commit_data_handler + { + if let Some(completer) = handler.completers.remove(&sha) { + completer.send(data.clone()).ok(); + } + } else { + debug_panic!("The handler state has to be open for this task to exist"); + } + cx.notify(); }); if result.is_err() { @@ -5108,10 +5160,11 @@ impl Repository { }) .detach(); - self.graph_commit_data_handler = GraphCommitHandlerState::Open(GraphCommitDataHandler { + GraphCommitDataHandler { _task: foreground_task, commit_data_request: request_tx_for_handler, - }); + completers: HashMap::default(), + } } async fn local_commit_data_reader( From 28329822b6a20e2f750cd48b35eaf4ff7b92171a Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Tue, 21 Apr 2026 20:41:36 -0400 Subject: [PATCH 05/28] More clean up --- crates/project/src/git_store.rs | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index e08c95e14db979..80a7d44143d1a8 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5052,31 +5052,27 @@ impl Repository { cx: &mut Context, ) -> &CommitDataState { if !self.commit_data.contains_key(&sha) { + let (state, completer) = if get_waiter { + let (tx, rx) = oneshot::channel(); + (CommitDataState::Loading(Some(rx.shared())), Some(tx)) + } else { + (CommitDataState::Loading(None), None) + }; + + self.commit_data.insert(sha, state); + match &mut self.graph_commit_data_handler { GraphCommitHandlerState::Open(handler) => { - if get_waiter { - let (tx, rx) = oneshot::channel(); + if let Some(tx) = completer { handler.completers.insert(sha, tx); - self.commit_data - .insert(sha, CommitDataState::Loading(Some(rx.shared()))); - } else { - self.commit_data.insert(sha, CommitDataState::Loading(None)); } - handler.commit_data_request.try_send(sha).ok(); } GraphCommitHandlerState::Closed => { let mut handler = self.open_graph_commit_data_handler(cx); - - if get_waiter { - let (tx, rx) = oneshot::channel(); + if let Some(tx) = completer { handler.completers.insert(sha, tx); - self.commit_data - .insert(sha, CommitDataState::Loading(Some(rx.shared()))); - } else { - self.commit_data.insert(sha, CommitDataState::Loading(None)); } - handler.commit_data_request.try_send(sha).ok(); self.graph_commit_data_handler = GraphCommitHandlerState::Open(handler); } From df67db6d146d69c7d00dd1318b6d569c69702f2a Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Tue, 21 Apr 2026 20:50:11 -0400 Subject: [PATCH 06/28] Final clean up --- crates/project/src/git_store.rs | 38 ++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 80a7d44143d1a8..7352fcf1800eb1 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -2547,27 +2547,35 @@ impl GitStore { .filter_map(|s| Oid::from_str(s).ok()) .collect(); - let receivers = repository_handle.update(&mut cx, |repository, cx| { - shas.iter() - .filter_map(|&sha| match repository.fetch_commit_data(sha, true, cx) { - CommitDataState::Loading(Some(shared)) => Some(shared.clone()), + let mut commits = Vec::with_capacity(shas.len()); + let mut receivers = Vec::new(); + + repository_handle.update(&mut cx, |repository, cx| { + for &sha in &shas { + match repository.fetch_commit_data(sha, true, cx) { CommitDataState::Loaded(data) => { - let (tx, rx) = oneshot::channel(); - tx.send(data.clone()).ok(); - Some(rx.shared()) + commits.push(graph_commit_data_to_proto(data)); } - _ => None, - }) - .collect::>() + CommitDataState::Loading(Some(shared)) => { + receivers.push(shared.clone()); + } + CommitDataState::Loading(None) => { + debug_panic!( + "This should never happen since we passed true into fetch commit data" + ); + } + } + } }); let results = future::join_all(receivers).await; - let commits = results - .into_iter() - .filter_map(|result| result.ok()) - .map(|data| graph_commit_data_to_proto(&data)) - .collect(); + commits.extend( + results + .into_iter() + .filter_map(|result| result.ok()) + .map(|data| graph_commit_data_to_proto(&data)), + ); Ok(proto::GetGraphCommitDataResponse { commits }) } From dad134de89152484b8870754c863b45e50a4c641 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 00:39:50 -0400 Subject: [PATCH 07/28] Fix clippy errors --- crates/project/src/git_store.rs | 62 ++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 7352fcf1800eb1..7bc5fa1a637938 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5059,35 +5059,40 @@ impl Repository { get_waiter: bool, cx: &mut Context, ) -> &CommitDataState { - if !self.commit_data.contains_key(&sha) { - let (state, completer) = if get_waiter { - let (tx, rx) = oneshot::channel(); - (CommitDataState::Loading(Some(rx.shared())), Some(tx)) - } else { - (CommitDataState::Loading(None), None) - }; + if self.commit_data.contains_key(&sha) { + return &self.commit_data[&sha]; + } + + let (state, completer) = if get_waiter { + let (tx, rx) = oneshot::channel(); + (CommitDataState::Loading(Some(rx.shared())), Some(tx)) + } else { + (CommitDataState::Loading(None), None) + }; - self.commit_data.insert(sha, state); + self.commit_data.insert(sha, state); - match &mut self.graph_commit_data_handler { - GraphCommitHandlerState::Open(handler) => { - if let Some(tx) = completer { - handler.completers.insert(sha, tx); - } - handler.commit_data_request.try_send(sha).ok(); + match &mut self.graph_commit_data_handler { + GraphCommitHandlerState::Open(handler) => { + if let Some(tx) = completer { + handler.completers.insert(sha, tx); } - GraphCommitHandlerState::Closed => { - let mut handler = self.open_graph_commit_data_handler(cx); - if let Some(tx) = completer { - handler.completers.insert(sha, tx); - } - handler.commit_data_request.try_send(sha).ok(); - self.graph_commit_data_handler = GraphCommitHandlerState::Open(handler); + handler.commit_data_request.try_send(sha).ok(); + } + GraphCommitHandlerState::Closed => { + let mut handler = self.open_graph_commit_data_handler(cx); + if let Some(tx) = completer { + handler.completers.insert(sha, tx); } + handler.commit_data_request.try_send(sha).ok(); + self.graph_commit_data_handler = GraphCommitHandlerState::Open(handler); } } - &self.commit_data[&sha] + &self.commit_data.get(&sha).unwrap_or_else(|| { + debug_panic!("This should always be inserted"); + &CommitDataState::Loading(None) + }) } fn open_graph_commit_data_handler(&self, cx: &Context) -> GraphCommitDataHandler { @@ -5099,13 +5104,6 @@ impl Repository { while let Ok((sha, commit_data)) = result_rx.recv().await { let result = this.update(cx, |this, cx| { let data = Arc::new(commit_data); - let old_value = this - .commit_data - .insert(sha, CommitDataState::Loaded(data.clone())); - debug_assert!( - !matches!(old_value, Some(CommitDataState::Loaded(_))), - "We should never overwrite commit data" - ); if let GraphCommitHandlerState::Open(handler) = &mut this.graph_commit_data_handler @@ -5117,6 +5115,12 @@ impl Repository { debug_panic!("The handler state has to be open for this task to exist"); } + let old_value = this.commit_data.insert(sha, CommitDataState::Loaded(data)); + debug_assert!( + !matches!(old_value, Some(CommitDataState::Loaded(_))), + "We should never overwrite commit data" + ); + cx.notify(); }); if result.is_err() { From 752b8599c269330698c3fe7c105781bee6f03aa7 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 01:46:00 -0400 Subject: [PATCH 08/28] Fix fetch commit data bug --- crates/project/src/git_store.rs | 47 ++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 7bc5fa1a637938..836cb24cd29fe1 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5060,6 +5060,19 @@ impl Repository { cx: &mut Context, ) -> &CommitDataState { if self.commit_data.contains_key(&sha) { + let data = &self.commit_data[&sha]; + + if let CommitDataState::Loading(None) = data + && get_waiter + { + let (tx, rx) = oneshot::channel(); + self.commit_data + .insert(sha, CommitDataState::Loading(Some(rx.shared()))); + + let handler = self.get_handler(cx); + handler.completers.insert(sha, tx); + } + return &self.commit_data[&sha]; } @@ -5072,22 +5085,11 @@ impl Repository { self.commit_data.insert(sha, state); - match &mut self.graph_commit_data_handler { - GraphCommitHandlerState::Open(handler) => { - if let Some(tx) = completer { - handler.completers.insert(sha, tx); - } - handler.commit_data_request.try_send(sha).ok(); - } - GraphCommitHandlerState::Closed => { - let mut handler = self.open_graph_commit_data_handler(cx); - if let Some(tx) = completer { - handler.completers.insert(sha, tx); - } - handler.commit_data_request.try_send(sha).ok(); - self.graph_commit_data_handler = GraphCommitHandlerState::Open(handler); - } + let handler = self.get_handler(cx); + if let Some(tx) = completer { + handler.completers.insert(sha, tx); } + handler.commit_data_request.try_send(sha).ok(); &self.commit_data.get(&sha).unwrap_or_else(|| { debug_panic!("This should always be inserted"); @@ -5095,6 +5097,21 @@ impl Repository { }) } + fn get_handler(&mut self, cx: &mut Context) -> &mut GraphCommitDataHandler { + if matches!( + self.graph_commit_data_handler, + GraphCommitHandlerState::Closed + ) { + self.graph_commit_data_handler = + GraphCommitHandlerState::Open(self.open_graph_commit_data_handler(cx)); + } + + match &mut self.graph_commit_data_handler { + GraphCommitHandlerState::Open(handler) => handler, + GraphCommitHandlerState::Closed => unreachable!(), + } + } + fn open_graph_commit_data_handler(&self, cx: &Context) -> GraphCommitDataHandler { let state = self.repository_state.clone(); let (result_tx, result_rx) = smol::channel::bounded::<(Oid, GraphCommitData)>(64); From a415fba9784417ff2e7ad86ecdb707f2ee1fbcdc Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 02:47:39 -0400 Subject: [PATCH 09/28] Make remote commit data reader send requests in parallel --- crates/project/src/git_store.rs | 161 ++++++++++++++++++++++++-------- 1 file changed, 124 insertions(+), 37 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 836cb24cd29fe1..52cd74559fbc19 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -26,7 +26,7 @@ use futures::{ oneshot::{self, Canceled}, }, future::{self, BoxFuture, Shared}, - stream::FuturesOrdered, + stream::{FuturesOrdered, FuturesUnordered}, }; use git::{ BuildPermalinkParams, GitHostingProviderRegistry, Oid, RunHook, @@ -324,6 +324,12 @@ enum GraphCommitHandlerState { Closed, } +enum NextGraphCommitDataRequest { + Request(BoxFuture<'static, Result>), + Idle, + Closed, +} + pub struct InitialGitGraphData { fetch_task: Task<()>, pub error: Option, @@ -5243,63 +5249,144 @@ impl Repository { result_tx: smol::channel::Sender<(Oid, GraphCommitData)>, background_executor: BackgroundExecutor, ) { + let mut response_futures = FuturesUnordered::< + BoxFuture<'static, Result>, + >::new(); + let mut accept_requests = true; + let mut next_request = Self::get_next_request( + project_id, + client.clone(), + repository_id, + &request_rx, + &background_executor, + ) + .boxed() + .fuse(); + loop { - let mut queued_shas = Vec::with_capacity(64); + if !accept_requests && response_futures.is_empty() { + break; + } - loop { - if queued_shas.len() >= 64 { - break; + if response_futures.is_empty() { + match (&mut next_request).await { + NextGraphCommitDataRequest::Request(request) => { + response_futures.push(request); + next_request = Self::get_next_request( + project_id, + client.clone(), + repository_id, + &request_rx, + &background_executor, + ) + .boxed() + .fuse(); + } + NextGraphCommitDataRequest::Idle => {} + NextGraphCommitDataRequest::Closed => break, } + } - let timeout = background_executor.timer(std::time::Duration::from_millis(5)); - - futures::select_biased! { - sha = futures::FutureExt::fuse(request_rx.recv()) => { - let Ok(sha) = sha else { - break; - }; + let next_response = response_futures.next().fuse(); + futures::pin_mut!(next_response); - queued_shas.push(sha); + futures::select_biased! { + request = next_request => { + match request { + NextGraphCommitDataRequest::Request(request) => { + response_futures.push(request); + } + NextGraphCommitDataRequest::Idle => {} + NextGraphCommitDataRequest::Closed => { + accept_requests = false; + } + } + if accept_requests { + next_request = Self::get_next_request( + project_id, + client.clone(), + repository_id, + &request_rx, + &background_executor, + ) + .boxed() + .fuse(); } - _ = futures::FutureExt::fuse(timeout) => { - break; + } + result = next_response => { + let Some(result) = result else { + continue; + }; + + if let Ok(commit_data) = result { + for commit in commit_data.commits { + let Ok(commit_data) = graph_commit_data_from_proto(commit) else { + continue; + }; + + if result_tx + .send((commit_data.sha, commit_data)) + .await + .is_err() + { + return; + } + } } } } + } + + drop(result_tx); + } + + async fn get_next_request( + project_id: ProjectId, + client: AnyProtoClient, + repository_id: RepositoryId, + request_rx: &smol::channel::Receiver, + background_executor: &BackgroundExecutor, + ) -> NextGraphCommitDataRequest { + let mut queued_shas = Vec::with_capacity(64); - if queued_shas.is_empty() { + loop { + if queued_shas.len() >= 64 { break; } - let result = client - .request(proto::GetGraphCommitData { - project_id: project_id.to_proto(), - repository_id: repository_id.to_proto(), - shas: queued_shas.into_iter().map(|oid| oid.to_string()).collect(), - }) - .await; + let timeout = background_executor.timer(Duration::from_millis(5)); - if let Ok(commit_data) = result { - for commit in commit_data.commits { - let Ok(commit_data) = graph_commit_data_from_proto(commit) else { - continue; + futures::select_biased! { + sha = futures::FutureExt::fuse(request_rx.recv()) => { + let Ok(sha) = sha else { + break; }; - if result_tx - .send((commit_data.sha, commit_data)) - .await - .is_err() - { - return; - } + queued_shas.push(sha); + + } + _ = futures::FutureExt::fuse(timeout) => { + break; } } - - background_executor.timer(Duration::from_millis(2)).await; } - drop(result_tx); + if queued_shas.is_empty() && request_rx.is_closed() { + NextGraphCommitDataRequest::Closed + } else if queued_shas.is_empty() { + NextGraphCommitDataRequest::Idle + } else { + NextGraphCommitDataRequest::Request( + client + .request(proto::GetGraphCommitData { + project_id: project_id.to_proto(), + repository_id: repository_id.to_proto(), + shas: queued_shas.into_iter().map(|oid| oid.to_string()).collect(), + }) + .boxed(), + ) + } } fn buffer_store(&self, cx: &App) -> Option> { From b9f1536cfe1797900da6124618e0ff0143b994eb Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 02:52:34 -0400 Subject: [PATCH 10/28] Fix idle case --- crates/project/src/git_store.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 52cd74559fbc19..0f82010c498ded 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5282,8 +5282,7 @@ impl Repository { .boxed() .fuse(); } - NextGraphCommitDataRequest::Idle => {} - NextGraphCommitDataRequest::Closed => break, + NextGraphCommitDataRequest::Closed | NextGraphCommitDataRequest::Idle => break, } } From 9c462dc3c514531ca635661e3c2aa82c01cf6bbe Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 03:06:49 -0400 Subject: [PATCH 11/28] Fix another edge case --- crates/project/src/git_store.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 0f82010c498ded..bf356e1529d1ea 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -312,6 +312,7 @@ struct GraphCommitDataHandler { _task: Task<()>, commit_data_request: smol::channel::Sender, completers: HashMap>>, + pending_requests: HashSet, } /// Represents the handler of a git cat-file --batch process within Zed @@ -5095,7 +5096,11 @@ impl Repository { if let Some(tx) = completer { handler.completers.insert(sha, tx); } - handler.commit_data_request.try_send(sha).ok(); + if handler.commit_data_request.try_send(sha).is_ok() { + handler.pending_requests.insert(sha); + } else { + handler.completers.remove(&sha); + } &self.commit_data.get(&sha).unwrap_or_else(|| { debug_panic!("This should always be inserted"); @@ -5131,6 +5136,7 @@ impl Repository { if let GraphCommitHandlerState::Open(handler) = &mut this.graph_commit_data_handler { + handler.pending_requests.remove(&sha); if let Some(completer) = handler.completers.remove(&sha) { completer.send(data.clone()).ok(); } @@ -5152,7 +5158,17 @@ impl Repository { } this.update(cx, |this, _cx| { - this.graph_commit_data_handler = GraphCommitHandlerState::Closed; + let GraphCommitHandlerState::Open(handler) = std::mem::replace( + &mut this.graph_commit_data_handler, + GraphCommitHandlerState::Closed, + ) else { + debug_panic!("The handler state has to be open for this task to exist"); + return; + }; + + for sha in handler.pending_requests { + this.commit_data.remove(&sha); + } }) .ok(); }); @@ -5195,6 +5211,7 @@ impl Repository { _task: foreground_task, commit_data_request: request_tx_for_handler, completers: HashMap::default(), + pending_requests: HashSet::default(), } } From a85af5fede5459936ce8e88e3d5cb21346b29f52 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 03:17:58 -0400 Subject: [PATCH 12/28] Fix some more edge cases --- crates/project/src/git_store.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index bf356e1529d1ea..ff2d063d95e239 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5063,14 +5063,14 @@ impl Repository { pub fn fetch_commit_data( &mut self, sha: Oid, - get_waiter: bool, + needs_waiter: bool, cx: &mut Context, ) -> &CommitDataState { if self.commit_data.contains_key(&sha) { let data = &self.commit_data[&sha]; if let CommitDataState::Loading(None) = data - && get_waiter + && needs_waiter { let (tx, rx) = oneshot::channel(); self.commit_data @@ -5083,7 +5083,7 @@ impl Repository { return &self.commit_data[&sha]; } - let (state, completer) = if get_waiter { + let (state, completer) = if needs_waiter { let (tx, rx) = oneshot::channel(); (CommitDataState::Loading(Some(rx.shared())), Some(tx)) } else { @@ -5096,14 +5096,23 @@ impl Repository { if let Some(tx) = completer { handler.completers.insert(sha, tx); } + let mut has_failed = false; if handler.commit_data_request.try_send(sha).is_ok() { handler.pending_requests.insert(sha); } else { + has_failed = true; handler.completers.remove(&sha); + debug_assert!( + matches!( + self.commit_data.remove(&sha), + Some(CommitDataState::Loading(_)) + ), + "Commit data should still be loading when enqueueing the request fails" + ); } &self.commit_data.get(&sha).unwrap_or_else(|| { - debug_panic!("This should always be inserted"); + debug_assert!(!has_failed, "This should always be inserted"); &CommitDataState::Loading(None) }) } From c99c7e0fb1b942b3aab13b1ba713df0f6291b264 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 04:03:31 -0400 Subject: [PATCH 13/28] Start adding property tests to commit_data --- crates/fs/src/fake_git_repo.rs | 27 +++++++- crates/git/src/repository.rs | 22 +++++++ crates/project/src/git_store.rs | 107 ++++++++++++++++++++++++++++++++ plan.md | 92 +++++++++++++++++++++++++++ 4 files changed, 245 insertions(+), 3 deletions(-) create mode 100644 plan.md diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index ca796c1d8376e6..692db1e4ff52f4 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -11,8 +11,8 @@ use git::{ repository::{ AskPassDelegate, Branch, CommitDataReader, CommitDetails, CommitOptions, CreateWorktreeTarget, FetchOptions, GRAPH_CHUNK_SIZE, GitRepository, - GitRepositoryCheckpoint, InitialGraphCommitData, LogOrder, LogSource, PushOptions, RefEdit, - Remote, RepoPath, ResetMode, SearchCommitArgs, Worktree, + GitRepositoryCheckpoint, GraphCommitData, InitialGraphCommitData, LogOrder, LogSource, + PushOptions, RefEdit, Remote, RepoPath, ResetMode, SearchCommitArgs, Worktree, }, stash::GitStash, status::{ @@ -1452,7 +1452,28 @@ impl GitRepository for FakeGitRepository { } fn commit_data_reader(&self) -> Result { - anyhow::bail!("commit_data_reader not supported for FakeGitRepository") + let fs = self.fs.clone(); + let dot_git_path = self.dot_git_path.clone(); + let executor = self.executor.clone(); + Ok(CommitDataReader::for_test(executor, move |sha| { + fs.with_git_state(&dot_git_path, false, |state| { + let commit = state + .graph_commits + .iter() + .find(|commit| commit.sha == sha) + .context(format!("graph commit data not found for {sha}"))?; + + // todo! we should have a random name and email so we can assert that this is equal properly + Ok(GraphCommitData { + sha: commit.sha, + parents: commit.parents.clone(), + author_name: SharedString::from("Test User"), + author_email: SharedString::from("test@example.com"), + commit_timestamp: 0, + subject: SharedString::from(commit.sha.to_string()), + }) + })? + })) } fn update_ref(&self, ref_name: String, commit: String) -> BoxFuture<'_, Result<()>> { diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 3056d91007694e..818720b19278a6 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -107,6 +107,7 @@ pub struct GraphCommitData { pub author_email: SharedString, pub commit_timestamp: i64, pub subject: SharedString, + // todo! we should add message as a field here } #[derive(Debug)] @@ -137,6 +138,27 @@ impl CommitDataReader { .await .map_err(|_| anyhow!("commit data reader task dropped response"))? } + + #[cfg(any(test, feature = "test-support"))] + pub fn for_test( + executor: BackgroundExecutor, + resolve: impl 'static + Send + Sync + Fn(Oid) -> Result, + ) -> Self { + let (request_tx, request_rx) = smol::channel::bounded::(64); + let resolve = Arc::new(resolve); + let delay_executor = executor.clone(); + let task = executor.spawn(async move { + while let Ok(CommitDataRequest { sha, response_tx }) = request_rx.recv().await { + delay_executor.simulate_random_delay().await; + response_tx.send(resolve(sha)).ok(); + } + }); + + Self { + request_tx, + _task: task, + } + } } fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index ff2d063d95e239..f7aa32c4b4cfc2 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -2567,6 +2567,7 @@ impl GitStore { receivers.push(shared.clone()); } CommitDataState::Loading(None) => { + // todo! this could happen if the request fails debug_panic!( "This should never happen since we passed true into fetch commit data" ); @@ -8064,6 +8065,112 @@ fn proto_to_commit_details(proto: &proto::GitCommitDetails) -> CommitDetails { } } +#[cfg(test)] +mod tests { + use super::*; + use crate::Project; + use fs::FakeFs; + use gpui::TestAppContext; + use gpui::proptest::prelude::*; + use rand::{SeedableRng, rngs::StdRng}; + use serde_json::json; + use settings::SettingsStore; + use std::path::Path; + + fn init_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + }); + } + + fn verify_invariants(repository: &Repository) -> anyhow::Result<()> { + let GraphCommitHandlerState::Open(handler) = &repository.graph_commit_data_handler else { + return Ok(()); + }; + + for (sha, state) in &repository.commit_data { + if matches!(state, CommitDataState::Loading(_)) { + anyhow::ensure!( + handler.pending_requests.contains(sha), + "loading commit data for {sha} must be tracked in pending_requests" + ); + } + } + + Ok(()) + } + + #[gpui::property_test(config = ProptestConfig { + cases: 20, + ..Default::default() + })] + async fn test_commit_data_random_invariants( + #[strategy = any::()] seed: u64, + #[strategy = gpui::proptest::collection::vec(0usize..2000, 1..200)] commit_indexes: Vec< + usize, + >, + #[strategy = gpui::proptest::collection::vec(any::(), 1..200)] needs_waiters: Vec< + bool, + >, + cx: &mut TestAppContext, + ) { + init_test(cx); + let mut rng = StdRng::seed_from_u64(seed); + + let commit_shas = (0..2000).map(|_| Oid::random(&mut rng)).collect::>(); + let commits = commit_shas + .iter() + .map(|sha| { + Arc::new(InitialGraphCommitData { + sha: *sha, + parents: SmallVec::new(), + ref_names: Vec::new(), + }) + }) + .collect::>(); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + Path::new("/project"), + json!({ + ".git": {}, + "file.txt": "content", + }), + ) + .await; + fs.set_graph_commits(Path::new("/project/.git"), commits); + + let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; + project + .update(cx, |project, cx| project.git_scans_complete(cx)) + .await; + + let repository = project.read_with(cx, |project, cx| { + project + .active_repository(cx) + .expect("should have a repository") + }); + + for (step, commit_index) in commit_indexes.into_iter().enumerate() { + let sha = commit_shas[commit_index % commit_shas.len()]; + let needs_waiter = needs_waiters[step % needs_waiters.len()]; + + repository.update(cx, |repository, cx| { + repository.fetch_commit_data(sha, needs_waiter, cx); + let result = verify_invariants(repository); + if let Err(error) = result { + panic!( + "commit data invariant violation after step {} for sha {}: {error:#}", + step + 1, + sha, + ); + } + }); + } + } +} + /// This snapshot computes the repository state on the foreground thread while /// running the git commands on the background thread. We update branch, head, /// remotes, and worktrees first so the UI can react sooner, then compute file diff --git a/plan.md b/plan.md new file mode 100644 index 00000000000000..aea901c1954301 --- /dev/null +++ b/plan.md @@ -0,0 +1,92 @@ +# Property test plan for git graph commit data loading + +## Goal + +Add randomized state-machine tests around `git_store` commit-data loading so we can validate handler lifecycle, pending request bookkeeping, and remote/host consistency. + +## Test style + +Use randomized state-machine / operation-sequence tests instead of generating arbitrary maps directly. + +That keeps the tested states reachable and lets us assert invariants after every step. + +## Operations to randomize + +Start with a small operation set: + +- Fetch commit data without a waiter +- Fetch commit data with a waiter +- Successfully enqueue a request +- Fail to enqueue a request +- Deliver a commit-data result +- Close the handler +- Reopen the handler +- For remote cases, deliver host-side loaded data to the remote client + +## Core invariants + +### Open-handler invariants + +When the handler is `Open`: + +- For all `sha` where `commit_data[sha] == Loading(_)`, `pending_requests.contains(sha)` must be true. +- For all `sha` where `commit_data[sha] == Loading(Some(_))`, `completers.contains_key(sha)` must be true. +- For all `sha` in `pending_requests`, `commit_data[sha]` must exist and be `Loading(_)`. +- For all `sha` in `completers`, `commit_data[sha]` must exist and be `Loading(Some(_))`. +- `completers.keys()` must be a subset of `pending_requests`. +- For all `sha` where `commit_data[sha] == Loading(None)`, `completers.contains_key(sha)` must be false. +- For all `sha` where `commit_data[sha] == Loaded(_)`, `pending_requests.contains(sha)` must be false. +- For all `sha` where `commit_data[sha] == Loaded(_)`, `completers.contains_key(sha)` must be false. + +### Closed-handler invariants + +When the handler is `Closed`: + +- `commit_data` must contain no `Loading(_)` entries. +- No pending request bookkeeping should survive the close transition. + +## Transition / postcondition checks + +### Result delivery + +If a result is delivered for `sha` while the handler is `Open`, afterwards: + +- `commit_data[sha] == Loaded(_)` +- `pending_requests.contains(sha)` is false +- `completers.contains_key(sha)` is false + +### Successful enqueue + +After a successful enqueue of `sha`: + +- `commit_data[sha]` exists and is `Loading(_)` +- `pending_requests.contains(sha)` is true +- if the request was waiter-backed, `commit_data[sha] == Loading(Some(_))` +- if the request was waiter-backed, `completers.contains_key(sha)` is true + +### Handler close + +Right after a handler close: + +- any `sha` that was still pending has been removed from `commit_data` +- no `Loading(_)` entries remain in `commit_data` + +## Remote / host consistency property + +For all loaded commit-data entries in a remote client, the host must also have those same entries as loaded. + +More concretely: + +- if the remote side has `commit_data[sha] == Loaded(data)` +- then the host side must also have `commit_data[sha] == Loaded(host_data)` +- and the loaded host entry must correspond to the same `sha` + +If we want to strengthen this later, we can also assert that the loaded payload fields match exactly, not just that both sides are loaded for the same `sha`. + +## Possible future property + +Once enqueue-failure semantics are finalized, add a property around waiter-backed requests: + +- calling `fetch_commit_data(..., needs_waiter = true, ...)` should never leave the system in a state where that `sha` is `Loading(None)` + +This one depends on the final failure / retry policy, so it can wait until that behavior is settled. From 022bc3a8d0ab4ee4ab09bf2158401cc9c88e433f Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 10:58:47 -0400 Subject: [PATCH 14/28] Add more verification testing for commit data --- crates/project/src/git_store.rs | 72 +++++++++++++++++++++++++++------ plan.md | 27 +++++++++++++ 2 files changed, 86 insertions(+), 13 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index f7aa32c4b4cfc2..8b9ebd7a0a36bd 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -311,7 +311,7 @@ pub struct JobInfo { struct GraphCommitDataHandler { _task: Task<()>, commit_data_request: smol::channel::Sender, - completers: HashMap>>, + completion_senders: HashMap>>, pending_requests: HashSet, } @@ -5064,27 +5064,27 @@ impl Repository { pub fn fetch_commit_data( &mut self, sha: Oid, - needs_waiter: bool, + await_result: bool, cx: &mut Context, ) -> &CommitDataState { if self.commit_data.contains_key(&sha) { let data = &self.commit_data[&sha]; if let CommitDataState::Loading(None) = data - && needs_waiter + && await_result { let (tx, rx) = oneshot::channel(); self.commit_data .insert(sha, CommitDataState::Loading(Some(rx.shared()))); let handler = self.get_handler(cx); - handler.completers.insert(sha, tx); + handler.completion_senders.insert(sha, tx); } return &self.commit_data[&sha]; } - let (state, completer) = if needs_waiter { + let (state, completer) = if await_result { let (tx, rx) = oneshot::channel(); (CommitDataState::Loading(Some(rx.shared())), Some(tx)) } else { @@ -5095,14 +5095,14 @@ impl Repository { let handler = self.get_handler(cx); if let Some(tx) = completer { - handler.completers.insert(sha, tx); + handler.completion_senders.insert(sha, tx); } let mut has_failed = false; if handler.commit_data_request.try_send(sha).is_ok() { handler.pending_requests.insert(sha); } else { has_failed = true; - handler.completers.remove(&sha); + handler.completion_senders.remove(&sha); debug_assert!( matches!( self.commit_data.remove(&sha), @@ -5147,8 +5147,8 @@ impl Repository { &mut this.graph_commit_data_handler { handler.pending_requests.remove(&sha); - if let Some(completer) = handler.completers.remove(&sha) { - completer.send(data.clone()).ok(); + if let Some(completion_sender) = handler.completion_senders.remove(&sha) { + completion_sender.send(data.clone()).ok(); } } else { debug_panic!("The handler state has to be open for this task to exist"); @@ -5220,7 +5220,7 @@ impl Repository { GraphCommitDataHandler { _task: foreground_task, commit_data_request: request_tx_for_handler, - completers: HashMap::default(), + completion_senders: HashMap::default(), pending_requests: HashSet::default(), } } @@ -8085,6 +8085,13 @@ mod tests { } fn verify_invariants(repository: &Repository) -> anyhow::Result<()> { + verify_loading_entries_are_pending(repository)?; + verify_await_result_loading_entries_have_completion_senders(repository)?; + verify_closed_handler_has_no_loading_entries(repository)?; + Ok(()) + } + + fn verify_loading_entries_are_pending(repository: &Repository) -> anyhow::Result<()> { let GraphCommitHandlerState::Open(handler) = &repository.graph_commit_data_handler else { return Ok(()); }; @@ -8101,6 +8108,43 @@ mod tests { Ok(()) } + fn verify_await_result_loading_entries_have_completion_senders( + repository: &Repository, + ) -> anyhow::Result<()> { + let GraphCommitHandlerState::Open(handler) = &repository.graph_commit_data_handler else { + return Ok(()); + }; + + for (sha, state) in &repository.commit_data { + if matches!(state, CommitDataState::Loading(Some(_))) { + anyhow::ensure!( + handler.completion_senders.contains_key(sha), + "await-result loading commit data for {sha} must have a completion sender" + ); + } + } + + Ok(()) + } + + fn verify_closed_handler_has_no_loading_entries(repository: &Repository) -> anyhow::Result<()> { + if !matches!( + repository.graph_commit_data_handler, + GraphCommitHandlerState::Closed + ) { + return Ok(()); + } + + for (sha, state) in &repository.commit_data { + anyhow::ensure!( + !matches!(state, CommitDataState::Loading(_)), + "closed handler must not keep loading commit data for {sha}" + ); + } + + Ok(()) + } + #[gpui::property_test(config = ProptestConfig { cases: 20, ..Default::default() @@ -8110,7 +8154,7 @@ mod tests { #[strategy = gpui::proptest::collection::vec(0usize..2000, 1..200)] commit_indexes: Vec< usize, >, - #[strategy = gpui::proptest::collection::vec(any::(), 1..200)] needs_waiters: Vec< + #[strategy = gpui::proptest::collection::vec(any::(), 1..200)] await_results: Vec< bool, >, cx: &mut TestAppContext, @@ -8154,10 +8198,10 @@ mod tests { for (step, commit_index) in commit_indexes.into_iter().enumerate() { let sha = commit_shas[commit_index % commit_shas.len()]; - let needs_waiter = needs_waiters[step % needs_waiters.len()]; + let await_result = await_results[step % await_results.len()]; repository.update(cx, |repository, cx| { - repository.fetch_commit_data(sha, needs_waiter, cx); + repository.fetch_commit_data(sha, await_result, cx); let result = verify_invariants(repository); if let Err(error) = result { panic!( @@ -8167,6 +8211,8 @@ mod tests { ); } }); + + // todo! wait until park, and the repository should have random shas we want to fetcg } } } diff --git a/plan.md b/plan.md index aea901c1954301..ed38a50d4385cf 100644 --- a/plan.md +++ b/plan.md @@ -83,6 +83,33 @@ More concretely: If we want to strengthen this later, we can also assert that the loaded payload fields match exactly, not just that both sides are loaded for the same `sha`. +## Duplicate SHA / waiter upgrade invariants + +When `fetch_commit_data` is called multiple times for the same SHA, possibly with different `needs_waiter` values: + +### Same SHA, `needs_waiter = false` then `needs_waiter = false` + +- Second call is a no-op; state remains unchanged. +- `commit_data[sha]` is still `Loading(None)` (or `Loaded` if the result arrived between calls). + +### Same SHA, `needs_waiter = false` then `needs_waiter = true` + +- The state must upgrade from `Loading(None)` to `Loading(Some(_))`. +- A completer must be inserted into `handler.completers` for that SHA. +- The shared future in `Loading(Some(_))` must be resolvable by that completer. +- If the result has already arrived (`Loaded`), the second call should return the loaded state directly. + +### Same SHA, `needs_waiter = true` then `needs_waiter = true` + +- Second call should return the existing `Loading(Some(shared))` — the same shared future. +- No additional completer should be created. +- Both callers awaiting the shared future should resolve to the same data. + +### Same SHA, `needs_waiter = true` then `needs_waiter = false` + +- Second call is a no-op; the existing `Loading(Some(_))` state is preserved. +- The completer and shared future remain intact. + ## Possible future property Once enqueue-failure semantics are finalized, add a property around waiter-backed requests: From 1ff448321800eb64cbbbad7b315e31924581c249 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 11:26:32 -0400 Subject: [PATCH 15/28] Simulate failures --- crates/fs/src/fake_git_repo.rs | 28 ++++++++++++++++------------ crates/fs/src/fs.rs | 27 +++++++++++++++++++++++++-- crates/project/src/git_store.rs | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 692db1e4ff52f4..751cd9254e629e 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -47,6 +47,12 @@ pub struct FakeCommitSnapshot { pub sha: String, } +#[derive(Debug, Clone)] +pub enum FakeGraphCommitDataEntry { + Success(GraphCommitData), + Fail(GraphCommitData), +} + #[derive(Debug, Clone)] pub struct FakeGitRepositoryState { pub commit_history: Vec, @@ -67,6 +73,7 @@ pub struct FakeGitRepositoryState { pub simulated_graph_error: Option, pub refs: HashMap, pub graph_commits: Vec>, + pub commit_data: HashMap, pub stash_entries: GitStash, } @@ -88,6 +95,7 @@ impl FakeGitRepositoryState { oids: Default::default(), remotes: HashMap::default(), graph_commits: Vec::new(), + commit_data: Default::default(), commit_history: Vec::new(), stash_entries: Default::default(), } @@ -1458,20 +1466,16 @@ impl GitRepository for FakeGitRepository { Ok(CommitDataReader::for_test(executor, move |sha| { fs.with_git_state(&dot_git_path, false, |state| { let commit = state - .graph_commits - .iter() - .find(|commit| commit.sha == sha) + .commit_data + .get(&sha) .context(format!("graph commit data not found for {sha}"))?; - // todo! we should have a random name and email so we can assert that this is equal properly - Ok(GraphCommitData { - sha: commit.sha, - parents: commit.parents.clone(), - author_name: SharedString::from("Test User"), - author_email: SharedString::from("test@example.com"), - commit_timestamp: 0, - subject: SharedString::from(commit.sha.to_string()), - }) + match commit { + FakeGraphCommitDataEntry::Success(data) => Ok(data.clone()), + FakeGraphCommitDataEntry::Fail(_) => { + bail!("simulated commit data read failure for {sha}") + } + } })? })) } diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index fa42c436f1b9be..6cab608f06549d 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -53,10 +53,10 @@ mod fake_git_repo; #[cfg(feature = "test-support")] use collections::{BTreeMap, btree_map}; #[cfg(feature = "test-support")] -use fake_git_repo::FakeGitRepositoryState; +use fake_git_repo::{FakeGitRepositoryState, FakeGraphCommitDataEntry}; #[cfg(feature = "test-support")] use git::{ - repository::{InitialGraphCommitData, RepoPath, Worktree, repo_path}, + repository::{GraphCommitData, InitialGraphCommitData, RepoPath, Worktree, repo_path}, status::{FileStatus, StatusCode, TrackedStatus, UnmergedStatus}, }; #[cfg(feature = "test-support")] @@ -2212,6 +2212,29 @@ impl FakeFs { .unwrap(); } + pub fn set_commit_data( + &self, + dot_git: &Path, + commit_data: impl IntoIterator, + ) { + self.with_git_state(dot_git, true, |state| { + state.commit_data = commit_data + .into_iter() + .map(|(data, should_fail)| { + ( + data.sha, + if should_fail { + FakeGraphCommitDataEntry::Fail(data) + } else { + FakeGraphCommitDataEntry::Success(data) + }, + ) + }) + .collect(); + }) + .unwrap(); + } + /// Put the given git repository into a state with the given status, /// by mutating the head, index, and unmerged state. pub fn set_status_for_repo(&self, dot_git: &Path, statuses: &[(&str, FileStatus)]) { diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 8b9ebd7a0a36bd..4600077bc51583 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8157,6 +8157,12 @@ mod tests { #[strategy = gpui::proptest::collection::vec(any::(), 1..200)] await_results: Vec< bool, >, + #[strategy = gpui::proptest::collection::vec(0usize..2000, 0..200)] failing_commit_indexes: Vec< + usize, + >, + #[strategy = gpui::proptest::collection::vec(0usize..2000, 0..200)] missing_commit_indexes: Vec< + usize, + >, cx: &mut TestAppContext, ) { init_test(cx); @@ -8173,6 +8179,31 @@ mod tests { }) }) .collect::>(); + let failing_shas = failing_commit_indexes + .into_iter() + .map(|index| commit_shas[index % commit_shas.len()]) + .collect::>(); + let missing_shas = missing_commit_indexes + .into_iter() + .map(|index| commit_shas[index % commit_shas.len()]) + .collect::>(); + let commit_data = commit_shas + .iter() + .filter(|sha| !missing_shas.contains(sha)) + .map(|sha| { + ( + GraphCommitData { + sha: *sha, + parents: SmallVec::new(), + author_name: SharedString::from(format!("Author {sha}")), + author_email: SharedString::from(format!("{sha}@example.com")), + commit_timestamp: rng.random_range(0..10_000), + subject: SharedString::from(format!("Subject {sha}")), + }, + failing_shas.contains(sha), + ) + }) + .collect::>(); let fs = FakeFs::new(cx.executor()); fs.insert_tree( @@ -8184,6 +8215,7 @@ mod tests { ) .await; fs.set_graph_commits(Path::new("/project/.git"), commits); + fs.set_commit_data(Path::new("/project/.git"), commit_data); let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; project From 869cf207e1e54449e4f6e68164e1f14f982ed641 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 11:43:24 -0400 Subject: [PATCH 16/28] Clean up again --- crates/project/src/git_store.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 4600077bc51583..92c99446b1c5af 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8169,16 +8169,6 @@ mod tests { let mut rng = StdRng::seed_from_u64(seed); let commit_shas = (0..2000).map(|_| Oid::random(&mut rng)).collect::>(); - let commits = commit_shas - .iter() - .map(|sha| { - Arc::new(InitialGraphCommitData { - sha: *sha, - parents: SmallVec::new(), - ref_names: Vec::new(), - }) - }) - .collect::>(); let failing_shas = failing_commit_indexes .into_iter() .map(|index| commit_shas[index % commit_shas.len()]) @@ -8214,7 +8204,6 @@ mod tests { }), ) .await; - fs.set_graph_commits(Path::new("/project/.git"), commits); fs.set_commit_data(Path::new("/project/.git"), commit_data); let project = Project::test(fs.clone(), [Path::new("/project")], cx).await; From 71a3d54a040ce55e6f861f0d9a73456ebc49cbe7 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 11:56:34 -0400 Subject: [PATCH 17/28] More clean up --- crates/project/src/git_store.rs | 36 +++++++++++++++------------------ 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 92c99446b1c5af..ea2c2f063abd40 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8085,17 +8085,23 @@ mod tests { } fn verify_invariants(repository: &Repository) -> anyhow::Result<()> { - verify_loading_entries_are_pending(repository)?; - verify_await_result_loading_entries_have_completion_senders(repository)?; - verify_closed_handler_has_no_loading_entries(repository)?; + match &repository.graph_commit_data_handler { + GraphCommitHandlerState::Open(handler) => { + verify_loading_entries_are_pending(repository, handler)?; + verify_await_result_loading_entries_have_completion_senders(repository, handler)?; + } + GraphCommitHandlerState::Closed => { + verify_closed_handler_invariants(repository)?; + } + } + Ok(()) } - fn verify_loading_entries_are_pending(repository: &Repository) -> anyhow::Result<()> { - let GraphCommitHandlerState::Open(handler) = &repository.graph_commit_data_handler else { - return Ok(()); - }; - + fn verify_loading_entries_are_pending( + repository: &Repository, + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loading(_)) { anyhow::ensure!( @@ -8110,11 +8116,8 @@ mod tests { fn verify_await_result_loading_entries_have_completion_senders( repository: &Repository, + handler: &GraphCommitDataHandler, ) -> anyhow::Result<()> { - let GraphCommitHandlerState::Open(handler) = &repository.graph_commit_data_handler else { - return Ok(()); - }; - for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loading(Some(_))) { anyhow::ensure!( @@ -8127,14 +8130,7 @@ mod tests { Ok(()) } - fn verify_closed_handler_has_no_loading_entries(repository: &Repository) -> anyhow::Result<()> { - if !matches!( - repository.graph_commit_data_handler, - GraphCommitHandlerState::Closed - ) { - return Ok(()); - } - + fn verify_closed_handler_invariants(repository: &Repository) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { anyhow::ensure!( !matches!(state, CommitDataState::Loading(_)), From 65bdaad012b5d63812d8b9b28c2f8b897a0486aa Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 12:05:35 -0400 Subject: [PATCH 18/28] Add error checks --- crates/project/src/git_store.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index ea2c2f063abd40..0151f69700659d 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8213,20 +8213,30 @@ mod tests { .expect("should have a repository") }); + cx.update(|cx| { + cx.observe(&repository, |repo, cx| { + verify_invariants(repo.read(cx)) + .context("Invariant weren't held after a cx.notify") + .unwrap(); + }) + }) + .detach(); + for (step, commit_index) in commit_indexes.into_iter().enumerate() { let sha = commit_shas[commit_index % commit_shas.len()]; let await_result = await_results[step % await_results.len()]; repository.update(cx, |repository, cx| { repository.fetch_commit_data(sha, await_result, cx); - let result = verify_invariants(repository); - if let Err(error) = result { - panic!( - "commit data invariant violation after step {} for sha {}: {error:#}", - step + 1, - sha, - ); - } + verify_invariants(repository) + .with_context(|| { + format!( + "commit data invariant violation after step {} for sha {}", + step + 1, + sha, + ) + }) + .unwrap(); }); // todo! wait until park, and the repository should have random shas we want to fetcg From da357cf6390274765be42810517a1c67b08a0134 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 12:12:39 -0400 Subject: [PATCH 19/28] Improve test again --- crates/project/src/git_store.rs | 68 +++++++++++++++++++++++++-------- 1 file changed, 52 insertions(+), 16 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 0151f69700659d..fee74539a3335a 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8141,6 +8141,16 @@ mod tests { Ok(()) } + fn verify_repository_invariants( + repository: &Entity, + context: impl FnOnce() -> String, + cx: &mut TestAppContext, + ) { + repository.read_with(cx, |repository, _cx| { + verify_invariants(repository).with_context(context).unwrap(); + }); + } + #[gpui::property_test(config = ProptestConfig { cases: 20, ..Default::default() @@ -8222,25 +8232,51 @@ mod tests { }) .detach(); - for (step, commit_index) in commit_indexes.into_iter().enumerate() { - let sha = commit_shas[commit_index % commit_shas.len()]; - let await_result = await_results[step % await_results.len()]; + let mut next_step = 0; + while next_step < commit_indexes.len() { + let remaining_steps = commit_indexes.len() - next_step; + let chunk_size = rng.random_range(1..=remaining_steps.min(16)); + let chunk_end = next_step + chunk_size; + + for step in next_step..chunk_end { + let sha = commit_shas[commit_indexes[step] % commit_shas.len()]; + let await_result = await_results[step % await_results.len()]; + + repository.update(cx, |repository, cx| { + repository.fetch_commit_data(sha, await_result, cx); + verify_invariants(repository) + .with_context(|| { + format!( + "commit data invariant violation after step {} for sha {}", + step + 1, + sha, + ) + }) + .unwrap(); + }); + } - repository.update(cx, |repository, cx| { - repository.fetch_commit_data(sha, await_result, cx); - verify_invariants(repository) - .with_context(|| { - format!( - "commit data invariant violation after step {} for sha {}", - step + 1, - sha, - ) - }) - .unwrap(); - }); + cx.run_until_parked(); + verify_repository_invariants( + &repository, + || { + format!( + "commit data invariant violation after draining through step {}", + chunk_end, + ) + }, + cx, + ); - // todo! wait until park, and the repository should have random shas we want to fetcg + next_step = chunk_end; } + + cx.run_until_parked(); + verify_repository_invariants( + &repository, + || "commit data invariant violation after final drain".to_string(), + cx, + ); } } From c85e26b279839c656fc9415f57213c0cbfbbf269 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 12:17:32 -0400 Subject: [PATCH 20/28] Add rest of invariants --- crates/project/src/git_store.rs | 143 ++++++++++++++++++++++++++------ 1 file changed, 118 insertions(+), 25 deletions(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index fee74539a3335a..807724c8925765 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8089,6 +8089,14 @@ mod tests { GraphCommitHandlerState::Open(handler) => { verify_loading_entries_are_pending(repository, handler)?; verify_await_result_loading_entries_have_completion_senders(repository, handler)?; + verify_pending_requests_are_loading(repository, handler)?; + verify_completion_senders_are_await_result_loading(repository, handler)?; + verify_completion_senders_are_pending(handler)?; + verify_non_await_result_loading_entries_have_no_completion_sender( + repository, handler, + )?; + verify_loaded_entries_are_not_pending(repository, handler)?; + verify_loaded_entries_have_no_completion_sender(repository, handler)?; } GraphCommitHandlerState::Closed => { verify_closed_handler_invariants(repository)?; @@ -8130,6 +8138,101 @@ mod tests { Ok(()) } + fn verify_pending_requests_are_loading( + repository: &Repository, + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { + for sha in &handler.pending_requests { + anyhow::ensure!( + matches!( + repository.commit_data.get(sha), + Some(CommitDataState::Loading(_)) + ), + "pending request for {sha} must correspond to loading commit data" + ); + } + + Ok(()) + } + + fn verify_completion_senders_are_await_result_loading( + repository: &Repository, + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { + for sha in handler.completion_senders.keys() { + anyhow::ensure!( + matches!( + repository.commit_data.get(sha), + Some(CommitDataState::Loading(Some(_))) + ), + "completion sender for {sha} must correspond to await-result loading commit data" + ); + } + + Ok(()) + } + + fn verify_completion_senders_are_pending( + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { + for sha in handler.completion_senders.keys() { + anyhow::ensure!( + handler.pending_requests.contains(sha), + "completion sender for {sha} must also be tracked as pending" + ); + } + + Ok(()) + } + + fn verify_non_await_result_loading_entries_have_no_completion_sender( + repository: &Repository, + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { + for (sha, state) in &repository.commit_data { + if matches!(state, CommitDataState::Loading(None)) { + anyhow::ensure!( + !handler.completion_senders.contains_key(sha), + "non-await-result loading commit data for {sha} must not have a completion sender" + ); + } + } + + Ok(()) + } + + fn verify_loaded_entries_are_not_pending( + repository: &Repository, + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { + for (sha, state) in &repository.commit_data { + if matches!(state, CommitDataState::Loaded(_)) { + anyhow::ensure!( + !handler.pending_requests.contains(sha), + "loaded commit data for {sha} must not still be pending" + ); + } + } + + Ok(()) + } + + fn verify_loaded_entries_have_no_completion_sender( + repository: &Repository, + handler: &GraphCommitDataHandler, + ) -> anyhow::Result<()> { + for (sha, state) in &repository.commit_data { + if matches!(state, CommitDataState::Loaded(_)) { + anyhow::ensure!( + !handler.completion_senders.contains_key(sha), + "loaded commit data for {sha} must not keep a completion sender" + ); + } + } + + Ok(()) + } + fn verify_closed_handler_invariants(repository: &Repository) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { anyhow::ensure!( @@ -8141,16 +8244,6 @@ mod tests { Ok(()) } - fn verify_repository_invariants( - repository: &Entity, - context: impl FnOnce() -> String, - cx: &mut TestAppContext, - ) { - repository.read_with(cx, |repository, _cx| { - verify_invariants(repository).with_context(context).unwrap(); - }); - } - #[gpui::property_test(config = ProptestConfig { cases: 20, ..Default::default() @@ -8257,26 +8350,26 @@ mod tests { } cx.run_until_parked(); - verify_repository_invariants( - &repository, - || { - format!( - "commit data invariant violation after draining through step {}", - chunk_end, - ) - }, - cx, - ); + repository.read_with(cx, |repository, _cx| { + verify_invariants(repository) + .with_context(|| { + format!( + "commit data invariant violation after draining through step {}", + chunk_end, + ) + }) + .unwrap(); + }); next_step = chunk_end; } cx.run_until_parked(); - verify_repository_invariants( - &repository, - || "commit data invariant violation after final drain".to_string(), - cx, - ); + repository.read_with(cx, |repository, _cx| { + verify_invariants(repository) + .with_context(|| "commit data invariant violation after final drain".to_string()) + .unwrap(); + }); } } From 5dc4cc04b4ad124007661d5cebeec6313b7b20d5 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 12:24:46 -0400 Subject: [PATCH 21/28] Finalize local property test --- crates/project/src/git_store.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 807724c8925765..c728d3148f8267 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -8293,6 +8293,11 @@ mod tests { ) }) .collect::>(); + let expected_loaded_shas = commit_indexes + .iter() + .map(|index| commit_shas[index % commit_shas.len()]) + .filter(|sha| !failing_shas.contains(sha) && !missing_shas.contains(sha)) + .collect::>(); let fs = FakeFs::new(cx.executor()); fs.insert_tree( @@ -8369,6 +8374,29 @@ mod tests { verify_invariants(repository) .with_context(|| "commit data invariant violation after final drain".to_string()) .unwrap(); + + let loaded_shas = repository + .commit_data + .iter() + .filter_map(|(sha, state)| match state { + CommitDataState::Loaded(_) => Some(*sha), + CommitDataState::Loading(_) => None, + }) + .collect::>(); + let missing_loaded_shas = expected_loaded_shas + .difference(&loaded_shas) + .copied() + .collect::>(); + let unexpected_loaded_shas = loaded_shas + .difference(&expected_loaded_shas) + .copied() + .collect::>(); + assert!( + missing_loaded_shas.is_empty() && unexpected_loaded_shas.is_empty(), + "loaded commit data SHAs after final drain did not match expectation. missing: {:?}, unexpected: {:?}", + missing_loaded_shas, + unexpected_loaded_shas, + ); }); } } From 0d752f4b4473f7d594825ecab65c005eba7d5c0f Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 12:31:24 -0400 Subject: [PATCH 22/28] Add message field to commit data --- crates/git/src/repository.rs | 11 ++++++++--- crates/project/src/git_store.rs | 3 +++ crates/proto/proto/git.proto | 1 + 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 818720b19278a6..5a3ef645dbd419 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -107,7 +107,7 @@ pub struct GraphCommitData { pub author_email: SharedString, pub commit_timestamp: i64, pub subject: SharedString, - // todo! we should add message as a field here + pub message: SharedString, } #[derive(Debug)] @@ -168,6 +168,7 @@ fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { let mut commit_timestamp = 0i64; let mut in_headers = true; let mut subject = None; + let mut message_lines = Vec::new(); for line in content.lines() { if in_headers { @@ -194,8 +195,11 @@ fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { } } } - } else if subject.is_none() { - subject = Some(SharedString::from(line.to_string())); + } else { + if subject.is_none() { + subject = Some(SharedString::from(line.to_string())); + } + message_lines.push(line); } } @@ -206,6 +210,7 @@ fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { author_email, commit_timestamp, subject: subject.unwrap_or_default(), + message: SharedString::from(message_lines.join("\n")), }) } diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 1f64125bedeee1..e091d937c88ce6 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -7938,6 +7938,7 @@ fn graph_commit_data_to_proto(commit: &GraphCommitData) -> proto::GraphCommitDat author_email: commit.author_email.to_string(), commit_timestamp: commit.commit_timestamp, subject: commit.subject.to_string(), + message: commit.message.to_string(), } } @@ -7954,6 +7955,7 @@ fn graph_commit_data_from_proto(commit: proto::GraphCommitData) -> Result Date: Wed, 22 Apr 2026 12:40:11 -0400 Subject: [PATCH 23/28] Rename graph commit data to commit data --- crates/fs/src/fake_git_repo.rs | 18 ++--- crates/fs/src/fs.rs | 10 +-- crates/git/src/repository.rs | 14 ++-- crates/project/src/git_store.rs | 136 +++++++++++++++----------------- crates/proto/proto/git.proto | 8 +- crates/proto/proto/zed.proto | 4 +- crates/proto/src/proto.rs | 8 +- 7 files changed, 95 insertions(+), 103 deletions(-) diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 751cd9254e629e..f47a57ff3a9e3f 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -9,10 +9,10 @@ use git::{ Oid, RunHook, blame::Blame, repository::{ - AskPassDelegate, Branch, CommitDataReader, CommitDetails, CommitOptions, + AskPassDelegate, Branch, CommitData, CommitDataReader, CommitDetails, CommitOptions, CreateWorktreeTarget, FetchOptions, GRAPH_CHUNK_SIZE, GitRepository, - GitRepositoryCheckpoint, GraphCommitData, InitialGraphCommitData, LogOrder, LogSource, - PushOptions, RefEdit, Remote, RepoPath, ResetMode, SearchCommitArgs, Worktree, + GitRepositoryCheckpoint, InitialGraphCommitData, LogOrder, LogSource, PushOptions, RefEdit, + Remote, RepoPath, ResetMode, SearchCommitArgs, Worktree, }, stash::GitStash, status::{ @@ -48,9 +48,9 @@ pub struct FakeCommitSnapshot { } #[derive(Debug, Clone)] -pub enum FakeGraphCommitDataEntry { - Success(GraphCommitData), - Fail(GraphCommitData), +pub enum FakeCommitDataEntry { + Success(CommitData), + Fail(CommitData), } #[derive(Debug, Clone)] @@ -73,7 +73,7 @@ pub struct FakeGitRepositoryState { pub simulated_graph_error: Option, pub refs: HashMap, pub graph_commits: Vec>, - pub commit_data: HashMap, + pub commit_data: HashMap, pub stash_entries: GitStash, } @@ -1471,8 +1471,8 @@ impl GitRepository for FakeGitRepository { .context(format!("graph commit data not found for {sha}"))?; match commit { - FakeGraphCommitDataEntry::Success(data) => Ok(data.clone()), - FakeGraphCommitDataEntry::Fail(_) => { + FakeCommitDataEntry::Success(data) => Ok(data.clone()), + FakeCommitDataEntry::Fail(_) => { bail!("simulated commit data read failure for {sha}") } } diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 6cab608f06549d..6694b19e373859 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -53,10 +53,10 @@ mod fake_git_repo; #[cfg(feature = "test-support")] use collections::{BTreeMap, btree_map}; #[cfg(feature = "test-support")] -use fake_git_repo::{FakeGitRepositoryState, FakeGraphCommitDataEntry}; +use fake_git_repo::{FakeCommitDataEntry, FakeGitRepositoryState}; #[cfg(feature = "test-support")] use git::{ - repository::{GraphCommitData, InitialGraphCommitData, RepoPath, Worktree, repo_path}, + repository::{CommitData, InitialGraphCommitData, RepoPath, Worktree, repo_path}, status::{FileStatus, StatusCode, TrackedStatus, UnmergedStatus}, }; #[cfg(feature = "test-support")] @@ -2215,7 +2215,7 @@ impl FakeFs { pub fn set_commit_data( &self, dot_git: &Path, - commit_data: impl IntoIterator, + commit_data: impl IntoIterator, ) { self.with_git_state(dot_git, true, |state| { state.commit_data = commit_data @@ -2224,9 +2224,9 @@ impl FakeFs { ( data.sha, if should_fail { - FakeGraphCommitDataEntry::Fail(data) + FakeCommitDataEntry::Fail(data) } else { - FakeGraphCommitDataEntry::Success(data) + FakeCommitDataEntry::Success(data) }, ) }) diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 5a3ef645dbd419..e914460fe6a082 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -99,7 +99,7 @@ pub fn original_repo_path_from_common_dir(common_dir: &Path) -> Option /// Commit data needed for the git graph visualization. #[derive(Debug, Clone)] -pub struct GraphCommitData { +pub struct CommitData { pub sha: Oid, /// Most commits have a single parent, so we use a SmallVec to avoid allocations. pub parents: SmallVec<[Oid; 1]>, @@ -119,7 +119,7 @@ pub struct InitialGraphCommitData { struct CommitDataRequest { sha: Oid, - response_tx: oneshot::Sender>, + response_tx: oneshot::Sender>, } pub struct CommitDataReader { @@ -128,7 +128,7 @@ pub struct CommitDataReader { } impl CommitDataReader { - pub async fn read(&self, sha: Oid) -> Result { + pub async fn read(&self, sha: Oid) -> Result { let (response_tx, response_rx) = oneshot::channel(); self.request_tx .send(CommitDataRequest { sha, response_tx }) @@ -142,7 +142,7 @@ impl CommitDataReader { #[cfg(any(test, feature = "test-support"))] pub fn for_test( executor: BackgroundExecutor, - resolve: impl 'static + Send + Sync + Fn(Oid) -> Result, + resolve: impl 'static + Send + Sync + Fn(Oid) -> Result, ) -> Self { let (request_tx, request_rx) = smol::channel::bounded::(64); let resolve = Arc::new(resolve); @@ -161,7 +161,7 @@ impl CommitDataReader { } } -fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { +fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { let mut parents = SmallVec::new(); let mut author_name = SharedString::default(); let mut author_email = SharedString::default(); @@ -203,7 +203,7 @@ fn parse_cat_file_commit(sha: Oid, content: &str) -> Option { } } - Some(GraphCommitData { + Some(CommitData { sha, parents, author_name, @@ -3237,7 +3237,7 @@ async fn run_commit_data_reader( async fn read_single_commit_response( stdout: &mut R, sha: &Oid, -) -> Result { +) -> Result { let mut header_bytes = Vec::new(); stdout.read_until(b'\n', &mut header_bytes).await?; let header_line = String::from_utf8_lossy(&header_bytes); diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index e091d937c88ce6..f5d357b391b4d6 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -33,9 +33,9 @@ use git::{ blame::Blame, parse_git_remote_url, repository::{ - Branch, CommitDetails, CommitDiff, CommitFile, CommitOptions, CreateWorktreeTarget, - DiffType, FetchOptions, GitCommitTemplate, GitRepository, GitRepositoryCheckpoint, - GraphCommitData, InitialGraphCommitData, LogOrder, LogSource, PushOptions, Remote, + Branch, CommitData, CommitDetails, CommitDiff, CommitFile, CommitOptions, + CreateWorktreeTarget, DiffType, FetchOptions, GitCommitTemplate, GitRepository, + GitRepositoryCheckpoint, InitialGraphCommitData, LogOrder, LogSource, PushOptions, Remote, RemoteCommandOutput, RepoPath, ResetMode, SearchCommitArgs, UpstreamTrackingStatus, Worktree as GitWorktree, }, @@ -274,8 +274,8 @@ pub struct MergeDetails { #[derive(Clone)] pub enum CommitDataState { - Loading(Option>>>), - Loaded(Arc), + Loading(Option>>>), + Loaded(Arc), } #[derive(Clone, Debug, PartialEq, Eq)] @@ -308,25 +308,25 @@ pub struct JobInfo { pub message: SharedString, } -struct GraphCommitDataHandler { +struct CommitDataHandler { _task: Task<()>, commit_data_request: smol::channel::Sender, - completion_senders: HashMap>>, + completion_senders: HashMap>>, pending_requests: HashSet, } /// Represents the handler of a git cat-file --batch process within Zed /// It's used to lazily fetch commit data as needed (whatever a user is viewing) -enum GraphCommitHandlerState { +enum CommitDataHandlerState { /// The handler is open and processing requests - Open(GraphCommitDataHandler), + Open(CommitDataHandler), /// The handler closed because it didn't receive any requests in the last 10s /// or hasn't been open before Closed, } -enum NextGraphCommitDataRequest { - Request(BoxFuture<'static, Result>), +enum NextCommitDataRequest { + Request(BoxFuture<'static, Result>), Idle, Closed, } @@ -360,7 +360,7 @@ pub struct Repository { latest_askpass_id: u64, repository_state: Shared>>, initial_graph_data: HashMap<(LogSource, LogOrder), InitialGitGraphData>, - graph_commit_data_handler: GraphCommitHandlerState, + commit_data_handler: CommitDataHandlerState, commit_data: HashMap, } @@ -2541,9 +2541,9 @@ impl GitStore { async fn handle_get_commit_data( this: Entity, - envelope: TypedEnvelope, + envelope: TypedEnvelope, mut cx: AsyncApp, - ) -> Result { + ) -> Result { let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; @@ -2561,7 +2561,7 @@ impl GitStore { for &sha in &shas { match repository.fetch_commit_data(sha, true, cx) { CommitDataState::Loaded(data) => { - commits.push(graph_commit_data_to_proto(data)); + commits.push(commit_data_to_proto(data)); } CommitDataState::Loading(Some(shared)) => { receivers.push(shared.clone()); @@ -2582,10 +2582,10 @@ impl GitStore { results .into_iter() .filter_map(|result| result.ok()) - .map(|data| graph_commit_data_to_proto(&data)), + .map(|data| commit_data_to_proto(&data)), ); - Ok(proto::GetGraphCommitDataResponse { commits }) + Ok(proto::GetCommitDataResponse { commits }) } async fn handle_edit_ref( @@ -4299,7 +4299,7 @@ impl Repository { active_jobs: Default::default(), initial_graph_data: Default::default(), commit_data: Default::default(), - graph_commit_data_handler: GraphCommitHandlerState::Closed, + commit_data_handler: CommitDataHandlerState::Closed, } } @@ -4337,7 +4337,7 @@ impl Repository { job_id: 0, initial_graph_data: Default::default(), commit_data: Default::default(), - graph_commit_data_handler: GraphCommitHandlerState::Closed, + commit_data_handler: CommitDataHandlerState::Closed, } } @@ -5120,24 +5120,21 @@ impl Repository { }) } - fn get_handler(&mut self, cx: &mut Context) -> &mut GraphCommitDataHandler { - if matches!( - self.graph_commit_data_handler, - GraphCommitHandlerState::Closed - ) { - self.graph_commit_data_handler = - GraphCommitHandlerState::Open(self.open_graph_commit_data_handler(cx)); + fn get_handler(&mut self, cx: &mut Context) -> &mut CommitDataHandler { + if matches!(self.commit_data_handler, CommitDataHandlerState::Closed) { + self.commit_data_handler = + CommitDataHandlerState::Open(self.open_commit_data_handler(cx)); } - match &mut self.graph_commit_data_handler { - GraphCommitHandlerState::Open(handler) => handler, - GraphCommitHandlerState::Closed => unreachable!(), + match &mut self.commit_data_handler { + CommitDataHandlerState::Open(handler) => handler, + CommitDataHandlerState::Closed => unreachable!(), } } - fn open_graph_commit_data_handler(&self, cx: &Context) -> GraphCommitDataHandler { + fn open_commit_data_handler(&self, cx: &Context) -> CommitDataHandler { let state = self.repository_state.clone(); - let (result_tx, result_rx) = smol::channel::bounded::<(Oid, GraphCommitData)>(64); + let (result_tx, result_rx) = smol::channel::bounded::<(Oid, CommitData)>(64); let (request_tx, request_rx) = smol::channel::unbounded::(); let foreground_task = cx.spawn(async move |this, cx| { @@ -5145,9 +5142,7 @@ impl Repository { let result = this.update(cx, |this, cx| { let data = Arc::new(commit_data); - if let GraphCommitHandlerState::Open(handler) = - &mut this.graph_commit_data_handler - { + if let CommitDataHandlerState::Open(handler) = &mut this.commit_data_handler { handler.pending_requests.remove(&sha); if let Some(completion_sender) = handler.completion_senders.remove(&sha) { completion_sender.send(data.clone()).ok(); @@ -5170,9 +5165,9 @@ impl Repository { } this.update(cx, |this, _cx| { - let GraphCommitHandlerState::Open(handler) = std::mem::replace( - &mut this.graph_commit_data_handler, - GraphCommitHandlerState::Closed, + let CommitDataHandlerState::Open(handler) = std::mem::replace( + &mut this.commit_data_handler, + CommitDataHandlerState::Closed, ) else { debug_panic!("The handler state has to be open for this task to exist"); return; @@ -5219,7 +5214,7 @@ impl Repository { }) .detach(); - GraphCommitDataHandler { + CommitDataHandler { _task: foreground_task, commit_data_request: request_tx_for_handler, completion_senders: HashMap::default(), @@ -5230,7 +5225,7 @@ impl Repository { async fn local_commit_data_reader( backend: Arc, request_rx: smol::channel::Receiver, - result_tx: smol::channel::Sender<(Oid, GraphCommitData)>, + result_tx: smol::channel::Sender<(Oid, CommitData)>, background_executor: BackgroundExecutor, ) { let reader = match backend.commit_data_reader() { @@ -5275,12 +5270,11 @@ impl Repository { client: AnyProtoClient, repository_id: RepositoryId, request_rx: smol::channel::Receiver, - result_tx: smol::channel::Sender<(Oid, GraphCommitData)>, + result_tx: smol::channel::Sender<(Oid, CommitData)>, background_executor: BackgroundExecutor, ) { - let mut response_futures = FuturesUnordered::< - BoxFuture<'static, Result>, - >::new(); + let mut response_futures = + FuturesUnordered::>>::new(); let mut accept_requests = true; let mut next_request = Self::get_next_request( project_id, @@ -5299,7 +5293,7 @@ impl Repository { if response_futures.is_empty() { match (&mut next_request).await { - NextGraphCommitDataRequest::Request(request) => { + NextCommitDataRequest::Request(request) => { response_futures.push(request); next_request = Self::get_next_request( project_id, @@ -5311,7 +5305,7 @@ impl Repository { .boxed() .fuse(); } - NextGraphCommitDataRequest::Closed | NextGraphCommitDataRequest::Idle => break, + NextCommitDataRequest::Closed | NextCommitDataRequest::Idle => break, } } @@ -5321,11 +5315,11 @@ impl Repository { futures::select_biased! { request = next_request => { match request { - NextGraphCommitDataRequest::Request(request) => { + NextCommitDataRequest::Request(request) => { response_futures.push(request); } - NextGraphCommitDataRequest::Idle => {} - NextGraphCommitDataRequest::Closed => { + NextCommitDataRequest::Idle => {} + NextCommitDataRequest::Closed => { accept_requests = false; } } @@ -5349,7 +5343,7 @@ impl Repository { if let Ok(commit_data) = result { for commit in commit_data.commits { - let Ok(commit_data) = graph_commit_data_from_proto(commit) else { + let Ok(commit_data) = commit_data_from_proto(commit) else { continue; }; @@ -5375,7 +5369,7 @@ impl Repository { repository_id: RepositoryId, request_rx: &smol::channel::Receiver, background_executor: &BackgroundExecutor, - ) -> NextGraphCommitDataRequest { + ) -> NextCommitDataRequest { let mut queued_shas = Vec::with_capacity(64); loop { @@ -5401,13 +5395,13 @@ impl Repository { } if queued_shas.is_empty() && request_rx.is_closed() { - NextGraphCommitDataRequest::Closed + NextCommitDataRequest::Closed } else if queued_shas.is_empty() { - NextGraphCommitDataRequest::Idle + NextCommitDataRequest::Idle } else { - NextGraphCommitDataRequest::Request( + NextCommitDataRequest::Request( client - .request(proto::GetGraphCommitData { + .request(proto::GetCommitData { project_id: project_id.to_proto(), repository_id: repository_id.to_proto(), shas: queued_shas.into_iter().map(|oid| oid.to_string()).collect(), @@ -7930,8 +7924,8 @@ fn deserialize_blame_buffer_response( Some(Blame { entries, messages }) } -fn graph_commit_data_to_proto(commit: &GraphCommitData) -> proto::GraphCommitData { - proto::GraphCommitData { +fn commit_data_to_proto(commit: &CommitData) -> proto::CommitData { + proto::CommitData { sha: commit.sha.to_string(), parents: commit.parents.iter().map(|p| p.to_string()).collect(), author_name: commit.author_name.to_string(), @@ -7942,13 +7936,13 @@ fn graph_commit_data_to_proto(commit: &GraphCommitData) -> proto::GraphCommitDat } } -fn graph_commit_data_from_proto(commit: proto::GraphCommitData) -> Result { +fn commit_data_from_proto(commit: proto::CommitData) -> Result { let sha = Oid::from_str(&commit.sha)?; let mut parents = SmallVec::with_capacity(commit.parents.len()); for parent in &commit.parents { parents.push(Oid::from_str(parent)?); } - Ok(GraphCommitData { + Ok(CommitData { sha, parents, author_name: SharedString::from(commit.author_name), @@ -8089,8 +8083,8 @@ mod tests { } fn verify_invariants(repository: &Repository) -> anyhow::Result<()> { - match &repository.graph_commit_data_handler { - GraphCommitHandlerState::Open(handler) => { + match &repository.commit_data_handler { + CommitDataHandlerState::Open(handler) => { verify_loading_entries_are_pending(repository, handler)?; verify_await_result_loading_entries_have_completion_senders(repository, handler)?; verify_pending_requests_are_loading(repository, handler)?; @@ -8102,7 +8096,7 @@ mod tests { verify_loaded_entries_are_not_pending(repository, handler)?; verify_loaded_entries_have_no_completion_sender(repository, handler)?; } - GraphCommitHandlerState::Closed => { + CommitDataHandlerState::Closed => { verify_closed_handler_invariants(repository)?; } } @@ -8112,7 +8106,7 @@ mod tests { fn verify_loading_entries_are_pending( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loading(_)) { @@ -8128,7 +8122,7 @@ mod tests { fn verify_await_result_loading_entries_have_completion_senders( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loading(Some(_))) { @@ -8144,7 +8138,7 @@ mod tests { fn verify_pending_requests_are_loading( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for sha in &handler.pending_requests { anyhow::ensure!( @@ -8161,7 +8155,7 @@ mod tests { fn verify_completion_senders_are_await_result_loading( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for sha in handler.completion_senders.keys() { anyhow::ensure!( @@ -8176,9 +8170,7 @@ mod tests { Ok(()) } - fn verify_completion_senders_are_pending( - handler: &GraphCommitDataHandler, - ) -> anyhow::Result<()> { + fn verify_completion_senders_are_pending(handler: &CommitDataHandler) -> anyhow::Result<()> { for sha in handler.completion_senders.keys() { anyhow::ensure!( handler.pending_requests.contains(sha), @@ -8191,7 +8183,7 @@ mod tests { fn verify_non_await_result_loading_entries_have_no_completion_sender( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loading(None)) { @@ -8207,7 +8199,7 @@ mod tests { fn verify_loaded_entries_are_not_pending( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loaded(_)) { @@ -8223,7 +8215,7 @@ mod tests { fn verify_loaded_entries_have_no_completion_sender( repository: &Repository, - handler: &GraphCommitDataHandler, + handler: &CommitDataHandler, ) -> anyhow::Result<()> { for (sha, state) in &repository.commit_data { if matches!(state, CommitDataState::Loaded(_)) { @@ -8285,7 +8277,7 @@ mod tests { .filter(|sha| !missing_shas.contains(sha)) .map(|sha| { ( - GraphCommitData { + CommitData { sha: *sha, parents: SmallVec::new(), author_name: SharedString::from(format!("Author {sha}")), diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index 3169e3ddbd582d..bb851ddbd3b3b7 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -693,13 +693,13 @@ message RunGitHook { GitHook hook = 3; } -message GetGraphCommitData { +message GetCommitData { uint64 project_id = 1; uint64 repository_id = 2; repeated string shas = 3; } -message GraphCommitData { +message CommitData { string sha = 1; repeated string parents = 2; string author_name = 3; @@ -709,6 +709,6 @@ message GraphCommitData { string message = 7; } -message GetGraphCommitDataResponse { - repeated GraphCommitData commits = 1; +message GetCommitDataResponse { + repeated CommitData commits = 1; } diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index 5bd4050e6527be..568c29435066c8 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -482,8 +482,8 @@ message Envelope { GitCreateArchiveCheckpoint git_create_archive_checkpoint = 444; GitCreateArchiveCheckpointResponse git_create_archive_checkpoint_response = 445; GitRestoreArchiveCheckpoint git_restore_archive_checkpoint = 446; - GetGraphCommitData get_graph_commit_data = 447; - GetGraphCommitDataResponse get_graph_commit_data_response = 448; // current max + GetCommitData get_commit_data = 447; + GetCommitDataResponse get_commit_data_response = 448; // current max } reserved 87 to 88; diff --git a/crates/proto/src/proto.rs b/crates/proto/src/proto.rs index d191562f8194ea..a4d382e40f3240 100644 --- a/crates/proto/src/proto.rs +++ b/crates/proto/src/proto.rs @@ -358,8 +358,8 @@ messages!( (GitGetHeadShaResponse, Background), (GitEditRef, Background), (GitRepairWorktrees, Background), - (GetGraphCommitData, Background), - (GetGraphCommitDataResponse, Background), + (GetCommitData, Background), + (GetCommitDataResponse, Background), (GitWorktreesResponse, Background), (GitCreateWorktree, Background), (GitRemoveWorktree, Background), @@ -575,7 +575,7 @@ request_messages!( (GitGetHeadSha, GitGetHeadShaResponse), (GitEditRef, Ack), (GitRepairWorktrees, Ack), - (GetGraphCommitData, GetGraphCommitDataResponse), + (GetCommitData, GetCommitDataResponse), (GitCreateWorktree, Ack), (GitRemoveWorktree, Ack), (GitRenameWorktree, Ack), @@ -770,7 +770,7 @@ entity_messages!( GitGetHeadSha, GitEditRef, GitRepairWorktrees, - GetGraphCommitData, + GetCommitData, GitCreateArchiveCheckpoint, GitRestoreArchiveCheckpoint, GitCreateWorktree, From 2befd0e2b310fe28af8266c3709f1fb3f8497f6e Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 12:42:35 -0400 Subject: [PATCH 24/28] Cargo cmt --- crates/project/src/git_store.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index f5d357b391b4d6..c4e7e8d3a98b04 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -2567,7 +2567,7 @@ impl GitStore { receivers.push(shared.clone()); } CommitDataState::Loading(None) => { - // todo! this could happen if the request fails + // todo(git_graph) this could happen if the request fails, we should encode an error case debug_panic!( "This should never happen since we passed true into fetch commit data" ); From 0e92fb75d92669f4de1938b0de3433b5ba117dc3 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 13:02:06 -0400 Subject: [PATCH 25/28] Add remote test for this --- crates/collab/src/rpc.rs | 1 + crates/collab/tests/integration/git_tests.rs | 206 ++++++++++++++++++- crates/project/src/git_store.rs | 11 + 3 files changed, 214 insertions(+), 4 deletions(-) diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 986330e118de9e..1294b06c8e553f 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -436,6 +436,7 @@ impl Server { .add_request_handler(forward_mutating_project_request::) .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_mutating_project_request::) .add_request_handler(disallow_guest_request::) .add_request_handler(disallow_guest_request::) diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index b8248ce00214be..6ec190d4213432 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -1,15 +1,22 @@ -use std::path::{self, Path, PathBuf}; +use std::{ + path::{self, Path, PathBuf}, + sync::Arc, +}; use call::ActiveCall; use client::RECEIVE_TIMEOUT; use collections::HashMap; use git::{ - repository::{RepoPath, Worktree as GitWorktree}, + Oid, + repository::{CommitData, RepoPath, Worktree as GitWorktree}, 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 gpui::{AppContext as _, BackgroundExecutor, SharedString, TestAppContext, VisualTestContext}; +use project::{ + ProjectPath, + git_store::{CommitDataState, Repository}, +}; use serde_json::json; use util::{path, rel_path::rel_path}; @@ -91,6 +98,93 @@ fn collect_diff_stats( }) } +async fn load_commit_data_batch( + repository: &gpui::Entity, + shas: &[Oid], + executor: &BackgroundExecutor, + cx: &mut TestAppContext, +) -> HashMap> { + let states = cx.update(|cx| { + shas.iter() + .map(|sha| { + ( + *sha, + repository.update(cx, |repository, cx| { + repository.fetch_commit_data(*sha, true, cx).clone() + }), + ) + }) + .collect::>() + }); + + executor.run_until_parked(); + + let mut commit_data = HashMap::default(); + for (sha, state) in states { + let data = match state { + CommitDataState::Loaded(data) => data, + CommitDataState::Loading(Some(shared)) => shared.await.unwrap(), + CommitDataState::Loading(None) => { + panic!("fetch_commit_data(..., true) should return a waiter-backed state") + } + }; + commit_data.insert(sha, data); + } + + commit_data +} + +fn loaded_commit_data_cache( + repository: &gpui::Entity, + cx: &mut TestAppContext, +) -> HashMap { + cx.update(|cx| repository.update(cx, |repository, _| repository.loaded_commit_data_for_test())) +} + +fn assert_remote_cache_matches_local_cache( + local_repository: &gpui::Entity, + remote_repository: &gpui::Entity, + cx_local: &mut TestAppContext, + cx_remote: &mut TestAppContext, +) { + let local_cache = loaded_commit_data_cache(local_repository, cx_local); + let remote_cache = loaded_commit_data_cache(remote_repository, cx_remote); + + for (sha, remote_commit_data) in &remote_cache { + let local_commit_data = local_cache + .get(sha) + .unwrap_or_else(|| panic!("local cache missing commit data for {sha}")); + assert_eq!( + local_commit_data.sha, remote_commit_data.sha, + "local and remote cache should agree on sha for {sha}" + ); + assert_eq!( + local_commit_data.parents, remote_commit_data.parents, + "local and remote cache should agree on parents for {sha}" + ); + assert_eq!( + local_commit_data.author_name, remote_commit_data.author_name, + "local and remote cache should agree on author_name for {sha}" + ); + assert_eq!( + local_commit_data.author_email, remote_commit_data.author_email, + "local and remote cache should agree on author_email for {sha}" + ); + assert_eq!( + local_commit_data.commit_timestamp, remote_commit_data.commit_timestamp, + "local and remote cache should agree on commit_timestamp for {sha}" + ); + assert_eq!( + local_commit_data.subject, remote_commit_data.subject, + "local and remote cache should agree on subject for {sha}" + ); + assert_eq!( + local_commit_data.message, remote_commit_data.message, + "local and remote cache should agree on message for {sha}" + ); + } +} + #[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; @@ -480,6 +574,110 @@ async fn test_remote_git_head_sha( assert_eq!(remote_head_sha.unwrap(), local_head_sha); } +#[gpui::test] +async fn test_remote_git_commit_data_batches( + executor: BackgroundExecutor, + cx_a: &mut TestAppContext, + cx_b: &mut TestAppContext, +) { + let mut server = TestServer::start(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; + server + .create_room(&mut [(&client_a, cx_a), (&client_b, cx_b)]) + .await; + let active_call_a = cx_a.read(ActiveCall::global); + + client_a + .fs() + .insert_tree( + path!("/project"), + json!({ ".git": {}, "file.txt": "content" }), + ) + .await; + + let commit_shas = [ + "0123456789abcdef0123456789abcdef01234567" + .parse::() + .unwrap(), + "1111111111111111111111111111111111111111" + .parse::() + .unwrap(), + "2222222222222222222222222222222222222222" + .parse::() + .unwrap(), + "3333333333333333333333333333333333333333" + .parse::() + .unwrap(), + ]; + + client_a.fs().set_commit_data( + Path::new(path!("/project/.git")), + commit_shas.iter().enumerate().map(|(index, sha)| { + ( + CommitData { + sha: *sha, + parents: Default::default(), + author_name: SharedString::from(format!("Author {index}")), + author_email: SharedString::from(format!("author{index}@example.com")), + commit_timestamp: 1_700_000_000 + index as i64, + subject: SharedString::from(format!("Subject {index}")), + message: SharedString::from(format!("Subject {index}\n\nBody {index}")), + }, + false, + ) + }), + ); + + let (project_a, _) = client_a.build_local_project(path!("/project"), cx_a).await; + executor.run_until_parked(); + + let repo_a = cx_a.update(|cx| project_a.read(cx).active_repository(cx).unwrap()); + + let primed_before = load_commit_data_batch(&repo_a, &commit_shas[..2], &executor, cx_a).await; + assert_eq!( + primed_before.len(), + 2, + "host should prime two commits before sharing" + ); + + 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; + + executor.run_until_parked(); + + let repo_b = cx_b.update(|cx| project_b.read(cx).active_repository(cx).unwrap()); + + let remote_batch_one = + load_commit_data_batch(&repo_b, &commit_shas[..3], &executor, cx_b).await; + assert_eq!(remote_batch_one.len(), 3); + for (index, sha) in commit_shas[..3].iter().enumerate() { + let commit_data = remote_batch_one.get(sha).unwrap(); + assert_eq!(commit_data.sha, *sha); + assert_eq!(commit_data.subject.as_ref(), format!("Subject {index}")); + assert_eq!( + commit_data.message.as_ref(), + format!("Subject {index}\n\nBody {index}") + ); + } + + let primed_after = load_commit_data_batch(&repo_a, &commit_shas[2..], &executor, cx_a).await; + assert_eq!( + primed_after.len(), + 2, + "host should prime remaining commits after remote fetches" + ); + + let remote_batch_two = + load_commit_data_batch(&repo_b, &commit_shas[1..], &executor, cx_b).await; + assert_eq!(remote_batch_two.len(), 3); + + assert_remote_cache_matches_local_cache(&repo_a, &repo_b, cx_a, cx_b); +} + #[gpui::test] async fn test_linked_worktrees_sync( executor: BackgroundExecutor, diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index c4e7e8d3a98b04..158dd4799aa22f 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5120,6 +5120,17 @@ impl Repository { }) } + #[cfg(any(test, feature = "test-support"))] + pub fn loaded_commit_data_for_test(&self) -> HashMap { + self.commit_data + .iter() + .filter_map(|(sha, state)| match state { + CommitDataState::Loaded(data) => Some((sha.clone(), data.as_ref().clone())), + CommitDataState::Loading(_) => None, + }) + .collect() + } + fn get_handler(&mut self, cx: &mut Context) -> &mut CommitDataHandler { if matches!(self.commit_data_handler, CommitDataHandlerState::Closed) { self.commit_data_handler = From 01175ac4e6c65b1e6d724f46e7b1f1414a5c4210 Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 13:48:24 -0400 Subject: [PATCH 26/28] Final clean up --- crates/collab/tests/integration/git_tests.rs | 71 ++++++++------------ crates/project/src/git_store.rs | 24 ++++--- 2 files changed, 42 insertions(+), 53 deletions(-) diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index 6ec190d4213432..0ea4ed45fd3179 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -1,7 +1,4 @@ -use std::{ - path::{self, Path, PathBuf}, - sync::Arc, -}; +use std::path::{self, Path, PathBuf}; use call::ActiveCall; use client::RECEIVE_TIMEOUT; @@ -13,10 +10,7 @@ use git::{ }; use git_ui::{git_panel::GitPanel, project_diff::ProjectDiff}; use gpui::{AppContext as _, BackgroundExecutor, SharedString, TestAppContext, VisualTestContext}; -use project::{ - ProjectPath, - git_store::{CommitDataState, Repository}, -}; +use project::{ProjectPath, git_store::Repository}; use serde_json::json; use util::{path, rel_path::rel_path}; @@ -103,42 +97,31 @@ async fn load_commit_data_batch( shas: &[Oid], executor: &BackgroundExecutor, cx: &mut TestAppContext, -) -> HashMap> { - let states = cx.update(|cx| { - shas.iter() - .map(|sha| { - ( - *sha, - repository.update(cx, |repository, cx| { - repository.fetch_commit_data(*sha, true, cx).clone() - }), - ) - }) - .collect::>() +) -> HashMap { + cx.update(|cx| { + for sha in shas { + repository.update(cx, |repository, cx| { + repository.fetch_commit_data(*sha, true, cx); + }); + } }); executor.run_until_parked(); - let mut commit_data = HashMap::default(); - for (sha, state) in states { - let data = match state { - CommitDataState::Loaded(data) => data, - CommitDataState::Loading(Some(shared)) => shared.await.unwrap(), - CommitDataState::Loading(None) => { - panic!("fetch_commit_data(..., true) should return a waiter-backed state") - } - }; - commit_data.insert(sha, data); - } - - commit_data -} - -fn loaded_commit_data_cache( - repository: &gpui::Entity, - cx: &mut TestAppContext, -) -> HashMap { - cx.update(|cx| repository.update(cx, |repository, _| repository.loaded_commit_data_for_test())) + let loaded_commit_data = cx.update(|cx| { + repository.update(cx, |repository, _| repository.loaded_commit_data_for_test()) + }); + shas.iter() + .map(|sha| { + ( + *sha, + loaded_commit_data + .get(sha) + .unwrap_or_else(|| panic!("missing loaded commit data for {sha}")) + .clone(), + ) + }) + .collect() } fn assert_remote_cache_matches_local_cache( @@ -147,8 +130,12 @@ fn assert_remote_cache_matches_local_cache( cx_local: &mut TestAppContext, cx_remote: &mut TestAppContext, ) { - let local_cache = loaded_commit_data_cache(local_repository, cx_local); - let remote_cache = loaded_commit_data_cache(remote_repository, cx_remote); + let local_cache = cx_local.update(|cx| { + local_repository.update(cx, |repository, _| repository.loaded_commit_data_for_test()) + }); + let remote_cache = cx_remote.update(|cx| { + remote_repository.update(cx, |repository, _| repository.loaded_commit_data_for_test()) + }); for (sha, remote_commit_data) in &remote_cache { let local_commit_data = local_cache diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 158dd4799aa22f..99e5ad16ab2682 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5120,17 +5120,6 @@ impl Repository { }) } - #[cfg(any(test, feature = "test-support"))] - pub fn loaded_commit_data_for_test(&self) -> HashMap { - self.commit_data - .iter() - .filter_map(|(sha, state)| match state { - CommitDataState::Loaded(data) => Some((sha.clone(), data.as_ref().clone())), - CommitDataState::Loading(_) => None, - }) - .collect() - } - fn get_handler(&mut self, cx: &mut Context) -> &mut CommitDataHandler { if matches!(self.commit_data_handler, CommitDataHandlerState::Closed) { self.commit_data_handler = @@ -8074,6 +8063,19 @@ fn proto_to_commit_details(proto: &proto::GitCommitDetails) -> CommitDetails { } } +#[cfg(any(test, feature = "test-support"))] +impl Repository { + pub fn loaded_commit_data_for_test(&self) -> HashMap { + self.commit_data + .iter() + .filter_map(|(sha, state)| match state { + CommitDataState::Loaded(data) => Some((*sha, data.as_ref().clone())), + CommitDataState::Loading(_) => None, + }) + .collect() + } +} + #[cfg(test)] mod tests { use super::*; From b3c639b7312fc8f95b1077fb07b417feb37d247c Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 13:48:49 -0400 Subject: [PATCH 27/28] Remove plan.md --- plan.md | 119 -------------------------------------------------------- 1 file changed, 119 deletions(-) delete mode 100644 plan.md diff --git a/plan.md b/plan.md deleted file mode 100644 index ed38a50d4385cf..00000000000000 --- a/plan.md +++ /dev/null @@ -1,119 +0,0 @@ -# Property test plan for git graph commit data loading - -## Goal - -Add randomized state-machine tests around `git_store` commit-data loading so we can validate handler lifecycle, pending request bookkeeping, and remote/host consistency. - -## Test style - -Use randomized state-machine / operation-sequence tests instead of generating arbitrary maps directly. - -That keeps the tested states reachable and lets us assert invariants after every step. - -## Operations to randomize - -Start with a small operation set: - -- Fetch commit data without a waiter -- Fetch commit data with a waiter -- Successfully enqueue a request -- Fail to enqueue a request -- Deliver a commit-data result -- Close the handler -- Reopen the handler -- For remote cases, deliver host-side loaded data to the remote client - -## Core invariants - -### Open-handler invariants - -When the handler is `Open`: - -- For all `sha` where `commit_data[sha] == Loading(_)`, `pending_requests.contains(sha)` must be true. -- For all `sha` where `commit_data[sha] == Loading(Some(_))`, `completers.contains_key(sha)` must be true. -- For all `sha` in `pending_requests`, `commit_data[sha]` must exist and be `Loading(_)`. -- For all `sha` in `completers`, `commit_data[sha]` must exist and be `Loading(Some(_))`. -- `completers.keys()` must be a subset of `pending_requests`. -- For all `sha` where `commit_data[sha] == Loading(None)`, `completers.contains_key(sha)` must be false. -- For all `sha` where `commit_data[sha] == Loaded(_)`, `pending_requests.contains(sha)` must be false. -- For all `sha` where `commit_data[sha] == Loaded(_)`, `completers.contains_key(sha)` must be false. - -### Closed-handler invariants - -When the handler is `Closed`: - -- `commit_data` must contain no `Loading(_)` entries. -- No pending request bookkeeping should survive the close transition. - -## Transition / postcondition checks - -### Result delivery - -If a result is delivered for `sha` while the handler is `Open`, afterwards: - -- `commit_data[sha] == Loaded(_)` -- `pending_requests.contains(sha)` is false -- `completers.contains_key(sha)` is false - -### Successful enqueue - -After a successful enqueue of `sha`: - -- `commit_data[sha]` exists and is `Loading(_)` -- `pending_requests.contains(sha)` is true -- if the request was waiter-backed, `commit_data[sha] == Loading(Some(_))` -- if the request was waiter-backed, `completers.contains_key(sha)` is true - -### Handler close - -Right after a handler close: - -- any `sha` that was still pending has been removed from `commit_data` -- no `Loading(_)` entries remain in `commit_data` - -## Remote / host consistency property - -For all loaded commit-data entries in a remote client, the host must also have those same entries as loaded. - -More concretely: - -- if the remote side has `commit_data[sha] == Loaded(data)` -- then the host side must also have `commit_data[sha] == Loaded(host_data)` -- and the loaded host entry must correspond to the same `sha` - -If we want to strengthen this later, we can also assert that the loaded payload fields match exactly, not just that both sides are loaded for the same `sha`. - -## Duplicate SHA / waiter upgrade invariants - -When `fetch_commit_data` is called multiple times for the same SHA, possibly with different `needs_waiter` values: - -### Same SHA, `needs_waiter = false` then `needs_waiter = false` - -- Second call is a no-op; state remains unchanged. -- `commit_data[sha]` is still `Loading(None)` (or `Loaded` if the result arrived between calls). - -### Same SHA, `needs_waiter = false` then `needs_waiter = true` - -- The state must upgrade from `Loading(None)` to `Loading(Some(_))`. -- A completer must be inserted into `handler.completers` for that SHA. -- The shared future in `Loading(Some(_))` must be resolvable by that completer. -- If the result has already arrived (`Loaded`), the second call should return the loaded state directly. - -### Same SHA, `needs_waiter = true` then `needs_waiter = true` - -- Second call should return the existing `Loading(Some(shared))` — the same shared future. -- No additional completer should be created. -- Both callers awaiting the shared future should resolve to the same data. - -### Same SHA, `needs_waiter = true` then `needs_waiter = false` - -- Second call is a no-op; the existing `Loading(Some(_))` state is preserved. -- The completer and shared future remain intact. - -## Possible future property - -Once enqueue-failure semantics are finalized, add a property around waiter-backed requests: - -- calling `fetch_commit_data(..., needs_waiter = true, ...)` should never leave the system in a state where that `sha` is `Loading(None)` - -This one depends on the final failure / retry policy, so it can wait until that behavior is settled. From f0b4804f66f0241b7cc878e68fb578b6ba15501b Mon Sep 17 00:00:00 2001 From: Anthony Eid Date: Wed, 22 Apr 2026 14:41:15 -0400 Subject: [PATCH 28/28] fix failing tests --- crates/collab/tests/integration/git_tests.rs | 49 +++++++++++--------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index 0ea4ed45fd3179..cfa142ab005547 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -10,7 +10,10 @@ use git::{ }; use git_ui::{git_panel::GitPanel, project_diff::ProjectDiff}; use gpui::{AppContext as _, BackgroundExecutor, SharedString, TestAppContext, VisualTestContext}; -use project::{ProjectPath, git_store::Repository}; +use project::{ + ProjectPath, + git_store::{CommitDataState, Repository}, +}; use serde_json::json; use util::{path, rel_path::rel_path}; @@ -98,30 +101,34 @@ async fn load_commit_data_batch( executor: &BackgroundExecutor, cx: &mut TestAppContext, ) -> HashMap { - cx.update(|cx| { - for sha in shas { - repository.update(cx, |repository, cx| { - repository.fetch_commit_data(*sha, true, cx); - }); - } + let states = cx.update(|cx| { + shas.iter() + .map(|sha| { + ( + *sha, + repository.update(cx, |repository, cx| { + repository.fetch_commit_data(*sha, true, cx).clone() + }), + ) + }) + .collect::>() }); executor.run_until_parked(); - let loaded_commit_data = cx.update(|cx| { - repository.update(cx, |repository, _| repository.loaded_commit_data_for_test()) - }); - shas.iter() - .map(|sha| { - ( - *sha, - loaded_commit_data - .get(sha) - .unwrap_or_else(|| panic!("missing loaded commit data for {sha}")) - .clone(), - ) - }) - .collect() + let mut commit_data = HashMap::default(); + for (sha, state) in states { + let data = match state { + CommitDataState::Loaded(data) => data.as_ref().clone(), + CommitDataState::Loading(Some(shared)) => shared.await.unwrap().as_ref().clone(), + CommitDataState::Loading(None) => { + panic!("fetch_commit_data(..., true) should return an await-result state") + } + }; + commit_data.insert(sha, data); + } + + commit_data } fn assert_remote_cache_matches_local_cache(