Skip to content

Commit

Permalink
next/prev: update error message when no movement targets are found.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@essiene
essiene committed Aug 20, 2024
1 parent 7173dd9 commit 0dde77f
Show file tree
Hide file tree
Showing 2 changed files with 127 additions and 40 deletions.
80 changes: 62 additions & 18 deletions cli/src/movement_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,14 @@ use std::io::Write;
use std::rc::Rc;

use itertools::Itertools;
use jj_lib::backend::CommitId;
use jj_lib::backend::{ChangeId, CommitId};
use jj_lib::commit::Commit;
use jj_lib::repo::Repo;
use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};

use crate::cli_util::{short_commit_hash, CommandHelper, WorkspaceCommandHelper};
use crate::cli_util::{
short_change_hash, short_commit_hash, CommandHelper, WorkspaceCommandHelper,
};
use crate::command_error::{user_error, CommandError};
use crate::ui::Ui;

Expand Down Expand Up @@ -54,27 +56,47 @@ impl Direction {
}
}

fn target_not_found_message(&self, change_offset: u64) -> String {
fn target_not_found_message(
&self,
edit: bool,
change_offset: u64,
change_ids: &[ChangeId],
) -> String {
let from = change_ids.iter().map(short_change_hash).join(", ");
match self {
Direction::Next => format!("No descendant found {} commit(s) forward", change_offset),
Direction::Prev => format!("No ancestor found {} commit(s) back", change_offset),
Direction::Next => {
if edit {
// in edit mode, start_revset is the WC, so
// we only look for direct descendants.
format!(
"No descendant found {} commit(s) forward from {}",
change_offset, from
)
} else {
// in non-edit mode, start_revset is the parent
// of WC, so we look for other descendants of
// start_revset.
format!(
"No other descendant found {} commit(s) forward from {}",
change_offset, from
)
}
}
// The WC can never be an ancestor of the start_revset since
// start_revset is either itself or it's parent.
Direction::Prev => format!(
"No ancestor found {} commit(s) back from {}",
change_offset, from
),
}
}

fn get_target_revset(
&self,
working_commit_id: &CommitId,
working_revset: &Rc<RevsetExpression>,
start_revset: &Rc<RevsetExpression>,
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()
};

let target_revset = match self {
Direction::Next => if args.conflict {
start_revset
Expand All @@ -85,7 +107,7 @@ impl Direction {
} else {
start_revset.descendants_at(args.offset)
}
.minus(&wc_revset),
.minus(working_revset),

Direction::Prev => {
if args.conflict {
Expand Down Expand Up @@ -113,7 +135,25 @@ 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 start_commits: Vec<ChangeId> = start_revset
.clone()
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
.commits(workspace_command.repo().store())
.map_ok(|c| c.change_id().clone())
.try_collect()?;

let target_revset = direction.get_target_revset(&wc_revset, &start_revset, args)?;

let targets: Vec<Commit> = target_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
Expand All @@ -124,7 +164,11 @@ fn get_target_commit(
[target] => target,
[] => {
// We found no ancestor/descendant.
return Err(user_error(direction.target_not_found_message(args.offset)));
return Err(user_error(direction.target_not_found_message(
args.should_edit,
args.offset,
&start_commits,
)));
}
commits => choose_commit(ui, workspace_command, direction, commits)?,
};
Expand Down
87 changes: 65 additions & 22 deletions cli/tests/test_next_prev_commands.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ 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 qpvuntsmwlqt
"###);
}

Expand Down Expand Up @@ -542,10 +542,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
Expand All @@ -557,7 +559,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
Expand All @@ -567,9 +569,42 @@ 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
"###);

let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]);
let (stdout, stderr) = 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 mzvwutvlkqwt, kkmpptxzrspx, qpvuntsmwlqt
"###)
}

#[test]
Expand All @@ -592,7 +627,7 @@ 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 zsuskulnrvyr
"###);
}

Expand Down Expand Up @@ -854,14 +889,15 @@ 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: No other descendant found 1 commit(s) forward from zzzzzzzzzzzz
"###);

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: No descendant found 1 commit(s) forward from rlvkpnrzqnoo
"###);
}

Expand Down Expand Up @@ -921,6 +957,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###"
Expand Down Expand Up @@ -952,12 +993,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
├─╯
Expand All @@ -968,18 +1009,23 @@ 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 xtnwkqumpolk
"###);
}

#[test]
Expand Down Expand Up @@ -1028,15 +1074,20 @@ 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 qpvuntsmwlqt
"###);

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
Expand All @@ -1047,18 +1098,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###"
Expand Down

0 comments on commit 0dde77f

Please sign in to comment.