revset: pass separate repo to disambiguation index

The idea is that the disambiguation index can be loaded from a repo which is
different from the symbol resolution context.

Suppose we add at_operation(op, expr) revset, a symbol inside at_operation()
expression will have to be resolved within that operation, whereas the
disambiguation index is cached globally by WorkspaceCommandHelper. We could
build temporary disambiguation index for each at-op repo, but that would be
complicated implementation-wise, and wouldn't be useful. For example, a query
"x | at_operation(@-, x)" might be resolved to "xy | at_operation(@-, xz)"
if disambiguation index were reloaded for the @- operation. Instead, the
short change ID "x" can be disambiguated to "xy", then resolved to the
corresponding commit IDs at each operation.

lib/src/id_prefix.rs

@@ -161,7 +161,13 @@ impl IdPrefixIndex<'_> {
                 .commit_index
                 .resolve_prefix_to_key(&*indexes.commit_change_ids, prefix);
             if let PrefixResolution::SingleMatch(id) = resolution {
-                return PrefixResolution::SingleMatch(id);
+                // The disambiguation set may be loaded from a different repo,
+                // and contain a commit that doesn't exist in the current repo.
+                if repo.index().has_id(&id) {
+                    return PrefixResolution::SingleMatch(id);
+                } else {
+                    return PrefixResolution::NoMatch;
+                }
             }
         }
         repo.index().resolve_commit_id_prefix(prefix)

lib/src/revset.rs

@@ -1531,14 +1531,23 @@ fn make_no_such_symbol_error(repo: &dyn Repo, name: impl Into<String>) -> Revset
 }
 
 pub trait SymbolResolver {
-    fn resolve_symbol(&self, symbol: &str) -> Result<Vec<CommitId>, RevsetResolutionError>;
+    /// Looks up `symbol` in the given `repo`.
+    fn resolve_symbol(
+        &self,
+        repo: &dyn Repo,
+        symbol: &str,
+    ) -> Result<Vec<CommitId>, RevsetResolutionError>;
 }
 
 /// Fails on any attempt to resolve a symbol.
 pub struct FailingSymbolResolver;
 
 impl SymbolResolver for FailingSymbolResolver {
-    fn resolve_symbol(&self, symbol: &str) -> Result<Vec<CommitId>, RevsetResolutionError> {
+    fn resolve_symbol(
+        &self,
+        _repo: &dyn Repo,
+        symbol: &str,
+    ) -> Result<Vec<CommitId>, RevsetResolutionError> {
         Err(RevsetResolutionError::NoSuchRevision {
             name: format!(
                 "Won't resolve symbol {symbol:?}. When creating revsets programmatically, avoid \
@@ -1613,8 +1622,8 @@ impl PartialSymbolResolver for GitRefResolver {
 const DEFAULT_RESOLVERS: &[&'static dyn PartialSymbolResolver] =
     &[&TagResolver, &BookmarkResolver, &GitRefResolver];
 
-#[derive(Default)]
 struct CommitPrefixResolver<'a> {
+    context_repo: &'a dyn Repo,
     context: Option<&'a IdPrefixContext>,
 }
 
@@ -1625,9 +1634,9 @@ impl PartialSymbolResolver for CommitPrefixResolver<'_> {
         symbol: &str,
     ) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
         if let Some(prefix) = HexPrefix::new(symbol) {
-            let index = self
-                .context
-                .map_or(IdPrefixIndex::empty(), |ctx| ctx.populate(repo));
+            let index = self.context.map_or(IdPrefixIndex::empty(), |ctx| {
+                ctx.populate(self.context_repo)
+            });
             match index.resolve_commit_prefix(repo, &prefix) {
                 PrefixResolution::AmbiguousMatch => Err(
                     RevsetResolutionError::AmbiguousCommitIdPrefix(symbol.to_owned()),
@@ -1641,8 +1650,8 @@ impl PartialSymbolResolver for CommitPrefixResolver<'_> {
     }
 }
 
-#[derive(Default)]
 struct ChangePrefixResolver<'a> {
+    context_repo: &'a dyn Repo,
     context: Option<&'a IdPrefixContext>,
 }
 
@@ -1653,9 +1662,9 @@ impl PartialSymbolResolver for ChangePrefixResolver<'_> {
         symbol: &str,
     ) -> Result<Option<Vec<CommitId>>, RevsetResolutionError> {
         if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) {
-            let index = self
-                .context
-                .map_or(IdPrefixIndex::empty(), |ctx| ctx.populate(repo));
+            let index = self.context.map_or(IdPrefixIndex::empty(), |ctx| {
+                ctx.populate(self.context_repo)
+            });
             match index.resolve_change_prefix(repo, &prefix) {
                 PrefixResolution::AmbiguousMatch => Err(
                     RevsetResolutionError::AmbiguousChangeIdPrefix(symbol.to_owned()),
@@ -1676,30 +1685,43 @@ impl PartialSymbolResolver for ChangePrefixResolver<'_> {
 /// may provide a way for extensions to override native resolvers like tags and
 /// bookmarks.
 pub trait SymbolResolverExtension {
-    /// PartialSymbolResolvers can capture `repo` for caching purposes if
-    /// desired, but they do not have to since `repo` is passed into
-    /// `resolve_symbol()` as well.
-    fn new_resolvers<'a>(&self, repo: &'a dyn Repo) -> Vec<Box<dyn PartialSymbolResolver + 'a>>;
+    /// PartialSymbolResolvers can initialize some global data by using the
+    /// `context_repo`, but the `context_repo` may point to a different
+    /// operation from the `repo` passed into `resolve_symbol()`. For
+    /// resolution, the latter `repo` should be used.
+    fn new_resolvers<'a>(
+        &self,
+        context_repo: &'a dyn Repo,
+    ) -> Vec<Box<dyn PartialSymbolResolver + 'a>>;
 }
 
 /// Resolves bookmarks, remote bookmarks, tags, git refs, and full and
 /// abbreviated commit and change ids.
 pub struct DefaultSymbolResolver<'a> {
-    repo: &'a dyn Repo,
     commit_id_resolver: CommitPrefixResolver<'a>,
     change_id_resolver: ChangePrefixResolver<'a>,
     extensions: Vec<Box<dyn PartialSymbolResolver + 'a>>,
 }
 
 impl<'a> DefaultSymbolResolver<'a> {
-    pub fn new(repo: &'a dyn Repo, extensions: &[impl AsRef<dyn SymbolResolverExtension>]) -> Self {
+    /// Creates new symbol resolver that will first disambiguate short ID
+    /// prefixes within the given `context_repo` if configured.
+    pub fn new(
+        context_repo: &'a dyn Repo,
+        extensions: &[impl AsRef<dyn SymbolResolverExtension>],
+    ) -> Self {
         DefaultSymbolResolver {
-            repo,
-            commit_id_resolver: Default::default(),
-            change_id_resolver: Default::default(),
+            commit_id_resolver: CommitPrefixResolver {
+                context_repo,
+                context: None,
+            },
+            change_id_resolver: ChangePrefixResolver {
+                context_repo,
+                context: None,
+            },
             extensions: extensions
                 .iter()
-                .flat_map(|ext| ext.as_ref().new_resolvers(repo))
+                .flat_map(|ext| ext.as_ref().new_resolvers(context_repo))
                 .collect(),
         }
     }
@@ -1722,18 +1744,22 @@ impl<'a> DefaultSymbolResolver<'a> {
 }
 
 impl SymbolResolver for DefaultSymbolResolver<'_> {
-    fn resolve_symbol(&self, symbol: &str) -> Result<Vec<CommitId>, RevsetResolutionError> {
+    fn resolve_symbol(
+        &self,
+        repo: &dyn Repo,
+        symbol: &str,
+    ) -> Result<Vec<CommitId>, RevsetResolutionError> {
         if symbol.is_empty() {
             return Err(RevsetResolutionError::EmptyString);
         }
 
         for partial_resolver in self.partial_resolvers() {
-            if let Some(ids) = partial_resolver.resolve_symbol(self.repo, symbol)? {
+            if let Some(ids) = partial_resolver.resolve_symbol(repo, symbol)? {
                 return Ok(ids);
             }
         }
 
-        Err(make_no_such_symbol_error(self.repo, symbol))
+        Err(make_no_such_symbol_error(repo, symbol))
     }
 }
 
@@ -1743,7 +1769,7 @@ fn resolve_commit_ref(
     symbol_resolver: &dyn SymbolResolver,
 ) -> Result<Vec<CommitId>, RevsetResolutionError> {
     match commit_ref {
-        RevsetCommitRef::Symbol(symbol) => symbol_resolver.resolve_symbol(symbol),
+        RevsetCommitRef::Symbol(symbol) => symbol_resolver.resolve_symbol(repo, symbol),
         RevsetCommitRef::RemoteSymbol { name, remote } => {
             resolve_remote_bookmark(repo, name, remote)
                 .ok_or_else(|| make_no_such_symbol_error(repo, format!("{name}@{remote}")))

lib/tests/test_revset.rs

@@ -29,6 +29,8 @@ use jj_lib::git;
 use jj_lib::git_backend::GitBackend;
 use jj_lib::graph::GraphEdge;
 use jj_lib::graph::ReverseGraphIterator;
+use jj_lib::hex_util::to_reverse_hex;
+use jj_lib::id_prefix::IdPrefixContext;
 use jj_lib::object_id::ObjectId;
 use jj_lib::op_store::RefTarget;
 use jj_lib::op_store::RemoteRef;
@@ -51,6 +53,7 @@ use jj_lib::revset::RevsetFilterPredicate;
 use jj_lib::revset::RevsetParseContext;
 use jj_lib::revset::RevsetResolutionError;
 use jj_lib::revset::RevsetWorkspaceContext;
+use jj_lib::revset::SymbolResolver;
 use jj_lib::revset::SymbolResolverExtension;
 use jj_lib::settings::GitSettings;
 use jj_lib::workspace::Workspace;
@@ -369,6 +372,67 @@ fn test_resolve_symbol_change_id(readonly: bool) {
     );
 }
 
+#[test]
+fn test_resolve_symbol_in_different_disambiguation_context() {
+    let settings = testutils::user_settings();
+    let test_repo = TestRepo::init();
+    let repo0 = &test_repo.repo;
+
+    let mut tx = repo0.start_transaction(&settings);
+    let commit1 = write_random_commit(tx.repo_mut(), &settings);
+    // Create more commits that are likely to conflict with 1-char hex prefix.
+    for _ in 0..50 {
+        write_random_commit(tx.repo_mut(), &settings);
+    }
+    let repo1 = tx.commit("test");
+
+    let mut tx = repo1.start_transaction(&settings);
+    let commit2 = tx
+        .repo_mut()
+        .rewrite_commit(&settings, &commit1)
+        .write()
+        .unwrap();
+    tx.repo_mut().rebase_descendants(&settings).unwrap();
+    let repo2 = tx.commit("test");
+
+    // Set up disambiguation index which only contains the commit2.id().
+    let id_prefix_context = IdPrefixContext::new(Default::default())
+        .disambiguate_within(RevsetExpression::commit(commit2.id().clone()));
+    let symbol_resolver =
+        DefaultSymbolResolver::new(repo2.as_ref(), &[] as &[Box<dyn SymbolResolverExtension>])
+            .with_id_prefix_context(&id_prefix_context);
+
+    // Sanity check
+    let change_hex = &to_reverse_hex(&commit2.change_id().hex()).unwrap();
+    assert_eq!(
+        symbol_resolver
+            .resolve_symbol(repo2.as_ref(), &change_hex[0..1])
+            .unwrap(),
+        vec![commit2.id().clone()]
+    );
+    assert_eq!(
+        symbol_resolver
+            .resolve_symbol(repo2.as_ref(), &commit2.id().hex()[0..1])
+            .unwrap(),
+        vec![commit2.id().clone()]
+    );
+
+    // Change ID is disambiguated within repo2, then resolved in repo1.
+    assert_eq!(
+        symbol_resolver
+            .resolve_symbol(repo1.as_ref(), &change_hex[0..1])
+            .unwrap(),
+        vec![commit1.id().clone()]
+    );
+
+    // Commit ID can be found in the disambiguation index, but doesn't exist in
+    // repo1.
+    assert_matches!(
+        symbol_resolver.resolve_symbol(repo1.as_ref(), &commit2.id().hex()[0..1]),
+        Err(RevsetResolutionError::NoSuchRevision { .. })
+    );
+}
+
 #[test]
 fn test_resolve_working_copy() {
     let settings = testutils::user_settings();