diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index 7f299a1ff..4f24756e7 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -301,9 +301,10 @@ fn create_parent_dirs( working_copy_path: &Path, repo_path: &RepoPath, ) -> Result { - let (_, dir_components) = repo_path - .components() - .split_last() + // TODO: make RepoPath::parent() cheap and use it instead + let mut dir_components = repo_path.components(); + dir_components + .next_back() .expect("repo path shouldn't be root"); let mut dir_path = working_copy_path.to_owned(); for c in dir_components { diff --git a/lib/src/matchers.rs b/lib/src/matchers.rs index e8e66f05d..c1ca8034c 100644 --- a/lib/src/matchers.rs +++ b/lib/src/matchers.rs @@ -19,7 +19,7 @@ use std::iter; use tracing::instrument; -use crate::repo_path::{RepoPath, RepoPathComponentBuf}; +use crate::repo_path::{RepoPath, RepoPathComponentBuf, RepoPathComponentsIter}; #[derive(PartialEq, Eq, Debug)] pub enum Visit { @@ -146,13 +146,13 @@ impl Matcher for PrefixMatcher { } fn visit(&self, dir: &RepoPath) -> Visit { - for (sub, tail_components) in self.tree.walk_to(dir) { + for (sub, mut tail_components) in self.tree.walk_to(dir) { // 'is_file' means the current path matches prefix paths if sub.is_file { return Visit::AllRecursively; } // 'dir' found, and is an ancestor of prefix paths - if tail_components.is_empty() { + if tail_components.next().is_none() { return sub.to_visit_sets(); } } @@ -280,11 +280,11 @@ impl RepoPathTree { } fn add(&mut self, dir: &RepoPath) -> &mut RepoPathTree { - dir.components().iter().fold(self, |sub, name| { + dir.components().fold(self, |sub, name| { // Avoid name.clone() if entry already exists. if !sub.entries.contains_key(name) { sub.is_dir = true; - sub.entries.insert(name.clone(), RepoPathTree::new()); + sub.entries.insert(name.to_owned(), RepoPathTree::new()); } sub.entries.get_mut(name).unwrap() }) @@ -300,7 +300,6 @@ impl RepoPathTree { fn get(&self, dir: &RepoPath) -> Option<&RepoPathTree> { dir.components() - .iter() .try_fold(self, |sub, name| sub.entries.get(name)) } @@ -310,17 +309,17 @@ impl RepoPathTree { .unwrap_or(Visit::Nothing) } - fn walk_to<'a>( + fn walk_to<'a, 'b: 'a>( &'a self, - dir: &'a RepoPath, - ) -> impl Iterator + 'a { - iter::successors( - Some((self, dir.components().as_slice())), - |(sub, components)| { - let (name, tail) = components.split_first()?; - Some((sub.entries.get(name)?, tail)) - }, - ) + dir: &'b RepoPath, + ) -> impl Iterator)> + 'a { + iter::successors(Some((self, dir.components())), |(sub, components)| { + // TODO: Add cheap as_path() method to the components iterator. + // Cloning iterator should be cheap, but looks a bit weird. + let mut components = components.clone(); + let name = components.next()?; + Some((sub.entries.get(name)?, components)) + }) } fn to_visit_sets(&self) -> Visit { diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index 513242a84..d03ab4817 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -30,7 +30,7 @@ use pollster::FutureExt; use crate::backend::{BackendError, BackendResult, ConflictId, MergedTreeId, TreeId, TreeValue}; use crate::matchers::{EverythingMatcher, Matcher}; use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; -use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathComponentBuf}; +use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathComponentsIter}; use crate::store::Store; use crate::tree::{try_resolve_file_conflict, Tree, TreeMergeError}; use crate::tree_builder::TreeBuilder; @@ -268,11 +268,9 @@ impl MergedTree { } } - // TODO: switch to borrowed &RepoPath type or RepoPathComponents iterator - fn sub_tree_recursive(&self, components: &[RepoPathComponentBuf]) -> Option { - if let Some((first, tail)) = components.split_first() { - tail.iter() - .try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name)) + fn sub_tree_recursive(&self, mut components: RepoPathComponentsIter) -> Option { + if let Some(first) = components.next() { + components.try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name)) } else { Some(self.clone()) } diff --git a/lib/src/repo_path.rs b/lib/src/repo_path.rs index 0a0275e2e..7e8fcd0dd 100644 --- a/lib/src/repo_path.rs +++ b/lib/src/repo_path.rs @@ -111,6 +111,12 @@ impl ToOwned for RepoPathComponent { } } +/// Iterator over `RepoPath` components. +pub type RepoPathComponentsIter<'a> = std::iter::Map< + std::slice::Iter<'a, RepoPathComponentBuf>, + fn(&RepoPathComponentBuf) -> &RepoPathComponent, +>; + #[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct RepoPath { components: Vec, @@ -244,8 +250,8 @@ impl RepoPath { } } - pub fn components(&self) -> &Vec { - &self.components + pub fn components(&self) -> RepoPathComponentsIter<'_> { + self.components.iter().map(AsRef::as_ref) } pub fn join(&self, entry: &RepoPathComponent) -> RepoPath { @@ -361,16 +367,30 @@ mod tests { #[test] fn test_components() { - assert_eq!(RepoPath::root().components(), &vec![]); + assert!(RepoPath::root().components().next().is_none()); assert_eq!( - repo_path("dir").components(), - &vec![RepoPathComponentBuf::from("dir")] + repo_path("dir").components().collect_vec(), + vec![RepoPathComponent::new("dir")] ); assert_eq!( - repo_path("dir/subdir").components(), - &vec![ - RepoPathComponentBuf::from("dir"), - RepoPathComponentBuf::from("subdir") + repo_path("dir/subdir").components().collect_vec(), + vec![ + RepoPathComponent::new("dir"), + RepoPathComponent::new("subdir"), + ] + ); + + // Iterates from back + assert!(RepoPath::root().components().next_back().is_none()); + assert_eq!( + repo_path("dir").components().rev().collect_vec(), + vec![RepoPathComponent::new("dir")] + ); + assert_eq!( + repo_path("dir/subdir").components().rev().collect_vec(), + vec![ + RepoPathComponent::new("subdir"), + RepoPathComponent::new("dir"), ] ); } diff --git a/lib/src/tree.rs b/lib/src/tree.rs index ca04e1733..eb4f4ee69 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -30,7 +30,7 @@ use crate::backend::{ use crate::files::MergeResult; use crate::matchers::{EverythingMatcher, Matcher}; use crate::merge::{trivial_merge, Merge, MergedTreeValue}; -use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathComponentBuf}; +use crate::repo_path::{RepoPath, RepoPathComponent, RepoPathComponentsIter}; use crate::store::Store; use crate::{backend, files}; @@ -159,11 +159,9 @@ impl Tree { self.store.get_tree(subdir, id).unwrap() } - // TODO: switch to borrowed &RepoPath type or RepoPathComponents iterator - fn sub_tree_recursive(&self, components: &[RepoPathComponentBuf]) -> Option { - if let Some((first, tail)) = components.split_first() { - tail.iter() - .try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name)) + fn sub_tree_recursive(&self, mut components: RepoPathComponentsIter) -> Option { + if let Some(first) = components.next() { + components.try_fold(self.sub_tree(first)?, |tree, name| tree.sub_tree(name)) } else { // TODO: It would be nice to be able to return a reference here, but // then we would have to figure out how to share Tree instances diff --git a/lib/tests/test_merged_tree.rs b/lib/tests/test_merged_tree.rs index 92affea01..2a0aa15c3 100644 --- a/lib/tests/test_merged_tree.rs +++ b/lib/tests/test_merged_tree.rs @@ -140,7 +140,7 @@ fn test_from_legacy_tree() { ); // file1: regular file without conflicts assert_eq!( - merged_tree.value(&file1_path.components()[0]), + merged_tree.value(file1_path.components().next().unwrap()), MergedTreeVal::Resolved(Some(&TreeValue::File { id: file1_id.clone(), executable: false, @@ -148,7 +148,7 @@ fn test_from_legacy_tree() { ); // file2: 3-way conflict assert_eq!( - merged_tree.value(&file2_path.components()[0]), + merged_tree.value(file2_path.components().next().unwrap()), MergedTreeVal::Conflict(Merge::from_removes_adds( vec![Some(file_value(&file2_v1_id)), None], vec![ @@ -160,7 +160,7 @@ fn test_from_legacy_tree() { ); // file3: modify/delete conflict assert_eq!( - merged_tree.value(&file3_path.components()[0]), + merged_tree.value(file3_path.components().next().unwrap()), MergedTreeVal::Conflict(Merge::from_removes_adds( vec![Some(file_value(&file3_v1_id)), None], vec![Some(file_value(&file3_v2_id)), None, None], @@ -168,7 +168,7 @@ fn test_from_legacy_tree() { ); // file4: add/add conflict assert_eq!( - merged_tree.value(&file4_path.components()[0]), + merged_tree.value(file4_path.components().next().unwrap()), MergedTreeVal::Conflict(Merge::from_removes_adds( vec![None, None], vec![ @@ -180,7 +180,7 @@ fn test_from_legacy_tree() { ); // file5: 5-way conflict assert_eq!( - merged_tree.value(&file5_path.components()[0]), + merged_tree.value(file5_path.components().next().unwrap()), MergedTreeVal::Conflict(Merge::from_removes_adds( vec![ Some(file_value(&file5_v1_id)),