From 3ff1f985f3bf9016b746dade45c7cee09905b464 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 30 Sep 2024 13:14:59 +0900 Subject: [PATCH] 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 | 8 ++++- lib/src/revset.rs | 74 +++++++++++++++++++++++++++------------- lib/tests/test_revset.rs | 64 ++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 25 deletions(-) diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index cd13782c4..83a3c1e01 100644 --- a/lib/src/id_prefix.rs +++ b/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) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 5d9693882..e4a867e2e 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1531,14 +1531,23 @@ fn make_no_such_symbol_error(repo: &dyn Repo, name: impl Into) -> Revset } pub trait SymbolResolver { - fn resolve_symbol(&self, symbol: &str) -> Result, RevsetResolutionError>; + /// Looks up `symbol` in the given `repo`. + fn resolve_symbol( + &self, + repo: &dyn Repo, + symbol: &str, + ) -> Result, RevsetResolutionError>; } /// Fails on any attempt to resolve a symbol. pub struct FailingSymbolResolver; impl SymbolResolver for FailingSymbolResolver { - fn resolve_symbol(&self, symbol: &str) -> Result, RevsetResolutionError> { + fn resolve_symbol( + &self, + _repo: &dyn Repo, + symbol: &str, + ) -> Result, 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>, 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>, 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>; + /// 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>; } /// 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>, } impl<'a> DefaultSymbolResolver<'a> { - pub fn new(repo: &'a dyn Repo, extensions: &[impl AsRef]) -> 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], + ) -> 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, RevsetResolutionError> { + fn resolve_symbol( + &self, + repo: &dyn Repo, + symbol: &str, + ) -> Result, 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, 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}"))) diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 540fbb1d1..f4b0eeecf 100644 --- a/lib/tests/test_revset.rs +++ b/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]) + .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();