From 9a39501cbfc001e2711d710dd55725ed844a7abd Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Tue, 20 Aug 2024 22:33:35 +0900 Subject: [PATCH] copies: filter rename source entries by CopiesTreeDiffStream --- cli/src/commit_templater.rs | 1 - cli/src/diff_util.rs | 33 ++-------------------------- lib/src/copies.rs | 41 ++++++++++++++++++++--------------- lib/tests/test_merged_tree.rs | 13 +---------- 4 files changed, 27 insertions(+), 61 deletions(-) diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 0ac64be31a..1883e12109 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -1355,7 +1355,6 @@ fn builtin_tree_diff_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, T formatter, store, tree_diff, - &Default::default(), // TODO: real copy tracking path_converter, &options, ) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 4678ad5c6b..63a841ecd8 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -328,14 +328,7 @@ impl<'a> DiffRenderer<'a> { DiffFormat::ColorWords(options) => { let tree_diff = from_tree.diff_stream_with_copies(to_tree, matcher, copy_records); - show_color_words_diff( - formatter, - store, - tree_diff, - copy_records, - path_converter, - options, - )?; + show_color_words_diff(formatter, store, tree_diff, path_converter, options)?; } DiffFormat::Tool(tool) => { match tool.diff_invocation_mode { @@ -348,7 +341,6 @@ impl<'a> DiffRenderer<'a> { store, tree_diff, path_converter, - copy_records, tool, ) } @@ -759,7 +751,6 @@ pub fn show_color_words_diff( formatter: &mut dyn Formatter, store: &Store, tree_diff: BoxStream, - copy_records: &CopyRecords, path_converter: &RepoPathUiConverter, options: &ColorWordsOptions, ) -> Result<(), DiffRenderError> { @@ -774,9 +765,6 @@ pub fn show_color_words_diff( 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?; - if right_value.is_absent() && copy_records.has_source(&left_path) { - continue; - } match (&left_value, &right_value) { (MaterializedTreeValue::AccessDenied(source), _) => { @@ -909,7 +897,6 @@ pub fn show_file_by_file_diff( store: &Store, tree_diff: BoxStream, path_converter: &RepoPathUiConverter, - copy_records: &CopyRecords, tool: &ExternalMergeTool, ) -> Result<(), DiffRenderError> { fn create_file( @@ -936,10 +923,6 @@ pub fn show_file_by_file_diff( }) = diff_stream.next().await { let (left_value, right_value) = diff?; - if right_value.is_absent() && copy_records.has_source(&left_path) { - continue; - } - let left_ui_path = path_converter.format_file_path(&left_path); let right_ui_path = path_converter.format_file_path(&right_path); @@ -1290,11 +1273,6 @@ pub fn show_git_diff( let left_part = git_diff_part(&left_path, left_value)?; let right_part = git_diff_part(&right_path, right_value)?; - // Skip the "delete" entry when there is a rename. - if right_part.mode.is_none() && copy_records.has_source(&left_path) { - continue; - } - formatter.with_label("file_header", |formatter| { writeln!( formatter, @@ -1405,11 +1383,7 @@ pub fn show_diff_summary( match (before.is_present(), after.is_present()) { (true, true) => writeln!(formatter.labeled("modified"), "M {path}")?, (false, true) => writeln!(formatter.labeled("added"), "A {path}")?, - (true, false) => { - if !copy_records.has_source(&before_path) { - writeln!(formatter.labeled("removed"), "D {path}")?; - } - } + (true, false) => writeln!(formatter.labeled("removed"), "D {path}")?, (false, false) => unreachable!(), } } @@ -1565,9 +1539,6 @@ pub fn show_types( }) = tree_diff.next().await { let (before, after) = diff?; - if after.is_absent() && copy_records.has_source(&source) { - continue; - } writeln!( formatter.labeled("modified"), "{}{} {}", diff --git a/lib/src/copies.rs b/lib/src/copies.rs index 7ce3baceb5..961c0fe104 100644 --- a/lib/src/copies.rs +++ b/lib/src/copies.rs @@ -121,26 +121,33 @@ impl Stream for CopiesTreeDiffStream<'_> { type Item = CopiesTreeDiffEntry; fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - let Some(diff_entry) = ready!(self.inner.as_mut().poll_next(cx)) else { - return Poll::Ready(None); - }; + while let Some(diff_entry) = ready!(self.inner.as_mut().poll_next(cx)) { + let Some(CopyRecord { source, .. }) = self.copy_records.for_target(&diff_entry.path) + else { + let target_deleted = + matches!(&diff_entry.value, Ok((_, target_value)) if target_value.is_absent()); + if target_deleted && self.copy_records.has_source(&diff_entry.path) { + // Skip the "delete" entry when there is a rename. + continue; + } + return Poll::Ready(Some(CopiesTreeDiffEntry { + source: diff_entry.path.clone(), + target: diff_entry.path, + value: diff_entry.value, + })); + }; - let Some(CopyRecord { source, .. }) = self.copy_records.for_target(&diff_entry.path) else { return Poll::Ready(Some(CopiesTreeDiffEntry { - source: diff_entry.path.clone(), + source: source.clone(), target: diff_entry.path, - value: diff_entry.value, + value: diff_entry.value.and_then(|(_, target_value)| { + self.source_tree + .path_value(source) + .map(|source_value| (source_value, target_value)) + }), })); - }; - - Poll::Ready(Some(CopiesTreeDiffEntry { - source: source.clone(), - target: diff_entry.path, - value: diff_entry.value.and_then(|(_, target_value)| { - self.source_tree - .path_value(source) - .map(|source_value| (source_value, target_value)) - }), - })) + } + + Poll::Ready(None) } } diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 77b4d0776a..3450c52c46 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -855,7 +855,7 @@ fn test_diff_copy_tracing() { .map(|diff| (diff.source, diff.target, diff.value.unwrap())) .collect() .block_on(); - assert_eq!(diff.len(), 4); + assert_eq!(diff.len(), 3); assert_eq!( diff[0].clone(), ( @@ -880,17 +880,6 @@ fn test_diff_copy_tracing() { ); assert_eq!( diff[2].clone(), - ( - removed_path.to_owned(), - removed_path.to_owned(), - ( - Merge::resolved(before.path_value(removed_path).unwrap()), - Merge::absent() - ), - ) - ); - assert_eq!( - diff[3].clone(), ( removed_path.to_owned(), added_path.to_owned(),