Skip to content

Commit

Permalink
revset: allow iterators to return evaluation errors
Browse files Browse the repository at this point in the history
Custom backends may rely on networking or other unreliable implementations to support revsets, this change allows them to return errors cleanly instead of panicking.

For simplicity, only the public-facing Revset and RevsetGraph types are changed in this commit; the internal revset engine remains mostly unchanged and error-free since it cannot generally produce errors.
  • Loading branch information
torquestomp committed Oct 18, 2024
1 parent c970181 commit 49e9003
Show file tree
Hide file tree
Showing 23 changed files with 265 additions and 164 deletions.
2 changes: 2 additions & 0 deletions cli/examples/custom-commit-templater/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ impl MostDigitsInId {
.evaluate_programmatic(repo)
.unwrap()
.iter()
.map(Result::unwrap)
.map(|id| num_digits_in_id(&id))
.max()
.unwrap_or(0)
Expand Down Expand Up @@ -102,6 +103,7 @@ impl PartialSymbolResolver for TheDigitestResolver {
.evaluate_programmatic(repo)
.map_err(|err| RevsetResolutionError::Other(err.into()))?
.iter()
.map(Result::unwrap)
.filter(|id| num_digits_in_id(id) == self.cache.count(repo))
.collect_vec(),
))
Expand Down
1 change: 1 addition & 0 deletions cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,7 @@ impl WorkspaceCommandEnvironment {
})?;

if let Some(commit_id) = commit_id_iter.next() {
let commit_id = commit_id?;
let error = if &commit_id == repo.store().root_commit_id() {
user_error(format!(
"The root commit {} is immutable",
Expand Down
4 changes: 3 additions & 1 deletion cli/src/commands/bookmark/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

use std::collections::HashSet;

use itertools::Itertools;
use jj_lib::git;
use jj_lib::revset::RevsetExpression;
use jj_lib::str_util::StringPattern;
Expand Down Expand Up @@ -100,7 +101,8 @@ pub fn cmd_bookmark_list(
// Intersects with the set of local bookmark targets to minimize the lookup
// space.
expression.intersect_with(&RevsetExpression::bookmarks(StringPattern::everything()));
let filtered_targets: HashSet<_> = expression.evaluate_to_commit_ids()?.collect();
let filtered_targets: HashSet<_> =
expression.evaluate_to_commit_ids()?.try_collect()?;
bookmark_names.extend(
view.local_bookmarks()
.filter(|(_, target)| {
Expand Down
3 changes: 2 additions & 1 deletion cli/src/commands/debug/revset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use std::fmt::Debug;
use std::io::Write as _;

use jj_lib::object_id::ObjectId;
use jj_lib::revset;
use jj_lib::revset::RevsetDiagnostics;

Expand Down Expand Up @@ -68,7 +69,7 @@ pub fn cmd_debug_revset(

writeln!(ui.stdout(), "-- Commit IDs:")?;
for commit_id in revset.iter() {
writeln!(ui.stdout(), "{commit_id}")?;
writeln!(ui.stdout(), "{}", commit_id?.hex())?;
}
Ok(())
}
3 changes: 2 additions & 1 deletion cli/src/commands/duplicate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use std::io::Write;

use indexmap::IndexMap;
use itertools::Itertools;
use jj_lib::backend::CommitId;
use jj_lib::commit::Commit;
use jj_lib::repo::Repo;
Expand Down Expand Up @@ -48,7 +49,7 @@ pub(crate) fn cmd_duplicate(
let to_duplicate: Vec<CommitId> = workspace_command
.parse_union_revsets(ui, &args.revisions)?
.evaluate_to_commit_ids()?
.collect(); // in reverse topological order
.try_collect()?; // in reverse topological order
if to_duplicate.is_empty() {
writeln!(ui.status(), "No revisions to duplicate.")?;
return Ok(());
Expand Down
2 changes: 1 addition & 1 deletion cli/src/commands/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ pub(crate) fn cmd_fix(
workspace_command.parse_union_revsets(ui, &args.source)?
}
.evaluate_to_commit_ids()?
.collect();
.try_collect()?;
workspace_command.check_rewritable(root_commits.iter())?;
let matcher = workspace_command
.parse_file_patterns(ui, &args.paths)?
Expand Down
8 changes: 6 additions & 2 deletions cli/src/commands/git/push.rs
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,9 @@ fn find_bookmarks_targeted_by_revisions<'a>(
.intersection(&RevsetExpression::bookmarks(StringPattern::everything()));
let current_bookmarks_revset = current_bookmarks_expression
.evaluate_programmatic(workspace_command.repo().as_ref())?;
revision_commit_ids.extend(current_bookmarks_revset.iter());
for commit_id in current_bookmarks_revset.iter() {
revision_commit_ids.insert(commit_id?);
}
if revision_commit_ids.is_empty() {
writeln!(
ui.warning_default(),
Expand All @@ -631,7 +633,9 @@ fn find_bookmarks_targeted_by_revisions<'a>(
"No bookmarks point to the specified revisions: {rev_arg}"
)?;
}
revision_commit_ids.extend(commit_ids);
for commit_id in commit_ids {
revision_commit_ids.insert(commit_id?);
}
}
let bookmarks_targeted = workspace_command
.repo()
Expand Down
18 changes: 11 additions & 7 deletions cli/src/commands/log.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use jj_lib::graph::GraphEdgeType;
use jj_lib::graph::ReverseGraphIterator;
use jj_lib::graph::TopoGroupedGraphIterator;
use jj_lib::repo::Repo;
use jj_lib::revset::RevsetEvaluationError;
use jj_lib::revset::RevsetExpression;
use jj_lib::revset::RevsetFilterPredicate;
use jj_lib::revset::RevsetIteratorExt;
Expand Down Expand Up @@ -173,11 +174,13 @@ pub(crate) fn cmd_log(
let mut graph = get_graphlog(graph_style, raw_output.as_mut());
let forward_iter = TopoGroupedGraphIterator::new(revset.iter_graph());
let iter: Box<dyn Iterator<Item = _>> = if args.reversed {
Box::new(ReverseGraphIterator::new(forward_iter))
Box::new(ReverseGraphIterator::new(forward_iter)?)
} else {
Box::new(forward_iter)
};
for (commit_id, edges) in iter.take(limit) {
for node in iter.take(limit) {
let (commit_id, edges) = node?;

// The graph is keyed by (CommitId, is_synthetic)
let mut graphlog_edges = vec![];
// TODO: Should we update revset.iter_graph() to yield this flag instead of all
Expand Down Expand Up @@ -255,11 +258,12 @@ pub(crate) fn cmd_log(
}
}
} else {
let iter: Box<dyn Iterator<Item = CommitId>> = if args.reversed {
Box::new(revset.iter().reversed())
} else {
Box::new(revset.iter())
};
let iter: Box<dyn Iterator<Item = Result<CommitId, RevsetEvaluationError>>> =
if args.reversed {
Box::new(revset.iter().reversed()?)
} else {
Box::new(revset.iter())
};
for commit_or_error in iter.commits(store).take(limit) {
let commit = commit_or_error?;
with_content_format
Expand Down
1 change: 1 addition & 0 deletions cli/src/commands/new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ fn ensure_no_commit_loop(
.iter()
.next()
{
let commit_id = commit_id?;
return Err(user_error(format!(
"Refusing to create a loop: commit {} would be both an ancestor and a descendant of \
the new commit",
Expand Down
15 changes: 9 additions & 6 deletions cli/src/commands/operation/diff.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

use std::collections::HashMap;
use std::convert::Infallible;
use std::sync::Arc;

use indexmap::IndexMap;
Expand Down Expand Up @@ -198,19 +199,21 @@ pub fn show_op_diff(
let mut raw_output = formatter.raw()?;
let mut graph = get_graphlog(graph_style, raw_output.as_mut());

let graph_iter =
TopoGroupedGraphIterator::new(ordered_change_ids.iter().map(|change_id| {
let graph_iter = TopoGroupedGraphIterator::new(ordered_change_ids.iter().map(
|change_id| -> Result<_, Infallible> {
let parent_change_ids = change_parents.get(change_id).unwrap();
(
Ok((
change_id.clone(),
parent_change_ids
.iter()
.map(|parent_change_id| GraphEdge::direct(parent_change_id.clone()))
.collect_vec(),
)
}));
))
},
));

for (change_id, edges) in graph_iter {
for node in graph_iter {
let (change_id, edges) = node.unwrap();
let modified_change = changes.get(&change_id).unwrap();
let edges = edges
.iter()
Expand Down
1 change: 1 addition & 0 deletions cli/src/commands/rebase.rs
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,7 @@ fn ensure_no_commit_loop(
.iter()
.next()
{
let commit_id = commit_id?;
return Err(user_error(format!(
"Refusing to create a loop: commit {} would be both an ancestor and a descendant of \
the rebased commits",
Expand Down
4 changes: 2 additions & 2 deletions cli/src/commands/simplify_parents.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ pub(crate) fn cmd_simplify_parents(
.parse_union_revsets(ui, &args.revisions)?
.expression(),
);
let commit_ids = workspace_command
let commit_ids: Vec<_> = workspace_command
.attach_revset_evaluator(revs)
.evaluate_to_commit_ids()?
.collect_vec();
.try_collect()?;
workspace_command.check_rewritable(&commit_ids)?;
let commit_ids_set: HashSet<_> = commit_ids.iter().cloned().collect();
let num_orig_commits = commit_ids.len();
Expand Down
4 changes: 2 additions & 2 deletions cli/src/commands/status.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ pub(crate) fn cmd_status(
let wc_revset = RevsetExpression::commit(wc_commit.id().clone());

// Ancestors with conflicts, excluding the current working copy commit.
let ancestors_conflicts = workspace_command
let ancestors_conflicts: Vec<_> = workspace_command
.attach_revset_evaluator(
wc_revset
.parents()
Expand All @@ -120,7 +120,7 @@ pub(crate) fn cmd_status(
.minus(&workspace_command.env().immutable_expression()),
)
.evaluate_to_commit_ids()?
.collect();
.try_collect()?;

workspace_command.report_repo_conflicts(formatter, repo, ancestors_conflicts)?;
} else {
Expand Down
5 changes: 4 additions & 1 deletion cli/src/revset_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,10 @@ impl<'repo> RevsetExpressionEvaluator<'repo> {
/// sorted in reverse topological order.
pub fn evaluate_to_commit_ids(
&self,
) -> Result<Box<dyn Iterator<Item = CommitId> + 'repo>, UserRevsetEvaluationError> {
) -> Result<
Box<dyn Iterator<Item = Result<CommitId, RevsetEvaluationError>> + 'repo>,
UserRevsetEvaluationError,
> {
Ok(self.evaluate()?.iter())
}

Expand Down
8 changes: 5 additions & 3 deletions lib/src/annotate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use crate::graph::GraphEdgeType;
use crate::merged_tree::MergedTree;
use crate::repo::Repo;
use crate::repo_path::RepoPath;
use crate::revset::RevsetEvaluationError;
use crate::revset::RevsetExpression;
use crate::revset::RevsetFilterPredicate;
use crate::store::Store;
Expand Down Expand Up @@ -132,7 +133,7 @@ pub fn get_annotation_for_file(
repo: &dyn Repo,
starting_commit: &Commit,
file_path: &RepoPath,
) -> Result<AnnotateResults, BackendError> {
) -> Result<AnnotateResults, RevsetEvaluationError> {
let original_contents =
get_file_contents(starting_commit.store(), file_path, &starting_commit.tree()?)?;
let num_lines = original_contents.split_inclusive(|b| *b == b'\n').count();
Expand All @@ -154,7 +155,7 @@ fn process_commits(
starting_commit_id: &CommitId,
file_name: &RepoPath,
num_lines: usize,
) -> Result<OriginalLineMap, BackendError> {
) -> Result<OriginalLineMap, RevsetEvaluationError> {
let predicate = RevsetFilterPredicate::File(FilesetExpression::file_path(file_name.to_owned()));
let revset = RevsetExpression::commit(starting_commit_id.clone())
.union(
Expand All @@ -167,7 +168,8 @@ fn process_commits(
let mut commit_line_map = get_initial_commit_line_map(starting_commit_id, num_lines);
let mut original_line_map = HashMap::new();

for (cid, edge_list) in revset.iter_graph() {
for node in revset.iter_graph() {
let (cid, edge_list) = node?;
let current_commit = repo.store().get_commit(&cid)?;
process_commit(
repo,
Expand Down
20 changes: 13 additions & 7 deletions lib/src/default_index/revset_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ use crate::default_index::AsCompositeIndex;
use crate::default_index::CompositeIndex;
use crate::default_index::IndexPosition;
use crate::graph::GraphEdge;
use crate::graph::GraphNodeResult;
use crate::matchers::Matcher;
use crate::matchers::Visit;
use crate::merged_tree::resolve_file_values;
Expand Down Expand Up @@ -130,11 +131,12 @@ impl<I: AsCompositeIndex + Clone> RevsetImpl<I> {
pub fn iter_graph_impl(
&self,
skip_transitive_edges: bool,
) -> impl Iterator<Item = (CommitId, Vec<GraphEdge<CommitId>>)> {
) -> impl Iterator<Item = Result<(CommitId, Vec<GraphEdge<CommitId>>), RevsetEvaluationError>>
{
let index = self.index.clone();
let walk = self.inner.positions();
let mut graph_walk = RevsetGraphWalk::new(walk, skip_transitive_edges);
iter::from_fn(move || graph_walk.next(index.as_composite()))
iter::from_fn(move || graph_walk.next(index.as_composite()).map(Ok))
}
}

Expand All @@ -147,7 +149,7 @@ impl<I> fmt::Debug for RevsetImpl<I> {
}

impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
fn iter<'a>(&self) -> Box<dyn Iterator<Item = CommitId> + 'a>
fn iter<'a>(&self) -> Box<dyn Iterator<Item = Result<CommitId, RevsetEvaluationError>> + 'a>
where
Self: 'a,
{
Expand All @@ -156,11 +158,13 @@ impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
Box::new(iter::from_fn(move || {
let index = index.as_composite();
let pos = walk.next(index)?;
Some(index.entry_by_pos(pos).commit_id())
Some(Ok(index.entry_by_pos(pos).commit_id()))
}))
}

fn commit_change_ids<'a>(&self) -> Box<dyn Iterator<Item = (CommitId, ChangeId)> + 'a>
fn commit_change_ids<'a>(
&self,
) -> Box<dyn Iterator<Item = Result<(CommitId, ChangeId), RevsetEvaluationError>> + 'a>
where
Self: 'a,
{
Expand All @@ -170,11 +174,13 @@ impl<I: AsCompositeIndex + Clone> Revset for RevsetImpl<I> {
let index = index.as_composite();
let pos = walk.next(index)?;
let entry = index.entry_by_pos(pos);
Some((entry.commit_id(), entry.change_id()))
Some(Ok((entry.commit_id(), entry.change_id())))
}))
}

fn iter_graph<'a>(&self) -> Box<dyn Iterator<Item = (CommitId, Vec<GraphEdge<CommitId>>)> + 'a>
fn iter_graph<'a>(
&self,
) -> Box<dyn Iterator<Item = GraphNodeResult<CommitId, RevsetEvaluationError>> + 'a>
where
Self: 'a,
{
Expand Down
1 change: 1 addition & 0 deletions lib/src/git.rs
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ fn abandon_unreachable_commits(
.evaluate_programmatic(mut_repo)
.unwrap()
.iter()
.map(Result::unwrap) // TODO: Return error to caller
.collect_vec();
for abandoned_commit in &abandoned_commits {
mut_repo.record_abandoned_commit(abandoned_commit.clone());
Expand Down
Loading

0 comments on commit 49e9003

Please sign in to comment.