cli: expand $left/$right parameters in merge-tools.<name>.edit-args

And set edit_args = ["$left", "$right"] by default.

CHANGELOG.md

@@ -44,6 +44,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * The `diff.format` config option is now called `ui.diff.format`. The old name
   is still supported for now.
 
+* `merge-tools.<name>.edit-args` now requires `$left`/`$right` parameters.
+  The default is `edit-args = ["$left", "$right"]`.
+
 ### New features
 
 * The default log format now uses the committer timestamp instead of the author

docs/config.md

@@ -199,21 +199,31 @@ Obviously, you would only set one line, don't copy them all in!
 ## Editing diffs
 
 The `ui.diff-editor` setting affects the tool used for editing diffs (e.g.
-`jj split`, `jj amend -i`). The default is `meld`. The left and right
-directories to diff are passed as the first and second argument respectively.
+`jj split`, `jj amend -i`). The default is `meld`.
+
+`jj` replaces the following arguments:
+
+- `$left` and `$right` are replaced with the paths to the left and right
+  directories to diff respectively.
+
+If no arguments are specified, `["$left", "$right"]` are set by default.
 
 For example:
 
 ```toml
-ui.diff-editor = "kdiff3" # Use merge-tools.kdiff3.edit-args
-ui.diff-editor = ["kdiff3", "--merge"] # Specify edit-args inline
+# Use merge-tools.kdiff3.edit-args
+ui.diff-editor = "kdiff3"
+# Specify edit-args inline
+ui.diff-editor = ["kdiff3", "--merge", "$left", "$right"]
 ```
 
-Custom arguments can be added, and will be inserted before the paths to diff:
+If `ui.diff-editor` consists of a single word, e.g. `"kdiff3"`, the arguments
+will be read from the following config keys.
 
 ```toml
 # merge-tools.kdiff3.program = "kdiff3"      # Defaults to the name of the tool if not specified
-merge-tools.kdiff3.edit-args = ["--merge", "--cs", "CreateBakFiles=0"]
+merge-tools.kdiff3.edit-args = [
+    "--merge", "--cs", "CreateBakFiles=0", "$left", "$right"]
 ```
 
 ### Using Vim as a diff editor

src/config/merge_tools.toml

@@ -1,9 +1,10 @@
 [merge-tools.kdiff3]
 # --merge to open output pane, CreateBakFiles=0 to not include backup files in commit
-edit-args = ["--merge", "--cs", "CreateBakFiles=0"]
+edit-args = ["--merge", "--cs", "CreateBakFiles=0", "$left", "$right"]
 merge-args = ["$base", "$left", "$right", "-o", "$output", "--auto"]
 
 [merge-tools.meld]
+edit-args = ["$left", "$right"]
 merge-args = ["$left", "$base", "$right", "-o", "$output", "--auto-merge"]
 
 [merge-tools.vimdiff]
@@ -16,4 +17,4 @@ merge-tool-edits-conflict-markers = true
 # Using vimdiff as a diff editor is not recommended. For instructions on configuring
 # the DirDiff Vim plugin for a better experience, see
 # https://gist.github.com/ilyagr/5d6339fb7dac5e7ab06fe1561ec62d45
-edit-args = ["-f", "-d"]
+edit-args = ["-f", "-d", "$left", "$right"]

src/merge_tools.rs

@@ -351,12 +351,14 @@ pub fn edit_diff(
             .map_err(ExternalToolError::SetUpDirError)?;
     }
 
-    let editor = get_diff_editor_from_settings(ui, settings)?;
     // Start a diff editor on the two directories.
+    let editor = get_diff_editor_from_settings(ui, settings)?;
+    let patterns = maplit::hashmap! {
+        "left" => left_wc_dir.to_str().expect("temp_dir would be valid utf-8"),
+        "right" => right_wc_dir.to_str().expect("temp_dir would be valid utf-8"),
+    };
     let mut cmd = Command::new(&editor.program);
-    cmd.args(&editor.edit_args)
-        .arg(&left_wc_dir)
-        .arg(&right_wc_dir);
+    cmd.args(interpolate_variables(&editor.edit_args, &patterns));
     tracing::debug!(?cmd, "Invoking the external diff editor:");
     let exit_status = cmd
         .status()
@@ -385,6 +387,7 @@ struct MergeTool {
     /// if not specified in the config.
     pub program: String,
     /// Arguments to pass to the program when editing diffs.
+    /// `$left` and `$right` are replaced with the corresponding directories.
     pub edit_args: Vec<String>,
     /// Arguments to pass to the program when resolving 3-way conflicts.
     /// `$left`, `$right`, `$base`, and `$output` are replaced with
@@ -404,12 +407,11 @@ struct MergeTool {
     pub merge_tool_edits_conflict_markers: bool,
 }
 
-#[allow(clippy::derivable_impls)] // TODO
 impl Default for MergeTool {
     fn default() -> Self {
         MergeTool {
             program: String::new(),
-            edit_args: vec![],
+            edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(),
             merge_args: vec![],
             merge_tool_edits_conflict_markers: false,
         }
@@ -541,7 +543,10 @@ mod tests {
         insta::assert_debug_snapshot!(get("").unwrap(), @r###"
         MergeTool {
             program: "meld",
-            edit_args: [],
+            edit_args: [
+                "$left",
+                "$right",
+            ],
             merge_args: [
                 "$left",
                 "$base",
@@ -554,36 +559,44 @@ mod tests {
         }
         "###);
 
-        // Just program name
+        // Just program name, edit_args are filled by default
         insta::assert_debug_snapshot!(get(r#"ui.diff-editor = "my-diff""#).unwrap(), @r###"
-        MergeTool {
-            program: "my-diff",
-            edit_args: [],
-            merge_args: [],
-            merge_tool_edits_conflict_markers: false,
-        }
-        "###);
-
-        // String args
-        insta::assert_debug_snapshot!(
-            get(r#"ui.diff-editor = "my-diff --diff""#).unwrap(), @r###"
         MergeTool {
             program: "my-diff",
             edit_args: [
-                "--diff",
+                "$left",
+                "$right",
             ],
             merge_args: [],
             merge_tool_edits_conflict_markers: false,
         }
         "###);
 
-        // List args
+        // String args (with interpolation variables)
         insta::assert_debug_snapshot!(
-            get(r#"ui.diff-editor = ["my-diff", "--diff"]"#).unwrap(), @r###"
+            get(r#"ui.diff-editor = "my-diff -l $left -r $right""#).unwrap(), @r###"
+        MergeTool {
+            program: "my-diff",
+            edit_args: [
+                "-l",
+                "$left",
+                "-r",
+                "$right",
+            ],
+            merge_args: [],
+            merge_tool_edits_conflict_markers: false,
+        }
+        "###);
+
+        // List args (with interpolation variables)
+        insta::assert_debug_snapshot!(
+            get(r#"ui.diff-editor = ["my-diff", "--diff", "$left", "$right"]"#).unwrap(), @r###"
         MergeTool {
             program: "my-diff",
             edit_args: [
                 "--diff",
+                "$left",
+                "$right",
             ],
             merge_args: [],
             merge_tool_edits_conflict_markers: false,
@@ -595,7 +608,7 @@ mod tests {
         r#"
         ui.diff-editor = "foo bar"
         [merge-tools."foo bar"]
-        edit-args = ["--edit", "args"]
+        edit-args = ["--edit", "args", "$left", "$right"]
         "#,
         ).unwrap(), @r###"
         MergeTool {
@@ -603,17 +616,41 @@ mod tests {
             edit_args: [
                 "--edit",
                 "args",
+                "$left",
+                "$right",
             ],
             merge_args: [],
             merge_tool_edits_conflict_markers: false,
         }
         "###);
 
-        // List args should never be a merge-tools key
+        // Pick from merge-tools, but no edit-args specified
+        insta::assert_debug_snapshot!(get(
+        r#"
+        ui.diff-editor = "my-diff"
+        [merge-tools.my-diff]
+        program = "MyDiff"
+        "#,
+        ).unwrap(), @r###"
+        MergeTool {
+            program: "MyDiff",
+            edit_args: [
+                "$left",
+                "$right",
+            ],
+            merge_args: [],
+            merge_tool_edits_conflict_markers: false,
+        }
+        "###);
+
+        // List args should never be a merge-tools key, edit_args are filled by default
         insta::assert_debug_snapshot!(get(r#"ui.diff-editor = ["meld"]"#).unwrap(), @r###"
         MergeTool {
             program: "meld",
-            edit_args: [],
+            edit_args: [
+                "$left",
+                "$right",
+            ],
             merge_args: [],
             merge_tool_edits_conflict_markers: false,
         }
@@ -636,7 +673,10 @@ mod tests {
         insta::assert_debug_snapshot!(get("").unwrap(), @r###"
         MergeTool {
             program: "meld",
-            edit_args: [],
+            edit_args: [
+                "$left",
+                "$right",
+            ],
             merge_args: [
                 "$left",
                 "$base",
@@ -661,7 +701,10 @@ mod tests {
             get(r#"ui.merge-editor = "my-merge $left $base $right $output""#).unwrap(), @r###"
         MergeTool {
             program: "my-merge",
-            edit_args: [],
+            edit_args: [
+                "$left",
+                "$right",
+            ],
             merge_args: [
                 "$left",
                 "$base",
@@ -679,7 +722,10 @@ mod tests {
             ).unwrap(), @r###"
         MergeTool {
             program: "my-merge",
-            edit_args: [],
+            edit_args: [
+                "$left",
+                "$right",
+            ],
             merge_args: [
                 "$left",
                 "$base",
@@ -700,7 +746,10 @@ mod tests {
         ).unwrap(), @r###"
         MergeTool {
             program: "foo bar",
-            edit_args: [],
+            edit_args: [
+                "$left",
+                "$right",
+            ],
             merge_args: [
                 "$base",
                 "$left",