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
@@ -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,
@@ -73,9 +83,7 @@ pub(crate) fn cmd_next(
     move_to_commit(
         ui,
         &mut workspace_command,
-        &Direction::Next,
+        Direction::Next,
-        args.edit,
+        &MovementArgs::from(args),
-        args.conflict,
-        args.offset,
     )
 }

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
 ///
@@ -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,
@@ -69,9 +79,7 @@ pub(crate) fn cmd_prev(
     move_to_commit(
         ui,
         &mut workspace_command,
-        &Direction::Prev,
+        Direction::Prev,
-        args.edit,
+        &MovementArgs::from(args),
-        args.conflict,
-        args.offset,
     )
 }

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, Debug, Eq, PartialEq)]
+pub(crate) struct MovementArgs {
+    pub offset: u64,
+    pub edit: bool,
+    pub 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::Next => "next",
-            Direction::Prev => "prev".to_string(),
+            Direction::Prev => "prev",
         }
     }
 
@@ -48,33 +56,31 @@ impl Direction {
     fn get_target_revset(
         &self,
         working_commit_id: &CommitId,
-        edit: bool,
+        args: &MovementArgs,
-        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 {
+        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)
                 }
             }
         };
@@ -95,14 +101,11 @@ impl Direction {
 fn get_target_commit(
     ui: &mut Ui,
     workspace_command: &WorkspaceCommandHelper,
-    direction: &Direction,
+    direction: Direction,
     working_commit_id: &CommitId,
-    edit: bool,
+    args: &MovementArgs,
-    has_conflict: bool,
-    change_offset: u64,
 ) -> Result<Commit, CommandError> {
-    let target_revset =
+    let target_revset = direction.get_target_revset(working_commit_id, args)?;
-        direction.get_target_revset(working_commit_id, edit, has_conflict, change_offset)?;
     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(
+            return Err(user_error(direction.target_not_found_message(args.offset)));
-                direction.target_not_found_message(change_offset),
-            ));
         }
         commits => choose_commit(ui, workspace_command, direction, commits)?,
     };
@@ -126,7 +127,7 @@ fn get_target_commit(
 fn choose_commit<'a>(
     ui: &mut Ui,
     workspace_command: &WorkspaceCommandHelper,
-    direction: &Direction,
+    direction: Direction,
     commits: &'a [Commit],
 ) -> Result<&'a Commit, CommandError> {
     writeln!(
@@ -159,39 +160,31 @@ 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,
+    direction: Direction,
-    edit: bool,
+    args: &MovementArgs,
-    has_conflict: bool,
-    change_offset: u64,
 ) -> 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
+
+    let mut args = args.clone();
+    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();