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

@@ -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.

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(),
                         )
                     })
                 })

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<DiffStat> = 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

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

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::<PathBuf>().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}");
+    }
 }