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

CHANGELOG.md

@@ -76,6 +76,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   commits with no description) if authored by the current user.
   [#2000](https://github.com/martinvonz/jj/issues/2000)
 
+* External diff tools now support `--tool-mode=file-by-file` which invokes the
+  tool on individual files instead of a directory.
+
 ### Fixed bugs
 
 * `jj git push` now ignores immutable commits when checking whether a

cli/src/config-schema.json

@@ -348,6 +348,14 @@
                         "type": "boolean",
                         "description": "Whether to populate the output file with conflict markers before starting the merge tool. See https://github.com/martinvonz/jj/blob/main/docs/config.md#editing-conflict-markers-with-a-tool-or-a-text-editor",
                         "default": false
+                    },
+                    "mode": {
+                      "description": "Invoke the tool with directories or individual files",
+                      "enum": [
+                        "dir",
+                        "file-by-file"
+                      ],
+                      "default": "dir"
                     }
                 }
             }

cli/src/diff_util.rs

@@ -14,8 +14,11 @@
 
 use std::cmp::max;
 use std::collections::VecDeque;
+use std::fs::File;
 use std::io;
+use std::io::Write;
 use std::ops::Range;
+use std::path::{Path, PathBuf};
 
 use futures::{try_join, Stream, StreamExt};
 use itertools::Itertools;
@@ -40,7 +43,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;
 
@@ -81,6 +87,8 @@ pub struct DiffFormatArgs {
     /// Generate diff by external command
     #[arg(long)]
     pub tool: Option<String>,
+    #[arg(long, requires("tool"))]
+    pub tool_mode: Option<DiffToolMode>,
     /// Number of lines of context to show
     #[arg(long)]
     context: Option<usize>,
@@ -152,8 +160,11 @@ fn diff_formats_from_args(
     .filter_map(|(arg, format)| arg.then_some(format))
     .collect_vec();
     if let Some(name) = &args.tool {
-        let tool = merge_tools::get_external_tool_config(settings, name)?
+        let mut tool = merge_tools::get_external_tool_config(settings, name)?
             .unwrap_or_else(|| ExternalMergeTool::with_program(name));
+        if let Some(mode) = args.tool_mode {
+            tool.mode = mode;
+        }
         formats.push(DiffFormat::Tool(Box::new(tool)));
     }
     Ok(formats)
@@ -273,15 +284,23 @@ impl<'a> DiffRenderer<'a> {
                     show_color_words_diff(repo, formatter, *context, tree_diff, path_converter)?;
                 }
                 DiffFormat::Tool(tool) => {
-                    merge_tools::generate_diff(
-                        ui,
-                        formatter.raw(),
-                        from_tree,
-                        to_tree,
-                        matcher,
-                        tool,
-                    )
-                    .map_err(DiffRenderError::DiffGenerate)?;
+                    match tool.mode {
+                        DiffToolMode::FileByFile => {
+                            let tree_diff = from_tree.diff_stream(to_tree, matcher);
+                            show_file_by_file_diff(
+                                ui,
+                                repo,
+                                formatter,
+                                tool,
+                                tree_diff,
+                                path_converter,
+                            )
+                        }
+                        DiffToolMode::Dir => {
+                            generate_diff(ui, formatter.raw(), from_tree, to_tree, matcher, tool)
+                                .map_err(DiffRenderError::DiffGenerate)
+                        }
+                    }?;
                 }
             }
         }
@@ -648,6 +667,67 @@ 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)?;
+        let mut buffer = File::create(&fs_path)?;
+        buffer.write_all(&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

@@ -48,6 +48,18 @@ pub struct ExternalMergeTool {
     // TODO: Instead of a boolean, this could denote the flavor of conflict markers to put in
     // the file (`jj` or `diff3` for example).
     pub merge_tool_edits_conflict_markers: bool,
+    /// Whether to execute the tool with a pair of directories or individual
+    /// files.
+    pub mode: DiffToolMode,
+}
+
+#[derive(clap::ValueEnum, 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 {
@@ -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,
+            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,9 +285,17 @@ 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));
     tracing::info!(?cmd, "Invoking the external diff generator:");

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;
 
@@ -357,6 +358,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -383,6 +385,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -416,6 +419,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -438,6 +442,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -459,6 +464,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -486,6 +492,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -511,6 +518,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -530,6 +538,7 @@ mod tests {
                 ],
                 merge_args: [],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -580,6 +589,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -625,6 +635,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -652,6 +663,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);
@@ -682,6 +694,7 @@ mod tests {
                     "$output",
                 ],
                 merge_tool_edits_conflict_markers: false,
+                mode: Dir,
             },
         )
         "###);

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() {
-        let dir_entry = dir_entry.unwrap();
-        let base_name = dir_entry.file_name().to_str().unwrap().to_string();
-        if dir_entry.path().is_dir() {
-            for sub_path in files_recursively(&dir_entry.path()) {
-                files.insert(format!("{base_name}/{sub_path}"));
+    if !p.is_dir() {
+        files.insert(p.file_name().unwrap().to_str().unwrap().to_string());
+    } else {
+        for dir_entry in std::fs::read_dir(p).unwrap() {
+            let dir_entry = dir_entry.unwrap();
+            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