revset: use different errors for ambiguous commit/change IDs

I made a typo and got something like this:

```
Error: Commit or change id prefix "wl" is ambiguous
```

Since we can tell commit ids from change ids these days, let's make
the error message say which kind of id it is. Changing that also kind
of forced me to make a special error for empty strings. Otherwise we
would have to arbitrarily say that an empty string is a commit id or
change id. A specific error message for empty strings seems helpful,
so that's probably for the better anyway.
This commit is contained in:
Martin von Zweigbergk 2023-05-30 21:07:48 -07:00 committed by Martin von Zweigbergk
parent 97b81a0f12
commit fb17e6a50e
3 changed files with 32 additions and 23 deletions

View file

@ -43,8 +43,12 @@ use crate::store::Store;
pub enum RevsetResolutionError {
#[error("Revision \"{0}\" doesn't exist")]
NoSuchRevision(String),
#[error("Commit or change id prefix \"{0}\" is ambiguous")]
AmbiguousIdPrefix(String),
#[error("An empty string is not a valid revision")]
EmptyString,
#[error("Commit ID prefix \"{0}\" is ambiguous")]
AmbiguousCommitIdPrefix(String),
#[error("Change ID prefix \"{0}\" is ambiguous")]
AmbiguousChangeIdPrefix(String),
#[error("Unexpected error from store: {0}")]
StoreError(#[source] BackendError),
}
@ -1719,6 +1723,8 @@ impl SymbolResolver for DefaultSymbolResolver<'_> {
}
} else if symbol == "root" {
Ok(vec![self.repo.store().root_commit_id().clone()])
} else if symbol.is_empty() {
Err(RevsetResolutionError::EmptyString)
} else {
// Try to resolve as a tag
if let Some(target) = self.repo.view().tags().get(symbol) {
@ -1744,7 +1750,9 @@ impl SymbolResolver for DefaultSymbolResolver<'_> {
if let Some(prefix) = HexPrefix::new(symbol) {
match (self.commit_id_resolver)(self.repo, &prefix) {
PrefixResolution::AmbiguousMatch => {
return Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned()));
return Err(RevsetResolutionError::AmbiguousCommitIdPrefix(
symbol.to_owned(),
));
}
PrefixResolution::SingleMatch(id) => {
return Ok(vec![id]);
@ -1759,7 +1767,9 @@ impl SymbolResolver for DefaultSymbolResolver<'_> {
if let Some(prefix) = to_forward_hex(symbol).as_deref().and_then(HexPrefix::new) {
match (self.change_id_resolver)(self.repo, &prefix) {
PrefixResolution::AmbiguousMatch => {
return Err(RevsetResolutionError::AmbiguousIdPrefix(symbol.to_owned()));
return Err(RevsetResolutionError::AmbiguousChangeIdPrefix(
symbol.to_owned(),
));
}
PrefixResolution::SingleMatch(ids) => {
return Ok(ids);
@ -1849,7 +1859,9 @@ fn resolve_symbols(
resolve_symbols(repo, candidates.clone(), symbol_resolver)
.or_else(|err| match err {
RevsetResolutionError::NoSuchRevision(_) => Ok(RevsetExpression::none()),
RevsetResolutionError::AmbiguousIdPrefix(_)
RevsetResolutionError::EmptyString
| RevsetResolutionError::AmbiguousCommitIdPrefix(_)
| RevsetResolutionError::AmbiguousChangeIdPrefix(_)
| RevsetResolutionError::StoreError(_) => Err(err),
})
.map(Some) // Always rewrite subtree

View file

@ -69,6 +69,17 @@ fn test_resolve_symbol_root(use_git: bool) {
);
}
#[test]
fn test_resolve_symbol_empty_string() {
let test_repo = TestRepo::init(true);
let repo = &test_repo.repo;
assert_matches!(
resolve_symbol(repo.as_ref(), "", None),
Err(RevsetResolutionError::EmptyString)
);
}
#[test]
fn test_resolve_symbol_commit_id() {
let settings = testutils::user_settings();
@ -152,12 +163,6 @@ fn test_resolve_symbol_commit_id() {
vec![commits[2].id().clone()]
);
// Test empty commit id
assert_matches!(
resolve_symbol(repo.as_ref(), "", None),
Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s.is_empty()
);
// Test commit id prefix
assert_eq!(
resolve_symbol(repo.as_ref(), "046", None).unwrap(),
@ -165,11 +170,7 @@ fn test_resolve_symbol_commit_id() {
);
assert_matches!(
resolve_symbol(repo.as_ref(), "04", None),
Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "04"
);
assert_matches!(
resolve_symbol(repo.as_ref(), "", None),
Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s.is_empty()
Err(RevsetResolutionError::AmbiguousCommitIdPrefix(s)) if s == "04"
);
assert_matches!(
resolve_symbol(repo.as_ref(), "040", None),
@ -187,7 +188,7 @@ fn test_resolve_symbol_commit_id() {
let symbol_resolver = DefaultSymbolResolver::new(repo.as_ref(), None);
assert_matches!(
optimize(parse("present(04)", &RevsetAliasesMap::new(), None).unwrap()).resolve_user_expression(repo.as_ref(), &symbol_resolver),
Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "04"
Err(RevsetResolutionError::AmbiguousCommitIdPrefix(s)) if s == "04"
);
assert_eq!(
resolve_commit_ids(repo.as_ref(), "present(046)"),
@ -308,11 +309,7 @@ fn test_resolve_symbol_change_id(readonly: bool) {
);
assert_matches!(
resolve_symbol(repo, "zvly", None),
Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s == "zvly"
);
assert_matches!(
resolve_symbol(repo, "", None),
Err(RevsetResolutionError::AmbiguousIdPrefix(s)) if s.is_empty()
Err(RevsetResolutionError::AmbiguousChangeIdPrefix(s)) if s == "zvly"
);
assert_matches!(
resolve_symbol(repo, "zvlyw", None),

View file

@ -561,7 +561,7 @@ fn test_log_prefix_highlight_counts_hidden_commits() {
insta::assert_snapshot!(
test_env.jj_cmd_failure(&repo_path, &["log", "-r", "4", "-T", prefix_format]),
@r###"
Error: Commit or change id prefix "4" is ambiguous
Error: Commit ID prefix "4" is ambiguous
"###
);
insta::assert_snapshot!(