From 916b00c33efb5d1fb343553a3e419342fdd8372a Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 11 May 2023 22:09:48 -0700 Subject: [PATCH] id_prefix: remove `repo` field from `IdPrefixContext` By passing the repo as argument to the methods instead, we can remove the `repo` field and the associated lifetime. Thanks to Yuya for the suggestion. --- lib/src/id_prefix.rs | 51 +++++++++-------- lib/src/revset.rs | 6 +- lib/tests/test_id_prefix.rs | 106 +++++++++++++++++++++++++++--------- src/cli_util.rs | 21 ++++--- src/commit_templater.rs | 10 ++-- 5 files changed, 124 insertions(+), 70 deletions(-) diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index a2c22e8353..834e719378 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -66,19 +66,12 @@ impl DisambiguationData { } } -pub struct IdPrefixContext<'repo> { - repo: &'repo dyn Repo, +#[derive(Default)] +pub struct IdPrefixContext { disambiguation: Option, } -impl IdPrefixContext<'_> { - pub fn new(repo: &dyn Repo) -> IdPrefixContext { - IdPrefixContext { - repo, - disambiguation: None, - } - } - +impl IdPrefixContext { pub fn disambiguate_within( mut self, expression: Rc, @@ -92,59 +85,65 @@ impl IdPrefixContext<'_> { self } - fn disambiguation_indexes(&self) -> Option<&Indexes> { + fn disambiguation_indexes(&self, repo: &dyn Repo) -> Option<&Indexes> { // TODO: propagate errors instead of treating them as if no revset was specified self.disambiguation .as_ref() - .and_then(|disambiguation| disambiguation.indexes(self.repo).ok()) + .and_then(|disambiguation| disambiguation.indexes(repo).ok()) } /// Resolve an unambiguous commit ID prefix. - pub fn resolve_commit_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { - if let Some(indexes) = self.disambiguation_indexes() { + pub fn resolve_commit_prefix( + &self, + repo: &dyn Repo, + prefix: &HexPrefix, + ) -> PrefixResolution { + if let Some(indexes) = self.disambiguation_indexes(repo) { let resolution = indexes.commit_index.resolve_prefix(prefix); if let PrefixResolution::SingleMatch(mut ids) = resolution { assert_eq!(ids.len(), 1); return PrefixResolution::SingleMatch(ids.pop().unwrap()); } } - self.repo.index().resolve_prefix(prefix) + repo.index().resolve_prefix(prefix) } /// 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, commit_id: &CommitId) -> usize { - if let Some(indexes) = self.disambiguation_indexes() { + pub fn shortest_commit_prefix_len(&self, repo: &dyn Repo, commit_id: &CommitId) -> usize { + if let Some(indexes) = self.disambiguation_indexes(repo) { // TODO: Avoid the double lookup here (has_key() + shortest_unique_prefix_len()) if indexes.commit_index.has_key(commit_id) { return indexes.commit_index.shortest_unique_prefix_len(commit_id); } } - self.repo - .index() - .shortest_unique_commit_id_prefix_len(commit_id) + repo.index().shortest_unique_commit_id_prefix_len(commit_id) } /// Resolve an unambiguous change ID prefix to the commit IDs in the revset. - pub fn resolve_change_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { - if let Some(indexes) = self.disambiguation_indexes() { + pub fn resolve_change_prefix( + &self, + repo: &dyn Repo, + prefix: &HexPrefix, + ) -> PrefixResolution> { + if let Some(indexes) = self.disambiguation_indexes(repo) { let resolution = indexes.change_index.resolve_prefix(prefix); if let PrefixResolution::SingleMatch(ids) = resolution { return PrefixResolution::SingleMatch(ids); } } - self.repo.resolve_change_id_prefix(prefix) + repo.resolve_change_id_prefix(prefix) } /// 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, change_id: &ChangeId) -> usize { - if let Some(indexes) = self.disambiguation_indexes() { + pub fn shortest_change_prefix_len(&self, repo: &dyn Repo, change_id: &ChangeId) -> usize { + if let Some(indexes) = self.disambiguation_indexes(repo) { if indexes.change_index.has_key(change_id) { return indexes.change_index.shortest_unique_prefix_len(change_id); } } - self.repo.shortest_unique_change_id_prefix_len(change_id) + repo.shortest_unique_change_id_prefix_len(change_id) } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 49d85d4494..8e9252aaf2 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1647,7 +1647,7 @@ impl SymbolResolver for FailingSymbolResolver { } } -pub type PrefixResolver<'a, T> = Box PrefixResolution + 'a>; +pub type PrefixResolver<'a, T> = Box PrefixResolution + 'a>; /// Resolves the "root" and "@" symbols, branches, remote branches, tags, git /// refs, and full and abbreviated commit and change ids. @@ -1728,7 +1728,7 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { // Try to resolve as a commit id. if let Some(prefix) = HexPrefix::new(symbol) { let prefix_resolution = if let Some(commit_id_resolver) = &self.commit_id_resolver { - commit_id_resolver(&prefix) + commit_id_resolver(self.repo, &prefix) } else { self.repo.index().resolve_prefix(&prefix) }; @@ -1748,7 +1748,7 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { // Try to resolve as a change id. if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) { let prefix_resolution = if let Some(change_id_resolver) = &self.change_id_resolver { - change_id_resolver(&prefix) + change_id_resolver(self.repo, &prefix) } else { self.repo.resolve_change_id_prefix(&prefix) }; diff --git a/lib/tests/test_id_prefix.rs b/lib/tests/test_id_prefix.rs index 40b9320780..d27cf18263 100644 --- a/lib/tests/test_id_prefix.rs +++ b/lib/tests/test_id_prefix.rs @@ -130,25 +130,55 @@ fn test_id_prefix() { // Without a disambiguation revset // --------------------------------------------------------------------------------------------- - let c = IdPrefixContext::new(repo.as_ref()); - assert_eq!(c.shortest_commit_prefix_len(commits[2].id()), 2); - assert_eq!(c.shortest_commit_prefix_len(commits[5].id()), 1); - assert_eq!(c.resolve_commit_prefix(&prefix("2")), AmbiguousMatch); + let c = IdPrefixContext::default(); assert_eq!( - c.resolve_commit_prefix(&prefix("2a")), + c.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), + 2 + ); + assert_eq!( + c.shortest_commit_prefix_len(repo.as_ref(), commits[5].id()), + 1 + ); + assert_eq!( + c.resolve_commit_prefix(repo.as_ref(), &prefix("2")), + AmbiguousMatch + ); + assert_eq!( + c.resolve_commit_prefix(repo.as_ref(), &prefix("2a")), SingleMatch(commits[2].id().clone()) ); - assert_eq!(c.resolve_commit_prefix(&prefix("20")), NoMatch); - assert_eq!(c.resolve_commit_prefix(&prefix("2a0")), NoMatch); - assert_eq!(c.shortest_change_prefix_len(commits[0].change_id()), 2); - assert_eq!(c.shortest_change_prefix_len(commits[6].change_id()), 1); - assert_eq!(c.resolve_change_prefix(&prefix("7")), AmbiguousMatch); assert_eq!( - c.resolve_change_prefix(&prefix("78")), + c.resolve_commit_prefix(repo.as_ref(), &prefix("20")), + NoMatch + ); + assert_eq!( + c.resolve_commit_prefix(repo.as_ref(), &prefix("2a0")), + NoMatch + ); + assert_eq!( + c.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()), + 1 + ); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("7")), + AmbiguousMatch + ); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("78")), SingleMatch(vec![commits[0].id().clone()]) ); - assert_eq!(c.resolve_change_prefix(&prefix("70")), NoMatch); - assert_eq!(c.resolve_change_prefix(&prefix("780")), NoMatch); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("70")), + NoMatch + ); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("780")), + NoMatch + ); // Disambiguate within a revset // --------------------------------------------------------------------------------------------- @@ -156,20 +186,26 @@ fn test_id_prefix() { RevsetExpression::commits(vec![commits[0].id().clone(), commits[2].id().clone()]); let c = c.disambiguate_within(expression, None); // The prefix is now shorter - assert_eq!(c.shortest_commit_prefix_len(commits[2].id()), 1); + assert_eq!( + c.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(&prefix("2")), + c.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(&prefix("21")), + c.resolve_commit_prefix(repo.as_ref(), &prefix("21")), SingleMatch(commits[24].id().clone()) ); - assert_eq!(c.shortest_change_prefix_len(commits[0].change_id()), 1); assert_eq!( - c.resolve_change_prefix(&prefix("7")), + c.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), + 1 + ); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("7")), SingleMatch(vec![commits[0].id().clone()]) ); @@ -178,16 +214,28 @@ fn test_id_prefix() { // --------------------------------------------------------------------------------------------- let expression = RevsetExpression::commit(root_commit_id.clone()); let c = c.disambiguate_within(expression, None); - assert_eq!(c.shortest_commit_prefix_len(root_commit_id), 1); - assert_eq!(c.resolve_commit_prefix(&prefix("")), AmbiguousMatch); assert_eq!( - c.resolve_commit_prefix(&prefix("0")), + c.shortest_commit_prefix_len(repo.as_ref(), root_commit_id), + 1 + ); + assert_eq!( + c.resolve_commit_prefix(repo.as_ref(), &prefix("")), + AmbiguousMatch + ); + assert_eq!( + c.resolve_commit_prefix(repo.as_ref(), &prefix("0")), SingleMatch(root_commit_id.clone()) ); - assert_eq!(c.shortest_change_prefix_len(root_change_id), 1); - assert_eq!(c.resolve_change_prefix(&prefix("")), AmbiguousMatch); assert_eq!( - c.resolve_change_prefix(&prefix("0")), + c.shortest_change_prefix_len(repo.as_ref(), root_change_id), + 1 + ); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("")), + AmbiguousMatch + ); + assert_eq!( + c.resolve_change_prefix(repo.as_ref(), &prefix("0")), SingleMatch(vec![root_commit_id.clone()]) ); @@ -196,6 +244,12 @@ fn test_id_prefix() { // TODO: Should be an error let expression = RevsetExpression::symbol("nonexistent".to_string()); let context = c.disambiguate_within(expression, None); - assert_eq!(context.shortest_commit_prefix_len(commits[2].id()), 2); - assert_eq!(context.resolve_commit_prefix(&prefix("2")), AmbiguousMatch); + assert_eq!( + context.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), + 2 + ); + assert_eq!( + context.resolve_commit_prefix(repo.as_ref(), &prefix("2")), + AmbiguousMatch + ); } diff --git a/src/cli_util.rs b/src/cli_util.rs index a629f087b9..167cb790b4 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -548,7 +548,7 @@ impl CommandHelper { /// data is lazily loaded. struct ReadonlyUserRepo { repo: Arc, - id_prefix_context: OnceCell>, + id_prefix_context: OnceCell, } impl ReadonlyUserRepo { @@ -590,7 +590,7 @@ impl WorkspaceCommandHelper { // Parse commit_summary template early to report error before starting mutable // operation. // TODO: Parsed template can be cached if it doesn't capture repo - let id_prefix_context = IdPrefixContext::new(repo.as_ref()); + let id_prefix_context = IdPrefixContext::default(); parse_commit_summary_template( repo.as_ref(), workspace.workspace_id(), @@ -925,18 +925,18 @@ impl WorkspaceCommandHelper { pub(crate) fn revset_symbol_resolver(&self) -> DefaultSymbolResolver<'_> { let id_prefix_context = self.id_prefix_context(); - let commit_id_resolver: revset::PrefixResolver<'_, CommitId> = - Box::new(|prefix| id_prefix_context.resolve_commit_prefix(prefix)); - let change_id_resolver: revset::PrefixResolver<'_, Vec> = - Box::new(|prefix| id_prefix_context.resolve_change_prefix(prefix)); + let commit_id_resolver: revset::PrefixResolver = + Box::new(|repo, prefix| id_prefix_context.resolve_commit_prefix(repo, prefix)); + let change_id_resolver: revset::PrefixResolver> = + Box::new(|repo, prefix| id_prefix_context.resolve_change_prefix(repo, prefix)); DefaultSymbolResolver::new(self.repo().as_ref(), Some(self.workspace_id())) .with_commit_id_resolver(commit_id_resolver) .with_change_id_resolver(change_id_resolver) } - pub fn id_prefix_context(&self) -> &IdPrefixContext<'_> { + pub fn id_prefix_context(&self) -> &IdPrefixContext { self.user_repo.id_prefix_context.get_or_init(|| { - let mut context: IdPrefixContext<'_> = IdPrefixContext::new(self.repo().as_ref()); + let mut context: IdPrefixContext = IdPrefixContext::default(); let revset_string: String = self .settings .config() @@ -947,7 +947,6 @@ impl WorkspaceCommandHelper { context = context .disambiguate_within(disambiguation_revset, Some(self.workspace_id().clone())); } - let context: IdPrefixContext<'static> = unsafe { std::mem::transmute(context) }; context }) } @@ -1295,7 +1294,7 @@ impl WorkspaceCommandTransaction<'_> { commit: &Commit, ) -> std::io::Result<()> { // TODO: Use the disambiguation revset - let id_prefix_context = IdPrefixContext::new(self.tx.repo()); + let id_prefix_context = IdPrefixContext::default(); let template = parse_commit_summary_template( self.tx.repo(), self.helper.workspace_id(), @@ -1734,7 +1733,7 @@ fn load_template_aliases( fn parse_commit_summary_template<'a>( repo: &'a dyn Repo, workspace_id: &WorkspaceId, - id_prefix_context: &'a IdPrefixContext<'a>, + id_prefix_context: &'a IdPrefixContext, aliases_map: &TemplateAliasesMap, settings: &UserSettings, ) -> Result + 'a>, CommandError> { diff --git a/src/commit_templater.rs b/src/commit_templater.rs index db679c60a6..7e38a4d4a6 100644 --- a/src/commit_templater.rs +++ b/src/commit_templater.rs @@ -40,7 +40,7 @@ use crate::text_util; struct CommitTemplateLanguage<'repo, 'b> { repo: &'repo dyn Repo, workspace_id: &'b WorkspaceId, - id_prefix_context: &'repo IdPrefixContext<'repo>, + id_prefix_context: &'repo IdPrefixContext, } impl<'repo> TemplateLanguage<'repo> for CommitTemplateLanguage<'repo, '_> { @@ -387,13 +387,14 @@ impl CommitOrChangeId { /// length of the shortest unique prefix pub fn shortest( &self, + repo: &dyn Repo, id_prefix_context: &IdPrefixContext, total_len: usize, ) -> ShortestIdPrefix { let mut hex = self.hex(); let prefix_len = match self { - CommitOrChangeId::Commit(id) => id_prefix_context.shortest_commit_prefix_len(id), - CommitOrChangeId::Change(id) => id_prefix_context.shortest_change_prefix_len(id), + CommitOrChangeId::Commit(id) => id_prefix_context.shortest_commit_prefix_len(repo, id), + CommitOrChangeId::Change(id) => id_prefix_context.shortest_change_prefix_len(repo, id), }; hex.truncate(max(prefix_len, total_len)); let rest = hex.split_off(prefix_len); @@ -434,6 +435,7 @@ fn build_commit_or_change_id_method<'repo>( (self_property, len_property), |(id, len)| { id.shortest( + language.repo, id_prefix_context, len.and_then(|l| l.try_into().ok()).unwrap_or(0), ) @@ -515,7 +517,7 @@ fn build_shortest_id_prefix_method<'repo>( pub fn parse<'repo>( repo: &'repo dyn Repo, workspace_id: &WorkspaceId, - id_prefix_context: &'repo IdPrefixContext<'repo>, + id_prefix_context: &'repo IdPrefixContext, template_text: &str, aliases_map: &TemplateAliasesMap, ) -> TemplateParseResult + 'repo>> {