diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index 9846ec83d0..0a48be34e8 100644 --- a/cli/src/commands/next.rs +++ b/cli/src/commands/next.rs @@ -14,7 +14,7 @@ use crate::cli_util::CommandHelper; use crate::command_error::CommandError; -use crate::movement_util::{move_to_commit, Direction}; +use crate::movement_util::{move_to_commit, Direction, MovementArgs}; use crate::ui::Ui; /// Move the working-copy commit to the child revision @@ -52,7 +52,7 @@ use crate::ui::Ui; pub(crate) struct NextArgs { /// How many revisions to move forward. Advances to the next child by /// default. - #[arg(default_value = "1")] + #[arg(default_value_t = 1)] offset: u64, /// Instead of creating a new working-copy commit on top of the target /// commit (like `jj new`), edit the target commit directly (like `jj @@ -64,6 +64,16 @@ pub(crate) struct NextArgs { conflict: bool, } +impl From<&NextArgs> for MovementArgs { + fn from(val: &NextArgs) -> Self { + MovementArgs { + offset: val.offset, + edit: val.edit, + conflict: val.conflict, + } + } +} + pub(crate) fn cmd_next( ui: &mut Ui, command: &CommandHelper, @@ -74,8 +84,6 @@ pub(crate) fn cmd_next( ui, &mut workspace_command, &Direction::Next, - args.edit, - args.conflict, - args.offset, + &mut MovementArgs::from(args), ) } diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index 34f0a046d7..fa4088732f 100644 --- a/cli/src/commands/prev.rs +++ b/cli/src/commands/prev.rs @@ -14,7 +14,7 @@ use crate::cli_util::CommandHelper; use crate::command_error::CommandError; -use crate::movement_util::{move_to_commit, Direction}; +use crate::movement_util::{move_to_commit, Direction, MovementArgs}; use crate::ui::Ui; /// Change the working copy revision relative to the parent revision /// @@ -50,7 +50,7 @@ use crate::ui::Ui; #[command(verbatim_doc_comment)] pub(crate) struct PrevArgs { /// How many revisions to move backward. Moves to the parent by default. - #[arg(default_value = "1")] + #[arg(default_value_t = 1)] offset: u64, /// Edit the parent directly, instead of moving the working-copy commit. #[arg(long, short)] @@ -60,6 +60,16 @@ pub(crate) struct PrevArgs { conflict: bool, } +impl From<&PrevArgs> for MovementArgs { + fn from(val: &PrevArgs) -> Self { + MovementArgs { + offset: val.offset, + edit: val.edit, + conflict: val.conflict, + } + } +} + pub(crate) fn cmd_prev( ui: &mut Ui, command: &CommandHelper, @@ -70,8 +80,6 @@ pub(crate) fn cmd_prev( ui, &mut workspace_command, &Direction::Prev, - args.edit, - args.conflict, - args.offset, + &mut MovementArgs::from(args), ) } diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs index 0c88b3ea71..5be3c825bb 100644 --- a/cli/src/movement_util.rs +++ b/cli/src/movement_util.rs @@ -25,16 +25,24 @@ use crate::cli_util::{short_commit_hash, WorkspaceCommandHelper}; use crate::command_error::{user_error, CommandError}; use crate::ui::Ui; -pub enum Direction { +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) struct MovementArgs { + pub(crate) offset: u64, + pub(crate) edit: bool, + pub(crate) conflict: bool, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub(crate) enum Direction { Next, Prev, } impl Direction { - fn cmd(&self) -> String { + fn cmd(&self) -> &'static str { match self { - Direction::Next => "next".to_string(), - Direction::Prev => "prev".to_string(), + Direction::Next => "next", + Direction::Prev => "prev", } } @@ -48,33 +56,31 @@ impl Direction { fn get_target_revset( &self, working_commit_id: &CommitId, - edit: bool, - has_conflict: bool, - change_offset: u64, + args: &MovementArgs, ) -> Result, 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 { + let start_revset = if args.edit { wc_revset.clone() } else { wc_revset.parents() }; let target_revset = match self { - Direction::Next => if has_conflict { + Direction::Next => if args.conflict { start_revset .children() .descendants() .filtered(RevsetFilterPredicate::HasConflict) .roots() } else { - start_revset.descendants_at(change_offset) + start_revset.descendants_at(args.offset) } .minus(&wc_revset), Direction::Prev => { - if has_conflict { + 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 @@ -83,7 +89,7 @@ impl Direction { .filtered(RevsetFilterPredicate::HasConflict) .heads() } else { - start_revset.ancestors_at(change_offset) + start_revset.ancestors_at(args.offset) } } }; @@ -97,12 +103,9 @@ fn get_target_commit( workspace_command: &WorkspaceCommandHelper, direction: &Direction, working_commit_id: &CommitId, - edit: bool, - has_conflict: bool, - change_offset: u64, + args: &MovementArgs, ) -> Result { - let target_revset = - direction.get_target_revset(working_commit_id, edit, has_conflict, change_offset)?; + let target_revset = direction.get_target_revset(working_commit_id, args)?; let targets: Vec = target_revset .evaluate_programmatic(workspace_command.repo().as_ref())? .iter() @@ -113,9 +116,7 @@ fn get_target_commit( [target] => target, [] => { // We found no ancestor/descendant. - return Err(user_error( - direction.target_not_found_message(change_offset), - )); + return Err(user_error(direction.target_not_found_message(args.offset))); } commits => choose_commit(ui, workspace_command, direction, commits)?, }; @@ -159,39 +160,29 @@ fn choose_commit<'a>( Ok(&commits[choice.parse::().unwrap() - 1]) } -pub fn move_to_commit( +pub(crate) fn move_to_commit( ui: &mut Ui, workspace_command: &mut WorkspaceCommandHelper, direction: &Direction, - edit: bool, - has_conflict: bool, - change_offset: u64, + args: &mut MovementArgs, ) -> Result<(), CommandError> { let current_wc_id = workspace_command .get_wc_commit_id() .ok_or_else(|| user_error("This command requires a working copy"))?; - let edit = edit + args.edit = args.edit || !&workspace_command .repo() .view() .heads() .contains(current_wc_id); - let target = get_target_commit( - ui, - workspace_command, - direction, - current_wc_id, - edit, - has_conflict, - change_offset, - )?; + let target = get_target_commit(ui, workspace_command, direction, current_wc_id, args)?; let current_short = short_commit_hash(current_wc_id); let target_short = short_commit_hash(target.id()); let cmd = direction.cmd(); // We're editing, just move to the target commit. - if edit { + if args.edit { // We're editing, the target must be rewritable. workspace_command.check_rewritable([target.id()])?; let mut tx = workspace_command.start_transaction(); diff --git a/cli/tests/test_next_prev_commands.rs b/cli/tests/test_next_prev_commands.rs index bea7c3911c..66fa0c930f 100644 --- a/cli/tests/test_next_prev_commands.rs +++ b/cli/tests/test_next_prev_commands.rs @@ -231,14 +231,14 @@ fn test_next_parent_has_multiple_descendants() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["next"]); insta::assert_snapshot!(stdout,@r###""###); insta::assert_snapshot!(stderr,@r###" - Working copy now at: mzvwutvl 1b8531ce (empty) 4 - Parent commit : zsuskuln b1394455 (empty) 3 + Working copy now at: qpvuntsm 8b64ddff (empty) 1 + Parent commit : zzzzzzzz 00000000 (empty) (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" - @ mzvwutvlkqwt 4 + ○ mzvwutvlkqwt 4 ○ zsuskulnrvyr 3 │ ○ kkmpptxzrspx 2 - │ ○ qpvuntsmwlqt 1 + │ @ qpvuntsmwlqt 1 ├─╯ ◆ zzzzzzzzzzzz "###); @@ -638,15 +638,15 @@ fn test_prev_editing() { let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["prev"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - Working copy now at: rlvkpnrz 9ed53a4a (empty) second - Parent commit : qpvuntsm fa15625b (empty) first + Working copy now at: qpvuntsm fa15625b (empty) first + Parent commit : zzzzzzzz 00000000 (empty) (no description set) "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ○ zsuskulnrvyr fourth ○ kkmpptxzrspx third - @ rlvkpnrzqnoo second - ○ qpvuntsmwlqt first + ○ rlvkpnrzqnoo second + @ qpvuntsmwlqt first ◆ zzzzzzzzzzzz "###); }