From 6d65d98c0e1f2644b2fe5952806922971f9d212a Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 10 May 2023 23:26:23 -0700 Subject: [PATCH 01/14] cli: rename `ui.default-revset` to `revsets.log` I plan to add `revsets.short-prefixes` and `revsets.immutable` soon, and I think `[revsets]` seems like reasonable place to put them. It seems consistent with our `[templates]` section. However, it also suffers from the same problem as that section, which is that the difference between `[templates]` and `[template-aliases]` is not clear. We can decide about about templates and revsets later. --- CHANGELOG.md | 2 ++ docs/config.md | 9 +++++++++ lib/src/settings.rs | 14 +++++++++----- src/commands/mod.rs | 2 +- src/config-schema.json | 19 ++++++++++++++----- tests/test_log_command.rs | 4 ++-- 6 files changed, 37 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c15dedc749..63b0d70203 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 without arguments is now called `visible_heads()`. `heads()` with one argument is unchanged. +* The `ui.default-revset` config was renamed to `revsets.log`. + ### New features * `jj git push --deleted` will remove all locally deleted branches from the remote. diff --git a/docs/config.md b/docs/config.md index 182cbfa4af..e4cfdd7f01 100644 --- a/docs/config.md +++ b/docs/config.md @@ -141,6 +141,15 @@ ui.default-command = "log" ui.diff.format = "git" ``` +### Default revisions to log + +You can configure the revisions `jj log` without `-r` should show. + +```toml +# Show commits that are not in `main` +revsets.log = "main.." +``` + ### Graph style ```toml diff --git a/lib/src/settings.rs b/lib/src/settings.rs index 783c576d4b..92f72187cd 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -136,11 +136,15 @@ impl UserSettings { } pub fn default_revset(&self) -> String { - self.config - .get_string("ui.default-revset") - .unwrap_or_else(|_| { - "@ | (remote_branches() | tags()).. | ((remote_branches() | tags())..)-".to_string() - }) + self.config.get_string("revsets.log").unwrap_or_else(|_| { + // For compatibility with old config files (<0.8.0) + self.config + .get_string("ui.default-revset") + .unwrap_or_else(|_| { + "@ | (remote_branches() | tags()).. | ((remote_branches() | tags())..)-" + .to_string() + }) + }) } pub fn signature(&self) -> Signature { diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 97e604aa9d..0b2289425c 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -328,7 +328,7 @@ struct StatusArgs {} /// Show commit history #[derive(clap::Args, Clone, Debug)] struct LogArgs { - /// Which revisions to show. Defaults to the `ui.default-revset` setting, + /// Which revisions to show. Defaults to the `revsets.log` setting, /// or `@ | (remote_branches() | tags()).. | ((remote_branches() | /// tags())..)-` if it is not set. #[arg(long, short)] diff --git a/src/config-schema.json b/src/config-schema.json index 6638a972e4..6bcfbdf8b7 100644 --- a/src/config-schema.json +++ b/src/config-schema.json @@ -56,11 +56,6 @@ "description": "Default command to run when no explicit command is given", "default": "log" }, - "default-revset": { - "type": "string", - "description": "Default set of revisions to show when no explicit revset is given for jj log and similar commands", - "default": "@ | (remote_branches() | tags()).. | ((remote_branches() | tags())..)-" - }, "color": { "description": "Whether to colorize command output", "enum": [ @@ -267,6 +262,20 @@ } } }, + "revsets": { + "type": "object", + "description": "Revset expressions used by various commands", + "properties": { + "log": { + "type": "string", + "description": "Default set of revisions to show when no explicit revset is given for jj log and similar commands", + "default": "@ | (remote_branches() | tags()).. | ((remote_branches() | tags())..)-" + } + }, + "additionalProperties": { + "type": "string" + } + }, "revset-aliases": { "type": "object", "description": "Custom symbols/function aliases that can used in revset expressions", diff --git a/tests/test_log_command.rs b/tests/test_log_command.rs index c8b2ca7e54..d1c286e2b8 100644 --- a/tests/test_log_command.rs +++ b/tests/test_log_command.rs @@ -788,7 +788,7 @@ fn test_default_revset() { test_env.jj_cmd_success(&repo_path, &["describe", "-m", "add a file"]); // Set configuration to only show the root commit. - test_env.add_config(r#"ui.default-revset = "root""#); + test_env.add_config(r#"revsets.log = "root""#); // Log should only contain one line (for the root commit), and not show the // commit created above. @@ -813,7 +813,7 @@ fn test_default_revset_per_repo() { // Set configuration to only show the root commit. std::fs::write( repo_path.join(".jj/repo/config.toml"), - r#"ui.default-revset = "root""#, + r#"revsets.log = "root""#, ) .unwrap(); From cbbf588665977cafdada2fbc960e80194e5c0470 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 11:33:39 -0700 Subject: [PATCH 02/14] revset: introduce a trait for resolving symbols I'd like to make the symbol resolution more flexible, both so we can support customizing it (in custom `jj` binaries) and so we can use it for resolving short prefixes within a small revset. --- lib/src/revset.rs | 135 +++++++++++++++++++++++++++------------------- 1 file changed, 81 insertions(+), 54 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index dcedcb4ca3..93afac975e 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -436,7 +436,9 @@ impl RevsetExpression { self: Rc, repo: &dyn Repo, ) -> Result { - resolve_symbols(repo, self, None).map(|expression| resolve_visibility(repo, &expression)) + let symbol_resolver = DefaultSymbolResolver::new(repo, None); + resolve_symbols(repo, self, &symbol_resolver) + .map(|expression| resolve_visibility(repo, &expression)) } pub fn resolve_in_workspace( @@ -444,7 +446,8 @@ impl RevsetExpression { repo: &dyn Repo, workspace_ctx: &RevsetWorkspaceContext, ) -> Result { - resolve_symbols(repo, self, Some(workspace_ctx)) + let symbol_resolver = DefaultSymbolResolver::new(repo, Some(workspace_ctx.workspace_id)); + resolve_symbols(repo, self, &symbol_resolver) .map(|expression| resolve_visibility(repo, &expression)) } } @@ -1663,72 +1666,96 @@ fn resolve_change_id( } } -pub fn resolve_symbol( - repo: &dyn Repo, - symbol: &str, - workspace_id: Option<&WorkspaceId>, -) -> Result, RevsetResolutionError> { - if symbol.ends_with('@') { - let target_workspace = if symbol == "@" { - if let Some(workspace_id) = workspace_id { - workspace_id.clone() +pub trait SymbolResolver { + fn resolve_symbol(&self, symbol: &str) -> Result, RevsetResolutionError>; +} + +/// Resolves the "root" and "@" symbols, branches, remote branches, tags, git +/// refs, and full and abbreviated commit and change ids. +pub struct DefaultSymbolResolver<'a> { + repo: &'a dyn Repo, + workspace_id: Option<&'a WorkspaceId>, +} + +impl DefaultSymbolResolver<'_> { + pub fn new<'a>( + repo: &'a dyn Repo, + workspace_id: Option<&'a WorkspaceId>, + ) -> DefaultSymbolResolver<'a> { + DefaultSymbolResolver { repo, workspace_id } + } +} + +impl SymbolResolver for DefaultSymbolResolver<'_> { + fn resolve_symbol(&self, symbol: &str) -> Result, RevsetResolutionError> { + if symbol.ends_with('@') { + let target_workspace = if symbol == "@" { + if let Some(workspace_id) = self.workspace_id { + workspace_id.clone() + } else { + return Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())); + } } else { - return Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())); + WorkspaceId::new(symbol.strip_suffix('@').unwrap().to_string()) + }; + if let Some(commit_id) = self.repo.view().get_wc_commit_id(&target_workspace) { + Ok(vec![commit_id.clone()]) + } else { + Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())) } + } else if symbol == "root" { + Ok(vec![self.repo.store().root_commit_id().clone()]) } 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(RevsetResolutionError::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 tag + if let Some(target) = self.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 branch + if let Some(ids) = resolve_branch(self.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 git ref + if let Some(ids) = resolve_git_ref(self.repo, symbol) { + return Ok(ids); + } - // Try to resolve as a full commit id. - if let Some(ids) = resolve_full_commit_id(repo, symbol)? { - return Ok(ids); - } + // Try to resolve as a full commit id. + if let Some(ids) = resolve_full_commit_id(self.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 commit id. + if let Some(ids) = resolve_short_commit_id(self.repo, symbol)? { + return Ok(ids); + } - // Try to resolve as a change id. - if let Some(ids) = resolve_change_id(repo, symbol)? { - return Ok(ids); - } + // Try to resolve as a change id. + if let Some(ids) = resolve_change_id(self.repo, symbol)? { + return Ok(ids); + } - Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())) + Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())) + } } } +pub fn resolve_symbol( + repo: &dyn Repo, + symbol: &str, + workspace_id: Option<&WorkspaceId>, +) -> Result, RevsetResolutionError> { + DefaultSymbolResolver::new(repo, workspace_id).resolve_symbol(symbol) +} + fn resolve_commit_ref( repo: &dyn Repo, commit_ref: &RevsetCommitRef, - workspace_ctx: Option<&RevsetWorkspaceContext>, + symbol_resolver: &dyn SymbolResolver, ) -> Result, RevsetResolutionError> { match commit_ref { - RevsetCommitRef::Symbol(symbol) => { - resolve_symbol(repo, symbol, workspace_ctx.map(|ctx| ctx.workspace_id)) - } + RevsetCommitRef::Symbol(symbol) => symbol_resolver.resolve_symbol(symbol), RevsetCommitRef::VisibleHeads => Ok(repo.view().heads().iter().cloned().collect_vec()), RevsetCommitRef::Branches(needle) => { let mut commit_ids = vec![]; @@ -1786,14 +1813,14 @@ fn resolve_commit_ref( fn resolve_symbols( repo: &dyn Repo, expression: Rc, - workspace_ctx: Option<&RevsetWorkspaceContext>, + symbol_resolver: &dyn SymbolResolver, ) -> Result, RevsetResolutionError> { Ok(try_transform_expression( &expression, |expression| match expression.as_ref() { // 'present(x)' opens new symbol resolution scope to map error to 'none()'. RevsetExpression::Present(candidates) => { - resolve_symbols(repo, candidates.clone(), workspace_ctx) + resolve_symbols(repo, candidates.clone(), symbol_resolver) .or_else(|err| match err { RevsetResolutionError::NoSuchRevision(_) => Ok(RevsetExpression::none()), RevsetResolutionError::AmbiguousIdPrefix(_) @@ -1806,7 +1833,7 @@ fn resolve_symbols( }, |expression| match expression.as_ref() { RevsetExpression::CommitRef(commit_ref) => { - let commit_ids = resolve_commit_ref(repo, commit_ref, workspace_ctx)?; + let commit_ids = resolve_commit_ref(repo, commit_ref, symbol_resolver)?; Ok(Some(RevsetExpression::commits(commit_ids))) } _ => Ok(None), From 319341c6d9b4f3a2d97f77e732ec494c9ada681e Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 14:01:25 -0700 Subject: [PATCH 03/14] cli: make `WorkspaceCommandHelper` create `SymbolResolver` I would eventually want the `SymbolResolver` to be customizable (in custom `jj` binaries), so we want to make sure we always use the customized version of it. I left `RevsetExpression::resolve()` unchanged. I consider that to be for programmatically created expressions. --- lib/src/revset.rs | 7 +++---- lib/tests/test_revset.rs | 9 +++++---- src/cli_util.rs | 13 +++++++++---- src/commands/mod.rs | 3 ++- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 93afac975e..076338b851 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -441,13 +441,12 @@ impl RevsetExpression { .map(|expression| resolve_visibility(repo, &expression)) } - pub fn resolve_in_workspace( + pub fn resolve_user_expression( self: Rc, repo: &dyn Repo, - workspace_ctx: &RevsetWorkspaceContext, + symbol_resolver: &dyn SymbolResolver, ) -> Result { - let symbol_resolver = DefaultSymbolResolver::new(repo, Some(workspace_ctx.workspace_id)); - resolve_symbols(repo, self, &symbol_resolver) + resolve_symbols(repo, self, symbol_resolver) .map(|expression| resolve_visibility(repo, &expression)) } } diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index d5923b70b6..3899b9f5a2 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -24,9 +24,9 @@ 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_symbol, ReverseRevsetGraphIterator, Revset, RevsetAliasesMap, - RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, RevsetResolutionError, - RevsetWorkspaceContext, + optimize, parse, resolve_symbol, DefaultSymbolResolver, ReverseRevsetGraphIterator, Revset, + RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, + RevsetResolutionError, RevsetWorkspaceContext, }; use jujutsu_lib::settings::GitSettings; use jujutsu_lib::tree::merge_trees; @@ -521,8 +521,9 @@ fn resolve_commit_ids_in_workspace( }; let expression = optimize(parse(revset_str, &RevsetAliasesMap::new(), Some(&workspace_ctx)).unwrap()); + let symbol_resolver = DefaultSymbolResolver::new(repo, Some(workspace_ctx.workspace_id)); let expression = expression - .resolve_in_workspace(repo, &workspace_ctx) + .resolve_user_expression(repo, &symbol_resolver) .unwrap(); expression.evaluate(repo).unwrap().iter().collect() } diff --git a/src/cli_util.rs b/src/cli_util.rs index 61c30df991..efbb9b6d4d 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -44,8 +44,9 @@ use jujutsu_lib::repo::{ }; use jujutsu_lib::repo_path::{FsPathParseError, RepoPath}; use jujutsu_lib::revset::{ - Revset, RevsetAliasesMap, RevsetEvaluationError, RevsetExpression, RevsetIteratorExt, - RevsetParseError, RevsetParseErrorKind, RevsetResolutionError, RevsetWorkspaceContext, + DefaultSymbolResolver, Revset, RevsetAliasesMap, RevsetEvaluationError, RevsetExpression, + RevsetIteratorExt, RevsetParseError, RevsetParseErrorKind, RevsetResolutionError, + RevsetWorkspaceContext, }; use jujutsu_lib::settings::UserSettings; use jujutsu_lib::transaction::Transaction; @@ -883,8 +884,8 @@ impl WorkspaceCommandHelper { &'repo self, revset_expression: Rc, ) -> Result + 'repo>, CommandError> { - let revset_expression = - revset_expression.resolve_in_workspace(self.repo.as_ref(), &self.revset_context())?; + let revset_expression = revset_expression + .resolve_user_expression(self.repo.as_ref(), &self.revset_symbol_resolver())?; Ok(revset_expression.evaluate(self.repo.as_ref())?) } @@ -900,6 +901,10 @@ impl WorkspaceCommandHelper { } } + pub(crate) fn revset_symbol_resolver(&self) -> DefaultSymbolResolver<'_> { + DefaultSymbolResolver::new(self.repo().as_ref(), Some(self.workspace_id())) + } + pub fn template_aliases_map(&self) -> &TemplateAliasesMap { &self.template_aliases_map } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 0b2289425c..112e2a053b 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -3224,7 +3224,8 @@ fn cmd_debug_revset( writeln!(ui, "{expression:#?}")?; writeln!(ui)?; - let expression = expression.resolve_in_workspace(repo, &workspace_ctx)?; + let expression = + expression.resolve_user_expression(repo, &workspace_command.revset_symbol_resolver())?; writeln!(ui, "-- Resolved:")?; writeln!(ui, "{expression:#?}")?; writeln!(ui)?; From 1f6fa786c635c47313d84da076fa423ea798097b Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 15:50:37 -0700 Subject: [PATCH 04/14] revset: don't allow symbols in `RevsetExpression::resolve()` When creating `RevsetExpression` programmatically, I think we should use commit ids instead of symbols in the expression. This commit adds a check for that by using a `SymbolResolver` that always errors out. --- lib/src/revset.rs | 14 +++++++++++++- lib/tests/test_revset.rs | 11 ++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 076338b851..9647744505 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -436,7 +436,7 @@ impl RevsetExpression { self: Rc, repo: &dyn Repo, ) -> Result { - let symbol_resolver = DefaultSymbolResolver::new(repo, None); + let symbol_resolver = FailingSymbolResolver; resolve_symbols(repo, self, &symbol_resolver) .map(|expression| resolve_visibility(repo, &expression)) } @@ -1669,6 +1669,18 @@ pub trait SymbolResolver { fn resolve_symbol(&self, 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> { + Err(RevsetResolutionError::NoSuchRevision(format!( + "Won't resolve symbol {symbol:?}. When creating revsets programmatically, avoid using \ + RevsetExpression::symbol(); use RevsetExpression::commits() instead." + ))) + } +} + /// Resolves the "root" and "@" symbols, branches, remote branches, tags, git /// refs, and full and abbreviated commit and change ids. pub struct DefaultSymbolResolver<'a> { diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 3899b9f5a2..a4c72689d0 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -40,8 +40,9 @@ fn revset_for_commits<'index>( repo: &'index dyn Repo, commits: &[&Commit], ) -> Box + 'index> { + let symbol_resolver = DefaultSymbolResolver::new(repo, None); RevsetExpression::commits(commits.iter().map(|commit| commit.id().clone()).collect()) - .resolve(repo) + .resolve_user_expression(repo, &symbol_resolver) .unwrap() .evaluate(repo) .unwrap() @@ -173,8 +174,9 @@ fn test_resolve_symbol_commit_id() { // Test present() suppresses only NoSuchRevision error assert_eq!(resolve_commit_ids(repo.as_ref(), "present(foo)"), []); + let symbol_resolver = DefaultSymbolResolver::new(repo.as_ref(), None); assert_matches!( - optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()).resolve(repo.as_ref()), + optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()).resolve_user_expression(repo.as_ref(), &symbol_resolver), Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "04" ); assert_eq!( @@ -504,7 +506,10 @@ fn test_resolve_symbol_git_refs() { fn resolve_commit_ids(repo: &dyn Repo, revset_str: &str) -> Vec { let expression = optimize(parse(revset_str, &RevsetAliasesMap::new(), None).unwrap()); - let expression = expression.resolve(repo).unwrap(); + let symbol_resolver = DefaultSymbolResolver::new(repo, None); + let expression = expression + .resolve_user_expression(repo, &symbol_resolver) + .unwrap(); expression.evaluate(repo).unwrap().iter().collect() } From 578afd3ed9dd6c2ff6ca2461c17068dee6fcf31b Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 6 May 2023 16:44:43 -0700 Subject: [PATCH 05/14] cli: wrap repo in a struct to prepare for adding cached data I want to store some lazily calculated data associated with a repo. The data will depend on the user's config, which means it shouldn't live in the `ReadonlyRepo` itself. We could store it directly in `WorkspaceCommandHelper` - and I did that at first - but it's annoying and risky to remember to reset the cached data when we update the repo instance (which we do when a transaction finishes). This commit therefore introduces a wrapper type where we can store it. Having a wrapper also means that we can use `OnceCell` instead of more manually initializing it with a `RefCell`. --- src/cli_util.rs | 86 +++++++++++++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 35 deletions(-) diff --git a/src/cli_util.rs b/src/cli_util.rs index efbb9b6d4d..023fb910aa 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -542,6 +542,18 @@ impl CommandHelper { } } +/// A ReadonlyRepo along with user-config-dependent derived data. The derived +/// data is lazily loaded. +struct ReadonlyUserRepo { + repo: Arc, +} + +impl ReadonlyUserRepo { + fn new(repo: Arc) -> Self { + Self { repo } + } +} + // Provides utilities for writing a command that works on a workspace (like most // commands do). pub struct WorkspaceCommandHelper { @@ -550,7 +562,7 @@ pub struct WorkspaceCommandHelper { global_args: GlobalArgs, settings: UserSettings, workspace: Workspace, - repo: Arc, + user_repo: ReadonlyUserRepo, revset_aliases_map: RevsetAliasesMap, template_aliases_map: TemplateAliasesMap, may_update_working_copy: bool, @@ -595,7 +607,7 @@ impl WorkspaceCommandHelper { global_args: global_args.clone(), settings, workspace, - repo, + user_repo: ReadonlyUserRepo::new(repo), revset_aliases_map, template_aliases_map, may_update_working_copy, @@ -624,7 +636,7 @@ impl WorkspaceCommandHelper { pub fn snapshot(&mut self, ui: &mut Ui) -> Result<(), CommandError> { if self.may_update_working_copy { if self.working_copy_shared_with_git { - let maybe_git_repo = self.repo.store().git_repo(); + let maybe_git_repo = self.repo().store().git_repo(); self.import_git_refs_and_head(ui, maybe_git_repo.as_ref().unwrap())?; } self.snapshot_working_copy(ui)?; @@ -640,16 +652,16 @@ impl WorkspaceCommandHelper { let mut tx = self.start_transaction("import git refs").into_inner(); git::import_refs(tx.mut_repo(), git_repo, &self.settings.git_settings())?; if tx.mut_repo().has_changes() { - let old_git_head = self.repo.view().git_head().cloned(); + let old_git_head = self.repo().view().git_head().cloned(); let new_git_head = tx.mut_repo().view().git_head().cloned(); // If the Git HEAD has changed, abandon our old checkout and check out the new // Git HEAD. match new_git_head { Some(RefTarget::Normal(new_git_head_id)) if new_git_head != old_git_head => { let workspace_id = self.workspace_id().to_owned(); - let mut locked_working_copy = - self.workspace.working_copy_mut().start_mutation(); - if let Some(old_wc_commit_id) = self.repo.view().get_wc_commit_id(&workspace_id) + let op_id = self.repo().op_id().clone(); + if let Some(old_wc_commit_id) = + self.repo().view().get_wc_commit_id(&workspace_id) { tx.mut_repo() .record_abandoned_commit(old_wc_commit_id.clone()); @@ -657,13 +669,15 @@ impl WorkspaceCommandHelper { let new_git_head_commit = tx.mut_repo().store().get_commit(&new_git_head_id)?; tx.mut_repo() .check_out(workspace_id, &self.settings, &new_git_head_commit)?; + let mut locked_working_copy = + self.workspace.working_copy_mut().start_mutation(); // The working copy was presumably updated by the git command that updated // HEAD, so we just need to reset our working copy // state to it without updating working copy files. locked_working_copy.reset(&new_git_head_commit.tree())?; tx.mut_repo().rebase_descendants(&self.settings)?; - self.repo = tx.commit(); - locked_working_copy.finish(self.repo.op_id().clone()); + self.user_repo = ReadonlyUserRepo::new(tx.commit()); + locked_working_copy.finish(op_id); } _ => { let num_rebased = tx.mut_repo().rebase_descendants(&self.settings)?; @@ -710,7 +724,7 @@ impl WorkspaceCommandHelper { } pub fn repo(&self) -> &Arc { - &self.repo + &self.user_repo.repo } pub fn working_copy(&self) -> &WorkingCopy { @@ -722,7 +736,7 @@ impl WorkspaceCommandHelper { ) -> Result<(LockedWorkingCopy, Commit), CommandError> { self.check_working_copy_writable()?; let wc_commit = if let Some(wc_commit_id) = self.get_wc_commit_id() { - self.repo.store().get_commit(wc_commit_id)? + self.repo().store().get_commit(wc_commit_id)? } else { return Err(user_error("Nothing checked out in this workspace")); }; @@ -751,7 +765,7 @@ impl WorkspaceCommandHelper { } pub fn get_wc_commit_id(&self) -> Option<&CommitId> { - self.repo.view().get_wc_commit_id(self.workspace_id()) + self.repo().view().get_wc_commit_id(self.workspace_id()) } pub fn working_copy_shared_with_git(&self) -> bool { @@ -785,7 +799,7 @@ impl WorkspaceCommandHelper { } pub fn git_config(&self) -> Result { - if let Some(git_repo) = self.repo.store().git_repo() { + if let Some(git_repo) = self.repo().store().git_repo() { git_repo.config() } else { git2::Config::open_default() @@ -814,7 +828,7 @@ impl WorkspaceCommandHelper { { git_ignores = git_ignores.chain_with_file("", excludes_file_path); } - if let Some(git_repo) = self.repo.store().git_repo() { + if let Some(git_repo) = self.repo().store().git_repo() { git_ignores = git_ignores.chain_with_file("", git_repo.path().join("info").join("exclude")); } @@ -825,9 +839,9 @@ impl WorkspaceCommandHelper { // When resolving the "@" operation in a `ReadonlyRepo`, we resolve it to the // operation the repo was loaded at. resolve_single_op( - self.repo.op_store(), - self.repo.op_heads_store(), - || Ok(self.repo.operation().clone()), + self.repo().op_store(), + self.repo().op_heads_store(), + || Ok(self.repo().operation().clone()), op_str, ) } @@ -835,7 +849,7 @@ impl WorkspaceCommandHelper { pub fn resolve_single_rev(&self, revision_str: &str) -> Result { let revset_expression = self.parse_revset(revision_str)?; let revset = self.evaluate_revset(revset_expression)?; - let mut iter = revset.iter().commits(self.repo.store()).fuse(); + let mut iter = revset.iter().commits(self.repo().store()).fuse(); match (iter.next(), iter.next()) { (Some(commit), None) => Ok(commit?), (None, _) => Err(user_error(format!( @@ -865,7 +879,7 @@ impl WorkspaceCommandHelper { pub fn resolve_revset(&self, revision_str: &str) -> Result, CommandError> { let revset_expression = self.parse_revset(revision_str)?; let revset = self.evaluate_revset(revset_expression)?; - Ok(revset.iter().commits(self.repo.store()).try_collect()?) + Ok(revset.iter().commits(self.repo().store()).try_collect()?) } pub fn parse_revset( @@ -885,8 +899,8 @@ impl WorkspaceCommandHelper { revset_expression: Rc, ) -> Result + 'repo>, CommandError> { let revset_expression = revset_expression - .resolve_user_expression(self.repo.as_ref(), &self.revset_symbol_resolver())?; - Ok(revset_expression.evaluate(self.repo.as_ref())?) + .resolve_user_expression(self.repo().as_ref(), &self.revset_symbol_resolver())?; + Ok(revset_expression.evaluate(self.repo().as_ref())?) } pub(crate) fn revset_aliases_map(&self) -> &RevsetAliasesMap { @@ -914,7 +928,7 @@ impl WorkspaceCommandHelper { template_text: &str, ) -> Result + '_>, TemplateParseError> { commit_templater::parse( - self.repo.as_ref(), + self.repo().as_ref(), self.workspace_id(), template_text, &self.template_aliases_map, @@ -936,7 +950,7 @@ impl WorkspaceCommandHelper { commit: &Commit, ) -> std::io::Result<()> { let template = parse_commit_summary_template( - self.repo.as_ref(), + self.repo().as_ref(), self.workspace_id(), &self.template_aliases_map, &self.settings, @@ -946,7 +960,7 @@ impl WorkspaceCommandHelper { } pub fn check_rewritable(&self, commit: &Commit) -> Result<(), CommandError> { - if commit.id() == self.repo.store().root_commit_id() { + if commit.id() == self.repo().store().root_commit_id() { return Err(user_error("Cannot rewrite the root commit")); } Ok(()) @@ -960,7 +974,7 @@ impl WorkspaceCommandHelper { } pub fn snapshot_working_copy(&mut self, ui: &mut Ui) -> Result<(), CommandError> { - let repo = self.repo.clone(); + let repo = self.repo().clone(); let workspace_id = self.workspace_id().to_owned(); let wc_commit_id = match repo.view().get_wc_commit_id(&workspace_id) { Some(wc_commit_id) => wc_commit_id.clone(), @@ -974,7 +988,7 @@ impl WorkspaceCommandHelper { let mut locked_wc = self.workspace.working_copy_mut().start_mutation(); let old_op_id = locked_wc.old_operation_id().clone(); let wc_commit = repo.store().get_commit(&wc_commit_id)?; - self.repo = match check_stale_working_copy(&locked_wc, &wc_commit, repo.clone()) { + let repo = match check_stale_working_copy(&locked_wc, &wc_commit, repo.clone()) { Ok(repo) => repo, Err(StaleWorkingCopyError::WorkingCopyStale) => { locked_wc.discard(); @@ -1005,12 +1019,13 @@ impl WorkspaceCommandHelper { ))); } }; + self.user_repo = ReadonlyUserRepo::new(repo); let progress = crate::progress::snapshot_progress(ui); let new_tree_id = locked_wc.snapshot(base_ignores, progress.as_ref().map(|x| x as _))?; drop(progress); if new_tree_id != *wc_commit.tree_id() { let mut tx = start_repo_transaction( - &self.repo, + &self.user_repo.repo, &self.settings, &self.string_args, "snapshot working copy", @@ -1032,14 +1047,14 @@ impl WorkspaceCommandHelper { } if self.working_copy_shared_with_git { - let git_repo = self.repo.store().git_repo().unwrap(); + let git_repo = self.user_repo.repo.store().git_repo().unwrap(); let failed_branches = git::export_refs(mut_repo, &git_repo)?; print_failed_git_export(ui, &failed_branches)?; } - self.repo = tx.commit(); + self.user_repo = ReadonlyUserRepo::new(tx.commit()); } - locked_wc.finish(self.repo.op_id().clone()); + locked_wc.finish(self.user_repo.repo.op_id().clone()); Ok(()) } @@ -1050,14 +1065,14 @@ impl WorkspaceCommandHelper { ) -> Result<(), CommandError> { assert!(self.may_update_working_copy); let new_commit = match self.get_wc_commit_id() { - Some(commit_id) => self.repo.store().get_commit(commit_id)?, + Some(commit_id) => self.repo().store().get_commit(commit_id)?, None => { // It seems the workspace was deleted, so we shouldn't try to update it. return Ok(()); } }; let stats = update_working_copy( - &self.repo, + &self.user_repo.repo, self.workspace.working_copy_mut(), maybe_old_commit, &new_commit, @@ -1080,7 +1095,8 @@ impl WorkspaceCommandHelper { } pub fn start_transaction(&mut self, description: &str) -> WorkspaceCommandTransaction { - let tx = start_repo_transaction(&self.repo, &self.settings, &self.string_args, description); + let tx = + start_repo_transaction(self.repo(), &self.settings, &self.string_args, description); WorkspaceCommandTransaction { helper: self, tx } } @@ -1097,7 +1113,7 @@ impl WorkspaceCommandHelper { } if self.working_copy_shared_with_git { self.export_head_to_git(mut_repo)?; - let git_repo = self.repo.store().git_repo().unwrap(); + let git_repo = self.repo().store().git_repo().unwrap(); let failed_branches = git::export_refs(mut_repo, &git_repo)?; print_failed_git_export(ui, &failed_branches)?; } @@ -1107,7 +1123,7 @@ impl WorkspaceCommandHelper { .get_wc_commit_id(self.workspace_id()) .map(|commit_id| store.get_commit(commit_id)) .transpose()?; - self.repo = tx.commit(); + self.user_repo = ReadonlyUserRepo::new(tx.commit()); if self.may_update_working_copy { self.update_working_copy(ui, maybe_old_commit.as_ref())?; } From 7189c52f29de4dc852c5c314ccb6ed41c0913876 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Wed, 3 May 2023 17:54:08 -0700 Subject: [PATCH 06/14] templater: move id prefix shortening onto a new type I would like to copy Mercurial's way of abbreviating ids within a user-configurable revset. We would do it for both commit ids and change ids. For that feature, we need a place to keep the set of commits the revset evaluates to. This commit adds a new `IdPrefixContext` type which will eventually be that place. The new type has functions for going back and forth between full and abbreviated ids. I've updated the templater to use it. --- lib/src/id_prefix.rs | 51 +++++++++++++++++++++++++++++++++++++++++ lib/src/lib.rs | 1 + src/cli_util.rs | 24 ++++++++++++++++++- src/commit_templater.rs | 28 +++++++++++++++++----- 4 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 lib/src/id_prefix.rs diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs new file mode 100644 index 0000000000..ec9cc1471c --- /dev/null +++ b/lib/src/id_prefix.rs @@ -0,0 +1,51 @@ +// Copyright 2023 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::backend::{ChangeId, CommitId}; +use crate::index::{HexPrefix, PrefixResolution}; +use crate::repo::Repo; + +pub struct IdPrefixContext<'repo> { + repo: &'repo dyn Repo, +} + +impl IdPrefixContext<'_> { + pub fn new(repo: &dyn Repo) -> IdPrefixContext { + IdPrefixContext { repo } + } + + /// Resolve an unambiguous commit ID prefix. + pub fn resolve_commit_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + self.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 { + self.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> { + self.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 { + self.repo.shortest_unique_change_id_prefix_len(change_id) + } +} diff --git a/lib/src/lib.rs b/lib/src/lib.rs index 2cbf7e7ef9..affc1c073b 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -32,6 +32,7 @@ pub mod git; pub mod git_backend; pub mod gitignore; pub mod hex_util; +pub mod id_prefix; pub mod index; pub mod local_backend; pub mod lock; diff --git a/src/cli_util.rs b/src/cli_util.rs index 023fb910aa..c99824570f 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -34,6 +34,7 @@ use jujutsu_lib::commit::Commit; use jujutsu_lib::git::{GitExportError, GitImportError}; use jujutsu_lib::gitignore::GitIgnoreFile; use jujutsu_lib::hex_util::to_reverse_hex; +use jujutsu_lib::id_prefix::IdPrefixContext; use jujutsu_lib::matchers::{EverythingMatcher, Matcher, PrefixMatcher, Visit}; use jujutsu_lib::op_heads_store::{self, OpHeadResolutionError, OpHeadsStore}; use jujutsu_lib::op_store::{OpStore, OpStoreError, OperationId, RefTarget, WorkspaceId}; @@ -56,6 +57,7 @@ use jujutsu_lib::working_copy::{ }; use jujutsu_lib::workspace::{Workspace, WorkspaceInitError, WorkspaceLoadError, WorkspaceLoader}; use jujutsu_lib::{dag_walk, file_util, git, revset}; +use once_cell::unsync::OnceCell; use thiserror::Error; use toml_edit; use tracing_subscriber::prelude::*; @@ -546,11 +548,15 @@ impl CommandHelper { /// data is lazily loaded. struct ReadonlyUserRepo { repo: Arc, + id_prefix_context: OnceCell>, } impl ReadonlyUserRepo { fn new(repo: Arc) -> Self { - Self { repo } + Self { + repo, + id_prefix_context: OnceCell::new(), + } } } @@ -584,9 +590,11 @@ 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()); parse_commit_summary_template( repo.as_ref(), workspace.workspace_id(), + &id_prefix_context, &template_aliases_map, &settings, )?; @@ -919,6 +927,14 @@ impl WorkspaceCommandHelper { DefaultSymbolResolver::new(self.repo().as_ref(), Some(self.workspace_id())) } + pub fn id_prefix_context(&self) -> &IdPrefixContext<'_> { + self.user_repo.id_prefix_context.get_or_init(|| { + let context: IdPrefixContext<'_> = IdPrefixContext::new(self.user_repo.repo.as_ref()); + let context: IdPrefixContext<'static> = unsafe { std::mem::transmute(context) }; + context + }) + } + pub fn template_aliases_map(&self) -> &TemplateAliasesMap { &self.template_aliases_map } @@ -930,6 +946,7 @@ impl WorkspaceCommandHelper { commit_templater::parse( self.repo().as_ref(), self.workspace_id(), + self.id_prefix_context(), template_text, &self.template_aliases_map, ) @@ -952,6 +969,7 @@ impl WorkspaceCommandHelper { let template = parse_commit_summary_template( self.repo().as_ref(), self.workspace_id(), + self.id_prefix_context(), &self.template_aliases_map, &self.settings, ) @@ -1259,9 +1277,11 @@ impl WorkspaceCommandTransaction<'_> { formatter: &mut dyn Formatter, commit: &Commit, ) -> std::io::Result<()> { + let id_prefix_context = IdPrefixContext::new(self.tx.repo()); let template = parse_commit_summary_template( self.tx.repo(), self.helper.workspace_id(), + &id_prefix_context, &self.helper.template_aliases_map, &self.helper.settings, ) @@ -1696,6 +1716,7 @@ fn load_template_aliases( fn parse_commit_summary_template<'a>( repo: &'a dyn Repo, workspace_id: &WorkspaceId, + id_prefix_context: &'a IdPrefixContext<'a>, aliases_map: &TemplateAliasesMap, settings: &UserSettings, ) -> Result + 'a>, CommandError> { @@ -1703,6 +1724,7 @@ fn parse_commit_summary_template<'a>( Ok(commit_templater::parse( repo, workspace_id, + id_prefix_context, &template_text, aliases_map, )?) diff --git a/src/commit_templater.rs b/src/commit_templater.rs index 27c9d2419e..db679c60a6 100644 --- a/src/commit_templater.rs +++ b/src/commit_templater.rs @@ -19,6 +19,7 @@ use itertools::Itertools as _; use jujutsu_lib::backend::{ChangeId, CommitId, ObjectId as _}; use jujutsu_lib::commit::Commit; use jujutsu_lib::hex_util::to_reverse_hex; +use jujutsu_lib::id_prefix::IdPrefixContext; use jujutsu_lib::op_store::WorkspaceId; use jujutsu_lib::repo::Repo; use jujutsu_lib::rewrite; @@ -39,6 +40,7 @@ use crate::text_util; struct CommitTemplateLanguage<'repo, 'b> { repo: &'repo dyn Repo, workspace_id: &'b WorkspaceId, + id_prefix_context: &'repo IdPrefixContext<'repo>, } impl<'repo> TemplateLanguage<'repo> for CommitTemplateLanguage<'repo, '_> { @@ -383,11 +385,15 @@ impl CommitOrChangeId { /// The length of the id printed will be the maximum of `total_len` and the /// length of the shortest unique prefix - pub fn shortest(&self, repo: &dyn Repo, total_len: usize) -> ShortestIdPrefix { + pub fn shortest( + &self, + id_prefix_context: &IdPrefixContext, + total_len: usize, + ) -> ShortestIdPrefix { let mut hex = self.hex(); let prefix_len = match self { - CommitOrChangeId::Commit(id) => repo.index().shortest_unique_commit_id_prefix_len(id), - CommitOrChangeId::Change(id) => repo.shortest_unique_change_id_prefix_len(id), + CommitOrChangeId::Commit(id) => id_prefix_context.shortest_commit_prefix_len(id), + CommitOrChangeId::Change(id) => id_prefix_context.shortest_change_prefix_len(id), }; hex.truncate(max(prefix_len, total_len)); let rest = hex.split_off(prefix_len); @@ -422,11 +428,16 @@ fn build_commit_or_change_id_method<'repo>( )) } "shortest" => { - let repo = language.repo; + let id_prefix_context = &language.id_prefix_context; let len_property = parse_optional_integer(function)?; language.wrap_shortest_id_prefix(TemplateFunction::new( (self_property, len_property), - |(id, len)| id.shortest(repo, len.and_then(|l| l.try_into().ok()).unwrap_or(0)), + |(id, len)| { + id.shortest( + id_prefix_context, + len.and_then(|l| l.try_into().ok()).unwrap_or(0), + ) + }, )) } _ => { @@ -504,10 +515,15 @@ fn build_shortest_id_prefix_method<'repo>( pub fn parse<'repo>( repo: &'repo dyn Repo, workspace_id: &WorkspaceId, + id_prefix_context: &'repo IdPrefixContext<'repo>, template_text: &str, aliases_map: &TemplateAliasesMap, ) -> TemplateParseResult + 'repo>> { - let language = CommitTemplateLanguage { repo, workspace_id }; + let language = CommitTemplateLanguage { + repo, + workspace_id, + id_prefix_context, + }; let node = template_parser::parse(template_text, aliases_map)?; template_builder::build(&language, &node) } From 36d1cc01f436b92b8a90ad7b955c1b1f0f8c7004 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 16:45:52 -0700 Subject: [PATCH 07/14] revset: inline resolution of change/commit ids This prepares for adding callbacks to resolve these ids. --- lib/src/revset.rs | 62 ++++++++++++++++++----------------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 9647744505..397f720440 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1631,40 +1631,6 @@ fn resolve_full_commit_id( } } -fn resolve_short_commit_id( - repo: &dyn Repo, - symbol: &str, -) -> Result>, RevsetResolutionError> { - if let Some(prefix) = HexPrefix::new(symbol) { - match repo.index().resolve_prefix(&prefix) { - PrefixResolution::NoMatch => Ok(None), - PrefixResolution::AmbiguousMatch => { - Err(RevsetResolutionError::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>, RevsetResolutionError> { - 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(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned())) - } - PrefixResolution::SingleMatch(entries) => Ok(Some(entries)), - } - } else { - Ok(None) - } -} - pub trait SymbolResolver { fn resolve_symbol(&self, symbol: &str) -> Result, RevsetResolutionError>; } @@ -1738,13 +1704,33 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { } // Try to resolve as a commit id. - if let Some(ids) = resolve_short_commit_id(self.repo, symbol)? { - return Ok(ids); + if let Some(prefix) = HexPrefix::new(symbol) { + match self.repo.index().resolve_prefix(&prefix) { + PrefixResolution::AmbiguousMatch => { + return Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned())); + } + PrefixResolution::SingleMatch(id) => { + return Ok(vec![id]); + } + PrefixResolution::NoMatch => { + // Fall through + } + } } // Try to resolve as a change id. - if let Some(ids) = resolve_change_id(self.repo, symbol)? { - return Ok(ids); + if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) { + match self.repo.resolve_change_id_prefix(&prefix) { + PrefixResolution::AmbiguousMatch => { + return Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned())); + } + PrefixResolution::SingleMatch(ids) => { + return Ok(ids); + } + PrefixResolution::NoMatch => { + // Fall through + } + } } Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())) From 593cd8c791f77984cce682ec5f655277cec87ae2 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 16:53:27 -0700 Subject: [PATCH 08/14] revset: use IdPrefixContext for resolving commit/change ids This is another step towards resolving abbreviated commit ids within a configured revset. --- lib/src/revset.rs | 48 +++++++++++++++++++++++++++++++++++++++-------- src/cli_util.rs | 7 +++++++ 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 397f720440..49d85d4494 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1647,19 +1647,41 @@ impl SymbolResolver for FailingSymbolResolver { } } +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. pub struct DefaultSymbolResolver<'a> { repo: &'a dyn Repo, workspace_id: Option<&'a WorkspaceId>, + commit_id_resolver: Option>, + change_id_resolver: Option>>, } -impl DefaultSymbolResolver<'_> { - pub fn new<'a>( - repo: &'a dyn Repo, - workspace_id: Option<&'a WorkspaceId>, - ) -> DefaultSymbolResolver<'a> { - DefaultSymbolResolver { repo, workspace_id } +impl<'a> DefaultSymbolResolver<'a> { + pub fn new(repo: &'a dyn Repo, workspace_id: Option<&'a WorkspaceId>) -> Self { + DefaultSymbolResolver { + repo, + workspace_id, + commit_id_resolver: None, + change_id_resolver: None, + } + } + + pub fn with_commit_id_resolver( + mut self, + commit_id_resolver: PrefixResolver<'a, CommitId>, + ) -> Self { + self.commit_id_resolver = Some(commit_id_resolver); + self + } + + pub fn with_change_id_resolver( + mut self, + change_id_resolver: PrefixResolver<'a, Vec>, + ) -> Self { + self.change_id_resolver = Some(change_id_resolver); + self } } @@ -1705,7 +1727,12 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { // Try to resolve as a commit id. if let Some(prefix) = HexPrefix::new(symbol) { - match self.repo.index().resolve_prefix(&prefix) { + let prefix_resolution = if let Some(commit_id_resolver) = &self.commit_id_resolver { + commit_id_resolver(&prefix) + } else { + self.repo.index().resolve_prefix(&prefix) + }; + match prefix_resolution { PrefixResolution::AmbiguousMatch => { return Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned())); } @@ -1720,7 +1747,12 @@ 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) { - match self.repo.resolve_change_id_prefix(&prefix) { + let prefix_resolution = if let Some(change_id_resolver) = &self.change_id_resolver { + change_id_resolver(&prefix) + } else { + self.repo.resolve_change_id_prefix(&prefix) + }; + match prefix_resolution { PrefixResolution::AmbiguousMatch => { return Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned())); } diff --git a/src/cli_util.rs b/src/cli_util.rs index c99824570f..b8cca08e10 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -924,7 +924,14 @@ 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)); 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<'_> { From 8ff5e1c16d767d8bf05667b73a3d33317f93a6b7 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 16:11:42 -0700 Subject: [PATCH 09/14] prefixes: move `IdIndex` to `id_prefix` module I'll reuse it there next. --- lib/src/default_revset_engine.rs | 187 +----------------------------- lib/src/id_prefix.rs | 190 ++++++++++++++++++++++++++++++- 2 files changed, 192 insertions(+), 185 deletions(-) diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index bfa9ba4fd8..fcbce9476a 100644 --- a/lib/src/default_revset_engine.rs +++ b/lib/src/default_revset_engine.rs @@ -21,11 +21,12 @@ use std::sync::Arc; use itertools::Itertools; -use crate::backend::{ChangeId, CommitId, MillisSinceEpoch, ObjectId}; +use crate::backend::{ChangeId, CommitId, MillisSinceEpoch}; use crate::default_index_store::{ CompositeIndex, IndexEntry, IndexEntryByPosition, IndexPosition, RevWalk, }; use crate::default_revset_graph_iterator::RevsetGraphIterator; +use crate::id_prefix::IdIndex; use crate::index::{HexPrefix, Index, PrefixResolution}; use crate::matchers::{EverythingMatcher, Matcher, PrefixMatcher, Visit}; use crate::repo_path::RepoPath; @@ -33,8 +34,8 @@ use crate::revset::{ ChangeIdIndex, ResolvedExpression, ResolvedPredicateExpression, Revset, RevsetEvaluationError, RevsetFilterPredicate, RevsetGraphEdge, GENERATION_RANGE_FULL, }; +use crate::rewrite; use crate::store::Store; -use crate::{backend, rewrite}; trait ToPredicateFn: fmt::Debug { /// Creates function that tests if the given entry is included in the set. @@ -130,82 +131,6 @@ impl ChangeIdIndex for ChangeIdIndexImpl<'_> { } } -#[derive(Debug, Clone)] -struct IdIndex(Vec<(K, V)>); - -impl IdIndex -where - K: ObjectId + Ord, -{ - /// Creates new index from the given entries. Multiple values can be - /// associated with a single key. - pub fn from_vec(mut vec: Vec<(K, V)>) -> Self { - vec.sort_unstable_by(|(k0, _), (k1, _)| k0.cmp(k1)); - IdIndex(vec) - } - - /// Looks up entries with the given prefix, and collects values if matched - /// entries have unambiguous keys. - pub fn resolve_prefix_with( - &self, - prefix: &HexPrefix, - mut value_mapper: impl FnMut(&V) -> U, - ) -> PrefixResolution> { - let mut range = self.resolve_prefix_range(prefix).peekable(); - if let Some((first_key, _)) = range.peek().copied() { - let maybe_entries: Option> = range - .map(|(k, v)| (k == first_key).then(|| value_mapper(v))) - .collect(); - if let Some(entries) = maybe_entries { - PrefixResolution::SingleMatch(entries) - } else { - PrefixResolution::AmbiguousMatch - } - } else { - PrefixResolution::NoMatch - } - } - - /// Iterates over entries with the given prefix. - pub fn resolve_prefix_range<'a: 'b, 'b>( - &'a self, - prefix: &'b HexPrefix, - ) -> impl Iterator + 'b { - let min_bytes = prefix.min_prefix_bytes(); - let pos = self.0.partition_point(|(k, _)| k.as_bytes() < min_bytes); - self.0[pos..] - .iter() - .take_while(|(k, _)| prefix.matches(k)) - .map(|(k, v)| (k, v)) - } - - /// This function returns the shortest length of a prefix of `key` that - /// disambiguates it from every other key in the index. - /// - /// The length to be returned is a number of hexadecimal digits. - /// - /// This has some properties that we do not currently make much use of: - /// - /// - The algorithm works even if `key` itself is not in the index. - /// - /// - In the special case when there are keys in the trie for which our - /// `key` is an exact prefix, returns `key.len() + 1`. Conceptually, in - /// order to disambiguate, you need every letter of the key *and* the - /// additional fact that it's the entire key). This case is extremely - /// unlikely for hashes with 12+ hexadecimal characters. - pub fn shortest_unique_prefix_len(&self, key: &K) -> usize { - let pos = self.0.partition_point(|(k, _)| k < key); - let left = pos.checked_sub(1).map(|p| &self.0[p]); - let right = self.0[pos..].iter().find(|(k, _)| k != key); - itertools::chain(left, right) - .map(|(neighbor, _value)| { - backend::common_hex_len(key.as_bytes(), neighbor.as_bytes()) + 1 - }) - .max() - .unwrap_or(0) - } -} - #[derive(Debug)] struct EagerRevset<'index> { index_entries: Vec>, @@ -954,112 +879,6 @@ mod tests { use crate::backend::{ChangeId, CommitId, ObjectId}; use crate::default_index_store::MutableIndexImpl; - #[test] - fn test_id_index_resolve_prefix() { - fn sorted(resolution: PrefixResolution>) -> PrefixResolution> { - match resolution { - PrefixResolution::SingleMatch(mut xs) => { - xs.sort(); // order of values might not be preserved by IdIndex - PrefixResolution::SingleMatch(xs) - } - _ => resolution, - } - } - let id_index = IdIndex::from_vec(vec![ - (ChangeId::from_hex("0000"), 0), - (ChangeId::from_hex("0099"), 1), - (ChangeId::from_hex("0099"), 2), - (ChangeId::from_hex("0aaa"), 3), - (ChangeId::from_hex("0aab"), 4), - ]); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0").unwrap(), |&v| v), - PrefixResolution::AmbiguousMatch, - ); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("00").unwrap(), |&v| v), - PrefixResolution::AmbiguousMatch, - ); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("000").unwrap(), |&v| v), - PrefixResolution::SingleMatch(vec![0]), - ); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0001").unwrap(), |&v| v), - PrefixResolution::NoMatch, - ); - assert_eq!( - sorted(id_index.resolve_prefix_with(&HexPrefix::new("009").unwrap(), |&v| v)), - PrefixResolution::SingleMatch(vec![1, 2]), - ); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0aa").unwrap(), |&v| v), - PrefixResolution::AmbiguousMatch, - ); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0aab").unwrap(), |&v| v), - PrefixResolution::SingleMatch(vec![4]), - ); - assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("f").unwrap(), |&v| v), - PrefixResolution::NoMatch, - ); - } - - #[test] - fn test_id_index_shortest_unique_prefix_len() { - // No crash if empty - let id_index = IdIndex::from_vec(vec![] as Vec<(ChangeId, ())>); - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("00")), - 0 - ); - - let id_index = IdIndex::from_vec(vec![ - (ChangeId::from_hex("ab"), ()), - (ChangeId::from_hex("acd0"), ()), - (ChangeId::from_hex("acd0"), ()), // duplicated key is allowed - ]); - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("acd0")), - 2 - ); - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("ac")), - 3 - ); - - let id_index = IdIndex::from_vec(vec![ - (ChangeId::from_hex("ab"), ()), - (ChangeId::from_hex("acd0"), ()), - (ChangeId::from_hex("acf0"), ()), - (ChangeId::from_hex("a0"), ()), - (ChangeId::from_hex("ba"), ()), - ]); - - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("a0")), - 2 - ); - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("ba")), - 1 - ); - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("ab")), - 2 - ); - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("acd0")), - 3 - ); - // If it were there, the length would be 1. - assert_eq!( - id_index.shortest_unique_prefix_len(&ChangeId::from_hex("c0")), - 1 - ); - } - /// Generator of unique 16-byte ChangeId excluding root id fn change_id_generator() -> impl FnMut() -> ChangeId { let mut iter = (1_u128..).map(|n| ChangeId::new(n.to_le_bytes().into())); diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index ec9cc1471c..347b74bfa7 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::backend::{ChangeId, CommitId}; +use crate::backend::{self, ChangeId, CommitId, ObjectId}; use crate::index::{HexPrefix, PrefixResolution}; use crate::repo::Repo; @@ -49,3 +49,191 @@ impl IdPrefixContext<'_> { self.repo.shortest_unique_change_id_prefix_len(change_id) } } + +#[derive(Debug, Clone)] +pub struct IdIndex(Vec<(K, V)>); + +impl IdIndex +where + K: ObjectId + Ord, +{ + /// Creates new index from the given entries. Multiple values can be + /// associated with a single key. + pub fn from_vec(mut vec: Vec<(K, V)>) -> Self { + vec.sort_unstable_by(|(k0, _), (k1, _)| k0.cmp(k1)); + IdIndex(vec) + } + + /// Looks up entries with the given prefix, and collects values if matched + /// entries have unambiguous keys. + pub fn resolve_prefix_with( + &self, + prefix: &HexPrefix, + mut value_mapper: impl FnMut(&V) -> U, + ) -> PrefixResolution> { + let mut range = self.resolve_prefix_range(prefix).peekable(); + if let Some((first_key, _)) = range.peek().copied() { + let maybe_entries: Option> = range + .map(|(k, v)| (k == first_key).then(|| value_mapper(v))) + .collect(); + if let Some(entries) = maybe_entries { + PrefixResolution::SingleMatch(entries) + } else { + PrefixResolution::AmbiguousMatch + } + } else { + PrefixResolution::NoMatch + } + } + + /// Iterates over entries with the given prefix. + pub fn resolve_prefix_range<'a: 'b, 'b>( + &'a self, + prefix: &'b HexPrefix, + ) -> impl Iterator + 'b { + let min_bytes = prefix.min_prefix_bytes(); + let pos = self.0.partition_point(|(k, _)| k.as_bytes() < min_bytes); + self.0[pos..] + .iter() + .take_while(|(k, _)| prefix.matches(k)) + .map(|(k, v)| (k, v)) + } + + /// This function returns the shortest length of a prefix of `key` that + /// disambiguates it from every other key in the index. + /// + /// The length to be returned is a number of hexadecimal digits. + /// + /// This has some properties that we do not currently make much use of: + /// + /// - The algorithm works even if `key` itself is not in the index. + /// + /// - In the special case when there are keys in the trie for which our + /// `key` is an exact prefix, returns `key.len() + 1`. Conceptually, in + /// order to disambiguate, you need every letter of the key *and* the + /// additional fact that it's the entire key). This case is extremely + /// unlikely for hashes with 12+ hexadecimal characters. + pub fn shortest_unique_prefix_len(&self, key: &K) -> usize { + let pos = self.0.partition_point(|(k, _)| k < key); + let left = pos.checked_sub(1).map(|p| &self.0[p]); + let right = self.0[pos..].iter().find(|(k, _)| k != key); + itertools::chain(left, right) + .map(|(neighbor, _value)| { + backend::common_hex_len(key.as_bytes(), neighbor.as_bytes()) + 1 + }) + .max() + .unwrap_or(0) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::backend::{ChangeId, ObjectId}; + + #[test] + fn test_id_index_resolve_prefix() { + fn sorted(resolution: PrefixResolution>) -> PrefixResolution> { + match resolution { + PrefixResolution::SingleMatch(mut xs) => { + xs.sort(); // order of values might not be preserved by IdIndex + PrefixResolution::SingleMatch(xs) + } + _ => resolution, + } + } + let id_index = IdIndex::from_vec(vec![ + (ChangeId::from_hex("0000"), 0), + (ChangeId::from_hex("0099"), 1), + (ChangeId::from_hex("0099"), 2), + (ChangeId::from_hex("0aaa"), 3), + (ChangeId::from_hex("0aab"), 4), + ]); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("0").unwrap(), |&v| v), + PrefixResolution::AmbiguousMatch, + ); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("00").unwrap(), |&v| v), + PrefixResolution::AmbiguousMatch, + ); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("000").unwrap(), |&v| v), + PrefixResolution::SingleMatch(vec![0]), + ); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("0001").unwrap(), |&v| v), + PrefixResolution::NoMatch, + ); + assert_eq!( + sorted(id_index.resolve_prefix_with(&HexPrefix::new("009").unwrap(), |&v| v)), + PrefixResolution::SingleMatch(vec![1, 2]), + ); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("0aa").unwrap(), |&v| v), + PrefixResolution::AmbiguousMatch, + ); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("0aab").unwrap(), |&v| v), + PrefixResolution::SingleMatch(vec![4]), + ); + assert_eq!( + id_index.resolve_prefix_with(&HexPrefix::new("f").unwrap(), |&v| v), + PrefixResolution::NoMatch, + ); + } + + #[test] + fn test_id_index_shortest_unique_prefix_len() { + // No crash if empty + let id_index = IdIndex::from_vec(vec![] as Vec<(ChangeId, ())>); + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("00")), + 0 + ); + + let id_index = IdIndex::from_vec(vec![ + (ChangeId::from_hex("ab"), ()), + (ChangeId::from_hex("acd0"), ()), + (ChangeId::from_hex("acd0"), ()), // duplicated key is allowed + ]); + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("acd0")), + 2 + ); + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("ac")), + 3 + ); + + let id_index = IdIndex::from_vec(vec![ + (ChangeId::from_hex("ab"), ()), + (ChangeId::from_hex("acd0"), ()), + (ChangeId::from_hex("acf0"), ()), + (ChangeId::from_hex("a0"), ()), + (ChangeId::from_hex("ba"), ()), + ]); + + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("a0")), + 2 + ); + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("ba")), + 1 + ); + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("ab")), + 2 + ); + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("acd0")), + 3 + ); + // If it were there, the length would be 1. + assert_eq!( + id_index.shortest_unique_prefix_len(&ChangeId::from_hex("c0")), + 1 + ); + } +} From 3b072b28acf23b6af639e95c856366ff5d254d8f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 7 May 2023 08:42:16 -0700 Subject: [PATCH 10/14] id_prefix: add `IdIndex::resolve_prefix()` I'll use this in `IdPrefixContext` soon. --- lib/src/id_prefix.rs | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index 347b74bfa7..59d976ae61 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -86,6 +86,15 @@ where } } + /// Looks up entries with the given prefix, and collects values if matched + /// entries have unambiguous keys. + pub fn resolve_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> + where + V: Clone, + { + self.resolve_prefix_with(prefix, |v: &V| v.clone()) + } + /// Iterates over entries with the given prefix. pub fn resolve_prefix_range<'a: 'b, 'b>( &'a self, @@ -150,35 +159,35 @@ mod tests { (ChangeId::from_hex("0aab"), 4), ]); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("0").unwrap()), PrefixResolution::AmbiguousMatch, ); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("00").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("00").unwrap()), PrefixResolution::AmbiguousMatch, ); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("000").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("000").unwrap()), PrefixResolution::SingleMatch(vec![0]), ); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0001").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("0001").unwrap()), PrefixResolution::NoMatch, ); assert_eq!( - sorted(id_index.resolve_prefix_with(&HexPrefix::new("009").unwrap(), |&v| v)), + sorted(id_index.resolve_prefix(&HexPrefix::new("009").unwrap())), PrefixResolution::SingleMatch(vec![1, 2]), ); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0aa").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("0aa").unwrap()), PrefixResolution::AmbiguousMatch, ); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("0aab").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("0aab").unwrap()), PrefixResolution::SingleMatch(vec![4]), ); assert_eq!( - id_index.resolve_prefix_with(&HexPrefix::new("f").unwrap(), |&v| v), + id_index.resolve_prefix(&HexPrefix::new("f").unwrap()), PrefixResolution::NoMatch, ); } From 5a63eeb872bbabf17d6d9966d4b39306761c3f0f Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 8 May 2023 06:30:25 -0700 Subject: [PATCH 11/14] id_prefix: add `IdIndex::has_key()` For the support for shorter prefixes within a revset, we'll want to be able to check if an id is in the index. --- lib/src/id_prefix.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index 59d976ae61..f081fe653e 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -108,6 +108,10 @@ where .map(|(k, v)| (k, v)) } + pub fn has_key(&self, key: &K) -> bool { + self.0.binary_search_by(|(k, _)| k.cmp(key)).is_ok() + } + /// This function returns the shortest length of a prefix of `key` that /// disambiguates it from every other key in the index. /// @@ -192,6 +196,18 @@ mod tests { ); } + #[test] + fn test_has_key() { + // No crash if empty + let id_index = IdIndex::from_vec(vec![] as Vec<(ChangeId, ())>); + assert!(!id_index.has_key(&ChangeId::from_hex("00"))); + + let id_index = IdIndex::from_vec(vec![(ChangeId::from_hex("ab"), ())]); + assert!(!id_index.has_key(&ChangeId::from_hex("aa"))); + assert!(id_index.has_key(&ChangeId::from_hex("ab"))); + assert!(!id_index.has_key(&ChangeId::from_hex("ac"))); + } + #[test] fn test_id_index_shortest_unique_prefix_len() { // No crash if empty From 2ff8124ec320a11a7ef0963516a590b635d14826 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 17:12:08 -0700 Subject: [PATCH 12/14] prefixes: allow resolving shorter ids within a revset In large repos, the unique prefixes can get somewhat long (~6 hex digits seems typical in the Linux repo), which makes them less useful for manually entering on the CLI. The user typically cares most about a small set of commits, so it would be nice to give shorter unique ids to those. That's what Mercurial enables with its `experimental.revisions.disambiguatewithin` config. This commit provides an implementation of that feature in `IdPrefixContext`. In very large repos, it can also be slow to calculate the unique prefixes, especially if it involves a request to a server. This feature becomes much more important in such repos. --- lib/src/id_prefix.rs | 109 ++++++++++++++++++- lib/tests/test_id_prefix.rs | 201 ++++++++++++++++++++++++++++++++++++ 2 files changed, 307 insertions(+), 3 deletions(-) create mode 100644 lib/tests/test_id_prefix.rs diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index f081fe653e..a2c22e8353 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -12,27 +12,114 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::rc::Rc; + +use once_cell::unsync::OnceCell; + use crate::backend::{self, ChangeId, CommitId, ObjectId}; use crate::index::{HexPrefix, PrefixResolution}; +use crate::op_store::WorkspaceId; use crate::repo::Repo; +use crate::revset::{DefaultSymbolResolver, RevsetExpression, RevsetIteratorExt}; + +struct PrefixDisambiguationError; + +struct DisambiguationData { + expression: Rc, + workspace_id: Option, + // TODO: We shouldn't have to duplicate the CommitId as value + indexes: OnceCell, +} + +struct Indexes { + commit_index: IdIndex, + change_index: IdIndex, +} + +impl DisambiguationData { + fn indexes(&self, repo: &dyn Repo) -> Result<&Indexes, PrefixDisambiguationError> { + self.indexes.get_or_try_init(|| { + let symbol_resolver = DefaultSymbolResolver::new(repo, self.workspace_id.as_ref()); + let resolved_expression = self + .expression + .clone() + .resolve_user_expression(repo, &symbol_resolver) + .map_err(|_| PrefixDisambiguationError)?; + let revset = resolved_expression + .evaluate(repo) + .map_err(|_| PrefixDisambiguationError)?; + + // TODO: We should be able to get the change IDs from the revset, without having + // to read the whole commit objects + let mut commit_id_vec = vec![]; + let mut change_id_vec = vec![]; + for commit in revset.iter().commits(repo.store()) { + let commit = commit.map_err(|_| PrefixDisambiguationError)?; + commit_id_vec.push((commit.id().clone(), commit.id().clone())); + change_id_vec.push((commit.change_id().clone(), commit.id().clone())); + } + Ok(Indexes { + commit_index: IdIndex::from_vec(commit_id_vec), + change_index: IdIndex::from_vec(change_id_vec), + }) + }) + } +} pub struct IdPrefixContext<'repo> { repo: &'repo dyn Repo, + disambiguation: Option, } impl IdPrefixContext<'_> { pub fn new(repo: &dyn Repo) -> IdPrefixContext { - IdPrefixContext { repo } + IdPrefixContext { + repo, + disambiguation: None, + } + } + + pub fn disambiguate_within( + mut self, + expression: Rc, + workspace_id: Option, + ) -> Self { + self.disambiguation = Some(DisambiguationData { + workspace_id, + expression, + indexes: OnceCell::new(), + }); + self + } + + fn disambiguation_indexes(&self) -> 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()) } /// Resolve an unambiguous commit ID prefix. pub fn resolve_commit_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + if let Some(indexes) = self.disambiguation_indexes() { + 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) } /// 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() { + // 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) @@ -40,12 +127,23 @@ impl IdPrefixContext<'_> { /// 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() { + 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) } /// 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() { + 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) } } @@ -71,6 +169,10 @@ where prefix: &HexPrefix, mut value_mapper: impl FnMut(&V) -> U, ) -> PrefixResolution> { + if prefix.min_prefix_bytes().is_empty() { + // We consider an empty prefix ambiguous even if the index has a single entry. + return PrefixResolution::AmbiguousMatch; + } let mut range = self.resolve_prefix_range(prefix).peekable(); if let Some((first_key, _)) = range.peek().copied() { let maybe_entries: Option> = range @@ -135,7 +237,8 @@ where backend::common_hex_len(key.as_bytes(), neighbor.as_bytes()) + 1 }) .max() - .unwrap_or(0) + // Even if the key is the only one in the index, we require at least one digit. + .unwrap_or(1) } } @@ -214,7 +317,7 @@ mod tests { let id_index = IdIndex::from_vec(vec![] as Vec<(ChangeId, ())>); assert_eq!( id_index.shortest_unique_prefix_len(&ChangeId::from_hex("00")), - 0 + 1 ); let id_index = IdIndex::from_vec(vec![ diff --git a/lib/tests/test_id_prefix.rs b/lib/tests/test_id_prefix.rs new file mode 100644 index 0000000000..40b9320780 --- /dev/null +++ b/lib/tests/test_id_prefix.rs @@ -0,0 +1,201 @@ +// Copyright 2023 The Jujutsu Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use itertools::Itertools; +use jujutsu_lib::backend::{CommitId, MillisSinceEpoch, ObjectId, Signature, Timestamp}; +use jujutsu_lib::id_prefix::IdPrefixContext; +use jujutsu_lib::index::HexPrefix; +use jujutsu_lib::index::PrefixResolution::{AmbiguousMatch, NoMatch, SingleMatch}; +use jujutsu_lib::repo::Repo; +use jujutsu_lib::revset::RevsetExpression; +use testutils::TestRepo; + +#[test] +fn test_id_prefix() { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(true); + let repo = &test_repo.repo; + let root_commit_id = repo.store().root_commit_id(); + let root_change_id = repo.store().root_change_id(); + + let mut tx = repo.start_transaction(&settings, "test"); + let mut create_commit = |parent_id: &CommitId| { + let signature = Signature { + name: "Some One".to_string(), + email: "some.one@example.com".to_string(), + timestamp: Timestamp { + timestamp: MillisSinceEpoch(0), + tz_offset: 0, + }, + }; + tx.mut_repo() + .new_commit( + &settings, + vec![parent_id.clone()], + repo.store().empty_tree_id().clone(), + ) + .set_author(signature.clone()) + .set_committer(signature) + .write() + .unwrap() + }; + let mut commits = vec![create_commit(root_commit_id)]; + for _ in 0..25 { + commits.push(create_commit(commits.last().unwrap().id())); + } + let repo = tx.commit(); + + // Print the commit IDs and change IDs for reference + let commit_prefixes = commits + .iter() + .enumerate() + .map(|(i, commit)| format!("{} {}", &commit.id().hex()[..3], i)) + .sorted() + .join("\n"); + insta::assert_snapshot!(commit_prefixes, @r###" + 11a 5 + 214 24 + 2a6 2 + 33e 14 + 3be 16 + 3ea 18 + 593 20 + 5d3 1 + 5f6 13 + 676 3 + 7b6 25 + 7da 9 + 81c 10 + 87e 12 + 997 21 + 9f7 22 + a0e 4 + a55 19 + ac4 23 + c18 17 + ce9 0 + d42 6 + d9d 8 + eec 15 + efe 7 + fa3 11 + "###); + let change_prefixes = commits + .iter() + .enumerate() + .map(|(i, commit)| format!("{} {}", &commit.change_id().hex()[..3], i)) + .sorted() + .join("\n"); + insta::assert_snapshot!(change_prefixes, @r###" + 026 9 + 030 13 + 1b5 6 + 26b 3 + 26c 8 + 271 10 + 439 2 + 44a 17 + 4e9 16 + 5b2 23 + 6c2 19 + 781 0 + 79f 14 + 7d5 24 + 86b 20 + 871 7 + 896 5 + 9e4 18 + a2c 1 + a63 22 + b19 11 + b93 4 + bf5 21 + c24 15 + d64 12 + fee 25 + "###); + + let prefix = |x| HexPrefix::new(x).unwrap(); + + // 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); + assert_eq!( + c.resolve_commit_prefix(&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")), + SingleMatch(vec![commits[0].id().clone()]) + ); + assert_eq!(c.resolve_change_prefix(&prefix("70")), NoMatch); + assert_eq!(c.resolve_change_prefix(&prefix("780")), NoMatch); + + // Disambiguate within a revset + // --------------------------------------------------------------------------------------------- + let expression = + 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); + // Shorter prefix within the set can be used + assert_eq!( + c.resolve_commit_prefix(&prefix("2")), + SingleMatch(commits[2].id().clone()) + ); + // Can still resolve commits outside the set + assert_eq!( + c.resolve_commit_prefix(&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")), + SingleMatch(vec![commits[0].id().clone()]) + ); + + // Single commit in revset. Length 0 is unambiguous, but we pretend 1 digit is + // needed. + // --------------------------------------------------------------------------------------------- + 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")), + 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")), + SingleMatch(vec![root_commit_id.clone()]) + ); + + // Disambiguate within revset that fails to evaluate + // --------------------------------------------------------------------------------------------- + // 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); +} From 20ef171d7a983a44fe4fa501a46228b9a1116227 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 5 May 2023 17:12:08 -0700 Subject: [PATCH 13/14] cli: allow resolving shorter ids within a configured revset This adds a config called `revsets.short-prefixes`, which lets the user specify a revset in which to disambiguate otherwise ambiguous change/commit ids. It defaults to the value of `revsets.log`. I made it so you can disable the feature by setting `revsets.short-prefixes = ""`. I don't like that the default value (using `revsets.log`) cannot be configured explicitly by the user. That will be addressed if we decide to merge the `[revsets]` and `[revset-aliases]` sections some day. --- CHANGELOG.md | 4 +++ docs/config.md | 7 +++++ src/cli_util.rs | 13 ++++++++- src/config-schema.json | 5 ++++ tests/test_commit_template.rs | 2 +- tests/test_log_command.rs | 50 ++++++++++++++++++++++++++++++----- tests/test_obslog_command.rs | 2 +- 7 files changed, 73 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63b0d70203..406d6c62a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Description tempfiles created via `jj describe` now have the file extension `.jjdescription` to help external tooling detect a unique filetype. +* The shortest unique change ID prefixes and commit ID prefixes in `jj log` are + now shorter within the default log revset. You can override the default by + setting the `revsets.short-prefixes` config to a different revset. + ### Fixed bugs * Modify/delete conflicts now include context lines diff --git a/docs/config.md b/docs/config.md index e4cfdd7f01..2fcf457b3f 100644 --- a/docs/config.md +++ b/docs/config.md @@ -192,6 +192,13 @@ To customize these separately, use the `format_short_commit_id()` and 'format_short_change_id(id)' = 'format_short_id(id).upper()' ``` +To get shorter prefixes for certain revisions, set `revsets.short-prefixes`: + +```toml +# Prioritize the current branch +revsets.short-prefixes = "(main..@):" +``` + ### Relative timestamps Can be customized by the `format_timestamp()` template alias. diff --git a/src/cli_util.rs b/src/cli_util.rs index b8cca08e10..a629f087b9 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -936,7 +936,17 @@ impl WorkspaceCommandHelper { pub fn id_prefix_context(&self) -> &IdPrefixContext<'_> { self.user_repo.id_prefix_context.get_or_init(|| { - let context: IdPrefixContext<'_> = IdPrefixContext::new(self.user_repo.repo.as_ref()); + let mut context: IdPrefixContext<'_> = IdPrefixContext::new(self.repo().as_ref()); + let revset_string: String = self + .settings + .config() + .get_string("revsets.short-prefixes") + .unwrap_or_else(|_| self.settings.default_revset()); + if !revset_string.is_empty() { + let disambiguation_revset = self.parse_revset(&revset_string).unwrap(); + context = context + .disambiguate_within(disambiguation_revset, Some(self.workspace_id().clone())); + } let context: IdPrefixContext<'static> = unsafe { std::mem::transmute(context) }; context }) @@ -1284,6 +1294,7 @@ impl WorkspaceCommandTransaction<'_> { formatter: &mut dyn Formatter, commit: &Commit, ) -> std::io::Result<()> { + // TODO: Use the disambiguation revset let id_prefix_context = IdPrefixContext::new(self.tx.repo()); let template = parse_commit_summary_template( self.tx.repo(), diff --git a/src/config-schema.json b/src/config-schema.json index 6bcfbdf8b7..f568dd6f9c 100644 --- a/src/config-schema.json +++ b/src/config-schema.json @@ -270,6 +270,11 @@ "type": "string", "description": "Default set of revisions to show when no explicit revset is given for jj log and similar commands", "default": "@ | (remote_branches() | tags()).. | ((remote_branches() | tags())..)-" + }, + "short-prefixes": { + "type": "string", + "description": "Revisions to give shorter change and commit IDs to", + "default": "" } }, "additionalProperties": { diff --git a/tests/test_commit_template.rs b/tests/test_commit_template.rs index 40a54edd64..d9886888ee 100644 --- a/tests/test_commit_template.rs +++ b/tests/test_commit_template.rs @@ -281,7 +281,7 @@ fn test_log_git_head() { insta::assert_snapshot!(stdout, @r###" @ rlvkpnrzqnoo test.user@example.com 2001-02-03 04:05:09.000 +07:00 50aaf4754c1e │ initial - ◉ qpvuntsmwlqt test.user@example.com 2001-02-03 04:05:07.000 +07:00 master HEAD@git 230dd059e1b0 + ◉ qpvuntsmwlqt test.user@example.com 2001-02-03 04:05:07.000 +07:00 master HEAD@git 230dd059e1b0 │ (empty) (no description set) ◉ zzzzzzzzzzzz 1970-01-01 00:00:00.000 +00:00 000000000000 (empty) (no description set) diff --git a/tests/test_log_command.rs b/tests/test_log_command.rs index d1c286e2b8..680673685c 100644 --- a/tests/test_log_command.rs +++ b/tests/test_log_command.rs @@ -315,13 +315,49 @@ fn test_log_shortest_accessors() { zn 38 yo 0cf vr 9e - yq 06f + yq 06 ro 1f mz 7b qpv ba1 zzz 00 "###); + insta::assert_snapshot!( + render(":@", r#"format_id(change_id) ++ " " ++ format_id(commit_id) ++ "\n""#), + @r###" + wq[nwkozpkust] 03[f51310b83e] + km[kuslswpqwq] f7[7fb1909080] + kp[qxywonksrl] e7[15ad5db646] + zn[kkpsqqskkl] 38[622e54e2e5] + yo[stqsxwqrlt] 0cf[42f60199c] + vr[uxwmqvtpmx] 9e[6015e4e622] + yq[osqzytrlsw] 06[f34d9b1475] + ro[yxmykxtrkr] 1f[99a5e19891] + mz[vwutvlkqwt] 7b[1f7dee65b4] + qpv[untsmwlqt] ba1[a30916d29] + zzz[zzzzzzzzz] 00[0000000000] + "###); + + // Can get shorter prefixes in configured revset + test_env.add_config(r#"revsets.short-prefixes = "(@----):""#); + insta::assert_snapshot!( + render(":@", r#"format_id(change_id) ++ " " ++ format_id(commit_id) ++ "\n""#), + @r###" + w[qnwkozpkust] 03[f51310b83e] + km[kuslswpqwq] f[77fb1909080] + kp[qxywonksrl] e[715ad5db646] + z[nkkpsqqskkl] 3[8622e54e2e5] + y[ostqsxwqrlt] 0c[f42f60199c] + vr[uxwmqvtpmx] 9e[6015e4e622] + yq[osqzytrlsw] 06f[34d9b1475] + ro[yxmykxtrkr] 1f[99a5e19891] + mz[vwutvlkqwt] 7b[1f7dee65b4] + qpv[untsmwlqt] ba1[a30916d29] + zzz[zzzzzzzzz] 00[0000000000] + "###); + + // Can disable short prefixes by setting to empty string + test_env.add_config(r#"revsets.short-prefixes = """#); insta::assert_snapshot!( render(":@", r#"format_id(change_id) ++ " " ++ format_id(commit_id) ++ "\n""#), @r###" @@ -409,7 +445,7 @@ fn test_log_prefix_highlight_styled() { ◉ Change znkkpsqqskkl commit6 38622e54e2e5 ◉ Change yostqsxwqrlt commit5 0cf42f60199c ◉ Change vruxwmqvtpmx commit4 9e6015e4e622 - ◉ Change yqosqzytrlsw commit3 06f34d9b1475 + ◉ Change yqosqzytrlsw commit3 06f34d9b1475 ◉ Change royxmykxtrkr commit2 1f99a5e19891 ◉ Change mzvwutvlkqwt commit1 7b1f7dee65b4 ◉ Change qpvuntsmwlqt initial ba1a30916d29 original @@ -435,7 +471,7 @@ fn test_log_prefix_highlight_styled() { ◉ Change znk commit6 386 ◉ Change yos commit5 0cf ◉ Change vru commit4 9e6 - ◉ Change yqo commit3 06f + ◉ Change yqo commit3 06f ◉ Change roy commit2 1f9 ◉ Change mzv commit1 7b1 ◉ Change qpv initial ba1 original @@ -461,7 +497,7 @@ fn test_log_prefix_highlight_styled() { ◉ Change zn commit6 38 ◉ Change yo commit5 0cf ◉ Change vr commit4 9e - ◉ Change yq commit3 06f + ◉ Change yq commit3 06 ◉ Change ro commit2 1f ◉ Change mz commit1 7b ◉ Change qpv initial ba1 original @@ -514,10 +550,10 @@ fn test_log_prefix_highlight_counts_hidden_commits() { insta::assert_snapshot!( test_env.jj_cmd_success(&repo_path, &["log", "-T", prefix_format]), @r###" - @ Change w[qnwkozpkust] 44[4c3c5066d3] - │ ◉ Change q[pvuntsmwlqt] initial ba[1a30916d29] original + @ Change w[qnwkozpkust] 4[44c3c5066d3] + │ ◉ Change q[pvuntsmwlqt] initial b[a1a30916d29] original ├─╯ - ◉ Change z[zzzzzzzzzzz] 00[0000000000] + ◉ Change z[zzzzzzzzzzz] 0[00000000000] "### ); insta::assert_snapshot!( diff --git a/tests/test_obslog_command.rs b/tests/test_obslog_command.rs index 5e29369e5f..d3660436ef 100644 --- a/tests/test_obslog_command.rs +++ b/tests/test_obslog_command.rs @@ -44,7 +44,7 @@ fn test_obslog_with_or_without_diff() { // Color let stdout = test_env.jj_cmd_success(&repo_path, &["--color=always", "obslog"]); insta::assert_snapshot!(stdout, @r###" - @ rlvkpnrzqnoo test.user@example.com 2001-02-03 04:05:10.000 +07:00 66b42ad36073 + @ rlvkpnrzqnoo test.user@example.com 2001-02-03 04:05:10.000 +07:00 66b42ad36073 │ my description ◉ rlvkpnrzqnoo hidden test.user@example.com 2001-02-03 04:05:09.000 +07:00 af536e5af67e conflict │ my description From 6bfee4af3e4e659c67dedaefcd5148a22799b187 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 11 May 2023 22:09:48 -0700 Subject: [PATCH 14/14] 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>> {