Skip to content

Commit

Permalink
copies: determine copy/rename operation by CopiesTreeDiffStream
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
yuja committed Aug 23, 2024
1 parent b6060ce commit f5187fa
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 23 deletions.
22 changes: 11 additions & 11 deletions cli/src/diff_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}")?;
Expand Down Expand Up @@ -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()) {
Expand Down
55 changes: 45 additions & 10 deletions lib/src/copies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -98,31 +107,38 @@ 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<RepoPathBuf>,
/// The source path and copy information if this is a copy or rename.
pub source: Option<(RepoPathBuf, CopyOperation)>,
/// The target path.
pub target: RepoPathBuf,
}

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<CopyOperation> {
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,
}

Expand All @@ -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<'_> {
Expand All @@ -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,
}));
}

Expand Down
1 change: 1 addition & 0 deletions lib/src/merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ impl MergedTree {
Box::pin(CopiesTreeDiffStream::new(
stream,
self.clone(),
other.clone(),
copy_records,
))
}
Expand Down
5 changes: 3 additions & 2 deletions lib/tests/test_merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
},
(
Expand All @@ -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(),
},
(
Expand Down

0 comments on commit f5187fa

Please sign in to comment.