From 0b9d23c3ada9b3885dec1118f0ff7b080641c4c3 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Wed, 2 Aug 2023 12:12:11 +0900 Subject: [PATCH] cli: add option to generate textual diff by external command This is basic implementation. There's no config knob to enable the external diff command by default. It reuses the merge-tools table because that's how external diff/merge commands are currently configured. We might want to reorganize them in #1285. If you run "jj diff --tool meld", GUI diff will open and jj will wait for meld to quit. This also applies to "jj log -p". The "diff --tool gui" behavior is somewhat useful, but "log -p --tool gui" wouldn't. We might want some flag to mark the tool output can't be streamed. Another thing to consider is tools that can't generate directory diffs. Git executes ext-diff tool per file, but we don't. Difftastic can compare directories, and doing that should be more efficient since diffs can be computed in parallel (at the expense of unsorted output.) Closes #1886 --- CHANGELOG.md | 3 + docs/config.md | 15 +++++ src/cli_util.rs | 8 ++- src/config-schema.json | 6 ++ src/diff_util.rs | 37 ++++++++++--- src/merge_tools.rs | 106 ++++++++++++++++++++++++++++++++++-- testing/fake-diff-editor.rs | 13 +++++ tests/common/mod.rs | 7 ++- tests/test_diff_command.rs | 95 ++++++++++++++++++++++++++++++++ 9 files changed, 276 insertions(+), 14 deletions(-) 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 + "###); +}