From ec99a17ae889fe524cfdd3a7e539c24787e1e016 Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Thu, 8 Aug 2024 20:00:50 -0400 Subject: [PATCH] copy-tracking: improve --summary and add --stat - add support for copy tracking to `diff --stat` - switch `--summary` to match git's output more closely - rework `show_diff_summary` signature to be more consistent --- CHANGELOG.md | 2 +- cli/src/commit_templater.rs | 10 +++- cli/src/diff_util.rs | 71 ++++++++++++++-------- cli/tests/test_diff_command.rs | 14 ++--- lib/src/repo_path.rs | 104 +++++++++++++++++++++++++++++++++ 5 files changed, 165 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31fb7f809..608a329c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### New features * The following diff formats now include information about copies and moves: - `--color-words`, `--summary` + `--color-words`, `--stat`, `--summary` * 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 d6b708401..b821d16ff 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -1394,14 +1394,18 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T let path_converter = language.path_converter; let template = self_property .map(move |diff| { + // TODO: don't pass separate copies of from_tree/to_tree/matcher + let from_tree = diff.from_tree.clone(); let to_tree = diff.to_tree.clone(); - diff.into_formatted(move |formatter, _store, tree_diff| { + let matcher = diff.matcher.clone(); + diff.into_formatted(move |formatter, _store, _tree_diff| { diff_util::show_diff_summary( formatter, - tree_diff, path_converter, - &Default::default(), + &from_tree, &to_tree, + matcher.as_ref(), + &Default::default(), ) }) }) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 1f14cb71a..a590c2aae 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -280,12 +280,17 @@ impl<'a> DiffRenderer<'a> { for format in &self.formats { match format { DiffFormat::Summary => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); - show_diff_summary(formatter, tree_diff, path_converter, copy_records, to_tree)?; + show_diff_summary( + formatter, + path_converter, + from_tree, + to_tree, + matcher, + copy_records, + )?; } DiffFormat::Stat => { - let no_copy_tracking = Default::default(); - let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); + let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); show_diff_stat(formatter, store, tree_diff, path_converter, width)?; } DiffFormat::Types => { @@ -1146,18 +1151,26 @@ pub fn show_git_diff( .block_on() } -// TODO rework this signature to pass both from_tree and to_tree explicitly #[instrument(skip_all)] pub fn show_diff_summary( formatter: &mut dyn Formatter, - mut tree_diff: TreeDiffStream, path_converter: &RepoPathUiConverter, - copy_records: &CopyRecords, + from_tree: &MergedTree, to_tree: &MergedTree, + matcher: &dyn Matcher, + copy_records: &CopyRecords, ) -> io::Result<()> { + let mut tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let copied_sources: HashSet<&RepoPath> = copy_records .iter() - .map(|record| record.source.as_ref()) + .filter_map(|record| { + if matcher.matches(&record.target) { + Some(record.source.as_ref()) + } else { + None + } + }) .collect(); async { @@ -1168,22 +1181,15 @@ pub fn show_diff_summary( }) = tree_diff.next().await { let (before, after) = diff.unwrap(); - let after_ui_path = path_converter.format_file_path(&after_path); if before_path != after_path { - let before_ui_path = path_converter.format_file_path(&before_path); + let path = path_converter.format_copied_path(&before_path, &after_path); if to_tree.path_value(&before_path).unwrap().is_absent() { - writeln!( - formatter.labeled("renamed"), - "R {before_ui_path} => {after_ui_path}" - )? + writeln!(formatter.labeled("renamed"), "R {path}")? } else { - writeln!( - formatter.labeled("copied"), - "C {before_ui_path} => {after_ui_path}" - )? + writeln!(formatter.labeled("copied"), "C {path}")? } } else { - let path = after_ui_path; + let path = path_converter.format_file_path(&after_path); match (before.is_present(), after.is_present()) { (true, true) => writeln!(formatter.labeled("modified"), "M {path}")?, (false, true) => writeln!(formatter.labeled("added"), "A {path}")?, @@ -1206,6 +1212,7 @@ struct DiffStat { path: String, added: usize, removed: usize, + is_deletion: bool, } fn get_diff_stat( @@ -1233,6 +1240,7 @@ fn get_diff_stat( path, added, removed, + is_deletion: right_content.contents.is_empty(), } } @@ -1244,21 +1252,29 @@ pub fn show_diff_stat( display_width: usize, ) -> Result<(), DiffRenderError> { let mut stats: Vec = vec![]; + let mut unresolved_renames = HashSet::new(); let mut max_path_width = 0; let mut max_diffs = 0; let mut diff_stream = materialized_diff_stream(store, tree_diff); async { while let Some(MaterializedTreeDiffEntry { - source: _, // TODO handle copy tracking - target: repo_path, + source: left_path, + target: right_path, value: diff, }) = diff_stream.next().await { let (left, right) = diff?; - let path = path_converter.format_file_path(&repo_path); - let left_content = diff_content(&repo_path, left)?; - let right_content = diff_content(&repo_path, right)?; + let left_content = diff_content(&left_path, left)?; + let right_content = diff_content(&right_path, right)?; + + let left_ui_path = path_converter.format_file_path(&left_path); + let path = if left_path == right_path { + left_ui_path + } else { + unresolved_renames.insert(left_ui_path); + path_converter.format_copied_path(&left_path, &right_path) + }; max_path_width = max(max_path_width, path.width()); let stat = get_diff_stat(path, &left_content, &right_content); max_diffs = max(max_diffs, stat.added + stat.removed); @@ -1283,10 +1299,15 @@ pub fn show_diff_stat( let mut total_added = 0; let mut total_removed = 0; - let total_files = stats.len(); + let mut total_files = 0; for stat in &stats { + if stat.is_deletion && unresolved_renames.contains(&stat.path) { + continue; + } + total_added += stat.added; total_removed += stat.removed; + total_files += 1; let bar_added = (stat.added as f64 * factor).ceil() as usize; let bar_removed = (stat.removed as f64 * factor).ceil() as usize; // replace start of path with ellipsis if the path is too long diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 63b8af6df..aba7cb7d6 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -65,7 +65,7 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###" M file2 - R file1 => file3 + R {file1 => file3} "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--types"]); @@ -158,7 +158,7 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "--git"]); insta::assert_snapshot!(stdout, @r###" M file2 - R file1 => file3 + R {file1 => file3} diff --git a/file1 b/file1 deleted file mode 100644 index 257cc5642c..0000000000 @@ -186,15 +186,15 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--stat"]); insta::assert_snapshot!(stdout, @r###" - file1 | 1 - - file2 | 3 ++- - file3 | 1 + - 3 files changed, 3 insertions(+), 2 deletions(-) + file2 | 3 ++- + {file1 => file3} | 0 + 2 files changed, 2 insertions(+), 1 deletion(-) "###); // Filter by glob pattern let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "glob:file[12]"]); insta::assert_snapshot!(stdout, @r###" + D file1 M file2 "###); @@ -213,7 +213,7 @@ fn test_diff_basic() { ); insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" M repo/file2 - R repo/file1 => repo/file3 + R repo/{file1 => file3} "###); insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" Warning: No matching entries for paths: repo/x, repo/y/z diff --git a/lib/src/repo_path.rs b/lib/src/repo_path.rs index dbb02fe78..0e305b5e9 100644 --- a/lib/src/repo_path.rs +++ b/lib/src/repo_path.rs @@ -21,6 +21,7 @@ use std::iter::FusedIterator; use std::ops::Deref; use std::path::{Component, Path, PathBuf}; +use itertools::Itertools; use ref_cast::{ref_cast_custom, RefCastCustom}; use thiserror::Error; @@ -502,6 +503,75 @@ impl RepoPathUiConverter { } } + /// Format a copy from `source` to `target` for display in the UI by + /// extracting common components and producing something like + /// "common/prefix/{source => target}/common/suffix". + /// + /// If `source == target`, returns `format_file_path(source)`. + pub fn format_copied_path(&self, source: &RepoPath, target: &RepoPath) -> String { + if source == target { + return self.format_file_path(source); + } + let mut formatted = String::new(); + match self { + RepoPathUiConverter::Fs { cwd, base } => { + let source_path = file_util::relative_path(cwd, &source.to_fs_path(base)); + let target_path = file_util::relative_path(cwd, &target.to_fs_path(base)); + + let source_components = source_path.components().collect_vec(); + let target_components = target_path.components().collect_vec(); + + let prefix_count = source_components + .iter() + .zip(target_components.iter()) + .take_while(|(source_component, target_component)| { + source_component == target_component + }) + .count() + .min(source_components.len().saturating_sub(1)) + .min(target_components.len().saturating_sub(1)); + + let suffix_count = source_components + .iter() + .rev() + .zip(target_components.iter().rev()) + .take_while(|(source_component, target_component)| { + source_component == target_component + }) + .count() + .min(source_components.len().saturating_sub(1)) + .min(target_components.len().saturating_sub(1)); + + fn format_components(c: &[std::path::Component]) -> String { + c.iter().collect::().to_str().unwrap().to_owned() + } + + if prefix_count > 0 { + formatted.push_str(&format_components(&source_components[0..prefix_count])); + formatted.push_str(std::path::MAIN_SEPARATOR_STR); + } + formatted.push('{'); + formatted.push_str(&format_components( + &source_components + [prefix_count..(source_components.len() - suffix_count).max(prefix_count)], + )); + formatted.push_str(" => "); + formatted.push_str(&format_components( + &target_components + [prefix_count..(target_components.len() - suffix_count).max(prefix_count)], + )); + formatted.push('}'); + if suffix_count > 0 { + formatted.push_str(std::path::MAIN_SEPARATOR_STR); + formatted.push_str(&format_components( + &source_components[source_components.len() - suffix_count..], + )); + } + } + } + formatted + } + /// Parses a path from the UI. /// /// It's up to the implementation whether absolute paths are allowed, and @@ -858,4 +928,38 @@ mod tests { Ok(repo_path("dir/file")) ); } + + #[test] + fn test_format_copied_path() { + let ui = RepoPathUiConverter::Fs { + cwd: PathBuf::from("."), + base: PathBuf::from("."), + }; + + let format = |before, after| { + ui.format_copied_path(repo_path(before), repo_path(after)) + .replace("\\", "/") + }; + + assert_eq!(format("one/two/three", "one/two/three"), "one/two/three"); + assert_eq!(format("one/two", "one/two/three"), "one/{two => two/three}"); + assert_eq!(format("one/two", "zero/one/two"), "{one => zero/one}/two"); + assert_eq!(format("one/two/three", "one/two"), "one/{two/three => two}"); + assert_eq!(format("zero/one/two", "one/two"), "{zero/one => one}/two"); + assert_eq!( + format("one/two", "one/two/three/one/two"), + "one/{ => two/three/one}/two" + ); + + assert_eq!(format("two/three", "four/three"), "{two => four}/three"); + assert_eq!( + format("one/two/three", "one/four/three"), + "one/{two => four}/three" + ); + assert_eq!(format("one/two/three", "one/three"), "one/{two => }/three"); + assert_eq!(format("one/two", "one/four"), "one/{two => four}"); + assert_eq!(format("two", "four"), "{two => four}"); + assert_eq!(format("file1", "file2"), "{file1 => file2}"); + assert_eq!(format("file-1", "file-2"), "{file-1 => file-2}"); + } }