diff --git a/Cargo.lock b/Cargo.lock index 28fd637c212ef6d50b327eada4fafde4cfae556d..6b90e82d0078560a9989e53923042989c7795c51 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -10251,7 +10251,6 @@ dependencies = [ "async-recursion", "collections", "editor", - "fs", "gpui", "html5ever 0.27.0", "language", diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index f1ad3a21c7510776e444c3c28616e7a89abfa56c..c2d3da40a45f2e5698d0e4d65f15aa457a39480c 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -60,7 +60,6 @@ use editor::{Anchor, AnchorRangeExt as _, Editor, EditorEvent, MultiBuffer}; use extension::ExtensionEvents; use extension_host::ExtensionStore; use fs::Fs; -use git::repository::validate_worktree_directory; use gpui::{ Action, Animation, AnimationExt, AnyElement, App, AsyncWindowContext, ClipboardItem, Corner, DismissEvent, Entity, EventEmitter, ExternalPaths, FocusHandle, Focusable, KeyContext, Pixels, @@ -2613,11 +2612,10 @@ impl AgentPanel { for repo in git_repos { let (work_dir, new_path, receiver) = repo.update(cx, |repo, _cx| { - let original_repo = repo.original_repo_abs_path.clone(); - let directory = - validate_worktree_directory(&original_repo, worktree_directory_setting)?; - let new_path = directory.join(branch_name); - let receiver = repo.create_worktree(branch_name.to_string(), directory, None); + let new_path = + repo.path_for_new_linked_worktree(branch_name, worktree_directory_setting)?; + let receiver = + repo.create_worktree(branch_name.to_string(), new_path.clone(), None); let work_dir = repo.work_directory_abs_path.clone(); anyhow::Ok((work_dir, new_path, receiver)) })?; diff --git a/crates/collab/tests/integration/git_tests.rs b/crates/collab/tests/integration/git_tests.rs index fc20150d662b96be9b6ad4f99ae1f33032b6fb7b..0cbd2a97dabcb832e6b25298341272be783e4024 100644 --- a/crates/collab/tests/integration/git_tests.rs +++ b/crates/collab/tests/integration/git_tests.rs @@ -215,7 +215,7 @@ async fn test_remote_git_worktrees( repo_b.update(cx, |repository, _| { repository.create_worktree( "feature-branch".to_string(), - worktree_directory.clone(), + worktree_directory.join("feature-branch"), Some("abc123".to_string()), ) }) @@ -266,7 +266,7 @@ async fn test_remote_git_worktrees( repo_b.update(cx, |repository, _| { repository.create_worktree( "bugfix-branch".to_string(), - worktree_directory.clone(), + worktree_directory.join("bugfix-branch"), None, ) }) diff --git a/crates/collab/tests/integration/remote_editing_collaboration_tests.rs b/crates/collab/tests/integration/remote_editing_collaboration_tests.rs index ceb7db145970b52d23a6ef7ace82cd84acf1e840..59ba868de745b1364f7dd9cff45030b08e24d6a2 100644 --- a/crates/collab/tests/integration/remote_editing_collaboration_tests.rs +++ b/crates/collab/tests/integration/remote_editing_collaboration_tests.rs @@ -473,7 +473,7 @@ async fn test_ssh_collaboration_git_worktrees( repo_b.update(cx, |repo, _| { repo.create_worktree( "feature-branch".to_string(), - worktree_directory.clone(), + worktree_directory.join("feature-branch"), Some("abc123".to_string()), ) }) diff --git a/crates/extension/src/extension.rs b/crates/extension/src/extension.rs index 02db6befb72b53f4610cdfddea80d7c030e5d29a..2ec8c8ea5f4032522dcaf846736aeacc00de585f 100644 --- a/crates/extension/src/extension.rs +++ b/crates/extension/src/extension.rs @@ -11,7 +11,6 @@ use std::sync::Arc; use ::lsp::LanguageServerName; use anyhow::{Context as _, Result, bail}; use async_trait::async_trait; -use fs::normalize_path; use gpui::{App, Task}; use language::LanguageName; use semver::Version; @@ -57,7 +56,7 @@ pub trait Extension: Send + Sync + 'static { /// Returns a path relative to this extension's working directory. fn path_from_extension(&self, path: &Path) -> PathBuf { - normalize_path(&self.work_dir().join(path)) + util::normalize_path(&self.work_dir().join(path)) } async fn language_server_command( diff --git a/crates/fs/src/fake_git_repo.rs b/crates/fs/src/fake_git_repo.rs index 91103953f0da1f1cdcf99fec90f40f92526282bf..f94f8c1f62b07ea66961828d289123ad59b60978 100644 --- a/crates/fs/src/fake_git_repo.rs +++ b/crates/fs/src/fake_git_repo.rs @@ -438,15 +438,14 @@ impl GitRepository for FakeGitRepository { fn create_worktree( &self, - name: String, - directory: PathBuf, + branch_name: String, + path: PathBuf, from_commit: Option, ) -> BoxFuture<'_, Result<()>> { let fs = self.fs.clone(); let executor = self.executor.clone(); let dot_git_path = self.dot_git_path.clone(); async move { - let path = directory.join(&name); executor.simulate_random_delay().await; // Check for simulated error before any side effects fs.with_git_state(&dot_git_path, false, |state| { @@ -461,10 +460,10 @@ impl GitRepository for FakeGitRepository { fs.with_git_state(&dot_git_path, true, { let path = path.clone(); move |state| { - if state.branches.contains(&name) { - bail!("a branch named '{}' already exists", name); + if state.branches.contains(&branch_name) { + bail!("a branch named '{}' already exists", branch_name); } - let ref_name = format!("refs/heads/{name}"); + let ref_name = format!("refs/heads/{branch_name}"); let sha = from_commit.unwrap_or_else(|| "fake-sha".to_string()); state.refs.insert(ref_name.clone(), sha.clone()); state.worktrees.push(Worktree { @@ -472,7 +471,7 @@ impl GitRepository for FakeGitRepository { ref_name: ref_name.into(), sha: sha.into(), }); - state.branches.insert(name); + state.branches.insert(branch_name); Ok::<(), anyhow::Error>(()) } })??; diff --git a/crates/fs/src/fs.rs b/crates/fs/src/fs.rs index 3efb03b38ce3077d99979fc638d7b9fc1392eea7..99efafadc0421791c526bfe80a751d186de4ff8a 100644 --- a/crates/fs/src/fs.rs +++ b/crates/fs/src/fs.rs @@ -60,6 +60,8 @@ use git::{ repository::{InitialGraphCommitData, RepoPath, repo_path}, status::{FileStatus, StatusCode, TrackedStatus, UnmergedStatus}, }; +#[cfg(feature = "test-support")] +use util::normalize_path; #[cfg(feature = "test-support")] use smol::io::AsyncReadExt; @@ -2885,10 +2887,6 @@ impl Fs for FakeFs { } } -pub fn normalize_path(path: &Path) -> PathBuf { - util::normalize_path(path) -} - pub async fn copy_recursive<'a>( fs: &'a dyn Fs, source: &'a Path, diff --git a/crates/fs/tests/integration/fake_git_repo.rs b/crates/fs/tests/integration/fake_git_repo.rs index bae7f2fc94dd5161793f85f64cc0a1448a187134..cc68b77ee862bf09b1d02bfaf719544a52ac29c3 100644 --- a/crates/fs/tests/integration/fake_git_repo.rs +++ b/crates/fs/tests/integration/fake_git_repo.rs @@ -6,139 +6,108 @@ 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()), - ) + 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")); + + fs.create_dir("/my-worktrees".as_ref()).await.unwrap(); + let worktrees_dir = Path::new("/my-worktrees"); + + // Create a worktree + let worktree_1_dir = worktrees_dir.join("feature-branch"); + repo.create_worktree( + "feature-branch".to_string(), + worktree_1_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, worktree_1_dir); + 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(&worktrees_dir.join("feature-branch")).await); + + // Create a second worktree (without explicit commit) + let worktree_2_dir = worktrees_dir.join("bugfix-branch"); + repo.create_worktree("bugfix-branch".to_string(), worktree_2_dir.clone(), None) .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"), - ) + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 3); + assert!(fs.is_dir(&worktree_2_dir).await); + + // Rename the first worktree + repo.rename_worktree(worktree_1_dir, worktrees_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:?}" - ); - } + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 3); + assert!( + worktrees + .iter() + .any(|w| w.path == worktrees_dir.join("renamed-branch")), + ); + assert!( + worktrees + .iter() + .all(|w| w.path != worktrees_dir.join("feature-branch")), + ); + + // Directory should be moved in FakeFs after rename + assert!(!fs.is_dir(&worktrees_dir.join("feature-branch")).await); + assert!(fs.is_dir(&worktrees_dir.join("renamed-branch")).await); + + // 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(worktrees_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, worktree_2_dir); + + // Directory should be removed from FakeFs after remove + assert!(!fs.is_dir(&worktrees_dir.join("renamed-branch")).await); + + // 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(worktree_2_dir.clone(), 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(&worktree_2_dir).await); } #[gpui::test] diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index 074c397cec90858601eec0d5ac8adc3bc16c345c..d67086d20e2ea3e5b38b1fdb4c0bcb52dc9b4126 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -36,7 +36,7 @@ use thiserror::Error; use util::command::{Stdio, new_command}; use util::paths::PathStyle; use util::rel_path::RelPath; -use util::{ResultExt, normalize_path, paths}; +use util::{ResultExt, paths}; use uuid::Uuid; pub use askpass::{AskPassDelegate, AskPassResult, AskPassSession}; @@ -76,97 +76,6 @@ pub fn original_repo_path_from_common_dir(common_dir: &Path) -> PathBuf { } } -/// Resolves the configured worktree directory to an absolute path. -/// -/// `worktree_directory_setting` is the raw string from the user setting -/// (e.g. `"../worktrees"`, `".git/zed-worktrees"`, `"my-worktrees/"`). -/// Trailing slashes are stripped. The path is resolved relative to -/// `working_directory` (the repository's working directory root). -/// -/// When the resolved directory falls outside the working directory -/// (e.g. `"../worktrees"`), the repository's directory name is -/// automatically appended so that sibling repos don't collide. -/// For example, with working directory `~/code/zed` and setting -/// `"../worktrees"`, this returns `~/code/worktrees/zed`. -/// -/// When the resolved directory is inside the working directory -/// (e.g. `".git/zed-worktrees"`), no extra component is added -/// because the path is already project-scoped. -pub fn resolve_worktree_directory( - working_directory: &Path, - worktree_directory_setting: &str, -) -> PathBuf { - let trimmed = worktree_directory_setting.trim_end_matches(['/', '\\']); - let joined = working_directory.join(trimmed); - let resolved = normalize_path(&joined); - - if resolved.starts_with(working_directory) { - resolved - } else if let Some(repo_dir_name) = working_directory.file_name() { - resolved.join(repo_dir_name) - } else { - resolved - } -} - -/// Validates that the resolved worktree directory is acceptable: -/// - The setting must not be an absolute path. -/// - The resolved path must be either a subdirectory of the working -/// directory or a subdirectory of its parent (i.e., a sibling). -/// -/// Returns `Ok(resolved_path)` or an error with a user-facing message. -pub fn validate_worktree_directory( - working_directory: &Path, - worktree_directory_setting: &str, -) -> Result { - // Check the original setting before trimming, since a path like "///" - // is absolute but becomes "" after stripping trailing separators. - // Also check for leading `/` or `\` explicitly, because on Windows - // `Path::is_absolute()` requires a drive letter — so `/tmp/worktrees` - // would slip through even though it's clearly not a relative path. - if Path::new(worktree_directory_setting).is_absolute() - || worktree_directory_setting.starts_with('/') - || worktree_directory_setting.starts_with('\\') - { - anyhow::bail!( - "git.worktree_directory must be a relative path, got: {worktree_directory_setting:?}" - ); - } - - if worktree_directory_setting.is_empty() { - anyhow::bail!("git.worktree_directory must not be empty"); - } - - let trimmed = worktree_directory_setting.trim_end_matches(['/', '\\']); - if trimmed == ".." { - anyhow::bail!("git.worktree_directory must not be \"..\" (use \"../some-name\" instead)"); - } - - let resolved = resolve_worktree_directory(working_directory, worktree_directory_setting); - - let parent = working_directory.parent().unwrap_or(working_directory); - - if !resolved.starts_with(parent) { - anyhow::bail!( - "git.worktree_directory resolved to {resolved:?}, which is outside \ - the project root and its parent directory. It must resolve to a \ - subdirectory of {working_directory:?} or a sibling of it." - ); - } - - Ok(resolved) -} - -/// Returns the full absolute path for a specific branch's worktree -/// given the resolved worktree directory. -pub fn worktree_path_for_branch( - working_directory: &Path, - worktree_directory_setting: &str, - branch: &str, -) -> PathBuf { - resolve_worktree_directory(working_directory, worktree_directory_setting).join(branch) -} - /// Commit data needed for the git graph visualization. #[derive(Debug, Clone)] pub struct GraphCommitData { @@ -769,8 +678,8 @@ pub trait GitRepository: Send + Sync { fn create_worktree( &self, - name: String, - directory: PathBuf, + branch_name: String, + path: PathBuf, from_commit: Option, ) -> BoxFuture<'_, Result<()>>; @@ -1712,20 +1621,19 @@ impl GitRepository for RealGitRepository { fn create_worktree( &self, - name: String, - directory: PathBuf, + branch_name: String, + path: PathBuf, from_commit: Option, ) -> BoxFuture<'_, Result<()>> { let git_binary = self.git_binary(); - let final_path = directory.join(&name); let mut args = vec![ OsString::from("--no-optional-locks"), OsString::from("worktree"), OsString::from("add"), OsString::from("-b"), - OsString::from(name.as_str()), + OsString::from(branch_name.as_str()), OsString::from("--"), - OsString::from(final_path.as_os_str()), + OsString::from(path.as_os_str()), ]; if let Some(from_commit) = from_commit { args.push(OsString::from(from_commit)); @@ -1735,7 +1643,7 @@ impl GitRepository for RealGitRepository { self.executor .spawn(async move { - std::fs::create_dir_all(final_path.parent().unwrap_or(&final_path))?; + std::fs::create_dir_all(path.parent().unwrap_or(&path))?; let git = git_binary?; let output = git.build_command(&args).output().await?; if output.status.success() { @@ -3390,6 +3298,8 @@ fn checkpoint_author_envs() -> HashMap { #[cfg(test)] mod tests { + use std::fs; + use super::*; use gpui::TestAppContext; @@ -3912,86 +3822,76 @@ mod tests { assert_eq!(result[0].ref_name.as_ref(), "refs/heads/main"); } - const TEST_WORKTREE_DIRECTORIES: &[&str] = - &["../worktrees", ".git/zed-worktrees", "my-worktrees/"]; - #[gpui::test] async fn test_create_and_list_worktrees(cx: &mut TestAppContext) { disable_git_global_config(); cx.executor().allow_parking(); - for worktree_dir_setting in TEST_WORKTREE_DIRECTORIES { - let repo_dir = tempfile::tempdir().unwrap(); - git2::Repository::init(repo_dir.path()).unwrap(); + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + let worktrees_dir = temp_dir.path().join("worktrees"); - let repo = RealGitRepository::new( - &repo_dir.path().join(".git"), - None, - Some("git".into()), - cx.executor(), - ) - .unwrap(); + fs::create_dir_all(&repo_dir).unwrap(); + fs::create_dir_all(&worktrees_dir).unwrap(); - // Create an initial commit (required for worktrees) - smol::fs::write(repo_dir.path().join("file.txt"), "content") - .await - .unwrap(); - repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) - .await - .unwrap(); - repo.commit( - "Initial commit".into(), - None, - CommitOptions::default(), - AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), - Arc::new(checkpoint_author_envs()), - ) - .await - .unwrap(); + git2::Repository::init(&repo_dir).unwrap(); - // List worktrees — should have just the main one - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 1); - assert_eq!( - worktrees[0].path.canonicalize().unwrap(), - repo_dir.path().canonicalize().unwrap() - ); + let repo = RealGitRepository::new( + &repo_dir.join(".git"), + None, + Some("git".into()), + cx.executor(), + ) + .unwrap(); - // Create a new worktree - repo.create_worktree( - "test-branch".to_string(), - resolve_worktree_directory(repo_dir.path(), worktree_dir_setting), - Some("HEAD".to_string()), - ) + // Create an initial commit (required for worktrees) + smol::fs::write(repo_dir.join("file.txt"), "content") + .await + .unwrap(); + repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) .await .unwrap(); + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), + Arc::new(checkpoint_author_envs()), + ) + .await + .unwrap(); - // List worktrees — should have two - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 2); - - let expected_path = - worktree_path_for_branch(repo_dir.path(), worktree_dir_setting, "test-branch"); - let new_worktree = worktrees - .iter() - .find(|w| w.branch() == "test-branch") - .expect("should find worktree with test-branch"); - assert_eq!( - new_worktree.path.canonicalize().unwrap(), - expected_path.canonicalize().unwrap(), - "failed for worktree_directory setting: {worktree_dir_setting:?}" - ); + // List worktrees — should have just the main one + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 1); + assert_eq!( + worktrees[0].path.canonicalize().unwrap(), + repo_dir.canonicalize().unwrap() + ); - // Clean up so the next iteration starts fresh - repo.remove_worktree(expected_path, true).await.unwrap(); + let worktree_path = worktrees_dir.join("some-worktree"); - // Clean up the worktree base directory if it was created outside repo_dir - // (e.g. for the "../worktrees" setting, it won't be inside the TempDir) - let resolved_dir = resolve_worktree_directory(repo_dir.path(), worktree_dir_setting); - if !resolved_dir.starts_with(repo_dir.path()) { - let _ = std::fs::remove_dir_all(&resolved_dir); - } - } + // Create a new worktree + repo.create_worktree( + "test-branch".to_string(), + worktree_path.clone(), + Some("HEAD".to_string()), + ) + .await + .unwrap(); + + // List worktrees — should have two + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 2); + + let new_worktree = worktrees + .iter() + .find(|w| w.branch() == "test-branch") + .expect("should find worktree with test-branch"); + assert_eq!( + new_worktree.path.canonicalize().unwrap(), + worktree_path.canonicalize().unwrap(), + ); } #[gpui::test] @@ -3999,147 +3899,92 @@ mod tests { disable_git_global_config(); cx.executor().allow_parking(); - for worktree_dir_setting in TEST_WORKTREE_DIRECTORIES { - let repo_dir = tempfile::tempdir().unwrap(); - git2::Repository::init(repo_dir.path()).unwrap(); + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + let worktrees_dir = temp_dir.path().join("worktrees"); + git2::Repository::init(&repo_dir).unwrap(); - let repo = RealGitRepository::new( - &repo_dir.path().join(".git"), - None, - Some("git".into()), - cx.executor(), - ) - .unwrap(); + let repo = RealGitRepository::new( + &repo_dir.join(".git"), + None, + Some("git".into()), + cx.executor(), + ) + .unwrap(); - // Create an initial commit - smol::fs::write(repo_dir.path().join("file.txt"), "content") - .await - .unwrap(); - repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) - .await - .unwrap(); - repo.commit( - "Initial commit".into(), - None, - CommitOptions::default(), - AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), - Arc::new(checkpoint_author_envs()), - ) + // Create an initial commit + smol::fs::write(repo_dir.join("file.txt"), "content") .await .unwrap(); - - // Create a worktree - repo.create_worktree( - "to-remove".to_string(), - resolve_worktree_directory(repo_dir.path(), worktree_dir_setting), - Some("HEAD".to_string()), - ) + repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) .await .unwrap(); + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), + Arc::new(checkpoint_author_envs()), + ) + .await + .unwrap(); - let worktree_path = - worktree_path_for_branch(repo_dir.path(), worktree_dir_setting, "to-remove"); - assert!(worktree_path.exists()); - - // Remove the worktree - repo.remove_worktree(worktree_path.clone(), false) - .await - .unwrap(); - - // Verify it's gone from the list - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 1); - assert!( - worktrees.iter().all(|w| w.branch() != "to-remove"), - "removed worktree should not appear in list" - ); - - // Verify the directory is removed - assert!(!worktree_path.exists()); - - // Clean up the worktree base directory if it was created outside repo_dir - // (e.g. for the "../worktrees" setting, it won't be inside the TempDir) - let resolved_dir = resolve_worktree_directory(repo_dir.path(), worktree_dir_setting); - if !resolved_dir.starts_with(repo_dir.path()) { - let _ = std::fs::remove_dir_all(&resolved_dir); - } - } - } + // Create a worktree + let worktree_path = worktrees_dir.join("worktree-to-remove"); + repo.create_worktree( + "to-remove".to_string(), + worktree_path.clone(), + Some("HEAD".to_string()), + ) + .await + .unwrap(); - #[gpui::test] - async fn test_remove_worktree_force(cx: &mut TestAppContext) { - disable_git_global_config(); - cx.executor().allow_parking(); + // Remove the worktree + repo.remove_worktree(worktree_path.clone(), false) + .await + .unwrap(); - for worktree_dir_setting in TEST_WORKTREE_DIRECTORIES { - let repo_dir = tempfile::tempdir().unwrap(); - git2::Repository::init(repo_dir.path()).unwrap(); + // Verify the directory is removed + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 1); + assert!( + worktrees.iter().all(|w| w.branch() != "to-remove"), + "removed worktree should not appear in list" + ); + assert!(!worktree_path.exists()); + + // Create a worktree + let worktree_path = worktrees_dir.join("dirty-wt"); + repo.create_worktree( + "dirty-wt".to_string(), + worktree_path.clone(), + Some("HEAD".to_string()), + ) + .await + .unwrap(); - let repo = RealGitRepository::new( - &repo_dir.path().join(".git"), - None, - Some("git".into()), - cx.executor(), - ) - .unwrap(); + assert!(worktree_path.exists()); - // Create an initial commit - smol::fs::write(repo_dir.path().join("file.txt"), "content") - .await - .unwrap(); - repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) - .await - .unwrap(); - repo.commit( - "Initial commit".into(), - None, - CommitOptions::default(), - AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), - Arc::new(checkpoint_author_envs()), - ) + // Add uncommitted changes in the worktree + smol::fs::write(worktree_path.join("dirty-file.txt"), "uncommitted") .await .unwrap(); - // Create a worktree - repo.create_worktree( - "dirty-wt".to_string(), - resolve_worktree_directory(repo_dir.path(), worktree_dir_setting), - Some("HEAD".to_string()), - ) + // Non-force removal should fail with dirty worktree + let result = repo.remove_worktree(worktree_path.clone(), false).await; + assert!( + result.is_err(), + "non-force removal of dirty worktree should fail" + ); + + // Force removal should succeed + repo.remove_worktree(worktree_path.clone(), true) .await .unwrap(); - let worktree_path = - worktree_path_for_branch(repo_dir.path(), worktree_dir_setting, "dirty-wt"); - - // Add uncommitted changes in the worktree - smol::fs::write(worktree_path.join("dirty-file.txt"), "uncommitted") - .await - .unwrap(); - - // Non-force removal should fail with dirty worktree - let result = repo.remove_worktree(worktree_path.clone(), false).await; - assert!( - result.is_err(), - "non-force removal of dirty worktree should fail" - ); - - // Force removal should succeed - repo.remove_worktree(worktree_path.clone(), true) - .await - .unwrap(); - - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 1); - assert!(!worktree_path.exists()); - - // Clean up the worktree base directory if it was created outside repo_dir - // (e.g. for the "../worktrees" setting, it won't be inside the TempDir) - let resolved_dir = resolve_worktree_directory(repo_dir.path(), worktree_dir_setting); - if !resolved_dir.starts_with(repo_dir.path()) { - let _ = std::fs::remove_dir_all(&resolved_dir); - } - } + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 1); + assert!(!worktree_path.exists()); } #[gpui::test] @@ -4147,141 +3992,69 @@ mod tests { disable_git_global_config(); cx.executor().allow_parking(); - for worktree_dir_setting in TEST_WORKTREE_DIRECTORIES { - let repo_dir = tempfile::tempdir().unwrap(); - git2::Repository::init(repo_dir.path()).unwrap(); + let temp_dir = tempfile::tempdir().unwrap(); + let repo_dir = temp_dir.path().join("repo"); + let worktrees_dir = temp_dir.path().join("worktrees"); - let repo = RealGitRepository::new( - &repo_dir.path().join(".git"), - None, - Some("git".into()), - cx.executor(), - ) - .unwrap(); + git2::Repository::init(&repo_dir).unwrap(); - // Create an initial commit - smol::fs::write(repo_dir.path().join("file.txt"), "content") - .await - .unwrap(); - repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) - .await - .unwrap(); - repo.commit( - "Initial commit".into(), - None, - CommitOptions::default(), - AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), - Arc::new(checkpoint_author_envs()), - ) + let repo = RealGitRepository::new( + &repo_dir.join(".git"), + None, + Some("git".into()), + cx.executor(), + ) + .unwrap(); + + // Create an initial commit + smol::fs::write(repo_dir.join("file.txt"), "content") .await .unwrap(); - - // Create a worktree - repo.create_worktree( - "old-name".to_string(), - resolve_worktree_directory(repo_dir.path(), worktree_dir_setting), - Some("HEAD".to_string()), - ) + repo.stage_paths(vec![repo_path("file.txt")], Arc::new(HashMap::default())) .await .unwrap(); + repo.commit( + "Initial commit".into(), + None, + CommitOptions::default(), + AskPassDelegate::new(&mut cx.to_async(), |_, _, _| {}), + Arc::new(checkpoint_author_envs()), + ) + .await + .unwrap(); - let old_path = - worktree_path_for_branch(repo_dir.path(), worktree_dir_setting, "old-name"); - assert!(old_path.exists()); - - // Move the worktree to a new path - let new_path = - resolve_worktree_directory(repo_dir.path(), worktree_dir_setting).join("new-name"); - repo.rename_worktree(old_path.clone(), new_path.clone()) - .await - .unwrap(); - - // Verify the old path is gone and new path exists - assert!(!old_path.exists()); - assert!(new_path.exists()); - - // Verify it shows up in worktree list at the new path - let worktrees = repo.worktrees().await.unwrap(); - assert_eq!(worktrees.len(), 2); - let moved_worktree = worktrees - .iter() - .find(|w| w.branch() == "old-name") - .expect("should find worktree by branch name"); - assert_eq!( - moved_worktree.path.canonicalize().unwrap(), - new_path.canonicalize().unwrap() - ); - - // Clean up so the next iteration starts fresh - repo.remove_worktree(new_path, true).await.unwrap(); - - // Clean up the worktree base directory if it was created outside repo_dir - // (e.g. for the "../worktrees" setting, it won't be inside the TempDir) - let resolved_dir = resolve_worktree_directory(repo_dir.path(), worktree_dir_setting); - if !resolved_dir.starts_with(repo_dir.path()) { - let _ = std::fs::remove_dir_all(&resolved_dir); - } - } - } - - #[test] - fn test_resolve_worktree_directory() { - let work_dir = Path::new("/code/my-project"); - - // Sibling directory — outside project, so repo dir name is appended - assert_eq!( - resolve_worktree_directory(work_dir, "../worktrees"), - PathBuf::from("/code/worktrees/my-project") - ); - - // Git subdir — inside project, no repo name appended - assert_eq!( - resolve_worktree_directory(work_dir, ".git/zed-worktrees"), - PathBuf::from("/code/my-project/.git/zed-worktrees") - ); - - // Simple subdir — inside project, no repo name appended - assert_eq!( - resolve_worktree_directory(work_dir, "my-worktrees"), - PathBuf::from("/code/my-project/my-worktrees") - ); - - // Trailing slash is stripped - assert_eq!( - resolve_worktree_directory(work_dir, "../worktrees/"), - PathBuf::from("/code/worktrees/my-project") - ); - assert_eq!( - resolve_worktree_directory(work_dir, "my-worktrees/"), - PathBuf::from("/code/my-project/my-worktrees") - ); - - // Multiple trailing slashes - assert_eq!( - resolve_worktree_directory(work_dir, "foo///"), - PathBuf::from("/code/my-project/foo") - ); + // Create a worktree + let old_path = worktrees_dir.join("old-worktree-name"); + repo.create_worktree( + "old-name".to_string(), + old_path.clone(), + Some("HEAD".to_string()), + ) + .await + .unwrap(); - // Trailing backslashes (Windows-style) - assert_eq!( - resolve_worktree_directory(work_dir, "my-worktrees\\"), - PathBuf::from("/code/my-project/my-worktrees") - ); - assert_eq!( - resolve_worktree_directory(work_dir, "foo\\/\\"), - PathBuf::from("/code/my-project/foo") - ); + assert!(old_path.exists()); - // Empty string resolves to the working directory itself (inside) - assert_eq!( - resolve_worktree_directory(work_dir, ""), - PathBuf::from("/code/my-project") - ); + // Move the worktree to a new path + let new_path = worktrees_dir.join("new-worktree-name"); + repo.rename_worktree(old_path.clone(), new_path.clone()) + .await + .unwrap(); - // Just ".." — outside project, repo dir name appended + // Verify the old path is gone and new path exists + assert!(!old_path.exists()); + assert!(new_path.exists()); + + // Verify it shows up in worktree list at the new path + let worktrees = repo.worktrees().await.unwrap(); + assert_eq!(worktrees.len(), 2); + let moved_worktree = worktrees + .iter() + .find(|w| w.branch() == "old-name") + .expect("should find worktree by branch name"); assert_eq!( - resolve_worktree_directory(work_dir, ".."), - PathBuf::from("/code/my-project") + moved_worktree.path.canonicalize().unwrap(), + new_path.canonicalize().unwrap() ); } @@ -4313,71 +4086,6 @@ mod tests { ); } - #[test] - fn test_validate_worktree_directory() { - let work_dir = Path::new("/code/my-project"); - - // Valid: sibling - assert!(validate_worktree_directory(work_dir, "../worktrees").is_ok()); - - // Valid: subdirectory - assert!(validate_worktree_directory(work_dir, ".git/zed-worktrees").is_ok()); - assert!(validate_worktree_directory(work_dir, "my-worktrees").is_ok()); - - // Invalid: just ".." would resolve back to the working directory itself - let err = validate_worktree_directory(work_dir, "..").unwrap_err(); - assert!(err.to_string().contains("must not be \"..\"")); - - // Invalid: ".." with trailing separators - let err = validate_worktree_directory(work_dir, "..\\").unwrap_err(); - assert!(err.to_string().contains("must not be \"..\"")); - let err = validate_worktree_directory(work_dir, "../").unwrap_err(); - assert!(err.to_string().contains("must not be \"..\"")); - - // Invalid: empty string would resolve to the working directory itself - let err = validate_worktree_directory(work_dir, "").unwrap_err(); - assert!(err.to_string().contains("must not be empty")); - - // Invalid: absolute path - let err = validate_worktree_directory(work_dir, "/tmp/worktrees").unwrap_err(); - assert!(err.to_string().contains("relative path")); - - // Invalid: "/" is absolute on Unix - let err = validate_worktree_directory(work_dir, "/").unwrap_err(); - assert!(err.to_string().contains("relative path")); - - // Invalid: "///" is absolute - let err = validate_worktree_directory(work_dir, "///").unwrap_err(); - assert!(err.to_string().contains("relative path")); - - // Invalid: escapes too far up - let err = validate_worktree_directory(work_dir, "../../other-project/wt").unwrap_err(); - assert!(err.to_string().contains("outside")); - } - - #[test] - fn test_worktree_path_for_branch() { - let work_dir = Path::new("/code/my-project"); - - // Outside project — repo dir name is part of the resolved directory - assert_eq!( - worktree_path_for_branch(work_dir, "../worktrees", "feature/foo"), - PathBuf::from("/code/worktrees/my-project/feature/foo") - ); - - // Inside project — no repo dir name inserted - assert_eq!( - worktree_path_for_branch(work_dir, ".git/zed-worktrees", "my-branch"), - PathBuf::from("/code/my-project/.git/zed-worktrees/my-branch") - ); - - // Trailing slash on setting (inside project) - assert_eq!( - worktree_path_for_branch(work_dir, "my-worktrees/", "branch"), - PathBuf::from("/code/my-project/my-worktrees/branch") - ); - } - impl RealGitRepository { /// Force a Git garbage collection on the repository. fn gc(&self) -> BoxFuture<'_, Result<()>> { diff --git a/crates/git_ui/src/worktree_picker.rs b/crates/git_ui/src/worktree_picker.rs index 6c35e7c99ffb8f6efa1a2bd7a07c2ded8d158668..91195e4eab4b023313f3b086da67c20d7f22f129 100644 --- a/crates/git_ui/src/worktree_picker.rs +++ b/crates/git_ui/src/worktree_picker.rs @@ -2,7 +2,7 @@ use anyhow::Context as _; use collections::HashSet; use fuzzy::StringMatchCandidate; -use git::repository::{Worktree as GitWorktree, validate_worktree_directory}; +use git::repository::Worktree as GitWorktree; use gpui::{ Action, App, AsyncWindowContext, Context, DismissEvent, Entity, EventEmitter, FocusHandle, Focusable, InteractiveElement, IntoElement, Modifiers, ModifiersChangedEvent, ParentElement, @@ -300,11 +300,10 @@ impl WorktreeListDelegate { .git .worktree_directory .clone(); - let original_repo = repo.original_repo_abs_path.clone(); - let directory = - validate_worktree_directory(&original_repo, &worktree_directory_setting)?; - let new_worktree_path = directory.join(&branch); - let receiver = repo.create_worktree(branch.clone(), directory, commit); + let new_worktree_path = + repo.path_for_new_linked_worktree(&branch, &worktree_directory_setting)?; + let receiver = + repo.create_worktree(branch.clone(), new_worktree_path.clone(), commit); anyhow::Ok((receiver, new_worktree_path)) })?; receiver.await??; diff --git a/crates/markdown_preview/Cargo.toml b/crates/markdown_preview/Cargo.toml index c72de7274a407c168e7a3cdd7a253070cc6f858a..782de627ec26273820bb3505b778a862659f315f 100644 --- a/crates/markdown_preview/Cargo.toml +++ b/crates/markdown_preview/Cargo.toml @@ -19,7 +19,6 @@ anyhow.workspace = true async-recursion.workspace = true collections.workspace = true editor.workspace = true -fs.workspace = true gpui.workspace = true html5ever.workspace = true language.workspace = true diff --git a/crates/markdown_preview/src/markdown_renderer.rs b/crates/markdown_preview/src/markdown_renderer.rs index 4d26b7e8958a04f1bb64abc5be5502e23896f313..a6666fd97dccf2adef7e70ac374314cd55ff5821 100644 --- a/crates/markdown_preview/src/markdown_renderer.rs +++ b/crates/markdown_preview/src/markdown_renderer.rs @@ -9,7 +9,6 @@ use crate::{ markdown_preview_view::MarkdownPreviewView, }; use collections::HashMap; -use fs::normalize_path; use gpui::{ AbsoluteLength, Animation, AnimationExt, AnyElement, App, AppContext as _, Context, Div, Element, ElementId, Entity, HighlightStyle, Hsla, ImageSource, InteractiveText, IntoElement, @@ -25,6 +24,7 @@ use std::{ }; use theme::{ActiveTheme, SyntaxTheme, ThemeSettings}; use ui::{CopyButton, LinkPreview, ToggleState, prelude::*, tooltip_container}; +use util::normalize_path; use workspace::{OpenOptions, OpenVisible, Workspace}; pub struct CheckboxClickedEvent { diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 75bd6cb59ff51499facb1d8fb7328881298c19c1..3e22857282bed51c703f3f7a7ee1b8af9a130bdb 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -5755,6 +5755,31 @@ impl Repository { }) } + /// If this is a linked worktree (*NOT* the main checkout of a repository), + /// returns the pathed for the linked worktree. + /// + /// Returns None if this is the main checkout. + pub fn linked_worktree_path(&self) -> Option<&Arc> { + if self.work_directory_abs_path != self.original_repo_abs_path { + Some(&self.work_directory_abs_path) + } else { + None + } + } + + pub fn path_for_new_linked_worktree( + &self, + branch_name: &str, + worktree_directory_setting: &str, + ) -> Result { + let original_repo = self.original_repo_abs_path.clone(); + let project_name = original_repo + .file_name() + .ok_or_else(|| anyhow!("git repo must have a directory name"))?; + let directory = worktrees_directory_for_repo(&original_repo, worktree_directory_setting)?; + Ok(directory.join(branch_name).join(project_name)) + } + pub fn worktrees(&mut self) -> oneshot::Receiver>> { let id = self.id; self.send_job(None, move |repo, _| async move { @@ -5784,25 +5809,25 @@ impl Repository { pub fn create_worktree( &mut self, - name: String, - directory: PathBuf, + branch_name: String, + path: PathBuf, commit: Option, ) -> oneshot::Receiver> { let id = self.id; self.send_job( - Some("git worktree add".into()), + Some(format!("git worktree add: {}", branch_name).into()), move |repo, _cx| async move { match repo { RepositoryState::Local(LocalRepositoryState { backend, .. }) => { - backend.create_worktree(name, directory, commit).await + backend.create_worktree(branch_name, path, commit).await } RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { client .request(proto::GitCreateWorktree { project_id: project_id.0, repository_id: id.to_proto(), - name, - directory: directory.to_string_lossy().to_string(), + name: branch_name, + directory: path.to_string_lossy().to_string(), commit, }) .await?; @@ -6716,6 +6741,94 @@ impl Repository { } } +/// If `path` is a git linked worktree checkout, resolves it to the main +/// repository's working directory path. Returns `None` if `path` is a normal +/// repository, not a git repo, or if resolution fails. +/// +/// Resolution works by: +/// 1. Reading the `.git` file to get the `gitdir:` pointer +/// 2. Following that to the worktree-specific git directory +/// 3. Reading the `commondir` file to find the shared `.git` directory +/// 4. Deriving the main repo's working directory from the common dir +pub async fn resolve_git_worktree_to_main_repo(fs: &dyn Fs, path: &Path) -> Option { + let dot_git = path.join(".git"); + let metadata = fs.metadata(&dot_git).await.ok()??; + if metadata.is_dir { + return None; // Normal repo, not a linked worktree + } + // It's a .git file — parse the gitdir: pointer + let content = fs.load(&dot_git).await.ok()?; + let gitdir_rel = content.strip_prefix("gitdir:")?.trim(); + let gitdir_abs = fs.canonicalize(&path.join(gitdir_rel)).await.ok()?; + // Read commondir to find the main .git directory + let commondir_content = fs.load(&gitdir_abs.join("commondir")).await.ok()?; + let common_dir = fs + .canonicalize(&gitdir_abs.join(commondir_content.trim())) + .await + .ok()?; + Some(git::repository::original_repo_path_from_common_dir( + &common_dir, + )) +} + +/// Validates that the resolved worktree directory is acceptable: +/// - The setting must not be an absolute path. +/// - The resolved path must be either a subdirectory of the working +/// directory or a subdirectory of its parent (i.e., a sibling). +/// +/// Returns `Ok(resolved_path)` or an error with a user-facing message. +pub fn worktrees_directory_for_repo( + original_repo_abs_path: &Path, + worktree_directory_setting: &str, +) -> Result { + // Check the original setting before trimming, since a path like "///" + // is absolute but becomes "" after stripping trailing separators. + // Also check for leading `/` or `\` explicitly, because on Windows + // `Path::is_absolute()` requires a drive letter — so `/tmp/worktrees` + // would slip through even though it's clearly not a relative path. + if Path::new(worktree_directory_setting).is_absolute() + || worktree_directory_setting.starts_with('/') + || worktree_directory_setting.starts_with('\\') + { + anyhow::bail!( + "git.worktree_directory must be a relative path, got: {worktree_directory_setting:?}" + ); + } + + if worktree_directory_setting.is_empty() { + anyhow::bail!("git.worktree_directory must not be empty"); + } + + let trimmed = worktree_directory_setting.trim_end_matches(['/', '\\']); + if trimmed == ".." { + anyhow::bail!("git.worktree_directory must not be \"..\" (use \"../some-name\" instead)"); + } + + let joined = original_repo_abs_path.join(trimmed); + let resolved = util::normalize_path(&joined); + let resolved = if resolved.starts_with(original_repo_abs_path) { + resolved + } else if let Some(repo_dir_name) = original_repo_abs_path.file_name() { + resolved.join(repo_dir_name) + } else { + resolved + }; + + let parent = original_repo_abs_path + .parent() + .unwrap_or(original_repo_abs_path); + + if !resolved.starts_with(parent) { + anyhow::bail!( + "git.worktree_directory resolved to {resolved:?}, which is outside \ + the project root and its parent directory. It must resolve to a \ + subdirectory of {original_repo_abs_path:?} or a sibling of it." + ); + } + + Ok(resolved) +} + fn get_permalink_in_rust_registry_src( provider_registry: Arc, path: PathBuf, diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 4f2ba9423f69bfc374b072142dbc4508191c3dc2..9231a5ea97730cad1ade5fde181b5519c7163684 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -47,6 +47,7 @@ pub use agent_server_store::{AgentId, AgentServerStore, AgentServersUpdated, Ext pub use git_store::{ ConflictRegion, ConflictSet, ConflictSetSnapshot, ConflictSetUpdate, git_traversal::{ChildEntriesGitIter, GitEntry, GitEntryRef, GitTraversal}, + worktrees_directory_for_repo, }; pub use manifest_tree::ManifestTree; pub use project_search::{Search, SearchResults}; diff --git a/crates/project/tests/integration/git_store.rs b/crates/project/tests/integration/git_store.rs index 82e92bc4f1cfb606fb09d5efd5d341ed2951c067..13cc16f0a0271e26ad5aac098325b1899734d51c 100644 --- a/crates/project/tests/integration/git_store.rs +++ b/crates/project/tests/integration/git_store.rs @@ -1176,14 +1176,13 @@ mod git_traversal { } mod git_worktrees { - use std::path::PathBuf; - use fs::FakeFs; use gpui::TestAppContext; + use project::worktrees_directory_for_repo; use serde_json::json; use settings::SettingsStore; + use std::path::{Path, PathBuf}; use util::path; - fn init_test(cx: &mut gpui::TestAppContext) { zlog::init_test(); @@ -1193,6 +1192,48 @@ mod git_worktrees { }); } + #[test] + fn test_validate_worktree_directory() { + let work_dir = Path::new("/code/my-project"); + + // Valid: sibling + assert!(worktrees_directory_for_repo(work_dir, "../worktrees").is_ok()); + + // Valid: subdirectory + assert!(worktrees_directory_for_repo(work_dir, ".git/zed-worktrees").is_ok()); + assert!(worktrees_directory_for_repo(work_dir, "my-worktrees").is_ok()); + + // Invalid: just ".." would resolve back to the working directory itself + let err = worktrees_directory_for_repo(work_dir, "..").unwrap_err(); + assert!(err.to_string().contains("must not be \"..\"")); + + // Invalid: ".." with trailing separators + let err = worktrees_directory_for_repo(work_dir, "..\\").unwrap_err(); + assert!(err.to_string().contains("must not be \"..\"")); + let err = worktrees_directory_for_repo(work_dir, "../").unwrap_err(); + assert!(err.to_string().contains("must not be \"..\"")); + + // Invalid: empty string would resolve to the working directory itself + let err = worktrees_directory_for_repo(work_dir, "").unwrap_err(); + assert!(err.to_string().contains("must not be empty")); + + // Invalid: absolute path + let err = worktrees_directory_for_repo(work_dir, "/tmp/worktrees").unwrap_err(); + assert!(err.to_string().contains("relative path")); + + // Invalid: "/" is absolute on Unix + let err = worktrees_directory_for_repo(work_dir, "/").unwrap_err(); + assert!(err.to_string().contains("relative path")); + + // Invalid: "///" is absolute + let err = worktrees_directory_for_repo(work_dir, "///").unwrap_err(); + assert!(err.to_string().contains("relative path")); + + // Invalid: escapes too far up + let err = worktrees_directory_for_repo(work_dir, "../../other-project/wt").unwrap_err(); + assert!(err.to_string().contains("outside")); + } + #[gpui::test] async fn test_git_worktrees_list_and_create(cx: &mut TestAppContext) { init_test(cx); @@ -1221,12 +1262,13 @@ mod git_worktrees { assert_eq!(worktrees.len(), 1); assert_eq!(worktrees[0].path, PathBuf::from(path!("/root"))); - let worktree_directory = PathBuf::from(path!("/root")); + let worktrees_directory = PathBuf::from(path!("/root")); + let worktree_1_directory = worktrees_directory.join("feature-branch"); cx.update(|cx| { repository.update(cx, |repository, _| { repository.create_worktree( "feature-branch".to_string(), - worktree_directory.clone(), + worktree_1_directory.clone(), Some("abc123".to_string()), ) }) @@ -1244,15 +1286,16 @@ mod git_worktrees { .unwrap(); assert_eq!(worktrees.len(), 2); assert_eq!(worktrees[0].path, PathBuf::from(path!("/root"))); - assert_eq!(worktrees[1].path, worktree_directory.join("feature-branch")); + assert_eq!(worktrees[1].path, worktree_1_directory); assert_eq!(worktrees[1].ref_name.as_ref(), "refs/heads/feature-branch"); assert_eq!(worktrees[1].sha.as_ref(), "abc123"); + let worktree_2_directory = worktrees_directory.join("bugfix-branch"); cx.update(|cx| { repository.update(cx, |repository, _| { repository.create_worktree( "bugfix-branch".to_string(), - worktree_directory.clone(), + worktree_2_directory.clone(), None, ) }) @@ -1271,24 +1314,18 @@ mod git_worktrees { .unwrap(); assert_eq!(worktrees.len(), 3); - let feature_worktree = worktrees + let worktree_1 = 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") - ); + assert_eq!(worktree_1.path, worktree_1_directory); - let bugfix_worktree = worktrees + let worktree_2 = 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"); + assert_eq!(worktree_2.path, worktree_2_directory); + assert_eq!(worktree_2.sha.as_ref(), "fake-sha"); } use crate::Project; @@ -1498,3 +1535,85 @@ mod trust_tests { }); } } + +mod resolve_worktree_tests { + use fs::FakeFs; + use gpui::TestAppContext; + use project::git_store::resolve_git_worktree_to_main_repo; + use serde_json::json; + use std::path::{Path, PathBuf}; + + #[gpui::test] + async fn test_resolve_git_worktree_to_main_repo(cx: &mut TestAppContext) { + let fs = FakeFs::new(cx.executor()); + // Set up a main repo with a worktree entry + fs.insert_tree( + "/main-repo", + json!({ + ".git": { + "worktrees": { + "feature": { + "commondir": "../../", + "HEAD": "ref: refs/heads/feature" + } + } + }, + "src": { "main.rs": "" } + }), + ) + .await; + // Set up a worktree checkout pointing back to the main repo + fs.insert_tree( + "/worktree-checkout", + json!({ + ".git": "gitdir: /main-repo/.git/worktrees/feature", + "src": { "main.rs": "" } + }), + ) + .await; + + let result = + resolve_git_worktree_to_main_repo(fs.as_ref(), Path::new("/worktree-checkout")).await; + assert_eq!(result, Some(PathBuf::from("/main-repo"))); + } + + #[gpui::test] + async fn test_resolve_git_worktree_normal_repo_returns_none(cx: &mut TestAppContext) { + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/repo", + json!({ + ".git": {}, + "src": { "main.rs": "" } + }), + ) + .await; + + let result = resolve_git_worktree_to_main_repo(fs.as_ref(), Path::new("/repo")).await; + assert_eq!(result, None); + } + + #[gpui::test] + async fn test_resolve_git_worktree_no_git_returns_none(cx: &mut TestAppContext) { + let fs = FakeFs::new(cx.executor()); + fs.insert_tree( + "/plain", + json!({ + "src": { "main.rs": "" } + }), + ) + .await; + + let result = resolve_git_worktree_to_main_repo(fs.as_ref(), Path::new("/plain")).await; + assert_eq!(result, None); + } + + #[gpui::test] + async fn test_resolve_git_worktree_nonexistent_returns_none(cx: &mut TestAppContext) { + let fs = FakeFs::new(cx.executor()); + + let result = + resolve_git_worktree_to_main_repo(fs.as_ref(), Path::new("/does-not-exist")).await; + assert_eq!(result, None); + } +} diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 4904d79ff73b903e01e6022fa47d7a7928213a5e..010e5c82e1cbab88062ff12481ae28478a89e6c9 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -517,6 +517,7 @@ impl RecentProjects { .await .log_err() .unwrap_or_default(); + let workspaces = workspace::resolve_worktree_workspaces(workspaces, fs.as_ref()).await; this.update_in(cx, move |this, window, cx| { this.picker.update(cx, move |picker, cx| { picker.delegate.set_workspaces(workspaces); @@ -1510,6 +1511,8 @@ impl RecentProjectsDelegate { .recent_workspaces_on_disk(fs.as_ref()) .await .unwrap_or_default(); + let workspaces = + workspace::resolve_worktree_workspaces(workspaces, fs.as_ref()).await; this.update_in(cx, move |picker, window, cx| { picker.delegate.set_workspaces(workspaces); picker diff --git a/crates/title_bar/src/title_bar.rs b/crates/title_bar/src/title_bar.rs index 6e96b52d6b78bccea6e1f7e4825508f0aa25e60a..dd901a8a21c35958a5973c36cf764454e5090d21 100644 --- a/crates/title_bar/src/title_bar.rs +++ b/crates/title_bar/src/title_bar.rs @@ -169,6 +169,28 @@ impl Render for TitleBar { let mut children = Vec::new(); + let mut project_name = None; + let mut repository = None; + let mut linked_worktree_name = None; + if let Some(worktree) = self.effective_active_worktree(cx) { + project_name = worktree + .read(cx) + .root_name() + .file_name() + .map(|name| SharedString::from(name.to_string())); + repository = self.get_repository_for_worktree(&worktree, cx); + linked_worktree_name = repository.as_ref().and_then(|repo| { + let path = repo.read(cx).linked_worktree_path()?; + let directory_name = path.file_name()?.to_str()?; + let unique_worktree_name = if directory_name != project_name.as_ref()?.as_str() { + directory_name.to_string() + } else { + path.parent()?.file_name()?.to_str()?.to_string() + }; + Some(SharedString::from(unique_worktree_name)) + }); + } + children.push( h_flex() .h_full() @@ -192,11 +214,18 @@ impl Render for TitleBar { .when(title_bar_settings.show_project_items, |title_bar| { title_bar .children(self.render_project_host(cx)) - .child(self.render_project_name(window, cx)) - }) - .when(title_bar_settings.show_branch_name, |title_bar| { - title_bar.children(self.render_project_branch(cx)) + .child(self.render_project_name(project_name, window, cx)) }) + .when_some( + repository.filter(|_| title_bar_settings.show_branch_name), + |title_bar, repository| { + title_bar.children(self.render_project_branch( + repository, + linked_worktree_name, + cx, + )) + }, + ) }) }) .on_mouse_down(MouseButton::Left, |_, _, cx| cx.stop_propagation()) @@ -739,14 +768,14 @@ impl TitleBar { ) } - pub fn render_project_name(&self, _: &mut Window, cx: &mut Context) -> impl IntoElement { + fn render_project_name( + &self, + name: Option, + _: &mut Window, + cx: &mut Context, + ) -> impl IntoElement { let workspace = self.workspace.clone(); - let name = self.effective_active_worktree(cx).map(|worktree| { - let worktree = worktree.read(cx); - SharedString::from(worktree.root_name().as_unix_str().to_string()) - }); - let is_project_selected = name.is_some(); let display_name = if let Some(ref name) = name { @@ -865,13 +894,17 @@ impl TitleBar { }) } - pub fn render_project_branch(&self, cx: &mut Context) -> Option { - let effective_worktree = self.effective_active_worktree(cx)?; - let repository = self.get_repository_for_worktree(&effective_worktree, cx)?; + fn render_project_branch( + &self, + repository: Entity, + linked_worktree_name: Option, + cx: &mut Context, + ) -> Option { let workspace = self.workspace.upgrade()?; let (branch_name, icon_info) = { let repo = repository.read(cx); + let branch_name = repo .branch .as_ref() @@ -904,6 +937,13 @@ impl TitleBar { (branch_name, icon_info) }; + let branch_name = branch_name?; + let button_text = if let Some(worktree_name) = linked_worktree_name { + format!("{}/{}", worktree_name, branch_name) + } else { + branch_name + }; + let settings = TitleBarSettings::get_global(cx); let effective_repository = Some(repository); @@ -921,7 +961,7 @@ impl TitleBar { )) }) .trigger_with_tooltip( - Button::new("project_branch_trigger", branch_name?) + Button::new("project_branch_trigger", button_text) .selected_style(ButtonStyle::Tinted(TintColor::Accent)) .label_size(LabelSize::Small) .color(Color::Muted) diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index 020f04578b0dc325c09130b195f4cce95393126a..08f4fa84613436e6c5c34b3410df324e666b5fb8 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -2359,6 +2359,86 @@ VALUES {placeholders};"# } } +type WorkspaceEntry = ( + WorkspaceId, + SerializedWorkspaceLocation, + PathList, + DateTime, +); + +/// Resolves workspace entries whose paths are git linked worktree checkouts +/// to their main repository paths. +/// +/// For each workspace entry: +/// - If any path is a linked worktree checkout, all worktree paths in that +/// entry are resolved to their main repository paths, producing a new +/// `PathList`. +/// - The resolved entry is then deduplicated against existing entries: if a +/// workspace with the same paths already exists, the entry with the most +/// recent timestamp is kept. +pub async fn resolve_worktree_workspaces( + workspaces: impl IntoIterator, + fs: &dyn Fs, +) -> Vec { + // First pass: resolve worktree paths to main repo paths concurrently. + let resolved = futures::future::join_all(workspaces.into_iter().map(|entry| async move { + let paths = entry.2.paths(); + if paths.is_empty() { + return entry; + } + + // Resolve each path concurrently + let resolved_paths = futures::future::join_all( + paths + .iter() + .map(|path| project::git_store::resolve_git_worktree_to_main_repo(fs, path)), + ) + .await; + + // If no paths were resolved, this entry is not a worktree — keep as-is + if resolved_paths.iter().all(|r| r.is_none()) { + return entry; + } + + // Build new path list, substituting resolved paths + let new_paths: Vec = paths + .iter() + .zip(resolved_paths.iter()) + .map(|(original, resolved)| { + resolved + .as_ref() + .cloned() + .unwrap_or_else(|| original.clone()) + }) + .collect(); + + let new_path_refs: Vec<&Path> = new_paths.iter().map(|p| p.as_path()).collect(); + (entry.0, entry.1, PathList::new(&new_path_refs), entry.3) + })) + .await; + + // Second pass: deduplicate by PathList. + // When two entries resolve to the same paths, keep the one with the + // more recent timestamp. + let mut seen: collections::HashMap, usize> = collections::HashMap::default(); + let mut result: Vec = Vec::new(); + + for entry in resolved { + let key: Vec = entry.2.paths().to_vec(); + if let Some(&existing_idx) = seen.get(&key) { + // Keep the entry with the more recent timestamp + if entry.3 > result[existing_idx].3 { + result[existing_idx] = entry; + } + } else { + seen.insert(key, result.len()); + result.push(entry); + } + } + + result +} + pub fn delete_unloaded_items( alive_items: Vec, workspace_id: WorkspaceId, @@ -4489,4 +4569,116 @@ mod tests { before the process exits." ); } + + #[gpui::test] + async fn test_resolve_worktree_workspaces(cx: &mut gpui::TestAppContext) { + let fs = fs::FakeFs::new(cx.executor()); + + // Main repo with a linked worktree entry + fs.insert_tree( + "/repo", + json!({ + ".git": { + "worktrees": { + "feature": { + "commondir": "../../", + "HEAD": "ref: refs/heads/feature" + } + } + }, + "src": { "main.rs": "" } + }), + ) + .await; + + // Linked worktree checkout pointing back to /repo + fs.insert_tree( + "/worktree", + json!({ + ".git": "gitdir: /repo/.git/worktrees/feature", + "src": { "main.rs": "" } + }), + ) + .await; + + // A plain non-git project + fs.insert_tree( + "/plain-project", + json!({ + "src": { "main.rs": "" } + }), + ) + .await; + + // Another normal git repo (used in mixed-path entry) + fs.insert_tree( + "/other-repo", + json!({ + ".git": {}, + "src": { "lib.rs": "" } + }), + ) + .await; + + let t0 = Utc::now() - chrono::Duration::hours(4); + let t1 = Utc::now() - chrono::Duration::hours(3); + let t2 = Utc::now() - chrono::Duration::hours(2); + let t3 = Utc::now() - chrono::Duration::hours(1); + + let workspaces = vec![ + // 1: Main checkout of /repo (opened earlier) + ( + WorkspaceId(1), + SerializedWorkspaceLocation::Local, + PathList::new(&["/repo"]), + t0, + ), + // 2: Linked worktree of /repo (opened more recently) + // Should dedup with #1; more recent timestamp wins. + ( + WorkspaceId(2), + SerializedWorkspaceLocation::Local, + PathList::new(&["/worktree"]), + t1, + ), + // 3: Mixed-path workspace: one root is a linked worktree, + // the other is a normal repo. The worktree path should be + // resolved; the normal path kept as-is. + ( + WorkspaceId(3), + SerializedWorkspaceLocation::Local, + PathList::new(&["/other-repo", "/worktree"]), + t2, + ), + // 4: Non-git project — passed through unchanged. + ( + WorkspaceId(4), + SerializedWorkspaceLocation::Local, + PathList::new(&["/plain-project"]), + t3, + ), + ]; + + let result = resolve_worktree_workspaces(workspaces, fs.as_ref()).await; + + // Should have 3 entries: #1 and #2 deduped into one, plus #3 and #4. + assert_eq!(result.len(), 3); + + // First entry: /repo — deduplicated from #1 and #2. + // Keeps the position of #1 (first seen), but with #2's later timestamp. + assert_eq!(result[0].2.paths(), &[PathBuf::from("/repo")]); + assert_eq!(result[0].3, t1); + + // Second entry: mixed-path workspace with worktree resolved. + // /worktree → /repo, so paths become [/other-repo, /repo] (sorted). + assert_eq!( + result[1].2.paths(), + &[PathBuf::from("/other-repo"), PathBuf::from("/repo")] + ); + assert_eq!(result[1].0, WorkspaceId(3)); + + // Third entry: non-git project, unchanged. + assert_eq!(result[2].2.paths(), &[PathBuf::from("/plain-project")]); + assert_eq!(result[2].0, WorkspaceId(4)); + } } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index 6c159bd5c9ca06902033358d37dc8810e38c35c3..c87634d4f7db10a430d26d558ba987604842835f 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -82,7 +82,7 @@ pub use persistence::{ DockStructure, ItemId, SerializedMultiWorkspace, SerializedWorkspaceLocation, SessionWorkspace, }, - read_serialized_multi_workspaces, + read_serialized_multi_workspaces, resolve_worktree_workspaces, }; use postage::stream::Stream; use project::{