From 67a730c451d1f827a06163960f025f8d3c69863a Mon Sep 17 00:00:00 2001
From: Essien Ita Essien <34972+essiene@users.noreply.github.com>
Date: Thu, 15 Aug 2024 16:51:27 +0100
Subject: [PATCH] next/prev: refactor movement utilities into
 cli/src/movement_utils.rs

The code in both cli/src/commands/{next,prev}.rs is identical except
for the direction of movement. This commit pull the parts that make
sense out into cli/src/movement_util.rs so it's easier to see the
differences.

Part of #3947
---
 cli/src/commands/next.rs | 118 ++-------------------
 cli/src/commands/prev.rs |  83 ++-------------
 cli/src/lib.rs           |   1 +
 cli/src/movement_util.rs | 217 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 240 insertions(+), 179 deletions(-)
 create mode 100644 cli/src/movement_util.rs

diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs
index 6a78a790b6d..716949fdfd6 100644
--- a/cli/src/commands/next.rs
+++ b/cli/src/commands/next.rs
@@ -12,15 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::io::Write;
-
-use itertools::Itertools;
-use jj_lib::commit::Commit;
-use jj_lib::repo::Repo;
-use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};
-
-use crate::cli_util::{short_commit_hash, CommandHelper, WorkspaceCommandHelper};
-use crate::command_error::{user_error, CommandError};
+use crate::cli_util::CommandHelper;
+use crate::command_error::CommandError;
+use crate::movement_util::{move_to_commit, Direction};
 use crate::ui::Ui;
 
 /// Move the working-copy commit to the child revision
@@ -70,108 +64,18 @@ pub(crate) struct NextArgs {
     conflict: bool,
 }
 
-pub fn choose_commit<'a>(
-    ui: &mut Ui,
-    workspace_command: &WorkspaceCommandHelper,
-    cmd: &str,
-    commits: &'a [Commit],
-) -> Result<&'a Commit, CommandError> {
-    writeln!(ui.stdout(), "ambiguous {cmd} commit, choose one to target:")?;
-    let mut formatter = ui.stdout_formatter();
-    let template = workspace_command.commit_summary_template();
-    let mut choices: Vec<String> = Default::default();
-    for (i, commit) in commits.iter().enumerate() {
-        write!(formatter, "{}: ", i + 1)?;
-        template.format(commit, formatter.as_mut())?;
-        writeln!(formatter)?;
-        choices.push(format!("{}", i + 1));
-    }
-    writeln!(formatter, "q: quit the prompt")?;
-    choices.push("q".to_string());
-    drop(formatter);
-
-    let choice = ui.prompt_choice(
-        "enter the index of the commit you want to target",
-        &choices,
-        None,
-    )?;
-    if choice == "q" {
-        return Err(user_error("ambiguous target commit"));
-    }
-
-    Ok(&commits[choice.parse::<usize>().unwrap() - 1])
-}
-
 pub(crate) fn cmd_next(
     ui: &mut Ui,
     command: &CommandHelper,
     args: &NextArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let current_wc_id = workspace_command
-        .get_wc_commit_id()
-        .ok_or_else(|| user_error("This command requires a working copy"))?;
-    let edit = args.edit
-        || !workspace_command
-            .repo()
-            .view()
-            .heads()
-            .contains(current_wc_id);
-    let wc_revset = RevsetExpression::commit(current_wc_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 = if args.conflict {
-        start_revset
-            .children()
-            .descendants()
-            .filtered(RevsetFilterPredicate::HasConflict)
-            .roots()
-    } else {
-        start_revset.descendants_at(args.offset)
-    }
-    .minus(&wc_revset);
-
-    let targets: Vec<Commit> = target_revset
-        .evaluate_programmatic(workspace_command.repo().as_ref())?
-        .iter()
-        .commits(workspace_command.repo().store())
-        .try_collect()?;
-
-    let target = match targets.as_slice() {
-        [target] => target,
-        [] => {
-            // We found no descendant.
-            return Err(user_error(format!(
-                "No descendant found {} commit{} forward",
-                args.offset,
-                if args.offset > 1 { "s" } else { "" }
-            )));
-        }
-        commits => choose_commit(ui, &workspace_command, "next", commits)?,
-    };
-    let current_short = short_commit_hash(current_wc_id);
-    let target_short = short_commit_hash(target.id());
-    // We're editing, just move to the target commit.
-    if edit {
-        // We're editing, the target must be rewritable.
-        workspace_command.check_rewritable([target.id()])?;
-        let mut tx = workspace_command.start_transaction();
-        tx.edit(target)?;
-        tx.finish(
-            ui,
-            format!("next: {current_short} -> editing {target_short}"),
-        )?;
-        return Ok(());
-    }
-    let mut tx = workspace_command.start_transaction();
-    // Move the working-copy commit to the new parent.
-    tx.check_out(target)?;
-    tx.finish(ui, format!("next: {current_short} -> {target_short}"))?;
-    Ok(())
+    move_to_commit(
+        ui,
+        &mut workspace_command,
+        Direction::Next,
+        args.edit,
+        args.conflict,
+        args.offset,
+    )
 }
diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs
index 659f116e6c6..e855816ace1 100644
--- a/cli/src/commands/prev.rs
+++ b/cli/src/commands/prev.rs
@@ -12,13 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use itertools::Itertools;
-use jj_lib::repo::Repo;
-use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};
-
-use crate::cli_util::{short_commit_hash, CommandHelper};
-use crate::command_error::{user_error, CommandError};
-use crate::commands::next::choose_commit;
+use crate::cli_util::CommandHelper;
+use crate::command_error::CommandError;
+use crate::movement_util::{move_to_commit, Direction};
 use crate::ui::Ui;
 /// Change the working copy revision relative to the parent revision
 ///
@@ -70,69 +66,12 @@ pub(crate) fn cmd_prev(
     args: &PrevArgs,
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
-    let current_wc_id = workspace_command
-        .get_wc_commit_id()
-        .ok_or_else(|| user_error("This command requires a working copy"))?;
-    let edit = args.edit
-        || !workspace_command
-            .repo()
-            .view()
-            .heads()
-            .contains(current_wc_id);
-    let wc_revset = RevsetExpression::commit(current_wc_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 = 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)
-    };
-    let targets: Vec<_> = target_revset
-        .evaluate_programmatic(workspace_command.repo().as_ref())?
-        .iter()
-        .commits(workspace_command.repo().store())
-        .try_collect()?;
-    let target = match targets.as_slice() {
-        [target] => target,
-        [] => {
-            return Err(user_error(format!(
-                "No ancestor found {} commit{} back",
-                args.offset,
-                if args.offset > 1 { "s" } else { "" }
-            )))
-        }
-        commits => choose_commit(ui, &workspace_command, "prev", commits)?,
-    };
-
-    // Generate a short commit hash, to make it readable in the op log.
-    let current_short = short_commit_hash(current_wc_id);
-    let target_short = short_commit_hash(target.id());
-    // If we're editing, just move to the revision directly.
-    if edit {
-        // The target must be rewritable if we're editing.
-        workspace_command.check_rewritable([target.id()])?;
-        let mut tx = workspace_command.start_transaction();
-        tx.edit(target)?;
-        tx.finish(
-            ui,
-            format!("prev: {current_short} -> editing {target_short}"),
-        )?;
-        return Ok(());
-    }
-    let mut tx = workspace_command.start_transaction();
-    tx.check_out(target)?;
-    tx.finish(ui, format!("prev: {current_short} -> {target_short}"))?;
-    Ok(())
+    move_to_commit(
+        ui,
+        &mut workspace_command,
+        Direction::Prev,
+        args.edit,
+        args.conflict,
+        args.offset,
+    )
 }
diff --git a/cli/src/lib.rs b/cli/src/lib.rs
index 6e60b293a7d..20004e3ff58 100644
--- a/cli/src/lib.rs
+++ b/cli/src/lib.rs
@@ -27,6 +27,7 @@ pub mod generic_templater;
 pub mod git_util;
 pub mod graphlog;
 pub mod merge_tools;
+pub mod movement_util;
 pub mod operation_templater;
 mod progress;
 pub mod revset_util;
diff --git a/cli/src/movement_util.rs b/cli/src/movement_util.rs
new file mode 100644
index 00000000000..81cba00a7a1
--- /dev/null
+++ b/cli/src/movement_util.rs
@@ -0,0 +1,217 @@
+// Copyright 2020 The Jujutsu Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::io::Write;
+use std::rc::Rc;
+
+use itertools::Itertools;
+use jj_lib::backend::CommitId;
+use jj_lib::commit::Commit;
+use jj_lib::repo::Repo;
+use jj_lib::revset::{RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt};
+
+use crate::cli_util::{short_commit_hash, WorkspaceCommandHelper};
+use crate::command_error::{user_error, CommandError};
+use crate::ui::Ui;
+
+pub enum Direction {
+    Next,
+    Prev,
+}
+
+impl Direction {
+    fn cmd(&self) -> String {
+        match self {
+            Direction::Next => "next".to_string(),
+            Direction::Prev => "prev".to_string(),
+        }
+    }
+
+    fn target_not_found_message(&self, change_offset: u64) -> String {
+        let commit_text = if change_offset > 1 {
+            "commits"
+        } else {
+            "commit"
+        };
+
+        match self {
+            Direction::Next => format!(
+                "No descendant found {} {} forward",
+                change_offset, commit_text
+            ),
+            Direction::Prev => format!("No ancestor found {} {} back", change_offset, commit_text),
+        }
+    }
+
+    fn get_target_revset(
+        &self,
+        working_commit_id: &CommitId,
+        edit: bool,
+        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
+                    .children()
+                    .descendants()
+                    .filtered(RevsetFilterPredicate::HasConflict)
+                    .roots()
+            } else {
+                start_revset.descendants_at(change_offset)
+            }
+            .minus(&wc_revset),
+
+            Direction::Prev => {
+                if has_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(change_offset)
+                }
+            }
+        };
+
+        Ok(target_revset)
+    }
+}
+
+fn get_target_commit(
+    ui: &mut Ui,
+    workspace_command: &WorkspaceCommandHelper,
+    direction: Direction,
+    working_commit_id: &CommitId,
+    edit: bool,
+    has_conflict: bool,
+    change_offset: u64,
+) -> Result<Commit, CommandError> {
+    let target_revset =
+        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()
+        .commits(workspace_command.repo().store())
+        .try_collect()?;
+
+    let target = match targets.as_slice() {
+        [target] => target,
+        [] => {
+            // We found no ancestor/descendant.
+            return Err(user_error(
+                direction.target_not_found_message(change_offset),
+            ));
+        }
+        commits => choose_commit(ui, workspace_command, &direction, commits)?,
+    };
+
+    Ok(target.clone())
+}
+
+fn choose_commit<'a>(
+    ui: &mut Ui,
+    workspace_command: &WorkspaceCommandHelper,
+    direction: &Direction,
+    commits: &'a [Commit],
+) -> Result<&'a Commit, CommandError> {
+    writeln!(
+        ui.stdout(),
+        "ambiguous {} commit, choose one to target:",
+        direction.cmd()
+    )?;
+    let mut formatter = ui.stdout_formatter();
+    let template = workspace_command.commit_summary_template();
+    let mut choices: Vec<String> = Default::default();
+    for (i, commit) in commits.iter().enumerate() {
+        write!(formatter, "{}: ", i + 1)?;
+        template.format(commit, formatter.as_mut())?;
+        writeln!(formatter)?;
+        choices.push(format!("{}", i + 1));
+    }
+    writeln!(formatter, "q: quit the prompt")?;
+    choices.push("q".to_string());
+    drop(formatter);
+
+    let choice = ui.prompt_choice(
+        "enter the index of the commit you want to target",
+        &choices,
+        None,
+    )?;
+    if choice == "q" {
+        return Err(user_error("ambiguous target commit"));
+    }
+
+    Ok(&commits[choice.parse::<usize>().unwrap() - 1])
+}
+
+pub fn move_to_commit(
+    ui: &mut Ui,
+    workspace_command: &mut WorkspaceCommandHelper,
+    direction: Direction,
+    edit: bool,
+    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
+        || !&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 current_short = short_commit_hash(current_wc_id);
+    let target_short = short_commit_hash(target.id());
+    // We're editing, just move to the target commit.
+    if edit {
+        // We're editing, the target must be rewritable.
+        workspace_command.check_rewritable([target.id()])?;
+        let mut tx = workspace_command.start_transaction();
+        tx.edit(&target)?;
+        tx.finish(
+            ui,
+            format!("next: {current_short} -> editing {target_short}"),
+        )?;
+        return Ok(());
+    }
+    let mut tx = workspace_command.start_transaction();
+    // Move the working-copy commit to the new parent.
+    tx.check_out(&target)?;
+    tx.finish(ui, format!("next: {current_short} -> {target_short}"))?;
+    Ok(())
+}