Make check_rewritable take an iterator of &CommitId instead of &Commit

cli/src/cli_util.rs

@@ -957,14 +957,10 @@ impl WorkspaceCommandHelper {
 
     pub fn check_rewritable<'a>(
         &self,
-        commits: impl IntoIterator<Item = &'a Commit>,
+        commits: impl IntoIterator<Item = &'a CommitId>,
     ) -> Result<(), CommandError> {
-        let to_rewrite_revset = RevsetExpression::commits(
-            commits
-                .into_iter()
-                .map(|commit| commit.id().clone())
-                .collect(),
-        );
+        let to_rewrite_revset =
+            RevsetExpression::commits(commits.into_iter().cloned().collect_vec());
         let immutable = revset_util::parse_immutable_expression(&self.revset_parse_context())
             .map_err(|e| {
                 config_error_with_message("Invalid `revset-aliases.immutable_heads()`", e)

cli/src/commands/abandon.rs

@@ -15,6 +15,7 @@
 use std::io::Write;
 
 use itertools::Itertools as _;
+use jj_lib::commit::CommitIteratorExt;
 use jj_lib::object_id::ObjectId;
 use tracing::instrument;
 
@@ -58,7 +59,7 @@ pub(crate) fn cmd_abandon(
         writeln!(ui.status(), "No revisions to abandon.")?;
         return Ok(());
     }
-    workspace_command.check_rewritable(&to_abandon)?;
+    workspace_command.check_rewritable(to_abandon.iter().ids())?;
 
     let mut tx = workspace_command.start_transaction();
     for commit in &to_abandon {

cli/src/commands/chmod.rs

@@ -66,7 +66,7 @@ pub(crate) fn cmd_chmod(
         .map(|path| workspace_command.parse_file_path(path))
         .try_collect()?;
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
-    workspace_command.check_rewritable([&commit])?;
+    workspace_command.check_rewritable([commit.id()])?;
 
     let mut tx = workspace_command.start_transaction();
     let tree = commit.tree()?;

cli/src/commands/describe.rs

@@ -68,7 +68,7 @@ pub(crate) fn cmd_describe(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
-    workspace_command.check_rewritable([&commit])?;
+    workspace_command.check_rewritable([commit.id()])?;
     let description = if args.stdin {
         let mut buffer = String::new();
         io::stdin().read_to_string(&mut buffer).unwrap();

cli/src/commands/diffedit.rs

@@ -81,7 +81,7 @@ pub(crate) fn cmd_diffedit(
         base_commits = target_commit.parents();
         diff_description = "The diff initially shows the commit's changes.".to_string();
     };
-    workspace_command.check_rewritable([&target_commit])?;
+    workspace_command.check_rewritable([target_commit.id()])?;
 
     let diff_editor = workspace_command.diff_editor(ui, args.tool.as_deref())?;
     let mut tx = workspace_command.start_transaction();

cli/src/commands/edit.rs

@@ -44,7 +44,7 @@ pub(crate) fn cmd_edit(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let new_commit = workspace_command.resolve_single_rev(&args.revision)?;
-    workspace_command.check_rewritable([&new_commit])?;
+    workspace_command.check_rewritable([new_commit.id()])?;
     if workspace_command.get_wc_commit_id() == Some(new_commit.id()) {
         writeln!(ui.status(), "Already editing that commit")?;
     } else {

cli/src/commands/new.rs

@@ -16,7 +16,7 @@ use std::io::Write;
 
 use clap::ArgGroup;
 use itertools::Itertools;
-use jj_lib::commit::Commit;
+use jj_lib::commit::{Commit, CommitIteratorExt};
 use jj_lib::repo::Repo;
 use jj_lib::revset::{RevsetExpression, RevsetIteratorExt};
 use jj_lib::rewrite::{merge_commit_trees, rebase_commit};
@@ -107,8 +107,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
         // command line, add it between the changes' parents and the changes.
         // The parents of the new commit will be the parents of the target commits
         // which are not descendants of other target commits.
-        tx.base_workspace_helper()
-            .check_rewritable(&target_commits)?;
+        tx.base_workspace_helper().check_rewritable(&target_ids)?;
         let new_children = RevsetExpression::commits(target_ids.clone());
         let new_parents = new_children.parents();
         if let Some(commit_id) = new_children
@@ -160,7 +159,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
             vec![]
         };
         tx.base_workspace_helper()
-            .check_rewritable(&commits_to_rebase)?;
+            .check_rewritable(commits_to_rebase.iter().ids())?;
         let merged_tree = merge_commit_trees(tx.repo(), &target_commits)?;
         new_commit = tx
             .mut_repo()

cli/src/commands/next.rs

@@ -150,7 +150,7 @@ pub(crate) fn cmd_next(
     // We're editing, just move to the target commit.
     if edit {
         // We're editing, the target must be rewritable.
-        workspace_command.check_rewritable([target])?;
+        workspace_command.check_rewritable([target.id()])?;
         let mut tx = workspace_command.start_transaction();
         tx.edit(target)?;
         tx.finish(

cli/src/commands/prev.rs

@@ -118,7 +118,7 @@ pub(crate) fn cmd_prev(
     // 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])?;
+        workspace_command.check_rewritable([target.id()])?;
         let mut tx = workspace_command.start_transaction();
         tx.edit(target)?;
         tx.finish(

cli/src/commands/rebase.rs

@@ -21,7 +21,7 @@ use clap::ArgGroup;
 use indexmap::IndexSet;
 use itertools::Itertools;
 use jj_lib::backend::CommitId;
-use jj_lib::commit::Commit;
+use jj_lib::commit::{Commit, CommitIteratorExt};
 use jj_lib::object_id::ObjectId;
 use jj_lib::repo::{ReadonlyRepo, Repo};
 use jj_lib::revset::{RevsetExpression, RevsetIteratorExt};
@@ -314,7 +314,7 @@ fn rebase_descendants_transaction(
     old_commits: &IndexSet<Commit>,
     rebase_options: RebaseOptions,
 ) -> Result<(), CommandError> {
-    workspace_command.check_rewritable(old_commits)?;
+    workspace_command.check_rewritable(old_commits.iter().ids())?;
     let (skipped_commits, old_commits) = old_commits
         .iter()
         .partition::<Vec<_>, _>(|commit| commit.parents() == new_parents);
@@ -353,7 +353,7 @@ fn rebase_revision(
     rev_arg: &RevisionArg,
 ) -> Result<(), CommandError> {
     let old_commit = workspace_command.resolve_single_rev(rev_arg)?;
-    workspace_command.check_rewritable([&old_commit])?;
+    workspace_command.check_rewritable([old_commit.id()])?;
     if new_parents.contains(&old_commit) {
         return Err(user_error(format!(
             "Cannot rebase {} onto itself",
@@ -370,7 +370,9 @@ fn rebase_revision(
         .try_collect()?;
     // Currently, immutable commits are defined so that a child of a rewriteable
     // commit is always rewriteable.
-    debug_assert!(workspace_command.check_rewritable(&child_commits).is_ok());
+    debug_assert!(workspace_command
+        .check_rewritable(child_commits.iter().ids())
+        .is_ok());
 
     // First, rebase the children of `old_commit`.
     let mut tx = workspace_command.start_transaction();

cli/src/commands/resolve.rs

@@ -92,7 +92,7 @@ pub(crate) fn cmd_resolve(
     };
 
     let (repo_path, _) = conflicts.first().unwrap();
-    workspace_command.check_rewritable([&commit])?;
+    workspace_command.check_rewritable([commit.id()])?;
     let merge_editor = workspace_command.merge_editor(ui, args.tool.as_deref())?;
     writeln!(
         ui.status(),

cli/src/commands/restore.rs

@@ -96,7 +96,7 @@ pub(crate) fn cmd_restore(
             .resolve_single_rev(args.changes_in.as_ref().unwrap_or(&RevisionArg::AT))?;
         from_tree = merge_commit_trees(workspace_command.repo().as_ref(), &to_commit.parents())?;
     }
-    workspace_command.check_rewritable([&to_commit])?;
+    workspace_command.check_rewritable([to_commit.id()])?;
 
     let matcher = workspace_command.matcher_from_values(&args.paths)?;
     let to_tree = to_commit.tree()?;

cli/src/commands/split.rs

@@ -67,7 +67,7 @@ pub(crate) fn cmd_split(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
-    workspace_command.check_rewritable([&commit])?;
+    workspace_command.check_rewritable([commit.id()])?;
     let matcher = workspace_command.matcher_from_values(&args.paths)?;
     let diff_selector = workspace_command.diff_selector(
         ui,

cli/src/commands/squash.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use itertools::Itertools as _;
-use jj_lib::commit::Commit;
+use jj_lib::commit::{Commit, CommitIteratorExt};
 use jj_lib::matchers::Matcher;
 use jj_lib::object_id::ObjectId;
 use jj_lib::repo::Repo;
@@ -145,7 +145,7 @@ pub fn move_diff(
     path_arg: &[String],
 ) -> Result<(), CommandError> {
     tx.base_workspace_helper()
-        .check_rewritable(sources.iter().chain(std::iter::once(destination)))?;
+        .check_rewritable(sources.iter().chain(std::iter::once(destination)).ids())?;
     // Tree diffs to apply to the destination
     let mut tree_diffs = vec![];
     let mut abandoned_commits = vec![];

cli/src/commands/unsquash.rs

@@ -57,13 +57,12 @@ pub(crate) fn cmd_unsquash(
 ) -> Result<(), CommandError> {
     let mut workspace_command = command.workspace_helper(ui)?;
     let commit = workspace_command.resolve_single_rev(&args.revision)?;
-    workspace_command.check_rewritable([&commit])?;
-    let parents = commit.parents();
-    if parents.len() != 1 {
+    workspace_command.check_rewritable([commit.id()])?;
+    if commit.parent_ids().len() > 1 {
         return Err(user_error("Cannot unsquash merge commits"));
     }
-    let parent = &parents[0];
-    workspace_command.check_rewritable(&parents[..1])?;
+    let parent = commit.parents().pop().unwrap();
+    workspace_command.check_rewritable([parent.id()])?;
     let interactive_editor = if args.tool.is_some() || args.interactive {
         Some(workspace_command.diff_editor(ui, args.tool.as_deref())?)
     } else {
@@ -85,7 +84,7 @@ the parent commit. The changes you edited out will be moved into the
 child commit. If you don't make any changes, then the operation will be
 aborted.
 ",
-            tx.format_commit_summary(parent),
+            tx.format_commit_summary(&parent),
             tx.format_commit_summary(&commit)
         );
         let parent_tree = parent.tree()?;
@@ -105,7 +104,8 @@ aborted.
     // case).
     if new_parent_tree_id == parent_base_tree.id() {
         tx.mut_repo().record_abandoned_commit(parent.id().clone());
-        let description = combine_messages(tx.base_repo(), &[parent], &commit, command.settings())?;
+        let description =
+            combine_messages(tx.base_repo(), &[&parent], &commit, command.settings())?;
         // Commit the new child on top of the parent's parents.
         tx.mut_repo()
             .rewrite_commit(command.settings(), &commit)
@@ -115,7 +115,7 @@ aborted.
     } else {
         let new_parent = tx
             .mut_repo()
-            .rewrite_commit(command.settings(), parent)
+            .rewrite_commit(command.settings(), &parent)
             .set_tree_id(new_parent_tree_id)
             .set_predecessors(vec![parent.id().clone(), commit.id().clone()])
             .write()?;

lib/src/commit.rs

@@ -163,6 +163,19 @@ impl Commit {
     }
 }
 
+pub trait CommitIteratorExt<'c, I> {
+    fn ids(self) -> impl Iterator<Item = &'c CommitId> + 'c;
+}
+
+impl<'c, I> CommitIteratorExt<'c, I> for I
+where
+    I: Iterator<Item = &'c Commit> + 'c,
+{
+    fn ids(self) -> impl Iterator<Item = &'c CommitId> + 'c {
+        Box::new(self.map(|commit| commit.id()))
+    }
+}
+
 /// Wrapper to sort `Commit` by committer timestamp.
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub(crate) struct CommitByCommitterTimestamp(pub Commit);