diff --git a/cli/src/commands/bookmark/move.rs b/cli/src/commands/bookmark/move.rs index 25a394f627..6bd3e825da 100644 --- a/cli/src/commands/bookmark/move.rs +++ b/cli/src/commands/bookmark/move.rs @@ -13,7 +13,6 @@ // limitations under the License. use itertools::Itertools as _; -use jj_lib::backend::CommitId; use jj_lib::object_id::ObjectId as _; use jj_lib::op_store::RefTarget; use jj_lib::str_util::StringPattern; @@ -77,26 +76,41 @@ pub fn cmd_bookmark_move( let target_commit = workspace_command.resolve_single_rev(ui, &args.to)?; let matched_bookmarks = { - let is_source_commit = if !args.from.is_empty() { - workspace_command + let is_source_ref: Box _> = if !args.from.is_empty() { + let is_source_commit = workspace_command .parse_union_revsets(ui, &args.from)? .evaluate()? - .containing_fn() + .containing_fn(); + Box::new(move |target: &RefTarget| { + for id in target.added_ids() { + if is_source_commit(id)? { + return Ok(true); + } + } + Ok(false) + }) } else { - Box::new(|_: &CommitId| true) + Box::new(|_| Ok(true)) }; let mut bookmarks = if !args.names.is_empty() { find_bookmarks_with(&args.names, |pattern| { repo.view() .local_bookmarks_matching(pattern) - .filter(|(_, target)| target.added_ids().any(&is_source_commit)) - .map(Ok) + .filter_map(|(name, target)| { + is_source_ref(target) + .map(|matched| matched.then_some((name, target))) + .transpose() + }) })? } else { repo.view() .local_bookmarks() - .filter(|(_, target)| target.added_ids().any(&is_source_commit)) - .collect() + .filter_map(|(name, target)| { + is_source_ref(target) + .map(|matched| matched.then_some((name, target))) + .transpose() + }) + .try_collect()? }; // Noop matches aren't error, but should be excluded from stats. bookmarks.retain(|(_, old_target)| old_target.as_normal() != Some(target_commit.id())); diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index 72ae3f0691..c8295c9624 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -336,7 +336,7 @@ fn validate_commits_ready_to_push( .evaluate()? .containing_fn() } else { - Box::new(|_: &CommitId| false) + Box::new(|_: &CommitId| Ok(false)) }; for commit in workspace_helper @@ -362,7 +362,7 @@ fn validate_commits_ready_to_push( if commit.has_conflict()? { reasons.push("it has conflicts"); } - if !args.allow_private && is_private(commit.id()) { + if !args.allow_private && is_private(commit.id())? { reasons.push("it is private"); } if !reasons.is_empty() { diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 2169e7e8ee..60b36d31de 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -743,7 +743,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm .keyword_cache .is_immutable_fn(language, function.name_span)? .clone(); - let out_property = self_property.map(move |commit| is_immutable(commit.id())); + let out_property = self_property.and_then(move |commit| Ok(is_immutable(commit.id())?)); Ok(L::wrap_boolean(out_property)) }, ); @@ -757,7 +757,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm Ok(evaluate_user_revset(language, diagnostics, span, revset)?.containing_fn()) })?; - let out_property = self_property.map(move |commit| is_contained(commit.id())); + let out_property = self_property.and_then(move |commit| Ok(is_contained(commit.id())?)); Ok(L::wrap_boolean(out_property)) }, ); @@ -1020,7 +1020,7 @@ impl RefName { .get_or_try_init(|| { let self_ids = self.target.added_ids().cloned().collect_vec(); let other_ids = tracking.target.added_ids().cloned().collect_vec(); - Ok(revset::walk_revs(repo, &self_ids, &other_ids)?.count_estimate()) + Ok(revset::walk_revs(repo, &self_ids, &other_ids)?.count_estimate()?) }) .copied() } @@ -1035,7 +1035,7 @@ impl RefName { .get_or_try_init(|| { let self_ids = self.target.added_ids().cloned().collect_vec(); let other_ids = tracking.target.added_ids().cloned().collect_vec(); - Ok(revset::walk_revs(repo, &other_ids, &self_ids)?.count_estimate()) + Ok(revset::walk_revs(repo, &other_ids, &self_ids)?.count_estimate()?) }) .copied() } diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index 8663c0cb28..7a03102268 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -191,20 +191,21 @@ impl Revset for RevsetImpl { self.positions().next().is_none() } - fn count_estimate(&self) -> (usize, Option) { + fn count_estimate(&self) -> Result<(usize, Option), RevsetEvaluationError> { if cfg!(feature = "testing") { // 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(); if count < 10 { - (count, Some(count)) + Ok((count, Some(count))) } else { - (10, None) + Ok((10, None)) } } else { + // TODO: propagate error let count = self.positions().count(); - (count, Some(count)) + Ok((count, Some(count))) } } @@ -213,7 +214,7 @@ impl Revset for RevsetImpl { Self: 'a, { let positions = PositionsAccumulator::new(self.index.clone(), self.inner.positions()); - Box::new(move |commit_id| positions.contains(commit_id)) + Box::new(move |commit_id| Ok(positions.contains(commit_id))) } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index a11b31f5d2..79ace543a6 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -2207,13 +2207,14 @@ pub trait Revset: fmt::Debug { where Self: 'a; + /// Returns true if iterator will emit at least one commit or error. fn is_empty(&self) -> bool; /// Inclusive lower bound and, optionally, inclusive upper bound of how many /// commits are in the revset. The implementation can use its discretion as /// to how much effort should be put into the estimation, and how accurate /// the resulting estimate should be. - fn count_estimate(&self) -> (usize, Option); + fn count_estimate(&self) -> Result<(usize, Option), RevsetEvaluationError>; /// Returns a closure that checks if a commit is contained within the /// revset. @@ -2226,7 +2227,7 @@ pub trait Revset: fmt::Debug { } /// Function that checks if a commit is contained within the revset. -pub type RevsetContainingFn<'a> = dyn Fn(&CommitId) -> bool + 'a; +pub type RevsetContainingFn<'a> = dyn Fn(&CommitId) -> Result + 'a; pub trait RevsetIteratorExt<'index, I> { fn commits(self, store: &Arc) -> RevsetCommitIterator; diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 61e2eb7141..29ee125df2 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -3765,8 +3765,8 @@ fn test_revset_containing_fn() { let revset = revset_for_commits(repo.as_ref(), &[&commit_b, &commit_d]); let revset_has_commit = revset.containing_fn(); - assert!(!revset_has_commit(commit_a.id())); - assert!(revset_has_commit(commit_b.id())); - assert!(!revset_has_commit(commit_c.id())); - assert!(revset_has_commit(commit_d.id())); + assert!(!revset_has_commit(commit_a.id()).unwrap()); + assert!(revset_has_commit(commit_b.id()).unwrap()); + assert!(!revset_has_commit(commit_c.id()).unwrap()); + assert!(revset_has_commit(commit_d.id()).unwrap()); }