Use `commondir` from libgit2 instead of walking fs (#22028)

Michael Sloan created

Release Notes:

- N/A

Change summary

crates/git/src/repository.rs    | 38 +++++++++++++++++++++++++---------
crates/worktree/src/worktree.rs | 32 ++++++++++++----------------
2 files changed, 42 insertions(+), 28 deletions(-)

Detailed changes

crates/git/src/repository.rs 🔗

@@ -58,8 +58,17 @@ pub trait GitRepository: Send + Sync {
 
     fn blame(&self, path: &Path, content: Rope) -> Result<crate::blame::Blame>;
 
-    /// Returns the path to the repository, typically the `.git` folder.
-    fn dot_git_dir(&self) -> PathBuf;
+    /// Returns the absolute path to the repository. For worktrees, this will be the path to the
+    /// worktree's gitdir within the main repository (typically `.git/worktrees/<name>`).
+    fn path(&self) -> PathBuf;
+
+    /// Returns the absolute path to the ".git" dir for the main repository, typically a `.git`
+    /// folder. For worktrees, this will be the path to the repository the worktree was created
+    /// from. Otherwise, this is the same value as `path()`.
+    ///
+    /// Git documentation calls this the "commondir", and for git CLI is overridden by
+    /// `GIT_COMMON_DIR`.
+    fn main_repository_path(&self) -> PathBuf;
 
     /// Updates the index to match the worktree at the given paths.
     ///
@@ -109,11 +118,16 @@ impl GitRepository for RealGitRepository {
         }
     }
 
-    fn dot_git_dir(&self) -> PathBuf {
+    fn path(&self) -> PathBuf {
         let repo = self.repository.lock();
         repo.path().into()
     }
 
+    fn main_repository_path(&self) -> PathBuf {
+        let repo = self.repository.lock();
+        repo.commondir().into()
+    }
+
     fn load_index_text(&self, path: &RepoPath) -> Option<String> {
         fn logic(repo: &git2::Repository, path: &RepoPath) -> Result<Option<String>> {
             const STAGE_NORMAL: i32 = 0;
@@ -344,7 +358,7 @@ pub struct FakeGitRepository {
 
 #[derive(Debug, Clone)]
 pub struct FakeGitRepositoryState {
-    pub dot_git_dir: PathBuf,
+    pub path: PathBuf,
     pub event_emitter: smol::channel::Sender<PathBuf>,
     pub head_contents: HashMap<RepoPath, String>,
     pub index_contents: HashMap<RepoPath, String>,
@@ -361,9 +375,9 @@ impl FakeGitRepository {
 }
 
 impl FakeGitRepositoryState {
-    pub fn new(dot_git_dir: PathBuf, event_emitter: smol::channel::Sender<PathBuf>) -> Self {
+    pub fn new(path: PathBuf, event_emitter: smol::channel::Sender<PathBuf>) -> Self {
         FakeGitRepositoryState {
-            dot_git_dir,
+            path,
             event_emitter,
             head_contents: Default::default(),
             index_contents: Default::default(),
@@ -405,9 +419,13 @@ impl GitRepository for FakeGitRepository {
         vec![]
     }
 
-    fn dot_git_dir(&self) -> PathBuf {
+    fn path(&self) -> PathBuf {
         let state = self.state.lock();
-        state.dot_git_dir.clone()
+        state.path.clone()
+    }
+
+    fn main_repository_path(&self) -> PathBuf {
+        self.path()
     }
 
     fn status(&self, path_prefixes: &[RepoPath]) -> Result<GitStatus> {
@@ -458,7 +476,7 @@ impl GitRepository for FakeGitRepository {
         state.current_branch_name = Some(name.to_owned());
         state
             .event_emitter
-            .try_send(state.dot_git_dir.clone())
+            .try_send(state.path.clone())
             .expect("Dropped repo change event");
         Ok(())
     }
@@ -468,7 +486,7 @@ impl GitRepository for FakeGitRepository {
         state.branches.insert(name.to_owned());
         state
             .event_emitter
-            .try_send(state.dot_git_dir.clone())
+            .try_send(state.path.clone())
             .expect("Dropped repo change event");
         Ok(())
     }

crates/worktree/src/worktree.rs 🔗

@@ -3354,18 +3354,23 @@ impl BackgroundScannerState {
         let t0 = Instant::now();
         let repository = fs.open_repo(&dot_git_abs_path)?;
 
-        let actual_repo_path = repository.dot_git_dir();
+        let repository_path = repository.path();
+        watcher.add(&repository_path).log_err()?;
 
-        let actual_dot_git_dir_abs_path = smol::block_on(find_git_dir(&actual_repo_path, fs))?;
-        watcher.add(&actual_repo_path).log_err()?;
-
-        let dot_git_worktree_abs_path = if actual_dot_git_dir_abs_path.as_ref() == dot_git_abs_path
-        {
+        let actual_dot_git_dir_abs_path = repository.main_repository_path();
+        let dot_git_worktree_abs_path = if actual_dot_git_dir_abs_path == dot_git_abs_path {
             None
         } else {
             // The two paths could be different because we opened a git worktree.
-            // When that happens, the .git path in the worktree (`dot_git_abs_path`) is a file that
-            // points to the worktree-subdirectory in the actual .git directory (`git_dir_path`)
+            // When that happens:
+            //
+            // * `dot_git_abs_path` is a file that points to the worktree-subdirectory in the actual
+            // .git directory.
+            //
+            // * `repository_path` is the worktree-subdirectory.
+            //
+            // * `actual_dot_git_dir_abs_path` is the path to the actual .git directory. In git
+            // documentation this is called the "commondir".
             watcher.add(&dot_git_abs_path).log_err()?;
             Some(Arc::from(dot_git_abs_path))
         };
@@ -3400,7 +3405,7 @@ impl BackgroundScannerState {
             git_dir_scan_id: 0,
             status_scan_id: 0,
             repo_ptr: repository.clone(),
-            dot_git_dir_abs_path: actual_dot_git_dir_abs_path,
+            dot_git_dir_abs_path: actual_dot_git_dir_abs_path.into(),
             dot_git_worktree_abs_path,
             current_merge_head_shas: Default::default(),
         };
@@ -3429,15 +3434,6 @@ async fn is_git_dir(path: &Path, fs: &dyn Fs) -> bool {
     matches!(config_metadata, Ok(Some(_)))
 }
 
-async fn find_git_dir(path: &Path, fs: &dyn Fs) -> Option<Arc<Path>> {
-    for ancestor in path.ancestors() {
-        if is_git_dir(ancestor, fs).await {
-            return Some(Arc::from(ancestor));
-        }
-    }
-    None
-}
-
 async fn build_gitignore(abs_path: &Path, fs: &dyn Fs) -> Result<Gitignore> {
     let contents = fs.load(abs_path).await?;
     let parent = abs_path.parent().unwrap_or_else(|| Path::new("/"));