git: Use the real gitdir for checkpoints (#51563)

Stefan Junker created 3 weeks ago

## Summary

Zed's git checkpoint code was assuming that repository-local metadata
always lives under `<working-directory>/.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-<uuid>.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 `<working-directory>/.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 `<working-directory>/.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.

Change summary

crates/git/src/repository.rs | 65 +++++++++++++++++++++++++++++--------
1 file changed, 50 insertions(+), 15 deletions(-)

Detailed changes

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<RemoteCommandOutput>> {
         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<RemoteCommandOutput>> {
         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<RemoteCommandOutput>> {
         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<GitExcludeOverride> {
 pub(crate) struct GitBinary {
     git_binary_path: PathBuf,
     working_directory: PathBuf,
+    git_directory: PathBuf,
     executor: BackgroundExecutor,
     index_file_path: Option<PathBuf>,
     envs: HashMap<String, String>,
@@ -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<GitExcludeOverride> {
-        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<S>(&self, args: &[S]) -> Result<String>
@@ -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(())
                 })