From 1bca0c5a91014c98c1d24c0172edf83474d6cf77 Mon Sep 17 00:00:00 2001 From: Stefan Junker <1181362+steveej@users.noreply.github.com> Date: Thu, 19 Mar 2026 15:20:08 +0100 Subject: [PATCH] git: Use the real gitdir for checkpoints (#51563) ## Summary Zed's git checkpoint code was assuming that repository-local metadata always lives under `/.git/...`. That assumption breaks for repositories where `.git` is a file instead of a directory, most notably: - git submodules - git worktrees In those layouts, `.git` contains a `gitdir: ...` pointer to the real git directory. When checkpoint creation or restore tries to build paths like `.git/index-.tmp` or `.git/info/exclude`, those path operations fail with `Not a directory (os error 20)`. In practice, that shows up as checkpoint failures in ACP threads for worktrees and submodules. ## Root Cause `GitBinary` tracked the repository working directory, but it did not track the resolved git directory. As a result, checkpoint-related helpers derived temporary index and exclude paths from the working tree: - temp index path creation - copying the default index into the temp index - exclude override handling That works for a plain repository where `.git` is a directory, but not when `.git` is a pointer file. ## Changes - Thread the resolved git directory from `RealGitRepository::path()` into `GitBinary`. - Add a dedicated `git_directory` field to `GitBinary`. - Use `git_directory` instead of `/.git` for: - temp index file paths - copying the default `index` - `info/exclude` override paths - Keep the working directory unchanged for command execution; only metadata path resolution changes. - Add a regression test that verifies temp index files are created under the real gitdir, not under `/.git`. ## Why this approach The minimal correct fix is to distinguish between: - the working directory used to run git commands, and - the actual git directory used to store repository metadata Git itself already makes that distinction, and `git2::Repository::path()` gives us the resolved gitdir directly. Reusing that value keeps the fix small and avoids special-casing submodules or worktrees elsewhere. ## User-visible impact Before this change, checkpoint operations could fail in repositories where `.git` is not a directory, producing errors like: Not a directory (os error 20) After this change, checkpoint-related git metadata is read and written from the real gitdir, so ACP checkpoints work in submodules and worktrees the same way they do in regular repositories. ## Testing - `cargo fmt -p git` - `cargo test -p git --lib` - `cargo clippy -p git --lib --tests -- -D warnings` Added regression coverage: - `test_path_for_index_id_uses_real_git_directory` If you want, I can also compress this into a more GitHub-PR-native version with sections like `## Problem`, `## Fix`, and `## Validation`. Release Notes - Fixed `.git` handling when `.git` is a file instead of directory, e.g. when using worktrees or submodules. --- crates/git/src/repository.rs | 65 +++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/crates/git/src/repository.rs b/crates/git/src/repository.rs index d67086d20e2ea3e5b38b1fdb4c0bcb52dc9b4126..e868fb22343fd43d88c5432f11e7bf1f6e6ab728 100644 --- a/crates/git/src/repository.rs +++ b/crates/git/src/repository.rs @@ -943,6 +943,7 @@ impl RealGitRepository { self.any_git_binary_path.clone(), self.working_directory() .with_context(|| "Can't run git commands without a working directory")?, + self.path(), self.executor.clone(), self.is_trusted(), )) @@ -997,6 +998,7 @@ pub async fn get_git_committer(cx: &AsyncApp) -> GitCommitter { let git = GitBinary::new( git_binary_path.unwrap_or(PathBuf::from("git")), paths::home_dir().clone(), + paths::home_dir().join(".git"), cx.background_executor().clone(), true, ); @@ -2154,6 +2156,7 @@ impl GitRepository for RealGitRepository { cx: AsyncApp, ) -> BoxFuture<'_, Result> { let working_directory = self.working_directory(); + let git_directory = self.path(); let executor = cx.background_executor().clone(); let git_binary_path = self.system_git_binary_path.clone(); let is_trusted = self.is_trusted(); @@ -2165,6 +2168,7 @@ impl GitRepository for RealGitRepository { let git = GitBinary::new( git_binary_path, working_directory, + git_directory, executor.clone(), is_trusted, ); @@ -2196,6 +2200,7 @@ impl GitRepository for RealGitRepository { cx: AsyncApp, ) -> BoxFuture<'_, Result> { let working_directory = self.working_directory(); + let git_directory = self.path(); let executor = cx.background_executor().clone(); let git_binary_path = self.system_git_binary_path.clone(); let is_trusted = self.is_trusted(); @@ -2207,6 +2212,7 @@ impl GitRepository for RealGitRepository { let git = GitBinary::new( git_binary_path, working_directory, + git_directory, executor.clone(), is_trusted, ); @@ -2236,6 +2242,7 @@ impl GitRepository for RealGitRepository { cx: AsyncApp, ) -> BoxFuture<'_, Result> { let working_directory = self.working_directory(); + let git_directory = self.path(); let remote_name = format!("{}", fetch_options); let git_binary_path = self.system_git_binary_path.clone(); let executor = cx.background_executor().clone(); @@ -2248,6 +2255,7 @@ impl GitRepository for RealGitRepository { let git = GitBinary::new( git_binary_path, working_directory, + git_directory, executor.clone(), is_trusted, ); @@ -2900,6 +2908,7 @@ async fn exclude_files(git: &GitBinary) -> Result { pub(crate) struct GitBinary { git_binary_path: PathBuf, working_directory: PathBuf, + git_directory: PathBuf, executor: BackgroundExecutor, index_file_path: Option, envs: HashMap, @@ -2910,12 +2919,14 @@ impl GitBinary { pub(crate) fn new( git_binary_path: PathBuf, working_directory: PathBuf, + git_directory: PathBuf, executor: BackgroundExecutor, is_trusted: bool, ) -> Self { Self { git_binary_path, working_directory, + git_directory, executor, index_file_path: None, envs: HashMap::default(), @@ -2961,12 +2972,9 @@ impl GitBinary { // Copy the default index file so that Git doesn't have to rebuild the // whole index from scratch. This might fail if this is an empty repository. - smol::fs::copy( - self.working_directory.join(".git").join("index"), - &index_file_path, - ) - .await - .ok(); + smol::fs::copy(self.git_directory.join("index"), &index_file_path) + .await + .ok(); self.index_file_path = Some(index_file_path.clone()); let result = f(self).await; @@ -2980,19 +2988,13 @@ impl GitBinary { } pub async fn with_exclude_overrides(&self) -> Result { - let path = self - .working_directory - .join(".git") - .join("info") - .join("exclude"); + let path = self.git_directory.join("info").join("exclude"); GitExcludeOverride::new(path).await } fn path_for_index_id(&self, id: Uuid) -> PathBuf { - self.working_directory - .join(".git") - .join(format!("index-{}.tmp", id)) + self.git_directory.join(format!("index-{}.tmp", id)) } pub async fn run(&self, args: &[S]) -> Result @@ -3317,6 +3319,7 @@ mod tests { let git = GitBinary::new( PathBuf::from("git"), dir.path().to_path_buf(), + dir.path().join(".git"), cx.executor(), false, ); @@ -3330,6 +3333,7 @@ mod tests { let git = GitBinary::new( PathBuf::from("git"), dir.path().to_path_buf(), + dir.path().join(".git"), cx.executor(), false, ); @@ -3349,6 +3353,7 @@ mod tests { let git = GitBinary::new( PathBuf::from("git"), dir.path().to_path_buf(), + dir.path().join(".git"), cx.executor(), false, ); @@ -3374,6 +3379,7 @@ mod tests { let git = GitBinary::new( PathBuf::from("git"), dir.path().to_path_buf(), + dir.path().join(".git"), cx.executor(), true, ); @@ -3392,6 +3398,7 @@ mod tests { let git = GitBinary::new( PathBuf::from("git"), dir.path().to_path_buf(), + dir.path().join(".git"), cx.executor(), true, ); @@ -3406,6 +3413,27 @@ mod tests { ); } + #[gpui::test] + async fn test_path_for_index_id_uses_real_git_directory(cx: &mut TestAppContext) { + cx.executor().allow_parking(); + let working_directory = PathBuf::from("/code/worktree"); + let git_directory = PathBuf::from("/code/repo/.git/modules/worktree"); + let git = GitBinary::new( + PathBuf::from("git"), + working_directory, + git_directory.clone(), + cx.executor(), + false, + ); + + let path = git.path_for_index_id(Uuid::nil()); + + assert_eq!( + path, + git_directory.join(format!("index-{}.tmp", Uuid::nil())) + ); + } + #[gpui::test] async fn test_checkpoint_basic(cx: &mut TestAppContext) { disable_git_global_config(); @@ -4090,13 +4118,20 @@ mod tests { /// Force a Git garbage collection on the repository. fn gc(&self) -> BoxFuture<'_, Result<()>> { let working_directory = self.working_directory(); + let git_directory = self.path(); let git_binary_path = self.any_git_binary_path.clone(); let executor = self.executor.clone(); self.executor .spawn(async move { let git_binary_path = git_binary_path.clone(); let working_directory = working_directory?; - let git = GitBinary::new(git_binary_path, working_directory, executor, true); + let git = GitBinary::new( + git_binary_path, + working_directory, + git_directory, + executor, + true, + ); git.run(&["gc", "--prune"]).await?; Ok(()) })