From f5187fa06377a97093d49b6898f5492c3f89111f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 22 Aug 2024 19:27:25 +0900 Subject: [PATCH] copies: determine copy/rename operation by CopiesTreeDiffStream Not all callers need this information, but I assumed it's relatively cheap to look up the source path in the target tree compared to diffing. This could be represented as Regular(_)|Copied(_, _)|Renamed(_, _), but it's a bit weird if Copied and Renamed were separate variants. Instead, I decided to wrap copy metadata in Option. --- cli/src/diff_util.rs | 22 +++++++------- lib/src/copies.rs | 55 ++++++++++++++++++++++++++++------- lib/src/merged_tree.rs | 1 + lib/tests/test_merged_tree.rs | 5 ++-- 4 files changed, 60 insertions(+), 23 deletions(-) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index f381e7138..e02af772b 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -35,6 +35,7 @@ use jj_lib::conflicts::materialized_diff_stream; use jj_lib::conflicts::MaterializedTreeDiffEntry; use jj_lib::conflicts::MaterializedTreeValue; use jj_lib::copies::CopiesTreeDiffEntry; +use jj_lib::copies::CopyOperation; use jj_lib::copies::CopyRecords; use jj_lib::diff::Diff; use jj_lib::diff::DiffHunk; @@ -1297,11 +1298,10 @@ pub fn show_git_diff( writeln!(formatter, "index {left_hash}..{right_hash}")?; } (Some(left_mode), Some(right_mode)) => { - if left_path != right_path { - let operation = if to_tree.path_value(left_path)?.is_absent() { - "rename" - } else { - "copy" + if let Some(op) = path.copy_operation() { + let operation = match op { + CopyOperation::Copy => "copy", + CopyOperation::Rename => "rename", }; // TODO: include similarity index? writeln!(formatter, "{operation} from {left_path_string}")?; @@ -1375,13 +1375,13 @@ pub fn show_diff_summary( let (before, after) = values?; let before_path = path.source(); let after_path = path.target(); - if before_path != after_path { + if let Some(op) = path.copy_operation() { + let (label, sigil) = match op { + CopyOperation::Copy => ("copied", "C"), + CopyOperation::Rename => ("renamed", "R"), + }; 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 {path}")? - } else { - writeln!(formatter.labeled("copied"), "C {path}")? - } + writeln!(formatter.labeled(label), "{sigil} {path}")? } else { let path = path_converter.format_file_path(after_path); match (before.is_present(), after.is_present()) { diff --git a/lib/src/copies.rs b/lib/src/copies.rs index 6448e7a30..372387c12 100644 --- a/lib/src/copies.rs +++ b/lib/src/copies.rs @@ -90,6 +90,15 @@ impl CopyRecords { } } +/// Whether or not the source path was deleted. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum CopyOperation { + /// The source path was not deleted. + Copy, + /// The source path was renamed to the destination. + Rename, +} + /// A `TreeDiffEntry` with copy information. pub struct CopiesTreeDiffEntry { /// The path. @@ -98,11 +107,11 @@ pub struct CopiesTreeDiffEntry { pub values: BackendResult<(MergedTreeValue, MergedTreeValue)>, } -/// Path of `CopiesTreeDiffEntry`. +/// Path and copy information of `CopiesTreeDiffEntry`. #[derive(Clone, Debug, Eq, PartialEq)] pub struct CopiesTreeDiffEntryPath { - /// The source path if this is a copy or rename. - pub source: Option, + /// The source path and copy information if this is a copy or rename. + pub source: Option<(RepoPathBuf, CopyOperation)>, /// The target path. pub target: RepoPathBuf, } @@ -110,19 +119,26 @@ pub struct CopiesTreeDiffEntryPath { impl CopiesTreeDiffEntryPath { /// The source path. pub fn source(&self) -> &RepoPath { - self.source.as_ref().unwrap_or(&self.target) + self.source.as_ref().map_or(&self.target, |(path, _)| path) } /// The target path. pub fn target(&self) -> &RepoPath { &self.target } + + /// Whether this entry was copied or renamed from the source. Returns `None` + /// if the path is unchanged. + pub fn copy_operation(&self) -> Option { + self.source.as_ref().map(|(_, op)| *op) + } } /// Wraps a `TreeDiffStream`, adding support for copies and renames. pub struct CopiesTreeDiffStream<'a> { inner: TreeDiffStream<'a>, source_tree: MergedTree, + target_tree: MergedTree, copy_records: &'a CopyRecords, } @@ -131,14 +147,32 @@ impl<'a> CopiesTreeDiffStream<'a> { pub fn new( inner: TreeDiffStream<'a>, source_tree: MergedTree, + target_tree: MergedTree, copy_records: &'a CopyRecords, ) -> Self { Self { inner, source_tree, + target_tree, copy_records, } } + + fn resolve_copy_source( + &self, + source: &RepoPath, + values: BackendResult<(MergedTreeValue, MergedTreeValue)>, + ) -> BackendResult<(CopyOperation, (MergedTreeValue, MergedTreeValue))> { + let (_, target_value) = values?; + let source_value = self.source_tree.path_value(source)?; + // If the source path is deleted in the target tree, it's a rename. + let copy_op = if self.target_tree.path_value(source)?.is_absent() { + CopyOperation::Rename + } else { + CopyOperation::Copy + }; + Ok((copy_op, (source_value, target_value))) + } } impl Stream for CopiesTreeDiffStream<'_> { @@ -163,16 +197,17 @@ impl Stream for CopiesTreeDiffStream<'_> { })); }; + let (copy_op, values) = match self.resolve_copy_source(source, diff_entry.values) { + Ok((copy_op, values)) => (copy_op, Ok(values)), + // Fall back to "copy" (= path still exists) if unknown. + Err(err) => (CopyOperation::Copy, Err(err)), + }; return Poll::Ready(Some(CopiesTreeDiffEntry { path: CopiesTreeDiffEntryPath { - source: Some(source.clone()), + source: Some((source.clone(), copy_op)), target: diff_entry.path, }, - values: diff_entry.values.and_then(|(_, target_value)| { - self.source_tree - .path_value(source) - .map(|source_value| (source_value, target_value)) - }), + values, })); } diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 09c2f4b36..c2b0dc8af 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -313,6 +313,7 @@ impl MergedTree { Box::pin(CopiesTreeDiffStream::new( stream, self.clone(), + other.clone(), copy_records, )) } diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 9b7208eb0..4ba8a55f5 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -20,6 +20,7 @@ use jj_lib::backend::FileId; use jj_lib::backend::MergedTreeId; use jj_lib::backend::TreeValue; use jj_lib::copies::CopiesTreeDiffEntryPath; +use jj_lib::copies::CopyOperation; use jj_lib::copies::CopyRecords; use jj_lib::files::MergeResult; use jj_lib::matchers::EverythingMatcher; @@ -889,7 +890,7 @@ fn test_diff_copy_tracing() { diff[1].clone(), ( CopiesTreeDiffEntryPath { - source: Some(modified_path.to_owned()), + source: Some((modified_path.to_owned(), CopyOperation::Copy)), target: copied_path.to_owned(), }, ( @@ -902,7 +903,7 @@ fn test_diff_copy_tracing() { diff[2].clone(), ( CopiesTreeDiffEntryPath { - source: Some(removed_path.to_owned()), + source: Some((removed_path.to_owned(), CopyOperation::Rename)), target: added_path.to_owned(), }, (