From e515b2c714b426724206e4d6eae8479bb9c3fbfd Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 23 Apr 2025 13:37:55 +0200 Subject: [PATCH] Polish agent checkpoints (#29265) Release Notes: - Improved performance of agent checkpoint creation. - Fixed a bug that sometimes caused accidental deletions when restoring to a previous agent checkpoint. - Fixed a bug that caused checkpoints to be visible in the Git history. --- crates/agent/src/thread.rs | 14 +---------- crates/fs/src/fake_git_repo.rs | 4 --- crates/git/src/repository.rs | 44 +++++++++------------------------ crates/project/src/git_store.rs | 40 ------------------------------ 4 files changed, 13 insertions(+), 89 deletions(-) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index d2754c738a..65c8690c09 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -619,24 +619,12 @@ impl Thread { .await .unwrap_or(false); - if equal { - git_store - .update(cx, |store, cx| { - store.delete_checkpoint(pending_checkpoint.git_checkpoint, cx) - })? - .detach(); - } else { + if !equal { this.update(cx, |this, cx| { this.insert_checkpoint(pending_checkpoint, cx) })?; } - git_store - .update(cx, |store, cx| { - store.delete_checkpoint(final_checkpoint, cx) - })? - .detach(); - Ok(()) } Err(_) => this.update(cx, |this, cx| { diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 1a2d28e241..83542e240d 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -431,10 +431,6 @@ impl GitRepository for FakeGitRepository { unimplemented!() } - fn delete_checkpoint(&self, _checkpoint: GitRepositoryCheckpoint) -> BoxFuture> { - unimplemented!() - } - fn diff_checkpoints( &self, _base_checkpoint: GitRepositoryCheckpoint, diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 793d977e7d..c490c53a7d 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -314,9 +314,6 @@ pub trait GitRepository: Send + Sync { right: GitRepositoryCheckpoint, ) -> BoxFuture>; - /// Deletes a previously-created checkpoint. - fn delete_checkpoint(&self, checkpoint: GitRepositoryCheckpoint) -> BoxFuture>; - /// Computes a diff between two checkpoints. fn diff_checkpoints( &self, @@ -374,7 +371,6 @@ impl RealGitRepository { #[derive(Clone, Debug)] pub struct GitRepositoryCheckpoint { - pub ref_name: String, pub commit_sha: Oid, } @@ -1225,11 +1221,8 @@ impl GitRepository for RealGitRepository { } else { git.run(&["commit-tree", &tree, "-m", "Checkpoint"]).await? }; - let ref_name = format!("refs/zed/{}", Uuid::new_v4()); - git.run(&["update-ref", &ref_name, &checkpoint_sha]).await?; Ok(GitRepositoryCheckpoint { - ref_name, commit_sha: checkpoint_sha.parse()?, }) }) @@ -1308,21 +1301,6 @@ impl GitRepository for RealGitRepository { .boxed() } - fn delete_checkpoint(&self, checkpoint: GitRepositoryCheckpoint) -> BoxFuture> { - let working_directory = self.working_directory(); - let git_binary_path = self.git_binary_path.clone(); - - let executor = self.executor.clone(); - self.executor - .spawn(async move { - let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor); - git.run(&["update-ref", "-d", &checkpoint.ref_name]).await?; - Ok(()) - }) - .boxed() - } - fn diff_checkpoints( &self, base_checkpoint: GitRepositoryCheckpoint, @@ -1340,8 +1318,8 @@ impl GitRepository for RealGitRepository { "diff", "--find-renames", "--patch", - &base_checkpoint.ref_name, - &target_checkpoint.ref_name, + &base_checkpoint.commit_sha.to_string(), + &target_checkpoint.commit_sha.to_string(), ]) .await }) @@ -1414,6 +1392,15 @@ impl GitBinary { } }); + // Copy the default index file so that Git doesn't have to rebuild the + // whole index from scratch. This might fail if this is an empty repository. + smol::fs::copy( + self.working_directory.join(".git").join("index"), + &index_file_path, + ) + .await + .ok(); + self.index_file_path = Some(index_file_path.clone()); let result = f(self).await; self.index_file_path = None; @@ -1856,13 +1843,6 @@ mod tests { .ok(), None ); - - // Garbage collecting after deleting a checkpoint makes it unreachable. - repo.delete_checkpoint(checkpoint.clone()).await.unwrap(); - repo.gc().await.unwrap(); - repo.restore_checkpoint(checkpoint.clone()) - .await - .unwrap_err(); } #[gpui::test] @@ -1975,7 +1955,7 @@ mod tests { let git_binary_path = git_binary_path.clone(); let working_directory = working_directory?; let git = GitBinary::new(git_binary_path, working_directory, executor); - git.run(&["gc", "--prune=now"]).await?; + git.run(&["gc", "--prune"]).await?; Ok(()) }) .boxed() diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 1165373d1e..43bb3decec 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -832,32 +832,6 @@ impl GitStore { }) } - pub fn delete_checkpoint( - &self, - checkpoint: GitStoreCheckpoint, - cx: &mut App, - ) -> Task> { - let repositories_by_work_directory_abs_path = self - .repositories - .values() - .map(|repo| (repo.read(cx).snapshot.work_directory_abs_path.clone(), repo)) - .collect::>(); - - let mut tasks = Vec::new(); - for (work_dir_abs_path, checkpoint) in checkpoint.checkpoints_by_work_dir_abs_path { - if let Some(repository) = - repositories_by_work_directory_abs_path.get(&work_dir_abs_path) - { - let delete = repository.update(cx, |this, _| this.delete_checkpoint(checkpoint)); - tasks.push(async move { delete.await? }); - } - } - cx.background_spawn(async move { - future::try_join_all(tasks).await?; - Ok(()) - }) - } - /// Blames a buffer. pub fn blame_buffer( &self, @@ -3795,20 +3769,6 @@ impl Repository { }) } - pub fn delete_checkpoint( - &mut self, - checkpoint: GitRepositoryCheckpoint, - ) -> oneshot::Receiver> { - self.send_job(None, move |repo, _cx| async move { - match repo { - RepositoryState::Local { backend, .. } => { - backend.delete_checkpoint(checkpoint).await - } - RepositoryState::Remote { .. } => Err(anyhow!("not implemented yet")), - } - }) - } - pub fn diff_checkpoints( &mut self, base_checkpoint: GitRepositoryCheckpoint,