diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d3fa66c87..cd01fd92bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 from a merge revision's parents. This undoes the changes that `jj diff -r` would show. +* `jj diff`/`log` now supports `--tool ` option to generate diffs by + external program. For configuration, see [the documentation](docs/config.md). + [#1886](https://github.com/martinvonz/jj/issues/1886) ### Fixed bugs diff --git a/docs/config.md b/docs/config.md index 57294b2538..1ef3da7239 100644 --- a/docs/config.md +++ b/docs/config.md @@ -141,6 +141,21 @@ ui.default-command = "log" ui.diff.format = "git" ``` +### Generating diffs by external command + +If `diff --tool ` argument is given, the external diff command will be +called instead of the internal diff function. The command arguments can be +specified as follows. + +```toml +[merge-tools.] +# program = "" # Defaults to the name of the tool if not specified +diff-args = ["--color=always", "$left", "$right"] +``` + +- `$left` and `$right` are replaced with the paths to the left and right + directories to diff respectively. + ### Default revisions to log You can configure the revisions `jj log` without `-r` should show. diff --git a/src/cli_util.rs b/src/cli_util.rs index e128f4583f..dbe61cf8f6 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -72,7 +72,7 @@ use crate::config::{ new_config_path, AnnotatedValue, CommandNameAndArgs, ConfigSource, LayeredConfigs, }; use crate::formatter::{FormatRecorder, Formatter, PlainTextFormatter}; -use crate::merge_tools::{ConflictResolveError, DiffEditError}; +use crate::merge_tools::{ConflictResolveError, DiffEditError, DiffGenerateError}; use crate::template_parser::{TemplateAliasesMap, TemplateParseError}; use crate::templater::Template; use crate::ui::{ColorChoice, Ui}; @@ -239,6 +239,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: DiffGenerateError) -> Self { + user_error(format!("Failed to generate diff: {err}")) + } +} + impl From for CommandError { fn from(err: ConflictResolveError) -> Self { user_error(format!("Failed to use external tool to resolve: {err}")) diff --git a/src/config-schema.json b/src/config-schema.json index f568dd6f9c..a326b8a6cb 100644 --- a/src/config-schema.json +++ b/src/config-schema.json @@ -242,6 +242,12 @@ "program": { "type": "string" }, + "diff-args": { + "type": "array", + "items": { + "type": "string" + } + }, "edit-args": { "type": "array", "items": { diff --git a/src/diff_util.rs b/src/diff_util.rs index 81177f75cc..25a0424de4 100644 --- a/src/diff_util.rs +++ b/src/diff_util.rs @@ -32,10 +32,11 @@ use tracing::instrument; use crate::cli_util::{CommandError, WorkspaceCommandHelper}; use crate::formatter::Formatter; +use crate::merge_tools::{self, MergeTool}; #[derive(clap::Args, Clone, Debug)] #[command(group(clap::ArgGroup::new("short-format").args(&["summary", "types"])))] -#[command(group(clap::ArgGroup::new("long-format").args(&["git", "color_words"])))] +#[command(group(clap::ArgGroup::new("long-format").args(&["git", "color_words", "tool"])))] pub struct DiffFormatArgs { /// For each path, show only whether it was modified, added, or removed #[arg(long, short)] @@ -55,14 +56,18 @@ pub struct DiffFormatArgs { /// Show a word-level diff with changes indicated only by color #[arg(long)] pub color_words: bool, + /// Generate diff by external command + #[arg(long)] + pub tool: Option, } -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Debug, Eq, PartialEq)] pub enum DiffFormat { Summary, Types, Git, ColorWords, + Tool(Box), } /// Returns a list of requested diff formats, which will never be empty. @@ -70,7 +75,7 @@ pub fn diff_formats_for( settings: &UserSettings, args: &DiffFormatArgs, ) -> Result, config::ConfigError> { - let formats = diff_formats_from_args(args); + let formats = diff_formats_from_args(settings, args)?; if formats.is_empty() { Ok(vec![default_diff_format(settings)?]) } else { @@ -85,7 +90,7 @@ pub fn diff_formats_for_log( args: &DiffFormatArgs, patch: bool, ) -> Result, config::ConfigError> { - let mut formats = diff_formats_from_args(args); + let mut formats = diff_formats_from_args(settings, args)?; // --patch implies default if no format other than --summary is specified if patch && matches!(formats.as_slice(), [] | [DiffFormat::Summary]) { formats.push(default_diff_format(settings)?); @@ -94,8 +99,11 @@ pub fn diff_formats_for_log( Ok(formats) } -fn diff_formats_from_args(args: &DiffFormatArgs) -> Vec { - [ +fn diff_formats_from_args( + settings: &UserSettings, + args: &DiffFormatArgs, +) -> Result, config::ConfigError> { + let mut formats = [ (args.summary, DiffFormat::Summary), (args.types, DiffFormat::Types), (args.git, DiffFormat::Git), @@ -103,7 +111,13 @@ fn diff_formats_from_args(args: &DiffFormatArgs) -> Vec { ] .into_iter() .filter_map(|(arg, format)| arg.then_some(format)) - .collect() + .collect_vec(); + if let Some(name) = &args.tool { + let tool = merge_tools::get_tool_config(settings, name)? + .unwrap_or_else(|| MergeTool::with_program(name)); + formats.push(DiffFormat::Tool(Box::new(tool))); + } + Ok(formats) } fn default_diff_format(settings: &UserSettings) -> Result { @@ -120,6 +134,7 @@ fn default_diff_format(settings: &UserSettings) -> Result Ok(DiffFormat::Types), "git" => Ok(DiffFormat::Git), "color-words" => Ok(DiffFormat::ColorWords), + // TODO: add configurable default for DiffFormat::Tool? _ => Err(config::ConfigError::Message(format!( "invalid diff format: {name}" ))), @@ -135,20 +150,26 @@ pub fn show_diff( formats: &[DiffFormat], ) -> Result<(), CommandError> { for format in formats { - let tree_diff = from_tree.diff(to_tree, matcher); match format { DiffFormat::Summary => { + let tree_diff = from_tree.diff(to_tree, matcher); show_diff_summary(formatter, workspace_command, tree_diff)?; } DiffFormat::Types => { + let tree_diff = from_tree.diff(to_tree, matcher); show_types(formatter, workspace_command, tree_diff)?; } DiffFormat::Git => { + let tree_diff = from_tree.diff(to_tree, matcher); show_git_diff(formatter, workspace_command, tree_diff)?; } DiffFormat::ColorWords => { + let tree_diff = from_tree.diff(to_tree, matcher); show_color_words_diff(formatter, workspace_command, tree_diff)?; } + DiffFormat::Tool(tool) => { + merge_tools::generate_diff(formatter.raw(), from_tree, to_tree, matcher, tool)?; + } } } Ok(()) diff --git a/src/merge_tools.rs b/src/merge_tools.rs index e4b231391f..e71a46c215 100644 --- a/src/merge_tools.rs +++ b/src/merge_tools.rs @@ -16,7 +16,7 @@ use std::collections::HashMap; use std::fs::File; use std::io::{self, Write}; use std::path::{Path, PathBuf}; -use std::process::Command; +use std::process::{Command, Stdio}; use std::sync::Arc; use config::ConfigError; @@ -94,6 +94,14 @@ pub enum DiffEditError { Config(#[from] config::ConfigError), } +#[derive(Debug, Error)] +pub enum DiffGenerateError { + #[error(transparent)] + ExternalTool(#[from] ExternalToolError), + #[error(transparent)] + DiffCheckoutError(#[from] DiffCheckoutError), +} + #[derive(Debug, Error)] pub enum ConflictResolveError { #[error(transparent)] @@ -420,13 +428,48 @@ pub fn edit_diff( Ok(right_tree_state.current_tree_id().clone()) } +/// Generates textual diff by the specified `tool`, and writes into `writer`. +pub fn generate_diff( + writer: &mut dyn Write, + left_tree: &Tree, + right_tree: &Tree, + matcher: &dyn Matcher, + tool: &MergeTool, +) -> Result<(), DiffGenerateError> { + let store = left_tree.store(); + let diff_wc = check_out_trees(store, left_tree, right_tree, matcher)?; + // TODO: Add support for tools without directory diff functionality? + // 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:"); + let mut child = cmd + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .spawn() + .map_err(|source| ExternalToolError::FailedToExecute { + tool_binary: tool.program.clone(), + source, + })?; + io::copy(&mut child.stdout.take().unwrap(), writer).map_err(ExternalToolError::Io)?; + // Non-zero exit code isn't an error. For example, the traditional diff command + // will exit with 1 if inputs are different. + let exit_status = child.wait().map_err(ExternalToolError::Io)?; + tracing::info!(?cmd, ?exit_status, "The external diff generator exited:"); + Ok(()) +} + /// Merge/diff tool loaded from the settings. -#[derive(Clone, Debug, serde::Deserialize)] +#[derive(Clone, Debug, Eq, PartialEq, serde::Deserialize)] #[serde(default, rename_all = "kebab-case")] -struct MergeTool { +pub struct MergeTool { /// Program to execute. Must be defined; defaults to the tool name /// if not specified in the config. pub program: String, + /// Arguments to pass to the program when generating diffs. + /// `$left` and `$right` are replaced with the corresponding directories. + pub diff_args: Vec, /// Arguments to pass to the program when editing diffs. /// `$left` and `$right` are replaced with the corresponding directories. pub edit_args: Vec, @@ -450,6 +493,7 @@ impl Default for MergeTool { fn default() -> Self { MergeTool { program: String::new(), + diff_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(), edit_args: ["$left", "$right"].map(ToOwned::to_owned).to_vec(), merge_args: vec![], merge_tool_edits_conflict_markers: false, @@ -458,6 +502,13 @@ impl Default for MergeTool { } impl MergeTool { + pub fn with_program(program: impl Into) -> Self { + MergeTool { + program: program.into(), + ..Default::default() + } + } + pub fn with_edit_args(command_args: &CommandNameAndArgs) -> Self { let (name, args) = command_args.split_name_and_args(); let mut tool = MergeTool { @@ -484,7 +535,10 @@ impl MergeTool { } /// Loads merge tool options from `[merge-tools.]`. -fn get_tool_config(settings: &UserSettings, name: &str) -> Result, ConfigError> { +pub fn get_tool_config( + settings: &UserSettings, + name: &str, +) -> Result, ConfigError> { const TABLE_KEY: &str = "merge-tools"; let tools_table = settings.config().get_table(TABLE_KEY)?; if let Some(v) = tools_table.get(name) { @@ -583,6 +637,10 @@ mod tests { insta::assert_debug_snapshot!(get("").unwrap(), @r###" MergeTool { program: "meld", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -603,6 +661,10 @@ mod tests { insta::assert_debug_snapshot!(get(r#"ui.diff-editor = "my-diff""#).unwrap(), @r###" MergeTool { program: "my-diff", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -617,6 +679,10 @@ mod tests { get(r#"ui.diff-editor = "my-diff -l $left -r $right""#).unwrap(), @r###" MergeTool { program: "my-diff", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "-l", "$left", @@ -633,6 +699,10 @@ mod tests { get(r#"ui.diff-editor = ["my-diff", "--diff", "$left", "$right"]"#).unwrap(), @r###" MergeTool { program: "my-diff", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "--diff", "$left", @@ -653,6 +723,10 @@ mod tests { ).unwrap(), @r###" MergeTool { program: "foo bar", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "--edit", "args", @@ -674,6 +748,10 @@ mod tests { ).unwrap(), @r###" MergeTool { program: "MyDiff", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -687,6 +765,10 @@ mod tests { insta::assert_debug_snapshot!(get(r#"ui.diff-editor = ["meld"]"#).unwrap(), @r###" MergeTool { program: "meld", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -713,6 +795,10 @@ mod tests { insta::assert_debug_snapshot!(get("").unwrap(), @r###" MergeTool { program: "meld", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -741,6 +827,10 @@ mod tests { get(r#"ui.merge-editor = "my-merge $left $base $right $output""#).unwrap(), @r###" MergeTool { program: "my-merge", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -762,6 +852,10 @@ mod tests { ).unwrap(), @r###" MergeTool { program: "my-merge", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", @@ -786,6 +880,10 @@ mod tests { ).unwrap(), @r###" MergeTool { program: "foo bar", + diff_args: [ + "$left", + "$right", + ], edit_args: [ "$left", "$right", diff --git a/testing/fake-diff-editor.rs b/testing/fake-diff-editor.rs index ab1b83427f..e4a98d2f8f 100644 --- a/testing/fake-diff-editor.rs +++ b/testing/fake-diff-editor.rs @@ -80,6 +80,19 @@ fn main() { exit(1) } } + ["print", message] => { + println!("{message}"); + } + ["print-files-before"] => { + for base_name in files_recursively(&args.before).iter().sorted() { + println!("{base_name}"); + } + } + ["print-files-after"] => { + for base_name in files_recursively(&args.after).iter().sorted() { + println!("{base_name}"); + } + } ["rm", file] => { std::fs::remove_file(args.after.join(file)).unwrap(); } diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 00b333e939..03e94e8a59 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -255,7 +255,12 @@ impl TestEnvironment { // Simplified TOML escaping, hoping that there are no '"' or control characters // in it let escaped_diff_editor_path = diff_editor_path.to_str().unwrap().replace('\\', r"\\"); - self.add_config(&format!(r#"ui.diff-editor = "{escaped_diff_editor_path}""#)); + self.add_config(&format!( + r###" + ui.diff-editor = "fake-diff-editor" + merge-tools.fake-diff-editor.program = "{escaped_diff_editor_path}" + "### + )); let edit_script = self.env_root().join("diff_edit_script"); std::fs::write(&edit_script, "").unwrap(); self.add_env_var("DIFF_EDIT_SCRIPT", edit_script.to_str().unwrap()); diff --git a/tests/test_diff_command.rs b/tests/test_diff_command.rs index 9c4a4307e6..f0ae184fad 100644 --- a/tests/test_diff_command.rs +++ b/tests/test_diff_command.rs @@ -574,3 +574,98 @@ fn test_diff_skipped_context() { 10 10: j "###); } + +#[test] +fn test_diff_external_tool() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_success(test_env.env_root(), &["init", "repo", "--git"]); + 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_success(&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-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"]), @r###" + file1 + file2 + -- + file2 + file3 + "###); + + // diff with file patterns + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--tool=fake-diff-editor", "file1"]), @r###" + file1 + -- + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["log", "-p", "--tool=fake-diff-editor"]), @r###" + @ rlvkpnrz test.user@example.com 2001-02-03 04:05:09.000 +07:00 0cba70c7 + │ (no description set) + │ file1 + │ file2 + │ -- + │ file2 + │ file3 + ◉ qpvuntsm test.user@example.com 2001-02-03 04:05:08.000 +07:00 39b5a56f + │ (no description set) + │ -- + │ file1 + │ file2 + ◉ zzzzzzzz 1970-01-01 00:00:00.000 +00:00 00000000 + (empty) (no description set) + -- + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["show", "--tool=fake-diff-editor"]), @r###" + Commit ID: 0cba70c72186eabb5a2f91be63a8366b9f6da6c6 + Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp + Author: Test User (2001-02-03 04:05:08.000 +07:00) + Committer: Test User (2001-02-03 04:05:09.000 +07:00) + + (no description set) + + file1 + file2 + -- + file2 + file3 + "###); + + // Output of external diff tool shouldn't be escaped + std::fs::write(&edit_script, "print \x1b[1;31mred").unwrap(); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--color=always", "--tool=fake-diff-editor"]), + @r###" + red + "###); + + // Non-zero exit code isn't an error + std::fs::write(&edit_script, "print diff\0fail").unwrap(); + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["show", "--tool=fake-diff-editor"]), @r###" + Commit ID: 0cba70c72186eabb5a2f91be63a8366b9f6da6c6 + Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp + Author: Test User (2001-02-03 04:05:08.000 +07:00) + Committer: Test User (2001-02-03 04:05:09.000 +07:00) + + (no description set) + + diff + "###); +}