diff: add a file-by-file variant for external diff tools

CHANGELOG.md

@@ -98,6 +98,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 * `jj squash` now accepts a `--keep-emptied` option to keep the source commit.
 
+* External diff tools can now be configured to invoke the tool on each file
+  individually instead of being passed a directory by setting
+  `merge-tools.$TOOL.diff-invocation-mode="file-by-file"` in config.toml.
+
 ### Fixed bugs
 
 * `jj git push` now ignores immutable commits when checking whether a

cli/src/config-schema.json

@@ -332,6 +332,14 @@
                             "type": "string"
                         }
                     },
+                    "diff-invocation-mode": {
+                      "description": "Invoke the tool with directories or individual files",
+                      "enum": [
+                        "dir",
+                        "file-by-file"
+                      ],
+                      "default": "dir"
+                    },
                     "edit-args": {
                         "type": "array",
                         "items": {

cli/src/diff_util.rs

@@ -16,6 +16,7 @@ use std::cmp::max;
 use std::collections::VecDeque;
 use std::io;
 use std::ops::Range;
+use std::path::{Path, PathBuf};
 
 use futures::{try_join, Stream, StreamExt};
 use itertools::Itertools;
@@ -40,7 +41,10 @@ use unicode_width::UnicodeWidthStr as _;
 
 use crate::config::CommandNameAndArgs;
 use crate::formatter::Formatter;
-use crate::merge_tools::{self, DiffGenerateError, ExternalMergeTool};
+use crate::merge_tools::{
+    self, generate_diff, invoke_external_diff, new_utf8_temp_dir, DiffGenerateError, DiffToolMode,
+    ExternalMergeTool,
+};
 use crate::text_util;
 use crate::ui::Ui;
 
@@ -273,15 +277,23 @@ impl<'a> DiffRenderer<'a> {
                     show_color_words_diff(repo, formatter, *context, tree_diff, path_converter)?;
                 }
                 DiffFormat::Tool(tool) => {
-                    merge_tools::generate_diff(
+                    match tool.diff_invocation_mode {
-                        ui,
+                        DiffToolMode::FileByFile => {
-                        formatter.raw(),
+                            let tree_diff = from_tree.diff_stream(to_tree, matcher);
-                        from_tree,
+                            show_file_by_file_diff(
-                        to_tree,
+                                ui,
-                        matcher,
+                                repo,
-                        tool,
+                                formatter,
-                    )
+                                tool,
-                    .map_err(DiffRenderError::DiffGenerate)?;
+                                tree_diff,
+                                path_converter,
+                            )
+                        }
+                        DiffToolMode::Dir => {
+                            generate_diff(ui, formatter.raw(), from_tree, to_tree, matcher, tool)
+                                .map_err(DiffRenderError::DiffGenerate)
+                        }
+                    }?;
                 }
             }
         }
@@ -648,6 +660,66 @@ pub fn show_color_words_diff(
     Ok(())
 }
 
+pub fn show_file_by_file_diff(
+    ui: &Ui,
+    repo: &dyn Repo,
+    formatter: &mut dyn Formatter,
+    tool: &ExternalMergeTool,
+    tree_diff: TreeDiffStream,
+    path_converter: &RepoPathUiConverter,
+) -> Result<(), DiffRenderError> {
+    fn create_file(
+        path: &RepoPath,
+        wc_dir: &Path,
+        value: MaterializedTreeValue,
+    ) -> Result<PathBuf, DiffRenderError> {
+        let fs_path = path.to_fs_path(wc_dir);
+        std::fs::create_dir_all(fs_path.parent().unwrap())?;
+        let content = diff_content(path, value)?;
+        std::fs::write(&fs_path, content.contents)?;
+        Ok(fs_path)
+    }
+
+    let temp_dir = new_utf8_temp_dir("jj-diff-")?;
+    let left_wc_dir = temp_dir.path().join("left");
+    let right_wc_dir = temp_dir.path().join("right");
+    let mut diff_stream = materialized_diff_stream(repo.store(), tree_diff);
+    async {
+        while let Some((path, diff)) = diff_stream.next().await {
+            let ui_path = path_converter.format_file_path(&path);
+            let (left_value, right_value) = diff?;
+
+            match (&left_value, &right_value) {
+                (_, MaterializedTreeValue::AccessDenied(source))
+                | (MaterializedTreeValue::AccessDenied(source), _) => {
+                    write!(
+                        formatter.labeled("access-denied"),
+                        "Access denied to {ui_path}:"
+                    )?;
+                    writeln!(formatter, " {source}")?;
+                    continue;
+                }
+                _ => {}
+            }
+            let left_path = create_file(&path, &left_wc_dir, left_value)?;
+            let right_path = create_file(&path, &right_wc_dir, right_value)?;
+
+            invoke_external_diff(
+                ui,
+                formatter.raw(),
+                tool,
+                &maplit::hashmap! {
+                    "left" => left_path.to_str().expect("temp_dir should be valid utf-8"),
+                    "right" => right_path.to_str().expect("temp_dir should be valid utf-8"),
+                },
+            )
+            .map_err(DiffRenderError::DiffGenerate)?;
+        }
+        Ok::<(), DiffRenderError>(())
+    }
+    .block_on()
+}
+
 struct GitDiffPart {
     mode: String,
     hash: String,

cli/src/merge_tools/external.rs

@@ -31,6 +31,9 @@ pub struct ExternalMergeTool {
     /// Arguments to pass to the program when generating diffs.
     /// `$left` and `$right` are replaced with the corresponding directories.
     pub diff_args: Vec<String>,
+    /// Whether to execute the tool with a pair of directories or individual
+    /// files.
+    pub diff_invocation_mode: DiffToolMode,
     /// Arguments to pass to the program when editing diffs.
     /// `$left` and `$right` are replaced with the corresponding directories.
     pub edit_args: Vec<String>,
@@ -50,6 +53,15 @@ pub struct ExternalMergeTool {
     pub merge_tool_edits_conflict_markers: bool,
 }
 
+#[derive(serde::Deserialize, Copy, Clone, Debug, Eq, PartialEq)]
+#[serde(rename_all = "kebab-case")]
+pub enum DiffToolMode {
+    /// Invoke the diff tool on a temp directory of the modified files.
+    Dir,
+    /// Invoke the diff tool on each of the modified files individually.
+    FileByFile,
+}
+
 impl Default for ExternalMergeTool {
     fn default() -> Self {
         Self {
@@ -63,6 +75,7 @@ impl Default for ExternalMergeTool {
             edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(),
             merge_args: vec![],
             merge_tool_edits_conflict_markers: false,
+            diff_invocation_mode: DiffToolMode::Dir,
         }
     }
 }
@@ -257,7 +270,7 @@ pub fn edit_diff_external(
     diffedit_wc.snapshot_results(base_ignores)
 }
 
-/// Generates textual diff by the specified `tool`, and writes into `writer`.
+/// Generates textual diff by the specified `tool` and writes into `writer`.
 pub fn generate_diff(
     ui: &Ui,
     writer: &mut dyn Write,
@@ -272,11 +285,19 @@ pub fn generate_diff(
         .map_err(ExternalToolError::SetUpDir)?;
     set_readonly_recursively(diff_wc.right_working_copy_path())
         .map_err(ExternalToolError::SetUpDir)?;
-    // TODO: Add support for tools without directory diff functionality?
+    invoke_external_diff(ui, writer, tool, &diff_wc.to_command_variables())
+}
+
+/// Invokes the specified `tool` directing its output into `writer`.
+pub fn invoke_external_diff(
+    ui: &Ui,
+    writer: &mut dyn Write,
+    tool: &ExternalMergeTool,
+    patterns: &HashMap<&str, &str>,
+) -> Result<(), DiffGenerateError> {
     // TODO: Somehow propagate --color to the external command?
-    let patterns = diff_wc.to_command_variables();
     let mut cmd = Command::new(&tool.program);
-    cmd.args(interpolate_variables(&tool.diff_args, &patterns));
+    cmd.args(interpolate_variables(&tool.diff_args, patterns));
     tracing::info!(?cmd, "Invoking the external diff generator:");
     let mut child = cmd
         .stdin(Stdio::null())

cli/src/merge_tools/mod.rs

@@ -31,9 +31,10 @@ use pollster::FutureExt;
 use thiserror::Error;
 
 use self::builtin::{edit_diff_builtin, edit_merge_builtin, BuiltinToolError};
+pub(crate) use self::diff_working_copies::new_utf8_temp_dir;
 use self::diff_working_copies::DiffCheckoutError;
 use self::external::{edit_diff_external, ExternalToolError};
-pub use self::external::{generate_diff, ExternalMergeTool};
+pub use self::external::{generate_diff, invoke_external_diff, DiffToolMode, ExternalMergeTool};
 use crate::config::CommandNameAndArgs;
 use crate::ui::Ui;
 
@@ -351,6 +352,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -375,6 +377,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "--edit",
                     "args",
@@ -410,6 +413,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -430,6 +434,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "-l",
                     "$left",
@@ -452,6 +457,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "--diff",
                     "$left",
@@ -478,6 +484,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "--edit",
                     "args",
@@ -505,6 +512,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -524,6 +532,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -569,6 +578,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -614,6 +624,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -641,6 +652,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",
@@ -671,6 +683,7 @@ mod tests {
                     "$left",
                     "$right",
                 ],
+                diff_invocation_mode: Dir,
                 edit_args: [
                     "$left",
                     "$right",

cli/testing/fake-diff-editor.rs

@@ -34,17 +34,21 @@ struct Args {
     _ignore: Vec<String>,
 }
 
-fn files_recursively(dir: &Path) -> HashSet<String> {
+fn files_recursively(p: &Path) -> HashSet<String> {
     let mut files = HashSet::new();
-    for dir_entry in std::fs::read_dir(dir).unwrap() {
+    if !p.is_dir() {
-        let dir_entry = dir_entry.unwrap();
+        files.insert(p.file_name().unwrap().to_str().unwrap().to_string());
-        let base_name = dir_entry.file_name().to_str().unwrap().to_string();
+    } else {
-        if dir_entry.path().is_dir() {
+        for dir_entry in std::fs::read_dir(p).unwrap() {
-            for sub_path in files_recursively(&dir_entry.path()) {
+            let dir_entry = dir_entry.unwrap();
-                files.insert(format!("{base_name}/{sub_path}"));
+            let base_name = dir_entry.file_name().to_str().unwrap().to_string();
+            if !dir_entry.path().is_dir() {
+                files.insert(base_name);
+            } else {
+                for sub_path in files_recursively(&dir_entry.path()) {
+                    files.insert(format!("{base_name}/{sub_path}"));
+                }
             }
-        } else {
-            files.insert(base_name);
         }
     }
     files

cli/tests/test_diff_command.rs

@@ -1115,6 +1115,109 @@ fn test_diff_external_tool() {
     "###);
 }
 
+#[test]
+fn test_diff_external_file_by_file_tool() {
+    let mut test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    std::fs::write(repo_path.join("file1"), "foo\n").unwrap();
+    std::fs::write(repo_path.join("file2"), "foo\n").unwrap();
+    test_env.jj_cmd_ok(&repo_path, &["new"]);
+    std::fs::remove_file(repo_path.join("file1")).unwrap();
+    std::fs::write(repo_path.join("file2"), "foo\nbar\n").unwrap();
+    std::fs::write(repo_path.join("file3"), "foo\n").unwrap();
+
+    let edit_script = test_env.set_up_fake_diff_editor();
+    std::fs::write(
+        edit_script,
+        "print ==\0print-files-before\0print --\0print-files-after",
+    )
+    .unwrap();
+
+    // Enabled by default, looks up the merge-tools table
+    let config = "--config-toml=ui.diff.tool='fake-diff-editor'\nmerge-tools.fake-diff-editor.\
+                  diff-invocation-mode='file-by-file'";
+
+    // diff without file patterns
+    insta::assert_snapshot!(
+        test_env.jj_cmd_success(&repo_path, &["diff", config]), @r###"
+    ==
+    file1
+    --
+    file1
+    ==
+    file2
+    --
+    file2
+    ==
+    file3
+    --
+    file3
+    "###);
+
+    // diff with file patterns
+    insta::assert_snapshot!(
+        test_env.jj_cmd_success(&repo_path, &["diff", config, "file1"]), @r###"
+    ==
+    file1
+    --
+    file1
+    "###);
+
+    insta::assert_snapshot!(
+        test_env.jj_cmd_success(&repo_path, &["log", "-p", config]), @r###"
+    @  rlvkpnrz test.user@example.com 2001-02-03 08:05:09 39d9055d
+    │  (no description set)
+    │  ==
+    │  file1
+    │  --
+    │  file1
+    │  ==
+    │  file2
+    │  --
+    │  file2
+    │  ==
+    │  file3
+    │  --
+    │  file3
+    ◉  qpvuntsm test.user@example.com 2001-02-03 08:05:08 0ad4ef22
+    │  (no description set)
+    │  ==
+    │  file1
+    │  --
+    │  file1
+    │  ==
+    │  file2
+    │  --
+    │  file2
+    ◉  zzzzzzzz root() 00000000
+    "###);
+
+    insta::assert_snapshot!(
+        test_env.jj_cmd_success(&repo_path, &["show", config]), @r###"
+    Commit ID: 39d9055d70873099fd924b9af218289d5663eac8
+    Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp
+    Author: Test User <test.user@example.com> (2001-02-03 08:05:09)
+    Committer: Test User <test.user@example.com> (2001-02-03 08:05:09)
+
+        (no description set)
+
+    ==
+    file1
+    --
+    file1
+    ==
+    file2
+    --
+    file2
+    ==
+    file3
+    --
+    file3
+    "###);
+}
+
 #[cfg(unix)]
 #[test]
 fn test_diff_external_tool_symlink() {

docs/config.md

@@ -210,6 +210,18 @@ diff-args = ["--color=always", "$left", "$right"]
 - `$left` and `$right` are replaced with the paths to the left and right
   directories to diff respectively.
 
+By default `jj` will invoke external tools with a directory containing the left
+and right sides.  The `diff-invocation-mode` config can change this to file by file
+invocations as follows:
+
+```toml
+[ui]
+diff.tool = "vimdiff"
+
+[merge-tools.vimdiff]
+diff-invocation-mode = "file-by-file"
+```
+
 ### Set of immutable commits
 
 You can configure the set of immutable commits via `revset-aliases."immutable_heads()"`.