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

cli: avoid bad hint "Prefix the expression with 'all'..."

Browse source 
There is no point of showing the hint for non-existent revision.

Also appends ':' to the 'all' to make the hint more precise.

#2451
This commit is contained in:
Aleksey Kuznetsov 2024-03-17 18:44:20 +05:00
parent 00285be7a7
commit d8c84940d9
3 changed files with 79 additions and 42 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

77
cli/src/cli_util.rs
View file

@ -748,6 +748,37 @@ impl WorkspaceCommandHelper {
/// Resolve a revset to a single revision. Return an error if the revset is
/// empty or has multiple revisions.
pub fn resolve_single_rev(&self, revision_str: &str) -> Result<Commit, CommandError> {
self.resolve_single_rev_with_hint_about_all_prefix(revision_str, false)
}
/// Resolve a revset any number of revisions (including 0).
pub fn resolve_revset(&self, revision_str: &str) -> Result<Vec<Commit>, 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()?)
}
/// Resolve a revset any number of revisions (including 0), but require the
/// user to indicate if they allow multiple revisions by prefixing the
/// expression with `all:`.
pub fn resolve_revset_default_single(
&self,
revision_str: &str,
) -> Result<Vec<Commit>, CommandError> {
// TODO: Let pest parse the prefix too once we've dropped support for `:`
if let Some(revision_str) = revision_str.strip_prefix("all:") {
self.resolve_revset(revision_str)
} else {
self.resolve_single_rev_with_hint_about_all_prefix(revision_str, true)
.map(|commit| vec![commit])
}
}
fn resolve_single_rev_with_hint_about_all_prefix(
&self,
revision_str: &str,
should_hint_about_all_prefix: bool,
) -> Result<Commit, CommandError> {
let revset_expression = self.parse_revset(revision_str)?;
let revset = self.evaluate_revset(revset_expression.clone())?;
let mut iter = revset.iter().commits(self.repo().store()).fuse();
@ -783,10 +814,17 @@ It resolved to these revisions:
Set which revision the branch points to with `jj branch set {branch_name} -r <REVISION>`."#,
)
} else {
format!(
let mut hint = format!(
r#"The revset "{revision_str}" resolved to these revisions:
{commits_summary}"#,
)
);
if should_hint_about_all_prefix {
hint.push_str(&format!(
"\nPrefix the expression with 'all:' to allow any number of revisions \
(i.e. 'all:{revision_str}')."
));
}
hint
};
Err(user_error_with_hint(
format!(r#"Revset "{revision_str}" resolved to more than one revision"#),
@ -796,41 +834,6 @@ Set which revision the branch points to with `jj branch set {branch_name} -r <RE
}
}
/// Resolve a revset any number of revisions (including 0).
pub fn resolve_revset(&self, revision_str: &str) -> Result<Vec<Commit>, 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()?)
}
/// Resolve a revset any number of revisions (including 0), but require the
/// user to indicate if they allow multiple revisions by prefixing the
/// expression with `all:`.
pub fn resolve_revset_default_single(
&self,
revision_str: &str,
) -> Result<Vec<Commit>, CommandError> {
// TODO: Let pest parse the prefix too once we've dropped support for `:`
if let Some(revision_str) = revision_str.strip_prefix("all:") {
self.resolve_revset(revision_str)
} else {
self.resolve_single_rev(revision_str)
.map_err(|err| match err {
CommandError::UserError { err, hint } => CommandError::UserError {
err,
hint: Some(format!(
"{old_hint}Prefix the expression with 'all' to allow any number of \
revisions (i.e. 'all:{}').",
revision_str,
old_hint = hint.map(|hint| format!("{hint}\n")).unwrap_or_default()
)),
},
err => err,
})
.map(|commit| vec![commit])
}
}
pub fn parse_revset(
&self,
revision_str: &str,

17
cli/tests/test_new_command.rs
View file

@ -498,7 +498,6 @@ fn test_new_conflicting_branches() {
kkmpptxz 66c6502d foo?? | (empty) two
qpvuntsm a9330854 foo?? | (empty) one
Set which revision the branch points to with `jj branch set foo -r <REVISION>`.
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:foo').
"###);
}
@ -521,7 +520,21 @@ fn test_new_conflicting_change_ids() {
qpvuntsm?? d2ae6806 (empty) two
qpvuntsm?? a9330854 (empty) one
Some of these commits have the same change id. Abandon one of them with `jj abandon -r <REVISION>`.
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:qpvuntsm').
"###);
}
#[test]
fn test_new_error_revision_does_not_exist() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "one"]);
test_env.jj_cmd_ok(&repo_path, &["new", "-m", "two"]);
let stderr = test_env.jj_cmd_failure(&repo_path, &["new", "this"]);
insta::assert_snapshot!(stderr, @r###"
Error: Revision "this" doesn't exist
"###);
}

27
cli/tests/test_rebase_command.rs
View file

@ -169,7 +169,7 @@ fn test_rebase_branch() {
Hint: The revset "e|d" resolved to these revisions:
znkkpsqq e52756c8 e | e
vruxwmqv 514fa6b2 d | d
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:e|d').
Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:e|d').
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-b=all:e|d", "-d=b"]);
insta::assert_snapshot!(stdout, @"");
@ -480,7 +480,7 @@ fn test_rebase_multiple_destinations() {
Hint: The revset "b|c" resolved to these revisions:
royxmykx fe2e8e8b c | c
zsuskuln d370aee1 b | b
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:b|c').
Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:b|c').
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "a", "-d", "all:b|c"]);
insta::assert_snapshot!(stdout, @"");
@ -616,7 +616,7 @@ fn test_rebase_with_descendants() {
Hint: The revset "b|d" resolved to these revisions:
vruxwmqv df54a9fd d | d
zsuskuln d370aee1 b | b
Prefix the expression with 'all' to allow any number of revisions (i.e. 'all:b|d').
Prefix the expression with 'all:' to allow any number of revisions (i.e. 'all:b|d').
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s=all:b|d", "-d=a"]);
insta::assert_snapshot!(stdout, @"");
@ -638,6 +638,27 @@ fn test_rebase_with_descendants() {
"###);
}
#[test]
fn test_rebase_error_revision_does_not_exist() {
let test_env = TestEnvironment::default();
test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
let repo_path = test_env.env_root().join("repo");
test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "one"]);
test_env.jj_cmd_ok(&repo_path, &["branch", "create", "b-one"]);
test_env.jj_cmd_ok(&repo_path, &["new", "-r", "@-", "-m", "two"]);
let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-b", "b-one", "-d", "this"]);
insta::assert_snapshot!(stderr, @r###"
Error: Revision "this" doesn't exist
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-b", "this", "-d", "b-one"]);
insta::assert_snapshot!(stderr, @r###"
Error: Revision "this" doesn't exist
"###);
}
fn get_log_output(test_env: &TestEnvironment, repo_path: &Path) -> String {
test_env.jj_cmd_success(repo_path, &["log", "-T", "branches"])
}