Skip to content

Commit

Permalink
copies: extract (source, target) path pair to separate type
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@yuja
yuja committed Aug 23, 2024
1 parent 43bf195 commit 08262eb
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 102 deletions.
105 changes: 43 additions & 62 deletions cli/src/diff_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)) => {
Expand All @@ -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,
Expand Down Expand Up @@ -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!(
Expand All @@ -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"
Expand Down Expand Up @@ -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}")?,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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(())
Expand All @@ -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(())
}
Expand Down
42 changes: 15 additions & 27 deletions lib/src/conflicts.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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"<<<<<<<";
Expand Down Expand Up @@ -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)>,
}

Expand All @@ -353,31 +352,20 @@ pub fn materialized_diff_stream<'a>(
tree_diff: BoxStream<'a, CopiesTreeDiffEntry>,
) -> impl Stream<Item = MaterializedTreeDiffEntry> + '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))
}

Expand Down
35 changes: 29 additions & 6 deletions lib/src/copies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
Expand Down
21 changes: 14 additions & 7 deletions lib/tests/test_merged_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -867,15 +868,17 @@ fn test_diff_copy_tracing() {

let diff: Vec<_> = before_merged
.diff_stream_with_copies(&after_merged, &EverythingMatcher, &copy_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())
Expand All @@ -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()),
Expand All @@ -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())
Expand Down

0 comments on commit 08262eb

Please sign in to comment.