diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 018101460..b4680e6e8 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -33,6 +33,7 @@ use jj_lib::fileset::FilesetExpression; use jj_lib::git; use jj_lib::hex_util::to_reverse_hex; use jj_lib::id_prefix::IdPrefixContext; +use jj_lib::id_prefix::IdPrefixIndex; use jj_lib::matchers::Matcher; use jj_lib::merged_tree::MergedTree; use jj_lib::object_id::ObjectId as _; @@ -1267,13 +1268,13 @@ impl CommitOrChangeId { pub fn shortest( &self, repo: &dyn Repo, - id_prefix_context: &IdPrefixContext, + index: &IdPrefixIndex, total_len: usize, ) -> ShortestIdPrefix { let mut hex = self.hex(); let prefix_len = match self { - CommitOrChangeId::Commit(id) => id_prefix_context.shortest_commit_prefix_len(repo, id), - CommitOrChangeId::Change(id) => id_prefix_context.shortest_change_prefix_len(repo, id), + CommitOrChangeId::Commit(id) => index.shortest_commit_prefix_len(repo, id), + CommitOrChangeId::Change(id) => index.shortest_change_prefix_len(repo, id), }; hex.truncate(max(prefix_len, total_len)); let rest = hex.split_off(prefix_len); @@ -1330,7 +1331,6 @@ fn builtin_commit_or_change_id_methods<'repo>( map.insert( "shortest", |language, diagnostics, build_ctx, self_property, function| { - let id_prefix_context = &language.id_prefix_context; let ([], [len_node]) = function.expect_arguments()?; let len_property = len_node .map(|node| { @@ -1342,8 +1342,10 @@ fn builtin_commit_or_change_id_methods<'repo>( ) }) .transpose()?; + let repo = language.repo; + let index = language.id_prefix_context.populate(repo); let out_property = (self_property, len_property) - .map(|(id, len)| id.shortest(language.repo, id_prefix_context, len.unwrap_or(0))); + .map(move |(id, len)| id.shortest(repo, &index, len.unwrap_or(0))); Ok(L::wrap_shortest_id_prefix(out_property)) }, ); diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index 755a274a8..44c4e7a1f 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -103,6 +103,7 @@ impl IdIndexSourceEntry for &'_ (CommitId, ChangeId) { } } +/// Manages configuration and cache of commit/change ID disambiguation index. #[derive(Default)] pub struct IdPrefixContext { disambiguation: Option, @@ -125,22 +126,32 @@ impl IdPrefixContext { self } - fn disambiguation_indexes(&self, repo: &dyn Repo) -> Option<&Indexes> { + /// Loads disambiguation index once, returns a borrowed index to + /// disambiguate commit/change IDs. + pub fn populate(&self, repo: &dyn Repo) -> IdPrefixIndex<'_> { // TODO: propagate errors instead of treating them as if no revset was specified - self.disambiguation.as_ref().and_then(|disambiguation| { + let indexes = self.disambiguation.as_ref().and_then(|disambiguation| { disambiguation .indexes(repo, self.extensions.symbol_resolvers()) .ok() - }) + }); + IdPrefixIndex { indexes } } +} +/// Loaded index to disambiguate commit/change IDs. +pub struct IdPrefixIndex<'a> { + indexes: Option<&'a Indexes>, +} + +impl IdPrefixIndex<'_> { /// Resolve an unambiguous commit ID prefix. pub fn resolve_commit_prefix( &self, repo: &dyn Repo, prefix: &HexPrefix, ) -> PrefixResolution { - if let Some(indexes) = self.disambiguation_indexes(repo) { + if let Some(indexes) = self.indexes { let resolution = indexes .commit_index .resolve_prefix_to_key(&*indexes.commit_change_ids, prefix); @@ -154,7 +165,7 @@ impl IdPrefixContext { /// Returns the shortest length of a prefix of `commit_id` that /// can still be resolved by `resolve_commit_prefix()`. pub fn shortest_commit_prefix_len(&self, repo: &dyn Repo, commit_id: &CommitId) -> usize { - if let Some(indexes) = self.disambiguation_indexes(repo) { + if let Some(indexes) = self.indexes { if let Some(lookup) = indexes .commit_index .lookup_exact(&*indexes.commit_change_ids, commit_id) @@ -171,7 +182,7 @@ impl IdPrefixContext { repo: &dyn Repo, prefix: &HexPrefix, ) -> PrefixResolution> { - if let Some(indexes) = self.disambiguation_indexes(repo) { + if let Some(indexes) = self.indexes { let resolution = indexes .change_index .resolve_prefix_to_key(&*indexes.commit_change_ids, prefix); @@ -190,7 +201,7 @@ impl IdPrefixContext { /// Returns the shortest length of a prefix of `change_id` that /// can still be resolved by `resolve_change_prefix()`. pub fn shortest_change_prefix_len(&self, repo: &dyn Repo, change_id: &ChangeId) -> usize { - if let Some(indexes) = self.disambiguation_indexes(repo) { + if let Some(indexes) = self.indexes { if let Some(lookup) = indexes .change_index .lookup_exact(&*indexes.commit_change_ids, change_id) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 76c3030c8..73b36704a 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1624,11 +1624,10 @@ impl PartialSymbolResolver for CommitPrefixResolver<'_> { symbol: &str, ) -> Result>, RevsetResolutionError> { if let Some(prefix) = HexPrefix::new(symbol) { - let resolution = self - .context - .as_ref() - .map(|ctx| ctx.resolve_commit_prefix(repo, &prefix)) - .unwrap_or_else(|| repo.index().resolve_commit_id_prefix(&prefix)); + let resolution = match self.context.map(|ctx| ctx.populate(repo)) { + Some(index) => index.resolve_commit_prefix(repo, &prefix), + None => repo.index().resolve_commit_id_prefix(&prefix), + }; match resolution { PrefixResolution::AmbiguousMatch => Err( RevsetResolutionError::AmbiguousCommitIdPrefix(symbol.to_owned()), @@ -1654,11 +1653,10 @@ impl PartialSymbolResolver for ChangePrefixResolver<'_> { symbol: &str, ) -> Result>, RevsetResolutionError> { if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) { - let resolution = self - .context - .as_ref() - .map(|ctx| ctx.resolve_change_prefix(repo, &prefix)) - .unwrap_or_else(|| repo.resolve_change_id_prefix(&prefix)); + let resolution = match self.context.map(|ctx| ctx.populate(repo)) { + Some(index) => index.resolve_change_prefix(repo, &prefix), + None => repo.resolve_change_id_prefix(&prefix), + }; match resolution { PrefixResolution::AmbiguousMatch => Err( RevsetResolutionError::AmbiguousChangeIdPrefix(symbol.to_owned()), diff --git a/lib/tests/test_id_prefix.rs b/lib/tests/test_id_prefix.rs index f30bda9df..4bd6a61d7 100644 --- a/lib/tests/test_id_prefix.rs +++ b/lib/tests/test_id_prefix.rs @@ -138,53 +138,54 @@ fn test_id_prefix() { // Without a disambiguation revset // --------------------------------------------------------------------------------------------- - let c = IdPrefixContext::default(); + let context = IdPrefixContext::default(); + let index = context.populate(repo.as_ref()); assert_eq!( - c.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), + index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), 2 ); assert_eq!( - c.shortest_commit_prefix_len(repo.as_ref(), commits[5].id()), + index.shortest_commit_prefix_len(repo.as_ref(), commits[5].id()), 1 ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("2")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("2")), AmbiguousMatch ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("2a")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("2a")), SingleMatch(commits[2].id().clone()) ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("20")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("20")), NoMatch ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("2a0")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("2a0")), NoMatch ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), + index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), 2 ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), commits[6].change_id()), + index.shortest_change_prefix_len(repo.as_ref(), commits[6].change_id()), 1 ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("7")), + index.resolve_change_prefix(repo.as_ref(), &prefix("7")), AmbiguousMatch ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("78")), + index.resolve_change_prefix(repo.as_ref(), &prefix("78")), SingleMatch(vec![commits[0].id().clone()]) ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("70")), + index.resolve_change_prefix(repo.as_ref(), &prefix("70")), NoMatch ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("780")), + index.resolve_change_prefix(repo.as_ref(), &prefix("780")), NoMatch ); @@ -192,28 +193,29 @@ fn test_id_prefix() { // --------------------------------------------------------------------------------------------- let expression = RevsetExpression::commits(vec![commits[0].id().clone(), commits[2].id().clone()]); - let c = c.disambiguate_within(expression); + let context = context.disambiguate_within(expression); + let index = context.populate(repo.as_ref()); // The prefix is now shorter assert_eq!( - c.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), + index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), 1 ); // Shorter prefix within the set can be used assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("2")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("2")), SingleMatch(commits[2].id().clone()) ); // Can still resolve commits outside the set assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("21")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("21")), SingleMatch(commits[24].id().clone()) ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), + index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), 1 ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("7")), + index.resolve_change_prefix(repo.as_ref(), &prefix("7")), SingleMatch(vec![commits[0].id().clone()]) ); @@ -221,29 +223,30 @@ fn test_id_prefix() { // needed. // --------------------------------------------------------------------------------------------- let expression = RevsetExpression::commit(root_commit_id.clone()); - let c = c.disambiguate_within(expression); + let context = context.disambiguate_within(expression); + let index = context.populate(repo.as_ref()); assert_eq!( - c.shortest_commit_prefix_len(repo.as_ref(), root_commit_id), + index.shortest_commit_prefix_len(repo.as_ref(), root_commit_id), 1 ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("")), AmbiguousMatch ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix("0")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("0")), SingleMatch(root_commit_id.clone()) ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), root_change_id), + index.shortest_change_prefix_len(repo.as_ref(), root_change_id), 1 ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("")), + index.resolve_change_prefix(repo.as_ref(), &prefix("")), AmbiguousMatch ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("0")), + index.resolve_change_prefix(repo.as_ref(), &prefix("0")), SingleMatch(vec![root_commit_id.clone()]) ); @@ -251,13 +254,14 @@ fn test_id_prefix() { // --------------------------------------------------------------------------------------------- // TODO: Should be an error let expression = RevsetExpression::symbol("nonexistent".to_string()); - let context = c.disambiguate_within(expression); + let context = context.disambiguate_within(expression); + let index = context.populate(repo.as_ref()); assert_eq!( - context.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), + index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), 2 ); assert_eq!( - context.resolve_commit_prefix(repo.as_ref(), &prefix("2")), + index.resolve_commit_prefix(repo.as_ref(), &prefix("2")), AmbiguousMatch ); } @@ -335,29 +339,30 @@ fn test_id_prefix_divergent() { // Without a disambiguation revset // -------------------------------- - let c = IdPrefixContext::default(); + let context = IdPrefixContext::default(); + let index = context.populate(repo.as_ref()); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), + index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), 3 ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), commits[1].change_id()), + index.shortest_change_prefix_len(repo.as_ref(), commits[1].change_id()), 3 ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()), + index.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()), 3 ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("a5")), + index.resolve_change_prefix(repo.as_ref(), &prefix("a5")), AmbiguousMatch ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("a53")), + index.resolve_change_prefix(repo.as_ref(), &prefix("a53")), SingleMatch(vec![first_commit.id().clone()]) ); assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("a50")), + index.resolve_change_prefix(repo.as_ref(), &prefix("a50")), SingleMatch(vec![ second_commit.id().clone(), third_commit_divergent_with_second.id().clone() @@ -367,10 +372,11 @@ fn test_id_prefix_divergent() { // Now, disambiguate within the revset containing only the second commit // ---------------------------------------------------------------------- let expression = RevsetExpression::commits(vec![second_commit.id().clone()]); - let c = c.disambiguate_within(expression); + let context = context.disambiguate_within(expression); + let index = context.populate(repo.as_ref()); // The prefix is now shorter assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()), + index.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()), 1 ); // This tests two issues, both important: @@ -381,7 +387,7 @@ fn test_id_prefix_divergent() { // match is not ambiguous, even though the first commit's change id would also // match the prefix. assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("a")), + index.resolve_change_prefix(repo.as_ref(), &prefix("a")), SingleMatch(vec![ second_commit.id().clone(), third_commit_divergent_with_second.id().clone() @@ -390,11 +396,11 @@ fn test_id_prefix_divergent() { // We can still resolve commits outside the set assert_eq!( - c.resolve_change_prefix(repo.as_ref(), &prefix("a53")), + index.resolve_change_prefix(repo.as_ref(), &prefix("a53")), SingleMatch(vec![first_commit.id().clone()]) ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), first_commit.change_id()), + index.shortest_change_prefix_len(repo.as_ref(), first_commit.change_id()), 3 ); } @@ -479,32 +485,33 @@ fn test_id_prefix_hidden() { // Without a disambiguation revset // -------------------------------- - let c = IdPrefixContext::default(); + let context = IdPrefixContext::default(); + let index = context.populate(repo.as_ref()); assert_eq!( - c.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), + index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), 2 ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()), + index.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()), 3 ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])), + index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])), AmbiguousMatch ); assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..2])), + index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..2])), SingleMatch(hidden_commit.id().clone()) ); assert_eq!( - c.resolve_change_prefix( + index.resolve_change_prefix( repo.as_ref(), &prefix(&hidden_commit.change_id().hex()[..2]) ), AmbiguousMatch ); assert_eq!( - c.resolve_change_prefix( + index.resolve_change_prefix( repo.as_ref(), &prefix(&hidden_commit.change_id().hex()[..3]) ), @@ -514,32 +521,33 @@ fn test_id_prefix_hidden() { // Disambiguate within hidden // -------------------------- let expression = RevsetExpression::commit(hidden_commit.id().clone()); - let c = c.disambiguate_within(expression); + let context = context.disambiguate_within(expression); + let index = context.populate(repo.as_ref()); assert_eq!( - c.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), + index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), 1 ); assert_eq!( - c.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()), + index.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()), 1 ); // Short commit id can be resolved even if it's hidden. assert_eq!( - c.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])), + index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])), SingleMatch(hidden_commit.id().clone()) ); // OTOH, hidden change id should never be found. The resolution might be // ambiguous if hidden commits were excluded from the disambiguation set. // In that case, shortest_change_prefix_len() shouldn't be 1. assert_eq!( - c.resolve_change_prefix( + index.resolve_change_prefix( repo.as_ref(), &prefix(&hidden_commit.change_id().hex()[..1]) ), NoMatch ); assert_eq!( - c.resolve_change_prefix( + index.resolve_change_prefix( repo.as_ref(), &prefix(&hidden_commit.change_id().hex()[..2]) ),