ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

copy-tracking: create an explicit TreeDiffEntry struct

Browse source
This commit is contained in:
Matt Kulukundis 2024-07-25 14:57:43 -04:00 committed by Matt Fowles Kulukundis
parent ee6b922144
commit 8e84c60157
13 changed files with 163 additions and 85 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
cli/src/commands/diff.rs
View file

@ -14,14 +14,11 @@
use itertools::Itertools;
use jj_lib::backend::CopyRecords;
use jj_lib::commit::Commit;
use jj_lib::repo::Repo;
use jj_lib::rewrite::merge_commit_trees;
use tracing::instrument;
use crate::cli_util::{
print_unmatched_explicit_paths, CommandHelper, RevisionArg, WorkspaceCommandHelper,
};
use crate::cli_util::{print_unmatched_explicit_paths, CommandHelper, RevisionArg};
use crate::command_error::CommandError;
use crate::diff_util::DiffFormatArgs;
use crate::ui::Ui;

8
cli/src/commands/fix.rs
View file

@ -22,7 +22,7 @@ use itertools::Itertools;
use jj_lib::backend::{BackendError, CommitId, FileId, TreeValue};
use jj_lib::fileset::{self, FilesetExpression};
use jj_lib::matchers::{EverythingMatcher, Matcher};
use jj_lib::merged_tree::MergedTreeBuilder;
use jj_lib::merged_tree::{MergedTreeBuilder, TreeDiffEntry};
use jj_lib::repo::Repo;
use jj_lib::repo_path::{RepoPathBuf, RepoPathUiConverter};
use jj_lib::revset::{RevsetExpression, RevsetIteratorExt};
@ -174,7 +174,11 @@ pub(crate) fn cmd_fix(
let parent_tree = commit.parent_tree(tx.repo())?;
let mut diff_stream = parent_tree.diff_stream(&tree, &matcher);
async {
while let Some((repo_path, diff)) = diff_stream.next().await {
while let Some(TreeDiffEntry {
target: repo_path,
value: diff,
}) = diff_stream.next().await
{
let (_before, after) = diff?;
// Deleted files have no file content to fix, and they have no terms in `after`,
// so we don't add any tool inputs for them. Conflicted files produce one tool

19
cli/src/diff_util.rs
View file

@ -27,7 +27,7 @@ use jj_lib::diff::{Diff, DiffHunk};
use jj_lib::files::DiffLine;
use jj_lib::matchers::Matcher;
use jj_lib::merge::MergedTreeValue;
use jj_lib::merged_tree::{MergedTree, TreeDiffStream};
use jj_lib::merged_tree::{MergedTree, TreeDiffEntry, TreeDiffStream};
use jj_lib::object_id::ObjectId;
use jj_lib::repo::Repo;
use jj_lib::repo_path::{RepoPath, RepoPathUiConverter};
@ -1110,7 +1110,11 @@ pub fn show_diff_summary(
path_converter: &RepoPathUiConverter,
) -> io::Result<()> {
async {
while let Some((repo_path, diff)) = tree_diff.next().await {
while let Some(TreeDiffEntry {
target: repo_path,
value: diff,
}) = tree_diff.next().await
{
let (before, after) = diff.unwrap();
let ui_path = path_converter.format_file_path(&repo_path);
if before.is_present() && after.is_present() {
@ -1241,7 +1245,11 @@ pub fn show_types(
path_converter: &RepoPathUiConverter,
) -> io::Result<()> {
async {
while let Some((repo_path, diff)) = tree_diff.next().await {
while let Some(TreeDiffEntry {
target: repo_path,
value: diff,
}) = tree_diff.next().await
{
let (before, after) = diff.unwrap();
writeln!(
formatter.labeled("modified"),
@ -1275,7 +1283,10 @@ pub fn show_names(
path_converter: &RepoPathUiConverter,
) -> io::Result<()> {
async {
while let Some((repo_path, _)) = tree_diff.next().await {
while let Some(TreeDiffEntry {
target: repo_path, ..
}) = tree_diff.next().await
{
writeln!(formatter, "{}", path_converter.format_file_path(&repo_path))?;
}
Ok(())

9
cli/src/merge_tools/builtin.rs
View file

@ -10,7 +10,7 @@ use jj_lib::diff::{Diff, DiffHunk};
use jj_lib::files::{self, ContentHunk, MergeResult};
use jj_lib::matchers::Matcher;
use jj_lib::merge::Merge;
use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder};
use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder, TreeDiffEntry};
use jj_lib::object_id::ObjectId;
use jj_lib::repo_path::{RepoPath, RepoPathBuf};
use jj_lib::store::Store;
@ -496,7 +496,12 @@ pub fn edit_diff_builtin(
let store = left_tree.store().clone();
let changed_files: Vec<_> = left_tree
.diff_stream(right_tree, matcher)
.map(|(path, diff)| diff.map(|_| path))
.map(
|TreeDiffEntry {
target: path,
value: diff,
}| diff.map(|_| path),
)
.try_collect()
.block_on()?;
let files = make_diff_files(&store, left_tree, right_tree, &changed_files)?;

4
cli/src/merge_tools/diff_working_copies.rs
View file

@ -10,7 +10,7 @@ use jj_lib::fsmonitor::FsmonitorSettings;
use jj_lib::gitignore::GitIgnoreFile;
use jj_lib::local_working_copy::{TreeState, TreeStateError};
use jj_lib::matchers::Matcher;
use jj_lib::merged_tree::MergedTree;
use jj_lib::merged_tree::{MergedTree, TreeDiffEntry};
use jj_lib::repo_path::RepoPathBuf;
use jj_lib::store::Store;
use jj_lib::working_copy::{CheckoutError, SnapshotOptions};
@ -132,7 +132,7 @@ pub(crate) fn check_out_trees(
) -> Result<DiffWorkingCopies, DiffCheckoutError> {
let changed_files: Vec<_> = left_tree
.diff_stream(right_tree, matcher)
.map(|(path, _diff)| path)
.map(|TreeDiffEntry { target, .. }| target)
.collect()
.block_on();

27
lib/src/conflicts.rs
View file

@ -26,7 +26,7 @@ use crate::diff::{Diff, DiffHunk};
use crate::files;
use crate::files::{ContentHunk, MergeResult};
use crate::merge::{Merge, MergeBuilder, MergedTreeValue};
use crate::merged_tree::TreeDiffStream;
use crate::merged_tree::{TreeDiffEntry, TreeDiffStream};
use crate::repo_path::{RepoPath, RepoPathBuf};
use crate::store::Store;
@ -335,17 +335,22 @@ pub fn materialized_diff_stream<'a>(
),
> + 'a {
tree_diff
.map(|(path, diff)| async {
match diff {
Err(err) => (path, Err(err)),
Ok((before, after)) => {
let before_future = materialize_tree_value(store, &path, before);
let after_future = materialize_tree_value(store, &path, after);
let values = try_join!(before_future, after_future);
(path, values)
.map(
|TreeDiffEntry {
target: path,
value: diff,
}| async {
match diff {
Err(err) => (path, Err(err)),
Ok((before, after)) => {
let before_future = materialize_tree_value(store, &path, before);
let after_future = materialize_tree_value(store, &path, after);
let values = try_join!(before_future, after_future);
(path, values)
}
}
}
})
},
)
.buffered((store.concurrency() / 2).max(1))
}

7
lib/src/default_index/revset_engine.rs
View file

@ -34,6 +34,7 @@ use crate::conflicts::{materialized_diff_stream, MaterializedTreeValue};
use crate::default_index::{AsCompositeIndex, CompositeIndex, IndexPosition};
use crate::graph::GraphEdge;
use crate::matchers::{Matcher, Visit};
use crate::merged_tree::TreeDiffEntry;
use crate::repo_path::RepoPath;
use crate::revset::{
ResolvedExpression, ResolvedPredicateExpression, Revset, RevsetEvaluationError,
@ -1148,8 +1149,10 @@ fn has_diff_from_parent(
let mut tree_diff = from_tree.diff_stream(&to_tree, matcher);
async {
match tree_diff.next().await {
Some((_, Ok(_))) => Ok(true),
Some((_, Err(err))) => Err(err),
Some(TreeDiffEntry { value: Ok(_), .. }) => Ok(true),
Some(TreeDiffEntry {
value: Err(err), ..
}) => Err(err),
None => Ok(false),
}
}

30
lib/src/local_working_copy.rs
View file

@ -56,7 +56,7 @@ use crate::matchers::{
DifferenceMatcher, EverythingMatcher, FilesMatcher, IntersectionMatcher, Matcher, PrefixMatcher,
};
use crate::merge::{Merge, MergeBuilder, MergedTreeValue};
use crate::merged_tree::{MergedTree, MergedTreeBuilder};
use crate::merged_tree::{MergedTree, MergedTreeBuilder, TreeDiffEntry};
use crate::object_id::ObjectId;
use crate::op_store::{OperationId, WorkspaceId};
use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent};
@ -1353,15 +1353,21 @@ impl TreeState {
let mut diff_stream = Box::pin(
old_tree
.diff_stream(new_tree, matcher)
.map(|(path, diff)| async {
match diff {
Ok((before, after)) => {
let result = materialize_tree_value(&self.store, &path, after).await;
(path, result.map(|value| (before.is_present(), value)))
.map(
|TreeDiffEntry {
target: path,
value: diff,
}| async {
match diff {
Ok((before, after)) => {
let result =
materialize_tree_value(&self.store, &path, after).await;
(path, result.map(|value| (before.is_present(), value)))
}
Err(err) => (path, Err(err)),
}
Err(err) => (path, Err(err)),
}
})
},
)
.buffered(self.store.concurrency().max(1)),
);
while let Some((path, data)) = diff_stream.next().await {
@ -1447,7 +1453,11 @@ impl TreeState {
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());
while let Some((path, diff)) = diff_stream.next().await {
while let Some(TreeDiffEntry {
target: path,
value: diff,
}) = diff_stream.next().await
{
let (_before, after) = diff?;
if after.is_absent() {
deleted_files.insert(path);

88
lib/src/merged_tree.rs
View file

@ -321,16 +321,19 @@ impl MergedTree {
}
}
/// A single entry in a tree diff.
pub struct TreeDiffEntry {
// pub source: RepoPathBuf,
/// The target path.
pub target: RepoPathBuf,
/// The resolved tree values if available.
pub value: BackendResult<(MergedTreeValue, MergedTreeValue)>,
}
/// Type alias for the result from `MergedTree::diff_stream()`. We use a
/// `Stream` instead of an `Iterator` so high-latency backends (e.g. cloud-based
/// ones) can fetch trees asynchronously.
pub type TreeDiffStream<'matcher> = BoxStream<
'matcher,
(
RepoPathBuf,
BackendResult<(MergedTreeValue, MergedTreeValue)>,
),
>;
pub type TreeDiffStream<'matcher> = BoxStream<'matcher, TreeDiffEntry>;
fn all_tree_basenames(trees: &Merge<Tree>) -> impl Iterator<Item = &RepoPathComponent> {
trees
@ -694,10 +697,7 @@ impl TreeDiffDirItem {
}
impl Iterator for TreeDiffIterator<'_> {
type Item = (
RepoPathBuf,
BackendResult<(MergedTreeValue, MergedTreeValue)>,
);
type Item = TreeDiffEntry;
fn next(&mut self) -> Option<Self::Item> {
while let Some(top) = self.stack.last_mut() {
@ -711,7 +711,10 @@ impl Iterator for TreeDiffIterator<'_> {
},
TreeDiffItem::File(..) => {
if let TreeDiffItem::File(path, before, after) = self.stack.pop().unwrap() {
return Some((path, Ok((before, after))));
return Some(TreeDiffEntry {
target: path,
value: Ok((before, after)),
});
} else {
unreachable!();
}
@ -721,13 +724,23 @@ impl Iterator for TreeDiffIterator<'_> {
let tree_before = before.is_tree();
let tree_after = after.is_tree();
let post_subdir = if tree_before || tree_after {
let before_tree = match Self::trees(&self.store, &path, &before) {
Ok(tree) => tree,
Err(err) => return Some((path, Err(err))),
};
let after_tree = match Self::trees(&self.store, &path, &after) {
Ok(tree) => tree,
Err(err) => return Some((path, Err(err))),
let (before_tree, after_tree) = match (
Self::trees(&self.store, &path, &before),
Self::trees(&self.store, &path, &after),
) {
(Ok(before_tree), Ok(after_tree)) => (before_tree, after_tree),
(Err(before_err), _) => {
return Some(TreeDiffEntry {
target: path,
value: Err(before_err),
})
}
(_, Err(after_err)) => {
return Some(TreeDiffEntry {
target: path,
value: Err(after_err),
})
}
};
let subdir =
TreeDiffDirItem::from_trees(&path, &before_tree, &after_tree, self.matcher);
@ -738,7 +751,10 @@ impl Iterator for TreeDiffIterator<'_> {
};
if !tree_before && tree_after {
if before.is_present() {
return Some((path, Ok((before, Merge::absent()))));
return Some(TreeDiffEntry {
target: path,
value: Ok((before, Merge::absent())),
});
}
} else if tree_before && !tree_after {
if after.is_present() {
@ -748,7 +764,10 @@ impl Iterator for TreeDiffIterator<'_> {
);
}
} else if !tree_before && !tree_after {
return Some((path, Ok((before, after))));
return Some(TreeDiffEntry {
target: path,
value: Ok((before, after)),
});
}
}
None
@ -966,10 +985,7 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> {
}
impl Stream for TreeDiffStreamImpl<'_> {
type Item = (
RepoPathBuf,
BackendResult<(MergedTreeValue, MergedTreeValue)>,
);
type Item = TreeDiffEntry;
fn poll_next(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
// Go through all pending tree futures and poll them.
@ -981,12 +997,30 @@ impl Stream for TreeDiffStreamImpl<'_> {
Err(_) => {
// File or tree with error
let (key, result) = entry.remove_entry();
Poll::Ready(Some((key.path, result)))
Poll::Ready(Some(match result {
Err(err) => TreeDiffEntry {
target: key.path,
value: Err(err),
},
Ok((before, after)) => TreeDiffEntry {
target: key.path,
value: Ok((before, after)),
},
}))
}
Ok((before, after)) if !before.is_tree() && !after.is_tree() => {
// A diff with no trees involved
let (key, result) = entry.remove_entry();
Poll::Ready(Some((key.path, result)))
Poll::Ready(Some(match result {
Err(err) => TreeDiffEntry {
target: key.path,
value: Err(err),
},
Ok((before, after)) => TreeDiffEntry {
target: key.path,
value: Ok((before, after)),
},
}))
}
_ => {
// The first entry has a tree on at least one side (before or after). We need to

8
lib/src/rewrite.rs
View file

@ -27,7 +27,7 @@ use crate::commit::Commit;
use crate::commit_builder::CommitBuilder;
use crate::index::Index;
use crate::matchers::{Matcher, Visit};
use crate::merged_tree::{MergedTree, MergedTreeBuilder};
use crate::merged_tree::{MergedTree, MergedTreeBuilder, TreeDiffEntry};
use crate::repo::{MutableRepo, Repo};
use crate::repo_path::RepoPath;
use crate::settings::UserSettings;
@ -88,7 +88,11 @@ pub fn restore_tree(
let mut tree_builder = MergedTreeBuilder::new(destination.id().clone());
async {
let mut diff_stream = source.diff_stream(destination, matcher);
while let Some((repo_path, diff)) = diff_stream.next().await {
while let Some(TreeDiffEntry {
target: repo_path,
value: diff,
}) = diff_stream.next().await
{
let (source_value, _destination_value) = diff?;
tree_builder.set_or_remove(repo_path, source_value);
}

6
lib/tests/test_commit_builder.rs
View file

@ -27,9 +27,9 @@ 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)
.map(|(path, diff)| {
let _ = diff.unwrap();
path
.map(|diff| {
let _ = diff.value.unwrap();
diff.target
})
.collect()
.block_on()

6
lib/tests/test_local_working_copy_sparse.rs
View file

@ -201,7 +201,7 @@ fn test_sparse_commit() {
.collect()
.block_on();
assert_eq!(diff.len(), 1);
assert_eq!(diff[0].0.as_ref(), dir1_file1_path);
assert_eq!(diff[0].target.as_ref(), dir1_file1_path);
// Set sparse patterns to also include dir2/
let mut locked_ws = test_workspace
@ -223,8 +223,8 @@ fn test_sparse_commit() {
.collect()
.block_on();
assert_eq!(diff.len(), 2);
assert_eq!(diff[0].0.as_ref(), dir1_file1_path);
assert_eq!(diff[1].0.as_ref(), dir2_file1_path);
assert_eq!(diff[0].target.as_ref(), dir1_file1_path);
assert_eq!(diff[1].target.as_ref(), dir2_file1_path);
}
#[test]

31
lib/tests/test_merged_tree.rs
View file

@ -17,9 +17,10 @@ use itertools::Itertools;
use jj_lib::backend::{FileId, MergedTreeId, TreeValue};
use jj_lib::files::MergeResult;
use jj_lib::matchers::{EverythingMatcher, FilesMatcher, Matcher, PrefixMatcher};
use jj_lib::merge::{Merge, MergeBuilder};
use jj_lib::merge::{Merge, MergeBuilder, MergedTreeValue};
use jj_lib::merged_tree::{
MergedTree, MergedTreeBuilder, MergedTreeVal, TreeDiffIterator, TreeDiffStreamImpl,
MergedTree, MergedTreeBuilder, MergedTreeVal, TreeDiffEntry, TreeDiffIterator,
TreeDiffStreamImpl,
};
use jj_lib::repo::Repo;
use jj_lib::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent};
@ -34,14 +35,18 @@ fn file_value(file_id: &FileId) -> TreeValue {
}
}
fn diff_entry_tuple(diff: TreeDiffEntry) -> (RepoPathBuf, (MergedTreeValue, MergedTreeValue)) {
(diff.target, diff.value.unwrap())
}
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(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect();
let max_concurrent_reads = 10;
let stream_diff: Vec<_> =
TreeDiffStreamImpl::new(tree1.clone(), tree2.clone(), matcher, max_concurrent_reads)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
assert_eq!(stream_diff, iter_diff);
@ -748,7 +753,7 @@ fn test_diff_resolved() {
let diff: Vec<_> = before_merged
.diff_stream(&after_merged, &EverythingMatcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
assert_eq!(diff.len(), 3);
@ -858,7 +863,7 @@ fn test_diff_conflicted() {
// Test the forwards diff
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &EverythingMatcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = [path2, path3, path4]
@ -878,7 +883,7 @@ fn test_diff_conflicted() {
// Test the reverse diff
let actual_diff: Vec<_> = right_merged
.diff_stream(&left_merged, &EverythingMatcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = [path2, path3, path4]
@ -996,7 +1001,7 @@ fn test_diff_dir_file() {
{
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &EverythingMatcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = vec![
@ -1041,7 +1046,7 @@ fn test_diff_dir_file() {
{
let actual_diff: Vec<_> = right_merged
.diff_stream(&left_merged, &EverythingMatcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = vec![
@ -1087,7 +1092,7 @@ fn test_diff_dir_file() {
let matcher = FilesMatcher::new([&path1]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = vec![
@ -1103,7 +1108,7 @@ fn test_diff_dir_file() {
let matcher = FilesMatcher::new([path1.join(file)]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = vec![
@ -1122,7 +1127,7 @@ fn test_diff_dir_file() {
let matcher = PrefixMatcher::new([&path1]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = vec![
@ -1144,7 +1149,7 @@ fn test_diff_dir_file() {
let matcher = FilesMatcher::new([&path6]);
let actual_diff: Vec<_> = left_merged
.diff_stream(&right_merged, &matcher)
.map(|(path, diff)| (path, diff.unwrap()))
.map(diff_entry_tuple)
.collect()
.block_on();
let expected_diff = vec![(path6.to_owned(), (Merge::absent(), right_value(path6)))];