Skip to content

Commit

Permalink
copy-tracking: plumb CopyRecordMap through diff method
Browse files Browse the repository at this point in the history
  • Loading branch information
fowles committed Aug 11, 2024
1 parent 6bae5ea commit 34b0f87
Show file tree
Hide file tree
Showing 12 changed files with 75 additions and 40 deletions.
3 changes: 2 additions & 1 deletion cli/src/commands/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ pub(crate) fn cmd_fix(
// Also fix any new paths that were changed in this commit.
let tree = commit.tree()?;
let parent_tree = commit.parent_tree(tx.repo())?;
let mut diff_stream = parent_tree.diff_stream(&tree, &matcher);
let copy_records = Default::default();
let mut diff_stream = parent_tree.diff_stream(&tree, &matcher, &copy_records);
async {
while let Some(TreeDiffEntry {
source: _, // TODO handle copy tracking
Expand Down
7 changes: 5 additions & 2 deletions cli/src/commit_templater.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::io;
use std::rc::Rc;

use itertools::Itertools as _;
use jj_lib::backend::{BackendResult, ChangeId, CommitId};
use jj_lib::backend::{BackendResult, ChangeId, CommitId, CopyRecords};
use jj_lib::commit::Commit;
use jj_lib::extensions_map::ExtensionsMap;
use jj_lib::fileset::{self, FilesetExpression};
Expand Down Expand Up @@ -1278,6 +1278,7 @@ pub struct TreeDiff {
from_tree: MergedTree,
to_tree: MergedTree,
matcher: Rc<dyn Matcher>,
copy_records: CopyRecords,
}

impl TreeDiff {
Expand All @@ -1290,11 +1291,13 @@ impl TreeDiff {
from_tree: commit.parent_tree(repo)?,
to_tree: commit.tree()?,
matcher,
copy_records: Default::default(),
})
}

fn diff_stream(&self) -> TreeDiffStream<'_> {
self.from_tree.diff_stream(&self.to_tree, &*self.matcher)
self.from_tree
.diff_stream(&self.to_tree, &*self.matcher, &self.copy_records)
}

fn into_formatted<F, E>(self, show: F) -> TreeDiffFormatted<F>
Expand Down
26 changes: 18 additions & 8 deletions cli/src/diff_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ impl<'a> DiffRenderer<'a> {
}

/// Generates diff between `from_tree` and `to_tree`.
#[allow(clippy::too_many_arguments)]
pub fn show_diff(
&self,
ui: &Ui, // TODO: remove Ui dependency if possible
Expand All @@ -263,6 +264,7 @@ impl<'a> DiffRenderer<'a> {
})
}

#[allow(clippy::too_many_arguments)]
fn show_diff_inner(
&self,
ui: &Ui,
Expand All @@ -278,33 +280,41 @@ impl<'a> DiffRenderer<'a> {
for format in &self.formats {
match format {
DiffFormat::Summary => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_diff_summary(formatter, tree_diff, path_converter)?;
}
DiffFormat::Stat => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_diff_stat(formatter, store, tree_diff, path_converter, width)?;
}
DiffFormat::Types => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_types(formatter, tree_diff, path_converter)?;
}
DiffFormat::NameOnly => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_names(formatter, tree_diff, path_converter)?;
}
DiffFormat::Git { context } => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_git_diff(formatter, store, tree_diff, *context)?;
}
DiffFormat::ColorWords { context } => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff = from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_color_words_diff(formatter, store, tree_diff, path_converter, *context)?;
}
DiffFormat::Tool(tool) => {
match tool.diff_invocation_mode {
DiffToolMode::FileByFile => {
let tree_diff = from_tree.diff_stream(to_tree, matcher);
let no_copy_tracking = Default::default();
let tree_diff =
from_tree.diff_stream(to_tree, matcher, &no_copy_tracking);
show_file_by_file_diff(
ui,
formatter,
Expand Down Expand Up @@ -1052,7 +1062,7 @@ pub fn show_git_diff(
{
let path_string = path.as_internal_file_string();
let (left_value, right_value) = diff?;
let left_part = git_digf_part(&path, left_value)?;
let left_part = git_diff_part(&path, left_value)?;
let right_part = git_diff_part(&path, right_value)?;
formatter.with_label("file_header", |formatter| {
writeln!(formatter, "diff --git a/{path_string} b/{path_string}")?;
Expand Down
2 changes: 1 addition & 1 deletion cli/src/merge_tools/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ pub fn edit_diff_builtin(
) -> Result<MergedTreeId, BuiltinToolError> {
let store = left_tree.store().clone();
let changed_files: Vec<_> = left_tree
.diff_stream(right_tree, matcher)
.diff_stream(right_tree, matcher, &Default::default())
.map(
|TreeDiffEntry {
source: _, // TODO handle copy tracking
Expand Down
2 changes: 1 addition & 1 deletion cli/src/merge_tools/diff_working_copies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub(crate) fn check_out_trees(
output_is: Option<DiffSide>,
) -> Result<DiffWorkingCopies, DiffCheckoutError> {
let changed_files: Vec<_> = left_tree
.diff_stream(right_tree, matcher)
.diff_stream(right_tree, matcher, &Default::default())
.map(|TreeDiffEntry { target, .. }| target)
.collect()
.block_on();
Expand Down
6 changes: 4 additions & 2 deletions lib/src/default_index/revset_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,8 @@ fn has_diff_from_parent(
let from_tree =
rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?.resolve()?;
let to_tree = commit.tree()?;
let mut tree_diff = from_tree.diff_stream(&to_tree, matcher);
let copy_records = Default::default();
let mut tree_diff = from_tree.diff_stream(&to_tree, matcher, &copy_records);
async {
match tree_diff.next().await {
Some(TreeDiffEntry { value: Ok(_), .. }) => Ok(true),
Expand All @@ -1168,11 +1169,12 @@ fn matches_diff_from_parent(
text_pattern: &StringPattern,
files_matcher: &dyn Matcher,
) -> BackendResult<bool> {
let copy_records = Default::default();
let parents: Vec<_> = commit.parents().try_collect()?;
let from_tree =
rewrite::merge_commit_trees_no_resolve_without_repo(store, &index, &parents)?.resolve()?;
let to_tree = commit.tree()?;
let tree_diff = from_tree.diff_stream(&to_tree, files_matcher);
let tree_diff = from_tree.diff_stream(&to_tree, files_matcher, &copy_records);
// Conflicts are compared in materialized form. Alternatively, conflict
// pairs can be compared one by one. #4062
let mut diff_stream = materialized_diff_stream(store, tree_diff);
Expand Down
6 changes: 4 additions & 2 deletions lib/src/local_working_copy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1340,6 +1340,7 @@ impl TreeState {
new_tree: &MergedTree,
matcher: &dyn Matcher,
) -> Result<CheckoutStats, CheckoutError> {
let copy_records = Default::default();
// TODO: maybe it's better not include the skipped counts in the "intended"
// counts
let mut stats = CheckoutStats {
Expand All @@ -1352,7 +1353,7 @@ impl TreeState {
let mut deleted_files = HashSet::new();
let mut diff_stream = Box::pin(
old_tree
.diff_stream(new_tree, matcher)
.diff_stream(new_tree, matcher, &copy_records)
.map(
|TreeDiffEntry {
source: _, // TODO handle copy tracking
Expand Down Expand Up @@ -1451,9 +1452,10 @@ impl TreeState {
})?;

let matcher = self.sparse_matcher();
let copy_records = Default::default();
let mut changed_file_states = Vec::new();
let mut deleted_files = HashSet::new();
let mut diff_stream = old_tree.diff_stream(new_tree, matcher.as_ref());
let mut diff_stream = old_tree.diff_stream(new_tree, matcher.as_ref(), &copy_records);
while let Some(TreeDiffEntry {
source: _, // TODO handle copy tracking
target: path,
Expand Down
13 changes: 11 additions & 2 deletions lib/src/merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use futures::{Stream, TryStreamExt};
use itertools::{EitherOrBoth, Itertools};

use crate::backend;
use crate::backend::{BackendResult, MergedTreeId, TreeId, TreeValue};
use crate::backend::{BackendResult, CopyRecords, MergedTreeId, TreeId, TreeValue};
use crate::matchers::{EverythingMatcher, Matcher};
use crate::merge::{Merge, MergeBuilder, MergedTreeValue};
use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent};
Expand Down Expand Up @@ -283,19 +283,22 @@ impl MergedTree {
&self,
other: &MergedTree,
matcher: &'matcher dyn Matcher,
copy_records: &'matcher CopyRecords,
) -> TreeDiffStream<'matcher> {
let concurrency = self.store().concurrency();
if concurrency <= 1 {
Box::pin(futures::stream::iter(TreeDiffIterator::new(
&self.trees,
&other.trees,
matcher,
copy_records,
)))
} else {
Box::pin(TreeDiffStreamImpl::new(
self.clone(),
other.clone(),
matcher,
copy_records,
concurrency,
))
}
Expand Down Expand Up @@ -627,7 +630,12 @@ enum TreeDiffItem {
impl<'matcher> TreeDiffIterator<'matcher> {
/// Creates a iterator over the differences between two trees. Generally
/// prefer `MergedTree::diff()` of calling this directly.
pub fn new(trees1: &Merge<Tree>, trees2: &Merge<Tree>, matcher: &'matcher dyn Matcher) -> Self {
pub fn new(
trees1: &Merge<Tree>,
trees2: &Merge<Tree>,
matcher: &'matcher dyn Matcher,
_copy_records: &'matcher CopyRecords,
) -> Self {
assert!(Arc::ptr_eq(trees1.first().store(), trees2.first().store()));
let root_dir = RepoPath::root();
let mut stack = Vec::new();
Expand Down Expand Up @@ -857,6 +865,7 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> {
tree1: MergedTree,
tree2: MergedTree,
matcher: &'matcher dyn Matcher,
_copy_records: &'matcher CopyRecords,
max_concurrent_reads: usize,
) -> Self {
let mut stream = Self {
Expand Down
3 changes: 2 additions & 1 deletion lib/src/rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ pub fn restore_tree(
// TODO: We should be able to not traverse deeper in the diff if the matcher
// matches an entire subtree.
let mut tree_builder = MergedTreeBuilder::new(destination.id().clone());
let copy_records = Default::default();
async {
let mut diff_stream = source.diff_stream(destination, matcher);
let mut diff_stream = source.diff_stream(destination, matcher, &copy_records);
while let Some(TreeDiffEntry {
source: _, // TODO handle copy tracking
target: repo_path,
Expand Down
2 changes: 1 addition & 1 deletion lib/tests/test_commit_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use testutils::{assert_rebased_onto, create_tree, CommitGraphBuilder, TestRepo,

fn diff_paths(from_tree: &MergedTree, to_tree: &MergedTree) -> Vec<RepoPathBuf> {
from_tree
.diff_stream(to_tree, &EverythingMatcher)
.diff_stream(to_tree, &EverythingMatcher, &Default::default())
.map(|diff| {
let _ = diff.value.unwrap();
diff.target
Expand Down
4 changes: 2 additions & 2 deletions lib/tests/test_local_working_copy_sparse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ fn test_sparse_commit() {
// tree.
let modified_tree = test_workspace.snapshot().unwrap();
let diff: Vec<_> = tree
.diff_stream(&modified_tree, &EverythingMatcher)
.diff_stream(&modified_tree, &EverythingMatcher, &Default::default())
.collect()
.block_on();
assert_eq!(diff.len(), 1);
Expand All @@ -219,7 +219,7 @@ fn test_sparse_commit() {
// updated in the tree.
let modified_tree = test_workspace.snapshot().unwrap();
let diff: Vec<_> = tree
.diff_stream(&modified_tree, &EverythingMatcher)
.diff_stream(&modified_tree, &EverythingMatcher, &Default::default())
.collect()
.block_on();
assert_eq!(diff.len(), 2);
Expand Down
41 changes: 24 additions & 17 deletions lib/tests/test_merged_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,22 @@ fn diff_entry_tuple(diff: TreeDiffEntry) -> (RepoPathBuf, (MergedTreeValue, Merg
}

fn diff_stream_equals_iter(tree1: &MergedTree, tree2: &MergedTree, matcher: &dyn Matcher) {
let iter_diff: Vec<_> = TreeDiffIterator::new(tree1.as_merge(), tree2.as_merge(), matcher)
.map(diff_entry_tuple)
.collect();
let max_concurrent_reads = 10;
let stream_diff: Vec<_> =
TreeDiffStreamImpl::new(tree1.clone(), tree2.clone(), matcher, max_concurrent_reads)
let copy_records = Default::default();
let iter_diff: Vec<_> =
TreeDiffIterator::new(tree1.as_merge(), tree2.as_merge(), matcher, &copy_records)
.map(diff_entry_tuple)
.collect()
.block_on();
.collect();
let max_concurrent_reads = 10;
let stream_diff: Vec<_> = TreeDiffStreamImpl::new(
tree1.clone(),
tree2.clone(),
matcher,
&copy_records,
max_concurrent_reads,
)
.map(diff_entry_tuple)
.collect()
.block_on();
assert_eq!(stream_diff, iter_diff);
}

Expand Down Expand Up @@ -752,7 +759,7 @@ fn test_diff_resolved() {
let after_merged = MergedTree::new(Merge::resolved(after.clone()));

let diff: Vec<_> = before_merged
.diff_stream(&after_merged, &EverythingMatcher)
.diff_stream(&after_merged, &EverythingMatcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand Down Expand Up @@ -862,7 +869,7 @@ fn test_diff_conflicted() {

// Test the forwards diff
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &EverythingMatcher)
.diff_stream(&right_merged, &EverythingMatcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand All @@ -882,7 +889,7 @@ fn test_diff_conflicted() {
diff_stream_equals_iter(&left_merged, &right_merged, &EverythingMatcher);
// Test the reverse diff
let actual_diff: Vec<_> = right_merged
.diff_stream(&left_merged, &EverythingMatcher)
.diff_stream(&left_merged, &EverythingMatcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand Down Expand Up @@ -1000,7 +1007,7 @@ fn test_diff_dir_file() {
// Test the forwards diff
{
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &EverythingMatcher)
.diff_stream(&right_merged, &EverythingMatcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand Down Expand Up @@ -1045,7 +1052,7 @@ fn test_diff_dir_file() {
// Test the reverse diff
{
let actual_diff: Vec<_> = right_merged
.diff_stream(&left_merged, &EverythingMatcher)
.diff_stream(&left_merged, &EverythingMatcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand Down Expand Up @@ -1091,7 +1098,7 @@ fn test_diff_dir_file() {
{
let matcher = FilesMatcher::new([&path1]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.diff_stream(&right_merged, &matcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand All @@ -1107,7 +1114,7 @@ fn test_diff_dir_file() {
{
let matcher = FilesMatcher::new([path1.join(file)]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.diff_stream(&right_merged, &matcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand All @@ -1126,7 +1133,7 @@ fn test_diff_dir_file() {
{
let matcher = PrefixMatcher::new([&path1]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.diff_stream(&right_merged, &matcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand All @@ -1148,7 +1155,7 @@ fn test_diff_dir_file() {
{
let matcher = FilesMatcher::new([&path6]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.diff_stream(&right_merged, &matcher, &Default::default())
.map(diff_entry_tuple)
.collect()
.block_on();
Expand Down

0 comments on commit 34b0f87

Please sign in to comment.