From 08262eb15242f81df5ce1ed42869ebf9d61841c4 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Thu, 22 Aug 2024 18:58:50 +0900 Subject: [PATCH] copies: extract (source, target) path pair to separate type This patch adds accessor methods as I'm going to change the underlying data types. Since entry values are consumed separately, these methods are implemented on CopiesTreeDiffEntryPath, not on *TreeDiffEntry. --- cli/src/diff_util.rs | 105 ++++++++++++++-------------------- lib/src/conflicts.rs | 42 +++++--------- lib/src/copies.rs | 35 ++++++++++-- lib/tests/test_merged_tree.rs | 21 ++++--- 4 files changed, 101 insertions(+), 102 deletions(-) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index 1ff7f2a1d..f381e7138 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -772,14 +772,11 @@ pub fn show_color_words_diff( ) -> Result<(), DiffRenderError> { let mut diff_stream = materialized_diff_stream(store, tree_diff); async { - while let Some(MaterializedTreeDiffEntry { - source: left_path, - target: right_path, - values, - }) = diff_stream.next().await - { - let left_ui_path = path_converter.format_file_path(&left_path); - let right_ui_path = path_converter.format_file_path(&right_path); + while let Some(MaterializedTreeDiffEntry { path, values }) = diff_stream.next().await { + let left_path = path.source(); + let right_path = path.target(); + 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) = values?; match (&left_value, &right_value) { @@ -807,7 +804,7 @@ pub fn show_color_words_diff( formatter.labeled("header"), "Added {description} {right_ui_path}:" )?; - let right_content = diff_content(&right_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 { @@ -863,8 +860,8 @@ pub fn show_color_words_diff( ) } }; - let left_content = diff_content(&left_path, left_value)?; - let right_content = diff_content(&right_path, right_value)?; + 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"), @@ -892,7 +889,7 @@ pub fn show_color_words_diff( formatter.labeled("header"), "Removed {description} {right_ui_path}:" )?; - let left_content = diff_content(&left_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 { @@ -932,15 +929,12 @@ pub fn show_file_by_file_diff( let right_wc_dir = temp_dir.path().join("right"); let mut diff_stream = materialized_diff_stream(store, tree_diff); async { - while let Some(MaterializedTreeDiffEntry { - source: left_path, - target: right_path, - values, - }) = diff_stream.next().await - { + while let Some(MaterializedTreeDiffEntry { path, values }) = diff_stream.next().await { let (left_value, right_value) = values?; - let left_ui_path = path_converter.format_file_path(&left_path); - let right_ui_path = path_converter.format_file_path(&right_path); + let left_path = path.source(); + let right_path = path.target(); + let left_ui_path = path_converter.format_file_path(left_path); + let right_ui_path = path_converter.format_file_path(right_path); match (&left_value, &right_value) { (_, MaterializedTreeValue::AccessDenied(source)) => { @@ -961,8 +955,8 @@ pub fn show_file_by_file_diff( } _ => {} } - let left_path = create_file(&left_path, &left_wc_dir, left_value)?; - let right_path = create_file(&right_path, &right_wc_dir, right_value)?; + let left_path = create_file(left_path, &left_wc_dir, left_value)?; + let right_path = create_file(right_path, &right_wc_dir, right_value)?; invoke_external_diff( ui, @@ -1276,18 +1270,15 @@ pub fn show_git_diff( let mut diff_stream = materialized_diff_stream(store, tree_diff); async { - while let Some(MaterializedTreeDiffEntry { - source: left_path, - target: right_path, - values, - }) = diff_stream.next().await - { + while let Some(MaterializedTreeDiffEntry { path, values }) = diff_stream.next().await { + let left_path = path.source(); + let right_path = path.target(); let left_path_string = left_path.as_internal_file_string(); let right_path_string = right_path.as_internal_file_string(); let (left_value, right_value) = values?; - let left_part = git_diff_part(&left_path, left_value)?; - let right_part = git_diff_part(&right_path, right_value)?; + let left_part = git_diff_part(left_path, left_value)?; + let right_part = git_diff_part(right_path, right_value)?; formatter.with_label("file_header", |formatter| { writeln!( @@ -1307,7 +1298,7 @@ pub fn show_git_diff( } (Some(left_mode), Some(right_mode)) => { if left_path != right_path { - let operation = if to_tree.path_value(&left_path)?.is_absent() { + let operation = if to_tree.path_value(left_path)?.is_absent() { "rename" } else { "copy" @@ -1380,22 +1371,19 @@ pub fn show_diff_summary( let mut tree_diff = from_tree.diff_stream_with_copies(to_tree, matcher, copy_records); async { - while let Some(CopiesTreeDiffEntry { - source: before_path, - target: after_path, - values, - }) = tree_diff.next().await - { + while let Some(CopiesTreeDiffEntry { path, values }) = tree_diff.next().await { let (before, after) = values?; + let before_path = path.source(); + let after_path = path.target(); if before_path != after_path { - let path = path_converter.format_copied_path(&before_path, &after_path); - if to_tree.path_value(&before_path).unwrap().is_absent() { + 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}")? } } else { - let path = path_converter.format_file_path(&after_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}")?, @@ -1459,22 +1447,19 @@ pub fn show_diff_stat( let mut diff_stream = materialized_diff_stream(store, tree_diff); async { - while let Some(MaterializedTreeDiffEntry { - source: left_path, - target: right_path, - values, - }) = diff_stream.next().await - { + while let Some(MaterializedTreeDiffEntry { path, values }) = diff_stream.next().await { let (left, right) = values?; - let left_content = diff_content(&left_path, left)?; - let right_content = diff_content(&right_path, right)?; + let left_path = path.source(); + let right_path = path.target(); + 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 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) + 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); @@ -1548,19 +1533,14 @@ pub fn show_types( let mut tree_diff = from_tree.diff_stream_with_copies(to_tree, matcher, copy_records); async { - while let Some(CopiesTreeDiffEntry { - source, - target, - values, - }) = tree_diff.next().await - { + while let Some(CopiesTreeDiffEntry { path, values }) = tree_diff.next().await { let (before, after) = values?; writeln!( formatter.labeled("modified"), "{}{} {}", diff_summary_char(&before), diff_summary_char(&after), - path_converter.format_copied_path(&source, &target) + path_converter.format_copied_path(path.source(), path.target()) )?; } Ok(()) @@ -1587,11 +1567,12 @@ pub fn show_names( path_converter: &RepoPathUiConverter, ) -> io::Result<()> { async { - while let Some(CopiesTreeDiffEntry { - target: repo_path, .. - }) = tree_diff.next().await - { - writeln!(formatter, "{}", path_converter.format_file_path(&repo_path))?; + while let Some(CopiesTreeDiffEntry { path, .. }) = tree_diff.next().await { + writeln!( + formatter, + "{}", + path_converter.format_file_path(path.target()) + )?; } Ok(()) } diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index 3805b8fd4..e7fc41638 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -35,6 +35,7 @@ use crate::backend::SymlinkId; use crate::backend::TreeId; use crate::backend::TreeValue; use crate::copies::CopiesTreeDiffEntry; +use crate::copies::CopiesTreeDiffEntryPath; use crate::diff::Diff; use crate::diff::DiffHunk; use crate::files; @@ -44,7 +45,6 @@ use crate::merge::Merge; use crate::merge::MergeBuilder; use crate::merge::MergedTreeValue; use crate::repo_path::RepoPath; -use crate::repo_path::RepoPathBuf; use crate::store::Store; const CONFLICT_START_LINE: &[u8] = b"<<<<<<<"; @@ -343,8 +343,7 @@ fn diff_size(hunks: &[DiffHunk]) -> usize { } pub struct MaterializedTreeDiffEntry { - pub source: RepoPathBuf, - pub target: RepoPathBuf, + pub path: CopiesTreeDiffEntryPath, pub values: BackendResult<(MaterializedTreeValue, MaterializedTreeValue)>, } @@ -353,31 +352,20 @@ pub fn materialized_diff_stream<'a>( tree_diff: BoxStream<'a, CopiesTreeDiffEntry>, ) -> impl Stream + 'a { tree_diff - .map( - |CopiesTreeDiffEntry { - source, - target, - values, - }| async { - match values { - Err(err) => MaterializedTreeDiffEntry { - source, - target, - values: Err(err), - }, - Ok((before, after)) => { - let before_future = materialize_tree_value(store, &source, before); - let after_future = materialize_tree_value(store, &target, after); - let values = try_join!(before_future, after_future); - MaterializedTreeDiffEntry { - source, - target, - values, - } - } + .map(|CopiesTreeDiffEntry { path, values }| async { + match values { + Err(err) => MaterializedTreeDiffEntry { + path, + values: Err(err), + }, + Ok((before, after)) => { + let before_future = materialize_tree_value(store, path.source(), before); + let after_future = materialize_tree_value(store, path.target(), after); + let values = try_join!(before_future, after_future); + MaterializedTreeDiffEntry { path, values } } - }, - ) + } + }) .buffered((store.concurrency() / 2).max(1)) } diff --git a/lib/src/copies.rs b/lib/src/copies.rs index 78160a2e3..01b673283 100644 --- a/lib/src/copies.rs +++ b/lib/src/copies.rs @@ -92,12 +92,31 @@ impl CopyRecords { /// A `TreeDiffEntry` with copy information. pub struct CopiesTreeDiffEntry { + /// The path. + pub path: CopiesTreeDiffEntryPath, + /// The resolved tree values if available. + pub values: BackendResult<(MergedTreeValue, MergedTreeValue)>, +} + +/// Path of `CopiesTreeDiffEntry`. +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct CopiesTreeDiffEntryPath { /// The source path. pub source: RepoPathBuf, /// The target path. pub target: RepoPathBuf, - /// The resolved tree values if available. - pub values: BackendResult<(MergedTreeValue, MergedTreeValue)>, +} + +impl CopiesTreeDiffEntryPath { + /// The source path. + pub fn source(&self) -> &RepoPath { + &self.source + } + + /// The target path. + pub fn target(&self) -> &RepoPath { + &self.target + } } /// Wraps a `TreeDiffStream`, adding support for copies and renames. @@ -136,15 +155,19 @@ impl Stream for CopiesTreeDiffStream<'_> { continue; } return Poll::Ready(Some(CopiesTreeDiffEntry { - source: diff_entry.path.clone(), - target: diff_entry.path, + path: CopiesTreeDiffEntryPath { + source: diff_entry.path.clone(), + target: diff_entry.path, + }, values: diff_entry.values, })); }; return Poll::Ready(Some(CopiesTreeDiffEntry { - source: source.clone(), - target: diff_entry.path, + path: CopiesTreeDiffEntryPath { + source: source.clone(), + target: diff_entry.path, + }, values: diff_entry.values.and_then(|(_, target_value)| { self.source_tree .path_value(source) diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 0af49791e..7b2aa23c2 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -19,6 +19,7 @@ use jj_lib::backend::CopyRecord; use jj_lib::backend::FileId; use jj_lib::backend::MergedTreeId; use jj_lib::backend::TreeValue; +use jj_lib::copies::CopiesTreeDiffEntryPath; use jj_lib::copies::CopyRecords; use jj_lib::files::MergeResult; use jj_lib::matchers::EverythingMatcher; @@ -867,15 +868,17 @@ fn test_diff_copy_tracing() { let diff: Vec<_> = before_merged .diff_stream_with_copies(&after_merged, &EverythingMatcher, ©_records) - .map(|diff| (diff.source, diff.target, diff.values.unwrap())) + .map(|diff| (diff.path, diff.values.unwrap())) .collect() .block_on(); assert_eq!(diff.len(), 3); assert_eq!( diff[0].clone(), ( - modified_path.to_owned(), - modified_path.to_owned(), + CopiesTreeDiffEntryPath { + source: modified_path.to_owned(), + target: modified_path.to_owned(), + }, ( Merge::resolved(before.path_value(modified_path).unwrap()), Merge::resolved(after.path_value(modified_path).unwrap()) @@ -885,8 +888,10 @@ fn test_diff_copy_tracing() { assert_eq!( diff[1].clone(), ( - modified_path.to_owned(), - copied_path.to_owned(), + CopiesTreeDiffEntryPath { + source: modified_path.to_owned(), + target: copied_path.to_owned(), + }, ( Merge::resolved(before.path_value(modified_path).unwrap()), Merge::resolved(after.path_value(copied_path).unwrap()), @@ -896,8 +901,10 @@ fn test_diff_copy_tracing() { assert_eq!( diff[2].clone(), ( - removed_path.to_owned(), - added_path.to_owned(), + CopiesTreeDiffEntryPath { + source: removed_path.to_owned(), + target: added_path.to_owned(), + }, ( Merge::resolved(before.path_value(removed_path).unwrap()), Merge::resolved(after.path_value(added_path).unwrap())