From 4c9c92325824ab72f125dd252947a31fd051ec6d Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Fri, 11 Aug 2023 08:36:37 +0900
Subject: [PATCH] cli: show warning if external diff generator exited with
 non-zero status

---
 cli/src/merge_tools.rs         | 27 ++++++++++++++++++---------
 cli/tests/test_diff_command.rs |  7 +++++--
 2 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/cli/src/merge_tools.rs b/cli/src/merge_tools.rs
index bec0990fe..f317a0a69 100644
--- a/cli/src/merge_tools.rs
+++ b/cli/src/merge_tools.rs
@@ -16,7 +16,7 @@ use std::collections::HashMap;
 use std::fs::File;
 use std::io::{self, Write};
 use std::path::{Path, PathBuf};
-use std::process::{Command, Stdio};
+use std::process::{Command, ExitStatus, Stdio};
 use std::sync::Arc;
 
 use config::ConfigError;
@@ -61,13 +61,8 @@ pub enum ExternalToolError {
         #[source]
         source: std::io::Error,
     },
-    #[error(
-        "Tool exited with a non-zero code (run with --verbose to see the exact invocation). Exit code: {}.",
-         exit_status.code().map(|c| c.to_string()).unwrap_or_else(|| "<unknown>".to_string())
-    )]
-    ToolAborted {
-        exit_status: std::process::ExitStatus,
-    },
+    #[error("{}", format_tool_aborted(.exit_status))]
+    ToolAborted { exit_status: ExitStatus },
     #[error("I/O error: {0}")]
     Io(#[source] std::io::Error),
 }
@@ -430,7 +425,7 @@ pub fn edit_diff(
 
 /// Generates textual diff by the specified `tool`, and writes into `writer`.
 pub fn generate_diff(
-    _ui: &Ui,
+    ui: &Ui,
     writer: &mut dyn Write,
     left_tree: &Tree,
     right_tree: &Tree,
@@ -462,6 +457,9 @@ pub fn generate_diff(
     // will exit with 1 if inputs are different.
     let exit_status = child.wait().map_err(ExternalToolError::Io)?;
     tracing::info!(?cmd, ?exit_status, "The external diff generator exited:");
+    if !exit_status.success() {
+        writeln!(ui.warning(), "{}", format_tool_aborted(&exit_status)).ok();
+    }
     Ok(())
 }
 
@@ -624,6 +622,17 @@ fn editor_args_from_settings(
     }
 }
 
+fn format_tool_aborted(exit_status: &ExitStatus) -> String {
+    let code = exit_status
+        .code()
+        .map(|c| c.to_string())
+        .unwrap_or_else(|| "<unknown>".to_string());
+    format!(
+        "Tool exited with a non-zero code (run with --verbose to see the exact invocation). Exit \
+         code: {code}."
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs
index 923d1cd4c..059467379 100644
--- a/cli/tests/test_diff_command.rs
+++ b/cli/tests/test_diff_command.rs
@@ -678,8 +678,8 @@ fn test_diff_external_tool() {
 
     // Non-zero exit code isn't an error
     std::fs::write(&edit_script, "print diff\0fail").unwrap();
-    insta::assert_snapshot!(
-        test_env.jj_cmd_success(&repo_path, &["show", "--tool=fake-diff-editor"]), @r###"
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["show", "--tool=fake-diff-editor"]);
+    insta::assert_snapshot!(stdout, @r###"
     Commit ID: 0cba70c72186eabb5a2f91be63a8366b9f6da6c6
     Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp
     Author: Test User <test.user@example.com> (2001-02-03 04:05:08.000 +07:00)
@@ -689,4 +689,7 @@ fn test_diff_external_tool() {
 
     diff
     "###);
+    insta::assert_snapshot!(stderr, @r###"
+    Tool exited with a non-zero code (run with --verbose to see the exact invocation). Exit code: 1.
+    "###);
 }