From f6acee41dfa209262b6506fe52c87ee3de7b0777 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 1 May 2022 10:45:22 +0900
Subject: [PATCH] diff_edit: load command arguments from merge-tools.<name>
 config

Apparently, I need to pass `--merge` option to use kdiff3 as a diff editor.

We could add `diff-editor-args` or extend `diff-editor` to a list of command
arguments, but we'll eventually add stock merge tools and the configuration
would look like:

    [merge-tools.<name>]
    program = ...
    diff-args = [...]
    edit-args = [...]
    merge-args = [...]
---
 CHANGELOG.md     |  7 +++++++
 Cargo.lock       |  1 +
 Cargo.toml       |  1 +
 src/diff_edit.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++----
 4 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8d3e93e10..625a01622 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -66,6 +66,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   will create a branch named after the revision's change ID, so you don't have
   to create a branch yourself.
 
+* Diff editor command arguments can now be specified by config file.
+  Example:
+
+      [merge-tools.kdiff3]
+      program = "kdiff3"
+      edit-args = ["--merge", "--cs", "CreateBakFiles=0"]
+
 ### Fixed bugs
 
 * When rebasing a conflict where one side modified a file and the other side
diff --git a/Cargo.lock b/Cargo.lock
index 6ce916e8a..0662c86aa 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -748,6 +748,7 @@ dependencies = [
  "predicates",
  "rand",
  "regex",
+ "serde",
  "tempfile",
  "test-case",
  "textwrap 0.15.0",
diff --git a/Cargo.toml b/Cargo.toml
index 672c2a614..c7a65cece 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -52,6 +52,7 @@ pest = "2.1.3"
 pest_derive = "2.1.0"
 rand = "0.8.5"
 regex = "1.5.5"
+serde = { version = "1.0", features = ["derive"] }
 tempfile = "3.3.0"
 textwrap = "0.15.0"
 thiserror = "1.0.31"
diff --git a/src/diff_edit.rs b/src/diff_edit.rs
index 2d5b5f469..91dbcfe75 100644
--- a/src/diff_edit.rs
+++ b/src/diff_edit.rs
@@ -18,6 +18,7 @@ use std::path::{Path, PathBuf};
 use std::process::Command;
 use std::sync::Arc;
 
+use config::ConfigError;
 use itertools::Itertools;
 use jujutsu_lib::backend::TreeId;
 use jujutsu_lib::gitignore::GitIgnoreFile;
@@ -34,6 +35,8 @@ use crate::ui::Ui;
 
 #[derive(Debug, Error)]
 pub enum DiffEditError {
+    #[error("Invalid config: {0}")]
+    ConfigError(#[from] ConfigError),
     #[error("The diff tool exited with a non-zero code")]
     DifftoolAborted,
     #[error("Failed to write directories to diff: {0:?}")]
@@ -138,7 +141,7 @@ pub fn edit_diff(
 
     // TODO: Make this configuration have a table of possible editors and detect the
     // best one here.
-    let editor_binary = match settings.config().get_string("ui.diff-editor") {
+    let editor_name = match settings.config().get_string("ui.diff-editor") {
         Ok(editor_binary) => editor_binary,
         Err(_) => {
             let default_editor = "meld".to_string();
@@ -150,14 +153,15 @@ pub fn edit_diff(
             default_editor
         }
     };
-
+    let editor = get_tool(settings, &editor_name)?;
     // Start a diff editor on the two directories.
-    let exit_status = Command::new(&editor_binary)
+    let exit_status = Command::new(&editor.program)
+        .args(&editor.edit_args)
         .arg(&left_wc_dir)
         .arg(&right_wc_dir)
         .status()
         .map_err(|e| DiffEditError::ExecuteEditorError {
-            editor_binary,
+            editor_binary: editor.program,
             source: e,
         })?;
     if !exit_status.success() {
@@ -169,3 +173,42 @@ pub fn edit_diff(
 
     Ok(right_tree_state.snapshot(base_ignores)?)
 }
+
+/// Merge/diff tool loaded from the settings.
+#[derive(Clone, Debug, serde::Deserialize)]
+#[serde(rename_all = "kebab-case")]
+struct MergeTool {
+    /// Program to execute.
+    pub program: String,
+    /// Arguments to pass to the program when editing diffs.
+    #[serde(default)]
+    pub edit_args: Vec<String>,
+}
+
+impl MergeTool {
+    pub fn with_program(program: &str) -> Self {
+        MergeTool {
+            program: program.to_owned(),
+            edit_args: vec![],
+        }
+    }
+}
+
+/// Loads merge tool options from `[merge-tools.<name>]`. The given name is used
+/// as an executable name if no configuration found for that name.
+fn get_tool(settings: &UserSettings, name: &str) -> Result<MergeTool, ConfigError> {
+    const TABLE_KEY: &'static str = "merge-tools";
+    let tools_table = match settings.config().get_table(TABLE_KEY) {
+        Ok(table) => table,
+        Err(ConfigError::NotFound(_)) => return Ok(MergeTool::with_program(name)),
+        Err(err) => return Err(err),
+    };
+    if let Some(v) = tools_table.get(name) {
+        v.clone()
+            .try_deserialize()
+            // add config key, deserialize error is otherwise unclear
+            .map_err(|e| ConfigError::Message(format!("{TABLE_KEY}.{name}: {e}")))
+    } else {
+        Ok(MergeTool::with_program(name))
+    }
+}