Skip to content

Commit

Permalink
merged_tree: replace MergedTreeVal<'_> by Merge<Option<&TreeValue>>
Browse files Browse the repository at this point in the history 
MergedTreeVal was roughly equivalent to Merge<Option<Cow<_>>. As we've dropped
support for the legacy trees, it can be simplified to Merge<Option<&_>>.
  • Loading branch information
@yuja
yuja committed Aug 12, 2024
1 parent 2977900 commit f7377fb
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 78 deletions.
3 changes: 3 additions & 0 deletions lib/src/merge.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,9 @@ impl<T: ContentHash> ContentHash for Merge<T> {
}
}

/// Borrowed `MergedTreeValue`.
pub type MergedTreeVal<'a> = Merge<Option<&'a TreeValue>>;

/// The value at a given path in a commit. It depends on the context whether it
/// can be absent (`Merge::is_absent()`). For example, when getting the value at
/// a specific path, it may be, but when iterating over entries in a tree, it
Expand Down
87 changes: 30 additions & 57 deletions lib/src/merged_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use itertools::{EitherOrBoth, Itertools};
use crate::backend;
use crate::backend::{BackendResult, CopyRecord, CopyRecords, MergedTreeId, TreeId, TreeValue};
use crate::matchers::{EverythingMatcher, Matcher};
use crate::merge::{Merge, MergeBuilder, MergedTreeValue};
use crate::merge::{Merge, MergeBuilder, MergedTreeVal, MergedTreeValue};
use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent};
use crate::store::Store;
use crate::tree::{try_resolve_file_conflict, Tree};
Expand All @@ -43,26 +43,6 @@ pub struct MergedTree {
trees: Merge<Tree>,
}

/// The value at a given path in a `MergedTree`.
#[derive(PartialEq, Eq, Hash, Clone, Debug)]
pub enum MergedTreeVal<'a> {
/// A single non-conflicted value.
Resolved(Option<&'a TreeValue>),
/// TODO: Make this a `Merge<Option<&'a TreeValue>>` (reference to the
/// value) once we have removed the `MergedTree::Legacy` variant.
Conflict(MergedTreeValue),
}

impl MergedTreeVal<'_> {
/// Converts to an owned value.
pub fn to_merge(&self) -> MergedTreeValue {
match self {
MergedTreeVal::Resolved(value) => Merge::resolved(value.cloned()),
MergedTreeVal::Conflict(merge) => merge.clone(),
}
}
}

impl MergedTree {
/// Creates a new `MergedTree` representing a single tree without conflicts.
pub fn resolved(tree: Tree) -> Self {
Expand Down Expand Up @@ -193,15 +173,15 @@ impl MergedTree {
/// doesn't correspond to a tree in any of the inputs to the merge, then
/// that entry will be replace by an empty tree in the result.
pub fn sub_tree(&self, name: &RepoPathComponent) -> BackendResult<Option<MergedTree>> {
match self.value(name) {
MergedTreeVal::Resolved(Some(TreeValue::Tree(sub_tree_id))) => {
match self.value(name).into_resolved() {
Ok(Some(TreeValue::Tree(sub_tree_id))) => {
let subdir = self.dir().join(name);
Ok(Some(MergedTree::resolved(
self.store().get_tree(&subdir, sub_tree_id)?,
)))
}
MergedTreeVal::Resolved(_) => Ok(None),
MergedTreeVal::Conflict(merge) => {
Ok(_) => Ok(None),
Err(merge) => {
let trees = merge.try_map(|value| match value {
Some(TreeValue::Tree(sub_tree_id)) => {
let subdir = self.dir().join(name);
Expand All @@ -225,7 +205,7 @@ impl MergedTree {
match path.split() {
Some((dir, basename)) => match self.sub_tree_recursive(dir)? {
None => Ok(Merge::absent()),
Some(tree) => Ok(tree.value(basename).to_merge()),
Some(tree) => Ok(tree.value(basename).cloned()),
},
None => Ok(self
.trees
Expand Down Expand Up @@ -375,15 +355,16 @@ fn all_tree_entries(
if let Some(tree) = trees.as_resolved() {
let iter = tree
.entries_non_recursive()
.map(|entry| (entry.name(), MergedTreeVal::Resolved(Some(entry.value()))));
.map(|entry| (entry.name(), Merge::normal(entry.value())));
Either::Left(iter)
} else {
let iter = all_merged_tree_entries(trees).map(|(name, values)| {
let value = match values.resolve_trivial() {
Some(resolved) => MergedTreeVal::Resolved(*resolved),
None => MergedTreeVal::Conflict(values.cloned()),
// TODO: move resolve_trivial() to caller?
let values = match values.resolve_trivial() {
Some(resolved) => Merge::resolved(*resolved),
None => values,
};
(name, value)
(name, values)
});
Either::Right(iter)
}
Expand All @@ -394,7 +375,7 @@ fn all_tree_entries(
/// than `tree.entries_non_recursive()`.
fn all_merged_tree_entries(
trees: &Merge<Tree>,
) -> impl Iterator<Item = (&RepoPathComponent, Merge<Option<&TreeValue>>)> {
) -> impl Iterator<Item = (&RepoPathComponent, MergedTreeVal<'_>)> {
let mut entries_iters = trees
.iter()
.map(|tree| tree.entries_non_recursive().peekable())
Expand Down Expand Up @@ -427,21 +408,21 @@ fn merged_tree_entry_diff<'a>(
)
.map(|entry| match entry {
EitherOrBoth::Both((name, value1), (_, value2)) => (name, value1, value2),
EitherOrBoth::Left((name, value1)) => (name, value1, MergedTreeVal::Resolved(None)),
EitherOrBoth::Right((name, value2)) => (name, MergedTreeVal::Resolved(None), value2),
EitherOrBoth::Left((name, value1)) => (name, value1, Merge::absent()),
EitherOrBoth::Right((name, value2)) => (name, Merge::absent(), value2),
})
.filter(|(_, value1, value2)| value1 != value2)
}

fn trees_value<'a>(trees: &'a Merge<Tree>, basename: &RepoPathComponent) -> MergedTreeVal<'a> {
if let Some(tree) = trees.as_resolved() {
return MergedTreeVal::Resolved(tree.value(basename));
return Merge::resolved(tree.value(basename));
}
let value = trees.map(|tree| tree.value(basename));
if let Some(resolved) = value.resolve_trivial() {
return MergedTreeVal::Resolved(*resolved);
return Merge::resolved(*resolved);
}
MergedTreeVal::Conflict(value.cloned())
value
}

/// The returned conflict will either be resolved or have the same number of
Expand Down Expand Up @@ -498,7 +479,7 @@ fn merge_trees(merge: &Merge<Tree>) -> BackendResult<Merge<Tree>> {
fn merge_tree_values(
store: &Arc<Store>,
path: &RepoPath,
values: &Merge<Option<&TreeValue>>,
values: &MergedTreeVal,
) -> BackendResult<MergedTreeValue> {
if let Some(resolved) = values.resolve_trivial() {
return Ok(Merge::resolved(resolved.cloned()));
Expand Down Expand Up @@ -544,7 +525,6 @@ impl TreeEntriesDirItem {
let dir = trees.first().dir();
for (name, value) in all_tree_entries(trees) {
let path = dir.join(name);
let value = value.to_merge();
if value.is_tree() {
// TODO: Handle the other cases (specific files and trees)
if matcher.visit(&path).is_nothing() {
Expand All @@ -553,7 +533,7 @@ impl TreeEntriesDirItem {
} else if !matcher.matches(&path) {
continue;
}
entries.push((path, value));
entries.push((path, value.cloned()));
}
entries.reverse();
TreeEntriesDirItem { entries }
Expand Down Expand Up @@ -609,11 +589,8 @@ impl From<&Merge<Tree>> for ConflictsDirItem {

let mut entries = vec![];
for (basename, value) in all_tree_entries(trees) {
match value {
MergedTreeVal::Resolved(_) => {}
MergedTreeVal::Conflict(tree_values) => {
entries.push((dir.join(basename), tree_values));
}
if !value.is_resolved() {
entries.push((dir.join(basename), value.cloned()));
}
}
entries.reverse();
Expand Down Expand Up @@ -809,8 +786,6 @@ impl TreeDiffDirItem {
let mut entries = vec![];
for (name, before, after) in merged_tree_entry_diff(trees1, trees2) {
let path = dir.join(name);
let before = before.to_merge();
let after = after.to_merge();
let tree_before = before.is_tree();
let tree_after = after.is_tree();
// Check if trees and files match, but only if either side is a tree or a file
Expand All @@ -832,7 +807,7 @@ impl TreeDiffDirItem {
if before.is_absent() && after.is_absent() {
continue;
}
entries.push((path, before, after));
entries.push((path, before.cloned(), after.cloned()));
}
entries.reverse();
Self { entries }
Expand Down Expand Up @@ -985,12 +960,8 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> {
}
};

for (basename, value_before, value_after) in
merged_tree_entry_diff(&tree1.trees, &tree2.trees)
{
for (basename, before, after) in merged_tree_entry_diff(&tree1.trees, &tree2.trees) {
let path = dir.join(basename);
let before = value_before.to_merge();
let after = value_after.to_merge();
let tree_before = before.is_tree();
let tree_after = after.is_tree();
// Check if trees and files match, but only if either side is a tree or a file
Expand All @@ -1017,17 +988,19 @@ impl<'matcher> TreeDiffStreamImpl<'matcher> {
// If the path was a tree on either side of the diff, read those trees.
if tree_matches {
let before_tree_future =
Self::tree(tree1.store().clone(), path.clone(), before.clone());
Self::tree(tree1.store().clone(), path.clone(), before.cloned());
let after_tree_future =
Self::tree(tree2.store().clone(), path.clone(), after.clone());
Self::tree(tree2.store().clone(), path.clone(), after.cloned());
let both_trees_future =
async { futures::try_join!(before_tree_future, after_tree_future) };
self.pending_trees
.push_back((path.clone(), Box::pin(both_trees_future)));
}

self.items
.insert(DiffStreamKey::normal(path), Ok((before, after)));
self.items.insert(
DiffStreamKey::normal(path),
Ok((before.cloned(), after.cloned())),
);
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/src/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::backend::{
};
use crate::files::MergeResult;
use crate::matchers::{EverythingMatcher, Matcher};
use crate::merge::{trivial_merge, Merge};
use crate::merge::{trivial_merge, Merge, MergedTreeVal};
use crate::object_id::ObjectId;
use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent};
use crate::store::Store;
Expand Down Expand Up @@ -404,7 +404,7 @@ fn merge_tree_value(
pub fn try_resolve_file_conflict(
store: &Store,
filename: &RepoPath,
conflict: &Merge<Option<&TreeValue>>,
conflict: &MergedTreeVal,
) -> BackendResult<Option<TreeValue>> {
// If there are any non-file or any missing parts in the conflict, we can't
// merge it. We check early so we don't waste time reading file contents if
Expand Down
47 changes: 28 additions & 19 deletions lib/tests/test_merged_tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ use jj_lib::files::MergeResult;
use jj_lib::matchers::{EverythingMatcher, FilesMatcher, Matcher, PrefixMatcher};
use jj_lib::merge::{Merge, MergeBuilder, MergedTreeValue};
use jj_lib::merged_tree::{
MergedTree, MergedTreeBuilder, MergedTreeVal, TreeDiffEntry, TreeDiffIterator,
TreeDiffStreamImpl,
MergedTree, MergedTreeBuilder, TreeDiffEntry, TreeDiffIterator, TreeDiffStreamImpl,
};
use jj_lib::repo::Repo;
use jj_lib::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent};
Expand Down Expand Up @@ -163,52 +162,62 @@ fn test_from_legacy_tree() {
let merged_tree = MergedTree::from_legacy_tree(tree.clone()).unwrap();
assert_eq!(
merged_tree.value(RepoPathComponent::new("missing")),
MergedTreeVal::Resolved(None)
Merge::absent()
);
// file1: regular file without conflicts
assert_eq!(
merged_tree.value(file1_path.components().next().unwrap()),
MergedTreeVal::Resolved(Some(&TreeValue::File {
merged_tree
.value(file1_path.components().next().unwrap())
.cloned(),
Merge::normal(TreeValue::File {
id: file1_id.clone(),
executable: false,
}))
})
);
// file2: 3-way conflict
assert_eq!(
merged_tree.value(file2_path.components().next().unwrap()),
MergedTreeVal::Conflict(Merge::from_removes_adds(
merged_tree
.value(file2_path.components().next().unwrap())
.cloned(),
Merge::from_removes_adds(
vec![Some(file_value(&file2_v1_id)), None],
vec![
Some(file_value(&file2_v2_id)),
Some(file_value(&file2_v3_id)),
None,
],
))
)
);
// file3: modify/delete conflict
assert_eq!(
merged_tree.value(file3_path.components().next().unwrap()),
MergedTreeVal::Conflict(Merge::from_removes_adds(
merged_tree
.value(file3_path.components().next().unwrap())
.cloned(),
Merge::from_removes_adds(
vec![Some(file_value(&file3_v1_id)), None],
vec![Some(file_value(&file3_v2_id)), None, None],
))
)
);
// file4: add/add conflict
assert_eq!(
merged_tree.value(file4_path.components().next().unwrap()),
MergedTreeVal::Conflict(Merge::from_removes_adds(
merged_tree
.value(file4_path.components().next().unwrap())
.cloned(),
Merge::from_removes_adds(
vec![None, None],
vec![
Some(file_value(&file4_v1_id)),
Some(file_value(&file4_v2_id)),
None
],
))
)
);
// file5: 5-way conflict
assert_eq!(
merged_tree.value(file5_path.components().next().unwrap()),
MergedTreeVal::Conflict(Merge::from_removes_adds(
merged_tree
.value(file5_path.components().next().unwrap())
.cloned(),
Merge::from_removes_adds(
vec![
Some(file_value(&file5_v1_id)),
Some(file_value(&file5_v2_id)),
Expand All @@ -218,12 +227,12 @@ fn test_from_legacy_tree() {
Some(file_value(&file5_v4_id)),
Some(file_value(&file5_v5_id)),
],
))
)
);
// file6: directory without conflicts
assert_eq!(
merged_tree.value(dir1_basename),
MergedTreeVal::Resolved(tree.value(dir1_basename))
Merge::resolved(tree.value(dir1_basename))
);

// Create the merged tree by starting from an empty merged tree and adding
Expand Down

0 comments on commit f7377fb

Please sign in to comment.