From 2dace7c4985d18f0352f1f0766cf11d5f59fc11c Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Thu, 27 Jun 2024 11:53:14 -0400 Subject: [PATCH] diff: add a file-by-file variant for external diff tools --- CHANGELOG.md | 4 ++ cli/src/config-schema.json | 8 +++ cli/src/diff_util.rs | 92 ++++++++++++++++++++++++---- cli/src/merge_tools/external.rs | 29 +++++++-- cli/src/merge_tools/mod.rs | 15 ++++- cli/testing/fake-diff-editor.rs | 22 ++++--- cli/tests/test_diff_command.rs | 103 ++++++++++++++++++++++++++++++++ docs/config.md | 12 ++++ 8 files changed, 261 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 922d0aa2aa..a7bf662d7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -84,6 +84,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 diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 5fb08e129f..5fbfdba40d 100644 --- a/cli/src/config-schema.json +++ b/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": { diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 94a39baed4..7dc54c0318 100644 --- a/cli/src/diff_util.rs +++ b/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( - ui, - formatter.raw(), - from_tree, - to_tree, - matcher, - tool, - ) - .map_err(DiffRenderError::DiffGenerate)?; + match tool.diff_invocation_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 +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 { + 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, diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 1425a66d5d..3025f376db 100644 --- a/cli/src/merge_tools/external.rs +++ b/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, + /// 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, @@ -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()) diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 091989e95e..b23832fc63 100644 --- a/cli/src/merge_tools/mod.rs +++ b/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", diff --git a/cli/testing/fake-diff-editor.rs b/cli/testing/fake-diff-editor.rs index c76e17f56a..da7a5c79f8 100644 --- a/cli/testing/fake-diff-editor.rs +++ b/cli/testing/fake-diff-editor.rs @@ -34,17 +34,21 @@ struct Args { _ignore: Vec, } -fn files_recursively(dir: &Path) -> HashSet { +fn files_recursively(p: &Path) -> HashSet { 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 diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index da80381f88..216abc395d 100644 --- a/cli/tests/test_diff_command.rs +++ b/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 (2001-02-03 08:05:09) + Committer: Test User (2001-02-03 08:05:09) + + (no description set) + + == + file1 + -- + file1 + == + file2 + -- + file2 + == + file3 + -- + file3 + "###); +} + #[cfg(unix)] #[test] fn test_diff_external_tool_symlink() { diff --git a/docs/config.md b/docs/config.md index 72411f0d0e..ea57d13e0d 100644 --- a/docs/config.md +++ b/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()"`.