From 0a1a92131fc7bbe6b80c7b590d672814d7bbff5e Mon Sep 17 00:00:00 2001 From: Anthony Eid <56899983+Anthony-Eid@users.noreply.github.com> Date: Tue, 3 Mar 2026 19:01:28 +0100 Subject: [PATCH] git: Fix remote worktree support (#50614) The main issue is that we weren't forwarding the proto messages through the collab server to the host. After fixing that I added integration tests to cover local worktrees, remote worktrees, and ssh worktrees. I also fixed a bug with FakeRepository where it wouldn't name its current branch as a worktree when calling git worktree, which doesn't match the behavior of the git binary. Before you mark this PR as ready for review, make sure that you have: - [x] Added a solid test coverage and/or screenshots from doing manual testing - [x] Done a self-review taking into account security and performance aspects Release Notes: - git: Fix bug that caused the git worktree picker from displaying and creating worktrees over collab --- crates/collab/src/rpc.rs | 2 + crates/collab/tests/integration/git_tests.rs | 143 ++++++++++++++- .../remote_editing_collaboration_tests.rs | 126 ++++++++++++- crates/fs/Cargo.toml | 2 +- crates/fs/src/fake_git_repo.rs | 168 +++--------------- crates/fs/tests/integration/fake_git_repo.rs | 141 ++++++++++++++- crates/git/src/repository.rs | 3 + crates/project/tests/integration/git_store.rs | 119 +++++++++++++ 8 files changed, 555 insertions(+), 149 deletions(-) diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 087dbe2a0ba23851689e75401c62b64775cf2282..b521f6b083ae311d98ec46c900ce821fd8042e4a 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -437,6 +437,8 @@ impl Server { .add_request_handler(forward_mutating_project_request::) .add_request_handler(forward_mutating_project_request::) .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(forward_mutating_project_request::) .add_message_handler(broadcast_project_message_from_host::) .add_message_handler(update_context) diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index f3abb5bc3f3e1a12e7ecb56c985f2cff46582cee..6792eb92484d34f3085287b57f48a5761e760c92 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -1,9 +1,9 @@ -use std::path::Path; +use std::path::{Path, PathBuf}; use call::ActiveCall; use git::status::{FileStatus, StatusCode, TrackedStatus}; use git_ui::project_diff::ProjectDiff; -use gpui::{AppContext as _, TestAppContext, VisualTestContext}; +use gpui::{AppContext as _, BackgroundExecutor, TestAppContext, VisualTestContext}; use project::ProjectPath; use serde_json::json; use util::{path, rel_path::rel_path}; @@ -141,3 +141,142 @@ async fn test_project_diff(cx_a: &mut TestAppContext, cx_b: &mut TestAppContext) ); }); } + +#[gpui::test] +async fn test_remote_git_worktrees( + 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 (project_a, _) = client_a.build_local_project(path!("/project"), cx_a).await; + + 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()); + + // Initially only the main worktree (the repo itself) should be present + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!(worktrees.len(), 1); + assert_eq!(worktrees[0].path, PathBuf::from(path!("/project"))); + + // Client B creates a git worktree via the remote project + let worktree_directory = PathBuf::from(path!("/project")); + cx_b.update(|cx| { + repo_b.update(cx, |repository, _| { + repository.create_worktree( + "feature-branch".to_string(), + worktree_directory.clone(), + Some("abc123".to_string()), + ) + }) + }) + .await + .unwrap() + .unwrap(); + + executor.run_until_parked(); + + // Client B lists worktrees — should see main + the one just created + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!(worktrees.len(), 2); + assert_eq!(worktrees[0].path, PathBuf::from(path!("/project"))); + assert_eq!(worktrees[1].path, worktree_directory.join("feature-branch")); + assert_eq!(worktrees[1].ref_name.as_ref(), "refs/heads/feature-branch"); + assert_eq!(worktrees[1].sha.as_ref(), "abc123"); + + // Verify from the host side that the worktree was actually created + let host_worktrees = { + 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.worktrees())) + .await + .unwrap() + .unwrap() + }; + assert_eq!(host_worktrees.len(), 2); + assert_eq!(host_worktrees[0].path, PathBuf::from(path!("/project"))); + assert_eq!( + host_worktrees[1].path, + worktree_directory.join("feature-branch") + ); + + // Client B creates a second git worktree without an explicit commit + cx_b.update(|cx| { + repo_b.update(cx, |repository, _| { + repository.create_worktree( + "bugfix-branch".to_string(), + worktree_directory.clone(), + None, + ) + }) + }) + .await + .unwrap() + .unwrap(); + + executor.run_until_parked(); + + // Client B lists worktrees — should now have main + two created + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repository, _| repository.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!(worktrees.len(), 3); + + let feature_worktree = worktrees + .iter() + .find(|worktree| worktree.ref_name.as_ref() == "refs/heads/feature-branch") + .expect("should find feature-branch worktree"); + assert_eq!( + feature_worktree.path, + worktree_directory.join("feature-branch") + ); + + let bugfix_worktree = worktrees + .iter() + .find(|worktree| worktree.ref_name.as_ref() == "refs/heads/bugfix-branch") + .expect("should find bugfix-branch worktree"); + assert_eq!( + bugfix_worktree.path, + worktree_directory.join("bugfix-branch") + ); + assert_eq!(bugfix_worktree.sha.as_ref(), "fake-sha"); +} diff --git a/crates/collab/tests/integration/remote_editing_collaboration_tests.rs b/crates/collab/tests/integration/remote_editing_collaboration_tests.rs index 4556c740ec74f6fb1bc8a2c760812376dae6b4a8..6825c468e783ee8d3a2a6107a031accfc108abd0 100644 --- a/crates/collab/tests/integration/remote_editing_collaboration_tests.rs +++ b/crates/collab/tests/integration/remote_editing_collaboration_tests.rs @@ -33,7 +33,7 @@ use settings::{ SettingsStore, }; use std::{ - path::Path, + path::{Path, PathBuf}, sync::{ Arc, atomic::{AtomicUsize, Ordering}, @@ -396,6 +396,130 @@ async fn test_ssh_collaboration_git_branches( }); } +#[gpui::test] +async fn test_ssh_collaboration_git_worktrees( + executor: BackgroundExecutor, + cx_a: &mut TestAppContext, + cx_b: &mut TestAppContext, + server_cx: &mut TestAppContext, +) { + cx_a.set_name("a"); + cx_b.set_name("b"); + server_cx.set_name("server"); + + cx_a.update(|cx| { + release_channel::init(semver::Version::new(0, 0, 0), cx); + }); + server_cx.update(|cx| { + release_channel::init(semver::Version::new(0, 0, 0), cx); + }); + + 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 (opts, server_ssh, _) = RemoteClient::fake_server(cx_a, server_cx); + let remote_fs = FakeFs::new(server_cx.executor()); + remote_fs + .insert_tree("/project", json!({ ".git": {}, "file.txt": "content" })) + .await; + + server_cx.update(HeadlessProject::init); + let languages = Arc::new(LanguageRegistry::new(server_cx.executor())); + let headless_project = server_cx.new(|cx| { + HeadlessProject::new( + HeadlessAppState { + session: server_ssh, + fs: remote_fs.clone(), + http_client: Arc::new(BlockedHttpClient), + node_runtime: NodeRuntime::unavailable(), + languages, + extension_host_proxy: Arc::new(ExtensionHostProxy::new()), + startup_time: std::time::Instant::now(), + }, + false, + cx, + ) + }); + + let client_ssh = RemoteClient::connect_mock(opts, cx_a).await; + let (project_a, _) = client_a + .build_ssh_project("/project", client_ssh, false, cx_a) + .await; + + let active_call_a = cx_a.read(ActiveCall::global); + let project_id = active_call_a + .update(cx_a, |call, cx| call.share_project(project_a.clone(), cx)) + .await + .unwrap(); + let project_b = client_b.join_remote_project(project_id, cx_b).await; + + executor.run_until_parked(); + + let repo_b = cx_b.update(|cx| project_b.read(cx).active_repository(cx).unwrap()); + + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repo, _| repo.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!(worktrees.len(), 1); + + let worktree_directory = PathBuf::from("/project"); + cx_b.update(|cx| { + repo_b.update(cx, |repo, _| { + repo.create_worktree( + "feature-branch".to_string(), + worktree_directory.clone(), + Some("abc123".to_string()), + ) + }) + }) + .await + .unwrap() + .unwrap(); + + executor.run_until_parked(); + + let worktrees = cx_b + .update(|cx| repo_b.update(cx, |repo, _| repo.worktrees())) + .await + .unwrap() + .unwrap(); + assert_eq!(worktrees.len(), 2); + assert_eq!(worktrees[1].path, worktree_directory.join("feature-branch")); + assert_eq!(worktrees[1].ref_name.as_ref(), "refs/heads/feature-branch"); + assert_eq!(worktrees[1].sha.as_ref(), "abc123"); + + 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); + assert_eq!( + server_worktrees[1].path, + worktree_directory.join("feature-branch") + ); +} + #[gpui::test] async fn test_ssh_collaboration_formatting_with_prettier( executor: BackgroundExecutor, diff --git a/crates/fs/Cargo.toml b/crates/fs/Cargo.toml index 6355524e4f328df0ca7fcf24c1df0557676ba6a6..04cae2dd2ad18f85a7c2ed663c1c3482febb22d3 100644 --- a/crates/fs/Cargo.toml +++ b/crates/fs/Cargo.toml @@ -58,4 +58,4 @@ gpui = { workspace = true, features = ["test-support"] } git = { workspace = true, features = ["test-support"] } [features] -test-support = ["gpui/test-support", "git/test-support"] +test-support = ["gpui/test-support", "git/test-support", "util/test-support"] diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 12cd67cdae1a250d07468047617c8cc7a52737fa..99295c69d45427c799e3d850d605f63d3950ee57 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -406,7 +406,31 @@ impl GitRepository for FakeGitRepository { } fn worktrees(&self) -> BoxFuture<'_, Result>> { - self.with_state_async(false, |state| Ok(state.worktrees.clone())) + let dot_git_path = self.dot_git_path.clone(); + self.with_state_async(false, move |state| { + let work_dir = dot_git_path + .parent() + .map(PathBuf::from) + .unwrap_or(dot_git_path); + let head_sha = state + .refs + .get("HEAD") + .cloned() + .unwrap_or_else(|| "0000000".to_string()); + let branch_ref = state + .current_branch_name + .as_ref() + .map(|name| format!("refs/heads/{name}")) + .unwrap_or_else(|| "refs/heads/main".to_string()); + let main_worktree = Worktree { + path: work_dir, + ref_name: branch_ref.into(), + sha: head_sha.into(), + }; + let mut all = vec![main_worktree]; + all.extend(state.worktrees.iter().cloned()); + Ok(all) + }) } fn create_worktree( @@ -1012,145 +1036,3 @@ impl GitRepository for FakeGitRepository { anyhow::bail!("commit_data_reader not supported for FakeGitRepository") } } - -#[cfg(test)] -mod tests { - use super::*; - use crate::{FakeFs, Fs}; - use gpui::TestAppContext; - use serde_json::json; - use std::path::Path; - - #[gpui::test] - async fn test_fake_worktree_lifecycle(cx: &mut TestAppContext) { - let worktree_dir_settings = &["../worktrees", ".git/zed-worktrees", "my-worktrees/"]; - - for worktree_dir_setting in worktree_dir_settings { - let fs = FakeFs::new(cx.executor()); - fs.insert_tree("/project", json!({".git": {}, "file.txt": "content"})) - .await; - let repo = fs - .open_repo(Path::new("/project/.git"), None) - .expect("should open fake repo"); - - // Initially no worktrees - let worktrees = repo.worktrees().await.unwrap(); - assert!(worktrees.is_empty()); - - let expected_dir = git::repository::resolve_worktree_directory( - Path::new("/project"), - worktree_dir_setting, - ); - - // Create a worktree - repo.create_worktree( - "feature-branch".to_string(), - expected_dir.clone(), - Some("abc123".to_string()), - ) - .await - .unwrap(); - - // List worktrees — should have one - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 1); - assert_eq!( - worktrees[0].path, - expected_dir.join("feature-branch"), - "failed for worktree_directory setting: {worktree_dir_setting:?}" - ); - assert_eq!(worktrees[0].ref_name.as_ref(), "refs/heads/feature-branch"); - assert_eq!(worktrees[0].sha.as_ref(), "abc123"); - - // Directory should exist in FakeFs after create - assert!( - fs.is_dir(&expected_dir.join("feature-branch")).await, - "worktree directory should be created in FakeFs for setting {worktree_dir_setting:?}" - ); - - // Create a second worktree (without explicit commit) - repo.create_worktree("bugfix-branch".to_string(), expected_dir.clone(), None) - .await - .unwrap(); - - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 2); - assert!( - fs.is_dir(&expected_dir.join("bugfix-branch")).await, - "second worktree directory should be created in FakeFs for setting {worktree_dir_setting:?}" - ); - - // Rename the first worktree - repo.rename_worktree( - expected_dir.join("feature-branch"), - expected_dir.join("renamed-branch"), - ) - .await - .unwrap(); - - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 2); - assert!( - worktrees - .iter() - .any(|w| w.path == expected_dir.join("renamed-branch")), - "renamed worktree should exist at new path for setting {worktree_dir_setting:?}" - ); - assert!( - worktrees - .iter() - .all(|w| w.path != expected_dir.join("feature-branch")), - "old path should no longer exist for setting {worktree_dir_setting:?}" - ); - - // Directory should be moved in FakeFs after rename - assert!( - !fs.is_dir(&expected_dir.join("feature-branch")).await, - "old worktree directory should not exist after rename for setting {worktree_dir_setting:?}" - ); - assert!( - fs.is_dir(&expected_dir.join("renamed-branch")).await, - "new worktree directory should exist after rename for setting {worktree_dir_setting:?}" - ); - - // Rename a nonexistent worktree should fail - let result = repo - .rename_worktree(PathBuf::from("/nonexistent"), PathBuf::from("/somewhere")) - .await; - assert!(result.is_err()); - - // Remove a worktree - repo.remove_worktree(expected_dir.join("renamed-branch"), false) - .await - .unwrap(); - - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 1); - assert_eq!(worktrees[0].path, expected_dir.join("bugfix-branch")); - - // Directory should be removed from FakeFs after remove - assert!( - !fs.is_dir(&expected_dir.join("renamed-branch")).await, - "worktree directory should be removed from FakeFs for setting {worktree_dir_setting:?}" - ); - - // Remove a nonexistent worktree should fail - let result = repo - .remove_worktree(PathBuf::from("/nonexistent"), false) - .await; - assert!(result.is_err()); - - // Remove the last worktree - repo.remove_worktree(expected_dir.join("bugfix-branch"), false) - .await - .unwrap(); - - let worktrees = repo.worktrees().await.unwrap(); - assert!(worktrees.is_empty()); - assert!( - !fs.is_dir(&expected_dir.join("bugfix-branch")).await, - "last worktree directory should be removed from FakeFs for setting {worktree_dir_setting:?}" - ); - } - } -} diff --git a/crates/fs/tests/integration/fake_git_repo.rs b/crates/fs/tests/integration/fake_git_repo.rs index 36dfcaf168b4f0190c5c49bf4798fac7bc9bd37b..bae7f2fc94dd5161793f85f64cc0a1448a187134 100644 --- a/crates/fs/tests/integration/fake_git_repo.rs +++ b/crates/fs/tests/integration/fake_git_repo.rs @@ -1,9 +1,146 @@ use fs::{FakeFs, Fs}; -use gpui::BackgroundExecutor; +use gpui::{BackgroundExecutor, TestAppContext}; use serde_json::json; -use std::path::Path; +use std::path::{Path, PathBuf}; use util::path; +#[gpui::test] +async fn test_fake_worktree_lifecycle(cx: &mut TestAppContext) { + let worktree_dir_settings = &["../worktrees", ".git/zed-worktrees", "my-worktrees/"]; + + for worktree_dir_setting in worktree_dir_settings { + let fs = FakeFs::new(cx.executor()); + fs.insert_tree("/project", json!({".git": {}, "file.txt": "content"})) + .await; + let repo = fs + .open_repo(Path::new("/project/.git"), None) + .expect("should open fake repo"); + + // Initially only the main worktree exists + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 1); + assert_eq!(worktrees[0].path, PathBuf::from("/project")); + + let expected_dir = git::repository::resolve_worktree_directory( + Path::new("/project"), + worktree_dir_setting, + ); + + // Create a worktree + repo.create_worktree( + "feature-branch".to_string(), + expected_dir.clone(), + Some("abc123".to_string()), + ) + .await + .unwrap(); + + // List worktrees — should have main + one created + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 2); + assert_eq!(worktrees[0].path, PathBuf::from("/project")); + assert_eq!( + worktrees[1].path, + expected_dir.join("feature-branch"), + "failed for worktree_directory setting: {worktree_dir_setting:?}" + ); + assert_eq!(worktrees[1].ref_name.as_ref(), "refs/heads/feature-branch"); + assert_eq!(worktrees[1].sha.as_ref(), "abc123"); + + // Directory should exist in FakeFs after create + assert!( + fs.is_dir(&expected_dir.join("feature-branch")).await, + "worktree directory should be created in FakeFs for setting {worktree_dir_setting:?}" + ); + + // Create a second worktree (without explicit commit) + repo.create_worktree("bugfix-branch".to_string(), expected_dir.clone(), None) + .await + .unwrap(); + + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 3); + assert!( + fs.is_dir(&expected_dir.join("bugfix-branch")).await, + "second worktree directory should be created in FakeFs for setting {worktree_dir_setting:?}" + ); + + // Rename the first worktree + repo.rename_worktree( + expected_dir.join("feature-branch"), + expected_dir.join("renamed-branch"), + ) + .await + .unwrap(); + + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 3); + assert!( + worktrees + .iter() + .any(|w| w.path == expected_dir.join("renamed-branch")), + "renamed worktree should exist at new path for setting {worktree_dir_setting:?}" + ); + assert!( + worktrees + .iter() + .all(|w| w.path != expected_dir.join("feature-branch")), + "old path should no longer exist for setting {worktree_dir_setting:?}" + ); + + // Directory should be moved in FakeFs after rename + assert!( + !fs.is_dir(&expected_dir.join("feature-branch")).await, + "old worktree directory should not exist after rename for setting {worktree_dir_setting:?}" + ); + assert!( + fs.is_dir(&expected_dir.join("renamed-branch")).await, + "new worktree directory should exist after rename for setting {worktree_dir_setting:?}" + ); + + // Rename a nonexistent worktree should fail + let result = repo + .rename_worktree(PathBuf::from("/nonexistent"), PathBuf::from("/somewhere")) + .await; + assert!(result.is_err()); + + // Remove a worktree + repo.remove_worktree(expected_dir.join("renamed-branch"), false) + .await + .unwrap(); + + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 2); + assert_eq!(worktrees[0].path, PathBuf::from("/project")); + assert_eq!(worktrees[1].path, expected_dir.join("bugfix-branch")); + + // Directory should be removed from FakeFs after remove + assert!( + !fs.is_dir(&expected_dir.join("renamed-branch")).await, + "worktree directory should be removed from FakeFs for setting {worktree_dir_setting:?}" + ); + + // Remove a nonexistent worktree should fail + let result = repo + .remove_worktree(PathBuf::from("/nonexistent"), false) + .await; + assert!(result.is_err()); + + // Remove the last worktree + repo.remove_worktree(expected_dir.join("bugfix-branch"), false) + .await + .unwrap(); + + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 1); + assert_eq!(worktrees[0].path, PathBuf::from("/project")); + assert!( + !fs.is_dir(&expected_dir.join("bugfix-branch")).await, + "last worktree directory should be removed from FakeFs for setting {worktree_dir_setting:?}" + ); + } +} + #[gpui::test] async fn test_checkpoints(executor: BackgroundExecutor) { let fs = FakeFs::new(executor); diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index bd07555d05b759a33080b9ae9f166145c3d26d14..6dba1400dffe1fd00844dd7241f39f48a7a759a6 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -303,6 +303,7 @@ impl Branch { pub struct Worktree { pub path: PathBuf, pub ref_name: SharedString, + // todo(git_worktree) This type should be a Oid pub sha: SharedString, } @@ -340,6 +341,8 @@ pub fn parse_worktrees_from_str>(raw_worktrees: T) -> Vec