Skip to content

Commit

Permalink
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|ancestor...' helps make it clear what
`jj` is really looking for.

Part of #3947
  • Loading branch information
essiene committed Aug 15, 2024
1 parent 1a73a47 commit 079483a
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 20 deletions.
43 changes: 27 additions & 16 deletions cli/src/movement_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ use std::io::Write;
use std::rc::Rc;

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

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

Expand Down Expand Up @@ -53,20 +53,11 @@ impl Direction {

fn get_target_revset(
&self,
working_commit_id: &CommitId,
edit: bool,
working_revset: &Rc<RevsetExpression>,
start_revset: &Rc<RevsetExpression>,
has_conflict: bool,
change_offset: u64,
) -> 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 edit {
wc_revset.clone()
} else {
wc_revset.parents()
};

let target_revset = match self {
Direction::Next => if has_conflict {
start_revset
Expand All @@ -77,7 +68,7 @@ impl Direction {
} else {
start_revset.descendants_at(change_offset)
}
.minus(&wc_revset),
.minus(working_revset),

Direction::Prev => {
if has_conflict {
Expand Down Expand Up @@ -115,8 +106,27 @@ pub fn get_target_commit(
has_conflict: bool,
change_offset: u64,
) -> Result<Commit, 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 edit {
wc_revset.clone()
} else {
wc_revset.parents()
};

let start_commit: Commit = start_revset
.clone()
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
.commits(workspace_command.repo().store())
.collect::<Result<Vec<_>, BackendError>>()?
.pop()
.unwrap();

let target_revset =
direction.get_target_revset(working_commit_id, edit, has_conflict, change_offset)?;
direction.get_target_revset(&wc_revset, &start_revset, has_conflict, change_offset)?;

let targets: Vec<Commit> = target_revset
.evaluate_programmatic(workspace_command.repo().as_ref())?
.iter()
Expand All @@ -128,11 +138,12 @@ pub fn get_target_commit(
[] => {
// We found no descendant.
return Err(user_error(format!(
"No {} found {} commit{} {}",
"No other {} found {} commit{} {} from {}",
direction.next_node_type(),
change_offset,
if change_offset > 1 { "s" } else { "" },
direction.direction(),
short_change_hash(start_commit.change_id()),
)));
}
commits => choose_commit(ui, workspace_command, &direction, commits)?,
Expand Down
8 changes: 4 additions & 4 deletions cli/tests/test_next_prev_commands.rs
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 commits forward
Error: No descendant found 3 commits forward from rlvkpnrzqnoo
"###);
}

Expand Down Expand Up @@ -594,7 +594,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 commits back
Error: No ancestor found 5 commits back from zsuskulnrvyr
"###);
}

Expand Down Expand Up @@ -860,12 +860,12 @@ fn test_next_conflict_head() {
"###);
let stderr = test_env.jj_cmd_failure(&repo_path, &["next", "--conflict"]);
insta::assert_snapshot!(stderr, @r###"
Error: No descendant found 1 commit forward
Error: No descendant found 1 commit 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 forward
Error: No descendant found 1 commit forward from rlvkpnrzqnoo
"###);
}

Expand Down

0 comments on commit 079483a

Please sign in to comment.