ok/jj
1
0
Fork 0
forked from mirrors/jj
Code Pull requests Activity

id_prefix: propagate error from disambiguation index

Browse source 
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".
This commit is contained in:
Yuya Nishihara 2024-09-30 17:57:13 +09:00
parent 3ff1f985f3
commit 09d91efea5
8 changed files with 98 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
CHANGELOG.md
View file

@ -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

16
cli/src/commit_templater.rs
View file

@ -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))

2
cli/tests/test_immutable_commits.rs
View file

@ -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()`

31
cli/tests/test_log_command.rs
View file

@ -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 <identifier> or <expression>
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]

8
cli/tests/test_operations.rs
View file

@ -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#"

32
lib/src/id_prefix.rs
View file

@ -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<RevsetExpression>,
@ -52,17 +61,17 @@ impl DisambiguationData {
&self,
repo: &dyn Repo,
extensions: &[impl AsRef<dyn SymbolResolverExtension>],
) -> 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<IdPrefixIndex<'_>, IdPrefixIndexLoadError> {
let indexes = if let Some(disambiguation) = &self.disambiguation {
Some(disambiguation.indexes(repo, self.extensions.symbol_resolvers())?)
} else {
None
};
Ok(IdPrefixIndex { indexes })
}
}

18
lib/src/revset.rs
View file

@ -1634,9 +1634,12 @@ impl PartialSymbolResolver for CommitPrefixResolver<'_> {
symbol: &str,
) -> Result<Option<Vec<CommitId>>, 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<Option<Vec<CommitId>>, 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()),

25
lib/tests/test_id_prefix.rs
View file

@ -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