diff --git a/CHANGELOG.md b/CHANGELOG.md index 4868bfa5e..4194964b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). `--color-words`, `--git`, `--stat`, `--summary`, `--types`, and external diff tools in file-by-file mode. +* Color-words diff has gained [an option to display complex changes as separate + lines](docs/config.md#color-words-diff-options). + * A tilde (`~`) at the start of the path will now be expanded to the user's home directory when configuring a `signing.key` for SSH commit signing. diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 4eef2e9db..1883e1210 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -1345,8 +1345,10 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T let path_converter = language.path_converter; let template = (self_property, context_property) .map(move |(diff, context)| { + // TODO: load defaults from UserSettings? let options = diff_util::ColorWordsOptions { context: context.unwrap_or(diff_util::DEFAULT_CONTEXT_LINES), + max_inline_alternation: None, }; diff.into_formatted(move |formatter, store, tree_diff| { diff_util::show_color_words_diff( diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 953d3908a..073637e88 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -276,6 +276,22 @@ ] } }, + "diff": { + "type": "object", + "description": "Builtin diff formats settings", + "properties": { + "color-words": { + "type": "object", + "description": "Options for color-words diffs", + "properties": { + "max-inline-alternation": { + "type": "integer", + "description": "Maximum number of removed/added word alternation to inline" + } + } + } + } + }, "git": { "type": "object", "description": "Settings for git behavior (when using git backend)", diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index 85a9f18ff..a2a7f321a 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -5,6 +5,9 @@ amend = ["squash"] co = ["checkout"] unamend = ["unsquash"] +[diff.color-words] +max-inline-alternation = -1 + [ui] # TODO: delete ui.allow-filesets in jj 0.26+ allow-filesets = true diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 1fe336367..1b935cb80 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -407,15 +407,28 @@ fn collect_copied_sources<'a>( pub struct ColorWordsOptions { /// Number of context lines to show. pub context: usize, + /// Maximum number of removed/added word alternation to inline. + pub max_inline_alternation: Option, } impl ColorWordsOptions { fn from_settings_and_args( - _settings: &UserSettings, + settings: &UserSettings, args: &DiffFormatArgs, ) -> Result { + let config = settings.config(); + let max_inline_alternation = { + let key = "diff.color-words.max-inline-alternation"; + match config.get_int(key)? { + -1 => None, // unlimited + n => Some(usize::try_from(n).map_err(|err| { + config::ConfigError::Message(format!("invalid {key}: {err}")) + })?), + } + }; Ok(ColorWordsOptions { context: args.context.unwrap_or(DEFAULT_CONTEXT_LINES), + max_inline_alternation, }) } } @@ -467,13 +480,35 @@ fn show_color_words_diff_hunks( )?; } DiffHunk::Different(contents) => { - let word_diff = Diff::by_word(&contents); - let mut diff_line_iter = - DiffLineIterator::with_line_number(word_diff.hunks(), line_number); - for diff_line in diff_line_iter.by_ref() { - show_color_words_diff_line(formatter, &diff_line)?; + let word_diff_hunks = Diff::by_word(&contents).hunks().collect_vec(); + let can_inline = match options.max_inline_alternation { + None => true, // unlimited + Some(0) => false, // no need to count alternation + Some(max_num) => { + let groups = split_diff_hunks_by_matching_newline(&word_diff_hunks); + groups.map(count_diff_alternation).max().unwrap_or(0) <= max_num + } + }; + if can_inline { + let mut diff_line_iter = + DiffLineIterator::with_line_number(word_diff_hunks.iter(), line_number); + for diff_line in diff_line_iter.by_ref() { + show_color_words_diff_line(formatter, &diff_line)?; + } + line_number = diff_line_iter.next_line_number(); + } else { + let (left_lines, right_lines) = unzip_diff_hunks_to_lines(&word_diff_hunks); + for tokens in &left_lines { + show_color_words_line_number(formatter, Some(line_number.left), None)?; + show_color_words_single_sided_line(formatter, tokens, "removed")?; + line_number.left += 1; + } + for tokens in &right_lines { + show_color_words_line_number(formatter, None, Some(line_number.right))?; + show_color_words_single_sided_line(formatter, tokens, "added")?; + line_number.right += 1; + } } - line_number = diff_line_iter.next_line_number(); } } } @@ -544,6 +579,7 @@ fn show_color_words_line_number( Ok(()) } +/// Prints `diff_line` which may contain tokens originating from both sides. fn show_color_words_diff_line( formatter: &mut dyn Formatter, diff_line: &DiffLine, @@ -578,6 +614,56 @@ fn show_color_words_diff_line( Ok(()) } +/// Prints left/right-only line tokens with the given label. +fn show_color_words_single_sided_line( + formatter: &mut dyn Formatter, + tokens: &[(DiffTokenType, &[u8])], + label: &str, +) -> io::Result<()> { + formatter.with_label(label, |formatter| show_diff_line_tokens(formatter, tokens))?; + let (_, data) = tokens.last().expect("diff line must not be empty"); + if !data.ends_with(b"\n") { + writeln!(formatter)?; + }; + Ok(()) +} + +/// Counts number of diff-side alternation, ignoring matching hunks. +/// +/// This function is meant to measure visual complexity of diff hunks. It's easy +/// to read hunks containing some removed or added words, but is getting harder +/// as more removes and adds interleaved. +/// +/// For example, +/// - `[matching]` -> 0 +/// - `[left]` -> 1 +/// - `[left, matching, left]` -> 1 +/// - `[matching, left, right, matching, right]` -> 2 +/// - `[left, right, matching, right, left]` -> 3 +fn count_diff_alternation(diff_hunks: &[DiffHunk]) -> usize { + diff_hunks + .iter() + .filter_map(|hunk| match hunk { + DiffHunk::Matching(_) => None, + DiffHunk::Different(contents) => Some(contents), + }) + // Map non-empty diff side to index (0: left, 1: right) + .flat_map(|contents| contents.iter().positions(|content| !content.is_empty())) + // Omit e.g. left->(matching->)*left + .dedup() + .count() +} + +/// Splits hunks into slices of contiguous changed lines. +fn split_diff_hunks_by_matching_newline<'a, 'b>( + diff_hunks: &'a [DiffHunk<'b>], +) -> impl Iterator]> { + diff_hunks.split_inclusive(|hunk| match hunk { + DiffHunk::Matching(content) => content.contains(&b'\n'), + DiffHunk::Different(_) => false, + }) +} + struct FileContent { /// false if this file is likely text; true if it is likely binary. is_binary: bool, diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 66f2b6024..76adc698e 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use indoc::indoc; use itertools::Itertools; use crate::common::{escaped_fake_diff_editor_path, strip_last_line, TestEnvironment}; @@ -739,6 +740,507 @@ fn test_diff_hunks() { "###); } +#[test] +fn test_diff_color_words_inlining_threshold() { + let 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"); + + let render_diff = |max_alternation: i32, args: &[&str]| { + let config = format!("diff.color-words.max-inline-alternation={max_alternation}"); + test_env.jj_cmd_success( + &repo_path, + &[&["diff", "--config-toml", &config], args].concat(), + ) + }; + + let file1_path = "file1-single-line"; + let file2_path = "file2-multiple-lines-in-single-hunk"; + let file3_path = "file3-changes-across-lines"; + std::fs::write( + repo_path.join(file1_path), + indoc! {" + == adds == + a b c + == removes == + a b c d e f g + == adds + removes == + a b c d e + == adds + removes + adds == + a b c d e + == adds + removes + adds + removes == + a b c d e f g + "}, + ) + .unwrap(); + std::fs::write( + repo_path.join(file2_path), + indoc! {" + == adds; removes; adds + removes == + a b c + a b c d e f g + a b c d e + == adds + removes + adds; adds + removes + adds + removes == + a b c d e + a b c d e f g + "}, + ) + .unwrap(); + std::fs::write( + repo_path.join(file3_path), + indoc! {" + == adds == + a b c + == removes == + a b c d + e f g + == adds + removes == + a b c + d e + == adds + removes + adds == + a b c + d e + == adds + removes + adds + removes == + a b + c d e f g + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new"]); + std::fs::write( + repo_path.join(file1_path), + indoc! {" + == adds == + a X b Y Z c + == removes == + a c f + == adds + removes == + a X b d + == adds + removes + adds == + a X b d Y + == adds + removes + adds + removes == + X a Y b d Z e + "}, + ) + .unwrap(); + std::fs::write( + repo_path.join(file2_path), + indoc! {" + == adds; removes; adds + removes == + a X b Y Z c + a c f + a X b d + == adds + removes + adds; adds + removes + adds + removes == + a X b d Y + X a Y b d Z e + "}, + ) + .unwrap(); + std::fs::write( + repo_path.join(file3_path), + indoc! {" + == adds == + a X b + Y Z c + == removes == + a c f + == adds + removes == + a + X b d + == adds + removes + adds == + a X b d + Y + == adds + removes + adds + removes == + X a Y b d + Z e + "}, + ) + .unwrap(); + + // inline all by default + let stdout = test_env.jj_cmd_success(&repo_path, &["diff"]); + insta::assert_snapshot!(stdout, @r###" + Modified regular file file1-single-line: + 1 1: == adds == + 2 2: a X b Y Z c + 3 3: == removes == + 4 4: a b c d e f g + 5 5: == adds + removes == + 6 6: a X b c d e + 7 7: == adds + removes + adds == + 8 8: a X b c d eY + 9 9: == adds + removes + adds + removes == + 10 10: X a Y b c d Z e f g + Modified regular file file2-multiple-lines-in-single-hunk: + 1 1: == adds; removes; adds + removes == + 2 2: a X b Y Z c + 3 3: a b c d e f g + 4 4: a X b c d e + 5 5: == adds + removes + adds; adds + removes + adds + removes == + 6 6: a X b c d eY + 7 7: X a Y b c d Z e f g + Modified regular file file3-changes-across-lines: + 1 1: == adds == + 2 2: a X b + 2 3: Y Z c + 3 4: == removes == + 4 5: a b c d + 5 5: e f g + 6 6: == adds + removes == + 7 7: a + 7 8: X b c + 8 8: d e + 9 9: == adds + removes + adds == + 10 10: a X b c + 11 10: d e + 11 11: Y + 12 12: == adds + removes + adds + removes == + 13 13: X a Y b + 14 13: c d + 14 14: Z e f g + "###); + + // 0: no inlining + insta::assert_snapshot!(render_diff(0, &[]), @r###" + Modified regular file file1-single-line: + 1 1: == adds == + 2 : a b c + 2: a X b Y Z c + 3 3: == removes == + 4 : a b c d e f g + 4: a c f + 5 5: == adds + removes == + 6 : a b c d e + 6: a X b d + 7 7: == adds + removes + adds == + 8 : a b c d e + 8: a X b d Y + 9 9: == adds + removes + adds + removes == + 10 : a b c d e f g + 10: X a Y b d Z e + Modified regular file file2-multiple-lines-in-single-hunk: + 1 1: == adds; removes; adds + removes == + 2 : a b c + 3 : a b c d e f g + 4 : a b c d e + 2: a X b Y Z c + 3: a c f + 4: a X b d + 5 5: == adds + removes + adds; adds + removes + adds + removes == + 6 : a b c d e + 7 : a b c d e f g + 6: a X b d Y + 7: X a Y b d Z e + Modified regular file file3-changes-across-lines: + 1 1: == adds == + 2 : a b c + 2: a X b + 3: Y Z c + 3 4: == removes == + 4 : a b c d + 5 : e f g + 5: a c f + 6 6: == adds + removes == + 7 : a b c + 8 : d e + 7: a + 8: X b d + 9 9: == adds + removes + adds == + 10 : a b c + 11 : d e + 10: a X b d + 11: Y + 12 12: == adds + removes + adds + removes == + 13 : a b + 14 : c d e f g + 13: X a Y b d + 14: Z e + "###); + + // 1: inline adds-only or removes-only lines + insta::assert_snapshot!(render_diff(1, &[]), @r###" + Modified regular file file1-single-line: + 1 1: == adds == + 2 2: a X b Y Z c + 3 3: == removes == + 4 4: a b c d e f g + 5 5: == adds + removes == + 6 : a b c d e + 6: a X b d + 7 7: == adds + removes + adds == + 8 : a b c d e + 8: a X b d Y + 9 9: == adds + removes + adds + removes == + 10 : a b c d e f g + 10: X a Y b d Z e + Modified regular file file2-multiple-lines-in-single-hunk: + 1 1: == adds; removes; adds + removes == + 2 : a b c + 3 : a b c d e f g + 4 : a b c d e + 2: a X b Y Z c + 3: a c f + 4: a X b d + 5 5: == adds + removes + adds; adds + removes + adds + removes == + 6 : a b c d e + 7 : a b c d e f g + 6: a X b d Y + 7: X a Y b d Z e + Modified regular file file3-changes-across-lines: + 1 1: == adds == + 2 2: a X b + 2 3: Y Z c + 3 4: == removes == + 4 5: a b c d + 5 5: e f g + 6 6: == adds + removes == + 7 : a b c + 8 : d e + 7: a + 8: X b d + 9 9: == adds + removes + adds == + 10 : a b c + 11 : d e + 10: a X b d + 11: Y + 12 12: == adds + removes + adds + removes == + 13 : a b + 14 : c d e f g + 13: X a Y b d + 14: Z e + "###); + + // 2: inline up to adds + removes lines + insta::assert_snapshot!(render_diff(2, &[]), @r###" + Modified regular file file1-single-line: + 1 1: == adds == + 2 2: a X b Y Z c + 3 3: == removes == + 4 4: a b c d e f g + 5 5: == adds + removes == + 6 6: a X b c d e + 7 7: == adds + removes + adds == + 8 : a b c d e + 8: a X b d Y + 9 9: == adds + removes + adds + removes == + 10 : a b c d e f g + 10: X a Y b d Z e + Modified regular file file2-multiple-lines-in-single-hunk: + 1 1: == adds; removes; adds + removes == + 2 2: a X b Y Z c + 3 3: a b c d e f g + 4 4: a X b c d e + 5 5: == adds + removes + adds; adds + removes + adds + removes == + 6 : a b c d e + 7 : a b c d e f g + 6: a X b d Y + 7: X a Y b d Z e + Modified regular file file3-changes-across-lines: + 1 1: == adds == + 2 2: a X b + 2 3: Y Z c + 3 4: == removes == + 4 5: a b c d + 5 5: e f g + 6 6: == adds + removes == + 7 7: a + 7 8: X b c + 8 8: d e + 9 9: == adds + removes + adds == + 10 : a b c + 11 : d e + 10: a X b d + 11: Y + 12 12: == adds + removes + adds + removes == + 13 : a b + 14 : c d e f g + 13: X a Y b d + 14: Z e + "###); + + // 3: inline up to adds + removes + adds lines + insta::assert_snapshot!(render_diff(3, &[]), @r###" + Modified regular file file1-single-line: + 1 1: == adds == + 2 2: a X b Y Z c + 3 3: == removes == + 4 4: a b c d e f g + 5 5: == adds + removes == + 6 6: a X b c d e + 7 7: == adds + removes + adds == + 8 8: a X b c d eY + 9 9: == adds + removes + adds + removes == + 10 : a b c d e f g + 10: X a Y b d Z e + Modified regular file file2-multiple-lines-in-single-hunk: + 1 1: == adds; removes; adds + removes == + 2 2: a X b Y Z c + 3 3: a b c d e f g + 4 4: a X b c d e + 5 5: == adds + removes + adds; adds + removes + adds + removes == + 6 : a b c d e + 7 : a b c d e f g + 6: a X b d Y + 7: X a Y b d Z e + Modified regular file file3-changes-across-lines: + 1 1: == adds == + 2 2: a X b + 2 3: Y Z c + 3 4: == removes == + 4 5: a b c d + 5 5: e f g + 6 6: == adds + removes == + 7 7: a + 7 8: X b c + 8 8: d e + 9 9: == adds + removes + adds == + 10 10: a X b c + 11 10: d e + 11 11: Y + 12 12: == adds + removes + adds + removes == + 13 : a b + 14 : c d e f g + 13: X a Y b d + 14: Z e + "###); + + // 4: inline up to adds + removes + adds + removes lines + insta::assert_snapshot!(render_diff(4, &[]), @r###" + Modified regular file file1-single-line: + 1 1: == adds == + 2 2: a X b Y Z c + 3 3: == removes == + 4 4: a b c d e f g + 5 5: == adds + removes == + 6 6: a X b c d e + 7 7: == adds + removes + adds == + 8 8: a X b c d eY + 9 9: == adds + removes + adds + removes == + 10 10: X a Y b c d Z e f g + Modified regular file file2-multiple-lines-in-single-hunk: + 1 1: == adds; removes; adds + removes == + 2 2: a X b Y Z c + 3 3: a b c d e f g + 4 4: a X b c d e + 5 5: == adds + removes + adds; adds + removes + adds + removes == + 6 6: a X b c d eY + 7 7: X a Y b c d Z e f g + Modified regular file file3-changes-across-lines: + 1 1: == adds == + 2 2: a X b + 2 3: Y Z c + 3 4: == removes == + 4 5: a b c d + 5 5: e f g + 6 6: == adds + removes == + 7 7: a + 7 8: X b c + 8 8: d e + 9 9: == adds + removes + adds == + 10 10: a X b c + 11 10: d e + 11 11: Y + 12 12: == adds + removes + adds + removes == + 13 13: X a Y b + 14 13: c d + 14 14: Z e f g + "###); + + // context words in added/removed lines should be labeled as such + insta::assert_snapshot!(render_diff(2, &["--color=always"]), @r###" + Modified regular file file1-single-line: +  1  1: == adds == +  2  2: a X b Y Z c +  3  3: == removes == +  4  4: a b c d e f g +  5  5: == adds + removes == +  6  6: a X b c d e +  7  7: == adds + removes + adds == +  8 : a b c d e +  8: a X b d Y +  9  9: == adds + removes + adds + removes == +  10 : a b c d e f g +  10: X a Y b d Z e + Modified regular file file2-multiple-lines-in-single-hunk: +  1  1: == adds; removes; adds + removes == +  2  2: a X b Y Z c +  3  3: a b c d e f g +  4  4: a X b c d e +  5  5: == adds + removes + adds; adds + removes + adds + removes == +  6 : a b c d e +  7 : a b c d e f g +  6: a X b d Y +  7: X a Y b d Z e + Modified regular file file3-changes-across-lines: +  1  1: == adds == +  2  2: a X b +  2  3: Y Z c +  3  4: == removes == +  4  5: a b c d +  5  5: e f g +  6  6: == adds + removes == +  7  7: a +  7  8: X b c +  8  8: d e +  9  9: == adds + removes + adds == +  10 : a b c +  11 : d e +  10: a X b d +  11: Y +  12  12: == adds + removes + adds + removes == +  13 : a b +  14 : c d e f g +  13: X a Y b d +  14: Z e + "###); + insta::assert_snapshot!(render_diff(2, &["--color=debug"]), @r###" + <> + <><><><> + <><><><><><><><> + <><><><> + <><><><><><><><><><> + <><><><> + <><><><><><><><><><> + <><><><> + <><><><><><><> + <><><><><><><><> + <><><><> + <><><><><><><> + <><><><><><><><><> + <> + <><><><> + <><><><><><><><> + <><><><><><><><><><> + <><><><><><><><><><> + <><><><> + <><><><><><><> + <><><><><><><> + <><><><><><><><> + <><><><><><><><><> + <> + <><><><> + <><><><><><><> + <><><><><><> + <><><><> + <><><><><><><> + <><><><><><><><> + <><><><> + <><><><><> + <><><><><><><> + <><><><><><> + <><><><> + <><><><> + <><><><><> + <><><><><><><> + <><><><><> + <><><><> + <><><><> + <><><><><><> + <><><><><><><><> + <><><><><> + "###); +} + #[test] fn test_diff_missing_newline() { let test_env = TestEnvironment::default(); @@ -892,6 +1394,89 @@ fn test_color_words_diff_missing_newline() { 8 : h 9 : I "###); + + let stdout = test_env.jj_cmd_success( + &repo_path, + &[ + "log", + "--config-toml=diff.color-words.max-inline-alternation=0", + "-Tdescription", + "-pr::@-", + "--no-graph", + "--reversed", + ], + ); + insta::assert_snapshot!(stdout, @r###" + === Empty + Added regular file file1: + (empty) + === Add no newline + Modified regular file file1: + 1: a + 2: b + 3: c + 4: d + 5: e + 6: f + 7: g + 8: h + 9: i + === Modify first line + Modified regular file file1: + 1 : a + 1: A + 2 2: b + 3 3: c + 4 4: d + ... + === Modify middle line + Modified regular file file1: + 1 1: A + 2 2: b + 3 3: c + 4 4: d + 5 : e + 5: E + 6 6: f + 7 7: g + 8 8: h + 9 9: i + === Modify last line + Modified regular file file1: + ... + 6 6: f + 7 7: g + 8 8: h + 9 : i + 9: I + === Append newline + Modified regular file file1: + ... + 6 6: f + 7 7: g + 8 8: h + 9 : I + 9: I + === Remove newline + Modified regular file file1: + ... + 6 6: f + 7 7: g + 8 8: h + 9 : I + 9: I + === Empty + Modified regular file file1: + 1 : A + 2 : b + 3 : c + 4 : d + 5 : E + 6 : f + 7 : g + 8 : h + 9 : I + "###); } #[test] diff --git a/docs/config.md b/docs/config.md index 79adf6248..ae56ca74e 100644 --- a/docs/config.md +++ b/docs/config.md @@ -200,6 +200,29 @@ can override the default style with the following keys: ui.diff.format = "git" ``` +#### Color-words diff options + +In color-words diffs, changed words are displayed inline by default. Because +it's difficult to read a diff line with many removed/added words, there's a +threshold to switch to traditional separate-line format. + +* `max-inline-alternation`: Maximum number of removed/added word alternation to + inline. For example, ` ... ` sequence has 1 alternation, so the + line will be inline if `max-inline-alternation >= 1`. ` ... + ... ` sequence has 3 alternation. + + * `0`: disable inlining, making `--color-words` more similar to `--git` + * `1`: inline removes-only or adds-only lines + * `2`, `3`, ..: inline up to `2`, `3`, .. alternation + * `-1`: inline all lines (default) + + **This parameter is experimental.** The definition is subject to change. + +```toml +[diff.color-words] +max-inline-alternation = 3 +``` + ### Generating diffs by external command If `ui.diff.tool` is set, the specified diff command will be called instead of