From f7377fbbcda902981e50823e797e2b2013a52ac5 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 11 Aug 2024 21:39:13 +0900 Subject: [PATCH] merged_tree: replace MergedTreeVal<'_> by Merge> MergedTreeVal was roughly equivalent to Merge>. As we've dropped support for the legacy trees, it can be simplified to Merge>. --- lib/src/merge.rs | 3 ++ lib/src/merged_tree.rs | 87 ++++++++++++----------------------- lib/src/tree.rs | 4 +- lib/tests/test_merged_tree.rs | 47 +++++++++++-------- 4 files changed, 63 insertions(+), 78 deletions(-) diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 412e1f6466..04bba6ec0a 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -509,6 +509,9 @@ impl ContentHash for Merge { } } +/// Borrowed `MergedTreeValue`. +pub type MergedTreeVal<'a> = Merge>; + /// 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 diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 5d8d8d846f..7dc84a8a8f 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -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}; @@ -43,26 +43,6 @@ pub struct MergedTree { trees: Merge, } -/// 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>` (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 { @@ -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> { - 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); @@ -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 @@ -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) } @@ -394,7 +375,7 @@ fn all_tree_entries( /// than `tree.entries_non_recursive()`. fn all_merged_tree_entries( trees: &Merge, -) -> impl Iterator>)> { +) -> impl Iterator)> { let mut entries_iters = trees .iter() .map(|tree| tree.entries_non_recursive().peekable()) @@ -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, 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 @@ -498,7 +479,7 @@ fn merge_trees(merge: &Merge) -> BackendResult> { fn merge_tree_values( store: &Arc, path: &RepoPath, - values: &Merge>, + values: &MergedTreeVal, ) -> BackendResult { if let Some(resolved) = values.resolve_trivial() { return Ok(Merge::resolved(resolved.cloned())); @@ -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() { @@ -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 } @@ -609,11 +589,8 @@ impl From<&Merge> 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(); @@ -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 @@ -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 } @@ -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 @@ -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())), + ); } } diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 3fa14d0227..ed776e9938 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -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; @@ -404,7 +404,7 @@ fn merge_tree_value( pub fn try_resolve_file_conflict( store: &Store, filename: &RepoPath, - conflict: &Merge>, + conflict: &MergedTreeVal, ) -> BackendResult> { // 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 diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index cfe4217c68..52869428b4 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -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}; @@ -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)), @@ -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