From 0db91da478c21df772bd57c443a8226a248c6dc1 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 | 3 + cli/src/config-schema.json | 8 ++ cli/src/diff_util.rs | 130 +++++++++++++++++++++++++---- cli/src/merge_tools/external.rs | 14 +++- cli/src/merge_tools/mod.rs | 3 +- cli/testing/fake-diff-editor.rs | 22 ++--- cli/tests/cli-reference@.md.snap | 50 ++++++++++++ cli/tests/test_diff_command.rs | 135 +++++++++++++++++++++++++++++++ docs/config.md | 11 +++ 9 files changed, 347 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 24c05d90c5..39f0b3786d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * New command `jj git remote set-url` that sets the url of a git remote. +* 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 diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 5fb08e129f..802cc4e322 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -112,6 +112,14 @@ "tool": { "type": "string", "description": "External tool for generating diffs" + }, + "tool-mode": { + "description": "Invoke the external tool with directories or individual files", + "enum": [ + "dir", + "file-by-file" + ], + "default": "dir" } } }, diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 7ea57d2f44..78b1a1b51e 100644 --- a/cli/src/diff_util.rs +++ b/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, + ExternalMergeTool, +}; use crate::text_util; use crate::ui::Ui; @@ -81,20 +87,38 @@ pub struct DiffFormatArgs { /// Generate diff by external command #[arg(long)] pub tool: Option, + #[arg(long, requires("tool"), default_value("dir"))] + pub tool_mode: DiffToolMode, /// Number of lines of context to show #[arg(long)] context: Option, } +#[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, +} + #[derive(Clone, Debug, Eq, PartialEq)] pub enum DiffFormat { Summary, Stat, Types, NameOnly, - Git { context: usize }, - ColorWords { context: usize }, - Tool(Box), + Git { + context: usize, + }, + ColorWords { + context: usize, + }, + Tool { + tool: Box, + mode: DiffToolMode, + }, } /// Returns a list of requested diff formats, which will never be empty. @@ -154,7 +178,10 @@ fn diff_formats_from_args( if let Some(name) = &args.tool { let tool = merge_tools::get_external_tool_config(settings, name)? .unwrap_or_else(|| ExternalMergeTool::with_program(name)); - formats.push(DiffFormat::Tool(Box::new(tool))); + formats.push(DiffFormat::Tool { + tool: Box::new(tool), + mode: args.tool_mode, + }); } Ok(formats) } @@ -172,7 +199,14 @@ fn default_diff_format( None } .unwrap_or_else(|| ExternalMergeTool::with_diff_args(&args)); - return Ok(DiffFormat::Tool(Box::new(tool))); + let mode = config + .get("ui.diff.tool-mode") + .optional()? + .unwrap_or(DiffToolMode::Dir); + return Ok(DiffFormat::Tool { + tool: Box::new(tool), + mode, + }); } let name = if let Some(name) = config.get_string("ui.diff.format").optional()? { name @@ -272,16 +306,19 @@ impl<'a> DiffRenderer<'a> { let tree_diff = from_tree.diff_stream(to_tree, matcher); 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)?; + DiffFormat::Tool { + 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)?; + } + DiffFormat::Tool { + tool, + mode: DiffToolMode::Dir, + } => { + generate_diff(ui, formatter.raw(), from_tree, to_tree, matcher, tool) + .map_err(DiffRenderError::DiffGenerate)?; } } } @@ -644,6 +681,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 { + 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, diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 1425a66d5d..a6b981c717 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -257,7 +257,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 +272,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:"); diff --git a/cli/src/merge_tools/mod.rs b/cli/src/merge_tools/mod.rs index 091989e95e..2635776cc0 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, ExternalMergeTool}; use crate::config::CommandNameAndArgs; use crate::ui::Ui; 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/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index dd045bbd26..4edcd6b385 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -621,6 +621,16 @@ With the `--from` and/or `--to` options, shows the difference from/to the given * `--git` — Show a Git-format diff * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command +* `--tool-mode ` + + Default value: `dir` + + Possible values: + - `dir`: + Invoke the diff tool on a temp directory of the modified files + - `file-by-file`: + Invoke the diff tool on each of the modified files individually + * `--context ` — Number of lines of context to show @@ -1062,6 +1072,16 @@ This excludes changes from other commits by temporarily rebasing `--from` onto ` * `--git` — Show a Git-format diff * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command +* `--tool-mode ` + + Default value: `dir` + + Possible values: + - `dir`: + Invoke the diff tool on a temp directory of the modified files + - `file-by-file`: + Invoke the diff tool on each of the modified files individually + * `--context ` — Number of lines of context to show @@ -1103,6 +1123,16 @@ Spans of revisions that are not included in the graph per `--revisions` are rend * `--git` — Show a Git-format diff * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command +* `--tool-mode ` + + Default value: `dir` + + Possible values: + - `dir`: + Invoke the diff tool on a temp directory of the modified files + - `file-by-file`: + Invoke the diff tool on each of the modified files individually + * `--context ` — Number of lines of context to show @@ -1216,6 +1246,16 @@ Name is derived from Merciual's obsolescence markers. * `--git` — Show a Git-format diff * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command +* `--tool-mode ` + + Default value: `dir` + + Possible values: + - `dir`: + Invoke the diff tool on a temp directory of the modified files + - `file-by-file`: + Invoke the diff tool on each of the modified files individually + * `--context ` — Number of lines of context to show @@ -1627,6 +1667,16 @@ Show commit description and changes in a revision * `--git` — Show a Git-format diff * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command +* `--tool-mode ` + + Default value: `dir` + + Possible values: + - `dir`: + Invoke the diff tool on a temp directory of the modified files + - `file-by-file`: + Invoke the diff tool on each of the modified files individually + * `--context ` — Number of lines of context to show diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 1ad6b6648b..409a208932 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -936,6 +936,141 @@ 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(); + + // diff without file patterns + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--tool=fake-diff-editor", "--tool-mode=file-by-file"]), @r###" + == + file1 + -- + file1 + == + file2 + -- + file2 + == + file3 + -- + file3 + "###); + + // diff with file patterns + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--tool=fake-diff-editor", "--tool-mode=file-by-file", "file1"]), @r###" + == + file1 + -- + file1 + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["log", "-p", "--tool=fake-diff-editor", "--tool-mode=file-by-file"]), @r###" + @ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 0cba70c7 + │ (no description set) + │ == + │ file1 + │ -- + │ file1 + │ == + │ file2 + │ -- + │ file2 + │ == + │ file3 + │ -- + │ file3 + ◉ qpvuntsm test.user@example.com 2001-02-03 08:05:08 39b5a56f + │ (no description set) + │ == + │ file1 + │ -- + │ file1 + │ == + │ file2 + │ -- + │ file2 + ◉ zzzzzzzz root() 00000000 + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["show", "--tool=fake-diff-editor", "--tool-mode=file-by-file"]), @r###" + Commit ID: 0cba70c72186eabb5a2f91be63a8366b9f6da6c6 + Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp + Author: Test User (2001-02-03 08:05:08) + Committer: Test User (2001-02-03 08:05:09) + + (no description set) + + == + file1 + -- + file1 + == + file2 + -- + file2 + == + file3 + -- + file3 + "###); + + // Enabled by default, looks up the merge-tools table + let config = "--config-toml=ui.diff.tool='fake-diff-editor'"; + let config_mode = "--config-toml=ui.diff.tool-mode='file-by-file'"; + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", config, config_mode]), @r###" + == + file1 + -- + file1 + == + file2 + -- + file2 + == + file3 + -- + file3 + "###); + + // Inlined command arguments + let command = escaped_fake_diff_editor_path(); + let config = format!(r#"--config-toml=ui.diff.tool=["{command}", "$right", "$left"]"#); + insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", &config, config_mode]), @r###" + == + 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 d75a5de949..714916eb99 100644 --- a/docs/config.md +++ b/docs/config.md @@ -198,6 +198,17 @@ 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 the tool with a directory for the changes. The +`--tool-mode` parameter can change this to file by file invocations. This can +also be set in the configured as follows: + +```toml +[ui] +# Use pratdiff in file-by-file mode by default. +diff.tool = ["pratdiff", "--color=always", "$left", "$right"] +diff.tool-mode = "file-by-file" +``` + ### Set of immutable commits You can configure the set of immutable commits via `revset-aliases."immutable_heads()"`.