git: Use environment from login shell to search for system git binary, and prefer it to the bundled binary (#39302)

Cole Miller and Lukas Wirth created 1 month ago

Closes #38571

Release Notes:

- git: Fixed git features not working when git was installed in an
unusual location.

---------

Co-authored-by: Lukas Wirth <[e-mail]>

Change summary

crates/fs/src/fake_git_repo.rs      |   4 
crates/fs/src/fs.rs                 |  29 +++++--
crates/git/src/repository.rs        | 124 ++++++++++++++++++++----------
crates/project/src/git_store.rs     |   5 +
crates/project/src/project_tests.rs |   4 
5 files changed, 112 insertions(+), 54 deletions(-)

Detailed changes

crates/fs/src/fake_git_repo.rs

@@ -608,7 +608,9 @@ mod tests {
         .await;
         fs.with_git_state(Path::new("/foo/.git"), true, |_git| {})
             .unwrap();
-        let repository = fs.open_repo(Path::new("/foo/.git")).unwrap();
+        let repository = fs
+            .open_repo(Path::new("/foo/.git"), Some("git".as_ref()))
+            .unwrap();
 
         let checkpoint_1 = repository.checkpoint().await.unwrap();
         fs.write(Path::new("/foo/b"), b"IPSUM").await.unwrap();

crates/fs/src/fs.rs

@@ -134,7 +134,11 @@ pub trait Fs: Send + Sync {
         Arc<dyn Watcher>,
     );
 
-    fn open_repo(&self, abs_dot_git: &Path) -> Option<Arc<dyn GitRepository>>;
+    fn open_repo(
+        &self,
+        abs_dot_git: &Path,
+        system_git_binary_path: Option<&Path>,
+    ) -> Option<Arc<dyn GitRepository>>;
     async fn git_init(&self, abs_work_directory: &Path, fallback_branch_name: String)
     -> Result<()>;
     async fn git_clone(&self, repo_url: &str, abs_work_directory: &Path) -> Result<()>;
@@ -247,7 +251,7 @@ impl From<MTime> for proto::Timestamp {
 }
 
 pub struct RealFs {
-    git_binary_path: Option<PathBuf>,
+    bundled_git_binary_path: Option<PathBuf>,
     executor: BackgroundExecutor,
 }
 
@@ -325,7 +329,7 @@ pub struct RealWatcher {}
 impl RealFs {
     pub fn new(git_binary_path: Option<PathBuf>, executor: BackgroundExecutor) -> Self {
         Self {
-            git_binary_path,
+            bundled_git_binary_path: git_binary_path,
             executor,
         }
     }
@@ -828,10 +832,15 @@ impl Fs for RealFs {
         )
     }
 
-    fn open_repo(&self, dotgit_path: &Path) -> Option<Arc<dyn GitRepository>> {
+    fn open_repo(
+        &self,
+        dotgit_path: &Path,
+        system_git_binary_path: Option<&Path>,
+    ) -> Option<Arc<dyn GitRepository>> {
         Some(Arc::new(RealGitRepository::new(
             dotgit_path,
-            self.git_binary_path.clone(),
+            self.bundled_git_binary_path.clone(),
+            system_git_binary_path.map(|path| path.to_path_buf()),
             self.executor.clone(),
         )?))
     }
@@ -2425,7 +2434,11 @@ impl Fs for FakeFs {
         )
     }
 
-    fn open_repo(&self, abs_dot_git: &Path) -> Option<Arc<dyn GitRepository>> {
+    fn open_repo(
+        &self,
+        abs_dot_git: &Path,
+        _system_git_binary: Option<&Path>,
+    ) -> Option<Arc<dyn GitRepository>> {
         use util::ResultExt as _;
 
         self.with_git_state_and_paths(
@@ -3077,7 +3090,7 @@ mod tests {
         // With the file handle still open, the file should be replaced
         // https://github.com/zed-industries/zed/issues/30054
         let fs = RealFs {
-            git_binary_path: None,
+            bundled_git_binary_path: None,
             executor,
         };
         let temp_dir = TempDir::new().unwrap();
@@ -3095,7 +3108,7 @@ mod tests {
     #[gpui::test]
     async fn test_realfs_atomic_write_non_existing_file(executor: BackgroundExecutor) {
         let fs = RealFs {
-            git_binary_path: None,
+            bundled_git_binary_path: None,
             executor,
         };
         let temp_dir = TempDir::new().unwrap();

crates/git/src/repository.rs

@@ -545,21 +545,25 @@ impl std::fmt::Debug for dyn GitRepository {
 
 pub struct RealGitRepository {
     pub repository: Arc<Mutex<git2::Repository>>,
-    pub git_binary_path: PathBuf,
+    pub system_git_binary_path: Option<PathBuf>,
+    pub any_git_binary_path: PathBuf,
     executor: BackgroundExecutor,
 }
 
 impl RealGitRepository {
     pub fn new(
         dotgit_path: &Path,
-        git_binary_path: Option<PathBuf>,
+        bundled_git_binary_path: Option<PathBuf>,
+        system_git_binary_path: Option<PathBuf>,
         executor: BackgroundExecutor,
     ) -> Option<Self> {
+        let any_git_binary_path = system_git_binary_path.clone().or(bundled_git_binary_path)?;
         let workdir_root = dotgit_path.parent()?;
         let repository = git2::Repository::open(workdir_root).log_err()?;
         Some(Self {
             repository: Arc::new(Mutex::new(repository)),
-            git_binary_path: git_binary_path.unwrap_or_else(|| PathBuf::from("git")),
+            system_git_binary_path,
+            any_git_binary_path,
             executor,
         })
     }
@@ -640,11 +644,12 @@ impl GitRepository for RealGitRepository {
     }
 
     fn show(&self, commit: String) -> BoxFuture<'_, Result<CommitDetails>> {
+        let git_binary_path = self.any_git_binary_path.clone();
         let working_directory = self.working_directory();
         self.executor
             .spawn(async move {
                 let working_directory = working_directory?;
-                let output = new_smol_command("git")
+                let output = new_smol_command(git_binary_path)
                     .current_dir(&working_directory)
                     .args([
                         "--no-optional-locks",
@@ -681,8 +686,9 @@ impl GitRepository for RealGitRepository {
         else {
             return future::ready(Err(anyhow!("no working directory"))).boxed();
         };
+        let git_binary_path = self.any_git_binary_path.clone();
         cx.background_spawn(async move {
-            let show_output = util::command::new_smol_command("git")
+            let show_output = util::command::new_smol_command(&git_binary_path)
                 .current_dir(&working_directory)
                 .args([
                     "--no-optional-locks",
@@ -705,7 +711,7 @@ impl GitRepository for RealGitRepository {
             let parent_sha = lines.next().unwrap().trim().trim_end_matches('\0');
             let changes = parse_git_diff_name_status(lines.next().unwrap_or(""));
 
-            let mut cat_file_process = util::command::new_smol_command("git")
+            let mut cat_file_process = util::command::new_smol_command(&git_binary_path)
                 .current_dir(&working_directory)
                 .args(["--no-optional-locks", "cat-file", "--batch=%(objectsize)"])
                 .stdin(Stdio::piped())
@@ -809,7 +815,7 @@ impl GitRepository for RealGitRepository {
                 ResetMode::Soft => "--soft",
             };
 
-            let output = new_smol_command(&self.git_binary_path)
+            let output = new_smol_command(&self.any_git_binary_path)
                 .envs(env.iter())
                 .current_dir(&working_directory?)
                 .args(["reset", mode_flag, &commit])
@@ -832,7 +838,7 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         async move {
             if paths.is_empty() {
                 return Ok(());
@@ -909,7 +915,7 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, anyhow::Result<()>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 let working_directory = working_directory?;
@@ -972,10 +978,11 @@ impl GitRepository for RealGitRepository {
 
     fn revparse_batch(&self, revs: Vec<String>) -> BoxFuture<'_, Result<Vec<Option<String>>>> {
         let working_directory = self.working_directory();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 let working_directory = working_directory?;
-                let mut process = new_smol_command("git")
+                let mut process = new_smol_command(&git_binary_path)
                     .current_dir(&working_directory)
                     .args([
                         "--no-optional-locks",
@@ -1030,7 +1037,7 @@ impl GitRepository for RealGitRepository {
     }
 
     fn status(&self, path_prefixes: &[RepoPath]) -> Task<Result<GitStatus>> {
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         let working_directory = match self.working_directory() {
             Ok(working_directory) => working_directory,
             Err(e) => return Task::ready(Err(e)),
@@ -1054,7 +1061,7 @@ impl GitRepository for RealGitRepository {
     }
 
     fn stash_entries(&self) -> BoxFuture<'_, Result<GitStash>> {
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         let working_directory = self.working_directory();
         self.executor
             .spawn(async move {
@@ -1076,7 +1083,7 @@ impl GitRepository for RealGitRepository {
 
     fn branches(&self) -> BoxFuture<'_, Result<Vec<Branch>>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 let fields = [
@@ -1145,7 +1152,7 @@ impl GitRepository for RealGitRepository {
     fn change_branch(&self, name: String) -> BoxFuture<'_, Result<()>> {
         let repo = self.repository.clone();
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         let executor = self.executor.clone();
         let branch = self.executor.spawn(async move {
             let repo = repo.lock();
@@ -1193,7 +1200,7 @@ impl GitRepository for RealGitRepository {
     }
 
     fn rename_branch(&self, branch: String, new_name: String) -> BoxFuture<'_, Result<()>> {
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         let working_directory = self.working_directory();
         let executor = self.executor.clone();
 
@@ -1209,7 +1216,7 @@ impl GitRepository for RealGitRepository {
 
     fn blame(&self, path: RepoPath, content: Rope) -> BoxFuture<'_, Result<crate::blame::Blame>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
 
         let remote_url = self
             .remote_url("upstream")
@@ -1231,7 +1238,7 @@ impl GitRepository for RealGitRepository {
 
     fn diff(&self, diff: DiffType) -> BoxFuture<'_, Result<String>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 let args = match diff {
@@ -1262,7 +1269,7 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 if !paths.is_empty() {
@@ -1290,7 +1297,7 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
 
         self.executor
             .spawn(async move {
@@ -1320,9 +1327,10 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
-                let mut cmd = new_smol_command("git");
+                let mut cmd = new_smol_command(&git_binary_path);
                 cmd.current_dir(&working_directory?)
                     .envs(env.iter())
                     .args(["stash", "push", "--quiet"])
@@ -1348,9 +1356,10 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
-                let mut cmd = new_smol_command("git");
+                let mut cmd = new_smol_command(git_binary_path);
                 let mut args = vec!["stash".to_string(), "pop".to_string()];
                 if let Some(index) = index {
                     args.push(format!("stash@{{{}}}", index));
@@ -1377,9 +1386,10 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
-                let mut cmd = new_smol_command("git");
+                let mut cmd = new_smol_command(git_binary_path);
                 let mut args = vec!["stash".to_string(), "apply".to_string()];
                 if let Some(index) = index {
                     args.push(format!("stash@{{{}}}", index));
@@ -1406,9 +1416,10 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
-                let mut cmd = new_smol_command("git");
+                let mut cmd = new_smol_command(git_binary_path);
                 let mut args = vec!["stash".to_string(), "drop".to_string()];
                 if let Some(index) = index {
                     args.push(format!("stash@{{{}}}", index));
@@ -1437,9 +1448,10 @@ impl GitRepository for RealGitRepository {
         env: Arc<HashMap<String, String>>,
     ) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
-                let mut cmd = new_smol_command("git");
+                let mut cmd = new_smol_command(git_binary_path);
                 cmd.current_dir(&working_directory?)
                     .envs(env.iter())
                     .args(["commit", "--quiet", "-m"])
@@ -1481,9 +1493,11 @@ impl GitRepository for RealGitRepository {
     ) -> BoxFuture<'_, Result<RemoteCommandOutput>> {
         let working_directory = self.working_directory();
         let executor = cx.background_executor().clone();
+        let git_binary_path = self.system_git_binary_path.clone();
         async move {
+            let git_binary_path = git_binary_path.context("git not found on $PATH, can't push")?;
             let working_directory = working_directory?;
-            let mut command = new_smol_command("git");
+            let mut command = new_smol_command(git_binary_path);
             command
                 .envs(env.iter())
                 .current_dir(&working_directory)
@@ -1513,8 +1527,10 @@ impl GitRepository for RealGitRepository {
     ) -> BoxFuture<'_, Result<RemoteCommandOutput>> {
         let working_directory = self.working_directory();
         let executor = cx.background_executor().clone();
+        let git_binary_path = self.system_git_binary_path.clone();
         async move {
-            let mut command = new_smol_command("git");
+            let git_binary_path = git_binary_path.context("git not found on $PATH, can't pull")?;
+            let mut command = new_smol_command(git_binary_path);
             command
                 .envs(env.iter())
                 .current_dir(&working_directory?)
@@ -1538,9 +1554,11 @@ impl GitRepository for RealGitRepository {
     ) -> BoxFuture<'_, Result<RemoteCommandOutput>> {
         let working_directory = self.working_directory();
         let remote_name = format!("{}", fetch_options);
+        let git_binary_path = self.system_git_binary_path.clone();
         let executor = cx.background_executor().clone();
         async move {
-            let mut command = new_smol_command("git");
+            let git_binary_path = git_binary_path.context("git not found on $PATH, can't fetch")?;
+            let mut command = new_smol_command(git_binary_path);
             command
                 .envs(env.iter())
                 .current_dir(&working_directory?)
@@ -1555,7 +1573,7 @@ impl GitRepository for RealGitRepository {
 
     fn get_remotes(&self, branch_name: Option<String>) -> BoxFuture<'_, Result<Vec<Remote>>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 let working_directory = working_directory?;
@@ -1601,7 +1619,7 @@ impl GitRepository for RealGitRepository {
 
     fn check_for_pushed_commit(&self) -> BoxFuture<'_, Result<Vec<SharedString>>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         self.executor
             .spawn(async move {
                 let working_directory = working_directory?;
@@ -1660,7 +1678,7 @@ impl GitRepository for RealGitRepository {
 
     fn checkpoint(&self) -> BoxFuture<'static, Result<GitRepositoryCheckpoint>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
         let executor = self.executor.clone();
         self.executor
             .spawn(async move {
@@ -1693,7 +1711,7 @@ impl GitRepository for RealGitRepository {
 
     fn restore_checkpoint(&self, checkpoint: GitRepositoryCheckpoint) -> BoxFuture<'_, Result<()>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
 
         let executor = self.executor.clone();
         self.executor
@@ -1732,7 +1750,7 @@ impl GitRepository for RealGitRepository {
         right: GitRepositoryCheckpoint,
     ) -> BoxFuture<'_, Result<bool>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
 
         let executor = self.executor.clone();
         self.executor
@@ -1770,7 +1788,7 @@ impl GitRepository for RealGitRepository {
         target_checkpoint: GitRepositoryCheckpoint,
     ) -> BoxFuture<'_, Result<String>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
 
         let executor = self.executor.clone();
         self.executor
@@ -1791,7 +1809,7 @@ impl GitRepository for RealGitRepository {
 
     fn default_branch(&self) -> BoxFuture<'_, Result<Option<SharedString>>> {
         let working_directory = self.working_directory();
-        let git_binary_path = self.git_binary_path.clone();
+        let git_binary_path = self.any_git_binary_path.clone();
 
         let executor = self.executor.clone();
         self.executor
@@ -2259,8 +2277,13 @@ mod tests {
         let file_path = repo_dir.path().join("file");
         smol::fs::write(&file_path, "initial").await.unwrap();
 
-        let repo =
-            RealGitRepository::new(&repo_dir.path().join(".git"), None, cx.executor()).unwrap();
+        let repo = RealGitRepository::new(
+            &repo_dir.path().join(".git"),
+            None,
+            Some("git".into()),
+            cx.executor(),
+        )
+        .unwrap();
         repo.stage_paths(vec![repo_path("file")], Arc::new(HashMap::default()))
             .await
             .unwrap();
@@ -2335,8 +2358,13 @@ mod tests {
 
         let repo_dir = tempfile::tempdir().unwrap();
         git2::Repository::init(repo_dir.path()).unwrap();
-        let repo =
-            RealGitRepository::new(&repo_dir.path().join(".git"), None, cx.executor()).unwrap();
+        let repo = RealGitRepository::new(
+            &repo_dir.path().join(".git"),
+            None,
+            Some("git".into()),
+            cx.executor(),
+        )
+        .unwrap();
 
         smol::fs::write(repo_dir.path().join("foo"), "foo")
             .await
@@ -2374,8 +2402,13 @@ mod tests {
 
         let repo_dir = tempfile::tempdir().unwrap();
         git2::Repository::init(repo_dir.path()).unwrap();
-        let repo =
-            RealGitRepository::new(&repo_dir.path().join(".git"), None, cx.executor()).unwrap();
+        let repo = RealGitRepository::new(
+            &repo_dir.path().join(".git"),
+            None,
+            Some("git".into()),
+            cx.executor(),
+        )
+        .unwrap();
 
         smol::fs::write(repo_dir.path().join("file1"), "content1")
             .await
@@ -2418,8 +2451,13 @@ mod tests {
             .await
             .unwrap();
 
-        let repo =
-            RealGitRepository::new(&repo_dir.path().join(".git"), None, cx.executor()).unwrap();
+        let repo = RealGitRepository::new(
+            &repo_dir.path().join(".git"),
+            None,
+            Some("git".into()),
+            cx.executor(),
+        )
+        .unwrap();
 
         // initial commit
         repo.stage_paths(vec![repo_path("main.rs")], Arc::new(HashMap::default()))
@@ -2490,7 +2528,7 @@ mod tests {
         /// Force a Git garbage collection on the repository.
         fn gc(&self) -> BoxFuture<'_, Result<()>> {
             let working_directory = self.working_directory();
-            let git_binary_path = self.git_binary_path.clone();
+            let git_binary_path = self.any_git_binary_path.clone();
             let executor = self.executor.clone();
             self.executor
                 .spawn(async move {

crates/project/src/git_store.rs

@@ -4607,9 +4607,12 @@ impl Repository {
                     log::error!("failed to get working directory environment for repository {work_directory_abs_path:?}");
                     HashMap::default()
                 });
+            let search_paths = environment.get("PATH").map(|val| val.to_owned());
             let backend = cx
                 .background_spawn(async move {
-                    fs.open_repo(&dot_git_abs_path)
+                    let system_git_binary_path = search_paths.and_then(|search_paths| which::which_in("git", Some(search_paths), &work_directory_abs_path).ok())
+                        .or_else(|| which::which("git").ok());
+                    fs.open_repo(&dot_git_abs_path, system_git_binary_path.as_deref())
                         .with_context(|| format!("opening repository at {dot_git_abs_path:?}"))
                 })
                 .await?;

crates/project/src/project_tests.rs

@@ -7797,7 +7797,9 @@ async fn test_staging_random_hunks(
         path!("/dir/.git").as_ref(),
         &[("file.txt", index_text.clone())],
     );
-    let repo = fs.open_repo(path!("/dir/.git").as_ref()).unwrap();
+    let repo = fs
+        .open_repo(path!("/dir/.git").as_ref(), Some("git".as_ref()))
+        .unwrap();
 
     let project = Project::test(fs.clone(), [path!("/dir").as_ref()], cx).await;
     let buffer = project