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

next/prev: update error message when no movement targets are found.

Browse source 
If movement commands don't find a target commit, they fail. However,
it's usually not intuitive why they fail because in non-edit mode the
start commit is the parent of the working commit.

Adding the start commit change hash to the error message makes it easier
for the user to figure out what is going on.

Also, specifying 'No **other** descendant...' helps make it clear what
`jj` is really looking for.

Part of #3947
This commit is contained in:
Essien Ita Essien 2024-08-15 17:24:54 +01:00 committed by Essien Ita Essien
parent 8d166c7642
commit f9dc0589c1
2 changed files with 167 additions and 63 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

134
cli/src/movement_util.rs
View file

@ -59,52 +59,89 @@ impl Direction {
}
}
fn target_not_found_message(&self, change_offset: u64) -> String {
match self {
Direction::Next => format!("No descendant found {} commit(s) forward", change_offset),
Direction::Prev => format!("No ancestor found {} commit(s) back", change_offset),
}
}
fn get_target_revset(
fn target_not_found_error(
&self,
working_commit_id: &CommitId,
workspace_command: &WorkspaceCommandHelper,
args: &MovementArgsInternal,
) -> Result<Rc<RevsetExpression>, CommandError> {
let wc_revset = RevsetExpression::commit(working_commit_id.clone());
// If we're editing, start at the working-copy commit. Otherwise, start from
// its direct parent(s).
let start_revset = if args.should_edit {
wc_revset.clone()
} else {
wc_revset.parents()
commits: &[Commit],
) -> CommandError {
let offset = args.offset;
let err_msg = match (self, args.should_edit, args.conflict) {
// in edit mode, start_revset is the WC, so we only look for direct descendants.
(Direction::Next, true, true) => {
String::from("The working copy has no descendants with conflicts")
}
(Direction::Next, true, false) => {
format!("No descendant found {offset} commit(s) forward from the working copy",)
}
// in non-edit mode, start_revset is the parent of WC, so we look for other descendants
// of start_revset.
(Direction::Next, false, true) => {
String::from("The working copy parent(s) have no other descendants with conflicts")
}
(Direction::Next, false, false) => format!(
"No other descendant found {offset} commit(s) forward from the working copy \
parent(s)",
),
// The WC can never be an ancestor of the start_revset since start_revset is either
// itself or it's parent.
(Direction::Prev, true, true) => {
String::from("The working copy has no ancestors with conflicts")
}
(Direction::Prev, true, false) => {
format!("No ancestor found {offset} commit(s) back from the working copy",)
}
(Direction::Prev, false, true) => {
String::from("The working copy parent(s) have no ancestors with conflicts")
}
(Direction::Prev, false, false) => format!(
"No ancestor found {offset} commit(s) back from the working copy parents(s)",
),
};
let target_revset = match self {
Direction::Next => if args.conflict {
start_revset
.children()
.descendants()
.filtered(RevsetFilterPredicate::HasConflict)
.roots()
} else {
start_revset.descendants_at(args.offset)
}
.minus(&wc_revset),
Direction::Prev => {
if args.conflict {
// If people desire to move to the root conflict, replace the `heads()` below
// with `roots(). But let's wait for feedback.
start_revset
.parents()
.ancestors()
.filtered(RevsetFilterPredicate::HasConflict)
.heads()
let template = workspace_command.commit_summary_template();
let mut cmd_err = user_error(err_msg);
commits.iter().for_each(|commit| {
cmd_err.add_formatted_hint_with(|formatter| {
if args.should_edit {
write!(formatter, "Working copy: ")?;
} else {
start_revset.ancestors_at(args.offset)
write!(formatter, "Working copy parent: ")?;
}
template.format(commit, formatter)
})
});
cmd_err
}
fn build_target_revset(
&self,
working_revset: &Rc<RevsetExpression>,
start_revset: &Rc<RevsetExpression>,
args: &MovementArgsInternal,
) -> Result<Rc<RevsetExpression>, CommandError> {
let target_revset = match (self, args.conflict) {
(Direction::Next, true) => start_revset
.children()
.descendants()
.filtered(RevsetFilterPredicate::HasConflict)
.roots()
.minus(working_revset),
(Direction::Next, false) => start_revset
.descendants_at(args.offset)
.minus(working_revset),
(Direction::Prev, true) =>
// If people desire to move to the root conflict, replace the `heads()` below
// with `roots(). But let's wait for feedback.
{
start_revset
.parents()
.ancestors()
.filtered(RevsetFilterPredicate::HasConflict)
.heads()
}
(Direction::Prev, false) => start_revset.ancestors_at(args.offset),
};
Ok(target_revset)
@ -118,7 +155,17 @@ fn get_target_commit(
working_commit_id: &CommitId,
args: &MovementArgsInternal,
) -> Result<Commit, CommandError> {
let target_revset = direction.get_target_revset(working_commit_id, args)?;
let wc_revset = RevsetExpression::commit(working_commit_id.clone());
// If we're editing, start at the working-copy commit. Otherwise, start from
// its direct parent(s).
let start_revset = if args.should_edit {
wc_revset.clone()
} else {
wc_revset.parents()
};
let target_revset = direction.build_target_revset(&wc_revset, &start_revset, args)?;
let targets: Vec<Commit> = target_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
@ -129,7 +176,12 @@ fn get_target_commit(
[target] => target,
[] => {
// We found no ancestor/descendant.
return Err(user_error(direction.target_not_found_message(args.offset)));
let start_commits: Vec<Commit> = start_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
.commits(workspace_command.repo().store())
.try_collect()?;
return Err(direction.target_not_found_error(workspace_command, args, &start_commits));
}
commits => choose_commit(ui, workspace_command, direction, commits)?,
};

96
cli/tests/test_next_prev_commands.rs
View file

@ -203,7 +203,8 @@ fn test_next_exceeding_history() {
let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "3"]);
// `jj next` beyond existing history fails.
insta::assert_snapshot!(stderr, @r###"
Error: No descendant found 3 commit(s) forward
Error: No other descendant found 3 commit(s) forward from the working copy parent(s)
Hint: Working copy parent: qpvuntsm fa15625b (empty) first
"###);
}
@ -544,10 +545,12 @@ fn test_prev_prompts_on_multiple_parents() {
// Create a merge commit, which has two parents.
test_env.jj_cmd_ok(&repo_path, &["new", "all:@--+"]);
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "merge"]);
test_env.jj_cmd_ok(&repo_path, &["commit", "-m", "merge+1"]);
// Check that the graph looks the way we expect.
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ vruxwmqvtpmx
@ yostqsxwqrlt
○ vruxwmqvtpmx merge+1
○ yqosqzytrlsw merge
├─┬─╮
│ │ ○ qpvuntsmwlqt first
@ -559,7 +562,7 @@ fn test_prev_prompts_on_multiple_parents() {
"###);
// Move @ backwards.
let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev"], "3\n");
let (stdout, stderr) = test_env.jj_cmd_stdin_ok(&repo_path, &["prev", "2"], "3\n");
insta::assert_snapshot!(stdout,@r###"
ambiguous prev commit, choose one to target:
1: mzvwutvl bc4f4fe3 (empty) third
@ -569,9 +572,45 @@ fn test_prev_prompts_on_multiple_parents() {
enter the index of the commit you want to target:
"###);
insta::assert_snapshot!(stderr,@r###"
Working copy now at: znkkpsqq 07b409e8 (empty) (no description set)
Working copy now at: kpqxywon ddac00b0 (empty) (no description set)
Parent commit : qpvuntsm fa15625b (empty) first
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ kpqxywonksrl
│ ○ vruxwmqvtpmx merge+1
│ ○ yqosqzytrlsw merge
╭─┼─╮
○ │ │ qpvuntsmwlqt first
│ │ ○ kkmpptxzrspx second
├───╯
│ ○ mzvwutvlkqwt third
├─╯
◆ zzzzzzzzzzzz
"###);
test_env.jj_cmd_ok(&repo_path, &["next"]);
test_env.jj_cmd_ok(&repo_path, &["edit", "@-"]);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
○ vruxwmqvtpmx merge+1
@ yqosqzytrlsw merge
├─┬─╮
│ │ ○ qpvuntsmwlqt first
│ ○ │ kkmpptxzrspx second
│ ├─╯
○ │ mzvwutvlkqwt third
├─╯
◆ zzzzzzzzzzzz
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--no-edit"]);
insta::assert_snapshot!(stderr,@r###"
Error: No other descendant found 1 commit(s) forward from the working copy parent(s)
Hint: Working copy parent: mzvwutvl bc4f4fe3 (empty) third
Hint: Working copy parent: kkmpptxz b0d21db3 (empty) second
Hint: Working copy parent: qpvuntsm fa15625b (empty) first
"###)
}
#[test]
@ -594,7 +633,8 @@ fn test_prev_beyond_root_fails() {
// @- is at "fourth", and there is no parent 5 commits behind it.
let stderr = test_env.jj_cmd_failure(&repo_path, &["prev", "5"]);
insta::assert_snapshot!(stderr,@r###"
Error: No ancestor found 5 commit(s) back
Error: No ancestor found 5 commit(s) back from the working copy parents(s)
Hint: Working copy parent: zsuskuln 9d7e5e99 (empty) fourth
"###);
}
@ -826,14 +866,17 @@ fn test_next_conflict_head() {
@ rlvkpnrzqnoo conflict
◆ zzzzzzzzzzzz
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict"]);
insta::assert_snapshot!(stderr, @r###"
Error: No descendant found 1 commit(s) forward
Error: The working copy parent(s) have no other descendants with conflicts
Hint: Working copy parent: zzzzzzzz 00000000 (empty) (no description set)
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict", "--edit"]);
insta::assert_snapshot!(stderr, @r###"
Error: No descendant found 1 commit(s) forward
Error: The working copy has no descendants with conflicts
Hint: Working copy: rlvkpnrz da992bf2 (conflict) (no description set)
"###);
}
@ -893,6 +936,11 @@ fn test_movement_edit_mode_true() {
◆ zzzzzzzzzzzz
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["prev"]);
insta::assert_snapshot!(stderr, @r###"
Error: The root commit 000000000000 is immutable
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
@ -924,12 +972,12 @@ fn test_movement_edit_mode_true() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--no-edit"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Working copy now at: nkmrtpmo 67a001a6 (empty) (no description set)
Working copy now at: uyznsvlq 7ad57fb8 (empty) (no description set)
Parent commit : qpvuntsm fa15625b (empty) first
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ nkmrtpmomlro
@ uyznsvlquzzm
│ ○ kkmpptxzrspx third
│ ○ rlvkpnrzqnoo second
├─╯
@ -940,18 +988,24 @@ fn test_movement_edit_mode_true() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--no-edit"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Working copy now at: xznxytkn 22287813 (empty) (no description set)
Working copy now at: xtnwkqum 7ac7a1c4 (empty) (no description set)
Parent commit : rlvkpnrz 9ed53a4a (empty) second
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ xznxytknoqwo
@ xtnwkqumpolk
│ ○ kkmpptxzrspx third
├─╯
○ rlvkpnrzqnoo second
○ qpvuntsmwlqt first
◆ zzzzzzzzzzzz
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["next"]);
insta::assert_snapshot!(stderr, @r###"
Error: No descendant found 1 commit(s) forward from the working copy
Hint: Working copy: xtnwkqum 7ac7a1c4 (empty) (no description set)
"###);
}
#[test]
@ -1000,15 +1054,21 @@ fn test_movement_edit_mode_false() {
◆ zzzzzzzzzzzz
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["prev", "3"]);
insta::assert_snapshot!(stderr, @r###"
Error: No ancestor found 3 commit(s) back from the working copy parents(s)
Hint: Working copy parent: qpvuntsm fa15625b (empty) first
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Working copy now at: znkkpsqq a8419fd6 (empty) (no description set)
Working copy now at: kpqxywon d06750fb (empty) (no description set)
Parent commit : rlvkpnrz 9ed53a4a (empty) second
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ znkkpsqqskkl
@ kpqxywonksrl
│ ○ kkmpptxzrspx third
├─╯
○ rlvkpnrzqnoo second
@ -1019,18 +1079,10 @@ fn test_movement_edit_mode_false() {
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next", "--no-edit"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"
Working copy now at: kmkuslsw 8c4d85ef (empty) (no description set)
Working copy now at: wqnwkozp 10fa181f (empty) (no description set)
Parent commit : kkmpptxz 30056b0c (empty) third
"###);
insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
@ kmkuslswpqwq
○ kkmpptxzrspx third
○ rlvkpnrzqnoo second
○ qpvuntsmwlqt first
◆ zzzzzzzzzzzz
"###);
let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev", "--edit", "2"]);
insta::assert_snapshot!(stdout, @"");
insta::assert_snapshot!(stderr, @r###"