From 09d91efea5c0b659e53efdb4512da09101b2648f Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Mon, 30 Sep 2024 17:57:13 +0900 Subject: [PATCH] id_prefix: propagate error from disambiguation index The id.shortest() template prints a warning and falls back to repo-global resolution. This seems better than erroring out. There are a few edge cases in which the short-prefixes resolution can fail unexpectedly. For example, the trunk() revision might not exist in operations before "jj git clone". --- CHANGELOG.md | 2 ++ cli/src/commit_templater.rs | 16 ++++++++++++++- cli/tests/test_immutable_commits.rs | 2 ++ cli/tests/test_log_command.rs | 31 ++++++++++++++++++++++++++++ cli/tests/test_operations.rs | 8 ++++++++ lib/src/id_prefix.rs | 32 ++++++++++++++++++----------- lib/src/revset.rs | 18 ++++++++++------ lib/tests/test_id_prefix.rs | 25 ++++++++-------------- 8 files changed, 98 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f018d29cf..3b6abbd72 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). * Revset function names can no longer start with a number. +* Evaluation error of `revsets.short-prefixes` configuration is now reported. + ### Deprecations ### New features diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index b4680e6e8..54cab6365 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -1343,7 +1343,21 @@ fn builtin_commit_or_change_id_methods<'repo>( }) .transpose()?; let repo = language.repo; - let index = language.id_prefix_context.populate(repo); + let index = match language.id_prefix_context.populate(repo) { + Ok(index) => index, + Err(err) => { + // Not an error because we can still produce somewhat + // reasonable output. + diagnostics.add_warning( + TemplateParseError::expression( + "Failed to load short-prefixes index", + function.name_span, + ) + .with_source(err), + ); + IdPrefixIndex::empty() + } + }; let out_property = (self_property, len_property) .map(move |(id, len)| id.shortest(repo, &index, len.unwrap_or(0))); Ok(L::wrap_shortest_id_prefix(out_property)) diff --git a/cli/tests/test_immutable_commits.rs b/cli/tests/test_immutable_commits.rs index c5d9d3f30..133153aec 100644 --- a/cli/tests/test_immutable_commits.rs +++ b/cli/tests/test_immutable_commits.rs @@ -60,6 +60,8 @@ fn test_rewrite_immutable_generic() { // Error mutating the repo if immutable_heads() uses a ref that can't be // resolved test_env.add_config(r#"revset-aliases."immutable_heads()" = "bookmark_that_does_not_exist""#); + // Suppress warning in the commit summary template + test_env.add_config("template-aliases.'format_short_id(id)' = 'id.short(8)'"); let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "main"]); insta::assert_snapshot!(stderr, @r###" Config error: Invalid `revset-aliases.immutable_heads()` diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index dd17b13f8..f8f7d608d 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -444,6 +444,10 @@ fn test_log_bad_short_prefixes() { let test_env = TestEnvironment::default(); test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]); let repo_path = test_env.env_root().join("repo"); + + // Suppress warning in the commit summary template + test_env.add_config("template-aliases.'format_short_id(id)' = 'id.short(8)'"); + // Error on bad config of short prefixes test_env.add_config(r#"revsets.short-prefixes = "!nval!d""#); let stderr = test_env.jj_cmd_failure(&repo_path, &["status"]); @@ -458,6 +462,33 @@ fn test_log_bad_short_prefixes() { = expected or For help, see https://martinvonz.github.io/jj/latest/config/. "###); + + // Warn on resolution of short prefixes + test_env.add_config("revsets.short-prefixes = 'missing'"); + let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["log", "-Tcommit_id.shortest()"]); + insta::assert_snapshot!(stdout, @r#" + @ 2 + ◆ 0 + "#); + insta::assert_snapshot!(stderr, @r#" + Warning: In template expression + --> 1:11 + | + 1 | commit_id.shortest() + | ^------^ + | + = Failed to load short-prefixes index + Failed to resolve short-prefixes disambiguation revset + Revision "missing" doesn't exist + "#); + + // Error on resolution of short prefixes + test_env.add_config("revsets.short-prefixes = 'missing'"); + let stderr = test_env.jj_cmd_failure(&repo_path, &["log", "-r0"]); + insta::assert_snapshot!(stderr, @r#" + Error: Failed to resolve short-prefixes disambiguation revset + Caused by: Revision "missing" doesn't exist + "#); } #[test] diff --git a/cli/tests/test_operations.rs b/cli/tests/test_operations.rs index 793f6fb0b..cdfd0019d 100644 --- a/cli/tests/test_operations.rs +++ b/cli/tests/test_operations.rs @@ -789,6 +789,10 @@ fn test_op_diff() { test_env.jj_cmd_ok(test_env.env_root(), &["git", "clone", "git-repo", "repo"]); let repo_path = test_env.env_root().join("repo"); + // Suppress warning in the commit summary template. The configured "trunk()" + // can't be found in earlier operations. + test_env.add_config("template-aliases.'format_short_id(id)' = 'id.short(8)'"); + // Overview of op log. let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(&stdout, @r#" @@ -1582,6 +1586,10 @@ fn test_op_show() { test_env.jj_cmd_ok(test_env.env_root(), &["git", "clone", "git-repo", "repo"]); let repo_path = test_env.env_root().join("repo"); + // Suppress warning in the commit summary template. The configured "trunk()" + // can't be found in earlier operations. + test_env.add_config("template-aliases.'format_short_id(id)' = 'id.short(8)'"); + // Overview of op log. let stdout = test_env.jj_cmd_success(&repo_path, &["op", "log"]); insta::assert_snapshot!(&stdout, @r#" diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index 83a3c1e01..8a0458eb2 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -21,6 +21,7 @@ use std::sync::Arc; use itertools::Itertools as _; use once_cell::unsync::OnceCell; +use thiserror::Error; use crate::backend::ChangeId; use crate::backend::CommitId; @@ -30,11 +31,19 @@ use crate::object_id::ObjectId; use crate::object_id::PrefixResolution; use crate::repo::Repo; use crate::revset::DefaultSymbolResolver; +use crate::revset::RevsetEvaluationError; use crate::revset::RevsetExpression; use crate::revset::RevsetExtensions; +use crate::revset::RevsetResolutionError; use crate::revset::SymbolResolverExtension; -struct PrefixDisambiguationError; +#[derive(Debug, Error)] +pub enum IdPrefixIndexLoadError { + #[error("Failed to resolve short-prefixes disambiguation revset")] + Resolution(#[source] RevsetResolutionError), + #[error("Failed to evaluate short-prefixes disambiguation revset")] + Evaluation(#[source] RevsetEvaluationError), +} struct DisambiguationData { expression: Rc, @@ -52,17 +61,17 @@ impl DisambiguationData { &self, repo: &dyn Repo, extensions: &[impl AsRef], - ) -> Result<&Indexes, PrefixDisambiguationError> { + ) -> Result<&Indexes, IdPrefixIndexLoadError> { self.indexes.get_or_try_init(|| { let symbol_resolver = DefaultSymbolResolver::new(repo, extensions); let resolved_expression = self .expression .clone() .resolve_user_expression(repo, &symbol_resolver) - .map_err(|_| PrefixDisambiguationError)?; + .map_err(IdPrefixIndexLoadError::Resolution)?; let revset = resolved_expression .evaluate(repo) - .map_err(|_| PrefixDisambiguationError)?; + .map_err(IdPrefixIndexLoadError::Evaluation)?; let commit_change_ids = revset.commit_change_ids().collect_vec(); let mut commit_index = IdIndex::with_capacity(commit_change_ids.len()); @@ -128,14 +137,13 @@ impl IdPrefixContext { /// Loads disambiguation index once, returns a borrowed index to /// disambiguate commit/change IDs. - pub fn populate(&self, repo: &dyn Repo) -> IdPrefixIndex<'_> { - // TODO: propagate errors instead of treating them as if no revset was specified - let indexes = self.disambiguation.as_ref().and_then(|disambiguation| { - disambiguation - .indexes(repo, self.extensions.symbol_resolvers()) - .ok() - }); - IdPrefixIndex { indexes } + pub fn populate(&self, repo: &dyn Repo) -> Result, IdPrefixIndexLoadError> { + let indexes = if let Some(disambiguation) = &self.disambiguation { + Some(disambiguation.indexes(repo, self.extensions.symbol_resolvers())?) + } else { + None + }; + Ok(IdPrefixIndex { indexes }) } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index e4a867e2e..e775abf19 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -1634,9 +1634,12 @@ impl PartialSymbolResolver for CommitPrefixResolver<'_> { symbol: &str, ) -> Result>, RevsetResolutionError> { if let Some(prefix) = HexPrefix::new(symbol) { - let index = self.context.map_or(IdPrefixIndex::empty(), |ctx| { - ctx.populate(self.context_repo) - }); + let index = self + .context + .map(|ctx| ctx.populate(self.context_repo)) + .transpose() + .map_err(|err| RevsetResolutionError::Other(err.into()))? + .unwrap_or(IdPrefixIndex::empty()); match index.resolve_commit_prefix(repo, &prefix) { PrefixResolution::AmbiguousMatch => Err( RevsetResolutionError::AmbiguousCommitIdPrefix(symbol.to_owned()), @@ -1662,9 +1665,12 @@ impl PartialSymbolResolver for ChangePrefixResolver<'_> { symbol: &str, ) -> Result>, RevsetResolutionError> { if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) { - let index = self.context.map_or(IdPrefixIndex::empty(), |ctx| { - ctx.populate(self.context_repo) - }); + let index = self + .context + .map(|ctx| ctx.populate(self.context_repo)) + .transpose() + .map_err(|err| RevsetResolutionError::Other(err.into()))? + .unwrap_or(IdPrefixIndex::empty()); match index.resolve_change_prefix(repo, &prefix) { PrefixResolution::AmbiguousMatch => Err( RevsetResolutionError::AmbiguousChangeIdPrefix(symbol.to_owned()), diff --git a/lib/tests/test_id_prefix.rs b/lib/tests/test_id_prefix.rs index 4bd6a61d7..aa19f5214 100644 --- a/lib/tests/test_id_prefix.rs +++ b/lib/tests/test_id_prefix.rs @@ -139,7 +139,7 @@ fn test_id_prefix() { // Without a disambiguation revset // --------------------------------------------------------------------------------------------- let context = IdPrefixContext::default(); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); assert_eq!( index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), 2 @@ -194,7 +194,7 @@ fn test_id_prefix() { let expression = RevsetExpression::commits(vec![commits[0].id().clone(), commits[2].id().clone()]); let context = context.disambiguate_within(expression); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); // The prefix is now shorter assert_eq!( index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), @@ -224,7 +224,7 @@ fn test_id_prefix() { // --------------------------------------------------------------------------------------------- let expression = RevsetExpression::commit(root_commit_id.clone()); let context = context.disambiguate_within(expression); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); assert_eq!( index.shortest_commit_prefix_len(repo.as_ref(), root_commit_id), 1 @@ -252,18 +252,9 @@ fn test_id_prefix() { // Disambiguate within revset that fails to evaluate // --------------------------------------------------------------------------------------------- - // TODO: Should be an error let expression = RevsetExpression::symbol("nonexistent".to_string()); let context = context.disambiguate_within(expression); - let index = context.populate(repo.as_ref()); - assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), commits[2].id()), - 2 - ); - assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("2")), - AmbiguousMatch - ); + assert!(context.populate(repo.as_ref()).is_err()); } #[test] @@ -340,7 +331,7 @@ fn test_id_prefix_divergent() { // Without a disambiguation revset // -------------------------------- let context = IdPrefixContext::default(); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); assert_eq!( index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), 3 @@ -373,7 +364,7 @@ fn test_id_prefix_divergent() { // ---------------------------------------------------------------------- let expression = RevsetExpression::commits(vec![second_commit.id().clone()]); let context = context.disambiguate_within(expression); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); // The prefix is now shorter assert_eq!( index.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()), @@ -486,7 +477,7 @@ fn test_id_prefix_hidden() { // Without a disambiguation revset // -------------------------------- let context = IdPrefixContext::default(); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); assert_eq!( index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), 2 @@ -522,7 +513,7 @@ fn test_id_prefix_hidden() { // -------------------------- let expression = RevsetExpression::commit(hidden_commit.id().clone()); let context = context.disambiguate_within(expression); - let index = context.populate(repo.as_ref()); + let index = context.populate(repo.as_ref()).unwrap(); assert_eq!( index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), 1