From fbdeb934519e955fd33653ff00a9ad29752fed5f Mon Sep 17 00:00:00 2001 From: Oliver Azevedo Barnes Date: Thu, 2 Apr 2026 17:43:42 +0100 Subject: [PATCH] devcontainer: Implement remote support for git checkpoint operations (#48896) Closes #47907 Implements the four git checkpoint operations (`create`, `restore`, `compare`, `diff`) that had been stubbed out for remote repositories, and related test infrastructure. Testing steps: 1. Open a project with a `.devcontainer` configuration and connect to the Dev Container 2. Open an Agent thread and ask the agent to make a code change 3. After the agent completes, verify the "Restore from checkpoint" button appears (previously missing in Dev Container sessions) 4. Click "Restore from checkpoint" and confirm the file reverts to its prior state Release Notes: - Added support for git checkpoint operations in remote/Dev Container sessions, restoring the "Restore from checkpoint" button in Agent threads. --------- Co-authored-by: KyleBarton --- crates/fs/src/fake_git_repo.rs | 84 +++++++++- crates/fs/tests/integration/fake_git_repo.rs | 23 ++- crates/project/src/git_store.rs | 144 ++++++++++++++++- crates/proto/proto/git.proto | 37 +++++ crates/proto/proto/zed.proto | 9 +- crates/proto/src/proto.rs | 15 ++ .../remote_server/src/remote_editing_tests.rs | 147 ++++++++++++++++++ 7 files changed, 449 insertions(+), 10 deletions(-) diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index fc66e27fc9a32c2a8897eb5c9faee917c21177c5..a00061452e4dbd2051b961fdde9e33dc05fba0b1 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -1053,10 +1053,88 @@ impl GitRepository for FakeGitRepository { fn diff_checkpoints( &self, - _base_checkpoint: GitRepositoryCheckpoint, - _target_checkpoint: GitRepositoryCheckpoint, + base_checkpoint: GitRepositoryCheckpoint, + target_checkpoint: GitRepositoryCheckpoint, ) -> BoxFuture<'_, Result> { - unimplemented!() + let executor = self.executor.clone(); + let checkpoints = self.checkpoints.clone(); + async move { + executor.simulate_random_delay().await; + let checkpoints = checkpoints.lock(); + let base = checkpoints + .get(&base_checkpoint.commit_sha) + .context(format!( + "invalid base checkpoint: {}", + base_checkpoint.commit_sha + ))?; + let target = checkpoints + .get(&target_checkpoint.commit_sha) + .context(format!( + "invalid target checkpoint: {}", + target_checkpoint.commit_sha + ))?; + + fn collect_files( + entry: &FakeFsEntry, + prefix: String, + out: &mut std::collections::BTreeMap, + ) { + match entry { + FakeFsEntry::File { content, .. } => { + out.insert(prefix, String::from_utf8_lossy(content).into_owned()); + } + FakeFsEntry::Dir { entries, .. } => { + for (name, child) in entries { + let path = if prefix.is_empty() { + name.clone() + } else { + format!("{prefix}/{name}") + }; + collect_files(child, path, out); + } + } + FakeFsEntry::Symlink { .. } => {} + } + } + + let mut base_files = std::collections::BTreeMap::new(); + let mut target_files = std::collections::BTreeMap::new(); + collect_files(base, String::new(), &mut base_files); + collect_files(target, String::new(), &mut target_files); + + let all_paths: std::collections::BTreeSet<&String> = + base_files.keys().chain(target_files.keys()).collect(); + + let mut diff = String::new(); + for path in all_paths { + match (base_files.get(path), target_files.get(path)) { + (Some(base_content), Some(target_content)) + if base_content != target_content => + { + diff.push_str(&format!("diff --git a/{path} b/{path}\n")); + diff.push_str(&format!("--- a/{path}\n")); + diff.push_str(&format!("+++ b/{path}\n")); + for line in base_content.lines() { + diff.push_str(&format!("-{line}\n")); + } + for line in target_content.lines() { + diff.push_str(&format!("+{line}\n")); + } + } + (Some(_), None) => { + diff.push_str(&format!("diff --git a/{path} /dev/null\n")); + diff.push_str("deleted file\n"); + } + (None, Some(_)) => { + diff.push_str(&format!("diff --git /dev/null b/{path}\n")); + diff.push_str("new file\n"); + } + _ => {} + } + } + Ok(diff) + } + .boxed() } fn default_branch( diff --git a/crates/fs/tests/integration/fake_git_repo.rs b/crates/fs/tests/integration/fake_git_repo.rs index e327f92e996bfa0e89cc60a0a9c0d919bec8bc47..6428083c161235001ef29daf3583520e7f7d25a2 100644 --- a/crates/fs/tests/integration/fake_git_repo.rs +++ b/crates/fs/tests/integration/fake_git_repo.rs @@ -155,7 +155,10 @@ async fn test_checkpoints(executor: BackgroundExecutor) { .unwrap() ); - repository.restore_checkpoint(checkpoint_1).await.unwrap(); + repository + .restore_checkpoint(checkpoint_1.clone()) + .await + .unwrap(); assert_eq!( fs.files_with_contents(Path::new("")), [ @@ -164,4 +167,22 @@ async fn test_checkpoints(executor: BackgroundExecutor) { (Path::new(path!("/foo/b")).into(), b"ipsum".into()) ] ); + + // diff_checkpoints: identical checkpoints produce empty diff + let diff = repository + .diff_checkpoints(checkpoint_2.clone(), checkpoint_3.clone()) + .await + .unwrap(); + assert!( + diff.is_empty(), + "identical checkpoints should produce empty diff" + ); + + // diff_checkpoints: different checkpoints produce non-empty diff + let diff = repository + .diff_checkpoints(checkpoint_1.clone(), checkpoint_2.clone()) + .await + .unwrap(); + assert!(diff.contains("b"), "diff should mention changed file 'b'"); + assert!(diff.contains("c"), "diff should mention added file 'c'"); } diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 6f838f02768a38d1c84935f5a7e7a303e682847d..e22d13b5fe5fd0bc64b6d95c52432437a41569f1 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -560,6 +560,10 @@ impl GitStore { client.add_entity_request_handler(Self::handle_run_hook); client.add_entity_request_handler(Self::handle_reset); client.add_entity_request_handler(Self::handle_show); + client.add_entity_request_handler(Self::handle_create_checkpoint); + client.add_entity_request_handler(Self::handle_restore_checkpoint); + client.add_entity_request_handler(Self::handle_compare_checkpoints); + client.add_entity_request_handler(Self::handle_diff_checkpoints); client.add_entity_request_handler(Self::handle_load_commit_diff); client.add_entity_request_handler(Self::handle_file_history); client.add_entity_request_handler(Self::handle_checkout_files); @@ -2619,6 +2623,92 @@ impl GitStore { }) } + async fn handle_create_checkpoint( + this: Entity, + envelope: TypedEnvelope, + mut cx: AsyncApp, + ) -> Result { + let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); + let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; + + let checkpoint = repository_handle + .update(&mut cx, |repository, _| repository.checkpoint()) + .await??; + + Ok(proto::GitCreateCheckpointResponse { + commit_sha: checkpoint.commit_sha.as_bytes().to_vec(), + }) + } + + async fn handle_restore_checkpoint( + this: Entity, + envelope: TypedEnvelope, + mut cx: AsyncApp, + ) -> Result { + let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); + let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; + + let checkpoint = GitRepositoryCheckpoint { + commit_sha: Oid::from_bytes(&envelope.payload.commit_sha)?, + }; + + repository_handle + .update(&mut cx, |repository, _| { + repository.restore_checkpoint(checkpoint) + }) + .await??; + + Ok(proto::Ack {}) + } + + async fn handle_compare_checkpoints( + this: Entity, + envelope: TypedEnvelope, + mut cx: AsyncApp, + ) -> Result { + let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); + let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; + + let left = GitRepositoryCheckpoint { + commit_sha: Oid::from_bytes(&envelope.payload.left_commit_sha)?, + }; + let right = GitRepositoryCheckpoint { + commit_sha: Oid::from_bytes(&envelope.payload.right_commit_sha)?, + }; + + let equal = repository_handle + .update(&mut cx, |repository, _| { + repository.compare_checkpoints(left, right) + }) + .await??; + + Ok(proto::GitCompareCheckpointsResponse { equal }) + } + + async fn handle_diff_checkpoints( + this: Entity, + envelope: TypedEnvelope, + mut cx: AsyncApp, + ) -> Result { + let repository_id = RepositoryId::from_proto(envelope.payload.repository_id); + let repository_handle = Self::repository_for_request(&this, repository_id, &mut cx)?; + + let base = GitRepositoryCheckpoint { + commit_sha: Oid::from_bytes(&envelope.payload.base_commit_sha)?, + }; + let target = GitRepositoryCheckpoint { + commit_sha: Oid::from_bytes(&envelope.payload.target_commit_sha)?, + }; + + let diff = repository_handle + .update(&mut cx, |repository, _| { + repository.diff_checkpoints(base, target) + }) + .await??; + + Ok(proto::GitDiffCheckpointsResponse { diff }) + } + async fn handle_load_commit_diff( this: Entity, envelope: TypedEnvelope, @@ -6229,12 +6319,24 @@ impl Repository { } pub fn checkpoint(&mut self) -> oneshot::Receiver> { - self.send_job(None, |repo, _cx| async move { + let id = self.id; + self.send_job(None, move |repo, _cx| async move { match repo { RepositoryState::Local(LocalRepositoryState { backend, .. }) => { backend.checkpoint().await } - RepositoryState::Remote(..) => anyhow::bail!("not implemented yet"), + RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { + let response = client + .request(proto::GitCreateCheckpoint { + project_id: project_id.0, + repository_id: id.to_proto(), + }) + .await?; + + Ok(GitRepositoryCheckpoint { + commit_sha: Oid::from_bytes(&response.commit_sha)?, + }) + } } }) } @@ -6243,12 +6345,22 @@ impl Repository { &mut self, checkpoint: GitRepositoryCheckpoint, ) -> oneshot::Receiver> { + let id = self.id; self.send_job(None, move |repo, _cx| async move { match repo { RepositoryState::Local(LocalRepositoryState { backend, .. }) => { backend.restore_checkpoint(checkpoint).await } - RepositoryState::Remote { .. } => anyhow::bail!("not implemented yet"), + RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { + client + .request(proto::GitRestoreCheckpoint { + project_id: project_id.0, + repository_id: id.to_proto(), + commit_sha: checkpoint.commit_sha.as_bytes().to_vec(), + }) + .await?; + Ok(()) + } } }) } @@ -6342,12 +6454,23 @@ impl Repository { left: GitRepositoryCheckpoint, right: GitRepositoryCheckpoint, ) -> oneshot::Receiver> { + let id = self.id; self.send_job(None, move |repo, _cx| async move { match repo { RepositoryState::Local(LocalRepositoryState { backend, .. }) => { backend.compare_checkpoints(left, right).await } - RepositoryState::Remote { .. } => anyhow::bail!("not implemented yet"), + RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { + let response = client + .request(proto::GitCompareCheckpoints { + project_id: project_id.0, + repository_id: id.to_proto(), + left_commit_sha: left.commit_sha.as_bytes().to_vec(), + right_commit_sha: right.commit_sha.as_bytes().to_vec(), + }) + .await?; + Ok(response.equal) + } } }) } @@ -6357,6 +6480,7 @@ impl Repository { base_checkpoint: GitRepositoryCheckpoint, target_checkpoint: GitRepositoryCheckpoint, ) -> oneshot::Receiver> { + let id = self.id; self.send_job(None, move |repo, _cx| async move { match repo { RepositoryState::Local(LocalRepositoryState { backend, .. }) => { @@ -6364,7 +6488,17 @@ impl Repository { .diff_checkpoints(base_checkpoint, target_checkpoint) .await } - RepositoryState::Remote { .. } => anyhow::bail!("not implemented yet"), + RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { + let response = client + .request(proto::GitDiffCheckpoints { + project_id: project_id.0, + repository_id: id.to_proto(), + base_commit_sha: base_checkpoint.commit_sha.as_bytes().to_vec(), + target_commit_sha: target_checkpoint.commit_sha.as_bytes().to_vec(), + }) + .await?; + Ok(response.diff) + } } }) } diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index cb878cade726002e7e09670cf7c190880d8e66cb..0cbb635d78dddc81aa7c75340f2fbebe83a474e3 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -586,6 +586,43 @@ message GitCreateWorktree { optional string commit = 5; } +message GitCreateCheckpoint { + uint64 project_id = 1; + uint64 repository_id = 2; +} + +message GitCreateCheckpointResponse { + bytes commit_sha = 1; +} + +message GitRestoreCheckpoint { + uint64 project_id = 1; + uint64 repository_id = 2; + bytes commit_sha = 3; +} + +message GitCompareCheckpoints { + uint64 project_id = 1; + uint64 repository_id = 2; + bytes left_commit_sha = 3; + bytes right_commit_sha = 4; +} + +message GitCompareCheckpointsResponse { + bool equal = 1; +} + +message GitDiffCheckpoints { + uint64 project_id = 1; + uint64 repository_id = 2; + bytes base_commit_sha = 3; + bytes target_commit_sha = 4; +} + +message GitDiffCheckpointsResponse { + string diff = 1; +} + message GitRemoveWorktree { uint64 project_id = 1; uint64 repository_id = 2; diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index d165bcb9529a41294d2bc25572f454c425f8c3f0..24e7c5372f2679eab1726487e1967edcef6024ed 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -467,7 +467,14 @@ message Envelope { SpawnKernelResponse spawn_kernel_response = 427; KillKernel kill_kernel = 428; GitRemoveWorktree git_remove_worktree = 431; - GitRenameWorktree git_rename_worktree = 432; // current max + GitRenameWorktree git_rename_worktree = 432; + GitCreateCheckpoint git_create_checkpoint = 433; + GitCreateCheckpointResponse git_create_checkpoint_response = 434; + GitRestoreCheckpoint git_restore_checkpoint = 435; + GitCompareCheckpoints git_compare_checkpoints = 436; + GitCompareCheckpointsResponse git_compare_checkpoints_response = 437; + GitDiffCheckpoints git_diff_checkpoints = 438; + GitDiffCheckpointsResponse git_diff_checkpoints_response = 439; // current max } reserved 87 to 88; diff --git a/crates/proto/src/proto.rs b/crates/proto/src/proto.rs index 8c72fa08c57755dc45b9658db441a037d0a9fe2e..c21934338f97cc8ed3e04b917c7db84fccecd031 100644 --- a/crates/proto/src/proto.rs +++ b/crates/proto/src/proto.rs @@ -294,6 +294,13 @@ messages!( (GitCommitDetails, Background), (GitFileHistory, Background), (GitFileHistoryResponse, Background), + (GitCreateCheckpoint, Background), + (GitCreateCheckpointResponse, Background), + (GitRestoreCheckpoint, Background), + (GitCompareCheckpoints, Background), + (GitCompareCheckpointsResponse, Background), + (GitDiffCheckpoints, Background), + (GitDiffCheckpointsResponse, Background), (SetIndexText, Background), (Push, Background), (Fetch, Background), @@ -514,6 +521,10 @@ request_messages!( (RegisterBufferWithLanguageServers, Ack), (GitShow, GitCommitDetails), (GitFileHistory, GitFileHistoryResponse), + (GitCreateCheckpoint, GitCreateCheckpointResponse), + (GitRestoreCheckpoint, Ack), + (GitCompareCheckpoints, GitCompareCheckpointsResponse), + (GitDiffCheckpoints, GitDiffCheckpointsResponse), (GitReset, Ack), (GitDeleteBranch, Ack), (GitCheckoutFiles, Ack), @@ -696,6 +707,10 @@ entity_messages!( RegisterBufferWithLanguageServers, GitShow, GitFileHistory, + GitCreateCheckpoint, + GitRestoreCheckpoint, + GitCompareCheckpoints, + GitDiffCheckpoints, GitReset, GitDeleteBranch, GitCheckoutFiles, diff --git a/crates/remote_server/src/remote_editing_tests.rs b/crates/remote_server/src/remote_editing_tests.rs index 86b7f93eb2c737cac55dbf2882f91ec277e4e174..90546773df234767489df96ee37d50e3fcaeea3b 100644 --- a/crates/remote_server/src/remote_editing_tests.rs +++ b/crates/remote_server/src/remote_editing_tests.rs @@ -1917,6 +1917,153 @@ async fn test_remote_git_branches(cx: &mut TestAppContext, server_cx: &mut TestA assert_eq!(server_branch.name(), "totally-new-branch"); } +#[gpui::test] +async fn test_remote_git_checkpoints(cx: &mut TestAppContext, server_cx: &mut TestAppContext) { + let fs = FakeFs::new(server_cx.executor()); + fs.insert_tree( + path!("/code"), + json!({ + "project1": { + ".git": {}, + "file.txt": "original content", + }, + }), + ) + .await; + + let (project, _headless) = init_test(&fs, cx, server_cx).await; + + let (_worktree, _) = project + .update(cx, |project, cx| { + project.find_or_create_worktree(path!("/code/project1"), true, cx) + }) + .await + .unwrap(); + cx.run_until_parked(); + + let repository = project.update(cx, |project, cx| project.active_repository(cx).unwrap()); + + // 1. Create a checkpoint of the original state + let checkpoint_1 = repository + .update(cx, |repository, _| repository.checkpoint()) + .await + .unwrap() + .unwrap(); + + // 2. Modify a file on the server-side fs + fs.write( + Path::new(path!("/code/project1/file.txt")), + b"modified content", + ) + .await + .unwrap(); + + // 3. Create a second checkpoint with the modified state + let checkpoint_2 = repository + .update(cx, |repository, _| repository.checkpoint()) + .await + .unwrap() + .unwrap(); + + // 4. compare_checkpoints: same checkpoint with itself => equal + let equal = repository + .update(cx, |repository, _| { + repository.compare_checkpoints(checkpoint_1.clone(), checkpoint_1.clone()) + }) + .await + .unwrap() + .unwrap(); + assert!(equal, "a checkpoint compared with itself should be equal"); + + // 5. compare_checkpoints: different states => not equal + let equal = repository + .update(cx, |repository, _| { + repository.compare_checkpoints(checkpoint_1.clone(), checkpoint_2.clone()) + }) + .await + .unwrap() + .unwrap(); + assert!( + !equal, + "checkpoints of different states should not be equal" + ); + + // 6. diff_checkpoints: same checkpoint => empty diff + let diff = repository + .update(cx, |repository, _| { + repository.diff_checkpoints(checkpoint_1.clone(), checkpoint_1.clone()) + }) + .await + .unwrap() + .unwrap(); + assert!( + diff.is_empty(), + "diff of identical checkpoints should be empty" + ); + + // 7. diff_checkpoints: different checkpoints => non-empty diff mentioning the changed file + let diff = repository + .update(cx, |repository, _| { + repository.diff_checkpoints(checkpoint_1.clone(), checkpoint_2.clone()) + }) + .await + .unwrap() + .unwrap(); + assert!( + !diff.is_empty(), + "diff of different checkpoints should be non-empty" + ); + assert!( + diff.contains("file.txt"), + "diff should mention the changed file" + ); + assert!( + diff.contains("original content"), + "diff should contain removed content" + ); + assert!( + diff.contains("modified content"), + "diff should contain added content" + ); + + // 8. restore_checkpoint: restore to original state + repository + .update(cx, |repository, _| { + repository.restore_checkpoint(checkpoint_1.clone()) + }) + .await + .unwrap() + .unwrap(); + cx.run_until_parked(); + + // 9. Create a checkpoint after restore + let checkpoint_3 = repository + .update(cx, |repository, _| repository.checkpoint()) + .await + .unwrap() + .unwrap(); + + // 10. compare_checkpoints: restored state matches original + let equal = repository + .update(cx, |repository, _| { + repository.compare_checkpoints(checkpoint_1.clone(), checkpoint_3.clone()) + }) + .await + .unwrap() + .unwrap(); + assert!(equal, "restored state should match original checkpoint"); + + // 11. diff_checkpoints: restored state vs original => empty diff + let diff = repository + .update(cx, |repository, _| { + repository.diff_checkpoints(checkpoint_1.clone(), checkpoint_3.clone()) + }) + .await + .unwrap() + .unwrap(); + assert!(diff.is_empty(), "diff after restore should be empty"); +} + #[gpui::test] async fn test_remote_agent_fs_tool_calls(cx: &mut TestAppContext, server_cx: &mut TestAppContext) { let fs = FakeFs::new(server_cx.executor());