From d18e4a75bc8892090912bc536c081e3e84df1d2a Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 10 Mar 2026 15:57:06 +0100 Subject: [PATCH] git: Add SSH support for removing and renaming git worktrees (#50759) This should be the last step in implementing full git worktree support in the `GitStore`. We still need to add UI for that allows a user to rename a git worktree and, by extension git branches if we use the git picker to do so. Also, I added a helper function called `disallow_guest_request::` to the `Collab::rpc` that is used to specify a proto request isn't allowed to be sent by a guest. This enabled me to add a regression test that checks that a guest isn't allowed to delete a git worktree, without the test hanging forever because it's waiting for the proto server to respond. Since SSH connections send the proto message directly from client to remote host, this won't affect those requests. Before you mark this PR as ready for review, make sure that you have: - [x] Added solid test coverage and/or screenshots from doing manual testing - [x] Done a self-review taking into account security and performance aspects - [ ] Aligned any UI changes with the [UI checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) Release Notes: - git: Add SSH support for removing git worktrees --- crates/agent_ui/src/agent_panel.rs | 6 + crates/collab/src/rpc.rs | 20 +++ crates/collab/tests/integration/git_tests.rs | 61 +++++++++ .../remote_editing_collaboration_tests.rs | 116 ++++++++++++++++++ crates/project/src/git_store.rs | 86 ++++++++++++- crates/proto/proto/git.proto | 14 +++ crates/proto/proto/zed.proto | 4 +- crates/proto/src/proto.rs | 6 + 8 files changed, 308 insertions(+), 5 deletions(-) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index c63d41b683..2b9f2f5624 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -374,12 +374,15 @@ pub fn init(cx: &mut App) { panel.update(cx, |panel, cx| { panel.external_thread( + None, + None, None, None, Some(AgentInitialContent::ContentBlock { blocks: content_blocks, auto_submit: true, }), + true, window, cx, ); @@ -399,12 +402,15 @@ pub fn init(cx: &mut App) { panel.update(cx, |panel, cx| { panel.external_thread( + None, + None, None, None, Some(AgentInitialContent::ContentBlock { blocks: content_blocks, auto_submit: true, }), + true, window, cx, ); diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index b521f6b083..6c05bd4e53 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -439,6 +439,8 @@ impl Server { .add_request_handler(forward_mutating_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::) .add_request_handler(forward_mutating_project_request::) .add_message_handler(broadcast_project_message_from_host::) .add_message_handler(update_context) @@ -2250,6 +2252,24 @@ where Ok(()) } +async fn disallow_guest_request( + _request: T, + response: Response, + _session: MessageContext, +) -> Result<()> +where + T: RequestMessage, +{ + response.peer.respond_with_error( + response.receipt, + ErrorCode::Forbidden + .message("request is not allowed for guests".to_string()) + .to_proto(), + )?; + response.responded.store(true, SeqCst); + Ok(()) +} + async fn lsp_query( request: proto::LspQuery, response: Response, diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index dccc99a077..f8c461b91f 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -302,6 +302,67 @@ async fn test_remote_git_worktrees( worktree_directory.join("bugfix-branch") ); assert_eq!(bugfix_worktree.sha.as_ref(), "fake-sha"); + + // Client B (guest) attempts to rename a worktree. This should fail + // because worktree renaming is not forwarded through collab + let rename_result = cx_b + .update(|cx| { + repo_b.update(cx, |repository, _| { + repository.rename_worktree( + worktree_directory.join("feature-branch"), + worktree_directory.join("renamed-branch"), + ) + }) + }) + .await + .unwrap(); + assert!( + rename_result.is_err(), + "Guest should not be able to rename worktrees via collab" + ); + + executor.run_until_parked(); + + // Verify worktrees are unchanged — still 3 + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!( + worktrees.len(), + 3, + "Worktree count should be unchanged after failed rename" + ); + + // Client B (guest) attempts to remove a worktree. This should fail + // because worktree removal is not forwarded through collab + let remove_result = cx_b + .update(|cx| { + repo_b.update(cx, |repository, _| { + repository.remove_worktree(worktree_directory.join("feature-branch"), false) + }) + }) + .await + .unwrap(); + assert!( + remove_result.is_err(), + "Guest should not be able to remove worktrees via collab" + ); + + executor.run_until_parked(); + + // Verify worktrees are unchanged — still 3 + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!( + worktrees.len(), + 3, + "Worktree count should be unchanged after failed removal" + ); } #[gpui::test] diff --git a/crates/collab/tests/integration/remote_editing_collaboration_tests.rs b/crates/collab/tests/integration/remote_editing_collaboration_tests.rs index 6825c468e7..ceb7db1459 100644 --- a/crates/collab/tests/integration/remote_editing_collaboration_tests.rs +++ b/crates/collab/tests/integration/remote_editing_collaboration_tests.rs @@ -518,6 +518,122 @@ async fn test_ssh_collaboration_git_worktrees( server_worktrees[1].path, worktree_directory.join("feature-branch") ); + + // Host (client A) renames the worktree via SSH + let repo_a = cx_a.update(|cx| { + project_a + .read(cx) + .repositories(cx) + .values() + .next() + .unwrap() + .clone() + }); + cx_a.update(|cx| { + repo_a.update(cx, |repository, _| { + repository.rename_worktree( + PathBuf::from("/project/feature-branch"), + PathBuf::from("/project/renamed-branch"), + ) + }) + }) + .await + .unwrap() + .unwrap(); + + executor.run_until_parked(); + + let host_worktrees = cx_a + .update(|cx| repo_a.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!( + host_worktrees.len(), + 2, + "Host should still have 2 worktrees after rename" + ); + assert_eq!( + host_worktrees[1].path, + PathBuf::from("/project/renamed-branch") + ); + + let server_worktrees = { + let server_repo = server_cx.update(|cx| { + headless_project.update(cx, |headless_project, cx| { + headless_project + .git_store + .read(cx) + .repositories() + .values() + .next() + .unwrap() + .clone() + }) + }); + server_cx + .update(|cx| server_repo.update(cx, |repo, _| repo.worktrees())) + .await + .unwrap() + .unwrap() + }; + assert_eq!( + server_worktrees.len(), + 2, + "Server should still have 2 worktrees after rename" + ); + assert_eq!( + server_worktrees[1].path, + PathBuf::from("/project/renamed-branch") + ); + + // Host (client A) removes the renamed worktree via SSH + cx_a.update(|cx| { + repo_a.update(cx, |repository, _| { + repository.remove_worktree(PathBuf::from("/project/renamed-branch"), false) + }) + }) + .await + .unwrap() + .unwrap(); + + executor.run_until_parked(); + + let host_worktrees = cx_a + .update(|cx| repo_a.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!( + host_worktrees.len(), + 1, + "Host should only have the main worktree after removal" + ); + + let server_worktrees = { + let server_repo = server_cx.update(|cx| { + headless_project.update(cx, |headless_project, cx| { + headless_project + .git_store + .read(cx) + .repositories() + .values() + .next() + .unwrap() + .clone() + }) + }); + server_cx + .update(|cx| server_repo.update(cx, |repo, _| repo.worktrees())) + .await + .unwrap() + .unwrap() + }; + assert_eq!( + server_worktrees.len(), + 1, + "Server should only have the main worktree after removal" + ); } #[gpui::test] diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index eed1676197..0572fd1f4f 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -578,6 +578,8 @@ impl GitStore { client.add_entity_request_handler(Self::handle_git_clone); client.add_entity_request_handler(Self::handle_get_worktrees); client.add_entity_request_handler(Self::handle_create_worktree); + client.add_entity_request_handler(Self::handle_remove_worktree); + client.add_entity_request_handler(Self::handle_rename_worktree); } pub fn is_local(&self) -> bool { @@ -2384,6 +2386,44 @@ impl GitStore { Ok(proto::Ack {}) } + async fn handle_remove_worktree( + 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 path = PathBuf::from(envelope.payload.path); + let force = envelope.payload.force; + + repository_handle + .update(&mut cx, |repository_handle, _| { + repository_handle.remove_worktree(path, force) + }) + .await??; + + Ok(proto::Ack {}) + } + + async fn handle_rename_worktree( + 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 old_path = PathBuf::from(envelope.payload.old_path); + let new_path = PathBuf::from(envelope.payload.new_path); + + repository_handle + .update(&mut cx, |repository_handle, _| { + repository_handle.rename_worktree(old_path, new_path) + }) + .await??; + + Ok(proto::Ack {}) + } + async fn handle_get_branches( this: Entity, envelope: TypedEnvelope, @@ -5731,6 +5771,7 @@ impl Repository { } pub fn remove_worktree(&mut self, path: PathBuf, force: bool) -> oneshot::Receiver> { + let id = self.id; self.send_job( Some(format!("git worktree remove: {}", path.display()).into()), move |repo, _cx| async move { @@ -5738,10 +5779,47 @@ impl Repository { RepositoryState::Local(LocalRepositoryState { backend, .. }) => { backend.remove_worktree(path, force).await } - RepositoryState::Remote(_) => { - anyhow::bail!( - "Removing worktrees on remote repositories is not yet supported" - ) + RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { + client + .request(proto::GitRemoveWorktree { + project_id: project_id.0, + repository_id: id.to_proto(), + path: path.to_string_lossy().to_string(), + force, + }) + .await?; + + Ok(()) + } + } + }, + ) + } + + pub fn rename_worktree( + &mut self, + old_path: PathBuf, + new_path: PathBuf, + ) -> oneshot::Receiver> { + let id = self.id; + self.send_job( + Some(format!("git worktree move: {}", old_path.display()).into()), + move |repo, _cx| async move { + match repo { + RepositoryState::Local(LocalRepositoryState { backend, .. }) => { + backend.rename_worktree(old_path, new_path).await + } + RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { + client + .request(proto::GitRenameWorktree { + project_id: project_id.0, + repository_id: id.to_proto(), + old_path: old_path.to_string_lossy().to_string(), + new_path: new_path.to_string_lossy().to_string(), + }) + .await?; + + Ok(()) } } }, diff --git a/crates/proto/proto/git.proto b/crates/proto/proto/git.proto index 736abcdaa4..87fdc058f9 100644 --- a/crates/proto/proto/git.proto +++ b/crates/proto/proto/git.proto @@ -583,6 +583,20 @@ message GitCreateWorktree { optional string commit = 5; } +message GitRemoveWorktree { + uint64 project_id = 1; + uint64 repository_id = 2; + string path = 3; + bool force = 4; +} + +message GitRenameWorktree { + uint64 project_id = 1; + uint64 repository_id = 2; + string old_path = 3; + string new_path = 4; +} + message RunGitHook { enum GitHook { PRE_COMMIT = 0; diff --git a/crates/proto/proto/zed.proto b/crates/proto/proto/zed.proto index c129b6eff2..1fd7dfb89b 100644 --- a/crates/proto/proto/zed.proto +++ b/crates/proto/proto/zed.proto @@ -474,7 +474,9 @@ message Envelope { SpawnKernel spawn_kernel = 426; SpawnKernelResponse spawn_kernel_response = 427; - KillKernel kill_kernel = 428; // current max + KillKernel kill_kernel = 428; + GitRemoveWorktree git_remove_worktree = 431; + GitRenameWorktree git_rename_worktree = 432; // current max } reserved 87 to 88; diff --git a/crates/proto/src/proto.rs b/crates/proto/src/proto.rs index dd0a77beb2..88607abf6d 100644 --- a/crates/proto/src/proto.rs +++ b/crates/proto/src/proto.rs @@ -354,6 +354,8 @@ messages!( (GitGetWorktrees, Background), (GitWorktreesResponse, Background), (GitCreateWorktree, Background), + (GitRemoveWorktree, Background), + (GitRenameWorktree, Background), (ShareAgentThread, Foreground), (GetSharedAgentThread, Foreground), (GetSharedAgentThreadResponse, Foreground), @@ -557,6 +559,8 @@ request_messages!( (RemoteStarted, Ack), (GitGetWorktrees, GitWorktreesResponse), (GitCreateWorktree, Ack), + (GitRemoveWorktree, Ack), + (GitRenameWorktree, Ack), (TrustWorktrees, Ack), (RestrictWorktrees, Ack), (FindSearchCandidatesChunk, Ack), @@ -747,6 +751,8 @@ entity_messages!( NewExternalAgentVersionAvailable, GitGetWorktrees, GitCreateWorktree, + GitRemoveWorktree, + GitRenameWorktree, TrustWorktrees, RestrictWorktrees, FindSearchCandidatesChunk,