Fix commondir discovery for git submodules (#28802)

The implementation of commondir discovery in #27885 was wrong, most
significantly for submodules but also for worktrees in rarer cases. The
correct procedure, implemented in this PR, is:

> If `.git` is a file, look at the `gitdir` it points to. If that
directory has a file called `commondir`, read that file to find the
commondir. (This is what happens for worktrees.) Otherwise, the
commondir is the same as the gitdir. (This is what happens for
submodules.)

Release Notes:

- N/A

crates/fs/src/fs.rs

@@ -1333,13 +1333,23 @@ impl FakeFs {
             let Some((git_dir_entry, canonical_path)) = state.try_read_path(&path, true) else {
                 anyhow::bail!("pointed-to git dir {path:?} not found")
             };
-            let FakeFsEntry::Dir { git_repo_state, .. } = &mut *git_dir_entry.lock() else {
+            let FakeFsEntry::Dir {
+                git_repo_state,
+                entries,
+                ..
+            } = &mut *git_dir_entry.lock()
+            else {
                 anyhow::bail!("gitfile points to a non-directory")
             };
-            let common_dir = canonical_path
+            let common_dir = if let Some(child) = entries.get("commondir") {
-                .ancestors()
+                Path::new(
-                .find(|ancestor| ancestor.ends_with(".git"))
+                    std::str::from_utf8(child.lock().file_content("commondir".as_ref())?)
-                .ok_or_else(|| anyhow!("repository dir not contained in any .git"))?;
+                        .context("commondir content")?,
+                )
+                .to_owned()
+            } else {
+                canonical_path.clone()
+            };
             let repo_state = git_repo_state.get_or_insert_with(|| {
                 Arc::new(Mutex::new(FakeGitRepositoryState::new(
                     state.git_event_tx.clone(),
@@ -1347,7 +1357,7 @@ impl FakeFs {
             });
             let mut repo_state = repo_state.lock();
 
-            let result = f(&mut repo_state, &canonical_path, common_dir);
+            let result = f(&mut repo_state, &canonical_path, &common_dir);
 
             if emit_git_event {
                 state.emit_event([(canonical_path, None)]);

crates/project/src/project_tests.rs

@@ -8273,17 +8273,34 @@ async fn test_git_worktrees_and_submodules(cx: &mut gpui::TestAppContext) {
         json!({
             ".git": {
                 "worktrees": {
-                    "some-worktree": {}
+                    "some-worktree": {
+                        "commondir": "../..\n"
+                    }
                 },
+                "modules": {
+                    "subdir": {
+                        "some-submodule": {
+                            // For is_git_dir
+                            "HEAD": "",
+                            "config": "",
+                        }
+                    }
+                }
             },
             "src": {
                 "a.txt": "A",
             },
             "some-worktree": {
-                ".git": "gitdir: ../.git/worktrees/some-worktree",
+                ".git": "gitdir: ../.git/worktrees/some-worktree\n",
                 "src": {
                     "b.txt": "B",
                 }
+            },
+            "subdir": {
+                "some-submodule": {
+                    ".git": "gitdir: ../../.git/modules/subdir/some-submodule\n",
+                    "c.txt": "C",
+                }
             }
         }),
     )
@@ -8315,9 +8332,11 @@ async fn test_git_worktrees_and_submodules(cx: &mut gpui::TestAppContext) {
         [
             Path::new(path!("/project")).into(),
             Path::new(path!("/project/some-worktree")).into(),
+            Path::new(path!("/project/subdir/some-submodule")).into(),
         ]
     );
 
+    // Generate a git-related event for the worktree and check that it's refreshed.
     fs.with_git_state(
         path!("/project/some-worktree/.git").as_ref(),
         true,
@@ -8359,6 +8378,45 @@ async fn test_git_worktrees_and_submodules(cx: &mut gpui::TestAppContext) {
             StatusCode::Modified.worktree(),
         );
     });
+
+    // The same for the submodule.
+    fs.with_git_state(
+        path!("/project/subdir/some-submodule/.git").as_ref(),
+        true,
+        |state| {
+            state.head_contents.insert("c.txt".into(), "c".to_owned());
+            state.index_contents.insert("c.txt".into(), "c".to_owned());
+        },
+    )
+    .unwrap();
+    cx.run_until_parked();
+
+    let buffer = project
+        .update(cx, |project, cx| {
+            project.open_local_buffer(path!("/project/subdir/some-submodule/c.txt"), cx)
+        })
+        .await
+        .unwrap();
+    let (submodule_repo, barrier) = project.update(cx, |project, cx| {
+        let (repo, _) = project
+            .git_store()
+            .read(cx)
+            .repository_and_path_for_buffer_id(buffer.read(cx).remote_id(), cx)
+            .unwrap();
+        pretty_assertions::assert_eq!(
+            repo.read(cx).work_directory_abs_path,
+            Path::new(path!("/project/subdir/some-submodule")).into(),
+        );
+        let barrier = repo.update(cx, |repo, _| repo.barrier());
+        (repo.clone(), barrier)
+    });
+    barrier.await.unwrap();
+    submodule_repo.update(cx, |repo, _| {
+        pretty_assertions::assert_eq!(
+            repo.status_for_path(&"c.txt".into()).unwrap().status,
+            StatusCode::Modified.worktree(),
+        );
+    });
 }
 
 #[gpui::test]

crates/worktree/src/worktree.rs

@@ -3120,32 +3120,12 @@ impl BackgroundScannerState {
             .as_path()
             .into();
 
-        let mut common_dir_abs_path = dot_git_abs_path.clone();
+        let (repository_dir_abs_path, common_dir_abs_path) =
-        let mut repository_dir_abs_path = dot_git_abs_path.clone();
+            discover_git_paths(&dot_git_abs_path, fs);
-        // Parse .git if it's a "gitfile" pointing to a repository directory elsewhere.
-        if let Some(dot_git_contents) = smol::block_on(fs.load(&dot_git_abs_path)).ok() {
-            if let Some(path) = dot_git_contents.strip_prefix("gitdir:") {
-                let path = path.trim();
-                let path = dot_git_abs_path
-                    .parent()
-                    .unwrap_or(Path::new(""))
-                    .join(path);
-                if let Some(path) = smol::block_on(fs.canonicalize(&path)).log_err() {
-                    repository_dir_abs_path = Path::new(&path).into();
-                    common_dir_abs_path = repository_dir_abs_path.clone();
-                    if let Some(ancestor_dot_git) = path
-                        .ancestors()
-                        .skip(1)
-                        .find(|ancestor| smol::block_on(is_git_dir(ancestor, fs)))
-                    {
-                        common_dir_abs_path = ancestor_dot_git.into();
-                    }
-                }
-            } else {
-                log::error!("failed to parse contents of .git file: {dot_git_contents:?}");
-            }
-        };
         watcher.add(&common_dir_abs_path).log_err();
+        if !repository_dir_abs_path.starts_with(&common_dir_abs_path) {
+            watcher.add(&repository_dir_abs_path).log_err();
+        }
 
         let work_directory_id = work_dir_entry.id;
 
@@ -5508,3 +5488,40 @@ impl CreatedEntry {
         }
     }
 }
+
+fn parse_gitfile(content: &str) -> anyhow::Result<&Path> {
+    let path = content
+        .strip_prefix("gitdir:")
+        .ok_or_else(|| anyhow!("failed to parse gitfile content {content:?}"))?;
+    Ok(Path::new(path.trim()))
+}
+
+fn discover_git_paths(dot_git_abs_path: &Arc<Path>, fs: &dyn Fs) -> (Arc<Path>, Arc<Path>) {
+    let mut repository_dir_abs_path = dot_git_abs_path.clone();
+    let mut common_dir_abs_path = dot_git_abs_path.clone();
+
+    if let Some(path) = smol::block_on(fs.load(&dot_git_abs_path))
+        .ok()
+        .as_ref()
+        .and_then(|contents| parse_gitfile(contents).log_err())
+    {
+        let path = dot_git_abs_path
+            .parent()
+            .unwrap_or(Path::new(""))
+            .join(path);
+        if let Some(path) = smol::block_on(fs.canonicalize(&path)).log_err() {
+            repository_dir_abs_path = Path::new(&path).into();
+            common_dir_abs_path = repository_dir_abs_path.clone();
+            if let Some(commondir_contents) = smol::block_on(fs.load(&path.join("commondir"))).ok()
+            {
+                if let Some(commondir_path) =
+                    smol::block_on(fs.canonicalize(&path.join(commondir_contents.trim()))).log_err()
+                {
+                    common_dir_abs_path = commondir_path.as_path().into();
+                }
+            }
+        }
+    };
+
+    (repository_dir_abs_path, common_dir_abs_path)
+}