From 53bf82ed4bcff6c22c0f7cdbe887ffaecc58efbc Mon Sep 17 00:00:00 2001 From: Tom Ward Date: Tue, 19 Mar 2024 18:16:11 +0000 Subject: [PATCH] cli: introduce 'ui.diff-viewer' setting 'ui.diff-viewer' combines 'ui.diff.tool' and 'ui.diff.format' into a single setting --- CHANGELOG.md | 7 +++ cli/src/config-schema.json | 13 +++++ cli/src/diff_util.rs | 55 ++++++++++++++++++-- cli/tests/test_diff_command.rs | 91 +++++++++++++++++++++++++++++++++- cli/tests/test_log_command.rs | 2 +- docs/config.md | 17 +++---- 6 files changed, 169 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc394a59aa..6f222ee10a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Deprecations +* `ui.diff.format` and `ui.diff.tool` were deprecated in favor of `ui.diff-viewer` + * `jj move` was deprecated in favor of `jj squash`. ### Breaking changes @@ -18,6 +20,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* Add `ui.diff-viewer` combining the both `ui.diff.format` and `ui.diff.tool` into a single setting. Built-in formats previously set via `ui.diff.format` can now be specified prefixed with `:`. Having a single option allows a repository level built-in format to override a global tool format. + + * `"ui.diff-viewer" = ":color-words"` + * `"ui.diff-viewer" = ["difft", "--color=always", "$left", "$right"]` + * Config now supports rgb hex colors (in the form `#rrggbb`) wherever existing color names are supported. * `ui.default-command` now accepts multiple string arguments, for more complex diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index a6bca7fffc..30fe67945b 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -85,6 +85,19 @@ "description": "Pager to use for displaying command output", "default": "less -FRX" }, + "diff-viewer": { + "description": "Format or tool used to show diffs", + "oneOf": [ + { + "type": "string" + }, + { + "type": "array", + "items": { + "type": "string" + } + }] + }, "diff": { "type": "object", "description": "Options for how diffs are displayed", diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 8377d7ace7..4c0ca0ae6a 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -98,7 +98,7 @@ pub fn diff_formats_for( ) -> Result, config::ConfigError> { let formats = diff_formats_from_args(settings, args)?; if formats.is_empty() { - Ok(vec![default_diff_format(settings, args.context)?]) + Ok(vec![diff_format_from_settings(settings, args.context)?]) } else { Ok(formats) } @@ -114,7 +114,7 @@ pub fn diff_formats_for_log( 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, args.context)?); + formats.push(diff_format_from_settings(settings, args.context)?); formats.dedup(); } Ok(formats) @@ -153,7 +153,7 @@ fn diff_formats_from_args( } fn builtin_diff_format(name: &str, num_context_lines: Option) -> Option { - match name.as_ref() { + match name { "summary" => Some(DiffFormat::Summary), "types" => Some(DiffFormat::Types), "git" => Some(DiffFormat::Git { @@ -167,10 +167,57 @@ fn builtin_diff_format(name: &str, num_context_lines: Option) -> Option, ) -> Result { + let config = settings.config(); + let format = if let Some(args) = config.get("ui.diff-viewer").optional()? { + if let CommandNameAndArgs::String(name) = &args { + // If setting is a single name starting with a colon it's treated as a built-in + // format. It is still possible to use an external tool starting + // with a colon passing it as an array rather than a string. + if let Some(stripped_name) = name.strip_prefix(':') { + Some( + builtin_diff_format(stripped_name, num_context_lines).ok_or_else(|| { + config::ConfigError::Message( + "Unknown format setting for 'ui.diff-viewer', built-in formats are \ + ':summary', ':types', ':git', ':color-words', and ':stat', or use an \ + external tool" + .to_string(), + ) + })?, + ) + } else { + let tool = merge_tools::get_external_tool_config(settings, name)?; + tool.map(|tool| DiffFormat::Tool(Box::new(tool))) + } + } else { + None + } + .or_else(|| { + Some(DiffFormat::Tool(Box::new( + ExternalMergeTool::with_diff_args(&args), + ))) + }) + } else { + Some(diff_format_from_deprecated_settings( + settings, + num_context_lines, + )?) + }; + + Ok(format.unwrap_or(DiffFormat::ColorWords { + context: num_context_lines.unwrap_or(DEFAULT_CONTEXT_LINES), + })) +} + +fn diff_format_from_deprecated_settings( + settings: &UserSettings, + num_context_lines: Option, +) -> Result { + // ui.diff.tool and ui.diff.format are deprecated in favour of the combined + // ui.diff-viewer let config = settings.config(); if let Some(args) = config.get("ui.diff.tool").optional()? { // External "tool" overrides the internal "format" option. diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 90014dc075..da0c5ea86c 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -740,6 +740,93 @@ fn test_diff_skipped_context_nondefault() { "###); } +#[test] +fn test_diff_formatter_setting() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(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(); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--config-toml=ui.diff-viewer=':summary'"]), @r###" + A file1 + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--config-toml=ui.diff-viewer=':types'"]), @r###" + -F file1 + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--config-toml=ui.diff-viewer=':git'"]), @r###" + diff --git a/file1 b/file1 + new file mode 100644 + index 0000000000..257cc5642c + --- /dev/null + +++ b/file1 + @@ -1,0 +1,1 @@ + +foo + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--config-toml=ui.diff-viewer=':color-words'"]), @r###" + Added regular file file1: + 1: foo + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--config-toml=ui.diff-viewer=':stat'"]), @r###" + file1 | 1 + + 1 file changed, 1 insertion(+), 0 deletions(-) + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_failure(&repo_path, &["diff", "--config-toml=ui.diff-viewer=':unknown'"]), @r###" + Config error: Unknown format setting for 'ui.diff-viewer', built-in formats are ':summary', ':types', ':git', ':color-words', and ':stat', or use an external tool + For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md. + "###); + + let edit_script = test_env.set_up_fake_diff_editor(); + std::fs::write( + edit_script, + "print-files-before\0print --\0print-files-after", + ) + .unwrap(); + + let command = escaped_fake_diff_editor_path(); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", "--config-toml=ui.diff-viewer='fake-diff-editor'"]), @r###" + -- + file1 + "###); + + insta::assert_snapshot!( + test_env.jj_cmd_success(&repo_path, &["diff", &format!(r#"--config-toml=ui.diff-viewer=["{command}"]"#)]), @r###" + -- + file1 + "###); + + if cfg!(windows) { + insta::assert_snapshot!( + test_env.jj_cmd_failure(&repo_path, &["diff", r#"--config-toml=ui.diff-viewer=["unknown-diff-binary"]"#]), @r###" + Error: Failed to generate diff + Caused by: + 1: Error executing 'unknown-diff-binary' (run with --debug to see the exact invocation) + 2: program not found + "###); + } else { + insta::assert_snapshot!( + test_env.jj_cmd_failure(&repo_path, &["diff", r#"--config-toml=ui.diff-viewer=["unknown-diff-binary"]"#]), @r###" + Error: Failed to generate diff + Caused by: + 1: Error executing 'unknown-diff-binary' (run with --debug to see the exact invocation) + 2: No such file or directory (os error 2) + "###); + } +} + #[test] fn test_diff_external_tool() { let mut test_env = TestEnvironment::default(); @@ -812,7 +899,7 @@ fn test_diff_external_tool() { "###); // Enabled by default, looks up the merge-tools table - let config = "--config-toml=ui.diff.tool='fake-diff-editor'"; + let config = "--config-toml=ui.diff-viewer='fake-diff-editor'"; insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", config]), @r###" file1 file2 @@ -823,7 +910,7 @@ fn test_diff_external_tool() { // Inlined command arguments let command = escaped_fake_diff_editor_path(); - let config = format!(r#"--config-toml=ui.diff.tool=["{command}", "$right", "$left"]"#); + let config = format!(r#"--config-toml=ui.diff-viewer=["{command}", "$right", "$left"]"#); insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["diff", &config]), @r###" file2 file3 diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index af5d6c9a15..60c28a2b8c 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -138,7 +138,7 @@ fn test_log_with_or_without_diff() { "description", "-p", "-s", - "--config-toml=ui.diff.format='summary'", + "--config-toml=ui.diff-viewer=':summary'", ], ); insta::assert_snapshot!(stdout, @r###" diff --git a/docs/config.md b/docs/config.md index 3ec7a5a728..772342cd9f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -165,24 +165,23 @@ useful reminder to fill in things like BUG=, TESTED= etc. ui.default-description = "\n\nTESTED=TODO" ``` -### Diff format +### Generating diffs with built-in and external formatters + +Different diff formatters can be chosen with the `ui.diff-viewer` setting. `jj` provides three built-in formatters, `:color-words` (the default), `:git`, and `:summary`. ```toml -# Possible values: "color-words" (default), "git", "summary" -ui.diff.format = "git" +[ui] +diff-viewer = ":git" ``` -### Generating diffs by external command - -If `ui.diff.tool` is set, the specified diff command will be called instead of -the internal diff function. +An external diff command can be used instead of a built-in formatter, specifying the command and its arguments. ```toml [ui] # Use Difftastic by default -diff.tool = ["difft", "--color=always", "$left", "$right"] +diff-viewer = ["difft", "--color=always", "$left", "$right"] # Use tool named "" (see below) -diff.tool = "" +diff-viewer = "" ``` The external diff tool can also be enabled by `diff --tool ` argument.