DRAFT: Demonstrating fallible index traits

cli/src/cli_util.rs

@@ -2485,16 +2485,21 @@ 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<AdvanceableBookmark>, move_to: &CommitId) {
+    pub fn advance_bookmarks(
+        &mut self,
+        bookmarks: Vec<AdvanceableBookmark>,
+        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.
             self.repo_mut().merge_local_bookmark(
                 &bookmark.name,
                 &RefTarget::normal(bookmark.old_commit_id),
                 &RefTarget::normal(move_to.clone()),
-            );
+            )?;
         }
+        Ok(())
     }
 }
 

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<BackendError> for CommandError {
     }
 }
 
+impl From<IndexError> for CommandError {
+    fn from(err: IndexError) -> Self {
+        internal_error_with_message("Unexpected error from index", err)
+    }
+}
+
 impl From<OpHeadsStoreError> for CommandError {
     fn from(err: OpHeadsStoreError) -> Self {
         internal_error_with_message("Unexpected error from operation heads store", err)
@@ -422,6 +429,7 @@ impl From<WalkPredecessorsError> 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,
             };

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<bool, CommandError> {
     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)
     }
 }

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!(

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}",

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!(

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();

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",

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)?;

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}")
                     }

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")?;

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 = {

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()),

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<usize>;
 }
 
 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<usize> {
         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<usize> {
         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())
         },

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<bool> {
+        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

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.