From d988f3ff7a1c914e835861b04b8ead940415b053 Mon Sep 17 00:00:00 2001 From: Matt Kulukundis Date: Tue, 6 Aug 2024 14:04:58 -0400 Subject: [PATCH] copy-tracking: Add copy tracking as a post iteration step - force each diff command to explicitly enable copy tracking - enable copy tracking in diff_summary - post-process for diff iterator - post-process for diff stream - update changelog --- CHANGELOG.md | 2 + cli/src/diff_util.rs | 133 +++++++++++++++++++++++++-------- cli/tests/test_diff_command.rs | 23 +++--- lib/src/merged_tree.rs | 44 ++++++++++- 4 files changed, 154 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fbd7da8988a..b8e720725bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### New features +* The following diff formats now include information about copies and moves if supported by the backend (the Git backend does): `--summary` + ### Fixed bugs ## [0.20.0] - 2024-08-07 diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index b543b7087da..2a54136802e 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::cmp::max; -use std::collections::VecDeque; +use std::collections::{HashMap, VecDeque}; use std::ops::Range; use std::path::{Path, PathBuf}; use std::{io, mem}; @@ -284,29 +284,36 @@ impl<'a> DiffRenderer<'a> { show_diff_summary(formatter, tree_diff, path_converter)?; } DiffFormat::Stat => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let no_copy_tracking = Default::default(); + let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); show_diff_stat(formatter, store, tree_diff, path_converter, width)?; } DiffFormat::Types => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let no_copy_tracking = Default::default(); + let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); show_types(formatter, tree_diff, path_converter)?; } DiffFormat::NameOnly => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let no_copy_tracking = Default::default(); + let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); show_names(formatter, tree_diff, path_converter)?; } DiffFormat::Git { context } => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let no_copy_tracking = Default::default(); + let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); show_git_diff(formatter, store, tree_diff, *context)?; } DiffFormat::ColorWords { context } => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let no_copy_tracking = Default::default(); + let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); show_color_words_diff(formatter, store, tree_diff, path_converter, *context)?; } DiffFormat::Tool(tool) => { match tool.diff_invocation_mode { DiffToolMode::FileByFile => { - let tree_diff = from_tree.diff_stream(to_tree, matcher, copy_records); + let no_copy_tracking = Default::default(); + let tree_diff = + from_tree.diff_stream(to_tree, matcher, &no_copy_tracking); show_file_by_file_diff( ui, formatter, @@ -572,20 +579,28 @@ pub fn show_color_words_diff( let mut diff_stream = materialized_diff_stream(store, tree_diff); async { while let Some(MaterializedTreeDiffEntry { - source: _, - target: path, + source: left_path, + target: right_path, value: diff, }) = diff_stream.next().await { - let ui_path = path_converter.format_file_path(&path); + let left_ui_path = path_converter.format_file_path(&left_path); + let right_ui_path = path_converter.format_file_path(&right_path); let (left_value, right_value) = diff?; match (&left_value, &right_value) { - (_, MaterializedTreeValue::AccessDenied(source)) - | (MaterializedTreeValue::AccessDenied(source), _) => { + (MaterializedTreeValue::AccessDenied(source), _) => { write!( formatter.labeled("access-denied"), - "Access denied to {ui_path}:" + "Access denied to {left_ui_path}:" + )?; + writeln!(formatter, " {source}")?; + continue; + } + (_, MaterializedTreeValue::AccessDenied(source)) => { + write!( + formatter.labeled("access-denied"), + "Access denied to {right_ui_path}:" )?; writeln!(formatter, " {source}")?; continue; @@ -596,9 +611,9 @@ pub fn show_color_words_diff( let description = basic_diff_file_type(&right_value); writeln!( formatter.labeled("header"), - "Added {description} {ui_path}:" + "Added {description} {right_ui_path}:" )?; - let right_content = diff_content(&path, right_value)?; + let right_content = diff_content(&right_path, right_value)?; if right_content.is_empty() { writeln!(formatter.labeled("empty"), " (empty)")?; } else if right_content.is_binary { @@ -659,9 +674,19 @@ pub fn show_color_words_diff( ) } }; - let left_content = diff_content(&path, left_value)?; - let right_content = diff_content(&path, right_value)?; - writeln!(formatter.labeled("header"), "{description} {ui_path}:")?; + let left_content = diff_content(&left_path, left_value)?; + let right_content = diff_content(&right_path, right_value)?; + if left_path == right_path { + writeln!( + formatter.labeled("header"), + "{description} {right_ui_path}:" + )?; + } else { + writeln!( + formatter.labeled("header"), + "{description} {right_ui_path} ({left_ui_path} => {right_ui_path}):" + )?; + } if left_content.is_binary || right_content.is_binary { writeln!(formatter.labeled("binary"), " (binary)")?; } else { @@ -676,9 +701,9 @@ pub fn show_color_words_diff( let description = basic_diff_file_type(&left_value); writeln!( formatter.labeled("header"), - "Removed {description} {ui_path}:" + "Removed {description} {right_ui_path}:" )?; - let left_content = diff_content(&path, left_value)?; + let left_content = diff_content(&left_path, left_value)?; if left_content.is_empty() { writeln!(formatter.labeled("empty"), " (empty)")?; } else if left_content.is_binary { @@ -1128,27 +1153,73 @@ pub fn show_diff_summary( mut tree_diff: TreeDiffStream, path_converter: &RepoPathUiConverter, ) -> io::Result<()> { + #[derive(Debug)] + enum Type { + Copy(String, String), + Rename(String, String), + Modification(String), + Addition(String), + Deletion(String), + } + + let mut file_modifications = Vec::::new(); + let mut unresolved_renames = HashMap::::new(); async { while let Some(TreeDiffEntry { - source: _, // TODO handle copy tracking - target: repo_path, + source: before_path, + target: after_path, value: diff, }) = tree_diff.next().await { let (before, after) = diff.unwrap(); - let ui_path = path_converter.format_file_path(&repo_path); - if before.is_present() && after.is_present() { - writeln!(formatter.labeled("modified"), "M {ui_path}")?; - } else if before.is_absent() { - writeln!(formatter.labeled("added"), "A {ui_path}")?; + let before_ui_path = path_converter.format_file_path(&before_path); + let after_ui_path = path_converter.format_file_path(&after_path); + if before_path != after_path { + if let Some(&idx) = unresolved_renames.get(&before_ui_path) { + if let Some(Type::Deletion(_)) = file_modifications.get(idx) { + file_modifications[idx] = Type::Rename(before_ui_path, after_ui_path); + } + } else { + unresolved_renames.insert(before_ui_path.clone(), file_modifications.len()); + file_modifications.push(Type::Copy(before_ui_path, after_ui_path)); + } } else { - // `R` could be interpreted as "renamed" - writeln!(formatter.labeled("removed"), "D {ui_path}")?; + match (before.is_present(), after.is_present()) { + (true, true) => file_modifications.push(Type::Modification(before_ui_path)), + (false, true) => file_modifications.push(Type::Addition(before_ui_path)), + (true, false) => { + if let Some(&idx) = unresolved_renames.get(&before_ui_path) { + if let Some(Type::Copy(before, after)) = file_modifications.get(idx) { + file_modifications[idx] = + Type::Rename(before.clone(), after.clone()); + } + } else { + unresolved_renames + .insert(before_ui_path.clone(), file_modifications.len()); + file_modifications.push(Type::Deletion(before_ui_path)); + } + } + _ => {} + } } } - Ok(()) } - .block_on() + .block_on(); + + for modification in file_modifications { + match modification { + Type::Copy(before, after) => { + writeln!(formatter.labeled("copied"), "C {before} => {after}")? + } + Type::Rename(before, after) => { + writeln!(formatter.labeled("renamed"), "R {before} => {after}")? + } + Type::Modification(path) => writeln!(formatter.labeled("modified"), "M {path}")?, + Type::Addition(path) => writeln!(formatter.labeled("added"), "A {path}")?, + Type::Deletion(path) => writeln!(formatter.labeled("removed"), "D {path}")?, + } + } + Ok(()) } struct DiffStat { diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index b9f89c4e4e7..d4c7e0de730 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -67,9 +67,8 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s"]); insta::assert_snapshot!(stdout, @r###" - D file1 + R file1 => file3 M file2 - A file3 "###); let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "--types"]); @@ -161,9 +160,8 @@ fn test_diff_basic() { let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "--git"]); insta::assert_snapshot!(stdout, @r###" - D file1 + R file1 => file3 M file2 - A file3 diff --git a/file1 b/file1 deleted file mode 100644 index 257cc5642c..0000000000 @@ -218,9 +216,8 @@ fn test_diff_basic() { ], ); insta::assert_snapshot!(stdout.replace('\\', "/"), @r###" - D repo/file1 + R repo/file1 => repo/file3 M repo/file2 - A repo/file3 "###); insta::assert_snapshot!(stderr.replace('\\', "/"), @r###" Warning: No matching entries for paths: repo/x, repo/y/z @@ -1253,12 +1250,12 @@ fn test_diff_external_file_by_file_tool() { 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"), "foo\n").unwrap(); - std::fs::write(repo_path.join("file2"), "foo\n").unwrap(); + std::fs::write(repo_path.join("file1"), "file1\n").unwrap(); + std::fs::write(repo_path.join("file2"), "file2\n").unwrap(); test_env.jj_cmd_ok(&repo_path, &["new"]); std::fs::remove_file(repo_path.join("file1")).unwrap(); - std::fs::write(repo_path.join("file2"), "foo\nbar\n").unwrap(); - std::fs::write(repo_path.join("file3"), "foo\n").unwrap(); + std::fs::write(repo_path.join("file2"), "file2\nfile2\n").unwrap(); + std::fs::write(repo_path.join("file3"), "file3\n").unwrap(); let edit_script = test_env.set_up_fake_diff_editor(); std::fs::write( @@ -1299,7 +1296,7 @@ fn test_diff_external_file_by_file_tool() { insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["log", "-p", config]), @r###" - @ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 39d9055d + @ rlvkpnrz test.user@example.com 2001-02-03 08:05:09 f12f62fb │ (no description set) │ == │ file1 @@ -1313,7 +1310,7 @@ fn test_diff_external_file_by_file_tool() { │ file3 │ -- │ file3 - ○ qpvuntsm test.user@example.com 2001-02-03 08:05:08 0ad4ef22 + ○ qpvuntsm test.user@example.com 2001-02-03 08:05:08 6e485984 │ (no description set) │ == │ file1 @@ -1328,7 +1325,7 @@ fn test_diff_external_file_by_file_tool() { insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["show", config]), @r###" - Commit ID: 39d9055d70873099fd924b9af218289d5663eac8 + Commit ID: f12f62fb1d73629c1b44abd4d5bbb500d7f8b86c Change ID: rlvkpnrzqnoowoytxnquwvuryrwnrmlp Author: Test User (2001-02-03 08:05:09) Committer: Test User (2001-02-03 08:05:09) diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index c1f4845786e..0e726fb1af6 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -29,7 +29,7 @@ use futures::{Stream, TryStreamExt}; use itertools::{EitherOrBoth, Itertools}; use crate::backend; -use crate::backend::{BackendResult, CopyRecords, MergedTreeId, TreeId, TreeValue}; +use crate::backend::{BackendResult, CopyRecord, CopyRecords, MergedTreeId, TreeId, TreeValue}; use crate::matchers::{EverythingMatcher, Matcher}; use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent}; @@ -328,6 +328,29 @@ pub struct TreeDiffEntry { pub value: BackendResult<(MergedTreeValue, MergedTreeValue)>, } +impl TreeDiffEntry { + fn adjust_for_copy_tracking( + self, + source_tree: &MergedTree, + copy_records: &CopyRecords, + ) -> TreeDiffEntry { + let Some(CopyRecord { source, .. }) = copy_records.for_target(&self.target) else { + return self; + }; + + Self { + source: source.clone(), + target: self.target, + value: match self.value { + Ok((_, target_value)) => source_tree + .path_value(source) + .map(|source_value| (source_value, target_value)), + Err(e) => Err(e), + }, + } + } +} + /// Type alias for the result from `MergedTree::diff_stream()`. We use a /// `Stream` instead of an `Iterator` so high-latency backends (e.g. cloud-based /// ones) can fetch trees asynchronously. @@ -606,7 +629,9 @@ impl Iterator for ConflictIterator { pub struct TreeDiffIterator<'matcher> { store: Arc, stack: Vec, + source: MergedTree, matcher: &'matcher dyn Matcher, + copy_records: &'matcher CopyRecords, } struct TreeDiffDirItem { @@ -628,7 +653,7 @@ impl<'matcher> TreeDiffIterator<'matcher> { trees1: &Merge, trees2: &Merge, matcher: &'matcher dyn Matcher, - _copy_records: &'matcher CopyRecords, + copy_records: &'matcher CopyRecords, ) -> Self { assert!(Arc::ptr_eq(trees1.first().store(), trees2.first().store())); let root_dir = RepoPath::root(); @@ -641,7 +666,9 @@ impl<'matcher> TreeDiffIterator<'matcher> { Self { store: trees1.first().store().clone(), stack, + source: MergedTree::new(trees1.clone()), matcher, + copy_records, } } @@ -790,12 +817,15 @@ impl Iterator for TreeDiffIterator<'_> { fn next(&mut self) -> Option { self.next_impl() + .map(|diff_entry| diff_entry.adjust_for_copy_tracking(&self.source, self.copy_records)) } } /// Stream of differences between two trees. pub struct TreeDiffStreamImpl<'matcher> { matcher: &'matcher dyn Matcher, + copy_records: &'matcher CopyRecords, + source_tree: MergedTree, /// Pairs of tree values that may or may not be ready to emit, sorted in the /// order we want to emit them. If either side is a tree, there will be /// a corresponding entry in `pending_trees`. @@ -870,11 +900,13 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> { tree1: MergedTree, tree2: MergedTree, matcher: &'matcher dyn Matcher, - _copy_records: &'matcher CopyRecords, + copy_records: &'matcher CopyRecords, max_concurrent_reads: usize, ) -> Self { let mut stream = Self { matcher, + copy_records, + source_tree: tree1.clone(), items: BTreeMap::new(), pending_trees: VecDeque::new(), max_concurrent_reads, @@ -1062,7 +1094,11 @@ impl Stream for TreeDiffStreamImpl<'_> { type Item = TreeDiffEntry; fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.as_mut().poll_next_impl(cx) + self.as_mut().poll_next_impl(cx).map(|option| { + option.map(|diff_entry| { + diff_entry.adjust_for_copy_tracking(&self.source_tree, self.copy_records) + }) + }) } }