From aa7f2712d72528296db6824a6caf42191adbe40b Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 30 Mar 2024 16:16:32 +0900 Subject: [PATCH 1/4] tree: consolidate read error variants There isn't much difference between BackendError::ReadObject of file type and TreeMergeError::ReadError. They are both caused by the backend. --- lib/src/tree.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/src/tree.rs b/lib/src/tree.rs index e1b4c70a64..3b94dead5c 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -24,7 +24,7 @@ use thiserror::Error; use tracing::instrument; use crate::backend::{ - BackendError, ConflictId, FileId, TreeEntriesNonRecursiveIterator, TreeEntry, TreeId, TreeValue, + BackendError, ConflictId, TreeEntriesNonRecursiveIterator, TreeEntry, TreeId, TreeValue, }; use crate::files::MergeResult; use crate::matchers::{EverythingMatcher, Matcher}; @@ -36,11 +36,6 @@ use crate::{backend, files}; #[derive(Debug, Error)] pub enum TreeMergeError { - #[error("Failed to read file with ID {} ", .file_id.hex())] - ReadError { - source: std::io::Error, - file_id: FileId, - }, #[error(transparent)] BackendError(#[from] BackendError), } @@ -448,9 +443,10 @@ pub fn try_resolve_file_conflict( store .read_file(filename, file_id)? .read_to_end(&mut content) - .map_err(|err| TreeMergeError::ReadError { - source: err, - file_id: file_id.clone(), + .map_err(|err| BackendError::ReadObject { + object_type: file_id.object_type(), + hash: file_id.hex(), + source: err.into(), })?; Ok(content) })?; From 3b692b4c6bf65898417595f3a75635e021915299 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 30 Mar 2024 16:29:10 +0900 Subject: [PATCH 2/4] tree: flatten TreeMergeError into BackendError --- cli/src/command_error.rs | 11 ----------- cli/tests/test_abandon_command.rs | 3 +-- cli/tests/test_rebase_command.rs | 3 +-- lib/src/merged_tree.rs | 16 ++++++---------- lib/src/repo.rs | 13 ++++++------- lib/src/rewrite.rs | 24 ++++++++++-------------- lib/src/tree.rs | 24 +++++++----------------- 7 files changed, 31 insertions(+), 63 deletions(-) diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index 2df88f91ca..0083e2d06a 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -30,7 +30,6 @@ use jj_lib::revset::{ RevsetEvaluationError, RevsetParseError, RevsetParseErrorKind, RevsetResolutionError, }; use jj_lib::signing::SignInitError; -use jj_lib::tree::TreeMergeError; use jj_lib::working_copy::{ResetError, SnapshotError, WorkingCopyStateError}; use jj_lib::workspace::WorkspaceInitError; use thiserror::Error; @@ -306,16 +305,6 @@ want this file to be snapshotted. Otherwise add it to your `.gitignore` file."#, } } -impl From for CommandError { - fn from(err: TreeMergeError) -> Self { - let kind = match &err { - TreeMergeError::BackendError(BackendError::Unsupported(_)) => CommandErrorKind::User, - _ => CommandErrorKind::Internal, - }; - CommandError::with_message(kind, "Merge failed", err) - } -} - impl From for CommandError { fn from(err: OpStoreError) -> Self { internal_error_with_message("Failed to load an operation", err) diff --git a/cli/tests/test_abandon_command.rs b/cli/tests/test_abandon_command.rs index 2f8b9d3d61..f6214ac9cd 100644 --- a/cli/tests/test_abandon_command.rs +++ b/cli/tests/test_abandon_command.rs @@ -316,8 +316,7 @@ fn test_bug_2600_rootcommit_special_case() { // Now, the test let stderr = test_env.jj_cmd_failure(&repo_path, &["abandon", "base"]); insta::assert_snapshot!(stderr, @r###" - Error: Merge failed - Caused by: The Git backend does not support creating merge commits with the root commit as one of the parents. + Error: The Git backend does not support creating merge commits with the root commit as one of the parents. "###); } diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs index 8d22cdaf75..fec0f235bc 100644 --- a/cli/tests/test_rebase_command.rs +++ b/cli/tests/test_rebase_command.rs @@ -515,8 +515,7 @@ fn test_rebase_multiple_destinations() { &["rebase", "-r", "a", "-d", "b", "-d", "root()"], ); insta::assert_snapshot!(stderr, @r###" - Error: Merge failed - Caused by: The Git backend does not support creating merge commits with the root commit as one of the parents. + Error: The Git backend does not support creating merge commits with the root commit as one of the parents. "###); } diff --git a/lib/src/merged_tree.rs b/lib/src/merged_tree.rs index aa30177f2a..2bdab0d8a6 100644 --- a/lib/src/merged_tree.rs +++ b/lib/src/merged_tree.rs @@ -31,7 +31,7 @@ use crate::matchers::{EverythingMatcher, Matcher}; use crate::merge::{Merge, MergeBuilder, MergedTreeValue}; use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent, RepoPathComponentsIter}; use crate::store::Store; -use crate::tree::{try_resolve_file_conflict, Tree, TreeMergeError}; +use crate::tree::{try_resolve_file_conflict, Tree}; use crate::tree_builder::TreeBuilder; use crate::{backend, tree}; @@ -184,7 +184,7 @@ impl MergedTree { /// automatically resolved and leaving the rest unresolved. The returned /// conflict will either be resolved or have the same number of sides as /// the input. - pub fn resolve(&self) -> Result, TreeMergeError> { + pub fn resolve(&self) -> BackendResult> { match self { MergedTree::Legacy(tree) => Ok(Merge::resolved(tree.clone())), MergedTree::Merge(trees) => merge_trees(trees), @@ -362,11 +362,7 @@ impl MergedTree { } /// Merges this tree with `other`, using `base` as base. - pub fn merge( - &self, - base: &MergedTree, - other: &MergedTree, - ) -> Result { + pub fn merge(&self, base: &MergedTree, other: &MergedTree) -> BackendResult { if let (MergedTree::Legacy(this), MergedTree::Legacy(base), MergedTree::Legacy(other)) = (self, base, other) { @@ -374,7 +370,7 @@ impl MergedTree { Ok(MergedTree::legacy(merged_tree)) } else { // Convert legacy trees to merged trees and unwrap to `Merge` - let to_merge = |tree: &MergedTree| -> Result, TreeMergeError> { + let to_merge = |tree: &MergedTree| -> BackendResult> { match tree { MergedTree::Legacy(tree) => { let MergedTree::Merge(tree) = MergedTree::from_legacy_tree(tree.clone())? @@ -475,7 +471,7 @@ fn trees_value<'a>(trees: &'a Merge, basename: &RepoPathComponent) -> Merg MergedTreeVal::Conflict(value.map(|x| x.cloned())) } -fn merge_trees(merge: &Merge) -> Result, TreeMergeError> { +fn merge_trees(merge: &Merge) -> BackendResult> { if let Some(tree) = merge.resolve_trivial() { return Ok(Merge::resolved(tree.clone())); } @@ -529,7 +525,7 @@ fn merge_tree_values( store: &Arc, path: &RepoPath, values: MergedTreeValue, -) -> Result { +) -> BackendResult { if let Some(resolved) = values.resolve_trivial() { return Ok(Merge::resolved(resolved.clone())); } diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 541972dce1..7b5695a3a7 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -57,7 +57,6 @@ use crate::simple_op_store::SimpleOpStore; use crate::store::Store; use crate::submodule_store::SubmoduleStore; use crate::transaction::Transaction; -use crate::tree::TreeMergeError; use crate::view::View; use crate::{backend, dag_walk, op_store, revset}; @@ -580,7 +579,7 @@ pub fn read_store_type_compat( #[derive(Debug, Error)] pub enum RepoLoaderError { #[error(transparent)] - TreeMerge(#[from] TreeMergeError), + Backend(#[from] BackendError), #[error(transparent)] OpHeadResolution(#[from] OpHeadResolutionError), #[error(transparent)] @@ -962,7 +961,7 @@ impl MutableRepo { &'repo mut self, settings: &'settings UserSettings, options: RebaseOptions, - ) -> Result>, TreeMergeError> { + ) -> BackendResult>> { if !self.has_rewrites() { // Optimization return Ok(None); @@ -980,7 +979,7 @@ impl MutableRepo { &mut self, settings: &UserSettings, options: RebaseOptions, - ) -> Result { + ) -> BackendResult { let result = self .rebase_descendants_return_rebaser(settings, options)? .map_or(0, |rebaser| rebaser.into_map().len()); @@ -1005,7 +1004,7 @@ impl MutableRepo { &mut self, settings: &UserSettings, options: RebaseOptions, - ) -> Result, TreeMergeError> { + ) -> BackendResult> { let result = Ok(self // We do not set RebaseOptions here, since this function does not currently return // enough information to describe the results of a rebase if some commits got @@ -1016,14 +1015,14 @@ impl MutableRepo { result } - pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result { + pub fn rebase_descendants(&mut self, settings: &UserSettings) -> BackendResult { self.rebase_descendants_with_options(settings, Default::default()) } pub fn rebase_descendants_return_map( &mut self, settings: &UserSettings, - ) -> Result, TreeMergeError> { + ) -> BackendResult> { self.rebase_descendants_with_options_return_map(settings, Default::default()) } diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 7dd63b5ea0..4483fdfdd4 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -35,13 +35,9 @@ use crate::repo_path::RepoPath; use crate::revset::{RevsetExpression, RevsetIteratorExt}; use crate::settings::UserSettings; use crate::store::Store; -use crate::tree::TreeMergeError; #[instrument(skip(repo))] -pub fn merge_commit_trees( - repo: &dyn Repo, - commits: &[Commit], -) -> Result { +pub fn merge_commit_trees(repo: &dyn Repo, commits: &[Commit]) -> BackendResult { merge_commit_trees_without_repo(repo.store(), repo.index(), commits) } @@ -50,7 +46,7 @@ pub fn merge_commit_trees_without_repo( store: &Arc, index: &dyn Index, commits: &[Commit], -) -> Result { +) -> BackendResult { if commits.is_empty() { Ok(store.get_root_tree(&store.empty_merged_tree_id())?) } else { @@ -104,7 +100,7 @@ pub fn rebase_commit( mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], -) -> Result { +) -> BackendResult { let rebased_commit = rebase_commit_with_options( settings, mut_repo, @@ -129,7 +125,7 @@ pub fn rebase_commit_with_options( old_commit: &Commit, new_parents: &[Commit], options: &RebaseOptions, -) -> Result { +) -> BackendResult { // If specified, don't create commit where one parent is an ancestor of another. let simplified_new_parents; let new_parents = if options.simplify_ancestor_merge { @@ -213,7 +209,7 @@ pub fn rebase_to_dest_parent( repo: &dyn Repo, source: &Commit, destination: &Commit, -) -> Result { +) -> BackendResult { if source.parent_ids() == destination.parent_ids() { Ok(source.tree()?) } else { @@ -230,7 +226,7 @@ pub fn back_out_commit( mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], -) -> Result { +) -> BackendResult { let old_base_tree = merge_commit_trees(mut_repo, &old_commit.parents())?; let new_base_tree = merge_commit_trees(mut_repo, new_parents)?; let old_tree = old_commit.tree()?; @@ -240,10 +236,10 @@ pub fn back_out_commit( .map(|commit| commit.id().clone()) .collect(); // TODO: i18n the description based on repo language - Ok(mut_repo + mut_repo .new_commit(settings, new_parent_ids, new_tree.id()) .set_description(format!("backout of commit {}", &old_commit.id().hex())) - .write()?) + .write() } #[derive(Clone, Default, PartialEq, Eq, Debug)] @@ -439,7 +435,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { Ok(()) } - fn rebase_one(&mut self, old_commit: Commit) -> Result<(), TreeMergeError> { + fn rebase_one(&mut self, old_commit: Commit) -> BackendResult<()> { let old_commit_id = old_commit.id().clone(); assert!(!self.mut_repo.parent_mapping.contains_key(&old_commit_id)); let old_parent_ids = old_commit.parent_ids(); @@ -528,7 +524,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.mut_repo.set_view(view); } - pub fn rebase_all(&mut self) -> Result<(), TreeMergeError> { + pub fn rebase_all(&mut self) -> BackendResult<()> { while let Some(old_commit) = self.to_visit.pop() { self.rebase_one(old_commit)?; } diff --git a/lib/src/tree.rs b/lib/src/tree.rs index 3b94dead5c..a7088c645f 100644 --- a/lib/src/tree.rs +++ b/lib/src/tree.rs @@ -20,11 +20,11 @@ use std::io::Read; use std::sync::Arc; use itertools::Itertools; -use thiserror::Error; use tracing::instrument; use crate::backend::{ - BackendError, ConflictId, TreeEntriesNonRecursiveIterator, TreeEntry, TreeId, TreeValue, + BackendError, BackendResult, ConflictId, TreeEntriesNonRecursiveIterator, TreeEntry, TreeId, + TreeValue, }; use crate::files::MergeResult; use crate::matchers::{EverythingMatcher, Matcher}; @@ -34,12 +34,6 @@ use crate::repo_path::{RepoPath, RepoPathBuf, RepoPathComponent, RepoPathCompone use crate::store::Store; use crate::{backend, files}; -#[derive(Debug, Error)] -pub enum TreeMergeError { - #[error(transparent)] - BackendError(#[from] BackendError), -} - #[derive(Clone)] pub struct Tree { store: Arc, @@ -281,11 +275,7 @@ impl<'trees> Iterator for TreeEntryDiffIterator<'trees> { } } -pub fn merge_trees( - side1_tree: &Tree, - base_tree: &Tree, - side2_tree: &Tree, -) -> Result { +pub fn merge_trees(side1_tree: &Tree, base_tree: &Tree, side2_tree: &Tree) -> BackendResult { let store = base_tree.store(); let dir = base_tree.dir(); assert_eq!(side1_tree.dir(), dir); @@ -313,7 +303,7 @@ pub fn merge_trees( new_tree.set_or_remove(basename, new_value); } } - Ok(store.write_tree(dir, new_tree)?) + store.write_tree(dir, new_tree) } /// Returns `Some(TreeId)` if this is a directory or missing. If it's missing, @@ -336,7 +326,7 @@ fn merge_tree_value( maybe_base: Option<&TreeValue>, maybe_side1: Option<&TreeValue>, maybe_side2: Option<&TreeValue>, -) -> Result, TreeMergeError> { +) -> BackendResult> { // Resolve non-trivial conflicts: // * resolve tree conflicts by recursing // * try to resolve file conflicts by merging the file contents @@ -399,7 +389,7 @@ pub fn try_resolve_file_conflict( store: &Store, filename: &RepoPath, conflict: &MergedTreeValue, -) -> Result, TreeMergeError> { +) -> BackendResult> { // If there are any non-file or any missing parts in the conflict, we can't // merge it. We check early so we don't waste time reading file contents if // we can't merge them anyway. At the same time we determine whether the @@ -438,7 +428,7 @@ pub fn try_resolve_file_conflict( let file_id_conflict = file_id_conflict.simplify(); let contents: Merge> = - file_id_conflict.try_map(|&file_id| -> Result, TreeMergeError> { + file_id_conflict.try_map(|&file_id| -> BackendResult> { let mut content = vec![]; store .read_file(filename, file_id)? From 98fefce449918b99dcd653cf0b7c8a96a31c8218 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 30 Mar 2024 16:42:36 +0900 Subject: [PATCH 3/4] cli: don't silently omit root parent by "jj new --insert-before" Spotted by Benjamin Tan. --- cli/src/commands/new.rs | 8 +------- cli/tests/test_new_command.rs | 22 +++------------------- 2 files changed, 4 insertions(+), 26 deletions(-) diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index de7d670f95..eec01cd4dd 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -129,17 +129,11 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", short_commit_hash(&commit_id), ))); } - let mut new_parents_commits: Vec = new_parents + let new_parents_commits: Vec = new_parents .evaluate_programmatic(tx.repo())? .iter() .commits(tx.repo().store()) .try_collect()?; - // The git backend does not support creating merge commits involving the root - // commit. - if new_parents_commits.len() > 1 { - let root_commit = tx.repo().store().root_commit(); - new_parents_commits.retain(|c| c != &root_commit); - } let merged_tree = merge_commit_trees(tx.repo(), &new_parents_commits)?; let new_parents_commit_id = new_parents_commits.iter().map(|c| c.id().clone()).collect(); new_commit = tx diff --git a/cli/tests/test_new_command.rs b/cli/tests/test_new_command.rs index 55462dc56a..18de961231 100644 --- a/cli/tests/test_new_command.rs +++ b/cli/tests/test_new_command.rs @@ -417,26 +417,10 @@ fn test_new_insert_before_no_root_merge() { ◉ root "###); - let (stdout, stderr) = - test_env.jj_cmd_ok(&repo_path, &["new", "--insert-before", "-m", "G", "B", "D"]); - insta::assert_snapshot!(stdout, @""); + let stderr = + test_env.jj_cmd_failure(&repo_path, &["new", "--insert-before", "-m", "G", "B", "D"]); insta::assert_snapshot!(stderr, @r###" - Rebased 4 descendant commits - Working copy now at: kxryzmor bf9fc493 (empty) G - Parent commit : qpvuntsm 65b1ef43 A | (empty) A - "###); - insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" - ◉ F - ├─╮ - │ ◉ E - ◉ │ D - │ │ ◉ C - │ │ ◉ B - ├───╯ - @ │ G - ◉ │ A - ├─╯ - ◉ root + Error: The Git backend does not support creating merge commits with the root commit as one of the parents. "###); } From 2ed029fe52f07033e1bde0bd1ba76b3f5ee52b87 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sat, 30 Mar 2024 16:48:09 +0900 Subject: [PATCH 4/4] cli: simplify check for non-empty revisions with/without "all:" If "all:" is specified, an empty set should be allowed within that expression. So the additional check we need here is to ensure that the resulting set is not empty. --- cli/src/cli_util.rs | 9 +++++---- cli/src/commands/new.rs | 4 ---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index be61f4ad1f..1c37c99fbb 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1658,9 +1658,6 @@ pub fn resolve_multiple_nonempty_revsets_default_single( let mut all_commits = IndexSet::new(); for revision_str in revisions { let commits = workspace_command.resolve_revset_default_single(revision_str)?; - if commits.is_empty() { - return Err(user_error("Empty revision set")); - } for commit in commits { let commit_hash = short_commit_hash(commit.id()); if !all_commits.insert(commit) { @@ -1670,7 +1667,11 @@ pub fn resolve_multiple_nonempty_revsets_default_single( } } } - Ok(all_commits) + if all_commits.is_empty() { + Err(user_error("Empty revision set")) + } else { + Ok(all_commits) + } } pub fn update_working_copy( diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index eec01cd4dd..1b10eca862 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -96,10 +96,6 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", )); } let mut workspace_command = command.workspace_helper(ui)?; - assert!( - !args.revisions.is_empty(), - "expected a non-empty list from clap" - ); let target_commits = resolve_multiple_nonempty_revsets_default_single(&workspace_command, &args.revisions)? .into_iter()