revset: move resolve_symbol() back to revset module

The only caller is now in `revset.rs`.

lib/src/default_revset_engine.rs

@@ -18,14 +18,11 @@ use std::iter::Peekable;
 
 use itertools::Itertools;
 
-use crate::backend::{BackendError, CommitId, ObjectId};
+use crate::backend::CommitId;
 use crate::commit::Commit;
 use crate::default_index_store::IndexEntry;
 use crate::default_revset_graph_iterator::RevsetGraphIterator;
-use crate::hex_util::to_forward_hex;
-use crate::index::{HexPrefix, PrefixResolution};
 use crate::matchers::{EverythingMatcher, Matcher, PrefixMatcher};
-use crate::op_store::WorkspaceId;
 use crate::repo::Repo;
 use crate::revset::{
     Revset, RevsetError, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge,
@@ -33,147 +30,6 @@ use crate::revset::{
 };
 use crate::rewrite;
 
-fn resolve_git_ref(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
-    let view = repo.view();
-    for git_ref_prefix in &["", "refs/", "refs/heads/", "refs/tags/", "refs/remotes/"] {
-        if let Some(ref_target) = view.git_refs().get(&(git_ref_prefix.to_string() + symbol)) {
-            return Some(ref_target.adds());
-        }
-    }
-    None
-}
-
-fn resolve_branch(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
-    if let Some(branch_target) = repo.view().branches().get(symbol) {
-        return Some(
-            branch_target
-                .local_target
-                .as_ref()
-                .map(|target| target.adds())
-                .unwrap_or_default(),
-        );
-    }
-    if let Some((name, remote_name)) = symbol.split_once('@') {
-        if let Some(branch_target) = repo.view().branches().get(name) {
-            if let Some(target) = branch_target.remote_targets.get(remote_name) {
-                return Some(target.adds());
-            }
-        }
-    }
-    None
-}
-
-fn resolve_full_commit_id(
-    repo: &dyn Repo,
-    symbol: &str,
-) -> Result<Option<Vec<CommitId>>, RevsetError> {
-    if let Ok(binary_commit_id) = hex::decode(symbol) {
-        if repo.store().commit_id_length() != binary_commit_id.len() {
-            return Ok(None);
-        }
-        let commit_id = CommitId::new(binary_commit_id);
-        match repo.store().get_commit(&commit_id) {
-            // Only recognize a commit if we have indexed it
-            Ok(_) if repo.index().entry_by_id(&commit_id).is_some() => Ok(Some(vec![commit_id])),
-            Ok(_) | Err(BackendError::ObjectNotFound { .. }) => Ok(None),
-            Err(err) => Err(RevsetError::StoreError(err)),
-        }
-    } else {
-        Ok(None)
-    }
-}
-
-fn resolve_short_commit_id(
-    repo: &dyn Repo,
-    symbol: &str,
-) -> Result<Option<Vec<CommitId>>, RevsetError> {
-    if let Some(prefix) = HexPrefix::new(symbol) {
-        match repo.index().resolve_prefix(&prefix) {
-            PrefixResolution::NoMatch => Ok(None),
-            PrefixResolution::AmbiguousMatch => {
-                Err(RevsetError::AmbiguousIdPrefix(symbol.to_owned()))
-            }
-            PrefixResolution::SingleMatch(commit_id) => Ok(Some(vec![commit_id])),
-        }
-    } else {
-        Ok(None)
-    }
-}
-
-fn resolve_change_id(repo: &dyn Repo, symbol: &str) -> Result<Option<Vec<CommitId>>, RevsetError> {
-    if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) {
-        match repo.resolve_change_id_prefix(&prefix) {
-            PrefixResolution::NoMatch => Ok(None),
-            PrefixResolution::AmbiguousMatch => {
-                Err(RevsetError::AmbiguousIdPrefix(symbol.to_owned()))
-            }
-            PrefixResolution::SingleMatch(entries) => {
-                Ok(Some(entries.iter().map(|e| e.commit_id()).collect()))
-            }
-        }
-    } else {
-        Ok(None)
-    }
-}
-
-pub fn resolve_symbol(
-    repo: &dyn Repo,
-    symbol: &str,
-    workspace_id: Option<&WorkspaceId>,
-) -> Result<Vec<CommitId>, RevsetError> {
-    if symbol.ends_with('@') {
-        let target_workspace = if symbol == "@" {
-            if let Some(workspace_id) = workspace_id {
-                workspace_id.clone()
-            } else {
-                return Err(RevsetError::NoSuchRevision(symbol.to_owned()));
-            }
-        } else {
-            WorkspaceId::new(symbol.strip_suffix('@').unwrap().to_string())
-        };
-        if let Some(commit_id) = repo.view().get_wc_commit_id(&target_workspace) {
-            Ok(vec![commit_id.clone()])
-        } else {
-            Err(RevsetError::NoSuchRevision(symbol.to_owned()))
-        }
-    } else if symbol == "root" {
-        Ok(vec![repo.store().root_commit_id().clone()])
-    } else {
-        // Try to resolve as a tag
-        if let Some(target) = repo.view().tags().get(symbol) {
-            return Ok(target.adds());
-        }
-
-        // Try to resolve as a branch
-        if let Some(ids) = resolve_branch(repo, symbol) {
-            return Ok(ids);
-        }
-
-        // Try to resolve as a git ref
-        if let Some(ids) = resolve_git_ref(repo, symbol) {
-            return Ok(ids);
-        }
-
-        // Try to resolve as a full commit id. We assume a full commit id is unambiguous
-        // even if it's shorter than change id.
-        if let Some(ids) = resolve_full_commit_id(repo, symbol)? {
-            return Ok(ids);
-        }
-
-        // Try to resolve as a commit id.
-        if let Some(ids) = resolve_short_commit_id(repo, symbol)? {
-            return Ok(ids);
-        }
-
-        // Try to resolve as a change id.
-        if let Some(ids) = resolve_change_id(repo, symbol)? {
-            return Ok(ids);
-        }
-
-        Err(RevsetError::NoSuchRevision(symbol.to_owned()))
-    }
-}
-
 trait ToPredicateFn<'index> {
     /// Creates function that tests if the given entry is included in the set.
     ///
@@ -852,7 +708,7 @@ fn has_diff_from_parent(repo: &dyn Repo, entry: &IndexEntry<'_>, matcher: &dyn M
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::backend::{ChangeId, CommitId};
+    use crate::backend::{ChangeId, CommitId, ObjectId};
     use crate::default_index_store::MutableIndexImpl;
     use crate::index::Index;
 

lib/src/revset.rs

@@ -27,10 +27,11 @@ use pest::Parser;
 use pest_derive::Parser;
 use thiserror::Error;
 
-use crate::backend::{BackendError, BackendResult, CommitId};
+use crate::backend::{BackendError, BackendResult, CommitId, ObjectId};
 use crate::commit::Commit;
 use crate::default_index_store::{IndexEntry, IndexPosition};
-use crate::default_revset_engine::resolve_symbol;
+use crate::hex_util::to_forward_hex;
+use crate::index::{HexPrefix, PrefixResolution};
 use crate::op_store::WorkspaceId;
 use crate::repo::Repo;
 use crate::repo_path::{FsPathParseError, RepoPath};
@@ -1391,6 +1392,147 @@ pub fn optimize(expression: Rc<RevsetExpression>) -> Rc<RevsetExpression> {
     fold_difference(&expression).unwrap_or(expression)
 }
 
+fn resolve_git_ref(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
+    let view = repo.view();
+    for git_ref_prefix in &["", "refs/", "refs/heads/", "refs/tags/", "refs/remotes/"] {
+        if let Some(ref_target) = view.git_refs().get(&(git_ref_prefix.to_string() + symbol)) {
+            return Some(ref_target.adds());
+        }
+    }
+    None
+}
+
+fn resolve_branch(repo: &dyn Repo, symbol: &str) -> Option<Vec<CommitId>> {
+    if let Some(branch_target) = repo.view().branches().get(symbol) {
+        return Some(
+            branch_target
+                .local_target
+                .as_ref()
+                .map(|target| target.adds())
+                .unwrap_or_default(),
+        );
+    }
+    if let Some((name, remote_name)) = symbol.split_once('@') {
+        if let Some(branch_target) = repo.view().branches().get(name) {
+            if let Some(target) = branch_target.remote_targets.get(remote_name) {
+                return Some(target.adds());
+            }
+        }
+    }
+    None
+}
+
+fn resolve_full_commit_id(
+    repo: &dyn Repo,
+    symbol: &str,
+) -> Result<Option<Vec<CommitId>>, RevsetError> {
+    if let Ok(binary_commit_id) = hex::decode(symbol) {
+        if repo.store().commit_id_length() != binary_commit_id.len() {
+            return Ok(None);
+        }
+        let commit_id = CommitId::new(binary_commit_id);
+        match repo.store().get_commit(&commit_id) {
+            // Only recognize a commit if we have indexed it
+            Ok(_) if repo.index().entry_by_id(&commit_id).is_some() => Ok(Some(vec![commit_id])),
+            Ok(_) | Err(BackendError::ObjectNotFound { .. }) => Ok(None),
+            Err(err) => Err(RevsetError::StoreError(err)),
+        }
+    } else {
+        Ok(None)
+    }
+}
+
+fn resolve_short_commit_id(
+    repo: &dyn Repo,
+    symbol: &str,
+) -> Result<Option<Vec<CommitId>>, RevsetError> {
+    if let Some(prefix) = HexPrefix::new(symbol) {
+        match repo.index().resolve_prefix(&prefix) {
+            PrefixResolution::NoMatch => Ok(None),
+            PrefixResolution::AmbiguousMatch => {
+                Err(RevsetError::AmbiguousIdPrefix(symbol.to_owned()))
+            }
+            PrefixResolution::SingleMatch(commit_id) => Ok(Some(vec![commit_id])),
+        }
+    } else {
+        Ok(None)
+    }
+}
+
+fn resolve_change_id(repo: &dyn Repo, symbol: &str) -> Result<Option<Vec<CommitId>>, RevsetError> {
+    if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) {
+        match repo.resolve_change_id_prefix(&prefix) {
+            PrefixResolution::NoMatch => Ok(None),
+            PrefixResolution::AmbiguousMatch => {
+                Err(RevsetError::AmbiguousIdPrefix(symbol.to_owned()))
+            }
+            PrefixResolution::SingleMatch(entries) => {
+                Ok(Some(entries.iter().map(|e| e.commit_id()).collect()))
+            }
+        }
+    } else {
+        Ok(None)
+    }
+}
+
+pub fn resolve_symbol(
+    repo: &dyn Repo,
+    symbol: &str,
+    workspace_id: Option<&WorkspaceId>,
+) -> Result<Vec<CommitId>, RevsetError> {
+    if symbol.ends_with('@') {
+        let target_workspace = if symbol == "@" {
+            if let Some(workspace_id) = workspace_id {
+                workspace_id.clone()
+            } else {
+                return Err(RevsetError::NoSuchRevision(symbol.to_owned()));
+            }
+        } else {
+            WorkspaceId::new(symbol.strip_suffix('@').unwrap().to_string())
+        };
+        if let Some(commit_id) = repo.view().get_wc_commit_id(&target_workspace) {
+            Ok(vec![commit_id.clone()])
+        } else {
+            Err(RevsetError::NoSuchRevision(symbol.to_owned()))
+        }
+    } else if symbol == "root" {
+        Ok(vec![repo.store().root_commit_id().clone()])
+    } else {
+        // Try to resolve as a tag
+        if let Some(target) = repo.view().tags().get(symbol) {
+            return Ok(target.adds());
+        }
+
+        // Try to resolve as a branch
+        if let Some(ids) = resolve_branch(repo, symbol) {
+            return Ok(ids);
+        }
+
+        // Try to resolve as a git ref
+        if let Some(ids) = resolve_git_ref(repo, symbol) {
+            return Ok(ids);
+        }
+
+        // Try to resolve as a full commit id. We assume a full commit id is unambiguous
+        // even if it's shorter than change id.
+        if let Some(ids) = resolve_full_commit_id(repo, symbol)? {
+            return Ok(ids);
+        }
+
+        // Try to resolve as a commit id.
+        if let Some(ids) = resolve_short_commit_id(repo, symbol)? {
+            return Ok(ids);
+        }
+
+        // Try to resolve as a change id.
+        if let Some(ids) = resolve_change_id(repo, symbol)? {
+            return Ok(ids);
+        }
+
+        Err(RevsetError::NoSuchRevision(symbol.to_owned()))
+    }
+}
+
 // TODO: Maybe return a new type (RevsetParameters?) instead of
 // RevsetExpression. Then pass that to evaluate(), so it's clear which variants
 // are allowed.

lib/tests/test_revset.rs

@@ -17,14 +17,14 @@ use std::path::Path;
 use assert_matches::assert_matches;
 use itertools::Itertools;
 use jujutsu_lib::backend::{CommitId, MillisSinceEpoch, ObjectId, Signature, Timestamp};
-use jujutsu_lib::default_revset_engine::{resolve_symbol, revset_for_commits};
+use jujutsu_lib::default_revset_engine::revset_for_commits;
 use jujutsu_lib::git;
 use jujutsu_lib::op_store::{RefTarget, WorkspaceId};
 use jujutsu_lib::repo::Repo;
 use jujutsu_lib::repo_path::RepoPath;
 use jujutsu_lib::revset::{
-    optimize, parse, resolve_symbols, ReverseRevsetGraphIterator, RevsetAliasesMap, RevsetError,
+    optimize, parse, resolve_symbol, resolve_symbols, ReverseRevsetGraphIterator, RevsetAliasesMap,
-    RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, RevsetIteratorExt,
+    RevsetError, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, RevsetIteratorExt,
     RevsetWorkspaceContext,
 };
 use jujutsu_lib::settings::GitSettings;