Better error message git

Guillaume Launay created

In the error modal print the error that output the git command instead of a generic message

Change summary

crates/fs/src/fake_git_repo.rs     |  11 ++
crates/git/src/git.rs              |   1 
crates/git/src/repository.rs       | 119 +++++++++++++++++++++++++++----
crates/git_ui/src/branch_picker.rs |  65 ++++++++++------
crates/git_ui/src/git_ui.rs        |  13 ++-
5 files changed, 164 insertions(+), 45 deletions(-)

Detailed changes

crates/fs/src/fake_git_repo.rs 🔗

@@ -350,6 +350,17 @@ impl GitRepository for FakeGitRepository {
         })
     }
 
+    fn rename_branch(&self, new_name: String) -> BoxFuture<'_, Result<()>> {
+        self.with_state_async(true, move |state| {
+            if let Some(current_branch) = &state.current_branch_name {
+                state.branches.remove(current_branch);
+                state.branches.insert(new_name.clone());
+                state.current_branch_name = Some(new_name);
+            }
+            Ok(())
+        })
+    }
+
     fn blame(&self, path: RepoPath, _content: Rope) -> BoxFuture<'_, Result<git::blame::Blame>> {
         self.with_state_async(false, move |state| {
             state

crates/git/src/git.rs 🔗

@@ -11,6 +11,7 @@ use anyhow::{Context as _, Result};
 pub use git2 as libgit;
 use gpui::{Action, actions};
 pub use repository::WORK_DIRECTORY_REPO_PATH;
+pub use repository::{GitCommandOutput, RemoteCommandOutput};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use std::ffi::OsStr;

crates/git/src/repository.rs 🔗

@@ -132,6 +132,32 @@ pub struct RemoteCommandOutput {
     pub stderr: String,
 }
 
+#[derive(Debug, Clone)]
+pub struct GitCommandOutput {
+    pub stdout: String,
+    pub stderr: String,
+}
+
+impl std::fmt::Display for GitCommandOutput {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let stderr = self.stderr.trim();
+        let message = if stderr.is_empty() {
+            self.stdout.trim()
+        } else {
+            stderr
+        };
+        write!(f, "{}", message)
+    }
+}
+
+impl std::error::Error for GitCommandOutput {}
+
+impl GitCommandOutput {
+    pub fn is_empty(&self) -> bool {
+        self.stdout.is_empty() && self.stderr.is_empty()
+    }
+}
+
 impl RemoteCommandOutput {
     pub fn is_empty(&self) -> bool {
         self.stdout.is_empty() && self.stderr.is_empty()
@@ -1049,19 +1075,22 @@ impl GitRepository for RealGitRepository {
         let working_directory = self.working_directory();
         let git_binary_path = self.git_binary_path.clone();
         let executor = self.executor.clone();
+        let name_clone = name.clone();
         let branch = self.executor.spawn(async move {
             let repo = repo.lock();
-            let branch = if let Ok(branch) = repo.find_branch(&name, BranchType::Local) {
+            let branch = if let Ok(branch) = repo.find_branch(&name_clone, BranchType::Local) {
                 branch
-            } else if let Ok(revision) = repo.find_branch(&name, BranchType::Remote) {
-                let (_, branch_name) = name.split_once("/").context("Unexpected branch format")?;
+            } else if let Ok(revision) = repo.find_branch(&name_clone, BranchType::Remote) {
+                let (_, branch_name) = name_clone
+                    .split_once("/")
+                    .context("Unexpected branch format")?;
                 let revision = revision.get();
                 let branch_commit = revision.peel_to_commit()?;
                 let mut branch = repo.branch(&branch_name, &branch_commit, false)?;
-                branch.set_upstream(Some(&name))?;
+                branch.set_upstream(Some(&name_clone))?;
                 branch
             } else {
-                anyhow::bail!("Branch not found");
+                anyhow::bail!("Branch '{}' not found", name_clone);
             };
 
             Ok(branch
@@ -1074,11 +1103,18 @@ impl GitRepository for RealGitRepository {
             .spawn(async move {
                 let branch = branch.await?;
 
-                GitBinary::new(git_binary_path, working_directory?, executor)
-                    .run(&["checkout", &branch])
-                    .await?;
-
-                anyhow::Ok(())
+                match GitBinary::new(git_binary_path, working_directory?, executor)
+                    .run_with_output(&["checkout", &branch])
+                    .await
+                {
+                    Ok(_) => anyhow::Ok(()),
+                    Err(e) => {
+                        if let Some(git_error) = e.downcast_ref::<GitBranchCommandError>() {
+                            anyhow::bail!("{}", git_error.stderr.trim());
+                        }
+                        Err(e)
+                    }
+                }
             })
             .boxed()
     }
@@ -1102,10 +1138,18 @@ impl GitRepository for RealGitRepository {
 
         self.executor
             .spawn(async move {
-                GitBinary::new(git_binary_path, working_directory?, executor)
-                    .run(&["branch", "-m", &new_name])
-                    .await?;
-                Ok(())
+                match GitBinary::new(git_binary_path, working_directory?, executor)
+                    .run_with_output(&["branch", "-m", &new_name])
+                    .await
+                {
+                    Ok(_) => Ok(()),
+                    Err(e) => {
+                        if let Some(git_error) = e.downcast_ref::<GitBranchCommandError>() {
+                            anyhow::bail!("{}", git_error.stderr.trim());
+                        }
+                        Err(e)
+                    }
+                }
             })
             .boxed()
     }
@@ -1823,6 +1867,31 @@ impl GitBinary {
         Ok(stdout)
     }
 
+    pub async fn run_with_output<S>(
+        &self,
+        args: impl IntoIterator<Item = S>,
+    ) -> Result<GitCommandOutput>
+    where
+        S: AsRef<OsStr>,
+    {
+        let mut command = self.build_command(args);
+        let output = command.output().await?;
+
+        let stdout = String::from_utf8_lossy(&output.stdout).to_string();
+        let stderr = String::from_utf8_lossy(&output.stderr).to_string();
+
+        if !output.status.success() {
+            return Err(GitBranchCommandError {
+                stdout: stdout.clone(),
+                stderr: stderr.clone(),
+                status: output.status,
+            }
+            .into());
+        }
+
+        Ok(GitCommandOutput { stdout, stderr })
+    }
+
     /// Returns the result of the command without trimming the trailing newline.
     pub async fn run_raw<S>(&self, args: impl IntoIterator<Item = S>) -> Result<String>
     where
@@ -1834,6 +1903,7 @@ impl GitBinary {
             output.status.success(),
             GitBinaryCommandError {
                 stdout: String::from_utf8_lossy(&output.stdout).to_string(),
+                stderr: String::from_utf8_lossy(&output.stderr).to_string(),
                 status: output.status,
             }
         );
@@ -1856,12 +1926,31 @@ impl GitBinary {
 }
 
 #[derive(Error, Debug)]
-#[error("Git command failed: {stdout}")]
+#[error("Git command failed: {}", .stderr.trim().if_empty(.stdout.trim()))]
 struct GitBinaryCommandError {
     stdout: String,
+    stderr: String,
+    status: ExitStatus,
+}
+
+#[derive(Error, Debug)]
+#[error("Git branch command failed: {}", .stderr.trim().if_empty(.stdout.trim()))]
+struct GitBranchCommandError {
+    stdout: String,
+    stderr: String,
     status: ExitStatus,
 }
 
+trait StringExt {
+    fn if_empty<'a>(&'a self, fallback: &'a str) -> &'a str;
+}
+
+impl StringExt for str {
+    fn if_empty<'a>(&'a self, fallback: &'a str) -> &'a str {
+        if self.is_empty() { fallback } else { self }
+    }
+}
+
 async fn run_git_command(
     env: Arc<HashMap<String, String>>,
     ask_pass: AskPassDelegate,

crates/git_ui/src/branch_picker.rs 🔗

@@ -239,25 +239,28 @@ impl BranchListDelegate {
         let new_branch_name = new_branch_name.to_string().replace(' ', "-");
         cx.spawn(async move |_, cx| {
             if let Some(based_branch) = from_branch {
-                repo.update(cx, |repo, _| repo.change_branch(based_branch.to_string()))?
-                    .await??;
+                match repo.update(cx, |repo, _| repo.change_branch(based_branch.to_string()))?.await {
+                    Ok(Ok(_)) => {}
+                    Ok(Err(error)) => return Err(error),
+                    Err(_) => return Err(anyhow::anyhow!("Operation was canceled")),
+                }
+            }
+            
+            match repo.update(cx, |repo, _| repo.create_branch(new_branch_name.clone()))?.await {
+                Ok(Ok(_)) => {}
+                Ok(Err(error)) => return Err(error),
+                Err(_) => return Err(anyhow::anyhow!("Operation was canceled")),
+            }
+            
+            match repo.update(cx, |repo, _| repo.change_branch(new_branch_name))?.await {
+                Ok(Ok(_)) => {}
+                Ok(Err(error)) => return Err(error),
+                Err(_) => return Err(anyhow::anyhow!("Operation was canceled")),
             }
-
-            repo.update(cx, |repo, _| {
-                repo.create_branch(new_branch_name.to_string())
-            })?
-            .await??;
-            repo.update(cx, |repo, _| {
-                repo.change_branch(new_branch_name.to_string())
-            })?
-            .await??;
 
             Ok(())
         })
-        .detach_and_prompt_err("Failed to create branch", window, cx, |e, _, _| {
-            Some(e.to_string())
-        });
-        cx.emit(DismissEvent);
+        .detach_and_prompt_err("Failed to create branch", window, cx, |_, _, _| None);
     }
 }
 
@@ -409,6 +412,8 @@ impl PickerDelegate for BranchListDelegate {
         cx.spawn_in(window, {
             let branch = entry.branch.clone();
             async move |picker, cx| {
+                let branch_name = branch.name().to_string();
+
                 let branch_change_task = picker.update(cx, |this, cx| {
                     let repo = this
                         .delegate
@@ -420,23 +425,31 @@ impl PickerDelegate for BranchListDelegate {
                     let mut cx = cx.to_async();
 
                     anyhow::Ok(async move {
-                        repo.update(&mut cx, |repo, _| {
-                            repo.change_branch(branch.name().to_string())
-                        })?
-                        .await?
+                        repo.update(&mut cx, |repo, _| repo.change_branch(branch_name))?
+                            .await?
                     })
                 })??;
 
-                branch_change_task.await?;
-
-                picker.update(cx, |_, cx| {
-                    cx.emit(DismissEvent);
+                match branch_change_task.await {
+                    Ok(_) => {
+                        let _ = picker.update(cx, |_, cx| {
+                            cx.emit(DismissEvent);
+                            anyhow::Ok(())
+                        })?;
+                    }
+                    Err(error) => {
+                        let _ = picker.update(cx, |_, cx| {
+                            cx.emit(DismissEvent);
+                            anyhow::Ok(())
+                        })?;
+                        return Err(error);
+                    }
+                }
 
-                    anyhow::Ok(())
-                })
+                anyhow::Ok(())
             }
         })
-        .detach_and_prompt_err("Failed to change branch", window, cx, |_, _, _| None);
+        .detach_and_prompt_err("Failed to switch branch", window, cx, |_, _, _| None);
     }
 
     fn dismissed(&mut self, _: &mut Window, cx: &mut Context<Picker<Self>>) {

crates/git_ui/src/git_ui.rs 🔗

@@ -12,6 +12,7 @@ use ui::{
 use workspace::{ModalView, notifications::DetachAndPromptErr};
 
 mod blame_ui;
+
 use git::{
     repository::{Branch, Upstream, UpstreamTracking, UpstreamTrackingStatus},
     status::{FileStatus, StatusCode, UnmergedStatus, UnmergedStatusCode},
@@ -278,10 +279,14 @@ impl RenameBranchModal {
 
         let repo = self.repo.clone();
         cx.spawn(async move |_, cx| {
-            repo.update(cx, |repo, _| repo.rename_branch(new_name))?
-                .await??;
-
-            Ok(())
+            match repo
+                .update(cx, |repo, _| repo.rename_branch(new_name.clone()))?
+                .await
+            {
+                Ok(Ok(_)) => Ok(()),
+                Ok(Err(error)) => Err(error),
+                Err(_) => Err(anyhow::anyhow!("Operation was canceled")),
+            }
         })
         .detach_and_prompt_err("Failed to rename branch", window, cx, |_, _, _| None);
         cx.emit(DismissEvent);