diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 16160f2bd1..210f4466be 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -2485,7 +2485,11 @@ impl WorkspaceCommandTransaction<'_> { /// commit. If the bookmark is conflicted before the update, it will /// remain conflicted after the update, but the conflict will involve /// the `move_to` commit instead of the old commit. - pub fn advance_bookmarks(&mut self, bookmarks: Vec, move_to: &CommitId) { + pub fn advance_bookmarks( + &mut self, + bookmarks: Vec, + move_to: &CommitId, + ) -> Result<(), CommandError> { for bookmark in bookmarks { // This removes the old commit ID from the bookmark's RefTarget and // replaces it with the `move_to` ID. @@ -2493,8 +2497,9 @@ impl WorkspaceCommandTransaction<'_> { &bookmark.name, &RefTarget::normal(bookmark.old_commit_id), &RefTarget::normal(move_to.clone()), - ); + )?; } + Ok(()) } } diff --git a/cli/src/command_error.rs b/cli/src/command_error.rs index eaa42630e2..cb3bbb43d7 100644 --- a/cli/src/command_error.rs +++ b/cli/src/command_error.rs @@ -35,6 +35,7 @@ use jj_lib::fileset::FilesetParseError; use jj_lib::fileset::FilesetParseErrorKind; use jj_lib::fix::FixError; use jj_lib::gitignore::GitIgnoreError; +use jj_lib::index::IndexError; use jj_lib::op_heads_store::OpHeadResolutionError; use jj_lib::op_heads_store::OpHeadsStoreError; use jj_lib::op_store::OpStoreError; @@ -328,6 +329,12 @@ impl From for CommandError { } } +impl From for CommandError { + fn from(err: IndexError) -> Self { + internal_error_with_message("Unexpected error from index", err) + } +} + impl From for CommandError { fn from(err: OpHeadsStoreError) -> Self { internal_error_with_message("Unexpected error from operation heads store", err) @@ -422,6 +429,7 @@ impl From for CommandError { fn from(err: WalkPredecessorsError) -> Self { match err { WalkPredecessorsError::Backend(err) => err.into(), + WalkPredecessorsError::Index(err) => err.into(), WalkPredecessorsError::OpStore(err) => err.into(), WalkPredecessorsError::CycleDetected(_) => internal_error(err), } @@ -544,6 +552,7 @@ jj currently does not support partial clones. To use jj with this repository, tr .to_string(), ), GitImportError::Backend(_) => None, + GitImportError::Index(_) => None, GitImportError::Git(_) => None, GitImportError::UnexpectedBackend(_) => None, }; diff --git a/cli/src/commands/bookmark/mod.rs b/cli/src/commands/bookmark/mod.rs index 0d66cdfeae..d07bf408d8 100644 --- a/cli/src/commands/bookmark/mod.rs +++ b/cli/src/commands/bookmark/mod.rs @@ -24,6 +24,7 @@ mod untrack; use itertools::Itertools as _; use jj_lib::backend::CommitId; +use jj_lib::index::any_is_ancestor; use jj_lib::op_store::RefTarget; use jj_lib::op_store::RemoteRef; use jj_lib::ref_name::RefName; @@ -181,15 +182,19 @@ fn find_trackable_remote_bookmarks<'a>( } } -fn is_fast_forward(repo: &dyn Repo, old_target: &RefTarget, new_target_id: &CommitId) -> bool { +fn is_fast_forward( + repo: &dyn Repo, + old_target: &RefTarget, + new_target_id: &CommitId, +) -> Result { if old_target.is_present() { // Strictly speaking, "all" old targets should be ancestors, but we allow // conflict resolution by setting bookmark to "any" of the old target // descendants. - old_target - .added_ids() - .any(|old| repo.index().is_ancestor(old, new_target_id)) + + any_is_ancestor(repo.index(), old_target.added_ids(), new_target_id) + .map_err(|err| err.into()) } else { - true + Ok(true) } } diff --git a/cli/src/commands/bookmark/move.rs b/cli/src/commands/bookmark/move.rs index 5b3eb1466d..c1c7ca9b36 100644 --- a/cli/src/commands/bookmark/move.rs +++ b/cli/src/commands/bookmark/move.rs @@ -138,7 +138,14 @@ pub fn cmd_bookmark_move( if !args.allow_backwards && let Some((name, _)) = matched_bookmarks .iter() - .find(|(_, old_target)| !is_fast_forward(repo.as_ref(), old_target, target_commit.id())) + .find_map(|(name, old_target)| { + match is_fast_forward(repo.as_ref(), old_target, target_commit.id()) { + Ok(true) => None, + Ok(false) => Some(Ok((name, old_target))), + Err(e) => Some(Err(e)), + } + }) + .transpose()? { return Err(user_error_with_hint( format!( diff --git a/cli/src/commands/bookmark/set.rs b/cli/src/commands/bookmark/set.rs index 0a602b5c35..f4eb8d14db 100644 --- a/cli/src/commands/bookmark/set.rs +++ b/cli/src/commands/bookmark/set.rs @@ -75,7 +75,7 @@ pub fn cmd_bookmark_set( } else if old_target.as_normal() != Some(target_commit.id()) { moved_bookmark_count += 1; } - if !args.allow_backwards && !is_fast_forward(repo, old_target, target_commit.id()) { + if !args.allow_backwards && !is_fast_forward(repo, old_target, target_commit.id())? { return Err(user_error_with_hint( format!( "Refusing to move bookmark backwards or sideways: {name}", diff --git a/cli/src/commands/bookmark/track.rs b/cli/src/commands/bookmark/track.rs index e475855c3d..c7aed13775 100644 --- a/cli/src/commands/bookmark/track.rs +++ b/cli/src/commands/bookmark/track.rs @@ -71,7 +71,7 @@ pub fn cmd_bookmark_track( } let mut tx = workspace_command.start_transaction(); for &symbol in &symbols { - tx.repo_mut().track_remote_bookmark(symbol); + tx.repo_mut().track_remote_bookmark(symbol)?; } if !symbols.is_empty() { writeln!( diff --git a/cli/src/commands/commit.rs b/cli/src/commands/commit.rs index 3dd1430aab..972c5dcfa6 100644 --- a/cli/src/commands/commit.rs +++ b/cli/src/commands/commit.rs @@ -208,7 +208,7 @@ new working-copy commit. .write()?; // Does nothing if there's no bookmarks to advance. - tx.advance_bookmarks(advanceable_bookmarks, new_commit.id()); + tx.advance_bookmarks(advanceable_bookmarks, new_commit.id())?; for name in workspace_names { tx.repo_mut().edit(name, &new_wc_commit).unwrap(); diff --git a/cli/src/commands/duplicate.rs b/cli/src/commands/duplicate.rs index 93376cda83..fffa8542db 100644 --- a/cli/src/commands/duplicate.rs +++ b/cli/src/commands/duplicate.rs @@ -146,7 +146,7 @@ pub(crate) fn cmd_duplicate( { for commit_id in &to_duplicate { for parent_commit_id in parent_commit_ids { - if tx.repo().index().is_ancestor(commit_id, parent_commit_id) { + if tx.repo().index().is_ancestor(commit_id, parent_commit_id)? { writeln!( ui.warning_default(), "Duplicating commit {} as a descendant of itself", @@ -159,7 +159,7 @@ pub(crate) fn cmd_duplicate( for commit_id in &to_duplicate { for child_commit_id in children_commit_ids { - if tx.repo().index().is_ancestor(child_commit_id, commit_id) { + if tx.repo().index().is_ancestor(child_commit_id, commit_id)? { writeln!( ui.warning_default(), "Duplicating commit {} as an ancestor of itself", diff --git a/cli/src/commands/git/clone.rs b/cli/src/commands/git/clone.rs index 71f039b22e..bf6d5c163b 100644 --- a/cli/src/commands/git/clone.rs +++ b/cli/src/commands/git/clone.rs @@ -365,7 +365,7 @@ fn fetch_new_remote( let remote_symbol = name.to_remote_symbol(remote_name); let remote_ref = tx.repo().get_remote_bookmark(remote_symbol); if remote_ref.is_present() { - tx.repo_mut().track_remote_bookmark(remote_symbol); + tx.repo_mut().track_remote_bookmark(remote_symbol)?; } } print_git_import_stats(ui, tx.repo(), &import_stats, true)?; diff --git a/cli/src/commands/git/push.rs b/cli/src/commands/git/push.rs index 98c8e38311..869fe1d377 100644 --- a/cli/src/commands/git/push.rs +++ b/cli/src/commands/git/push.rs @@ -31,6 +31,7 @@ use jj_lib::config::ConfigGetResultExt as _; use jj_lib::git; use jj_lib::git::GitBranchPushTargets; use jj_lib::git::GitPushStats; +use jj_lib::index::IndexResult; use jj_lib::op_store::RefTarget; use jj_lib::ref_name::RefName; use jj_lib::ref_name::RefNameBuf; @@ -646,15 +647,15 @@ fn print_commits_ready_to_push( formatter: &mut dyn Formatter, repo: &dyn Repo, bookmark_updates: &[(RefNameBuf, BookmarkPushUpdate)], -) -> io::Result<()> { - let to_direction = |old_target: &CommitId, new_target: &CommitId| { +) -> Result<(), CommandError> { + let to_direction = |old_target: &CommitId, new_target: &CommitId| -> IndexResult<_> { assert_ne!(old_target, new_target); - if repo.index().is_ancestor(old_target, new_target) { - BookmarkMoveDirection::Forward - } else if repo.index().is_ancestor(new_target, old_target) { - BookmarkMoveDirection::Backward + if repo.index().is_ancestor(old_target, new_target)? { + Ok(BookmarkMoveDirection::Forward) + } else if repo.index().is_ancestor(new_target, old_target)? { + Ok(BookmarkMoveDirection::Backward) } else { - BookmarkMoveDirection::Sideways + Ok(BookmarkMoveDirection::Sideways) } }; @@ -670,7 +671,7 @@ fn print_commits_ready_to_push( // among many was moved sideways (say). TODO: People on Discord // suggest "Move bookmark ... forward by n commits", // possibly "Move bookmark ... sideways (X forward, Y back)". - let msg = match to_direction(old_target, new_target) { + let msg = match to_direction(old_target, new_target)? { BookmarkMoveDirection::Forward => { format!("Move forward bookmark {bookmark_name} from {old} to {new}") } diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index c14d5b16ed..07e41b0936 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -236,7 +236,7 @@ pub(crate) fn cmd_new( // Does nothing if there's no bookmarks to advance. if let Some(target) = advance_bookmarks_target { - tx.advance_bookmarks(advanceable_bookmarks, &target); + tx.advance_bookmarks(advanceable_bookmarks, &target)?; } tx.finish(ui, "new empty commit")?; diff --git a/cli/src/commands/operation/diff.rs b/cli/src/commands/operation/diff.rs index c299add6ef..3f77e84ad4 100644 --- a/cli/src/commands/operation/diff.rs +++ b/cli/src/commands/operation/diff.rs @@ -117,7 +117,7 @@ pub fn cmd_op_diff( let mut tx = to_repo.start_transaction(); // Merge index from `from_repo` to `to_repo`, so commits in `from_repo` are // accessible. - tx.repo_mut().merge_index(&from_repo); + tx.repo_mut().merge_index(&from_repo)?; let merged_repo = tx.repo(); let diff_renderer = { diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 14f01b09a7..c43c92ec31 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -554,7 +554,7 @@ fn check_rebase_destinations( short_commit_hash(commit.id()), ))); } - if repo.index().is_ancestor(commit.id(), parent_id) { + if repo.index().is_ancestor(commit.id(), parent_id)? { return Err(user_error(format!( "Cannot rebase {} onto descendant {}", short_commit_hash(commit.id()), diff --git a/cli/src/commit_templater.rs b/cli/src/commit_templater.rs index 0d72702265..b1517b4862 100644 --- a/cli/src/commit_templater.rs +++ b/cli/src/commit_templater.rs @@ -46,6 +46,7 @@ use jj_lib::fileset::FilesetDiagnostics; use jj_lib::fileset::FilesetExpression; use jj_lib::id_prefix::IdPrefixContext; use jj_lib::id_prefix::IdPrefixIndex; +use jj_lib::index::IndexResult; use jj_lib::matchers::Matcher; use jj_lib::merge::Diff; use jj_lib::merge::MergedTreeValue; @@ -1147,10 +1148,11 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm |language, _diagnostics, _build_ctx, self_property, function| { function.expect_no_arguments()?; let repo = language.repo; - let out_property = self_property.map(|commit| { + let out_property = self_property.and_then(|commit| { // The given commit could be hidden in e.g. `jj evolog`. - let maybe_entries = repo.resolve_change_id(commit.change_id()); - maybe_entries.map_or(0, |entries| entries.len()) > 1 + let maybe_entries = repo.resolve_change_id(commit.change_id())?; + let divergent = maybe_entries.map_or(0, |entries| entries.len()) > 1; + Ok(divergent) }); Ok(out_property.into_dyn_wrapped()) }, @@ -1160,7 +1162,8 @@ fn builtin_commit_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, Comm |language, _diagnostics, _build_ctx, self_property, function| { function.expect_no_arguments()?; let repo = language.repo; - let out_property = self_property.map(|commit| commit.is_hidden(repo)); + let out_property = + self_property.and_then(|commit| commit.is_hidden(repo).map_err(|err| err.into())); Ok(out_property.into_dyn_wrapped()) }, ); @@ -1901,11 +1904,11 @@ fn builtin_repo_path_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, R } trait ShortestIdPrefixLen { - fn shortest_prefix_len(&self, repo: &dyn Repo, index: &IdPrefixIndex) -> usize; + fn shortest_prefix_len(&self, repo: &dyn Repo, index: &IdPrefixIndex) -> IndexResult; } impl ShortestIdPrefixLen for ChangeId { - fn shortest_prefix_len(&self, repo: &dyn Repo, index: &IdPrefixIndex) -> usize { + fn shortest_prefix_len(&self, repo: &dyn Repo, index: &IdPrefixIndex) -> IndexResult { index.shortest_change_prefix_len(repo, self) } } @@ -1933,7 +1936,7 @@ fn builtin_change_id_methods<'repo>() -> CommitTemplateBuildMethodFnMap<'repo, C } impl ShortestIdPrefixLen for CommitId { - fn shortest_prefix_len(&self, repo: &dyn Repo, index: &IdPrefixIndex) -> usize { + fn shortest_prefix_len(&self, repo: &dyn Repo, index: &IdPrefixIndex) -> IndexResult { index.shortest_commit_prefix_len(repo, self) } } @@ -2020,11 +2023,11 @@ where }; // The length of the id printed will be the maximum of the minimum // `len` and the length of the shortest unique prefix. - let out_property = (self_property, len_property).map(move |(id, len)| { - let prefix_len = id.shortest_prefix_len(repo, &index); + let out_property = (self_property, len_property).and_then(move |(id, len)| { + let prefix_len = id.shortest_prefix_len(repo, &index)?; let mut hex = format!("{id:.len$}", len = max(prefix_len, len.unwrap_or(0))); let rest = hex.split_off(prefix_len); - ShortestIdPrefix { prefix: hex, rest } + Ok(ShortestIdPrefix { prefix: hex, rest }) }); Ok(out_property.into_dyn_wrapped()) }, diff --git a/lib/src/commit.rs b/lib/src/commit.rs index b0bfbcf425..c25babff44 100644 --- a/lib/src/commit.rs +++ b/lib/src/commit.rs @@ -33,6 +33,7 @@ use crate::backend::ChangeId; use crate::backend::CommitId; use crate::backend::MergedTreeId; use crate::backend::Signature; +use crate::index::IndexResult; use crate::merged_tree::MergedTree; use crate::repo::Repo; use crate::rewrite::merge_commit_trees; @@ -176,9 +177,9 @@ impl Commit { } /// A commit is hidden if its commit id is not in the change id index. - pub fn is_hidden(&self, repo: &dyn Repo) -> bool { - let maybe_entries = repo.resolve_change_id(self.change_id()); - maybe_entries.is_none_or(|entries| !entries.contains(&self.id)) + pub fn is_hidden(&self, repo: &dyn Repo) -> IndexResult { + let maybe_entries = repo.resolve_change_id(self.change_id())?; + Ok(maybe_entries.is_none_or(|entries| !entries.contains(&self.id))) } /// A commit is discardable if it has no change from its parent, and an diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 38a6da2935..f6de426ab7 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -368,7 +368,13 @@ impl DetachedCommitBuilder { let predecessors = self.commit.predecessors.clone(); let commit = write_to_store(&self.store, self.commit, &self.sign_settings)?; // FIXME: Google's index.has_id() always returns true. - if mut_repo.is_backed_by_default_index() && mut_repo.index().has_id(commit.id()) { + if mut_repo.is_backed_by_default_index() + && mut_repo + .index() + .has_id(commit.id()) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into()))? + { // Recording existing commit as new would create cycle in // predecessors/parent mappings within the current transaction, and // in predecessors graph globally. diff --git a/lib/src/default_index/composite.rs b/lib/src/default_index/composite.rs index 5735cb70c6..9f3fbe9c46 100644 --- a/lib/src/default_index/composite.rs +++ b/lib/src/default_index/composite.rs @@ -41,10 +41,9 @@ use super::revset_engine; use crate::backend::ChangeId; use crate::backend::CommitId; use crate::hex_util; -use crate::index::AllHeadsForGcUnsupported; use crate::index::ChangeIdIndex; use crate::index::Index; -use crate::index::IndexError; +use crate::index::IndexResult; use crate::object_id::HexPrefix; use crate::object_id::ObjectId as _; use crate::object_id::PrefixResolution; @@ -569,44 +568,46 @@ impl AsCompositeIndex for CompositeIndex { // In revset engine, we need to convert &CompositeIndex to &dyn Index. impl Index for CompositeIndex { - fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> usize { - self.commits() - .shortest_unique_commit_id_prefix_len(commit_id) + fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> IndexResult { + Ok(self + .commits() + .shortest_unique_commit_id_prefix_len(commit_id)) } - fn resolve_commit_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { - self.commits().resolve_commit_id_prefix(prefix) + fn resolve_commit_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult> { + Ok(self.commits().resolve_commit_id_prefix(prefix)) } - fn has_id(&self, commit_id: &CommitId) -> bool { - self.commits().has_id(commit_id) + fn has_id(&self, commit_id: &CommitId) -> IndexResult { + Ok(self.commits().has_id(commit_id)) } - fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> bool { - self.commits().is_ancestor(ancestor_id, descendant_id) + fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> IndexResult { + Ok(self.commits().is_ancestor(ancestor_id, descendant_id)) } - fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> Vec { - self.commits().common_ancestors(set1, set2) + fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> IndexResult> { + Ok(self.commits().common_ancestors(set1, set2)) } - fn all_heads_for_gc( - &self, - ) -> Result + '_>, AllHeadsForGcUnsupported> { + fn all_heads_for_gc(&self) -> IndexResult + '_>> { Ok(Box::new(self.commits().all_heads())) } fn heads( &self, candidate_ids: &mut dyn Iterator, - ) -> Result, IndexError> { + ) -> IndexResult> { Ok(self.commits().heads(candidate_ids)) } fn changed_paths_in_commit( &self, commit_id: &CommitId, - ) -> Result + '_>>, IndexError> { + ) -> IndexResult + '_>>> { let Some(paths) = self .commits() .commit_id_to_pos(commit_id) @@ -653,9 +654,9 @@ impl ChangeIdIndex for ChangeIdIndexImpl { // If `SingleMatch` is returned, the commits including in the set are all // visible. `AmbiguousMatch` may be returned even if the prefix is unique // within the visible entries. - fn resolve_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { + fn resolve_prefix(&self, prefix: &HexPrefix) -> IndexResult>> { let index = self.index.as_composite().commits(); - match index.resolve_change_id_prefix(prefix) { + Ok(match index.resolve_change_id_prefix(prefix) { PrefixResolution::NoMatch => PrefixResolution::NoMatch, PrefixResolution::SingleMatch((_change_id, positions)) => { debug_assert!(positions.is_sorted_by(|a, b| a < b)); @@ -673,7 +674,7 @@ impl ChangeIdIndex for ChangeIdIndexImpl { } } PrefixResolution::AmbiguousMatch => PrefixResolution::AmbiguousMatch, - } + }) } // Calculates the shortest prefix length of the given `change_id` among all @@ -682,9 +683,9 @@ impl ChangeIdIndex for ChangeIdIndexImpl { // The returned length is usually a few digits longer than the minimum // length necessary to disambiguate within the visible entries since hidden // entries are also considered when determining the prefix length. - fn shortest_unique_prefix_len(&self, change_id: &ChangeId) -> usize { + fn shortest_unique_prefix_len(&self, change_id: &ChangeId) -> IndexResult { let index = self.index.as_composite().commits(); - index.shortest_unique_change_id_prefix_len(change_id) + Ok(index.shortest_unique_change_id_prefix_len(change_id)) } } diff --git a/lib/src/default_index/mod.rs b/lib/src/default_index/mod.rs index d10bbc33b9..6c0570dd95 100644 --- a/lib/src/default_index/mod.rs +++ b/lib/src/default_index/mod.rs @@ -102,6 +102,22 @@ mod tests { index.stats() } + fn is_ancestor( + index: &DefaultMutableIndex, + ancestor_id: &CommitId, + descendant_id: &CommitId, + ) -> bool { + index.is_ancestor(ancestor_id, descendant_id).unwrap() + } + + fn common_ancestors( + index: &DefaultMutableIndex, + set1: &[CommitId], + set2: &[CommitId], + ) -> Vec { + index.common_ancestors(set1, set2).unwrap() + } + #[test_case(false; "memory")] #[test_case(true; "file")] fn index_empty(on_disk: bool) { @@ -1034,17 +1050,17 @@ mod tests { index.add_commit_data(id_4.clone(), new_change_id(), &[id_1.clone()]); index.add_commit_data(id_5.clone(), new_change_id(), &[id_4.clone(), id_2.clone()]); - assert!(index.is_ancestor(&id_0, &id_0)); - assert!(index.is_ancestor(&id_0, &id_1)); - assert!(index.is_ancestor(&id_2, &id_3)); - assert!(index.is_ancestor(&id_2, &id_5)); - assert!(index.is_ancestor(&id_1, &id_5)); - assert!(index.is_ancestor(&id_0, &id_5)); - assert!(!index.is_ancestor(&id_1, &id_0)); - assert!(!index.is_ancestor(&id_5, &id_3)); - assert!(!index.is_ancestor(&id_3, &id_5)); - assert!(!index.is_ancestor(&id_2, &id_4)); - assert!(!index.is_ancestor(&id_4, &id_2)); + assert!(is_ancestor(&index, &id_0, &id_0)); + assert!(is_ancestor(&index, &id_0, &id_1)); + assert!(is_ancestor(&index, &id_2, &id_3)); + assert!(is_ancestor(&index, &id_2, &id_5)); + assert!(is_ancestor(&index, &id_1, &id_5)); + assert!(is_ancestor(&index, &id_0, &id_5)); + assert!(!is_ancestor(&index, &id_1, &id_0)); + assert!(!is_ancestor(&index, &id_5, &id_3)); + assert!(!is_ancestor(&index, &id_3, &id_5)); + assert!(!is_ancestor(&index, &id_2, &id_4)); + assert!(!is_ancestor(&index, &id_4, &id_2)); } #[test] @@ -1075,70 +1091,70 @@ mod tests { index.add_commit_data(id_6.clone(), new_change_id(), &[id_4.clone()]); assert_eq!( - index.common_ancestors(&[id_0.clone()], &[id_0.clone()]), + common_ancestors(&index, &[id_0.clone()], &[id_0.clone()]), vec![id_0.clone()] ); assert_eq!( - index.common_ancestors(&[id_5.clone()], &[id_5.clone()]), + common_ancestors(&index, &[id_5.clone()], &[id_5.clone()]), vec![id_5.clone()] ); assert_eq!( - index.common_ancestors(&[id_1.clone()], &[id_2.clone()]), + common_ancestors(&index, &[id_1.clone()], &[id_2.clone()]), vec![id_0.clone()] ); assert_eq!( - index.common_ancestors(&[id_2.clone()], &[id_1.clone()]), + common_ancestors(&index, &[id_2.clone()], &[id_1.clone()]), vec![id_0.clone()] ); assert_eq!( - index.common_ancestors(&[id_1.clone()], &[id_4.clone()]), + common_ancestors(&index, &[id_1.clone()], &[id_4.clone()]), vec![id_1.clone()] ); assert_eq!( - index.common_ancestors(&[id_4.clone()], &[id_1.clone()]), + common_ancestors(&index, &[id_4.clone()], &[id_1.clone()]), vec![id_1.clone()] ); assert_eq!( - index.common_ancestors(&[id_3.clone()], &[id_5.clone()]), + common_ancestors(&index, &[id_3.clone()], &[id_5.clone()]), vec![id_0.clone()] ); assert_eq!( - index.common_ancestors(&[id_5.clone()], &[id_3.clone()]), + common_ancestors(&index, &[id_5.clone()], &[id_3.clone()]), vec![id_0.clone()] ); assert_eq!( - index.common_ancestors(&[id_2.clone()], &[id_6.clone()]), + common_ancestors(&index, &[id_2.clone()], &[id_6.clone()]), vec![id_0.clone()] ); // With multiple commits in an input set assert_eq!( - index.common_ancestors(&[id_0.clone(), id_1.clone()], &[id_0.clone()]), + common_ancestors(&index, &[id_0.clone(), id_1.clone()], &[id_0.clone()]), vec![id_0.clone()] ); assert_eq!( - index.common_ancestors(&[id_0.clone(), id_1.clone()], &[id_1.clone()]), + common_ancestors(&index, &[id_0.clone(), id_1.clone()], &[id_1.clone()]), vec![id_1.clone()] ); assert_eq!( - index.common_ancestors(&[id_1.clone(), id_2.clone()], &[id_1.clone()]), + common_ancestors(&index, &[id_1.clone(), id_2.clone()], &[id_1.clone()]), vec![id_1.clone()] ); assert_eq!( - index.common_ancestors(&[id_1.clone(), id_2.clone()], &[id_4]), + common_ancestors(&index, &[id_1.clone(), id_2.clone()], &[id_4]), vec![id_1.clone()] ); assert_eq!( - index.common_ancestors(&[id_5.clone(), id_6.clone()], &[id_2.clone()]), + common_ancestors(&index, &[id_5.clone(), id_6.clone()], &[id_2.clone()]), &[id_2.clone()] ); // Both 1 and 2 are returned since (5) expands to (2, 4), which expands // to (1,2) and matches the (1,2) of the first input set. assert_eq!( - index.common_ancestors(&[id_1.clone(), id_2.clone()], &[id_5]), + common_ancestors(&index, &[id_1.clone(), id_2.clone()], &[id_5]), vec![id_2.clone(), id_1.clone()] ); - assert_eq!(index.common_ancestors(&[id_1, id_2], &[id_3]), vec![id_0]); + assert_eq!(common_ancestors(&index, &[id_1, id_2], &[id_3]), vec![id_0]); } #[test] @@ -1161,9 +1177,9 @@ mod tests { index.add_commit_data(id_3.clone(), new_change_id(), &[id_1.clone(), id_2.clone()]); index.add_commit_data(id_4.clone(), new_change_id(), &[id_1.clone(), id_2.clone()]); - let mut common_ancestors = index.common_ancestors(&[id_3], &[id_4]); - common_ancestors.sort(); - assert_eq!(common_ancestors, vec![id_1, id_2]); + let mut common_ancestor_ids = common_ancestors(&index, &[id_3], &[id_4]); + common_ancestor_ids.sort(); + assert_eq!(common_ancestor_ids, vec![id_1, id_2]); } #[test] @@ -1188,9 +1204,9 @@ mod tests { index.add_commit_data(id_4.clone(), new_change_id(), &[id_0.clone(), id_2.clone()]); index.add_commit_data(id_5.clone(), new_change_id(), &[id_0, id_2.clone()]); - let mut common_ancestors = index.common_ancestors(&[id_4], &[id_5]); - common_ancestors.sort(); - assert_eq!(common_ancestors, vec![id_2]); + let mut common_ancestor_ids = common_ancestors(&index, &[id_4], &[id_5]); + common_ancestor_ids.sort(); + assert_eq!(common_ancestor_ids, vec![id_2]); } #[test] diff --git a/lib/src/default_index/mutable.rs b/lib/src/default_index/mutable.rs index 1809bb675e..31ce41a138 100644 --- a/lib/src/default_index/mutable.rs +++ b/lib/src/default_index/mutable.rs @@ -56,10 +56,10 @@ use crate::commit::Commit; use crate::file_util::IoResultExt as _; use crate::file_util::PathError; use crate::file_util::persist_content_addressed_temp_file; -use crate::index::AllHeadsForGcUnsupported; use crate::index::ChangeIdIndex; use crate::index::Index; use crate::index::IndexError; +use crate::index::IndexResult; use crate::index::MutableIndex; use crate::index::ReadonlyIndex; use crate::object_id::HexPrefix; @@ -545,43 +545,41 @@ impl AsCompositeIndex for DefaultMutableIndex { } impl Index for DefaultMutableIndex { - fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> usize { + fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> IndexResult { self.0.shortest_unique_commit_id_prefix_len(commit_id) } - fn resolve_commit_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + fn resolve_commit_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult> { self.0.resolve_commit_id_prefix(prefix) } - fn has_id(&self, commit_id: &CommitId) -> bool { + fn has_id(&self, commit_id: &CommitId) -> IndexResult { self.0.has_id(commit_id) } - fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> bool { + fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> IndexResult { self.0.is_ancestor(ancestor_id, descendant_id) } - fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> Vec { + fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> IndexResult> { self.0.common_ancestors(set1, set2) } - fn all_heads_for_gc( - &self, - ) -> Result + '_>, AllHeadsForGcUnsupported> { + fn all_heads_for_gc(&self) -> IndexResult + '_>> { self.0.all_heads_for_gc() } - fn heads( - &self, - candidates: &mut dyn Iterator, - ) -> Result, IndexError> { + fn heads(&self, candidates: &mut dyn Iterator) -> IndexResult> { self.0.heads(candidates) } fn changed_paths_in_commit( &self, commit_id: &CommitId, - ) -> Result + '_>>, IndexError> { + ) -> IndexResult + '_>>> { self.0.changed_paths_in_commit(commit_id) } @@ -602,21 +600,22 @@ impl MutableIndex for DefaultMutableIndex { fn change_id_index( &self, heads: &mut dyn Iterator, - ) -> Box { - Box::new(ChangeIdIndexImpl::new(self, heads)) + ) -> IndexResult> { + Ok(Box::new(ChangeIdIndexImpl::new(self, heads))) } - fn add_commit(&mut self, commit: &Commit) -> Result<(), IndexError> { + fn add_commit(&mut self, commit: &Commit) -> IndexResult<()> { Self::add_commit(self, commit) .block_on() - .map_err(|err| IndexError(err.into())) + .map_err(|err| IndexError::Other(err.into())) } - fn merge_in(&mut self, other: &dyn ReadonlyIndex) { + fn merge_in(&mut self, other: &dyn ReadonlyIndex) -> IndexResult<()> { let other: &DefaultReadonlyIndex = other .downcast_ref() .expect("index to merge in must be a DefaultReadonlyIndex"); Self::merge_in(self, other); + Ok(()) } } diff --git a/lib/src/default_index/readonly.rs b/lib/src/default_index/readonly.rs index f3c38d27ac..67fe944342 100644 --- a/lib/src/default_index/readonly.rs +++ b/lib/src/default_index/readonly.rs @@ -47,10 +47,9 @@ use super::revset_engine::RevsetImpl; use crate::backend::ChangeId; use crate::backend::CommitId; use crate::graph::GraphNode; -use crate::index::AllHeadsForGcUnsupported; use crate::index::ChangeIdIndex; use crate::index::Index; -use crate::index::IndexError; +use crate::index::IndexResult; use crate::index::MutableIndex; use crate::index::ReadonlyIndex; use crate::object_id::HexPrefix; @@ -713,43 +712,41 @@ impl AsCompositeIndex for DefaultReadonlyIndex { } impl Index for DefaultReadonlyIndex { - fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> usize { + fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> IndexResult { self.0.shortest_unique_commit_id_prefix_len(commit_id) } - fn resolve_commit_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution { + fn resolve_commit_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult> { self.0.resolve_commit_id_prefix(prefix) } - fn has_id(&self, commit_id: &CommitId) -> bool { + fn has_id(&self, commit_id: &CommitId) -> IndexResult { self.0.has_id(commit_id) } - fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> bool { + fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> IndexResult { self.0.is_ancestor(ancestor_id, descendant_id) } - fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> Vec { + fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> IndexResult> { self.0.common_ancestors(set1, set2) } - fn all_heads_for_gc( - &self, - ) -> Result + '_>, AllHeadsForGcUnsupported> { + fn all_heads_for_gc(&self) -> IndexResult + '_>> { self.0.all_heads_for_gc() } - fn heads( - &self, - candidates: &mut dyn Iterator, - ) -> Result, IndexError> { + fn heads(&self, candidates: &mut dyn Iterator) -> IndexResult> { self.0.heads(candidates) } fn changed_paths_in_commit( &self, commit_id: &CommitId, - ) -> Result + '_>>, IndexError> { + ) -> IndexResult + '_>>> { self.0.changed_paths_in_commit(commit_id) } @@ -770,8 +767,8 @@ impl ReadonlyIndex for DefaultReadonlyIndex { fn change_id_index( &self, heads: &mut dyn Iterator, - ) -> Box { - Box::new(ChangeIdIndexImpl::new(self.clone(), heads)) + ) -> IndexResult> { + Ok(Box::new(ChangeIdIndexImpl::new(self.clone(), heads))) } fn start_modification(&self) -> Box { diff --git a/lib/src/default_index/store.rs b/lib/src/default_index/store.rs index 8077aa4a72..a33cae1503 100644 --- a/lib/src/default_index/store.rs +++ b/lib/src/default_index/store.rs @@ -51,9 +51,11 @@ use crate::file_util::IoResultExt as _; use crate::file_util::PathError; use crate::file_util::persist_temp_file; use crate::index::Index as _; -use crate::index::IndexReadError; +use crate::index::IndexError; +use crate::index::IndexResult; use crate::index::IndexStore; -use crate::index::IndexWriteError; +use crate::index::IndexStoreError; +use crate::index::IndexStoreResult; use crate::index::MutableIndex; use crate::index::ReadonlyIndex; use crate::object_id::ObjectId as _; @@ -95,6 +97,8 @@ pub enum DefaultIndexStoreError { op_id: OperationId, source: BackendError, }, + #[error("Failed to access the index")] + AccessIndex(#[source] IndexError), #[error(transparent)] OpStore(#[from] OpStoreError), } @@ -316,10 +320,11 @@ impl DefaultIndexStore { ); // Build a list of ancestors of heads where parents come after the // commit itself. - let parent_index_has_id = |id: &CommitId| { - maybe_parent_index - .as_ref() - .is_some_and(|index| index.has_id(id)) + let parent_index_has_id = |id: &CommitId| -> IndexResult { + match maybe_parent_index.as_ref() { + Some(parent_index) => parent_index.has_id(id), + None => Ok(false), + } }; let get_commit_with_op = |commit_id: &CommitId, op_id: &OperationId| { let op_id = op_id.clone(); @@ -342,7 +347,10 @@ impl DefaultIndexStore { let mut ancestors = HashSet::new(); let mut work = historical_heads.keys().cloned().collect_vec(); while let Some(commit_id) = work.pop() { - if ancestors.contains(&commit_id) || parent_index_has_id(&commit_id) { + if ancestors.contains(&commit_id) + || parent_index_has_id(&commit_id) + .map_err(DefaultIndexStoreError::AccessIndex)? + { continue; } if let Ok(commit) = store.get_commit(&commit_id) { @@ -354,11 +362,23 @@ impl DefaultIndexStore { } else { HashSet::new() }; + let get_commit_with_op_if_parent_missing = + |(commit_id, op_id): (&CommitId, &OperationId)| -> Option> { + parent_index_has_id(commit_id) + .map_err(DefaultIndexStoreError::AccessIndex) + .and_then(|parent_has_id| { + if parent_has_id { + Ok(None) + } else { + get_commit_with_op(commit_id, op_id).map(Some) + } + }) + .transpose() + }; let commits = dag_walk::topo_order_reverse_ord_ok( historical_heads .iter() - .filter(|&(commit_id, _)| !parent_index_has_id(commit_id)) - .map(|(commit_id, op_id)| get_commit_with_op(commit_id, op_id)), + .filter_map(get_commit_with_op_if_parent_missing), |(CommitByCommitterTimestamp(commit), _)| commit.id().clone(), |(CommitByCommitterTimestamp(commit), op_id)| { let keep_predecessors = @@ -370,8 +390,8 @@ impl DefaultIndexStore { .into_iter() .flatten(), ) - .filter(|&id| !parent_index_has_id(id)) - .map(|commit_id| get_commit_with_op(commit_id, op_id)) + .map(|commit_id| (commit_id, op_id)) + .filter_map(get_commit_with_op_if_parent_missing) .collect_vec() }, |_| panic!("graph has cycle"), @@ -562,7 +582,7 @@ impl IndexStore for DefaultIndexStore { &self, op: &Operation, store: &Arc, - ) -> Result, IndexReadError> { + ) -> IndexStoreResult> { let field_lengths = FieldLengths { commit_id: store.commit_id_length(), change_id: store.change_id_length(), @@ -591,12 +611,13 @@ impl IndexStore for DefaultIndexStore { eprintln!("{err} (maybe the format has changed): {error}. Reindexing..."); } } - self.reinit().map_err(|err| IndexReadError(err.into()))?; + self.reinit() + .map_err(|err| IndexStoreError::Read(err.into()))?; self.build_index_at_operation(op, store).block_on() } result => result, } - .map_err(|err| IndexReadError(err.into()))?; + .map_err(|err| IndexStoreError::Read(err.into()))?; Ok(Box::new(index)) } @@ -604,13 +625,13 @@ impl IndexStore for DefaultIndexStore { &self, index: Box, op: &Operation, - ) -> Result, IndexWriteError> { + ) -> IndexStoreResult> { let index: Box = index .downcast() .expect("index to merge in must be a DefaultMutableIndex"); let index = self .save_mutable_index(*index, op.id()) - .map_err(|err| IndexWriteError(err.into()))?; + .map_err(|err| IndexStoreError::Write(err.into()))?; Ok(Box::new(index)) } } diff --git a/lib/src/evolution.rs b/lib/src/evolution.rs index dde6ca5fe0..f489481d95 100644 --- a/lib/src/evolution.rs +++ b/lib/src/evolution.rs @@ -17,6 +17,7 @@ use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::VecDeque; +use std::collections::hash_map::Entry; use std::slice; use itertools::Itertools as _; @@ -27,6 +28,7 @@ use crate::backend::BackendResult; use crate::backend::CommitId; use crate::commit::Commit; use crate::dag_walk; +use crate::index::IndexError; use crate::op_store::OpStoreError; use crate::op_store::OpStoreResult; use crate::op_walk; @@ -70,6 +72,8 @@ pub enum WalkPredecessorsError { #[error(transparent)] Backend(#[from] BackendError), #[error(transparent)] + Index(#[from] IndexError), + #[error(transparent)] OpStore(#[from] OpStoreError), #[error("Predecessors cycle detected around commit {0}")] CycleDetected(CommitId), @@ -168,27 +172,46 @@ where } /// Traverses predecessors from remainder commits. - fn scan_commits(&mut self) -> BackendResult<()> { + fn scan_commits(&mut self) -> Result<(), WalkPredecessorsError> { let store = self.repo.store(); let index = self.repo.index(); let mut commit_predecessors: HashMap> = HashMap::new(); let commits = dag_walk::topo_order_reverse_ok( - self.to_visit.drain(..).map(|id| store.get_commit(&id)), + self.to_visit.drain(..).map(|id| { + store + .get_commit(&id) + .map_err(WalkPredecessorsError::Backend) + }), |commit: &Commit| commit.id().clone(), |commit: &Commit| { - let ids = commit_predecessors - .entry(commit.id().clone()) - .or_insert_with(|| { - commit - .store_commit() - .predecessors - .iter() - // exclude unreachable predecessors - .filter(|id| index.has_id(id)) - .cloned() - .collect() - }); - ids.iter().map(|id| store.get_commit(id)).collect_vec() + let ids = match commit_predecessors.entry(commit.id().clone()) { + Entry::Occupied(e) => Ok(e.into_mut()), + Entry::Vacant(e) => commit + .store_commit() + .predecessors + .iter() + .filter_map(|id| { + index + .has_id(id) + .map_err(WalkPredecessorsError::Index) + .map( + |is_reachable| { + if is_reachable { Some(id.clone()) } else { None } + }, + ) + .transpose() + }) + .try_collect() + .map(|filtered| e.insert(filtered)), + }; + + match ids { + Ok(ids) => ids + .iter() + .map(|id| store.get_commit(id).map_err(WalkPredecessorsError::Backend)) + .collect_vec(), + Err(err) => vec![Err(err)], + } }, |_| panic!("graph has cycle"), )?; diff --git a/lib/src/git.rs b/lib/src/git.rs index a41d2b7195..1f66a47dd1 100644 --- a/lib/src/git.rs +++ b/lib/src/git.rs @@ -42,6 +42,7 @@ use crate::file_util::PathError; use crate::git_backend::GitBackend; use crate::git_subprocess::GitSubprocessContext; use crate::git_subprocess::GitSubprocessError; +use crate::index::IndexError; use crate::matchers::EverythingMatcher; use crate::merged_tree::MergedTree; use crate::merged_tree::TreeDiffEntry; @@ -382,7 +383,9 @@ pub enum GitImportError { err: BackendError, }, #[error(transparent)] - Backend(BackendError), + Backend(#[from] BackendError), + #[error(transparent)] + Index(#[from] IndexError), #[error(transparent)] Git(Box), #[error(transparent)] @@ -466,29 +469,36 @@ pub fn import_some_refs( // changed_git_refs, but such targets should have already been imported to // the backend. let index = mut_repo.index(); - let missing_head_ids = changed_git_refs + let missing_head_ids: Vec<&CommitId> = changed_git_refs .iter() .flat_map(|(_, new_target)| new_target.added_ids()) - .filter(|&id| !index.has_id(id)); + .map(|id| Ok::<_, IndexError>((id, index.has_id(id)?))) + .try_collect::<_, Vec<_>, _>()? + .into_iter() + .filter_map(|(id, has_id)| (!has_id).then_some(id)) + .collect(); let heads_imported = git_backend.import_head_commits(missing_head_ids).is_ok(); // Import new remote heads let mut head_commits = Vec::new(); - let get_commit = |id| { + let get_commit = |id: &CommitId, symbol: &RemoteRefSymbolBuf| { + let missing_ref_err = |err| GitImportError::MissingRefAncestor { + symbol: symbol.clone(), + err, + }; // If bulk-import failed, try again to find bad head or ref. - if !heads_imported && !index.has_id(id) { - git_backend.import_head_commits([id])?; + if !heads_imported && !index.has_id(id).map_err(GitImportError::Index)? { + git_backend + .import_head_commits([id]) + .map_err(missing_ref_err)?; } - store.get_commit(id) + store.get_commit(id).map_err(missing_ref_err) }; for (symbol, (_, new_target)) in itertools::chain(&changed_remote_bookmarks, &changed_remote_tags) { for id in new_target.added_ids() { - let commit = get_commit(id).map_err(|err| GitImportError::MissingRefAncestor { - symbol: symbol.clone(), - err, - })?; + let commit = get_commit(id, symbol)?; head_commits.push(commit); } } @@ -521,7 +531,7 @@ pub fn import_some_refs( }, }; if new_remote_ref.is_tracked() { - mut_repo.merge_local_bookmark(symbol.name, base_target, &new_remote_ref.target); + mut_repo.merge_local_bookmark(symbol.name, base_target, &new_remote_ref.target)?; } // Remote-tracking branch is the last known state of the branch in the remote. // It shouldn't diverge even if we had inconsistent view. @@ -539,7 +549,7 @@ pub fn import_some_refs( }, }; if new_remote_ref.is_tracked() { - mut_repo.merge_local_tag(symbol.name, base_target, &new_remote_ref.target); + mut_repo.merge_local_tag(symbol.name, base_target, &new_remote_ref.target)?; } // Remote-tracking tag is the last known state of the tag in the remote. // It shouldn't diverge even if we had inconsistent view. @@ -807,7 +817,7 @@ pub fn import_head(mut_repo: &mut MutableRepo) -> Result<(), GitImportError> { // Import new head if let Some(head_id) = &new_git_head_id { let index = mut_repo.index(); - if !index.has_id(head_id) { + if !index.has_id(head_id)? { git_backend.import_head_commits([head_id]).map_err(|err| { GitImportError::MissingHeadTarget { id: head_id.clone(), diff --git a/lib/src/id_prefix.rs b/lib/src/id_prefix.rs index ddc670d096..23a47e3256 100644 --- a/lib/src/id_prefix.rs +++ b/lib/src/id_prefix.rs @@ -25,6 +25,7 @@ use thiserror::Error; use crate::backend::ChangeId; use crate::backend::CommitId; use crate::hex_util; +use crate::index::IndexResult; use crate::object_id::HexPrefix; use crate::object_id::ObjectId; use crate::object_id::PrefixResolution; @@ -159,7 +160,7 @@ impl IdPrefixIndex<'_> { &self, repo: &dyn Repo, prefix: &HexPrefix, - ) -> PrefixResolution { + ) -> IndexResult> { if let Some(indexes) = self.indexes { let resolution = indexes .commit_index @@ -171,14 +172,14 @@ impl IdPrefixIndex<'_> { PrefixResolution::SingleMatch(id) => { // The disambiguation set may be loaded from a different repo, // and contain a commit that doesn't exist in the current repo. - if repo.index().has_id(&id) { - return PrefixResolution::SingleMatch(id); + if repo.index().has_id(&id)? { + return Ok(PrefixResolution::SingleMatch(id)); } else { - return PrefixResolution::NoMatch; + return Ok(PrefixResolution::NoMatch); } } PrefixResolution::AmbiguousMatch => { - return PrefixResolution::AmbiguousMatch; + return Ok(PrefixResolution::AmbiguousMatch); } } } @@ -187,18 +188,30 @@ impl IdPrefixIndex<'_> { /// Returns the shortest length of a prefix of `commit_id` that can still be /// resolved by `resolve_commit_prefix()` and [`SymbolResolver`]. - pub fn shortest_commit_prefix_len(&self, repo: &dyn Repo, commit_id: &CommitId) -> usize { - let len = self.shortest_commit_prefix_len_exact(repo, commit_id); - disambiguate_prefix_with_refs(repo.view(), &commit_id.to_string(), len) + pub fn shortest_commit_prefix_len( + &self, + repo: &dyn Repo, + commit_id: &CommitId, + ) -> IndexResult { + let len = self.shortest_commit_prefix_len_exact(repo, commit_id)?; + Ok(disambiguate_prefix_with_refs( + repo.view(), + &commit_id.to_string(), + len, + )) } - pub fn shortest_commit_prefix_len_exact(&self, repo: &dyn Repo, commit_id: &CommitId) -> usize { + pub fn shortest_commit_prefix_len_exact( + &self, + repo: &dyn Repo, + commit_id: &CommitId, + ) -> IndexResult { if let Some(indexes) = self.indexes && let Some(lookup) = indexes .commit_index .lookup_exact(&*indexes.commit_change_ids, commit_id) { - return lookup.shortest_unique_prefix_len(); + return Ok(lookup.shortest_unique_prefix_len()); } repo.index().shortest_unique_commit_id_prefix_len(commit_id) } @@ -208,7 +221,7 @@ impl IdPrefixIndex<'_> { &self, repo: &dyn Repo, prefix: &HexPrefix, - ) -> PrefixResolution> { + ) -> IndexResult>> { if let Some(indexes) = self.indexes { let resolution = indexes .change_index @@ -218,15 +231,15 @@ impl IdPrefixIndex<'_> { // Fall back to resolving in entire repo } PrefixResolution::SingleMatch(change_id) => { - return match repo.resolve_change_id(&change_id) { + return Ok(match repo.resolve_change_id(&change_id)? { // There may be more commits with this change id outside the narrower sets. Some(commit_ids) => PrefixResolution::SingleMatch(commit_ids), // The disambiguation set may contain hidden commits. None => PrefixResolution::NoMatch, - }; + }); } PrefixResolution::AmbiguousMatch => { - return PrefixResolution::AmbiguousMatch; + return Ok(PrefixResolution::AmbiguousMatch); } } } @@ -235,18 +248,30 @@ impl IdPrefixIndex<'_> { /// Returns the shortest length of a prefix of `change_id` that can still be /// resolved by `resolve_change_prefix()` and [`SymbolResolver`]. - pub fn shortest_change_prefix_len(&self, repo: &dyn Repo, change_id: &ChangeId) -> usize { - let len = self.shortest_change_prefix_len_exact(repo, change_id); - disambiguate_prefix_with_refs(repo.view(), &change_id.to_string(), len) + pub fn shortest_change_prefix_len( + &self, + repo: &dyn Repo, + change_id: &ChangeId, + ) -> IndexResult { + let len = self.shortest_change_prefix_len_exact(repo, change_id)?; + Ok(disambiguate_prefix_with_refs( + repo.view(), + &change_id.to_string(), + len, + )) } - fn shortest_change_prefix_len_exact(&self, repo: &dyn Repo, change_id: &ChangeId) -> usize { + fn shortest_change_prefix_len_exact( + &self, + repo: &dyn Repo, + change_id: &ChangeId, + ) -> IndexResult { if let Some(indexes) = self.indexes && let Some(lookup) = indexes .change_index .lookup_exact(&*indexes.commit_change_ids, change_id) { - return lookup.shortest_unique_prefix_len(); + return Ok(lookup.shortest_unique_prefix_len()); } repo.shortest_unique_change_id_prefix_len(change_id) } diff --git a/lib/src/index.rs b/lib/src/index.rs index 043b9b20cc..10a359ca6b 100644 --- a/lib/src/index.rs +++ b/lib/src/index.rs @@ -32,26 +32,34 @@ use crate::revset::Revset; use crate::revset::RevsetEvaluationError; use crate::store::Store; -/// Returned if an error occurs while reading an index from the [`IndexStore`]. +/// Returned by [`IndexStore`] in the event of an error. #[derive(Debug, Error)] -#[error(transparent)] -pub struct IndexReadError(pub Box); +pub enum IndexStoreError { + /// Error reading a [`ReadonlyIndex`] from the [`IndexStore`]. + #[error("Failed to read index")] + Read(#[source] Box), + /// Error writing a [`MutableIndex`] to the [`IndexStore`]. + #[error("Failed to write index")] + Write(#[source] Box), +} -/// Returned if an error occurs while writing an index to the [`IndexStore`]. -#[derive(Debug, Error)] -#[error(transparent)] -pub struct IndexWriteError(pub Box); +/// Result of [`IndexStore`] operations. +pub type IndexStoreResult = Result; -/// Returned by [`Index`] backend in case of an error. +/// Returned by [`Index`] backend in the event of an error. #[derive(Debug, Error)] -#[error(transparent)] -pub struct IndexError(pub Box); +pub enum IndexError { + /// Error returned if [`Index::all_heads_for_gc()`] is not supported by the + /// [`Index`] backend. + #[error("Cannot collect all heads by index of this type")] + AllHeadsForGcUnsupported, + /// Some other index error. + #[error(transparent)] + Other(Box), +} -/// An error returned if `Index::all_heads_for_gc()` is not supported by the -/// index backend. -#[derive(Debug, Error)] -#[error("Cannot collect all heads by index of this type")] -pub struct AllHeadsForGcUnsupported; +/// Result of [`Index`] operations. +pub type IndexResult = Result; /// Defines the interface for types that provide persistent storage for an /// index. @@ -66,7 +74,7 @@ pub trait IndexStore: Any + Send + Sync + Debug { &self, op: &Operation, store: &Arc, - ) -> Result, IndexReadError>; + ) -> IndexStoreResult>; /// Writes `index` to the index store and returns a read-only version of the /// index. @@ -74,7 +82,7 @@ pub trait IndexStore: Any + Send + Sync + Debug { &self, index: Box, op: &Operation, - ) -> Result, IndexWriteError>; + ) -> IndexStoreResult>; } impl dyn IndexStore { @@ -93,24 +101,27 @@ pub trait Index: Send + Sync { /// /// If the given `commit_id` doesn't exist, returns the minimum prefix /// length which matches none of the commits in the index. - fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> usize; + fn shortest_unique_commit_id_prefix_len(&self, commit_id: &CommitId) -> IndexResult; /// Searches the index for commit IDs matching `prefix`. Returns a /// [`PrefixResolution`] with a [`CommitId`] if the prefix matches a single /// commit. - fn resolve_commit_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution; + fn resolve_commit_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult>; /// Returns true if `commit_id` is present in the index. - fn has_id(&self, commit_id: &CommitId) -> bool; + fn has_id(&self, commit_id: &CommitId) -> IndexResult; /// Returns true if `ancestor_id` commit is an ancestor of the /// `descendant_id` commit, or if `ancestor_id` equals `descendant_id`. - fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> bool; + fn is_ancestor(&self, ancestor_id: &CommitId, descendant_id: &CommitId) -> IndexResult; /// Returns the best common ancestor or ancestors of the commits in `set1` /// and `set2`. A "best common ancestor" has no descendants that are also /// common ancestors. - fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> Vec; + fn common_ancestors(&self, set1: &[CommitId], set2: &[CommitId]) -> IndexResult>; /// Heads among all indexed commits at the associated operation. /// @@ -119,24 +130,19 @@ pub trait Index: Send + Sync { /// that should be preserved on garbage collection. /// /// The iteration order is unspecified. - fn all_heads_for_gc( - &self, - ) -> Result + '_>, AllHeadsForGcUnsupported>; + fn all_heads_for_gc(&self) -> IndexResult + '_>>; /// Returns the subset of commit IDs in `candidates` which are not ancestors /// of other commits in `candidates`. If a commit id is duplicated in the /// `candidates` list it will appear at most once in the output. - fn heads( - &self, - candidates: &mut dyn Iterator, - ) -> Result, IndexError>; + fn heads(&self, candidates: &mut dyn Iterator) -> IndexResult>; /// Returns iterator over paths changed at the specified commit. The paths /// are sorted. Returns `None` if the commit wasn't indexed. fn changed_paths_in_commit( &self, commit_id: &CommitId, - ) -> Result + '_>>, IndexError>; + ) -> IndexResult + '_>>>; /// Resolves the revset `expression` against the index and corresponding /// `store`. @@ -151,8 +157,10 @@ pub trait Index: Send + Sync { pub trait ReadonlyIndex: Any + Send + Sync { fn as_index(&self) -> &dyn Index; - fn change_id_index(&self, heads: &mut dyn Iterator) - -> Box; + fn change_id_index( + &self, + heads: &mut dyn Iterator, + ) -> IndexResult>; fn start_modification(&self) -> Box; } @@ -171,11 +179,11 @@ pub trait MutableIndex: Any { fn change_id_index( &self, heads: &mut dyn Iterator, - ) -> Box; + ) -> IndexResult>; - fn add_commit(&mut self, commit: &Commit) -> Result<(), IndexError>; + fn add_commit(&mut self, commit: &Commit) -> IndexResult<()>; - fn merge_in(&mut self, other: &dyn ReadonlyIndex); + fn merge_in(&mut self, other: &dyn ReadonlyIndex) -> IndexResult<()>; } impl dyn MutableIndex { @@ -196,7 +204,7 @@ pub trait ChangeIdIndex: Send + Sync { /// Resolve an unambiguous change ID prefix to the commit IDs in the index. /// /// The order of the returned commit IDs is unspecified. - fn resolve_prefix(&self, prefix: &HexPrefix) -> PrefixResolution>; + fn resolve_prefix(&self, prefix: &HexPrefix) -> IndexResult>>; /// This function returns the shortest length of a prefix of `key` that /// disambiguates it from every other key in the index. @@ -212,5 +220,26 @@ pub trait ChangeIdIndex: Send + Sync { /// order to disambiguate, you need every letter of the key *and* the /// additional fact that it's the entire key). This case is extremely /// unlikely for hashes with 12+ hexadecimal characters. - fn shortest_unique_prefix_len(&self, change_id: &ChangeId) -> usize; + fn shortest_unique_prefix_len(&self, change_id: &ChangeId) -> IndexResult; +} + +/// Helper function to determine if any of the candidate ancestor IDs are an +/// ancestor of descendant_id. +pub fn any_is_ancestor<'a>( + index: &dyn Index, + ancestor_candidates: impl IntoIterator, + descendant_id: &CommitId, +) -> IndexResult { + let found = ancestor_candidates + .into_iter() + .find_map( + |ancestor_id| match index.is_ancestor(ancestor_id, descendant_id) { + Ok(true) => Some(Ok(true)), + Ok(false) => None, + Err(e) => Some(Err(e)), + }, + ) + .transpose()? + .unwrap_or(false); + Ok(found) } diff --git a/lib/src/refs.rs b/lib/src/refs.rs index d69184c277..c322485792 100644 --- a/lib/src/refs.rs +++ b/lib/src/refs.rs @@ -18,6 +18,7 @@ use itertools::EitherOrBoth; use crate::backend::CommitId; use crate::index::Index; +use crate::index::IndexResult; use crate::merge::Merge; use crate::merge::SameChange; use crate::merge::trivial_merge; @@ -107,9 +108,9 @@ pub fn merge_ref_targets( left: &RefTarget, base: &RefTarget, right: &RefTarget, -) -> RefTarget { +) -> IndexResult { if let Some(&resolved) = trivial_merge(&[left, base, right], SameChange::Accept) { - return resolved.clone(); + return Ok(resolved.clone()); } let mut merge = Merge::from_vec(vec![ @@ -122,12 +123,12 @@ pub fn merge_ref_targets( // Suppose left = [A - C + B], base = [B], right = [A], the merge result is // [A - C + A], which can now be trivially resolved. if let Some(resolved) = merge.resolve_trivial(SameChange::Accept) { - RefTarget::resolved(resolved.clone()) + Ok(RefTarget::resolved(resolved.clone())) } else { - merge_ref_targets_non_trivial(index, &mut merge); + merge_ref_targets_non_trivial(index, &mut merge)?; // TODO: Maybe better to try resolve_trivial() again, but the result is // unreliable since merge_ref_targets_non_trivial() is order dependent. - RefTarget::from_merge(merge) + Ok(RefTarget::from_merge(merge)) } } @@ -136,31 +137,35 @@ pub fn merge_remote_refs( left: &RemoteRef, base: &RemoteRef, right: &RemoteRef, -) -> RemoteRef { +) -> IndexResult { // Just merge target and state fields separately. Strictly speaking, merging // target-only change and state-only change shouldn't automatically mark the // new target as tracking. However, many faulty merges will end up in local // or remote target conflicts (since fast-forwardable move can be safely // "tracked"), and the conflicts will require user intervention anyway. So // there wouldn't be much reason to handle these merges precisely. - let target = merge_ref_targets(index, &left.target, &base.target, &right.target); + let target = merge_ref_targets(index, &left.target, &base.target, &right.target)?; // Merged state shouldn't conflict atm since we only have two states, but if // it does, keep the original state. The choice is arbitrary. let state = *trivial_merge(&[left.state, base.state, right.state], SameChange::Accept) .unwrap_or(&base.state); - RemoteRef { target, state } + Ok(RemoteRef { target, state }) } -fn merge_ref_targets_non_trivial(index: &dyn Index, conflict: &mut Merge>) { - while let Some((remove_index, add_index)) = find_pair_to_remove(index, conflict) { +fn merge_ref_targets_non_trivial( + index: &dyn Index, + conflict: &mut Merge>, +) -> IndexResult<()> { + while let Some((remove_index, add_index)) = find_pair_to_remove(index, conflict)? { conflict.swap_remove(remove_index, add_index); } + Ok(()) } fn find_pair_to_remove( index: &dyn Index, conflict: &Merge>, -) -> Option<(usize, usize)> { +) -> IndexResult> { // If a "remove" is an ancestor of two different "adds" and one of the // "adds" is an ancestor of the other, then pick the descendant. for (add_index1, add1) in conflict.adds().enumerate() { @@ -169,20 +174,30 @@ fn find_pair_to_remove( // combination should be somehow weighted? let (add_index, add_id) = match (add1, add2) { (Some(id1), Some(id2)) if id1 == id2 => (add_index1, id1), - (Some(id1), Some(id2)) if index.is_ancestor(id1, id2) => (add_index1, id1), - (Some(id1), Some(id2)) if index.is_ancestor(id2, id1) => (add_index2, id2), + (Some(id1), Some(id2)) if index.is_ancestor(id1, id2)? => (add_index1, id1), + (Some(id1), Some(id2)) if index.is_ancestor(id2, id1)? => (add_index2, id2), _ => continue, }; - if let Some(remove_index) = conflict.removes().position(|remove| match remove { - Some(id) => index.is_ancestor(id, add_id), - None => true, // Absent ref can be considered a root - }) { - return Some((remove_index, add_index)); + + if let Some(remove_index) = conflict + .removes() + .enumerate() + .find_map(|(i, remove)| match remove { + Some(id) => match index.is_ancestor(id, add_id) { + Ok(true) => Some(Ok(i)), + Ok(false) => None, + Err(e) => Some(Err(e)), + }, + None => Some(Ok(i)), // Absent ref can be considered a root + }) + .transpose()? + { + return Ok(Some((remove_index, add_index))); } } } - None + Ok(None) } /// Pair of local and remote targets. diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 83e1964a3e..58902e7e30 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -52,8 +52,10 @@ use crate::file_util::IoResultExt as _; use crate::file_util::PathError; use crate::index::ChangeIdIndex; use crate::index::Index; -use crate::index::IndexReadError; +use crate::index::IndexError; +use crate::index::IndexResult; use crate::index::IndexStore; +use crate::index::IndexStoreError; use crate::index::MutableIndex; use crate::index::ReadonlyIndex; use crate::merge::MergeBuilder; @@ -125,19 +127,25 @@ pub trait Repo { fn submodule_store(&self) -> &Arc; - fn resolve_change_id(&self, change_id: &ChangeId) -> Option> { + fn resolve_change_id(&self, change_id: &ChangeId) -> IndexResult>> { // Replace this if we added more efficient lookup method. let prefix = HexPrefix::from_id(change_id); - match self.resolve_change_id_prefix(&prefix) { - PrefixResolution::NoMatch => None, - PrefixResolution::SingleMatch(entries) => Some(entries), + match self.resolve_change_id_prefix(&prefix)? { + PrefixResolution::NoMatch => Ok(None), + PrefixResolution::SingleMatch(entries) => Ok(Some(entries)), PrefixResolution::AmbiguousMatch => panic!("complete change_id should be unambiguous"), } } - fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution>; + fn resolve_change_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult>>; - fn shortest_unique_change_id_prefix_len(&self, target_id_bytes: &ChangeId) -> usize; + fn shortest_unique_change_id_prefix_len( + &self, + target_id_bytes: &ChangeId, + ) -> IndexResult; } pub struct ReadonlyRepo { @@ -290,13 +298,13 @@ impl ReadonlyRepo { self.index.as_ref() } - fn change_id_index(&self) -> &dyn ChangeIdIndex { + fn change_id_index(&self) -> IndexResult<&dyn ChangeIdIndex> { self.change_id_index - .get_or_init(|| { + .get_or_try_init(|| { self.readonly_index() .change_id_index(&mut self.view().heads().iter()) }) - .as_ref() + .map(|index| index.as_ref()) } pub fn op_heads_store(&self) -> &Arc { @@ -351,12 +359,16 @@ impl Repo for ReadonlyRepo { self.loader.submodule_store() } - fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { - self.change_id_index().resolve_prefix(prefix) + fn resolve_change_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult>> { + self.change_id_index()?.resolve_prefix(prefix) } - fn shortest_unique_change_id_prefix_len(&self, target_id: &ChangeId) -> usize { - self.change_id_index().shortest_unique_prefix_len(target_id) + fn shortest_unique_change_id_prefix_len(&self, target_id: &ChangeId) -> IndexResult { + self.change_id_index()? + .shortest_unique_prefix_len(target_id) } } @@ -630,7 +642,9 @@ pub enum RepoLoaderError { #[error(transparent)] Backend(#[from] BackendError), #[error(transparent)] - IndexRead(#[from] IndexReadError), + Index(#[from] IndexError), + #[error(transparent)] + IndexStore(#[from] IndexStoreError), #[error(transparent)] OpHeadResolution(#[from] OpHeadResolutionError), #[error(transparent)] @@ -1116,7 +1130,7 @@ impl MutableRepo { fn update_all_references(&mut self, options: &RewriteRefsOptions) -> BackendResult<()> { let rewrite_mapping = self.resolve_rewrite_mapping_with(|_| true); - self.update_local_bookmarks(&rewrite_mapping, options); + self.update_local_bookmarks(&rewrite_mapping, options)?; self.update_wc_commits(&rewrite_mapping)?; Ok(()) } @@ -1125,7 +1139,7 @@ impl MutableRepo { &mut self, rewrite_mapping: &HashMap>, options: &RewriteRefsOptions, - ) { + ) -> BackendResult<()> { let changed_branches = self .view() .local_bookmarks() @@ -1151,8 +1165,11 @@ impl MutableRepo { RefTarget::from_merge(MergeBuilder::from_iter(ids).build()) }; - self.merge_local_bookmark(&bookmark_name, &old_target, &new_target); + self.merge_local_bookmark(&bookmark_name, &old_target, &new_target) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into()))?; } + Ok(()) } fn update_wc_commits( @@ -1616,9 +1633,23 @@ impl MutableRepo { commit .parent_ids() .iter() - .filter(|id| !self.index().has_id(id)) - .map(|id| self.store().get_commit(id)) - .map_ok(CommitByCommitterTimestamp) + .filter_map(|id| { + self.index() + .has_id(id) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into())) + .and_then(|has_id| { + if has_id { + Ok(None) + } else { + self.store() + .get_commit(id) + .map(CommitByCommitterTimestamp) + .map(Some) + } + }) + .transpose() + }) .collect_vec() }, |_| panic!("graph has cycle"), @@ -1661,12 +1692,13 @@ impl MutableRepo { name: &RefName, base_target: &RefTarget, other_target: &RefTarget, - ) { + ) -> IndexResult<()> { let view = self.view.get_mut(); let index = self.index.as_index(); let self_target = view.get_local_bookmark(name); - let new_target = merge_ref_targets(index, self_target, base_target, other_target); + let new_target = merge_ref_targets(index, self_target, base_target, other_target)?; self.set_local_bookmark_target(name, new_target); + Ok(()) } pub fn get_remote_bookmark(&self, symbol: RemoteRefSymbol<'_>) -> RemoteRef { @@ -1683,22 +1715,24 @@ impl MutableRepo { symbol: RemoteRefSymbol<'_>, base_ref: &RemoteRef, other_ref: &RemoteRef, - ) { + ) -> IndexResult<()> { let view = self.view.get_mut(); let index = self.index.as_index(); let self_ref = view.get_remote_bookmark(symbol); - let new_ref = merge_remote_refs(index, self_ref, base_ref, other_ref); + let new_ref = merge_remote_refs(index, self_ref, base_ref, other_ref)?; view.set_remote_bookmark(symbol, new_ref); + Ok(()) } /// Merges the specified remote bookmark in to local bookmark, and starts /// tracking it. - pub fn track_remote_bookmark(&mut self, symbol: RemoteRefSymbol<'_>) { + pub fn track_remote_bookmark(&mut self, symbol: RemoteRefSymbol<'_>) -> IndexResult<()> { let mut remote_ref = self.get_remote_bookmark(symbol); let base_target = remote_ref.tracked_target(); - self.merge_local_bookmark(symbol.name, base_target, &remote_ref.target); + self.merge_local_bookmark(symbol.name, base_target, &remote_ref.target)?; remote_ref.state = RemoteRefState::Tracked; self.set_remote_bookmark(symbol, remote_ref); + Ok(()) } /// Stops tracking the specified remote bookmark. @@ -1733,12 +1767,13 @@ impl MutableRepo { name: &RefName, base_target: &RefTarget, other_target: &RefTarget, - ) { + ) -> IndexResult<()> { let view = self.view.get_mut(); let index = self.index.as_index(); let self_target = view.get_local_tag(name); - let new_target = merge_ref_targets(index, self_target, base_target, other_target); + let new_target = merge_ref_targets(index, self_target, base_target, other_target)?; view.set_local_tag_target(name, new_target); + Ok(()) } pub fn get_remote_tag(&self, symbol: RemoteRefSymbol<'_>) -> RemoteRef { @@ -1754,12 +1789,13 @@ impl MutableRepo { symbol: RemoteRefSymbol<'_>, base_ref: &RemoteRef, other_ref: &RemoteRef, - ) { + ) -> IndexResult<()> { let view = self.view.get_mut(); let index = self.index.as_index(); let self_ref = view.get_remote_tag(symbol); - let new_ref = merge_remote_refs(index, self_ref, base_ref, other_ref); + let new_ref = merge_remote_refs(index, self_ref, base_ref, other_ref)?; view.set_remote_tag(symbol, new_ref); + Ok(()) } pub fn get_git_ref(&self, name: &GitRefName) -> RefTarget { @@ -1775,12 +1811,13 @@ impl MutableRepo { name: &GitRefName, base_target: &RefTarget, other_target: &RefTarget, - ) { + ) -> IndexResult<()> { let view = self.view.get_mut(); let index = self.index.as_index(); let self_target = view.get_git_ref(name); - let new_target = merge_ref_targets(index, self_target, base_target, other_target); + let new_target = merge_ref_targets(index, self_target, base_target, other_target)?; view.set_git_ref_target(name, new_target); + Ok(()) } pub fn git_head(&self) -> RefTarget { @@ -1800,13 +1837,13 @@ impl MutableRepo { &mut self, base_repo: &ReadonlyRepo, other_repo: &ReadonlyRepo, - ) -> BackendResult<()> { + ) -> Result<(), RepoLoaderError> { // First, merge the index, so we can take advantage of a valid index when // merging the view. Merging in base_repo's index isn't typically // necessary, but it can be if base_repo is ahead of either self or other_repo // (e.g. because we're undoing an operation that hasn't been published). - self.index.merge_in(base_repo.readonly_index()); - self.index.merge_in(other_repo.readonly_index()); + self.index.merge_in(base_repo.readonly_index())?; + self.index.merge_in(other_repo.readonly_index())?; self.view.ensure_clean(|v| self.enforce_view_invariants(v)); self.merge_view(&base_repo.view, &other_repo.view)?; @@ -1814,11 +1851,11 @@ impl MutableRepo { Ok(()) } - pub fn merge_index(&mut self, other_repo: &ReadonlyRepo) { - self.index.merge_in(other_repo.readonly_index()); + pub fn merge_index(&mut self, other_repo: &ReadonlyRepo) -> IndexResult<()> { + self.index.merge_in(other_repo.readonly_index()) } - fn merge_view(&mut self, base: &View, other: &View) -> BackendResult<()> { + fn merge_view(&mut self, base: &View, other: &View) -> Result<(), RepoLoaderError> { let changed_wc_commits = diff_named_commit_ids(base.wc_commit_ids(), other.wc_commit_ids()); for (name, (base_id, other_id)) in changed_wc_commits { self.merge_wc_commit(name, base_id, other_id); @@ -1851,29 +1888,29 @@ impl MutableRepo { let changed_local_bookmarks = diff_named_ref_targets(base.local_bookmarks(), other.local_bookmarks()); for (name, (base_target, other_target)) in changed_local_bookmarks { - self.merge_local_bookmark(name, base_target, other_target); + self.merge_local_bookmark(name, base_target, other_target)?; } let changed_local_tags = diff_named_ref_targets(base.local_tags(), other.local_tags()); for (name, (base_target, other_target)) in changed_local_tags { - self.merge_local_tag(name, base_target, other_target); + self.merge_local_tag(name, base_target, other_target)?; } let changed_git_refs = diff_named_ref_targets(base.git_refs(), other.git_refs()); for (name, (base_target, other_target)) in changed_git_refs { - self.merge_git_ref(name, base_target, other_target); + self.merge_git_ref(name, base_target, other_target)?; } let changed_remote_bookmarks = diff_named_remote_refs(base.all_remote_bookmarks(), other.all_remote_bookmarks()); for (symbol, (base_ref, other_ref)) in changed_remote_bookmarks { - self.merge_remote_bookmark(symbol, base_ref, other_ref); + self.merge_remote_bookmark(symbol, base_ref, other_ref)?; } let changed_remote_tags = diff_named_remote_refs(base.all_remote_tags(), other.all_remote_tags()); for (symbol, (base_ref, other_ref)) in changed_remote_tags { - self.merge_remote_tag(symbol, base_ref, other_ref); + self.merge_remote_tag(symbol, base_ref, other_ref)?; } let new_git_head_target = merge_ref_targets( @@ -1881,7 +1918,7 @@ impl MutableRepo { self.view().git_head(), base.git_head(), other.git_head(), - ); + )?; self.set_git_head_target(new_git_head_target); Ok(()) @@ -1976,13 +2013,20 @@ impl Repo for MutableRepo { self.base_repo.submodule_store() } - fn resolve_change_id_prefix(&self, prefix: &HexPrefix) -> PrefixResolution> { - let change_id_index = self.index.change_id_index(&mut self.view().heads().iter()); + fn resolve_change_id_prefix( + &self, + prefix: &HexPrefix, + ) -> IndexResult>> { + let change_id_index = self + .index + .change_id_index(&mut self.view().heads().iter())?; change_id_index.resolve_prefix(prefix) } - fn shortest_unique_change_id_prefix_len(&self, target_id: &ChangeId) -> usize { - let change_id_index = self.index.change_id_index(&mut self.view().heads().iter()); + fn shortest_unique_change_id_prefix_len(&self, target_id: &ChangeId) -> IndexResult { + let change_id_index = self + .index + .change_id_index(&mut self.view().heads().iter())?; change_id_index.shortest_unique_prefix_len(target_id) } } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 08fdac08be..2edab1520f 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -2480,7 +2480,8 @@ fn reload_repo_at_operation( .map_err(|err| RevsetResolutionError::Other(err.into()))?; base_repo.reload_at(&operation).map_err(|err| match err { RepoLoaderError::Backend(err) => RevsetResolutionError::Backend(err), - RepoLoaderError::IndexRead(_) + RepoLoaderError::Index(_) + | RepoLoaderError::IndexStore(_) | RepoLoaderError::OpHeadResolution(_) | RepoLoaderError::OpHeadsStoreError(_) | RepoLoaderError::OpStore(_) @@ -2619,7 +2620,10 @@ impl CommitPrefixResolver<'_> { .transpose() .map_err(|err| RevsetResolutionError::Other(err.into()))? .unwrap_or(IdPrefixIndex::empty()); - match index.resolve_commit_prefix(repo, prefix) { + match index + .resolve_commit_prefix(repo, prefix) + .map_err(|err| RevsetResolutionError::Other(err.into()))? + { PrefixResolution::AmbiguousMatch => { Err(RevsetResolutionError::AmbiguousCommitIdPrefix(prefix.hex())) } @@ -2660,7 +2664,10 @@ impl ChangePrefixResolver<'_> { .transpose() .map_err(|err| RevsetResolutionError::Other(err.into()))? .unwrap_or(IdPrefixIndex::empty()); - match index.resolve_change_prefix(repo, prefix) { + match index + .resolve_change_prefix(repo, prefix) + .map_err(|err| RevsetResolutionError::Other(err.into()))? + { PrefixResolution::AmbiguousMatch => Err( RevsetResolutionError::AmbiguousChangeIdPrefix(prefix.reverse_hex()), ), diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 25f9929fe4..4bf33bcfe9 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -22,6 +22,7 @@ use std::sync::Arc; use futures::StreamExt as _; use futures::future::try_join_all; use futures::try_join; +use index::any_is_ancestor; use indexmap::IndexMap; use indexmap::IndexSet; use itertools::Itertools as _; @@ -35,8 +36,9 @@ use crate::backend::MergedTreeId; use crate::commit::Commit; use crate::commit::CommitIteratorExt as _; use crate::commit_builder::CommitBuilder; +use crate::index; use crate::index::Index; -use crate::index::IndexError; +use crate::index::IndexResult; use crate::matchers::Matcher; use crate::matchers::Visit; use crate::merge::Merge; @@ -98,8 +100,12 @@ pub fn find_recursive_merge_commits( } else { let mut result = Merge::resolved(commit_ids[0].clone()); for (i, other_commit_id) in commit_ids.iter().enumerate().skip(1) { - let ancestor_ids = index.common_ancestors(&commit_ids[0..i], &commit_ids[i..][..1]); + let ancestor_ids = index + .common_ancestors(&commit_ids[0..i], &commit_ids[i..][..1]) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into()))?; let ancestor_merge = find_recursive_merge_commits(store, index, ancestor_ids)?; + result = Merge::from_vec(vec![ result, ancestor_merge, @@ -218,7 +224,7 @@ impl<'repo> CommitRewriter<'repo> { /// If a merge commit would end up with one parent being an ancestor of the /// other, then filter out the ancestor. - pub fn simplify_ancestor_merge(&mut self) -> Result<(), IndexError> { + pub fn simplify_ancestor_merge(&mut self) -> IndexResult<()> { let head_set: HashSet<_> = self .mut_repo .index() @@ -332,10 +338,9 @@ pub fn rebase_commit_with_options( ) -> BackendResult { // If specified, don't create commit where one parent is an ancestor of another. if options.simplify_ancestor_merge { - // TODO: BackendError is not the right error here because - // the error does not come from `Backend`, but `Index`. rewriter .simplify_ancestor_merge() + // TODO: indexing error shouldn't be a "BackendError" .map_err(|err| BackendError::Other(err.into()))?; } @@ -725,65 +730,71 @@ pub fn compute_move_commits( let descendants = repo.find_descendants_for_rebase(roots.clone())?; let commit_new_parents_map = descendants .iter() - .map(|commit| { + .map(|commit| -> Result<_, BackendError> { let commit_id = commit.id(); let new_parent_ids = - // New child of the rebased target commits. - if let Some(new_child_parents) = new_children_parents.get(commit_id) { - new_child_parents.clone() - } - // Commit is in the target set. - else if target_commit_ids.contains(commit_id) { - // If the commit is a root of the target set, it should be rebased onto the new destination. - if target_roots.contains(commit_id) { - new_parent_ids.clone() + // New child of the rebased target commits. + if let Some(new_child_parents) = new_children_parents.get(commit_id) { + new_child_parents.clone() } - // Otherwise: - // 1. Keep parents which are within the target set. - // 2. Replace parents which are outside the target set but are part of the - // connected target set with their ancestor commits which are in the target - // set. - // 3. Keep other parents outside the target set if they are not descendants of the - // new children of the target set. - else { - let mut new_parents = vec![]; - for parent_id in commit.parent_ids() { - if target_commit_ids.contains(parent_id) { - new_parents.push(parent_id.clone()); - } else if let Some(parents) = + // Commit is in the target set. + else if target_commit_ids.contains(commit_id) { + // If the commit is a root of the target set, it should be rebased onto the new destination. + if target_roots.contains(commit_id) { + new_parent_ids.clone() + } + // Otherwise: + // 1. Keep parents which are within the target set. + // 2. Replace parents which are outside the target set but are part of the + // connected target set with their ancestor commits which are in the target + // set. + // 3. Keep other parents outside the target set if they are not descendants of the + // new children of the target set. + else { + let mut new_parents = vec![]; + for parent_id in commit.parent_ids() { + if target_commit_ids.contains(parent_id) { + new_parents.push(parent_id.clone()); + } else if let Some(parents) = connected_target_commits_internal_parents.get(parent_id) { - new_parents.extend(parents.iter().cloned()); - } else if !new_children.iter().any(|new_child| { - repo.index().is_ancestor(new_child.id(), parent_id) }) { - new_parents.push(parent_id.clone()); + new_parents.extend(parents.iter().cloned()); + } else if !any_is_ancestor( + repo.index(), + new_children.iter().map(|child| child.id()), + parent_id, + ) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into()))? + { + new_parents.push(parent_id.clone()); + } } + new_parents } - new_parents } - } - // Commits outside the target set should have references to commits inside the set - // replaced. - else if commit - .parent_ids() - .iter() - .any(|id| target_commits_external_parents.contains_key(id)) - { - let mut new_parents = vec![]; - for parent in commit.parent_ids() { - if let Some(parents) = target_commits_external_parents.get(parent) { - new_parents.extend(parents.iter().cloned()); - } else { - new_parents.push(parent.clone()); + // Commits outside the target set should have references to commits inside the set + // replaced. + else if commit + .parent_ids() + .iter() + .any(|id| target_commits_external_parents.contains_key(id)) + { + let mut new_parents = vec![]; + for parent in commit.parent_ids() { + if let Some(parents) = target_commits_external_parents.get(parent) { + new_parents.extend(parents.iter().cloned()); + } else { + new_parents.push(parent.clone()); + } } - } - new_parents - } else { - commit.parent_ids().iter().cloned().collect_vec() - }; - (commit.id().clone(), new_parent_ids) + new_parents + } else { + commit.parent_ids().iter().cloned().collect_vec() + }; + Ok((commit.id().clone(), new_parent_ids)) }) - .collect(); + .try_collect()?; Ok(ComputedMoveCommits { target_commit_ids, @@ -1203,10 +1214,14 @@ pub fn squash_commits<'repo>( } let mut rewritten_destination = destination.clone(); - if sources.iter().any(|source| { - repo.index() - .is_ancestor(source.commit.id(), destination.id()) - }) { + if any_is_ancestor( + repo.index(), + sources.iter().map(|source| source.commit.id()), + destination.id(), + ) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into()))? + { // If we're moving changes to a descendant, first rebase descendants onto the // rewritten sources. Otherwise it will likely already have the content // changes we're moving, so applying them will have no effect and the @@ -1277,13 +1292,17 @@ pub fn find_duplicate_divergent_commits( // commits with the same change ID which are not being rebased. let divergent_changes: Vec<(&Commit, Vec)> = target_commits .iter() - .map(|target_commit| { + .map(|target_commit| -> Result<_, BackendError> { let mut ancestor_candidates = repo .resolve_change_id(target_commit.change_id()) + // TODO: indexing error shouldn't be a "BackendError" + .map_err(|err| BackendError::Other(err.into()))? .unwrap_or_default(); ancestor_candidates.retain(|commit_id| !target_commit_ids.contains(commit_id)); - (target_commit, ancestor_candidates) + Ok((target_commit, ancestor_candidates)) }) + .try_collect::<_, Vec<_>, _>()? + .into_iter() .filter(|(_, candidates)| !candidates.is_empty()) .collect(); if divergent_changes.is_empty() { diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 2346e7e603..65c4bf1185 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -22,7 +22,7 @@ use thiserror::Error; use crate::backend::Timestamp; use crate::dag_walk; -use crate::index::IndexWriteError; +use crate::index::IndexStoreError; use crate::index::ReadonlyIndex; use crate::op_heads_store::OpHeadsStore; use crate::op_heads_store::OpHeadsStoreError; @@ -43,7 +43,7 @@ use crate::view::View; #[derive(Debug, Error)] #[error("Failed to commit new operation")] pub enum TransactionCommitError { - IndexWrite(#[from] IndexWriteError), + IndexStore(#[from] IndexStoreError), OpHeadsStore(#[from] OpHeadsStoreError), OpStore(#[from] OpStoreError), } diff --git a/lib/tests/test_git.rs b/lib/tests/test_git.rs index 9889db6a3a..45306a49f2 100644 --- a/lib/tests/test_git.rs +++ b/lib/tests/test_git.rs @@ -4562,11 +4562,11 @@ fn test_rewrite_imported_commit() { // The index should be consistent with the store. assert_eq!( - repo.resolve_change_id(imported_commit.change_id()), + repo.resolve_change_id(imported_commit.change_id()).unwrap(), Some(vec![imported_commit.id().clone()]), ); assert_eq!( - repo.resolve_change_id(authored_commit.change_id()), + repo.resolve_change_id(authored_commit.change_id()).unwrap(), Some(vec![authored_commit.id().clone()]), ); } @@ -4625,10 +4625,10 @@ fn test_concurrent_write_commit() { // The index should be consistent with the store. for commit_id in commit_change_ids.keys() { - assert!(repo.index().has_id(commit_id)); + assert!(repo.index().has_id(commit_id).unwrap()); let commit = repo.store().get_commit(commit_id).unwrap(); assert_eq!( - repo.resolve_change_id(commit.change_id()), + repo.resolve_change_id(commit.change_id()).unwrap(), Some(vec![commit_id.clone()]), ); } @@ -4751,10 +4751,10 @@ fn test_concurrent_read_write_commit() { // The index should be consistent with the store. let repo = repo.reload_at_head().unwrap(); for commit_id in &commit_ids { - assert!(repo.index().has_id(commit_id)); + assert!(repo.index().has_id(commit_id).unwrap()); let commit = repo.store().get_commit(commit_id).unwrap(); assert_eq!( - repo.resolve_change_id(commit.change_id()), + repo.resolve_change_id(commit.change_id()).unwrap(), Some(vec![commit_id.clone()]), ); } @@ -4873,7 +4873,7 @@ fn test_shallow_commits_lack_parents() { "unshallowed commits have correct parents" ); // FIXME: new ancestors should be indexed - assert!(!repo.index().has_id(&jj_id(a))); + assert!(!repo.index().has_id(&jj_id(a)).unwrap()); } #[test] diff --git a/lib/tests/test_id_prefix.rs b/lib/tests/test_id_prefix.rs index 6720ce3a19..63f5da0133 100644 --- a/lib/tests/test_id_prefix.rs +++ b/lib/tests/test_id_prefix.rs @@ -21,12 +21,14 @@ use jj_lib::backend::Timestamp; use jj_lib::config::ConfigLayer; use jj_lib::config::ConfigSource; use jj_lib::id_prefix::IdPrefixContext; +use jj_lib::id_prefix::IdPrefixIndex; use jj_lib::object_id::HexPrefix; use jj_lib::object_id::ObjectId as _; use jj_lib::object_id::PrefixResolution::AmbiguousMatch; use jj_lib::object_id::PrefixResolution::NoMatch; use jj_lib::object_id::PrefixResolution::SingleMatch; use jj_lib::op_store::RefTarget; +use jj_lib::repo::MutableRepo; use jj_lib::repo::Repo as _; use jj_lib::revset::RevsetExpression; use jj_lib::settings::UserSettings; @@ -144,59 +146,52 @@ fn test_id_prefix() { "); let prefix = |x| HexPrefix::try_from_hex(x).unwrap(); + let shortest_commit_prefix_len = |index: &IdPrefixIndex, commit_id| { + index + .shortest_commit_prefix_len(repo.as_ref(), commit_id) + .unwrap() + }; + let resolve_commit_prefix = |index: &IdPrefixIndex, prefix: HexPrefix| { + index.resolve_commit_prefix(repo.as_ref(), &prefix).unwrap() + }; + let shortest_change_prefix_len = |index: &IdPrefixIndex, change_id| { + index + .shortest_change_prefix_len(repo.as_ref(), change_id) + .unwrap() + }; + let resolve_change_prefix = |index: &IdPrefixIndex, prefix: HexPrefix| { + index.resolve_change_prefix(repo.as_ref(), &prefix).unwrap() + }; // Without a disambiguation revset // --------------------------------------------------------------------------------------------- let context = IdPrefixContext::default(); let index = context.populate(repo.as_ref()).unwrap(); + + assert_eq!(shortest_commit_prefix_len(&index, commits[7].id()), 2); + assert_eq!(shortest_commit_prefix_len(&index, commits[16].id()), 1); + assert_eq!(resolve_commit_prefix(&index, prefix("1")), AmbiguousMatch); assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), commits[7].id()), - 2 - ); - assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), commits[16].id()), - 1 - ); - assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("1")), - AmbiguousMatch - ); - assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("18")), + resolve_commit_prefix(&index, prefix("18")), SingleMatch(commits[7].id().clone()) ); + assert_eq!(resolve_commit_prefix(&index, prefix("10")), NoMatch); + assert_eq!(resolve_commit_prefix(&index, prefix("180")), NoMatch); assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("10")), - NoMatch - ); - assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("180")), - NoMatch - ); - assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()), + shortest_change_prefix_len(&index, commits[2].change_id()), 2 ); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), commits[6].change_id()), + shortest_change_prefix_len(&index, commits[6].change_id()), 1 ); + assert_eq!(resolve_change_prefix(&index, prefix("4")), AmbiguousMatch); assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("4")), - AmbiguousMatch - ); - assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("43")), + resolve_change_prefix(&index, prefix("43")), SingleMatch(vec![commits[2].id().clone()]) ); - assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("40")), - NoMatch - ); - assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("430")), - NoMatch - ); + assert_eq!(resolve_change_prefix(&index, prefix("40")), NoMatch); + assert_eq!(resolve_change_prefix(&index, prefix("430")), NoMatch); // Disambiguate within a revset // --------------------------------------------------------------------------------------------- @@ -205,26 +200,23 @@ fn test_id_prefix() { let context = context.disambiguate_within(expression); let index = context.populate(repo.as_ref()).unwrap(); // The prefix is now shorter - assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), commits[7].id()), - 1 - ); + assert_eq!(shortest_commit_prefix_len(&index, commits[7].id()), 1); // Shorter prefix within the set can be used assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("1")), + resolve_commit_prefix(&index, prefix("1")), SingleMatch(commits[7].id().clone()) ); // Can still resolve commits outside the set assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("19")), + resolve_commit_prefix(&index, prefix("19")), SingleMatch(commits[10].id().clone()) ); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()), + shortest_change_prefix_len(&index, commits[2].change_id()), 1 ); assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("4")), + resolve_change_prefix(&index, prefix("4")), SingleMatch(vec![commits[2].id().clone()]) ); @@ -234,28 +226,16 @@ fn test_id_prefix() { let expression = RevsetExpression::commit(root_commit_id.clone()); let context = context.disambiguate_within(expression); let index = context.populate(repo.as_ref()).unwrap(); + assert_eq!(shortest_commit_prefix_len(&index, root_commit_id), 1); + assert_eq!(resolve_commit_prefix(&index, prefix("")), AmbiguousMatch); assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), root_commit_id), - 1 - ); - assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("")), - AmbiguousMatch - ); - assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix("0")), + resolve_commit_prefix(&index, prefix("0")), SingleMatch(root_commit_id.clone()) ); + assert_eq!(shortest_change_prefix_len(&index, root_change_id), 1); + assert_eq!(resolve_change_prefix(&index, prefix("")), AmbiguousMatch); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), root_change_id), - 1 - ); - assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("")), - AmbiguousMatch - ); - assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("0")), + resolve_change_prefix(&index, prefix("0")), SingleMatch(vec![root_commit_id.clone()]) ); @@ -331,33 +311,38 @@ fn test_id_prefix_divergent() { "); let prefix = |x| HexPrefix::try_from_hex(x).unwrap(); + let shortest_change_prefix_len = |index: &IdPrefixIndex, change_id| { + index + .shortest_change_prefix_len(repo.as_ref(), change_id) + .unwrap() + }; + let resolve_change_prefix = |index: &IdPrefixIndex, prefix: HexPrefix| { + index.resolve_change_prefix(repo.as_ref(), &prefix).unwrap() + }; // Without a disambiguation revset // -------------------------------- let context = IdPrefixContext::default(); let index = context.populate(repo.as_ref()).unwrap(); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), commits[0].change_id()), + shortest_change_prefix_len(&index, commits[0].change_id()), 3 ); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), commits[1].change_id()), + shortest_change_prefix_len(&index, commits[1].change_id()), 3 ); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), commits[2].change_id()), + shortest_change_prefix_len(&index, commits[2].change_id()), 3 ); + assert_eq!(resolve_change_prefix(&index, prefix("a5")), AmbiguousMatch); assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("a5")), - AmbiguousMatch - ); - assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("a53")), + resolve_change_prefix(&index, prefix("a53")), SingleMatch(vec![first_commit.id().clone()]) ); assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("a50")), + resolve_change_prefix(&index, prefix("a50")), SingleMatch(vec![ second_commit.id().clone(), third_commit_divergent_with_second.id().clone() @@ -371,7 +356,7 @@ fn test_id_prefix_divergent() { let index = context.populate(repo.as_ref()).unwrap(); // The prefix is now shorter assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), second_commit.change_id()), + shortest_change_prefix_len(&index, second_commit.change_id()), 1 ); // This tests two issues, both important: @@ -382,7 +367,7 @@ fn test_id_prefix_divergent() { // match is not ambiguous, even though the first commit's change id would also // match the prefix. assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("a")), + resolve_change_prefix(&index, prefix("a")), SingleMatch(vec![ second_commit.id().clone(), third_commit_divergent_with_second.id().clone() @@ -391,11 +376,11 @@ fn test_id_prefix_divergent() { // We can still resolve commits outside the set assert_eq!( - index.resolve_change_prefix(repo.as_ref(), &prefix("a53")), + resolve_change_prefix(&index, prefix("a53")), SingleMatch(vec![first_commit.id().clone()]) ); assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), first_commit.change_id()), + shortest_change_prefix_len(&index, first_commit.change_id()), 3 ); } @@ -474,39 +459,46 @@ fn test_id_prefix_hidden() { let repo = tx.commit("test").unwrap(); let prefix = |x: &str| HexPrefix::try_from_hex(x).unwrap(); + let shortest_commit_prefix_len = |index: &IdPrefixIndex, commit_id| { + index + .shortest_commit_prefix_len(repo.as_ref(), commit_id) + .unwrap() + }; + let resolve_commit_prefix = |index: &IdPrefixIndex, prefix: HexPrefix| { + index.resolve_commit_prefix(repo.as_ref(), &prefix).unwrap() + }; + let shortest_change_prefix_len = |index: &IdPrefixIndex, change_id| { + index + .shortest_change_prefix_len(repo.as_ref(), change_id) + .unwrap() + }; + let resolve_change_prefix = |index: &IdPrefixIndex, prefix: HexPrefix| { + index.resolve_change_prefix(repo.as_ref(), &prefix).unwrap() + }; // Without a disambiguation revset // -------------------------------- let context = IdPrefixContext::default(); let index = context.populate(repo.as_ref()).unwrap(); + assert_eq!(shortest_commit_prefix_len(&index, hidden_commit.id()), 2); assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), - 2 - ); - assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()), + shortest_change_prefix_len(&index, hidden_commit.change_id()), 3 ); assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])), + resolve_commit_prefix(&index, prefix(&hidden_commit.id().hex()[..1])), AmbiguousMatch ); assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..2])), + resolve_commit_prefix(&index, prefix(&hidden_commit.id().hex()[..2])), SingleMatch(hidden_commit.id().clone()) ); assert_eq!( - index.resolve_change_prefix( - repo.as_ref(), - &prefix(&hidden_commit.change_id().hex()[..2]) - ), + resolve_change_prefix(&index, prefix(&hidden_commit.change_id().hex()[..2])), AmbiguousMatch ); assert_eq!( - index.resolve_change_prefix( - repo.as_ref(), - &prefix(&hidden_commit.change_id().hex()[..3]) - ), + resolve_change_prefix(&index, prefix(&hidden_commit.change_id().hex()[..3])), NoMatch ); @@ -515,34 +507,25 @@ fn test_id_prefix_hidden() { let expression = RevsetExpression::commit(hidden_commit.id().clone()); let context = context.disambiguate_within(expression); let index = context.populate(repo.as_ref()).unwrap(); + assert_eq!(shortest_commit_prefix_len(&index, hidden_commit.id()), 1); assert_eq!( - index.shortest_commit_prefix_len(repo.as_ref(), hidden_commit.id()), - 1 - ); - assert_eq!( - index.shortest_change_prefix_len(repo.as_ref(), hidden_commit.change_id()), + shortest_change_prefix_len(&index, hidden_commit.change_id()), 1 ); // Short commit id can be resolved even if it's hidden. assert_eq!( - index.resolve_commit_prefix(repo.as_ref(), &prefix(&hidden_commit.id().hex()[..1])), + resolve_commit_prefix(&index, prefix(&hidden_commit.id().hex()[..1])), SingleMatch(hidden_commit.id().clone()) ); // OTOH, hidden change id should never be found. The resolution might be // ambiguous if hidden commits were excluded from the disambiguation set. // In that case, shortest_change_prefix_len() shouldn't be 1. assert_eq!( - index.resolve_change_prefix( - repo.as_ref(), - &prefix(&hidden_commit.change_id().hex()[..1]) - ), + resolve_change_prefix(&index, prefix(&hidden_commit.change_id().hex()[..1])), NoMatch ); assert_eq!( - index.resolve_change_prefix( - repo.as_ref(), - &prefix(&hidden_commit.change_id().hex()[..2]) - ), + resolve_change_prefix(&index, prefix(&hidden_commit.change_id().hex()[..2])), NoMatch ); } @@ -571,10 +554,15 @@ fn test_id_prefix_shadowed_by_ref() { let context = IdPrefixContext::default(); let index = context.populate(tx.repo()).unwrap(); - - assert_eq!(index.shortest_commit_prefix_len(tx.repo(), commit.id()), 1); + let shortest_commit_prefix_len = |repo: &mut MutableRepo, commit_id| { + index.shortest_commit_prefix_len(repo, commit_id).unwrap() + }; + let shortest_change_prefix_len = |repo: &mut MutableRepo, change_id| { + index.shortest_change_prefix_len(repo, change_id).unwrap() + }; + assert_eq!(shortest_commit_prefix_len(tx.repo_mut(), commit.id()), 1); assert_eq!( - index.shortest_change_prefix_len(tx.repo(), commit.change_id()), + shortest_change_prefix_len(tx.repo_mut(), commit.change_id()), 1 ); @@ -584,9 +572,9 @@ fn test_id_prefix_shadowed_by_ref() { .set_local_tag_target(commit_id_sym[..2].as_ref(), dummy_target.clone()); tx.repo_mut() .set_local_tag_target(change_id_sym[..2].as_ref(), dummy_target.clone()); - assert_eq!(index.shortest_commit_prefix_len(tx.repo(), commit.id()), 1); + assert_eq!(shortest_commit_prefix_len(tx.repo_mut(), commit.id()), 1); assert_eq!( - index.shortest_change_prefix_len(tx.repo(), commit.change_id()), + shortest_change_prefix_len(tx.repo_mut(), commit.change_id()), 1 ); @@ -595,9 +583,9 @@ fn test_id_prefix_shadowed_by_ref() { .set_local_bookmark_target(commit_id_sym[..1].as_ref(), dummy_target.clone()); tx.repo_mut() .set_local_bookmark_target(change_id_sym[..1].as_ref(), dummy_target.clone()); - assert_eq!(index.shortest_commit_prefix_len(tx.repo(), commit.id()), 3); + assert_eq!(shortest_commit_prefix_len(tx.repo_mut(), commit.id()), 3); assert_eq!( - index.shortest_change_prefix_len(tx.repo(), commit.change_id()), + shortest_change_prefix_len(tx.repo_mut(), commit.change_id()), 3 ); @@ -611,11 +599,11 @@ fn test_id_prefix_shadowed_by_ref() { .set_local_tag_target(change_id_sym[..n].as_ref(), dummy_target.clone()); } assert_eq!( - index.shortest_commit_prefix_len(tx.repo(), commit.id()), + shortest_commit_prefix_len(tx.repo_mut(), commit.id()), commit_id_sym.len() ); assert_eq!( - index.shortest_change_prefix_len(tx.repo(), commit.change_id()), + shortest_change_prefix_len(tx.repo_mut(), commit.change_id()), change_id_sym.len() ); @@ -625,11 +613,11 @@ fn test_id_prefix_shadowed_by_ref() { tx.repo_mut() .set_local_tag_target(change_id_sym.as_ref(), dummy_target.clone()); assert_eq!( - index.shortest_commit_prefix_len(tx.repo(), commit.id()), + shortest_commit_prefix_len(tx.repo_mut(), commit.id()), commit_id_sym.len() ); assert_eq!( - index.shortest_change_prefix_len(tx.repo(), commit.change_id()), + shortest_change_prefix_len(tx.repo_mut(), commit.change_id()), change_id_sym.len() ); } diff --git a/lib/tests/test_index.rs b/lib/tests/test_index.rs index 20f9a2612e..7922fdc19c 100644 --- a/lib/tests/test_index.rs +++ b/lib/tests/test_index.rs @@ -25,7 +25,7 @@ use jj_lib::default_index::DefaultIndexStore; use jj_lib::default_index::DefaultIndexStoreError; use jj_lib::default_index::DefaultMutableIndex; use jj_lib::default_index::DefaultReadonlyIndex; -use jj_lib::index::Index as _; +use jj_lib::index::Index; use jj_lib::object_id::HexPrefix; use jj_lib::object_id::ObjectId as _; use jj_lib::object_id::PrefixResolution; @@ -80,6 +80,14 @@ fn collect_changed_paths(repo: &ReadonlyRepo, commit_id: &CommitId) -> Option bool { + index.has_id(commit_id).unwrap() +} + +fn is_ancestor(index: &dyn Index, ancestor_id: &CommitId, descendant_id: &CommitId) -> bool { + index.is_ancestor(ancestor_id, descendant_id).unwrap() +} + #[test] fn test_index_commits_empty_repo() { let test_repo = TestRepo::init(); @@ -148,19 +156,19 @@ fn test_index_commits_standard_cases() { assert_eq!(index.generation_number(commit_g.id()).unwrap(), 6); assert_eq!(index.generation_number(commit_h.id()).unwrap(), 5); - assert!(index.is_ancestor(root_commit_id, commit_a.id())); - assert!(!index.is_ancestor(commit_a.id(), root_commit_id)); + assert!(is_ancestor(index, root_commit_id, commit_a.id())); + assert!(!is_ancestor(index, commit_a.id(), root_commit_id)); - assert!(index.is_ancestor(root_commit_id, commit_b.id())); - assert!(!index.is_ancestor(commit_b.id(), root_commit_id)); + assert!(is_ancestor(index, root_commit_id, commit_b.id())); + assert!(!is_ancestor(index, commit_b.id(), root_commit_id)); - assert!(!index.is_ancestor(commit_b.id(), commit_c.id())); + assert!(!is_ancestor(index, commit_b.id(), commit_c.id())); - assert!(index.is_ancestor(commit_a.id(), commit_b.id())); - assert!(index.is_ancestor(commit_a.id(), commit_e.id())); - assert!(index.is_ancestor(commit_a.id(), commit_f.id())); - assert!(index.is_ancestor(commit_a.id(), commit_g.id())); - assert!(index.is_ancestor(commit_a.id(), commit_h.id())); + assert!(is_ancestor(index, commit_a.id(), commit_b.id())); + assert!(is_ancestor(index, commit_a.id(), commit_e.id())); + assert!(is_ancestor(index, commit_a.id(), commit_f.id())); + assert!(is_ancestor(index, commit_a.id(), commit_g.id())); + assert!(is_ancestor(index, commit_a.id(), commit_h.id())); } #[test] @@ -225,11 +233,13 @@ fn test_index_commits_criss_cross() { // The left and right commits of the same generation should not be ancestors of // each other for generation in 0..num_generations { - assert!(!index.is_ancestor( + assert!(!is_ancestor( + index, left_commits[generation].id(), right_commits[generation].id() )); - assert!(!index.is_ancestor( + assert!(!is_ancestor( + index, right_commits[generation].id(), left_commits[generation].id() )); @@ -240,12 +250,18 @@ fn test_index_commits_criss_cross() { for generation in 1..num_generations { for ancestor_side in &[&left_commits, &right_commits] { for descendant_side in &[&left_commits, &right_commits] { - assert!(index.is_ancestor(ancestor_side[0].id(), descendant_side[generation].id())); - assert!(index.is_ancestor( + assert!(is_ancestor( + index, + ancestor_side[0].id(), + descendant_side[generation].id() + )); + assert!(is_ancestor( + index, ancestor_side[generation - 1].id(), descendant_side[generation].id() )); - assert!(index.is_ancestor( + assert!(is_ancestor( + index, ancestor_side[generation / 2].id(), descendant_side[generation].id() )); @@ -412,12 +428,12 @@ fn test_index_commits_hidden_but_referenced() { state: jj_lib::op_store::RemoteRefState::New, }, ); - let repo = tx.commit("test").unwrap(); + let repo = tx.commit("test").unwrap(); // All commits should be indexed - assert!(repo.index().has_id(commit_a.id())); - assert!(repo.index().has_id(commit_b.id())); - assert!(repo.index().has_id(commit_c.id())); + assert!(index_has_id(repo.index(), commit_a.id())); + assert!(index_has_id(repo.index(), commit_b.id())); + assert!(index_has_id(repo.index(), commit_c.id())); // Delete index from disk let default_index_store: &DefaultIndexStore = repo.index_store().downcast_ref().unwrap(); @@ -425,9 +441,9 @@ fn test_index_commits_hidden_but_referenced() { let repo = test_env.load_repo_at_head(&settings, test_repo.repo_path()); // All commits should be reindexed - assert!(repo.index().has_id(commit_a.id())); - assert!(repo.index().has_id(commit_b.id())); - assert!(repo.index().has_id(commit_c.id())); + assert!(index_has_id(repo.index(), commit_a.id())); + assert!(index_has_id(repo.index(), commit_b.id())); + assert!(index_has_id(repo.index(), commit_c.id())); } #[test] @@ -536,7 +552,7 @@ fn test_index_commits_incremental_already_indexed() { let commit_a = write_random_commit_with_parents(tx.repo_mut(), &[&root_commit]); let repo = tx.commit("test").unwrap(); - assert!(repo.index().has_id(commit_a.id())); + assert!(index_has_id(repo.index(), commit_a.id())); assert_eq!(as_readonly_index(&repo).num_commits(), 1 + 1); let mut tx = repo.start_transaction(); let mut_repo = tx.repo_mut(); @@ -641,7 +657,7 @@ fn test_reindex_no_segments_dir() { let mut tx = repo.start_transaction(); let commit_a = write_random_commit(tx.repo_mut()); let repo = tx.commit("test").unwrap(); - assert!(repo.index().has_id(commit_a.id())); + assert!(index_has_id(repo.index(), commit_a.id())); // jj <= 0.14 doesn't have "segments" directory let segments_dir = test_repo.repo_path().join("index").join("segments"); @@ -649,7 +665,7 @@ fn test_reindex_no_segments_dir() { fs::remove_dir_all(&segments_dir).unwrap(); let repo = test_env.load_repo_at_head(&settings, test_repo.repo_path()); - assert!(repo.index().has_id(commit_a.id())); + assert!(index_has_id(repo.index(), commit_a.id())); } #[test] @@ -662,7 +678,7 @@ fn test_reindex_corrupt_segment_files() { let mut tx = repo.start_transaction(); let commit_a = write_random_commit(tx.repo_mut()); let repo = tx.commit("test").unwrap(); - assert!(repo.index().has_id(commit_a.id())); + assert!(index_has_id(repo.index(), commit_a.id())); // Corrupt the index files let segments_dir = test_repo.repo_path().join("index").join("segments"); @@ -678,7 +694,7 @@ fn test_reindex_corrupt_segment_files() { } let repo = test_env.load_repo_at_head(&settings, test_repo.repo_path()); - assert!(repo.index().has_id(commit_a.id())); + assert!(index_has_id(repo.index(), commit_a.id())); } #[test] @@ -1185,9 +1201,13 @@ fn test_change_id_index() { .mutable_index() .change_id_index(&mut commits.iter().map(|commit| commit.id())) }; - let change_id_index = index_for_heads(&[&commit_1, &commit_2, &commit_3, &commit_4, &commit_5]); - let prefix_len = - |commit: &Commit| change_id_index.shortest_unique_prefix_len(commit.change_id()); + let change_id_index = + index_for_heads(&[&commit_1, &commit_2, &commit_3, &commit_4, &commit_5]).unwrap(); + let prefix_len = |commit: &Commit| { + change_id_index + .shortest_unique_prefix_len(commit.change_id()) + .unwrap() + }; assert_eq!(prefix_len(&root_commit), 1); assert_eq!(prefix_len(&commit_1), 2); assert_eq!(prefix_len(&commit_2), 6); @@ -1197,6 +1217,7 @@ fn test_change_id_index() { let resolve_prefix = |prefix: &str| { change_id_index .resolve_prefix(&HexPrefix::try_from_hex(prefix).unwrap()) + .unwrap() .map(HashSet::from_iter) }; // Ambiguous matches @@ -1234,10 +1255,11 @@ fn test_change_id_index() { // Test with an index containing only some of the commits. The shortest // length doesn't have to be minimized further, but unreachable commits // should never be included in the resolved set. - let change_id_index = index_for_heads(&[&commit_1, &commit_2]); + let change_id_index = index_for_heads(&[&commit_1, &commit_2]).unwrap(); let resolve_prefix = |prefix: &str| { change_id_index .resolve_prefix(&HexPrefix::try_from_hex(prefix).unwrap()) + .unwrap() .map(HashSet::from_iter) }; assert_eq!( diff --git a/lib/tests/test_mut_repo.rs b/lib/tests/test_mut_repo.rs index 6819936586..db925ade6c 100644 --- a/lib/tests/test_mut_repo.rs +++ b/lib/tests/test_mut_repo.rs @@ -13,6 +13,7 @@ // limitations under the License. use jj_lib::backend::CommitId; +use jj_lib::index::Index; use jj_lib::op_store::RefTarget; use jj_lib::op_store::RemoteRef; use jj_lib::op_store::RemoteRefState; @@ -44,6 +45,10 @@ where } } +fn index_has_id(index: &dyn Index, commit_id: &CommitId) -> bool { + index.has_id(commit_id).unwrap() +} + #[test] fn test_edit() { // Test that MutableRepo::edit() uses the requested commit (not a new child) @@ -354,14 +359,15 @@ fn test_add_head_success() { let mut tx = repo.start_transaction(); let mut_repo = tx.repo_mut(); + assert!(!mut_repo.view().heads().contains(new_commit.id())); - assert!(!mut_repo.index().has_id(new_commit.id())); + assert!(!index_has_id(mut_repo.index(), new_commit.id())); mut_repo.add_head(&new_commit).unwrap(); assert!(mut_repo.view().heads().contains(new_commit.id())); - assert!(mut_repo.index().has_id(new_commit.id())); + assert!(index_has_id(mut_repo.index(), new_commit.id())); let repo = tx.commit("test").unwrap(); assert!(repo.view().heads().contains(new_commit.id())); - assert!(repo.index().has_id(new_commit.id())); + assert!(index_has_id(repo.index(), new_commit.id())); } #[test] @@ -414,9 +420,9 @@ fn test_add_head_not_immediate_child() { mut_repo.view().heads(), &hashset! {initial.id().clone(), child.id().clone()} ); - assert!(mut_repo.index().has_id(initial.id())); - assert!(mut_repo.index().has_id(rewritten.id())); - assert!(mut_repo.index().has_id(child.id())); + assert!(index_has_id(mut_repo.index(), initial.id())); + assert!(index_has_id(mut_repo.index(), rewritten.id())); + assert!(index_has_id(mut_repo.index(), child.id())); } #[test] @@ -441,17 +447,17 @@ fn test_remove_head() { assert!(!heads.contains(commit3.id())); assert!(!heads.contains(commit2.id())); assert!(!heads.contains(commit1.id())); - assert!(mut_repo.index().has_id(commit1.id())); - assert!(mut_repo.index().has_id(commit2.id())); - assert!(mut_repo.index().has_id(commit3.id())); + assert!(index_has_id(mut_repo.index(), commit1.id())); + assert!(index_has_id(mut_repo.index(), commit2.id())); + assert!(index_has_id(mut_repo.index(), commit3.id())); let repo = tx.commit("test").unwrap(); let heads = repo.view().heads().clone(); assert!(!heads.contains(commit3.id())); assert!(!heads.contains(commit2.id())); assert!(!heads.contains(commit1.id())); - assert!(repo.index().has_id(commit1.id())); - assert!(repo.index().has_id(commit2.id())); - assert!(repo.index().has_id(commit3.id())); + assert!(index_has_id(repo.index(), commit1.id())); + assert!(index_has_id(repo.index(), commit2.id())); + assert!(index_has_id(repo.index(), commit3.id())); } #[test] diff --git a/lib/tests/test_operations.rs b/lib/tests/test_operations.rs index c36bc4fd95..9edd424ae0 100644 --- a/lib/tests/test_operations.rs +++ b/lib/tests/test_operations.rs @@ -23,6 +23,7 @@ use jj_lib::backend::CommitId; use jj_lib::config::ConfigLayer; use jj_lib::config::ConfigSource; use jj_lib::evolution::walk_predecessors; +use jj_lib::index::Index; use jj_lib::object_id::ObjectId as _; use jj_lib::op_store::OperationId; use jj_lib::op_walk; @@ -56,6 +57,10 @@ fn list_dir(dir: &Path) -> Vec { .collect() } +fn index_has_id(index: &dyn Index, commit_id: &CommitId) -> bool { + index.has_id(commit_id).unwrap() +} + #[test] fn test_unpublished_operation() { // Test that the operation doesn't get published until that's requested. @@ -567,8 +572,8 @@ fn test_reparent_discarding_predecessors(op_stores_commit_predecessors: bool) { assert_eq!(stats.unreachable_count, 1); let repo = repo_at(&stats.new_head_ids[0]); // A0 - B0 are still reachable - assert!(repo.index().has_id(commit_a0.id())); - assert!(repo.index().has_id(commit_b0.id())); + assert!(index_has_id(repo.index(), commit_a0.id())); + assert!(index_has_id(repo.index(), commit_b0.id())); assert_eq!( get_predecessors(&repo, commit_a1.id()), [commit_a0.id().clone()] @@ -593,17 +598,17 @@ fn test_reparent_discarding_predecessors(op_stores_commit_predecessors: bool) { assert_eq!(stats.unreachable_count, 2); let repo = repo_at(&stats.new_head_ids[0]); // A0 is still reachable - assert!(repo.index().has_id(commit_a0.id())); + assert!(index_has_id(repo.index(), commit_a0.id())); if op_stores_commit_predecessors { // B0 is no longer reachable - assert!(!repo.index().has_id(commit_b0.id())); + assert!(!index_has_id(repo.index(), commit_b0.id())); // the predecessor record `A1: A0` no longer exists assert_eq!(get_predecessors(&repo, commit_a1.id()), []); // Unreachable predecessors should be excluded assert_eq!(get_predecessors(&repo, commit_b1.id()), []); } else { // B0 is retained because it is immediate predecessor of B1 - assert!(repo.index().has_id(commit_b0.id())); + assert!(index_has_id(repo.index(), commit_b0.id())); assert_eq!( get_predecessors(&repo, commit_a1.id()), [commit_a0.id().clone()] @@ -627,9 +632,9 @@ fn test_reparent_discarding_predecessors(op_stores_commit_predecessors: bool) { assert_eq!(stats.unreachable_count, 3); let repo = repo_at(&stats.new_head_ids[0]); // A0 is no longer reachable - assert!(!repo.index().has_id(commit_a0.id())); + assert!(!index_has_id(repo.index(), commit_a0.id())); // A1 is still reachable through A2 - assert!(repo.index().has_id(commit_a1.id())); + assert!(index_has_id(repo.index(), commit_a1.id())); assert_eq!( get_predecessors(&repo, commit_a2.id()), [commit_a1.id().clone()] diff --git a/lib/tests/test_refs.rs b/lib/tests/test_refs.rs index bd988e40ed..3255471705 100644 --- a/lib/tests/test_refs.rs +++ b/lib/tests/test_refs.rs @@ -21,7 +21,7 @@ use testutils::write_random_commit; use testutils::write_random_commit_with_parents; #[test] -fn test_merge_ref_targets() { +fn test_merge() { let test_workspace = TestWorkspace::init(); let repo = &test_workspace.repo; @@ -52,99 +52,58 @@ fn test_merge_ref_targets() { let _target7 = RefTarget::normal(commit7.id().clone()); let index = repo.index(); + let merge = |left: &RefTarget, base: &RefTarget, right: &RefTarget| { + merge_ref_targets(index, left, base, right).unwrap() + }; // Left moved forward - assert_eq!( - merge_ref_targets(index, &target3, &target1, &target1), - target3 - ); + assert_eq!(merge(&target3, &target1, &target1), target3); // Right moved forward - assert_eq!( - merge_ref_targets(index, &target1, &target1, &target3), - target3 - ); + assert_eq!(merge(&target1, &target1, &target3), target3); // Left moved backward - assert_eq!( - merge_ref_targets(index, &target1, &target3, &target3), - target1 - ); + assert_eq!(merge(&target1, &target3, &target3), target1); // Right moved backward - assert_eq!( - merge_ref_targets(index, &target3, &target3, &target1), - target1 - ); + assert_eq!(merge(&target3, &target3, &target1), target1); // Left moved sideways - assert_eq!( - merge_ref_targets(index, &target4, &target3, &target3), - target4 - ); + assert_eq!(merge(&target4, &target3, &target3), target4); // Right moved sideways - assert_eq!( - merge_ref_targets(index, &target3, &target3, &target4), - target4 - ); + assert_eq!(merge(&target3, &target3, &target4), target4); // Both moved sideways ("A - B + A" - type conflict) - assert_eq!( - merge_ref_targets(index, &target4, &target3, &target4), - target4 - ); + assert_eq!(merge(&target4, &target3, &target4), target4); // Both added same target ("A - B + A" - type conflict) - assert_eq!( - merge_ref_targets(index, &target3, RefTarget::absent_ref(), &target3), - target3 - ); + assert_eq!(merge(&target3, RefTarget::absent_ref(), &target3), target3); // Both removed ("A - B + A" - type conflict) assert_eq!( - merge_ref_targets( - index, - RefTarget::absent_ref(), - &target3, - RefTarget::absent_ref() - ), + merge(RefTarget::absent_ref(), &target3, RefTarget::absent_ref()), RefTarget::absent() ); // Left added target, right added descendant target - assert_eq!( - merge_ref_targets(index, &target2, RefTarget::absent_ref(), &target3), - target3 - ); + assert_eq!(merge(&target2, RefTarget::absent_ref(), &target3), target3); // Right added target, left added descendant target - assert_eq!( - merge_ref_targets(index, &target3, RefTarget::absent_ref(), &target2), - target3 - ); + assert_eq!(merge(&target3, RefTarget::absent_ref(), &target2), target3); // Both moved forward to same target - assert_eq!( - merge_ref_targets(index, &target3, &target1, &target3), - target3 - ); + assert_eq!(merge(&target3, &target1, &target3), target3); // Both moved forward, left moved further - assert_eq!( - merge_ref_targets(index, &target3, &target1, &target2), - target3 - ); + assert_eq!(merge(&target3, &target1, &target2), target3); // Both moved forward, right moved further - assert_eq!( - merge_ref_targets(index, &target2, &target1, &target3), - target3 - ); + assert_eq!(merge(&target2, &target1, &target3), target3); // Left and right moved forward to divergent targets assert_eq!( - merge_ref_targets(index, &target3, &target1, &target4), + merge(&target3, &target1, &target4), RefTarget::from_legacy_form( [commit1.id().clone()], [commit3.id().clone(), commit4.id().clone()] @@ -153,7 +112,7 @@ fn test_merge_ref_targets() { // Left moved back, right moved forward assert_eq!( - merge_ref_targets(index, &target1, &target2, &target3), + merge(&target1, &target2, &target3), RefTarget::from_legacy_form( [commit2.id().clone()], [commit1.id().clone(), commit3.id().clone()] @@ -162,7 +121,7 @@ fn test_merge_ref_targets() { // Right moved back, left moved forward assert_eq!( - merge_ref_targets(index, &target3, &target2, &target1), + merge(&target3, &target2, &target1), RefTarget::from_legacy_form( [commit2.id().clone()], [commit3.id().clone(), commit1.id().clone()] @@ -171,19 +130,19 @@ fn test_merge_ref_targets() { // Left removed assert_eq!( - merge_ref_targets(index, RefTarget::absent_ref(), &target3, &target3), + merge(RefTarget::absent_ref(), &target3, &target3), RefTarget::absent() ); // Right removed assert_eq!( - merge_ref_targets(index, &target3, &target3, RefTarget::absent_ref()), + merge(&target3, &target3, RefTarget::absent_ref()), RefTarget::absent() ); // Left removed, right moved forward assert_eq!( - merge_ref_targets(index, RefTarget::absent_ref(), &target1, &target3), + merge(RefTarget::absent_ref(), &target1, &target3), RefTarget::from_merge(Merge::from_vec(vec![ None, Some(commit1.id().clone()), @@ -193,7 +152,7 @@ fn test_merge_ref_targets() { // Right removed, left moved forward assert_eq!( - merge_ref_targets(index, &target3, &target1, RefTarget::absent_ref()), + merge(&target3, &target1, RefTarget::absent_ref()), RefTarget::from_merge(Merge::from_vec(vec![ Some(commit3.id().clone()), Some(commit1.id().clone()), @@ -203,8 +162,7 @@ fn test_merge_ref_targets() { // Left became conflicted, right moved forward assert_eq!( - merge_ref_targets( - index, + merge( &RefTarget::from_legacy_form( [commit2.id().clone()], [commit3.id().clone(), commit4.id().clone()] @@ -221,8 +179,7 @@ fn test_merge_ref_targets() { // Right became conflicted, left moved forward assert_eq!( - merge_ref_targets( - index, + merge( &target3, &target1, &RefTarget::from_legacy_form( @@ -245,8 +202,7 @@ fn test_merge_ref_targets() { // 3 // ``` assert_eq!( - merge_ref_targets( - index, + merge( &RefTarget::from_legacy_form( [commit2.id().clone()], [commit3.id().clone(), commit4.id().clone()] @@ -269,8 +225,7 @@ fn test_merge_ref_targets() { // 3 // ``` assert_eq!( - merge_ref_targets( - index, + merge( &target5, &target3, &RefTarget::from_legacy_form( @@ -294,8 +249,7 @@ fn test_merge_ref_targets() { // 3 // ``` assert_eq!( - merge_ref_targets( - index, + merge( &RefTarget::from_legacy_form( [commit2.id().clone()], [commit3.id().clone(), commit4.id().clone()] @@ -319,8 +273,7 @@ fn test_merge_ref_targets() { // 3 // ``` assert_eq!( - merge_ref_targets( - index, + merge( &target1, &target3, &RefTarget::from_legacy_form( @@ -336,8 +289,7 @@ fn test_merge_ref_targets() { // Existing conflict on left, right undoes one side of conflict assert_eq!( - merge_ref_targets( - index, + merge( &RefTarget::from_legacy_form( [commit2.id().clone()], [commit3.id().clone(), commit4.id().clone()] @@ -350,8 +302,7 @@ fn test_merge_ref_targets() { // Existing conflict on right, left undoes one side of conflict assert_eq!( - merge_ref_targets( - index, + merge( &target2, &target3, &RefTarget::from_legacy_form( @@ -365,8 +316,7 @@ fn test_merge_ref_targets() { // Existing conflict on left, right moves one side of conflict to the other // side ("A - B + A" - type conflict) assert_eq!( - merge_ref_targets( - index, + merge( &RefTarget::from_legacy_form( [commit5.id().clone()], // not an ancestor of commit3, 4 [commit3.id().clone(), commit4.id().clone()], @@ -380,8 +330,7 @@ fn test_merge_ref_targets() { // Existing conflict on right, left moves one side of conflict to the other // side ("A - B + A" - type conflict) assert_eq!( - merge_ref_targets( - index, + merge( &target4, &target3, &RefTarget::from_legacy_form( @@ -394,8 +343,7 @@ fn test_merge_ref_targets() { // Existing conflict on left, right makes unrelated update assert_eq!( - merge_ref_targets( - index, + merge( &RefTarget::from_legacy_form( [commit2.id().clone()], [commit3.id().clone(), commit4.id().clone()] @@ -415,8 +363,7 @@ fn test_merge_ref_targets() { // Existing conflict on right, left makes unrelated update assert_eq!( - merge_ref_targets( - index, + merge( &target6, &target5, &RefTarget::from_legacy_form(