From 3af4fae2348581f38dabe591f6ed1ff6af5fac94 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 5 May 2024 09:29:44 -0700 Subject: [PATCH] cleanup: propagate errors from `Commit::parents()` I updated callers to also propagate the error where it was trivial. --- cli/src/cli_util.rs | 2 +- cli/src/commands/commit.rs | 3 ++- cli/src/commands/diff.rs | 2 +- cli/src/commands/diffedit.rs | 2 +- cli/src/commands/rebase.rs | 2 +- cli/src/commands/restore.rs | 3 ++- cli/src/commands/split.rs | 3 ++- cli/src/commands/squash.rs | 5 +++-- cli/src/commands/status.rs | 5 +++-- cli/src/commands/unsquash.rs | 8 +++++--- cli/src/commands/workspace.rs | 2 +- cli/src/commit_templater.rs | 7 ++++--- cli/src/diff_util.rs | 2 +- lib/src/commit.rs | 12 ++++++------ lib/src/default_index/revset_engine.rs | 2 +- lib/src/repo.rs | 6 ++++-- lib/src/rewrite.rs | 11 +++++++---- lib/tests/test_commit_builder.rs | 7 +++++-- lib/tests/test_mut_repo.rs | 4 ++-- 19 files changed, 52 insertions(+), 36 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index ec892bd6df..9bf67470a1 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1238,7 +1238,7 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin write!(formatter, "Working copy now at: ")?; formatter.with_label("working_copy", |fmt| template.format(new_commit, fmt))?; writeln!(formatter)?; - for parent in new_commit.parents() { + for parent in new_commit.parents()? { // "Working copy now at: " write!(formatter, "Parent commit : ")?; template.format(&parent, formatter.as_mut())?; diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index a33e58624e..5c9f471218 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -61,7 +61,8 @@ pub(crate) fn cmd_commit( let diff_selector = workspace_command.diff_selector(ui, args.tool.as_deref(), args.interactive)?; let mut tx = workspace_command.start_transaction(); - let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?; + let parents = commit.parents()?; + let base_tree = merge_commit_trees(tx.repo(), &parents)?; let instructions = format!( "\ You are splitting the working-copy commit: {} diff --git a/cli/src/commands/diff.rs b/cli/src/commands/diff.rs index 6a44f67b61..0c70522f21 100644 --- a/cli/src/commands/diff.rs +++ b/cli/src/commands/diff.rs @@ -72,7 +72,7 @@ pub(crate) fn cmd_diff( } else { let commit = workspace_command .resolve_single_rev(args.revision.as_ref().unwrap_or(&RevisionArg::AT))?; - let parents = commit.parents(); + let parents = commit.parents()?; from_tree = merge_commit_trees(workspace_command.repo().as_ref(), &parents)?; to_tree = commit.tree()? } diff --git a/cli/src/commands/diffedit.rs b/cli/src/commands/diffedit.rs index 5d2136c27d..af7090bec8 100644 --- a/cli/src/commands/diffedit.rs +++ b/cli/src/commands/diffedit.rs @@ -81,7 +81,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()?; 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/rebase.rs b/cli/src/commands/rebase.rs index 6765197836..a8eebcf3ca 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.parents().unwrap() == new_parents); let num_skipped_rebases = skipped_commits.len(); if num_skipped_rebases > 0 { writeln!( diff --git a/cli/src/commands/restore.rs b/cli/src/commands/restore.rs index abee9a679b..32f5427185 100644 --- a/cli/src/commands/restore.rs +++ b/cli/src/commands/restore.rs @@ -94,7 +94,8 @@ pub(crate) fn cmd_restore( } else { to_commit = workspace_command .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())?; + let to_commit_parents = to_commit.parents()?; + from_tree = merge_commit_trees(workspace_command.repo().as_ref(), &to_commit_parents)?; } workspace_command.check_rewritable([to_commit.id()])?; diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 38514f04ec..9ceacc8a82 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -76,7 +76,8 @@ pub(crate) fn cmd_split( )?; let mut tx = workspace_command.start_transaction(); let end_tree = commit.tree()?; - let base_tree = merge_commit_trees(tx.repo(), &commit.parents())?; + let parents = commit.parents()?; + let base_tree = merge_commit_trees(tx.repo(), &parents)?; let instructions = format!( "\ You are splitting a commit into two: {} diff --git a/cli/src/commands/squash.rs b/cli/src/commands/squash.rs index 94656aae93..10f9b3e490 100644 --- a/cli/src/commands/squash.rs +++ b/cli/src/commands/squash.rs @@ -108,7 +108,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 = source.parents()?; if parents.len() != 1 { return Err(user_error("Cannot squash merge commits")); } @@ -191,7 +191,8 @@ pub fn move_diff( } let mut source_commits = vec![]; for source in sources { - let parent_tree = merge_commit_trees(tx.repo(), &source.parents())?; + let source_parents = source.parents()?; + let parent_tree = merge_commit_trees(tx.repo(), &source_parents)?; let source_tree = source.tree()?; let instructions = format!( "\ diff --git a/cli/src/commands/status.rs b/cli/src/commands/status.rs index 0030857922..09d1a34f11 100644 --- a/cli/src/commands/status.rs +++ b/cli/src/commands/status.rs @@ -59,7 +59,8 @@ pub(crate) fn cmd_status( let formatter = formatter.as_mut(); if let Some(wc_commit) = &maybe_wc_commit { - let parent_tree = merge_commit_trees(repo.as_ref(), &wc_commit.parents())?; + let wc_commit_parents = wc_commit.parents()?; + let parent_tree = merge_commit_trees(repo.as_ref(), &wc_commit_parents)?; let tree = wc_commit.tree()?; if tree.id() == parent_tree.id() { writeln!(formatter, "The working copy is clean")?; @@ -87,7 +88,7 @@ pub(crate) fn cmd_status( write!(formatter, "Working copy : ")?; formatter.with_label("working_copy", |fmt| template.format(wc_commit, fmt))?; writeln!(formatter)?; - for parent in wc_commit.parents() { + for parent in wc_commit.parents()? { 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 389512318f..53805ae037 100644 --- a/cli/src/commands/unsquash.rs +++ b/cli/src/commands/unsquash.rs @@ -58,10 +58,11 @@ pub(crate) fn cmd_unsquash( let mut workspace_command = command.workspace_helper(ui)?; let commit = workspace_command.resolve_single_rev(&args.revision)?; workspace_command.check_rewritable([commit.id()])?; - if commit.parent_ids().len() > 1 { + let mut parents = commit.parents()?; + if parents.len() > 1 { return Err(user_error("Cannot unsquash merge commits")); } - let parent = commit.parents().pop().unwrap(); + let parent = 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())?) @@ -69,7 +70,8 @@ pub(crate) fn cmd_unsquash( None }; let mut tx = workspace_command.start_transaction(); - let parent_base_tree = merge_commit_trees(tx.repo(), &parent.parents())?; + let grandparents = parent.parents()?; + let parent_base_tree = merge_commit_trees(tx.repo(), &grandparents)?; let new_parent_tree_id; if let Some(diff_editor) = &interactive_editor { let instructions = format!( diff --git a/cli/src/commands/workspace.rs b/cli/src/commands/workspace.rs index 5756e5a78c..b0bdeb7f13 100644 --- a/cli/src/commands/workspace.rs +++ b/cli/src/commands/workspace.rs @@ -198,7 +198,7 @@ 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()? } else { vec![tx.repo().store().root_commit()] } diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 1222f6cfc2..e997354a0e 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -467,7 +467,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().unwrap()); Ok(L::wrap_commit_list(out_property)) }, ); @@ -646,10 +646,11 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm template_parser::expect_no_arguments(function)?; let repo = language.repo; let out_property = self_property.and_then(|commit| { - if let [parent] = &commit.parents()[..] { + let parents = commit.parents()?; + if let [parent] = &parents[..] { return Ok(parent.tree_id() == commit.tree_id()); } - let parent_tree = rewrite::merge_commit_trees(repo, &commit.parents())?; + let parent_tree = rewrite::merge_commit_trees(repo, &parents)?; Ok(*commit.tree_id() == parent_tree.id()) }); Ok(L::wrap_boolean(out_property)) diff --git a/cli/src/diff_util.rs b/cli/src/diff_util.rs index fc9e06985b..7dc858cc03 100644 --- a/cli/src/diff_util.rs +++ b/cli/src/diff_util.rs @@ -237,7 +237,7 @@ pub fn show_patch( matcher: &dyn Matcher, formats: &[DiffFormat], ) -> Result<(), CommandError> { - let parents = commit.parents(); + let parents = commit.parents()?; let from_tree = rewrite::merge_commit_trees(workspace_command.repo().as_ref(), &parents)?; let to_tree = commit.tree()?; show_diff( diff --git a/lib/src/commit.rs b/lib/src/commit.rs index 0282c6d48a..64d9ae323a 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -80,11 +80,11 @@ impl Commit { &self.data.parents } - pub fn parents(&self) -> Vec { + pub fn parents(&self) -> BackendResult> { self.data .parents .iter() - .map(|id| self.store.get_commit(id).unwrap()) + .map(|id| self.store.get_commit(id)) .collect() } @@ -138,13 +138,13 @@ 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(); + if let [parent_commit] = &*self.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 452c1a4d10..852e4c172a 100644 --- a/lib/src/default_index/revset_engine.rs +++ b/lib/src/default_index/revset_engine.rs @@ -1060,7 +1060,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 = commit.parents().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 ea6ee95577..6daaa10e8c 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -1137,7 +1137,7 @@ impl MutableRepo { |commit| -> Vec> { visited.insert(commit.id().clone()); let mut dependents = vec![]; - for parent in commit.parents() { + for parent in commit.parents().unwrap() { if let Some((_, targets)) = self.parent_mapping.get(parent.id()) { for target in targets { if to_visit_set.contains(target) && !visited.contains(target) { @@ -1327,7 +1327,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())) @@ -1734,6 +1734,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 9e4b1c5ec4..7b5dd07c5c 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 = self.old_commit.parents()?; let old_parent_trees = old_parents .iter() .map(|parent| parent.tree_id().clone()) @@ -306,8 +306,10 @@ pub fn rebase_to_dest_parent( if source.parent_ids() == destination.parent_ids() { Ok(source.tree()?) } else { - let destination_parent_tree = merge_commit_trees(repo, &destination.parents())?; - let source_parent_tree = merge_commit_trees(repo, &source.parents())?; + let destination_parents = destination.parents()?; + let destination_parent_tree = merge_commit_trees(repo, &destination_parents)?; + let source_parents = source.parents()?; + let source_parent_tree = merge_commit_trees(repo, &source_parents)?; let source_tree = source.tree()?; let rebased_tree = destination_parent_tree.merge(&source_parent_tree, &source_tree)?; Ok(rebased_tree) @@ -320,7 +322,8 @@ pub fn back_out_commit( old_commit: &Commit, new_parents: &[Commit], ) -> BackendResult { - let old_base_tree = merge_commit_trees(mut_repo, &old_commit.parents())?; + let old_parents = old_commit.parents()?; + let old_base_tree = merge_commit_trees(mut_repo, &old_parents)?; let new_base_tree = merge_commit_trees(mut_repo, new_parents)?; let old_tree = old_commit.tree()?; let new_tree = new_base_tree.merge(&old_tree, &old_base_tree)?; diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index 976069a366..14d78eeaa3 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -77,7 +77,7 @@ fn test_initial(backend: TestRepoBackend) { let commit = builder.write().unwrap(); tx.commit("test"); - assert_eq!(commit.parents(), vec![store.root_commit()]); + assert_eq!(commit.parents().unwrap(), vec![store.root_commit()]); assert_eq!(commit.predecessors(), vec![]); assert_eq!(commit.description(), "description"); assert_eq!(commit.author(), &author_signature); @@ -152,7 +152,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.parents().unwrap(), + vec![store.root_commit()] + ); assert_eq!( rewritten_commit.predecessors(), vec![initial_commit.clone()] 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())); }