Follow ups on post-submission comments for #4282.

* Derive a bunch of standard and useful traits for `movement_util::Direction`
  as it is a simple type. Importantly `Copy`.
* Return `&'static str` from Direction.cmd()
* Refactor out `MovementArgs` to reduce the number of arguments
  to `movement_util::move_to_commit`.
* Implement `From<&NextArgs/&PrevArgs>` for MovementArgs

Part of #3947

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),
     )
 }

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),
     )
 }

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<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 {
+        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<Commit, CommandError> {
-    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<Commit> = 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::<usize>().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();

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
     "###);
 }