Skip to content

Commit

Permalink
revset: error out on unindexed commit ID instead of panicking
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
yuja committed Oct 8, 2024
1 parent ab79278 commit e4ffd3a
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 18 deletions.
27 changes: 21 additions & 6 deletions lib/src/default_index/revset_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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 {
Expand Down
13 changes: 1 addition & 12 deletions lib/src/revset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
4 changes: 4 additions & 0 deletions lib/tests/test_revset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit e4ffd3a

Please sign in to comment.