From fa59510dacf01a08e7487e829f0aa2f98b5c1cc2 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Wed, 3 Apr 2024 11:01:54 -0400 Subject: [PATCH] Make check_rewritable take an iterator of &CommitId instead of &Commit This function doesn't actually need commits, it only needs their IDs. In some contexts we may only have commit IDs, so there's no need to require an iterator of Commits. This commit also adds a `CommitIteratorExt` that makes it easy to convert an iterator of `&Commit` to an iterator of `&CommitId`. --- cli/src/cli_util.rs | 10 +++------- cli/src/commands/abandon.rs | 3 ++- cli/src/commands/chmod.rs | 2 +- cli/src/commands/describe.rs | 2 +- cli/src/commands/diffedit.rs | 2 +- cli/src/commands/edit.rs | 2 +- cli/src/commands/new.rs | 7 +++---- cli/src/commands/next.rs | 2 +- cli/src/commands/prev.rs | 2 +- cli/src/commands/rebase.rs | 10 ++++++---- cli/src/commands/resolve.rs | 2 +- cli/src/commands/restore.rs | 2 +- cli/src/commands/split.rs | 2 +- cli/src/commands/squash.rs | 4 ++-- cli/src/commands/unsquash.rs | 9 ++++----- lib/src/commit.rs | 13 +++++++++++++ 16 files changed, 42 insertions(+), 32 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 7b55b22064b..2e1c0de8062 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -957,14 +957,10 @@ impl WorkspaceCommandHelper { pub fn check_rewritable<'a>( &self, - commits: impl IntoIterator, + commits: impl IntoIterator, ) -> 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) diff --git a/cli/src/commands/abandon.rs b/cli/src/commands/abandon.rs index e46a7898ec2..48fcbbcbb8e 100644 --- a/cli/src/commands/abandon.rs +++ b/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 { diff --git a/cli/src/commands/chmod.rs b/cli/src/commands/chmod.rs index dfcd0ab1f8b..6f2d4af7dcd 100644 --- a/cli/src/commands/chmod.rs +++ b/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()?; diff --git a/cli/src/commands/describe.rs b/cli/src/commands/describe.rs index b746a92829b..a944eee50b4 100644 --- a/cli/src/commands/describe.rs +++ b/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(); diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 7d1c307d074..ad0eeaf0104 100644 --- a/cli/src/commands/diffedit.rs +++ b/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(); diff --git a/cli/src/commands/edit.rs b/cli/src/commands/edit.rs index a245bdafcf3..a209ce610ad 100644 --- a/cli/src/commands/edit.rs +++ b/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 { diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index eeeb50aee6d..592c4081d4f 100644 --- a/cli/src/commands/new.rs +++ b/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() diff --git a/cli/src/commands/next.rs b/cli/src/commands/next.rs index 0bddac84ac5..b47bd433e40 100644 --- a/cli/src/commands/next.rs +++ b/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( diff --git a/cli/src/commands/prev.rs b/cli/src/commands/prev.rs index 23370187308..f170008f2b8 100644 --- a/cli/src/commands/prev.rs +++ b/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( diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index e8de4bfe969..5a16be3cf42 100644 --- a/cli/src/commands/rebase.rs +++ b/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, 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::, _>(|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(); diff --git a/cli/src/commands/resolve.rs b/cli/src/commands/resolve.rs index fcae53b61ad..e8f6044af9f 100644 --- a/cli/src/commands/resolve.rs +++ b/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(), diff --git a/cli/src/commands/restore.rs b/cli/src/commands/restore.rs index 03fc4820ed8..49def89df2b 100644 --- a/cli/src/commands/restore.rs +++ b/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()?; diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index a871196237e..7521af558d9 100644 --- a/cli/src/commands/split.rs +++ b/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, diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 36b3c60c608..b18c8b8ad6f 100644 --- a/cli/src/commands/squash.rs +++ b/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![]; diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index ca120d9f291..b4f8e8579f3 100644 --- a/cli/src/commands/unsquash.rs +++ b/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 { diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 65e2e1379da..3847a9f08a7 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -163,6 +163,19 @@ impl Commit { } } +pub trait CommitIteratorExt<'c, I> { + fn ids(self) -> Box + 'c>; +} + +impl<'c, I> CommitIteratorExt<'c, I> for I +where + I: Iterator + 'c, +{ + fn ids(self) -> Box + '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);