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 24, 2024
1 parent 8d166c7 commit 3bd14c8
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 62 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

### Fixed bugs

* Fix confusing/unclear error message, when next/prev can not find a target commit
by explicitly specifying the commit where the search for the target started from.

* Release binaries for Intel Macs have been restored. They were previously
broken due to using a sunset version of GitHub's macOS runners (but nobody had
previously complained.)
Expand Down
154 changes: 114 additions & 40 deletions cli/src/movement_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,52 +59,110 @@ 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 target_not_found_error(
&self,
workspace_command: &WorkspaceCommandHelper,
args: &MovementArgsInternal,
commits: &[Commit],
) -> CommandError {
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.

// edit, conflict
(Direction::Next, true, true) => {
String::from("The working copy has no descendants with conflicts")
}

// edit, no-conflict
(Direction::Next, true, false) => format!(
"No descendant found {} commit(s) forward from the working copy",
args.offset
),
// in non-edit mode, start_revset is the parent
// of WC, so we look for other descendants of
// start_revset.

// no-edit, conflict
(Direction::Next, false, true) => {
String::from("The working copy parent(s) have no other descendants with conflicts")
}

// no-edit, no-conflict
(Direction::Next, false, false) => format!(
"No other descendant found {} commit(s) forward from the working copy parent(s)",
args.offset
),

// The WC can never be an ancestor of the start_revset since
// start_revset is either itself or it's parent.

// edit, conflict
(Direction::Prev, true, true) => {
String::from("The working copy has no ancestors with conflicts")
}

// edit, no-conflict
(Direction::Prev, true, false) => format!(
"No ancestor found {} commit(s) back from the working copy",
args.offset
),

// no-edit, conflict
(Direction::Prev, false, true) => {
String::from("The working copy parent(s) have no ancestors with conflicts")
}

// no-edit, no-conflict
(Direction::Prev, false, false) => format!(
"No ancestor found {} commit(s) back from the working copy parents(s)",
args.offset
),
};

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 {
write!(formatter, "Working copy parent: ")?;
}
template.format(commit, formatter)
})
});

cmd_err
}

fn get_target_revset(
fn build_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 {
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
.children()
.descendants()
.parents()
.ancestors()
.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()
} else {
start_revset.ancestors_at(args.offset)
}
.heads()
}
(Direction::Prev, false) => start_revset.ancestors_at(args.offset),
};

Ok(target_revset)
Expand All @@ -118,7 +176,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()
Expand All @@ -129,7 +197,13 @@ 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
.clone()
.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)?,
};
Expand Down
Loading

0 comments on commit 3bd14c8

Please sign in to comment.