diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index f2849a8514..1e7660c0f8 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1244,6 +1244,7 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin formatter.with_label("working_copy", |fmt| template.format(new_commit, fmt))?; writeln!(formatter)?; for parent in new_commit.parents() { + let parent = parent?; // "Working copy now at: " write!(formatter, "Parent commit : ")?; template.format(&parent, formatter.as_mut())?; diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 5d2136c27d..7aba4a0c12 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -14,6 +14,7 @@ use std::io::Write; +use itertools::Itertools; use jj_lib::matchers::EverythingMatcher; use jj_lib::object_id::ObjectId; use jj_lib::rewrite::merge_commit_trees; @@ -81,7 +82,7 @@ pub(crate) fn cmd_diffedit( } else { target_commit = workspace_command .resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?; - base_commits = target_commit.parents(); + base_commits = target_commit.parents().try_collect()?; diff_description = "The diff initially shows the commit's changes.".to_string(); }; workspace_command.check_rewritable([target_commit.id()])?; diff --git a/cli/src/commands/obslog.rs b/cli/src/commands/obslog.rs index 39f65b6aa8..11fc8f5715 100644 --- a/cli/src/commands/obslog.rs +++ b/cli/src/commands/obslog.rs @@ -12,8 +12,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +use itertools::Itertools; use jj_lib::commit::Commit; -use jj_lib::dag_walk::topo_order_reverse; +use jj_lib::dag_walk::topo_order_reverse_ok; use jj_lib::matchers::EverythingMatcher; use jj_lib::rewrite::rebase_to_dest_parent; use tracing::instrument; @@ -102,11 +103,11 @@ pub(crate) fn cmd_obslog( let mut formatter = ui.stdout_formatter(); let formatter = formatter.as_mut(); - let mut commits = topo_order_reverse( - vec![start_commit], + let mut commits = topo_order_reverse_ok( + vec![Ok(start_commit)], |commit: &Commit| commit.id().clone(), - |commit: &Commit| commit.predecessors(), - ); + |commit: &Commit| commit.predecessors().collect_vec(), + )?; if let Some(n) = args.limit { commits.truncate(n); } @@ -114,8 +115,8 @@ pub(crate) fn cmd_obslog( let mut graph = get_graphlog(command.settings(), formatter.raw()); for commit in commits { let mut edges = vec![]; - for predecessor in &commit.predecessors() { - edges.push(Edge::Direct(predecessor.id().clone())); + for predecessor in commit.predecessors() { + edges.push(Edge::Direct(predecessor?.id().clone())); } let mut buffer = vec![]; with_content_format.write_graph_text( @@ -164,13 +165,13 @@ fn show_predecessor_patch( commit: &Commit, diff_formats: &[DiffFormat], ) -> Result<(), CommandError> { - let predecessors = commit.predecessors(); - let predecessor = match predecessors.first() { - Some(predecessor) => predecessor, + let mut predecessors = commit.predecessors(); + let predecessor = match predecessors.next() { + Some(predecessor) => predecessor?, None => return Ok(()), }; let predecessor_tree = - rebase_to_dest_parent(workspace_command.repo().as_ref(), predecessor, commit)?; + rebase_to_dest_parent(workspace_command.repo().as_ref(), &predecessor, commit)?; let tree = commit.tree()?; diff_util::show_diff( ui, diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 6765197836..bd92e4feb3 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -394,7 +394,7 @@ fn rebase_descendants_transaction( workspace_command.check_rewritable(old_commits.iter().ids())?; let (skipped_commits, old_commits) = old_commits .iter() - .partition::, _>(|commit| commit.parents() == new_parents); + .partition::, _>(|commit| commit.parent_ids().iter().eq(new_parents.iter().ids())); let num_skipped_rebases = skipped_commits.len(); if num_skipped_rebases > 0 { writeln!( diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 825666351b..58290fb8ff 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -106,7 +106,7 @@ pub(crate) fn cmd_squash( } else { let source = workspace_command .resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?; - let mut parents = source.parents(); + let mut parents: Vec<_> = source.parents().try_collect()?; if parents.len() != 1 { return Err(user_error("Cannot squash merge commits")); } diff --git a/cli/src/commands/status.rs b/cli/src/commands/status.rs index 940c63b26e..3eaa52b1aa 100644 --- a/cli/src/commands/status.rs +++ b/cli/src/commands/status.rs @@ -87,6 +87,7 @@ pub(crate) fn cmd_status( formatter.with_label("working_copy", |fmt| template.format(wc_commit, fmt))?; writeln!(formatter)?; for parent in wc_commit.parents() { + let parent = parent?; write!(formatter, "Parent commit: ")?; template.format(&parent, formatter)?; writeln!(formatter)?; diff --git a/cli/src/commands/unsquash.rs b/cli/src/commands/unsquash.rs index da2c22178b..3cb3c9aad8 100644 --- a/cli/src/commands/unsquash.rs +++ b/cli/src/commands/unsquash.rs @@ -60,7 +60,7 @@ pub(crate) fn cmd_unsquash( if commit.parent_ids().len() > 1 { return Err(user_error("Cannot unsquash merge commits")); } - let parent = commit.parents().pop().unwrap(); + let parent = commit.parents().next().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())?) diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index 5756e5a78c..67ffa52f9f 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -198,7 +198,11 @@ fn cmd_workspace_add( .view() .get_wc_commit_id(old_workspace_command.workspace_id()) { - tx.repo().store().get_commit(old_wc_commit_id)?.parents() + tx.repo() + .store() + .get_commit(old_wc_commit_id)? + .parents() + .try_collect()? } else { vec![tx.repo().store().root_commit()] } diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 216e367464..cec7c9a24e 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -463,7 +463,7 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm "parents", |_language, _build_ctx, self_property, function| { template_parser::expect_no_arguments(function)?; - let out_property = self_property.map(|commit| commit.parents()); + let out_property = self_property.map(|commit| commit.parents().try_collect().unwrap()); Ok(L::wrap_commit_list(out_property)) }, ); diff --git a/lib/src/commit.rs b/lib/src/commit.rs index f8880471de..2165f12b6d 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -19,6 +19,8 @@ use std::fmt::{Debug, Error, Formatter}; use std::hash::{Hash, Hasher}; use std::sync::Arc; +use itertools::Itertools; + use crate::backend::{self, BackendResult, ChangeId, CommitId, MergedTreeId, Signature}; use crate::merged_tree::MergedTree; use crate::repo::Repo; @@ -82,24 +84,19 @@ impl Commit { &self.data.parents } - pub fn parents(&self) -> Vec { - self.data - .parents - .iter() - .map(|id| self.store.get_commit(id).unwrap()) - .collect() + pub fn parents(&self) -> impl Iterator> + '_ { + self.data.parents.iter().map(|id| self.store.get_commit(id)) } pub fn predecessor_ids(&self) -> &[CommitId] { &self.data.predecessors } - pub fn predecessors(&self) -> Vec { + pub fn predecessors(&self) -> impl Iterator> + '_ { self.data .predecessors .iter() - .map(|id| self.store.get_commit(id).unwrap()) - .collect() + .map(|id| self.store.get_commit(id)) } pub fn tree(&self) -> BackendResult { @@ -113,16 +110,18 @@ impl Commit { /// Return the parent tree, merging the parent trees if there are multiple /// parents. pub fn parent_tree(&self, repo: &dyn Repo) -> BackendResult { - merge_commit_trees(repo, &self.parents()) + let parents: Vec<_> = self.parents().try_collect()?; + merge_commit_trees(repo, &parents) } /// Returns whether commit's content is empty. Commit description is not /// taken into consideration. pub fn is_empty(&self, repo: &dyn Repo) -> BackendResult { - if let [parent] = &self.parents()[..] { + let parents: Vec<_> = self.parents().try_collect()?; + if let [parent] = &parents[..] { return Ok(parent.tree_id() == self.tree_id()); } - let parent_tree = self.parent_tree(repo)?; + let parent_tree = merge_commit_trees(repo, &parents)?; Ok(*self.tree_id() == parent_tree.id()) } @@ -156,13 +155,14 @@ impl Commit { /// A commit is discardable if it has one parent, no change from its /// parent, and an empty description. - pub fn is_discardable(&self) -> bool { + pub fn is_discardable(&self) -> BackendResult { if self.description().is_empty() { - if let [parent_commit] = &*self.parents() { - return self.tree_id() == parent_commit.tree_id(); + let parents: Vec<_> = self.parents().try_collect()?; + if let [parent_commit] = &*parents { + return Ok(self.tree_id() == parent_commit.tree_id()); } } - false + Ok(false) } /// A quick way to just check if a signature is present. diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs index 8583cbaed2..41140ad8ca 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -1068,7 +1068,7 @@ fn has_diff_from_parent( matcher: &dyn Matcher, ) -> bool { let commit = store.get_commit(&entry.commit_id()).unwrap(); - let parents = commit.parents(); + let parents: Vec<_> = commit.parents().try_collect().unwrap(); if let [parent] = parents.as_slice() { // Fast path: no need to load the root tree let unchanged = commit.tree_id() == parent.tree_id(); diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 91da297bbd..497274dab9 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -1158,6 +1158,10 @@ impl MutableRepo { visited.insert(commit.id().clone()); let mut dependents = vec![]; for parent in commit.parents() { + let Ok(parent) = parent else { + dependents.push(parent); + continue; + }; if let Some(rewrite) = self.parent_mapping.get(parent.id()) { for target in rewrite.new_parent_ids() { if to_visit_set.contains(target) && !visited.contains(target) { @@ -1347,7 +1351,7 @@ impl MutableRepo { .store() .get_commit(&wc_commit_id) .map_err(EditCommitError::WorkingCopyCommitNotFound)?; - if wc_commit.is_discardable() + if wc_commit.is_discardable()? && self .view .with_ref(|v| local_branch_target_ids(v).all(|id| id != wc_commit.id())) @@ -1762,6 +1766,8 @@ pub enum EditCommitError { WorkingCopyCommitNotFound(#[source] BackendError), #[error("Cannot rewrite the root commit")] RewriteRootCommit, + #[error(transparent)] + BackendError(#[from] BackendError), } /// Error from attempts to check out a commit diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index cd6190753a..b8df865e3f 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -199,7 +199,7 @@ impl<'repo> CommitRewriter<'repo> { settings: &UserSettings, empty: EmptyBehaviour, ) -> BackendResult>> { - let old_parents = self.old_commit.parents(); + let old_parents: Vec<_> = self.old_commit.parents().try_collect()?; let old_parent_trees = old_parents .iter() .map(|parent| parent.tree_id().clone()) diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index 976069a366..7ac52c69b1 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use itertools::Itertools; use jj_lib::backend::{ChangeId, MillisSinceEpoch, Signature, Timestamp}; use jj_lib::matchers::EverythingMatcher; use jj_lib::merged_tree::DiffSummary; @@ -77,8 +78,9 @@ fn test_initial(backend: TestRepoBackend) { let commit = builder.write().unwrap(); tx.commit("test"); - assert_eq!(commit.parents(), vec![store.root_commit()]); - assert_eq!(commit.predecessors(), vec![]); + let parents: Vec<_> = commit.parents().try_collect().unwrap(); + assert_eq!(parents, vec![store.root_commit()]); + assert!(commit.predecessors().next().is_none()); assert_eq!(commit.description(), "description"); assert_eq!(commit.author(), &author_signature); assert_eq!(commit.committer(), &committer_signature); @@ -152,11 +154,10 @@ fn test_rewrite(backend: TestRepoBackend) { .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap(); tx.commit("test"); - assert_eq!(rewritten_commit.parents(), vec![store.root_commit()]); - assert_eq!( - rewritten_commit.predecessors(), - vec![initial_commit.clone()] - ); + let parents: Vec<_> = rewritten_commit.parents().try_collect().unwrap(); + assert_eq!(parents, vec![store.root_commit()]); + let predecessors: Vec<_> = rewritten_commit.predecessors().try_collect().unwrap(); + assert_eq!(predecessors, vec![initial_commit.clone()]); assert_eq!(rewritten_commit.author().name, settings.user_name()); assert_eq!(rewritten_commit.author().email, settings.user_email()); assert_eq!( diff --git a/lib/tests/test_init.rs b/lib/tests/test_init.rs index b95bcb4f89..23ca072215 100644 --- a/lib/tests/test_init.rs +++ b/lib/tests/test_init.rs @@ -173,7 +173,7 @@ fn test_init_checkout(backend: TestRepoBackend) { wc_commit.store_commit().parents, vec![repo.store().root_commit_id().clone()] ); - assert_eq!(wc_commit.predecessors(), vec![]); + assert!(wc_commit.predecessors().next().is_none()); assert_eq!(wc_commit.description(), ""); assert_eq!(wc_commit.author().name, settings.user_name()); assert_eq!(wc_commit.author().email, settings.user_email()); diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index ca9dec82ab..a4139f6ff3 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -56,8 +56,8 @@ fn test_checkout() { .check_out(ws_id.clone(), &settings, &wc_commit_parent) .unwrap(); assert_eq!(wc_commit.tree_id(), wc_commit_parent.tree_id()); - assert_eq!(wc_commit.parents().len(), 1); - assert_eq!(wc_commit.parents()[0].id(), wc_commit_parent.id()); + assert_eq!(wc_commit.parent_ids().len(), 1); + assert_eq!(&wc_commit.parent_ids()[0], wc_commit_parent.id()); let repo = tx.commit("test"); assert_eq!(repo.view().get_wc_commit_id(&ws_id), Some(wc_commit.id())); }