From d4a04779c0c7acb9ee95398271f2c468f058c5eb 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 | 16 ++++++++-------- lib/src/commit.rs | 13 +++++++++++++ 16 files changed, 46 insertions(+), 35 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 620878232..a393fc58d 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -958,14 +958,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 e46a7898e..48fcbbcbb 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 dfcd0ab1f..6f2d4af7d 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 b746a9282..a944eee50 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 7d1c307d0..ad0eeaf01 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 a245bdafc..a209ce610 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 eeeb50aee..592c4081d 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 0bddac84a..b47bd433e 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 233701873..f170008f2 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 e8de4bfe9..5a16be3cf 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 ada794d82..626f9f5c7 100644 --- a/cli/src/commands/resolve.rs +++ b/cli/src/commands/resolve.rs @@ -87,7 +87,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 03fc4820e..49def89df 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 a87119623..7521af558 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 995fcb5db..0141b75dc 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; @@ -148,7 +148,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 ca120d9f2..389512318 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 { @@ -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()?; diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 65e2e1379..7a82aefa5 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) -> impl Iterator + 'c; +} + +impl<'c, I> CommitIteratorExt<'c, I> for I +where + I: Iterator + 'c, +{ + fn ids(self) -> impl Iterator + '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);