From 23a4124d20cdf00b00edb8ba8d203ec48971d7b3 Mon Sep 17 00:00:00 2001 From: Waleed Khan Date: Sun, 4 Jun 2023 23:35:34 -0500 Subject: [PATCH] feat(revset): suggest similar branch names --- lib/src/revset.rs | 40 ++++++++++++++++++------- lib/tests/test_revset.rs | 63 +++++++++++++++++++++++++++++++++------- src/cli_util.rs | 16 +++++++++- 3 files changed, 98 insertions(+), 21 deletions(-) diff --git a/lib/src/revset.rs b/lib/src/revset.rs index c05695030..2b80a5139 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -41,8 +41,11 @@ use crate::store::Store; /// Error occurred during symbol resolution. #[derive(Debug, Error)] pub enum RevsetResolutionError { - #[error("Revision \"{0}\" doesn't exist")] - NoSuchRevision(String), + #[error("Revision \"{name}\" doesn't exist")] + NoSuchRevision { + name: String, + candidates: Vec, + }, #[error("An empty string is not a valid revision")] EmptyString, #[error("Commit ID prefix \"{0}\" is ambiguous")] @@ -1670,10 +1673,13 @@ 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." - ))) + Err(RevsetResolutionError::NoSuchRevision { + name: format!( + "Won't resolve symbol {symbol:?}. When creating revsets programmatically, avoid \ + using RevsetExpression::symbol(); use RevsetExpression::commits() instead." + ), + candidates: Default::default(), + }) } } @@ -1722,7 +1728,10 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { if let Some(workspace_id) = self.workspace_id { workspace_id.clone() } else { - return Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())); + return Err(RevsetResolutionError::NoSuchRevision { + name: symbol.to_owned(), + candidates: Default::default(), + }); } } else { WorkspaceId::new(symbol.strip_suffix('@').unwrap().to_string()) @@ -1730,7 +1739,10 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { 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())) + Err(RevsetResolutionError::NoSuchRevision { + name: symbol.to_owned(), + candidates: Default::default(), + }) } } else if symbol == "root" { Ok(vec![self.repo.store().root_commit_id().clone()]) @@ -1791,7 +1803,13 @@ impl SymbolResolver for DefaultSymbolResolver<'_> { } } - Err(RevsetResolutionError::NoSuchRevision(symbol.to_owned())) + Err(RevsetResolutionError::NoSuchRevision { + name: symbol.to_owned(), + candidates: { + let branch_names = self.repo.view().branches().keys().collect_vec(); + collect_similar(symbol, &branch_names) + }, + }) } } } @@ -1869,7 +1887,9 @@ fn resolve_symbols( RevsetExpression::Present(candidates) => { resolve_symbols(repo, candidates.clone(), symbol_resolver) .or_else(|err| match err { - RevsetResolutionError::NoSuchRevision(_) => Ok(RevsetExpression::none()), + RevsetResolutionError::NoSuchRevision { .. } => { + Ok(RevsetExpression::none()) + } RevsetResolutionError::EmptyString | RevsetResolutionError::AmbiguousCommitIdPrefix(_) | RevsetResolutionError::AmbiguousChangeIdPrefix(_) diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs index 415510294..fb31d3afa 100644 --- a/lib/tests/test_revset.rs +++ b/lib/tests/test_revset.rs @@ -21,7 +21,7 @@ use jujutsu_lib::commit::Commit; use jujutsu_lib::git; use jujutsu_lib::git_backend::GitBackend; use jujutsu_lib::index::{HexPrefix, PrefixResolution}; -use jujutsu_lib::op_store::{RefTarget, WorkspaceId}; +use jujutsu_lib::op_store::{BranchTarget, RefTarget, WorkspaceId}; use jujutsu_lib::repo::Repo; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::revset::{ @@ -174,13 +174,13 @@ fn test_resolve_symbol_commit_id() { ); assert_matches!( resolve_symbol(repo.as_ref(), "040", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "040" + Err(RevsetResolutionError::NoSuchRevision{name, candidates}) if name == "040" && candidates.is_empty() ); // Test non-hex string assert_matches!( resolve_symbol(repo.as_ref(), "foo", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "foo" + Err(RevsetResolutionError::NoSuchRevision{name, candidates}) if name == "foo" && candidates.is_empty() ); // Test present() suppresses only NoSuchRevision error @@ -313,7 +313,7 @@ fn test_resolve_symbol_change_id(readonly: bool) { ); assert_matches!( resolve_symbol(repo, "zvlyw", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "zvlyw" + Err(RevsetResolutionError::NoSuchRevision{name, candidates}) if name == "zvlyw" && candidates.is_empty() ); // Test that commit and changed id don't conflict ("040" and "zvz" are the @@ -334,7 +334,10 @@ fn test_resolve_symbol_change_id(readonly: bool) { // Test non-hex string assert_matches!( resolve_symbol(repo, "foo", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "foo" + Err(RevsetResolutionError::NoSuchRevision{ + name, + candidates + }) if name == "foo" && candidates.is_empty() ); } @@ -357,15 +360,24 @@ fn test_resolve_symbol_checkout(use_git: bool) { // With no workspaces, no variation can be resolved assert_matches!( resolve_symbol(mut_repo, "@", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "@" + Err(RevsetResolutionError::NoSuchRevision{ + name, + candidates, + }) if name == "@" && candidates.is_empty() ); assert_matches!( resolve_symbol(mut_repo, "@", Some(&ws1)), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "@" + Err(RevsetResolutionError::NoSuchRevision{ + name, + candidates, + }) if name == "@" && candidates.is_empty() ); assert_matches!( resolve_symbol(mut_repo, "ws1@", Some(&ws1)), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "ws1@" + Err(RevsetResolutionError::NoSuchRevision{ + name, + candidates, + }) if name == "ws1@" && candidates.is_empty() ); // Add some workspaces @@ -376,7 +388,10 @@ fn test_resolve_symbol_checkout(use_git: bool) { // @ cannot be resolved without a default workspace ID assert_matches!( resolve_symbol(mut_repo, "@", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "@" + Err(RevsetResolutionError::NoSuchRevision{ + name, + candidates, + }) if name == "@" && candidates.is_empty() ); // Can resolve "@" shorthand with a default workspace ID assert_eq!( @@ -432,7 +447,7 @@ fn test_resolve_symbol_git_refs() { // Nonexistent ref assert_matches!( resolve_symbol(mut_repo, "nonexistent", None), - Err(RevsetResolutionError::NoSuchRevision(s)) if s == "nonexistent" + Err(RevsetResolutionError::NoSuchRevision{name, candidates}) if name == "nonexistent" && candidates.is_empty() ); // Full ref @@ -2484,3 +2499,31 @@ fn test_change_id_index() { ); assert_eq!(resolve_prefix("a"), PrefixResolution::AmbiguousMatch); } + +#[test] +fn test_no_such_revision_suggestion() { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(true); + let repo = &test_repo.repo; + + let mut tx = repo.start_transaction(&settings, "test"); + let mut_repo = tx.mut_repo(); + let commit = write_random_commit(mut_repo, &settings); + + for branch_name in ["foo", "bar", "baz"] { + mut_repo.set_branch( + branch_name.to_string(), + BranchTarget { + local_target: Some(RefTarget::Normal(commit.id().clone())), + remote_targets: Default::default(), + }, + ); + } + + assert_matches!(resolve_symbol(mut_repo, "bar", None), Ok(_)); + assert_matches!( + resolve_symbol(mut_repo, "bax", None), + Err(RevsetResolutionError::NoSuchRevision { name, candidates }) + if name == "bax" && candidates == vec!["bar".to_string(), "baz".to_string()] + ); +} diff --git a/src/cli_util.rs b/src/cli_util.rs index ccdfffbb4..41c6f1593 100644 --- a/src/cli_util.rs +++ b/src/cli_util.rs @@ -279,7 +279,21 @@ impl From for CommandError { impl From for CommandError { fn from(err: RevsetResolutionError) -> Self { - user_error(format!("{err}")) + let hint = match &err { + RevsetResolutionError::NoSuchRevision { + name: _, + candidates, + } => format_similarity_hint(candidates), + RevsetResolutionError::EmptyString + | RevsetResolutionError::AmbiguousCommitIdPrefix(_) + | RevsetResolutionError::AmbiguousChangeIdPrefix(_) + | RevsetResolutionError::StoreError(_) => None, + }; + + CommandError::UserError { + message: format!("{err}"), + hint, + } } }