From 4187847c606cb0f486c7897e63a8240625fa55fd Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 19 Oct 2024 17:49:54 +0900 Subject: [PATCH] revset: propagate errors from filter predicates All intermediate nodes are changed to RevWalk of Result type to pass BackendError around from filter predicates. Leaf ancestors/descendants computation is unchanged, and mapped to Result at revset_engine layer. This is simpler than converting all RevWalk impls to Result<_, _>. --- lib/src/default_index/revset_engine.rs | 506 ++++++++++++------ .../default_index/revset_graph_iterator.rs | 87 +-- 2 files changed, 399 insertions(+), 194 deletions(-) diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index 81be55a1bf..4ad2a2aca9 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -66,8 +66,10 @@ use crate::store::Store; use crate::str_util::StringPattern; use crate::union_find; -type BoxedPredicateFn<'a> = Box bool + 'a>; -pub(super) type BoxedRevWalk<'a> = Box + 'a>; +type BoxedPredicateFn<'a> = + Box Result + 'a>; +pub(super) type BoxedRevWalk<'a> = + Box> + 'a>; trait ToPredicateFn: fmt::Debug { /// Creates function that tests if the given entry is included in the set. @@ -124,7 +126,7 @@ impl RevsetImpl { Self { inner, index } } - fn positions(&self) -> impl Iterator + '_ { + fn positions(&self) -> impl Iterator> + '_ { self.inner.positions().attach(self.index.as_composite()) } @@ -135,7 +137,7 @@ impl RevsetImpl { let index = self.index.clone(); let walk = self.inner.positions(); let mut graph_walk = RevsetGraphWalk::new(walk, skip_transitive_edges); - iter::from_fn(move || graph_walk.next(index.as_composite()).map(Ok)) + iter::from_fn(move || graph_walk.next(index.as_composite())) } } @@ -156,7 +158,7 @@ impl Revset for RevsetImpl { let mut walk = self .inner .positions() - .map(|index, pos| Ok(index.entry_by_pos(pos).commit_id())); + .map(|index, pos| Ok(index.entry_by_pos(pos?).commit_id())); Box::new(iter::from_fn(move || walk.next(index.as_composite()))) } @@ -168,7 +170,7 @@ impl Revset for RevsetImpl { { let index = self.index.clone(); let mut walk = self.inner.positions().map(|index, pos| { - let entry = index.entry_by_pos(pos); + let entry = index.entry_by_pos(pos?); Ok((entry.commit_id(), entry.change_id())) }); Box::new(iter::from_fn(move || walk.next(index.as_composite()))) @@ -193,15 +195,17 @@ impl Revset for RevsetImpl { // Exercise the estimation feature in tests. (If we ever have a Revset // implementation in production code that returns estimates, we can probably // remove this and rewrite the associated tests.) - let count = self.positions().take(10).count(); + let count = self + .positions() + .take(10) + .process_results(|iter| iter.count())?; if count < 10 { Ok((count, Some(count))) } else { Ok((10, None)) } } else { - // TODO: propagate error - let count = self.positions().count(); + let count = self.positions().process_results(|iter| iter.count())?; Ok((count, Some(count))) } } @@ -211,7 +215,7 @@ impl Revset for RevsetImpl { Self: 'a, { let positions = PositionsAccumulator::new(self.index.clone(), self.inner.positions()); - Box::new(move |commit_id| Ok(positions.contains(commit_id))) + Box::new(move |commit_id| positions.contains(commit_id)) } } @@ -231,18 +235,19 @@ impl<'a, I: AsCompositeIndex> PositionsAccumulator<'a, I> { } /// Checks whether the commit is in the revset. - fn contains(&self, commit_id: &CommitId) -> bool { + fn contains(&self, commit_id: &CommitId) -> Result { let index = self.index.as_composite(); let Some(position) = index.commit_id_to_pos(commit_id) else { - return false; + return Ok(false); }; let mut inner = self.inner.borrow_mut(); - inner.consume_to(index, position); - inner + inner.consume_to(index, position)?; + let found = inner .consumed_positions .binary_search_by(|p| p.cmp(&position).reverse()) - .is_ok() + .is_ok(); + Ok(found) } #[cfg(test)] @@ -259,20 +264,26 @@ struct PositionsAccumulatorInner<'a> { impl PositionsAccumulatorInner<'_> { /// Consumes `RevWalk` to a desired position but not deeper. - fn consume_to(&mut self, index: &CompositeIndex, desired_position: IndexPosition) { + fn consume_to( + &mut self, + index: &CompositeIndex, + desired_position: IndexPosition, + ) -> Result<(), RevsetEvaluationError> { let last_position = self.consumed_positions.last(); if last_position.map_or(false, |&pos| pos <= desired_position) { - return; + return Ok(()); } - while let Some(position) = self.walk.next(index) { + while let Some(position) = self.walk.next(index).transpose()? { self.consumed_positions.push(position); if position <= desired_position { - return; + return Ok(()); } } + Ok(()) } } +/// Adapter for precomputed `IndexPosition`s. #[derive(Debug)] struct EagerRevset { positions: Vec, @@ -291,7 +302,8 @@ impl InternalRevset for EagerRevset { where Self: 'a, { - Box::new(EagerRevWalk::new(self.positions.clone().into_iter())) + let walk = EagerRevWalk::new(self.positions.clone().into_iter()); + Box::new(walk.map(|_index, pos| Ok(pos))) } fn into_predicate<'a>(self: Box) -> Box @@ -312,6 +324,7 @@ impl ToPredicateFn for EagerRevset { } } +/// Adapter for infallible `RevWalk` of `IndexPosition`s. struct RevWalkRevset { walk: W, } @@ -330,7 +343,7 @@ where where Self: 'a, { - Box::new(self.walk.clone()) + Box::new(self.walk.clone().map(|_index, pos| Ok(pos))) } fn into_predicate<'a>(self: Box) -> Box @@ -362,7 +375,7 @@ where while walk.next_if(index, |&pos| pos > entry_pos).is_some() { continue; } - walk.next_if(index, |&pos| pos == entry_pos).is_some() + Ok(walk.next_if(index, |&pos| pos == entry_pos).is_some()) }) } @@ -382,11 +395,10 @@ where Self: 'a, { let mut p = self.predicate.to_predicate_fn(); - Box::new( - self.candidates - .positions() - .filter_map(move |index, pos| p(index, pos).then_some(pos)), - ) + Box::new(self.candidates.positions().filter_map(move |index, pos| { + pos.and_then(|pos| Ok(p(index, pos)?.then_some(pos))) + .transpose() + })) } fn into_predicate<'a>(self: Box) -> Box @@ -408,7 +420,7 @@ where { let mut p1 = self.candidates.to_predicate_fn(); let mut p2 = self.predicate.to_predicate_fn(); - Box::new(move |index, pos| p1(index, pos) && p2(index, pos)) + Box::new(move |index, pos| Ok(p1(index, pos)? && p2(index, pos)?)) } } @@ -421,7 +433,7 @@ impl ToPredicateFn for NotInPredicate { Self: 'a, { let mut p = self.0.to_predicate_fn(); - Box::new(move |index, pos| !p(index, pos)) + Box::new(move |index, pos| Ok(!p(index, pos)?)) } } @@ -466,7 +478,7 @@ where { let mut p1 = self.set1.to_predicate_fn(); let mut p2 = self.set2.to_predicate_fn(); - Box::new(move |index, pos| p1(index, pos) || p2(index, pos)) + Box::new(move |index, pos| Ok(p1(index, pos)? || p2(index, pos)?)) } } @@ -479,12 +491,12 @@ struct UnionRevWalk, W2: RevWalk, C> { cmp: C, } -impl RevWalk for UnionRevWalk +impl RevWalk for UnionRevWalk where I: ?Sized, - W1: RevWalk, - W2: RevWalk, - C: FnMut(&W1::Item, &W2::Item) -> Ordering, + W1: RevWalk>, + W2: RevWalk>, + C: FnMut(&T, &T) -> Ordering, { type Item = W1::Item; @@ -492,7 +504,7 @@ where match (self.walk1.peek(index), self.walk2.peek(index)) { (None, _) => self.walk2.next(index), (_, None) => self.walk1.next(index), - (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) { + (Some(Ok(item1)), Some(Ok(item2))) => match (self.cmp)(item1, item2) { Ordering::Less => self.walk1.next(index), Ordering::Equal => { self.walk2.next(index); @@ -500,16 +512,18 @@ where } Ordering::Greater => self.walk2.next(index), }, + (Some(Err(_)), _) => self.walk1.next(index), + (_, Some(Err(_))) => self.walk2.next(index), } } } -fn union_by(walk1: W1, walk2: W2, cmp: C) -> UnionRevWalk +fn union_by(walk1: W1, walk2: W2, cmp: C) -> UnionRevWalk where I: ?Sized, - W1: RevWalk, - W2: RevWalk, - C: FnMut(&W1::Item, &W2::Item) -> Ordering, + W1: RevWalk>, + W2: RevWalk>, + C: FnMut(&T, &T) -> Ordering, { UnionRevWalk { walk1: walk1.peekable(), @@ -559,7 +573,7 @@ where { let mut p1 = self.set1.to_predicate_fn(); let mut p2 = self.set2.to_predicate_fn(); - Box::new(move |index, pos| p1(index, pos) && p2(index, pos)) + Box::new(move |index, pos| Ok(p1(index, pos)? && p2(index, pos)?)) } } @@ -572,12 +586,12 @@ struct IntersectionRevWalk, W2: RevWalk, C> { cmp: C, } -impl RevWalk for IntersectionRevWalk +impl RevWalk for IntersectionRevWalk where I: ?Sized, - W1: RevWalk, - W2: RevWalk, - C: FnMut(&W1::Item, &W2::Item) -> Ordering, + W1: RevWalk>, + W2: RevWalk>, + C: FnMut(&T, &T) -> Ordering, { type Item = W1::Item; @@ -590,7 +604,7 @@ where (_, None) => { return None; } - (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) { + (Some(Ok(item1)), Some(Ok(item2))) => match (self.cmp)(item1, item2) { Ordering::Less => { self.walk1.next(index); } @@ -602,17 +616,27 @@ where self.walk2.next(index); } }, + (Some(Err(_)), _) => { + return self.walk1.next(index); + } + (_, Some(Err(_))) => { + return self.walk2.next(index); + } } } } } -fn intersection_by(walk1: W1, walk2: W2, cmp: C) -> IntersectionRevWalk +fn intersection_by( + walk1: W1, + walk2: W2, + cmp: C, +) -> IntersectionRevWalk where I: ?Sized, - W1: RevWalk, - W2: RevWalk, - C: FnMut(&W1::Item, &W2::Item) -> Ordering, + W1: RevWalk>, + W2: RevWalk>, + C: FnMut(&T, &T) -> Ordering, { IntersectionRevWalk { walk1: walk1.peekable(), @@ -664,7 +688,7 @@ where { let mut p1 = self.set1.to_predicate_fn(); let mut p2 = self.set2.to_predicate_fn(); - Box::new(move |index, pos| p1(index, pos) && !p2(index, pos)) + Box::new(move |index, pos| Ok(p1(index, pos)? && !p2(index, pos)?)) } } @@ -677,12 +701,12 @@ struct DifferenceRevWalk, W2: RevWalk, C> { cmp: C, } -impl RevWalk for DifferenceRevWalk +impl RevWalk for DifferenceRevWalk where I: ?Sized, - W1: RevWalk, - W2: RevWalk, - C: FnMut(&W1::Item, &W2::Item) -> Ordering, + W1: RevWalk>, + W2: RevWalk>, + C: FnMut(&T, &T) -> Ordering, { type Item = W1::Item; @@ -695,7 +719,7 @@ where (_, None) => { return self.walk1.next(index); } - (Some(item1), Some(item2)) => match (self.cmp)(item1, item2) { + (Some(Ok(item1)), Some(Ok(item2))) => match (self.cmp)(item1, item2) { Ordering::Less => { return self.walk1.next(index); } @@ -707,17 +731,27 @@ where self.walk2.next(index); } }, + (Some(Err(_)), _) => { + return self.walk1.next(index); + } + (_, Some(Err(_))) => { + return self.walk2.next(index); + } } } } } -fn difference_by(walk1: W1, walk2: W2, cmp: C) -> DifferenceRevWalk +fn difference_by( + walk1: W1, + walk2: W2, + cmp: C, +) -> DifferenceRevWalk where I: ?Sized, - W1: RevWalk, - W2: RevWalk, - C: FnMut(&W1::Item, &W2::Item) -> Ordering, + W1: RevWalk>, + W2: RevWalk>, + C: FnMut(&T, &T) -> Ordering, { DifferenceRevWalk { walk1: walk1.peekable(), @@ -767,7 +801,8 @@ impl EvaluationContext<'_> { ResolvedExpression::Ancestors { heads, generation } => { let head_set = self.evaluate(heads)?; let head_positions = head_set.positions().attach(index); - let builder = RevWalkBuilder::new(index).wanted_heads(head_positions.collect()); + let builder = + RevWalkBuilder::new(index).wanted_heads(head_positions.try_collect()?); if generation == &GENERATION_RANGE_FULL { let walk = builder.ancestors().detach(); Ok(Box::new(RevWalkRevset { walk })) @@ -785,19 +820,19 @@ impl EvaluationContext<'_> { generation, } => { let root_set = self.evaluate(roots)?; - let root_positions = root_set.positions().attach(index).collect_vec(); + let root_positions: Vec<_> = root_set.positions().attach(index).try_collect()?; // Pre-filter heads so queries like 'immutable_heads()..' can // terminate early. immutable_heads() usually includes some // visible heads, which can be trivially rejected. let head_set = self.evaluate(heads)?; let head_positions = difference_by( head_set.positions(), - EagerRevWalk::new(root_positions.iter().copied()), + EagerRevWalk::new(root_positions.iter().copied().map(Ok)), |pos1, pos2| pos1.cmp(pos2).reverse(), ) .attach(index); let builder = RevWalkBuilder::new(index) - .wanted_heads(head_positions.collect()) + .wanted_heads(head_positions.try_collect()?) .unwanted_roots(root_positions); if generation == &GENERATION_RANGE_FULL { let walk = builder.ancestors().detach(); @@ -819,19 +854,20 @@ impl EvaluationContext<'_> { let root_positions = root_set.positions().attach(index); let head_set = self.evaluate(heads)?; let head_positions = head_set.positions().attach(index); - let builder = RevWalkBuilder::new(index).wanted_heads(head_positions.collect()); + let builder = + RevWalkBuilder::new(index).wanted_heads(head_positions.try_collect()?); if generation_from_roots == &(1..2) { - let root_positions: HashSet<_> = root_positions.collect(); + let root_positions: HashSet<_> = root_positions.try_collect()?; let walk = builder .ancestors_until_roots(root_positions.iter().copied()) .detach(); let candidates = RevWalkRevset { walk }; let predicate = as_pure_predicate_fn(move |index, pos| { - index + Ok(index .entry_by_pos(pos) .parent_positions() .iter() - .any(|parent_pos| root_positions.contains(parent_pos)) + .any(|parent_pos| root_positions.contains(parent_pos))) }); // TODO: Suppose heads include all visible heads, ToPredicateFn version can be // optimized to only test the predicate() @@ -840,7 +876,9 @@ impl EvaluationContext<'_> { predicate, })) } else if generation_from_roots == &GENERATION_RANGE_FULL { - let mut positions = builder.descendants(root_positions.collect()).collect_vec(); + let mut positions = builder + .descendants(root_positions.try_collect()?) + .collect_vec(); positions.reverse(); Ok(Box::new(EagerRevset { positions })) } else { @@ -849,7 +887,7 @@ impl EvaluationContext<'_> { // reachable[pos] = (reachable[parent_pos] | ...) << 1 let mut positions = builder .descendants_filtered_by_generation( - root_positions.collect(), + root_positions.try_collect()?, to_u32_generation_range(generation_from_roots)?, ) .map(|Reverse(pos)| pos) @@ -863,7 +901,7 @@ impl EvaluationContext<'_> { // Compute all reachable subgraphs. let domain_revset = self.evaluate(domain)?; - let domain_vec = domain_revset.positions().attach(index).collect_vec(); + let domain_vec: Vec<_> = domain_revset.positions().attach(index).try_collect()?; let domain_set: HashSet<_> = domain_vec.iter().copied().collect(); for pos in &domain_set { for parent_pos in index.entry_by_pos(*pos).parent_positions() { @@ -876,12 +914,12 @@ impl EvaluationContext<'_> { // Identify disjoint sets reachable from sources. let set_reps: HashSet<_> = intersection_by( self.evaluate(sources)?.positions(), - EagerRevWalk::new(domain_vec.iter().copied()), + EagerRevWalk::new(domain_vec.iter().copied().map(Ok)), |pos1, pos2| pos1.cmp(pos2).reverse(), ) .attach(index) - .map(|pos| sets.find(pos)) - .collect(); + .map_ok(|pos| sets.find(pos)) + .try_collect()?; let positions = domain_vec .into_iter() @@ -892,16 +930,16 @@ impl EvaluationContext<'_> { ResolvedExpression::Heads(candidates) => { let candidate_set = self.evaluate(candidates)?; let head_positions: BTreeSet<_> = - index.heads_pos(candidate_set.positions().attach(index).collect()); + index.heads_pos(candidate_set.positions().attach(index).try_collect()?); let positions = head_positions.into_iter().rev().collect(); Ok(Box::new(EagerRevset { positions })) } ResolvedExpression::Roots(candidates) => { - let mut positions = self + let mut positions: Vec<_> = self .evaluate(candidates)? .positions() .attach(index) - .collect_vec(); + .try_collect()?; let filled = RevWalkBuilder::new(index) .wanted_heads(positions.clone()) .descendants(positions.iter().copied().collect()) @@ -917,9 +955,7 @@ impl EvaluationContext<'_> { } ResolvedExpression::Latest { candidates, count } => { let candidate_set = self.evaluate(candidates)?; - Ok(Box::new( - self.take_latest_revset(candidate_set.as_ref(), *count), - )) + Ok(Box::new(self.take_latest_revset(&*candidate_set, *count)?)) } ResolvedExpression::Coalesce(expression1, expression2) => { let set1 = self.evaluate(expression1)?; @@ -1004,9 +1040,13 @@ impl EvaluationContext<'_> { Ok(EagerRevset { positions }) } - fn take_latest_revset(&self, candidate_set: &dyn InternalRevset, count: usize) -> EagerRevset { + fn take_latest_revset( + &self, + candidate_set: &dyn InternalRevset, + count: usize, + ) -> Result { if count == 0 { - return EagerRevset::empty(); + return Ok(EagerRevset::empty()); } #[derive(Clone, Eq, Ord, PartialEq, PartialOrd)] @@ -1015,13 +1055,13 @@ impl EvaluationContext<'_> { pos: IndexPosition, // tie-breaker } - let make_rev_item = |pos| { - let entry = self.index.entry_by_pos(pos); - let commit = self.store.get_commit(&entry.commit_id()).unwrap(); - Reverse(Item { + let make_rev_item = |pos| -> Result<_, RevsetEvaluationError> { + let entry = self.index.entry_by_pos(pos?); + let commit = self.store.get_commit(&entry.commit_id())?; + Ok(Reverse(Item { timestamp: commit.committer().timestamp.timestamp, pos: entry.position(), - }) + })) }; // Maintain min-heap containing the latest (greatest) count items. For small @@ -1032,8 +1072,9 @@ impl EvaluationContext<'_> { .attach(self.index) .map(make_rev_item) .fuse(); - let mut latest_items = BinaryHeap::from_iter(candidate_iter.by_ref().take(count)); + let mut latest_items: BinaryHeap<_> = candidate_iter.by_ref().take(count).try_collect()?; for item in candidate_iter { + let item = item?; let mut earliest = latest_items.peek_mut().unwrap(); if earliest.0 < item.0 { *earliest = item; @@ -1046,7 +1087,7 @@ impl EvaluationContext<'_> { .map(|item| item.0.pos) .collect_vec(); positions.sort_unstable_by_key(|&pos| Reverse(pos)); - EagerRevset { positions } + Ok(EagerRevset { positions }) } } @@ -1060,7 +1101,7 @@ impl fmt::Debug for PurePredicateFn { impl ToPredicateFn for PurePredicateFn where - F: Fn(&CompositeIndex, IndexPosition) -> bool + Clone, + F: Fn(&CompositeIndex, IndexPosition) -> Result + Clone, { fn to_predicate_fn<'a>(&self) -> BoxedPredicateFn<'a> where @@ -1072,13 +1113,13 @@ where fn as_pure_predicate_fn(f: F) -> PurePredicateFn where - F: Fn(&CompositeIndex, IndexPosition) -> bool + Clone, + F: Fn(&CompositeIndex, IndexPosition) -> Result + Clone, { PurePredicateFn(f) } fn box_pure_predicate_fn<'a>( - f: impl Fn(&CompositeIndex, IndexPosition) -> bool + Clone + 'a, + f: impl Fn(&CompositeIndex, IndexPosition) -> Result + Clone + 'a, ) -> Box { Box::new(PurePredicateFn(f)) } @@ -1087,21 +1128,20 @@ fn build_predicate_fn( store: Arc, predicate: &RevsetFilterPredicate, ) -> Box { - // TODO: propagate BackendError match predicate { RevsetFilterPredicate::ParentCount(parent_count_range) => { let parent_count_range = parent_count_range.clone(); box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - parent_count_range.contains(&entry.num_parents()) + Ok(parent_count_range.contains(&entry.num_parents())) }) } RevsetFilterPredicate::Description(pattern) => { let pattern = pattern.clone(); box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - pattern.matches(commit.description()) + let commit = store.get_commit(&entry.commit_id())?; + Ok(pattern.matches(commit.description())) }) } RevsetFilterPredicate::Author(pattern) => { @@ -1110,43 +1150,44 @@ fn build_predicate_fn( // syntax for specifying whether it's a regex. box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - pattern.matches(&commit.author().name) || pattern.matches(&commit.author().email) + let commit = store.get_commit(&entry.commit_id())?; + Ok(pattern.matches(&commit.author().name) + || pattern.matches(&commit.author().email)) }) } RevsetFilterPredicate::Committer(pattern) => { let pattern = pattern.clone(); box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - pattern.matches(&commit.committer().name) - || pattern.matches(&commit.committer().email) + let commit = store.get_commit(&entry.commit_id())?; + Ok(pattern.matches(&commit.committer().name) + || pattern.matches(&commit.committer().email)) }) } RevsetFilterPredicate::AuthorDate(expression) => { let expression = *expression; box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); + let commit = store.get_commit(&entry.commit_id())?; let author_date = &commit.author().timestamp; - expression.matches(author_date) + Ok(expression.matches(author_date)) }) } RevsetFilterPredicate::CommitterDate(expression) => { let expression = *expression; box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); + let commit = store.get_commit(&entry.commit_id())?; let committer_date = &commit.committer().timestamp; - expression.matches(committer_date) + Ok(expression.matches(committer_date)) }) } RevsetFilterPredicate::File(expr) => { let matcher: Rc = expr.to_matcher().into(); box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - has_diff_from_parent(&store, index, &commit, matcher.as_ref()).unwrap() + let commit = store.get_commit(&entry.commit_id())?; + Ok(has_diff_from_parent(&store, index, &commit, &*matcher)?) }) } RevsetFilterPredicate::DiffContains { text, files } => { @@ -1154,22 +1195,27 @@ fn build_predicate_fn( let files_matcher: Rc = files.to_matcher().into(); box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - matches_diff_from_parent(&store, index, &commit, &text_pattern, &*files_matcher) - .unwrap() + let commit = store.get_commit(&entry.commit_id())?; + Ok(matches_diff_from_parent( + &store, + index, + &commit, + &text_pattern, + &*files_matcher, + )?) }) } RevsetFilterPredicate::HasConflict => box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - commit.has_conflict().unwrap() + let commit = store.get_commit(&entry.commit_id())?; + Ok(commit.has_conflict()?) }), RevsetFilterPredicate::Extension(ext) => { let ext = ext.clone(); box_pure_predicate_fn(move |index, pos| { let entry = index.entry_by_pos(pos); - let commit = store.get_commit(&entry.commit_id()).unwrap(); - ext.matches_commit(&commit) + let commit = store.get_commit(&entry.commit_id())?; + Ok(ext.matches_commit(&commit)) }) } } @@ -1307,6 +1353,10 @@ mod tests { move || iter.next().unwrap() } + fn try_collect_vec(iter: impl IntoIterator>) -> Result, E> { + iter.into_iter().collect() + } + #[test] fn test_revset_combinator() { let mut new_change_id = change_id_generator(); @@ -1332,33 +1382,33 @@ mod tests { let set = make_set(&[&id_4, &id_3, &id_2, &id_0]); let mut p = set.to_predicate_fn(); - assert!(p(index, get_pos(&id_4))); - assert!(p(index, get_pos(&id_3))); - assert!(p(index, get_pos(&id_2))); - assert!(!p(index, get_pos(&id_1))); - assert!(p(index, get_pos(&id_0))); + assert!(p(index, get_pos(&id_4)).unwrap()); + assert!(p(index, get_pos(&id_3)).unwrap()); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).unwrap()); // Uninteresting entries can be skipped let mut p = set.to_predicate_fn(); - assert!(p(index, get_pos(&id_3))); - assert!(!p(index, get_pos(&id_1))); - assert!(p(index, get_pos(&id_0))); + assert!(p(index, get_pos(&id_3)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).unwrap()); let set = FilterRevset { candidates: make_set(&[&id_4, &id_2, &id_0]), predicate: as_pure_predicate_fn(|index, pos| { - index.entry_by_pos(pos).commit_id() != id_4 + Ok(index.entry_by_pos(pos).commit_id() != id_4) }), }; assert_eq!( - set.positions().attach(index).collect_vec(), + try_collect_vec(set.positions().attach(index)).unwrap(), make_positions(&[&id_2, &id_0]) ); let mut p = set.to_predicate_fn(); - assert!(!p(index, get_pos(&id_4))); - assert!(!p(index, get_pos(&id_3))); - assert!(p(index, get_pos(&id_2))); - assert!(!p(index, get_pos(&id_1))); - assert!(p(index, get_pos(&id_0))); + assert!(!p(index, get_pos(&id_4)).unwrap()); + assert!(!p(index, get_pos(&id_3)).unwrap()); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).unwrap()); // Intersection by FilterRevset let set = FilterRevset { @@ -1366,60 +1416,194 @@ mod tests { predicate: make_set(&[&id_3, &id_2, &id_1]), }; assert_eq!( - set.positions().attach(index).collect_vec(), + try_collect_vec(set.positions().attach(index)).unwrap(), make_positions(&[&id_2]) ); let mut p = set.to_predicate_fn(); - assert!(!p(index, get_pos(&id_4))); - assert!(!p(index, get_pos(&id_3))); - assert!(p(index, get_pos(&id_2))); - assert!(!p(index, get_pos(&id_1))); - assert!(!p(index, get_pos(&id_0))); + assert!(!p(index, get_pos(&id_4)).unwrap()); + assert!(!p(index, get_pos(&id_3)).unwrap()); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(!p(index, get_pos(&id_0)).unwrap()); let set = UnionRevset { set1: make_set(&[&id_4, &id_2]), set2: make_set(&[&id_3, &id_2, &id_1]), }; assert_eq!( - set.positions().attach(index).collect_vec(), + try_collect_vec(set.positions().attach(index)).unwrap(), make_positions(&[&id_4, &id_3, &id_2, &id_1]) ); let mut p = set.to_predicate_fn(); - assert!(p(index, get_pos(&id_4))); - assert!(p(index, get_pos(&id_3))); - assert!(p(index, get_pos(&id_2))); - assert!(p(index, get_pos(&id_1))); - assert!(!p(index, get_pos(&id_0))); + assert!(p(index, get_pos(&id_4)).unwrap()); + assert!(p(index, get_pos(&id_3)).unwrap()); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).unwrap()); + assert!(!p(index, get_pos(&id_0)).unwrap()); let set = IntersectionRevset { set1: make_set(&[&id_4, &id_2, &id_0]), set2: make_set(&[&id_3, &id_2, &id_1]), }; assert_eq!( - set.positions().attach(index).collect_vec(), + try_collect_vec(set.positions().attach(index)).unwrap(), make_positions(&[&id_2]) ); let mut p = set.to_predicate_fn(); - assert!(!p(index, get_pos(&id_4))); - assert!(!p(index, get_pos(&id_3))); - assert!(p(index, get_pos(&id_2))); - assert!(!p(index, get_pos(&id_1))); - assert!(!p(index, get_pos(&id_0))); + assert!(!p(index, get_pos(&id_4)).unwrap()); + assert!(!p(index, get_pos(&id_3)).unwrap()); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(!p(index, get_pos(&id_0)).unwrap()); let set = DifferenceRevset { set1: make_set(&[&id_4, &id_2, &id_0]), set2: make_set(&[&id_3, &id_2, &id_1]), }; assert_eq!( - set.positions().attach(index).collect_vec(), + try_collect_vec(set.positions().attach(index)).unwrap(), make_positions(&[&id_4, &id_0]) ); let mut p = set.to_predicate_fn(); - assert!(p(index, get_pos(&id_4))); - assert!(!p(index, get_pos(&id_3))); - assert!(!p(index, get_pos(&id_2))); - assert!(!p(index, get_pos(&id_1))); - assert!(p(index, get_pos(&id_0))); + assert!(p(index, get_pos(&id_4)).unwrap()); + assert!(!p(index, get_pos(&id_3)).unwrap()); + assert!(!p(index, get_pos(&id_2)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).unwrap()); + } + + #[test] + fn test_revset_combinator_error_propagation() { + let mut new_change_id = change_id_generator(); + let mut index = DefaultMutableIndex::full(3, 16); + let id_0 = CommitId::from_hex("000000"); + let id_1 = CommitId::from_hex("111111"); + let id_2 = CommitId::from_hex("222222"); + index.add_commit_data(id_0.clone(), new_change_id(), &[]); + index.add_commit_data(id_1.clone(), new_change_id(), &[id_0.clone()]); + index.add_commit_data(id_2.clone(), new_change_id(), &[id_1.clone()]); + + let index = index.as_composite(); + let get_pos = |id: &CommitId| index.commit_id_to_pos(id).unwrap(); + let make_positions = |ids: &[&CommitId]| ids.iter().copied().map(get_pos).collect_vec(); + let make_good_set = |ids: &[&CommitId]| -> Box { + let positions = make_positions(ids); + Box::new(EagerRevset { positions }) + }; + let make_bad_set = |ids: &[&CommitId], bad_id: &CommitId| -> Box { + let positions = make_positions(ids); + let bad_id = bad_id.clone(); + Box::new(FilterRevset { + candidates: EagerRevset { positions }, + predicate: as_pure_predicate_fn(move |index, pos| { + if index.entry_by_pos(pos).commit_id() == bad_id { + Err(RevsetEvaluationError::Other("bad".into())) + } else { + Ok(true) + } + }), + }) + }; + + // Error from filter predicate + let set = make_bad_set(&[&id_2, &id_1, &id_0], &id_1); + assert_eq!( + try_collect_vec(set.positions().attach(index).take(1)).unwrap(), + make_positions(&[&id_2]) + ); + assert!(try_collect_vec(set.positions().attach(index).take(2)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).is_err()); + + // Error from filter candidates + let set = FilterRevset { + candidates: make_bad_set(&[&id_2, &id_1, &id_0], &id_1), + predicate: as_pure_predicate_fn(|_, _| Ok(true)), + }; + assert_eq!( + try_collect_vec(set.positions().attach(index).take(1)).unwrap(), + make_positions(&[&id_2]) + ); + assert!(try_collect_vec(set.positions().attach(index).take(2)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).is_err()); + + // Error from left side of union, immediately + let set = UnionRevset { + set1: make_bad_set(&[&id_1], &id_1), + set2: make_good_set(&[&id_2, &id_1]), + }; + assert!(try_collect_vec(set.positions().attach(index).take(1)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_2)).unwrap()); // works because bad id isn't visited + assert!(p(index, get_pos(&id_1)).is_err()); + + // Error from right side of union, lazily + let set = UnionRevset { + set1: make_good_set(&[&id_2, &id_1]), + set2: make_bad_set(&[&id_1, &id_0], &id_0), + }; + assert_eq!( + try_collect_vec(set.positions().attach(index).take(2)).unwrap(), + make_positions(&[&id_2, &id_1]) + ); + assert!(try_collect_vec(set.positions().attach(index).take(3)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).is_err()); + + // Error from left side of intersection, immediately + let set = IntersectionRevset { + set1: make_bad_set(&[&id_1], &id_1), + set2: make_good_set(&[&id_2, &id_1]), + }; + assert!(try_collect_vec(set.positions().attach(index).take(1)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(!p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).is_err()); + + // Error from right side of intersection, lazily + let set = IntersectionRevset { + set1: make_good_set(&[&id_2, &id_1, &id_0]), + set2: make_bad_set(&[&id_1, &id_0], &id_0), + }; + assert_eq!( + try_collect_vec(set.positions().attach(index).take(1)).unwrap(), + make_positions(&[&id_1]) + ); + assert!(try_collect_vec(set.positions().attach(index).take(2)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(!p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).is_err()); + + // Error from left side of difference, immediately + let set = DifferenceRevset { + set1: make_bad_set(&[&id_1], &id_1), + set2: make_good_set(&[&id_2, &id_1]), + }; + assert!(try_collect_vec(set.positions().attach(index).take(1)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(!p(index, get_pos(&id_2)).unwrap()); + assert!(p(index, get_pos(&id_1)).is_err()); + + // Error from right side of difference, lazily + let set = DifferenceRevset { + set1: make_good_set(&[&id_2, &id_1, &id_0]), + set2: make_bad_set(&[&id_1, &id_0], &id_0), + }; + assert_eq!( + try_collect_vec(set.positions().attach(index).take(1)).unwrap(), + make_positions(&[&id_2]) + ); + assert!(try_collect_vec(set.positions().attach(index).take(2)).is_err()); + let mut p = set.to_predicate_fn(); + assert!(p(index, get_pos(&id_2)).unwrap()); + assert!(!p(index, get_pos(&id_1)).unwrap()); + assert!(p(index, get_pos(&id_0)).is_err()); } #[test] @@ -1450,34 +1634,36 @@ mod tests { // Consumes entries incrementally let positions_accum = PositionsAccumulator::new(index, full_set.positions()); - assert!(positions_accum.contains(&id_3)); + assert!(positions_accum.contains(&id_3).unwrap()); assert_eq!(positions_accum.consumed_len(), 2); - assert!(positions_accum.contains(&id_0)); + assert!(positions_accum.contains(&id_0).unwrap()); assert_eq!(positions_accum.consumed_len(), 5); - assert!(positions_accum.contains(&id_3)); + assert!(positions_accum.contains(&id_3).unwrap()); assert_eq!(positions_accum.consumed_len(), 5); // Does not consume positions for unknown commits let positions_accum = PositionsAccumulator::new(index, full_set.positions()); - assert!(!positions_accum.contains(&CommitId::from_hex("999999"))); + assert!(!positions_accum + .contains(&CommitId::from_hex("999999")) + .unwrap()); assert_eq!(positions_accum.consumed_len(), 0); // Does not consume without necessity let set = make_set(&[&id_3, &id_2, &id_1]); let positions_accum = PositionsAccumulator::new(index, set.positions()); - assert!(!positions_accum.contains(&id_4)); + assert!(!positions_accum.contains(&id_4).unwrap()); assert_eq!(positions_accum.consumed_len(), 1); - assert!(positions_accum.contains(&id_3)); + assert!(positions_accum.contains(&id_3).unwrap()); assert_eq!(positions_accum.consumed_len(), 1); - assert!(!positions_accum.contains(&id_0)); + assert!(!positions_accum.contains(&id_0).unwrap()); assert_eq!(positions_accum.consumed_len(), 3); - assert!(positions_accum.contains(&id_1)); + assert!(positions_accum.contains(&id_1).unwrap()); } } diff --git a/lib/src/default_index/revset_graph_iterator.rs b/lib/src/default_index/revset_graph_iterator.rs index 1a9cc8c7d9..45296ec78f 100644 --- a/lib/src/default_index/revset_graph_iterator.rs +++ b/lib/src/default_index/revset_graph_iterator.rs @@ -29,6 +29,7 @@ use crate::backend::CommitId; use crate::graph::GraphEdge; use crate::graph::GraphEdgeType; use crate::graph::GraphNode; +use crate::revset::RevsetEvaluationError; // This can be cheaply allocated and hashed compared to `CommitId`-based type. type IndexGraphEdge = GraphEdge; @@ -124,36 +125,40 @@ impl<'a> RevsetGraphWalk<'a> { } } - fn next_index_position(&mut self, index: &CompositeIndex) -> Option { - self.look_ahead - .pop_back() - .or_else(|| self.input_set_walk.next(index)) + fn next_index_position( + &mut self, + index: &CompositeIndex, + ) -> Result, RevsetEvaluationError> { + match self.look_ahead.pop_back() { + Some(position) => Ok(Some(position)), + None => self.input_set_walk.next(index).transpose(), + } } fn edges_from_internal_commit( &mut self, index: &CompositeIndex, index_entry: &IndexEntry, - ) -> &[IndexGraphEdge] { + ) -> Result<&[IndexGraphEdge], RevsetEvaluationError> { let position = index_entry.position(); // `if let Some(edges) = ...` doesn't pass lifetime check as of Rust 1.76.0 if self.edges.contains_key(&position) { - return self.edges.get(&position).unwrap(); + return Ok(self.edges.get(&position).unwrap()); } - let edges = self.new_edges_from_internal_commit(index, index_entry); - self.edges.entry(position).or_insert(edges) + let edges = self.new_edges_from_internal_commit(index, index_entry)?; + Ok(self.edges.entry(position).or_insert(edges)) } fn pop_edges_from_internal_commit( &mut self, index: &CompositeIndex, index_entry: &IndexEntry, - ) -> Vec { + ) -> Result, RevsetEvaluationError> { let position = index_entry.position(); while let Some(entry) = self.edges.last_entry() { match entry.key().cmp(&position) { Ordering::Less => break, // no cached edges found - Ordering::Equal => return entry.remove(), + Ordering::Equal => return Ok(entry.remove()), Ordering::Greater => entry.remove(), }; } @@ -164,16 +169,16 @@ impl<'a> RevsetGraphWalk<'a> { &mut self, index: &CompositeIndex, index_entry: &IndexEntry, - ) -> Vec { + ) -> Result, RevsetEvaluationError> { let mut edges = Vec::new(); let mut known_ancestors = HashSet::new(); for parent in index_entry.parents() { let parent_position = parent.position(); - self.consume_to(index, parent_position); + self.consume_to(index, parent_position)?; if self.look_ahead.binary_search(&parent_position).is_ok() { edges.push(IndexGraphEdge::direct(parent_position)); } else { - let parent_edges = self.edges_from_external_commit(index, parent); + let parent_edges = self.edges_from_external_commit(index, parent)?; if parent_edges .iter() .all(|edge| edge.edge_type == GraphEdgeType::Missing) @@ -188,14 +193,14 @@ impl<'a> RevsetGraphWalk<'a> { } } } - edges + Ok(edges) } fn edges_from_external_commit( &mut self, index: &CompositeIndex, index_entry: IndexEntry<'_>, - ) -> &[IndexGraphEdge] { + ) -> Result<&[IndexGraphEdge], RevsetEvaluationError> { let position = index_entry.position(); let mut stack = vec![index_entry]; while let Some(entry) = stack.last() { @@ -209,7 +214,7 @@ impl<'a> RevsetGraphWalk<'a> { let mut parents_complete = true; for parent in entry.parents() { let parent_position = parent.position(); - self.consume_to(index, parent_position); + self.consume_to(index, parent_position)?; if self.look_ahead.binary_search(&parent_position).is_ok() { // We have found a path back into the input set edges.push(IndexGraphEdge::indirect(parent_position)); @@ -241,19 +246,19 @@ impl<'a> RevsetGraphWalk<'a> { self.edges.insert(position, edges); } } - self.edges.get(&position).unwrap() + Ok(self.edges.get(&position).unwrap()) } fn remove_transitive_edges( &mut self, index: &CompositeIndex, edges: Vec, - ) -> Vec { + ) -> Result, RevsetEvaluationError> { if !edges .iter() .any(|edge| edge.edge_type == GraphEdgeType::Indirect) { - return edges; + return Ok(edges); } let mut min_generation = u32::MAX; let mut initial_targets = HashSet::new(); @@ -265,7 +270,7 @@ impl<'a> RevsetGraphWalk<'a> { assert!(self.look_ahead.binary_search(&edge.target).is_ok()); let entry = index.entry_by_pos(edge.target); min_generation = min(min_generation, entry.generation_number()); - work.extend_from_slice(self.edges_from_internal_commit(index, &entry)); + work.extend_from_slice(self.edges_from_internal_commit(index, &entry)?); } } // Find commits reachable transitively and add them to the `unwanted` set. @@ -287,41 +292,55 @@ impl<'a> RevsetGraphWalk<'a> { if entry.generation_number() < min_generation { continue; } - work.extend_from_slice(self.edges_from_internal_commit(index, &entry)); + work.extend_from_slice(self.edges_from_internal_commit(index, &entry)?); } - edges + Ok(edges .into_iter() .filter(|edge| !unwanted.contains(&edge.target)) - .collect() + .collect()) } - fn consume_to(&mut self, index: &CompositeIndex, pos: IndexPosition) { + fn consume_to( + &mut self, + index: &CompositeIndex, + pos: IndexPosition, + ) -> Result<(), RevsetEvaluationError> { while pos < self.min_position { - if let Some(next_position) = self.input_set_walk.next(index) { + if let Some(next_position) = self.input_set_walk.next(index).transpose()? { self.look_ahead.push_front(next_position); self.min_position = next_position; } else { break; } } + Ok(()) } -} - -impl RevWalk for RevsetGraphWalk<'_> { - type Item = GraphNode; - fn next(&mut self, index: &CompositeIndex) -> Option { - let position = self.next_index_position(index)?; + fn try_next( + &mut self, + index: &CompositeIndex, + ) -> Result>, RevsetEvaluationError> { + let Some(position) = self.next_index_position(index)? else { + return Ok(None); + }; let entry = index.entry_by_pos(position); - let mut edges = self.pop_edges_from_internal_commit(index, &entry); + let mut edges = self.pop_edges_from_internal_commit(index, &entry)?; if self.skip_transitive_edges { - edges = self.remove_transitive_edges(index, edges); + edges = self.remove_transitive_edges(index, edges)?; } let edges = edges .iter() .map(|edge| edge.map(|pos| index.entry_by_pos(pos).commit_id())) .collect(); - Some((entry.commit_id(), edges)) + Ok(Some((entry.commit_id(), edges))) + } +} + +impl RevWalk for RevsetGraphWalk<'_> { + type Item = Result, RevsetEvaluationError>; + + fn next(&mut self, index: &CompositeIndex) -> Option { + self.try_next(index).transpose() } }