From 760134f9c1045a5718bcb64b40c7eae13ce63f69 Mon Sep 17 00:00:00 2001
From: Yuya Nishihara <yuya@tcha.org>
Date: Mon, 7 Oct 2024 11:00:37 +0900
Subject: [PATCH] revset: error out on unindexed commit ID instead of panicking

It no longer makes sense to handle missing root commit by the revset frontend,
but panicking wouldn't be good either. Let's make it error out.
---
 lib/src/default_index/revset_engine.rs | 27 ++++++++++++++++++++------
 lib/src/revset.rs                      | 13 +------------
 lib/tests/test_revset.rs               |  4 ++++
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/lib/src/default_index/revset_engine.rs b/lib/src/default_index/revset_engine.rs
index b8473c468c..d84c48a333 100644
--- a/lib/src/default_index/revset_engine.rs
+++ b/lib/src/default_index/revset_engine.rs
@@ -52,6 +52,7 @@ use crate::graph::GraphEdge;
 use crate::matchers::Matcher;
 use crate::matchers::Visit;
 use crate::merged_tree::resolve_file_values;
+use crate::object_id::ObjectId as _;
 use crate::repo_path::RepoPath;
 use crate::revset::ResolvedExpression;
 use crate::revset::ResolvedPredicateExpression;
@@ -759,7 +760,7 @@ impl<'index> EvaluationContext<'index> {
         let index = self.index;
         match expression {
             ResolvedExpression::Commits(commit_ids) => {
-                Ok(Box::new(self.revset_for_commit_ids(commit_ids)))
+                Ok(Box::new(self.revset_for_commit_ids(commit_ids)?))
             }
             ResolvedExpression::Ancestors { heads, generation } => {
                 let head_set = self.evaluate(heads)?;
@@ -966,14 +967,28 @@ impl<'index> EvaluationContext<'index> {
         }
     }
 
-    fn revset_for_commit_ids(&self, commit_ids: &[CommitId]) -> EagerRevset {
-        let mut positions = commit_ids
+    fn revset_for_commit_ids(
+        &self,
+        commit_ids: &[CommitId],
+    ) -> Result<EagerRevset, RevsetEvaluationError> {
+        let mut positions: Vec<_> = commit_ids
             .iter()
-            .map(|id| self.index.commit_id_to_pos(id).unwrap())
-            .collect_vec();
+            .map(|id| {
+                // Invalid commit IDs should be rejected by the revset frontend,
+                // but there are a few edge cases that break the precondition.
+                // For example, in jj <= 0.22, the root commit doesn't exist in
+                // the root operation.
+                self.index.commit_id_to_pos(id).ok_or_else(|| {
+                    RevsetEvaluationError::Other(format!(
+                        "Commit ID {} not found in index (index or view might be corrupted)",
+                        id.hex()
+                    ))
+                })
+            })
+            .try_collect()?;
         positions.sort_unstable_by_key(|&pos| Reverse(pos));
         positions.dedup();
-        EagerRevset { positions }
+        Ok(EagerRevset { positions })
     }
 
     fn take_latest_revset(&self, candidate_set: &dyn InternalRevset, count: usize) -> EagerRevset {
diff --git a/lib/src/revset.rs b/lib/src/revset.rs
index 4f96ad16e1..76c3030c83 100644
--- a/lib/src/revset.rs
+++ b/lib/src/revset.rs
@@ -1765,18 +1765,7 @@ fn resolve_commit_ref(
             Ok(wc_commits)
         }
         RevsetCommitRef::VisibleHeads => Ok(repo.view().heads().iter().cloned().collect_vec()),
-        RevsetCommitRef::Root => {
-            let commit_id = repo.store().root_commit_id();
-            if repo.index().has_id(commit_id) {
-                Ok(vec![commit_id.clone()])
-            } else {
-                // The root commit doesn't exist at the root operation.
-                Err(RevsetResolutionError::NoSuchRevision {
-                    name: "root()".to_owned(),
-                    candidates: vec![],
-                })
-            }
-        }
+        RevsetCommitRef::Root => Ok(vec![repo.store().root_commit_id().clone()]),
         RevsetCommitRef::Bookmarks(pattern) => {
             let commit_ids = repo
                 .view()
diff --git a/lib/tests/test_revset.rs b/lib/tests/test_revset.rs
index 6ec652150a..540fbb1d16 100644
--- a/lib/tests/test_revset.rs
+++ b/lib/tests/test_revset.rs
@@ -971,6 +971,10 @@ fn test_evaluate_expression_root_and_checkout() {
         resolve_commit_ids_in_workspace(mut_repo, "@", &test_workspace.workspace, None),
         vec![commit1.id().clone()]
     );
+
+    // Shouldn't panic by unindexed commit ID
+    let expression = RevsetExpression::commit(commit1.id().clone()).resolve_programmatic(tx.repo());
+    assert!(expression.evaluate(tx.base_repo().as_ref()).is_err());
 }
 
 #[test]