revset: resolve visible heads prior to evaluation

lib/src/default_index_store.rs

@@ -731,15 +731,9 @@ impl Index for MutableIndexImpl {
         &'index self,
         expression: &ResolvedExpression,
         store: &Arc<Store>,
-        visible_heads: &[CommitId],
     ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError> {
-        let revset_impl = default_revset_engine::evaluate(
+        let revset_impl =
-            expression,
+            default_revset_engine::evaluate(expression, store, self, CompositeIndex(self))?;
-            store,
-            self,
-            CompositeIndex(self),
-            visible_heads,
-        )?;
         Ok(Box::new(revset_impl))
     }
 }
@@ -1819,15 +1813,9 @@ impl Index for ReadonlyIndexImpl {
         &'index self,
         expression: &ResolvedExpression,
         store: &Arc<Store>,
-        visible_heads: &[CommitId],
     ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError> {
-        let revset_impl = default_revset_engine::evaluate(
+        let revset_impl =
-            expression,
+            default_revset_engine::evaluate(expression, store, self, CompositeIndex(self))?;
-            store,
-            self,
-            CompositeIndex(self),
-            visible_heads,
-        )?;
         Ok(Box::new(revset_impl))
     }
 }

lib/src/default_revset_engine.rs

@@ -549,26 +549,23 @@ pub fn evaluate<'index>(
     store: &Arc<Store>,
     index: &'index dyn Index,
     composite_index: CompositeIndex<'index>,
-    visible_heads: &[CommitId],
 ) -> Result<RevsetImpl<'index>, RevsetEvaluationError> {
     let context = EvaluationContext {
         store: store.clone(),
         index,
         composite_index: composite_index.clone(),
-        visible_heads,
     };
     let internal_revset = context.evaluate(expression)?;
     Ok(RevsetImpl::new(internal_revset, composite_index))
 }
 
-struct EvaluationContext<'index, 'heads> {
+struct EvaluationContext<'index> {
     store: Arc<Store>,
     index: &'index dyn Index,
     composite_index: CompositeIndex<'index>,
-    visible_heads: &'heads [CommitId],
 }
 
-impl<'index, 'heads> EvaluationContext<'index, 'heads> {
+impl<'index> EvaluationContext<'index> {
     fn evaluate(
         &self,
         expression: &ResolvedExpression,
@@ -630,9 +627,6 @@ impl<'index, 'heads> EvaluationContext<'index, 'heads> {
                 let (dag_range_set, _) = self.collect_dag_range(&*root_set, &*head_set);
                 Ok(Box::new(dag_range_set))
             }
-            ResolvedExpression::VisibleHeads => {
-                Ok(Box::new(self.revset_for_commit_ids(self.visible_heads)))
-            }
             ResolvedExpression::Heads(candidates) => {
                 let candidate_set = self.evaluate(candidates)?;
                 let candidate_ids = candidate_set

lib/src/index.rs

@@ -72,7 +72,6 @@ pub trait Index: Send + Sync {
         &'index self,
         expression: &ResolvedExpression,
         store: &Arc<Store>,
-        visible_heads: &[CommitId],
     ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError>;
 }
 

lib/src/revset.rs

@@ -197,7 +197,7 @@ pub const GENERATION_RANGE_EMPTY: Range<u32> = 0..0;
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub enum RevsetCommitRef {
     Symbol(String),
-    // TODO: VisibleHeads
+    VisibleHeads,
     Branches(String),
     RemoteBranches {
         branch_needle: String,
@@ -248,7 +248,6 @@ pub enum RevsetExpression {
     },
     Heads(Rc<RevsetExpression>),
     Roots(Rc<RevsetExpression>),
-    VisibleHeads,
     Latest {
         candidates: Rc<RevsetExpression>,
         count: usize,
@@ -285,7 +284,7 @@ impl RevsetExpression {
     }
 
     pub fn visible_heads() -> Rc<RevsetExpression> {
-        Rc::new(RevsetExpression::VisibleHeads)
+        Rc::new(RevsetExpression::CommitRef(RevsetCommitRef::VisibleHeads))
     }
 
     pub fn branches(needle: String) -> Rc<RevsetExpression> {
@@ -482,7 +481,6 @@ pub enum ResolvedExpression {
     },
     Heads(Box<ResolvedExpression>),
     Roots(Box<ResolvedExpression>),
-    VisibleHeads, // TODO: should be substituted at resolve_visibility()
     Latest {
         candidates: Box<ResolvedExpression>,
         count: usize,
@@ -503,11 +501,7 @@ impl ResolvedExpression {
         &self,
         repo: &'index dyn Repo,
     ) -> Result<Box<dyn Revset<'index> + 'index>, RevsetEvaluationError> {
-        repo.index().evaluate_revset(
+        repo.index().evaluate_revset(self, repo.store())
-            self,
-            repo.store(),
-            &repo.view().heads().iter().cloned().collect_vec(),
-        )
     }
 }
 
@@ -1262,7 +1256,6 @@ fn try_transform_expression<E>(
                 transform_rec_pair((roots, heads), pre, post)?
                     .map(|(roots, heads)| RevsetExpression::DagRange { roots, heads })
             }
-            RevsetExpression::VisibleHeads => None,
             RevsetExpression::Heads(candidates) => {
                 transform_rec(candidates, pre, post)?.map(RevsetExpression::Heads)
             }
@@ -1709,6 +1702,7 @@ fn resolve_commit_ref(
         RevsetCommitRef::Symbol(symbol) => {
             resolve_symbol(repo, symbol, workspace_ctx.map(|ctx| ctx.workspace_id))
         }
+        RevsetCommitRef::VisibleHeads => Ok(repo.view().heads().iter().cloned().collect_vec()),
         RevsetCommitRef::Branches(needle) => {
             let mut commit_ids = vec![];
             for (branch_name, branch_target) in repo.view().branches() {
@@ -1803,17 +1797,22 @@ fn resolve_symbols(
 /// commit ids to make `all()` include hidden-but-specified commits. The
 /// return type `ResolvedExpression` is stricter than `RevsetExpression`,
 /// and isn't designed for such transformation.
-fn resolve_visibility(_repo: &dyn Repo, expression: &RevsetExpression) -> ResolvedExpression {
+fn resolve_visibility(repo: &dyn Repo, expression: &RevsetExpression) -> ResolvedExpression {
-    let context = VisibilityResolutionContext {};
+    // If we add "operation" scope (#1283), visible_heads might be translated to
+    // `RevsetExpression::WithinOperation(visible_heads, expression)` node to
+    // evaluate filter predicates and "all()" against that scope.
+    let context = VisibilityResolutionContext {
+        visible_heads: &repo.view().heads().iter().cloned().collect_vec(),
+    };
     context.resolve(expression)
 }
 
 #[derive(Clone, Debug)]
-struct VisibilityResolutionContext {
+struct VisibilityResolutionContext<'a> {
-    // TODO: visible_heads
+    visible_heads: &'a [CommitId],
 }
 
-impl VisibilityResolutionContext {
+impl VisibilityResolutionContext<'_> {
     /// Resolves expression tree as set.
     fn resolve(&self, expression: &RevsetExpression) -> ResolvedExpression {
         match expression {
@@ -1852,7 +1851,6 @@ impl VisibilityResolutionContext {
             RevsetExpression::Roots(candidates) => {
                 ResolvedExpression::Roots(self.resolve(candidates).into())
             }
-            RevsetExpression::VisibleHeads => self.resolve_visible_heads(),
             RevsetExpression::Latest { candidates, count } => ResolvedExpression::Latest {
                 candidates: self.resolve(candidates).into(),
                 count: *count,
@@ -1914,7 +1912,7 @@ impl VisibilityResolutionContext {
     }
 
     fn resolve_visible_heads(&self) -> ResolvedExpression {
-        ResolvedExpression::VisibleHeads
+        ResolvedExpression::Commits(self.visible_heads.to_owned())
     }
 
     /// Resolves expression tree as filter predicate.
@@ -1933,7 +1931,6 @@ impl VisibilityResolutionContext {
             | RevsetExpression::DagRange { .. }
             | RevsetExpression::Heads(_)
             | RevsetExpression::Roots(_)
-            | RevsetExpression::VisibleHeads
             | RevsetExpression::Latest { .. } => {
                 ResolvedPredicateExpression::Set(self.resolve(expression).into())
             }
@@ -2982,7 +2979,9 @@ mod tests {
                     "foo",
                 ),
             ),
-            heads: VisibleHeads,
+            heads: CommitRef(
+                VisibleHeads,
+            ),
             generation: 0..4294967295,
         }
         "###);

lib/tests/test_default_revset_graph_iterator.rs

@@ -29,14 +29,7 @@ fn revset_for_commits<'index>(repo: &'index dyn Repo, commits: &[&Commit]) -> Re
         .unwrap();
     let expression =
         ResolvedExpression::Commits(commits.iter().map(|commit| commit.id().clone()).collect());
-    evaluate(
+    evaluate(&expression, repo.store(), index, index.as_composite()).unwrap()
-        &expression,
-        repo.store(),
-        index,
-        index.as_composite(),
-        &repo.view().heads().iter().cloned().collect_vec(),
-    )
-    .unwrap()
 }
 
 fn direct(commit: &Commit) -> RevsetGraphEdge {