From 8de7a17e700929fd6a2f81e516d9603a9572d768 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 7b55b22064..2e1c0de806 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 e46a7898ec..48fcbbcbb8 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 dfcd0ab1f8..6f2d4af7dc 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 b746a92829..a944eee50b 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 7d1c307d07..ad0eeaf010 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 a245bdafcf..a209ce610a 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 eeeb50aee6..592c4081d4 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 0bddac84ac..b47bd433e4 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 2337018730..f170008f2b 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 e8de4bfe96..5a16be3cf4 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 fcae53b61a..e8f6044af9 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 03fc4820ed..49def89df2 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 a871196237..7521af558d 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 36b3c60c60..b18c8b8ad6 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 ca120d9f29..389512318f 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 65e2e1379d..3847a9f08a 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);