From 6877ec43167c1945689f0c2995f47bd422dc0585 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 22 Sep 2024 14:13:06 +0900 Subject: [PATCH] cli: add diff --ignore-all-space/--ignore-space-change flags These flags only apply to line-based diffs. This is easy, and seems still useful to highlight whitespace changes (that could be ignored by line diffing.) I've added short options only to "diff"-like commands. It seemed unclear if they were added to deeply-nested commands such as "op log". Closes #3781 --- CHANGELOG.md | 3 + cli/src/commands/diff.rs | 2 + cli/src/commands/interdiff.rs | 2 + cli/src/commit_templater.rs | 12 +++- cli/src/diff_util.rs | 109 +++++++++++++++++++++++++++---- cli/tests/cli-reference@.md.snap | 16 +++++ cli/tests/test_diff_command.rs | 104 +++++++++++++++++++++++++++++ 7 files changed, 234 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee16d34c9..8f7fa6416 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### New features +* Added diff options to ignore whitespace when comparing lines. Whitespace + changes are still highlighted. + ### Fixed bugs diff --git a/cli/src/commands/diff.rs b/cli/src/commands/diff.rs index 582d0b0e9..7394f5f14 100644 --- a/cli/src/commands/diff.rs +++ b/cli/src/commands/diff.rs @@ -38,6 +38,8 @@ use crate::ui::Ui; /// commit. For example, `jj diff --from main` shows the changes from "main" /// (perhaps a bookmark name) to the working-copy commit. #[derive(clap::Args, Clone, Debug)] +#[command(mut_arg("ignore_all_space", |a| a.short('w')))] +#[command(mut_arg("ignore_space_change", |a| a.short('b')))] pub(crate) struct DiffArgs { /// Show changes in this revision, compared to its parent(s) /// diff --git a/cli/src/commands/interdiff.rs b/cli/src/commands/interdiff.rs index adbf13bc7..b8d5cd487 100644 --- a/cli/src/commands/interdiff.rs +++ b/cli/src/commands/interdiff.rs @@ -30,6 +30,8 @@ use crate::ui::Ui; /// versions, consider `jj evolog -p` instead. #[derive(clap::Args, Clone, Debug)] #[command(group(ArgGroup::new("to_diff").args(&["from", "to"]).multiple(true).required(true)))] +#[command(mut_arg("ignore_all_space", |a| a.short('w')))] +#[command(mut_arg("ignore_space_change", |a| a.short('b')))] pub(crate) struct InterdiffArgs { /// Show changes from this revision #[arg(long)] diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index c99bb5d97..018101460 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -1506,6 +1506,9 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T // TODO: load defaults from UserSettings? let options = diff_util::ColorWordsDiffOptions { context: context.unwrap_or(diff_util::DEFAULT_CONTEXT_LINES), + line_diff: diff_util::LineDiffOptions { + compare_mode: diff_util::LineCompareMode::Exact, + }, max_inline_alternation: Some(3), }; diff.into_formatted(move |formatter, store, tree_diff| { @@ -1540,6 +1543,9 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T .map(|(diff, context)| { let options = diff_util::UnifiedDiffOptions { context: context.unwrap_or(diff_util::DEFAULT_CONTEXT_LINES), + line_diff: diff_util::LineDiffOptions { + compare_mode: diff_util::LineCompareMode::Exact, + }, }; diff.into_formatted(move |formatter, store, tree_diff| { diff_util::show_git_diff(formatter, store, tree_diff, &options) @@ -1562,7 +1568,11 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T let path_converter = language.path_converter; let template = (self_property, width_property) .map(move |(diff, width)| { - let options = diff_util::DiffStatOptions {}; + let options = diff_util::DiffStatOptions { + line_diff: diff_util::LineDiffOptions { + compare_mode: diff_util::LineCompareMode::Exact, + }, + }; diff.into_formatted(move |formatter, store, tree_diff| { diff_util::show_diff_stat( formatter, diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index f5afacba6..d59b60038 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -39,6 +39,10 @@ use jj_lib::conflicts::MaterializedTreeValue; use jj_lib::copies::CopiesTreeDiffEntry; use jj_lib::copies::CopyOperation; use jj_lib::copies::CopyRecords; +use jj_lib::diff::find_line_ranges; +use jj_lib::diff::CompareBytesExactly; +use jj_lib::diff::CompareBytesIgnoreAllWhitespace; +use jj_lib::diff::CompareBytesIgnoreWhitespaceAmount; use jj_lib::diff::Diff; use jj_lib::diff::DiffHunk; use jj_lib::diff::DiffHunkKind; @@ -113,6 +117,14 @@ pub struct DiffFormatArgs { /// Number of lines of context to show #[arg(long)] context: Option, + + // Short flags are set by command to avoid future conflicts. + /// Ignore whitespace when comparing lines. + #[arg(long)] // short = 'w' + ignore_all_space: bool, + /// Ignore changes in amount of whitespace when comparing lines. + #[arg(long, conflicts_with = "ignore_all_space")] // short = 'b' + ignore_space_change: bool, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -430,10 +442,63 @@ pub fn get_copy_records<'a>( Ok(block_on_stream(stream).filter_ok(|record| matcher.matches(&record.target))) } +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct LineDiffOptions { + /// How equivalence of lines is tested. + pub compare_mode: LineCompareMode, + // TODO: add --ignore-blank-lines, etc. which aren't mutually exclusive. +} + +impl LineDiffOptions { + fn from_args(args: &DiffFormatArgs) -> Self { + let compare_mode = if args.ignore_all_space { + LineCompareMode::IgnoreAllSpace + } else if args.ignore_space_change { + LineCompareMode::IgnoreSpaceChange + } else { + LineCompareMode::Exact + }; + LineDiffOptions { compare_mode } + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum LineCompareMode { + /// Compares lines literally. + Exact, + /// Compares lines ignoring any whitespace occurrences. + IgnoreAllSpace, + /// Compares lines ignoring changes in whitespace amount. + IgnoreSpaceChange, +} + +fn diff_by_line<'input, T: AsRef<[u8]> + ?Sized + 'input>( + inputs: impl IntoIterator, + options: &LineDiffOptions, +) -> Diff<'input> { + // TODO: If we add --ignore-blank-lines, its tokenizer will have to attach + // blank lines to the preceding range. Maybe it can also be implemented as a + // post-process (similar to refine_changed_regions()) that expands unchanged + // regions across blank lines. + match options.compare_mode { + LineCompareMode::Exact => { + Diff::for_tokenizer(inputs, find_line_ranges, CompareBytesExactly) + } + LineCompareMode::IgnoreAllSpace => { + Diff::for_tokenizer(inputs, find_line_ranges, CompareBytesIgnoreAllWhitespace) + } + LineCompareMode::IgnoreSpaceChange => { + Diff::for_tokenizer(inputs, find_line_ranges, CompareBytesIgnoreWhitespaceAmount) + } + } +} + #[derive(Clone, Debug, Eq, PartialEq)] pub struct ColorWordsDiffOptions { /// Number of context lines to show. pub context: usize, + /// How lines are tokenized and compared. + pub line_diff: LineDiffOptions, /// Maximum number of removed/added word alternation to inline. pub max_inline_alternation: Option, } @@ -455,6 +520,7 @@ impl ColorWordsDiffOptions { }; Ok(ColorWordsDiffOptions { context: args.context.unwrap_or(DEFAULT_CONTEXT_LINES), + line_diff: LineDiffOptions::from_args(args), max_inline_alternation, }) } @@ -466,7 +532,7 @@ fn show_color_words_diff_hunks( right: &[u8], options: &ColorWordsDiffOptions, ) -> io::Result<()> { - let line_diff = Diff::by_line([left, right]); + let line_diff = diff_by_line([left, right], &options.line_diff); let mut line_number = DiffLineNumber { left: 1, right: 1 }; // Matching entries shouldn't appear consecutively in diff of two inputs. // However, if the inputs have conflicts, there may be a hunk that can be @@ -477,9 +543,13 @@ fn show_color_words_diff_hunks( for hunk in line_diff.hunks() { match hunk.kind { DiffHunkKind::Matching => { - // TODO: add support for unmatched contexts - debug_assert!(hunk.contents.iter().all_equal()); - contexts.push(hunk.contents[0]); + // TODO: better handling of unmatched contexts + debug_assert!(hunk + .contents + .iter() + .map(|content| content.split_inclusive(|b| *b == b'\n').count()) + .all_equal()); + contexts.push(hunk.contents[1]); } DiffHunkKind::Different => { let num_after = if emitted { options.context } else { 0 }; @@ -1097,12 +1167,15 @@ fn git_diff_part( pub struct UnifiedDiffOptions { /// Number of context lines to show. pub context: usize, + /// How lines are tokenized and compared. + pub line_diff: LineDiffOptions, } impl UnifiedDiffOptions { fn from_args(args: &DiffFormatArgs) -> Self { UnifiedDiffOptions { context: args.context.unwrap_or(DEFAULT_CONTEXT_LINES), + line_diff: LineDiffOptions::from_args(args), } } } @@ -1165,14 +1238,16 @@ fn unified_diff_hunks<'content>( right_line_range: 1..1, lines: vec![], }; - let diff = Diff::by_line([left_content, right_content]); + let diff = diff_by_line([left_content, right_content], &options.line_diff); let mut diff_hunks = diff.hunks().peekable(); while let Some(hunk) = diff_hunks.next() { match hunk.kind { DiffHunkKind::Matching => { - // TODO: add support for unmatched contexts - debug_assert!(hunk.contents.iter().all_equal()); - let mut lines = hunk.contents[0].split_inclusive(|b| *b == b'\n').fuse(); + // Just use the right (i.e. new) content. We could count the + // number of skipped lines separately, but the number of the + // context lines should match the displayed content. + let [_, right] = hunk.contents.try_into().unwrap(); + let mut lines = right.split_inclusive(|b| *b == b'\n').fuse(); if !current_hunk.lines.is_empty() { // The previous hunk line should be either removed/added. current_hunk.extend_context_lines(lines.by_ref().take(options.context)); @@ -1448,11 +1523,16 @@ pub fn show_diff_summary( } #[derive(Clone, Debug, Eq, PartialEq)] -pub struct DiffStatOptions {} +pub struct DiffStatOptions { + /// How lines are tokenized and compared. + pub line_diff: LineDiffOptions, +} impl DiffStatOptions { - fn from_args(_args: &DiffFormatArgs) -> Self { - DiffStatOptions {} + fn from_args(args: &DiffFormatArgs) -> Self { + DiffStatOptions { + line_diff: LineDiffOptions::from_args(args), + } } } @@ -1467,12 +1547,15 @@ fn get_diff_stat( path: String, left_content: &FileContent, right_content: &FileContent, - _options: &DiffStatOptions, + options: &DiffStatOptions, ) -> DiffStat { // TODO: this matches git's behavior, which is to count the number of newlines // in the file. but that behavior seems unhelpful; no one really cares how // many `0x0a` characters are in an image. - let diff = Diff::by_line([&left_content.contents, &right_content.contents]); + let diff = diff_by_line( + [&left_content.contents, &right_content.contents], + &options.line_diff, + ); let mut added = 0; let mut removed = 0; for hunk in diff.hunks() { diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 62e8ed40c..5f6b7bf07 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -641,6 +641,8 @@ With the `--from` and/or `--to` options, shows the difference from/to the given * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `-w`, `--ignore-all-space` — Ignore whitespace when comparing lines +* `-b`, `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -741,6 +743,8 @@ Lists the previous commits which a change has pointed to. The current commit of * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `--ignore-all-space` — Ignore whitespace when comparing lines +* `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -1202,6 +1206,8 @@ This excludes changes from other commits by temporarily rebasing `--from` onto ` * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `-w`, `--ignore-all-space` — Ignore whitespace when comparing lines +* `-b`, `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -1245,6 +1251,8 @@ Spans of revisions that are not included in the graph per `--revisions` are rend * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `--ignore-all-space` — Ignore whitespace when comparing lines +* `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -1392,6 +1400,8 @@ Compare changes to the repository between two operations * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `--ignore-all-space` — Ignore whitespace when comparing lines +* `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -1426,6 +1436,8 @@ Like other commands, `jj op log` snapshots the current working-copy changes and * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `--ignore-all-space` — Ignore whitespace when comparing lines +* `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -1490,6 +1502,8 @@ Show changes to the repository in an operation * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `--ignore-all-space` — Ignore whitespace when comparing lines +* `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines @@ -1825,6 +1839,8 @@ Show commit description and changes in a revision * `--color-words` — Show a word-level diff with changes indicated only by color * `--tool ` — Generate diff by external command * `--context ` — Number of lines of context to show +* `--ignore-all-space` — Ignore whitespace when comparing lines +* `--ignore-space-change` — Ignore changes in amount of whitespace when comparing lines diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index f8311d4f2..3f7f8e93f 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -1521,6 +1521,110 @@ fn test_color_words_diff_missing_newline() { "###); } +#[test] +fn test_diff_ignore_whitespace() { + 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"); + + std::fs::write( + repo_path.join("file1"), + indoc! {" + foo { + bar; + } + baz {} + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["new", "-mindent + whitespace insertion"]); + std::fs::write( + repo_path.join("file1"), + indoc! {" + { + foo { + bar; + } + } + baz { } + "}, + ) + .unwrap(); + test_env.jj_cmd_ok(&repo_path, &["status"]); + + // Git diff as reference output + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--git", "--ignore-all-space"]); + insta::assert_snapshot!(stdout, @r#" + diff --git a/file1 b/file1 + index f532aa68ad..033c4a6168 100644 + --- a/file1 + +++ b/file1 + @@ -1,4 +1,6 @@ + +{ + foo { + bar; + } + +} + baz { } + "#); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--git", "--ignore-space-change"]); + insta::assert_snapshot!(stdout, @r#" + diff --git a/file1 b/file1 + index f532aa68ad..033c4a6168 100644 + --- a/file1 + +++ b/file1 + @@ -1,4 +1,6 @@ + -foo { + +{ + + foo { + bar; + + } + } + -baz {} + +baz { } + "#); + + // Diff-stat should respects the whitespace options + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--stat", "--ignore-all-space"]); + insta::assert_snapshot!(stdout, @r#" + file1 | 2 ++ + 1 file changed, 2 insertions(+), 0 deletions(-) + "#); + let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--stat", "--ignore-space-change"]); + insta::assert_snapshot!(stdout, @r#" + file1 | 6 ++++-- + 1 file changed, 4 insertions(+), 2 deletions(-) + "#); + + // Word-level changes are still highlighted + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diff", "--color=always", "--ignore-all-space"], + ); + insta::assert_snapshot!(stdout, @r#" + Modified regular file file1: +  1: { +  1  2: foo { +  2  3: bar; +  3  4: } +  5: } +  4  6: baz { } + "#); + let stdout = test_env.jj_cmd_success( + &repo_path, + &["diff", "--color=always", "--ignore-space-change"], + ); + insta::assert_snapshot!(stdout, @r#" + Modified regular file file1: +  1: { +  1  2:  foo { +  2  3: bar; +  4:  } +  3  5: } +  4  6: baz { } + "#); +} + #[test] fn test_diff_skipped_context() { let test_env = TestEnvironment::default();