From 309f1200d65160a00d34d23ad30a165fe8aa270b Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 7 Sep 2023 15:06:00 -0700 Subject: [PATCH] merge: introduce a type alias for `Merge>` Reasons to introduce this alias: * Reduces complexity of a type, to silence Clippy warnings in the future if we use this type as a type parameter * The type is used quite frequently, so it makes sense to have a name for it * It's easier to visually scan for the end of the type when you don't have to match opening and closing angle brackets --- cli/src/commands/mod.rs | 4 ++-- cli/src/diff_util.rs | 10 ++++----- cli/src/merge_tools/builtin.rs | 3 ++- cli/src/merge_tools/external.rs | 4 ++-- lib/src/conflicts.rs | 6 +++--- lib/src/local_working_copy.rs | 12 +++++------ lib/src/merge.rs | 10 +++++++-- lib/src/merged_tree.rs | 36 +++++++++++++++------------------ lib/src/store.rs | 9 ++++----- lib/src/tree.rs | 4 ++-- 10 files changed, 50 insertions(+), 48 deletions(-) diff --git a/cli/src/commands/mod.rs b/cli/src/commands/mod.rs index 461ea05e4..b41a81023 100644 --- a/cli/src/commands/mod.rs +++ b/cli/src/commands/mod.rs @@ -37,7 +37,7 @@ use jj_lib::commit::Commit; use jj_lib::dag_walk::topo_order_reverse; use jj_lib::git_backend::GitBackend; use jj_lib::matchers::EverythingMatcher; -use jj_lib::merge::Merge; +use jj_lib::merge::{Merge, MergedTreeValue}; use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder}; use jj_lib::op_store::WorkspaceId; use jj_lib::repo::{ReadonlyRepo, Repo}; @@ -3053,7 +3053,7 @@ fn cmd_resolve( #[instrument(skip_all)] fn print_conflicted_paths( - conflicts: &[(RepoPath, Merge>)], + conflicts: &[(RepoPath, MergedTreeValue)], formatter: &mut dyn Formatter, workspace_command: &WorkspaceCommandHelper, ) -> Result<(), CommandError> { diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index c61f13417..71d0eef53 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -25,7 +25,7 @@ use jj_lib::commit::Commit; use jj_lib::diff::{Diff, DiffHunk}; use jj_lib::files::DiffLine; use jj_lib::matchers::Matcher; -use jj_lib::merge::Merge; +use jj_lib::merge::{Merge, MergedTreeValue}; use jj_lib::merged_tree::{MergedTree, TreeDiffIterator}; use jj_lib::repo::{ReadonlyRepo, Repo}; use jj_lib::repo_path::RepoPath; @@ -345,7 +345,7 @@ fn show_color_words_diff_line( fn diff_content( repo: &Arc, path: &RepoPath, - value: &Merge>, + value: &MergedTreeValue, ) -> Result, CommandError> { match value.as_resolved() { Some(None) => Ok(vec![]), @@ -379,7 +379,7 @@ fn diff_content( } } -fn basic_diff_file_type(values: &Merge>) -> String { +fn basic_diff_file_type(values: &MergedTreeValue) -> String { match values.as_resolved() { Some(None) => { panic!("absent path in diff"); @@ -493,7 +493,7 @@ struct GitDiffPart { fn git_diff_part( repo: &Arc, path: &RepoPath, - value: &Merge>, + value: &MergedTreeValue, ) -> Result { let mode; let hash; @@ -886,7 +886,7 @@ pub fn show_types( }) } -fn diff_summary_char(value: &Merge>) -> char { +fn diff_summary_char(value: &MergedTreeValue) -> char { match value.as_resolved() { Some(None) => '-', Some(Some(TreeValue::File { .. })) => 'F', diff --git a/cli/src/merge_tools/builtin.rs b/cli/src/merge_tools/builtin.rs index 180836764..2fa31ff27 100644 --- a/cli/src/merge_tools/builtin.rs +++ b/cli/src/merge_tools/builtin.rs @@ -544,6 +544,7 @@ pub fn edit_merge_builtin( mod tests { use futures::executor::block_on; use jj_lib::conflicts::extract_as_single_hunk; + use jj_lib::merge::MergedTreeValue; use jj_lib::repo::Repo; use testutils::TestRepo; @@ -710,7 +711,7 @@ mod tests { &[(&path, "right 1\nbase 2\nbase 3\nbase 4\nright 5\n")], ); - fn to_file_id(tree_value: Merge>) -> Option { + fn to_file_id(tree_value: MergedTreeValue) -> Option { match tree_value.into_resolved() { Ok(Some(TreeValue::File { id, executable: _ })) => Some(id.clone()), other => { diff --git a/cli/src/merge_tools/external.rs b/cli/src/merge_tools/external.rs index 5cd574af2..f0ef9b7ad 100644 --- a/cli/src/merge_tools/external.rs +++ b/cli/src/merge_tools/external.rs @@ -13,7 +13,7 @@ use jj_lib::conflicts::{self, materialize_merge_result}; use jj_lib::gitignore::GitIgnoreFile; use jj_lib::local_working_copy::{TreeState, TreeStateError}; use jj_lib::matchers::Matcher; -use jj_lib::merge::Merge; +use jj_lib::merge::{Merge, MergedTreeValue}; use jj_lib::merged_tree::{MergedTree, MergedTreeBuilder}; use jj_lib::repo_path::RepoPath; use jj_lib::settings::UserSettings; @@ -291,7 +291,7 @@ pub fn run_mergetool_external( file_merge: Merge>, content: Merge, repo_path: &RepoPath, - conflict: Merge>, + conflict: MergedTreeValue, tree: &MergedTree, ) -> Result { let initial_output_content: Vec = if editor.merge_tool_edits_conflict_markers { diff --git a/lib/src/conflicts.rs b/lib/src/conflicts.rs index b09a1badd..071f9d864 100644 --- a/lib/src/conflicts.rs +++ b/lib/src/conflicts.rs @@ -20,11 +20,11 @@ use std::iter::zip; use futures::StreamExt; use itertools::Itertools; -use crate::backend::{BackendResult, FileId, TreeValue}; +use crate::backend::{BackendResult, FileId}; use crate::diff::{find_line_ranges, Diff, DiffHunk}; use crate::files; use crate::files::{ContentHunk, MergeResult}; -use crate::merge::{Merge, MergeBuilder}; +use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::repo_path::RepoPath; use crate::store::Store; @@ -89,7 +89,7 @@ pub async fn extract_as_single_hunk( } pub async fn materialize( - conflict: &Merge>, + conflict: &MergedTreeValue, store: &Store, path: &RepoPath, output: &mut dyn Write, diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 1d7c54753..16d67835a 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -53,7 +53,7 @@ use crate::lock::FileLock; use crate::matchers::{ DifferenceMatcher, EverythingMatcher, IntersectionMatcher, Matcher, PrefixMatcher, }; -use crate::merge::{Merge, MergeBuilder}; +use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::merged_tree::{MergedTree, MergedTreeBuilder}; use crate::op_store::{OperationId, WorkspaceId}; use crate::repo_path::{FsPathParseError, RepoPath, RepoPathComponent, RepoPathJoin}; @@ -662,7 +662,7 @@ impl TreeState { &self, matcher: &dyn Matcher, current_tree: &MergedTree, - tree_entries_tx: Sender<(RepoPath, Merge>)>, + tree_entries_tx: Sender<(RepoPath, MergedTreeValue)>, file_states_tx: Sender<(RepoPath, FileState)>, present_files_tx: Sender, directory_to_visit: DirectoryToVisit, @@ -887,7 +887,7 @@ impl TreeState { maybe_current_file_state: Option<&FileState>, current_tree: &MergedTree, new_file_state: &FileState, - ) -> Result>>, SnapshotError> { + ) -> Result, SnapshotError> { let clean = match maybe_current_file_state { None => { // untracked @@ -922,9 +922,9 @@ impl TreeState { &self, repo_path: &RepoPath, disk_path: &Path, - current_tree_values: &Merge>, + current_tree_values: &MergedTreeValue, file_type: FileType, - ) -> Result>, SnapshotError> { + ) -> Result { let executable = match file_type { FileType::Normal { executable } => executable, FileType::Symlink => { @@ -1052,7 +1052,7 @@ impl TreeState { &self, disk_path: &Path, path: &RepoPath, - conflict: &Merge>, + conflict: &MergedTreeValue, ) -> Result { let mut file = OpenOptions::new() .write(true) diff --git a/lib/src/merge.rs b/lib/src/merge.rs index 159172090..978e0bef9 100644 --- a/lib/src/merge.rs +++ b/lib/src/merge.rs @@ -414,7 +414,13 @@ impl ContentHash for Merge { } } -impl 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 +/// shouldn't be. +pub type MergedTreeValue = Merge>; + +impl MergedTreeValue { /// Create a `Merge` from a `backend::Conflict`, padding with `None` to /// make sure that there is exactly one more `adds()` than `removes()`. pub fn from_backend_conflict(conflict: backend::Conflict) -> Self { @@ -497,7 +503,7 @@ impl Merge> where T: Borrow, { - /// If every non-`None` term of a `Merge>` + /// If every non-`None` term of a `MergedTreeValue` /// is a `TreeValue::Tree`, this converts it to /// a `Merge`, with empty trees instead of /// any `None` terms. Otherwise, returns `None`. diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index c39f4bf27..8b701ff38 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -26,7 +26,7 @@ use itertools::Itertools; use crate::backend::{BackendError, BackendResult, ConflictId, MergedTreeId, TreeId, TreeValue}; use crate::matchers::{EverythingMatcher, Matcher}; -use crate::merge::{Merge, MergeBuilder}; +use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin}; use crate::store::Store; use crate::tree::{try_resolve_file_conflict, Tree, TreeMergeError}; @@ -50,11 +50,11 @@ pub enum MergedTreeVal<'a> { Resolved(Option<&'a TreeValue>), /// TODO: Make this a `Merge>` (reference to the /// value) once we have removed the `MergedTree::Legacy` variant. - Conflict(Merge>), + Conflict(MergedTreeValue), } impl MergedTreeVal<'_> { - fn to_merge(&self) -> Merge> { + fn to_merge(&self) -> MergedTreeValue { match self { MergedTreeVal::Resolved(value) => Merge::resolved(value.cloned()), MergedTreeVal::Conflict(merge) => merge.clone(), @@ -110,7 +110,7 @@ impl MergedTree { // build `2*num_removes + 1` trees let mut max_num_removes = 0; let store = tree.store(); - let mut conflicts: Vec<(&RepoPath, Merge>)> = vec![]; + let mut conflicts: Vec<(&RepoPath, MergedTreeValue)> = vec![]; for (path, conflict_id) in &conflict_ids { let conflict = store.read_conflict(path, conflict_id)?; max_num_removes = max(max_num_removes, conflict.removes().len()); @@ -218,7 +218,7 @@ impl MergedTree { /// all sides are trees, so tree/file conflicts will be reported as a single /// conflict, not one for each path in the tree. // TODO: Restrict this by a matcher (or add a separate method for that). - pub fn conflicts(&self) -> impl Iterator>)> { + pub fn conflicts(&self) -> impl Iterator { ConflictIterator::new(self.clone()) } @@ -265,7 +265,7 @@ impl MergedTree { /// The value at the given path. The value can be `Resolved` even if /// `self` is a `Conflict`, which happens if the value at the path can be /// trivially merged. - pub fn path_value(&self, path: &RepoPath) -> Merge> { + pub fn path_value(&self, path: &RepoPath) -> MergedTreeValue { assert_eq!(self.dir(), &RepoPath::root()); match path.split() { Some((dir, basename)) => match self.sub_tree_recursive(dir.components()) { @@ -475,8 +475,8 @@ fn merge_trees(merge: &Merge) -> Result, TreeMergeError> { fn merge_tree_values( store: &Arc, path: &RepoPath, - values: Merge>, -) -> Result>, TreeMergeError> { + values: MergedTreeValue, +) -> Result { if let Some(resolved) = values.resolve_trivial() { return Ok(Merge::resolved(resolved.clone())); } @@ -563,7 +563,7 @@ impl<'matcher> TreeEntriesIterator<'matcher> { } impl Iterator for TreeEntriesIterator<'_> { - type Item = (RepoPath, Merge>); + type Item = (RepoPath, MergedTreeValue); fn next(&mut self) -> Option { while let Some(top) = self.stack.last_mut() { @@ -622,7 +622,7 @@ impl<'a> ConflictEntriesNonRecursiveIterator<'a> { } impl<'a> Iterator for ConflictEntriesNonRecursiveIterator<'a> { - type Item = (&'a RepoPathComponent, Merge>); + type Item = (&'a RepoPathComponent, MergedTreeValue); fn next(&mut self) -> Option { for basename in self.basename_iter.by_ref() { @@ -684,7 +684,7 @@ impl ConflictIterator { } impl Iterator for ConflictIterator { - type Item = (RepoPath, Merge>); + type Item = (RepoPath, MergedTreeValue); fn next(&mut self) -> Option { match self { @@ -799,7 +799,7 @@ enum TreeDiffItem { // This is used for making sure that when a directory gets replaced by a file, we // yield the value for the addition of the file after we yield the values // for removing files in the directory. - File(RepoPath, Merge>, Merge>), + File(RepoPath, MergedTreeValue, MergedTreeValue), } impl<'matcher> TreeDiffIterator<'matcher> { @@ -822,11 +822,7 @@ impl<'matcher> TreeDiffIterator<'matcher> { } /// Gets the given tree if `value` is a tree, otherwise an empty tree. - async fn tree( - tree: &MergedTree, - dir: &RepoPath, - values: &Merge>, - ) -> MergedTree { + async fn tree(tree: &MergedTree, dir: &RepoPath, values: &MergedTreeValue) -> MergedTree { let trees = if values.is_tree() { let builder: MergeBuilder = futures::stream::iter(values.iter()) .then(|value| Self::single_tree(tree.store(), dir, value.as_ref())) @@ -861,7 +857,7 @@ impl TreeDiffDirItem { } impl Iterator for TreeDiffIterator<'_> { - type Item = (RepoPath, Merge>, Merge>); + type Item = (RepoPath, MergedTreeValue, MergedTreeValue); fn next(&mut self) -> Option { while let Some(top) = self.stack.last_mut() { @@ -926,7 +922,7 @@ impl Iterator for TreeDiffIterator<'_> { /// (allowing path-level conflicts) or as multiple conflict-free trees. pub struct MergedTreeBuilder { base_tree_id: MergedTreeId, - overrides: BTreeMap>>, + overrides: BTreeMap, } impl MergedTreeBuilder { @@ -944,7 +940,7 @@ impl MergedTreeBuilder { /// `Merge::absent()` to remove a value from the tree. When the base tree is /// a legacy tree, conflicts can be written either as a multi-way `Merge` /// value or as a resolved `Merge` value using `TreeValue::Conflict`. - pub fn set_or_remove(&mut self, path: RepoPath, values: Merge>) { + pub fn set_or_remove(&mut self, path: RepoPath, values: MergedTreeValue) { if let MergedTreeId::Merge(_) = &self.base_tree_id { assert!(!values .iter() diff --git a/lib/src/store.rs b/lib/src/store.rs index 46fe7e707..705a79940 100644 --- a/lib/src/store.rs +++ b/lib/src/store.rs @@ -24,11 +24,10 @@ use futures::executor::block_on; use crate::backend; use crate::backend::{ - Backend, BackendResult, ChangeId, CommitId, ConflictId, FileId, MergedTreeId, SymlinkId, - TreeId, TreeValue, + Backend, BackendResult, ChangeId, CommitId, ConflictId, FileId, MergedTreeId, SymlinkId, TreeId, }; use crate::commit::Commit; -use crate::merge::Merge; +use crate::merge::{Merge, MergedTreeValue}; use crate::merged_tree::MergedTree; use crate::repo_path::RepoPath; use crate::tree::Tree; @@ -229,7 +228,7 @@ impl Store { &self, path: &RepoPath, id: &ConflictId, - ) -> BackendResult>> { + ) -> BackendResult { let backend_conflict = block_on(self.backend.read_conflict(path, id))?; Ok(Merge::from_backend_conflict(backend_conflict)) } @@ -237,7 +236,7 @@ impl Store { pub fn write_conflict( &self, path: &RepoPath, - contents: &Merge>, + contents: &MergedTreeValue, ) -> BackendResult { self.backend .write_conflict(path, &contents.clone().into_backend_conflict()) diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 74a16bcd7..eca5839a9 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -29,7 +29,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, MergedTreeValue}; use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathJoin}; use crate::store::Store; use crate::{backend, files}; @@ -403,7 +403,7 @@ fn merge_tree_value( pub fn try_resolve_file_conflict( store: &Store, filename: &RepoPath, - conflict: &Merge>, + conflict: &MergedTreeValue, ) -> Result, TreeMergeError> { // 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