From 5b52617b344d8bdc71d1403baa82a68c624a335e Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Sun, 2 Apr 2023 13:41:55 +0900
Subject: [PATCH] revset: fully consume Present(_) node by resolve_symbols()

Since resolve_symbols() now removes Present(_) node, it make sense to
handle symbol resolution error there. That's why I added a "pre" callback
to try_transform_expression().

Perhaps, "operation" scope (#1283) can be implemented in a similar way,
(but somehow need to resolve operation id and call repo.reload_at(op).)
---
 lib/src/default_revset_engine.rs |  4 ++--
 lib/src/revset.rs                | 29 +++++++++++++++++------------
 2 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs
index d6ae312aba..5457022d62 100644
--- a/lib/src/default_revset_engine.rs
+++ b/lib/src/default_revset_engine.rs
@@ -536,7 +536,8 @@ impl<'index, 'heads> EvaluationContext<'index, 'heads> {
             | RevsetExpression::RemoteBranches { .. }
             | RevsetExpression::Tags
             | RevsetExpression::GitRefs
-            | RevsetExpression::GitHead => {
+            | RevsetExpression::GitHead
+            | RevsetExpression::Present(_) => {
                 panic!("Expression '{expression:?}' should have been resolved by caller");
             }
             RevsetExpression::None => Ok(Box::new(EagerRevset::empty())),
@@ -644,7 +645,6 @@ impl<'index, 'heads> EvaluationContext<'index, 'heads> {
                 predicate: build_predicate_fn(self.store.clone(), self.index, predicate),
             })),
             RevsetExpression::AsFilter(candidates) => self.evaluate(candidates),
-            RevsetExpression::Present(candidates) => self.evaluate(candidates),
             RevsetExpression::NotIn(complement) => {
                 let set1 = self.evaluate(&RevsetExpression::All)?;
                 let set2 = self.evaluate(complement)?;
diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 04203b9029..f303af090a 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -1186,17 +1186,9 @@ fn try_transform_expression(
             RevsetExpression::AsFilter(candidates) => {
                 transform_rec(candidates, pre, post)?.map(RevsetExpression::AsFilter)
             }
-            RevsetExpression::Present(candidates) => match transform_rec(candidates, pre, post) {
-                Ok(None) => None,
-                Ok(Some(expression)) => Some(RevsetExpression::Present(expression)),
-                Err(RevsetResolutionError::NoSuchRevision(_)) => Some(RevsetExpression::None),
-                r @ Err(
-                    RevsetResolutionError::AmbiguousIdPrefix(_)
-                    | RevsetResolutionError::StoreError(_),
-                ) => {
-                    return r;
-                }
-            },
+            RevsetExpression::Present(candidates) => {
+                transform_rec(candidates, pre, post)?.map(RevsetExpression::Present)
+            }
             RevsetExpression::NotIn(complement) => {
                 transform_rec(complement, pre, post)?.map(RevsetExpression::NotIn)
             }
@@ -1624,7 +1616,20 @@ pub fn resolve_symbols(
 ) -> Result<Rc<RevsetExpression>, RevsetResolutionError> {
     Ok(try_transform_expression(
         &expression,
-        |_| Ok(None),
+        |expression| match expression.as_ref() {
+            // 'present(x)' opens new symbol resolution scope to map error to 'none()'.
+            RevsetExpression::Present(candidates) => {
+                resolve_symbols(repo, candidates.clone(), workspace_ctx)
+                    .or_else(|err| match err {
+                        RevsetResolutionError::NoSuchRevision(_) => Ok(RevsetExpression::none()),
+                        RevsetResolutionError::AmbiguousIdPrefix(_)
+                        | RevsetResolutionError::StoreError(_) => Err(err),
+                    })
+                    .map(Some) // Always rewrite subtree
+            }
+            // Otherwise resolve symbols recursively.
+            _ => Ok(None),
+        },
         |expression| {
             Ok(match expression.as_ref() {
                 RevsetExpression::Symbol(symbol) => {