From f62fac24ac3dc739d5b72df2c08947582925ac32 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 13 Mar 2023 07:17:17 -0700 Subject: [PATCH] revset: move graph iteration onto Revset trait We want to allow custom revset engines define their own graph iterator. This commit helps with that by adding a `Revset::iter_graph()` function that returns an abstract iterator. The current `RevsetGraphIterator` can be configured to skip or include transitive edges. It skips them by default and we don't expose option in the CLI. I didn't bother including that functionality in the new `iter_graph()` either. At least for now, it will be up to the implementation whether it includes such edges (it would of course be free to ignore the caller's request even if we added an option for it in the API). --- lib/src/default_revset_engine.rs | 11 +++++-- lib/src/revset.rs | 40 +++++++++++++++++++++- lib/src/revset_graph_iterator.rs | 44 +++---------------------- lib/tests/test_revset_graph_iterator.rs | 7 ++-- src/commands/mod.rs | 11 +++---- 5 files changed, 60 insertions(+), 53 deletions(-) diff --git a/lib/src/default_revset_engine.rs b/lib/src/default_revset_engine.rs index 6b6074459..a603e54f9 100644 --- a/lib/src/default_revset_engine.rs +++ b/lib/src/default_revset_engine.rs @@ -27,9 +27,10 @@ use crate::matchers::{EverythingMatcher, Matcher, PrefixMatcher}; use crate::op_store::WorkspaceId; use crate::repo::Repo; use crate::revset::{ - Revset, RevsetError, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, - RevsetWorkspaceContext, GENERATION_RANGE_FULL, + Revset, RevsetError, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, + RevsetIteratorExt, RevsetWorkspaceContext, GENERATION_RANGE_FULL, }; +use crate::revset_graph_iterator::RevsetGraphIterator; use crate::rewrite; fn resolve_git_ref(repo: &dyn Repo, symbol: &str) -> Option> { @@ -215,6 +216,12 @@ impl<'index> Revset<'index> for RevsetImpl<'index> { self.inner.iter() } + fn iter_graph( + &self, + ) -> Box, Vec)> + '_> { + Box::new(RevsetGraphIterator::new(self)) + } + fn is_empty(&self) -> bool { self.iter().next().is_none() } diff --git a/lib/src/revset.rs b/lib/src/revset.rs index 13d34d3e7..483b11a5f 100644 --- a/lib/src/revset.rs +++ b/lib/src/revset.rs @@ -29,7 +29,7 @@ use thiserror::Error; use crate::backend::{BackendError, BackendResult, CommitId}; use crate::commit::Commit; -use crate::default_index_store::IndexEntry; +use crate::default_index_store::{IndexEntry, IndexPosition}; use crate::op_store::WorkspaceId; use crate::repo::Repo; use crate::repo_path::{FsPathParseError, RepoPath}; @@ -1377,9 +1377,47 @@ pub trait Revset<'index> { // All revsets currently iterate in order of descending index position fn iter(&self) -> Box> + '_>; + fn iter_graph( + &self, + ) -> Box, Vec)> + '_>; + fn is_empty(&self) -> bool; } +#[derive(Debug, PartialEq, Eq, Clone, Hash)] +pub struct RevsetGraphEdge { + pub target: IndexPosition, + pub edge_type: RevsetGraphEdgeType, +} + +impl RevsetGraphEdge { + pub fn missing(target: IndexPosition) -> Self { + Self { + target, + edge_type: RevsetGraphEdgeType::Missing, + } + } + pub fn direct(target: IndexPosition) -> Self { + Self { + target, + edge_type: RevsetGraphEdgeType::Direct, + } + } + pub fn indirect(target: IndexPosition) -> Self { + Self { + target, + edge_type: RevsetGraphEdgeType::Indirect, + } + } +} + +#[derive(Debug, PartialEq, Eq, Clone, Hash)] +pub enum RevsetGraphEdgeType { + Missing, + Direct, + Indirect, +} + pub trait RevsetIteratorExt<'index, I> { fn commit_ids(self) -> RevsetCommitIdIterator; fn commits(self, store: &Arc) -> RevsetCommitIterator; diff --git a/lib/src/revset_graph_iterator.rs b/lib/src/revset_graph_iterator.rs index 89d38b1af..47b04a70f 100644 --- a/lib/src/revset_graph_iterator.rs +++ b/lib/src/revset_graph_iterator.rs @@ -17,41 +17,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use crate::default_index_store::{IndexEntry, IndexPosition}; use crate::nightly_shims::BTreeMapExt; -use crate::revset::Revset; - -#[derive(Debug, PartialEq, Eq, Clone, Hash)] -pub struct RevsetGraphEdge { - pub target: IndexPosition, - pub edge_type: RevsetGraphEdgeType, -} - -impl RevsetGraphEdge { - pub fn missing(target: IndexPosition) -> Self { - Self { - target, - edge_type: RevsetGraphEdgeType::Missing, - } - } - pub fn direct(target: IndexPosition) -> Self { - Self { - target, - edge_type: RevsetGraphEdgeType::Direct, - } - } - pub fn indirect(target: IndexPosition) -> Self { - Self { - target, - edge_type: RevsetGraphEdgeType::Indirect, - } - } -} - -#[derive(Debug, PartialEq, Eq, Clone, Hash)] -pub enum RevsetGraphEdgeType { - Missing, - Direct, - Indirect, -} +use crate::revset::{Revset, RevsetGraphEdge, RevsetGraphEdgeType}; // Given an iterator over some set of revisions, yields the same revisions with // associated edge types. @@ -149,10 +115,6 @@ impl<'revset, 'index> RevsetGraphIterator<'revset, 'index> { self } - pub fn reversed(self) -> ReverseRevsetGraphIterator<'index> { - ReverseRevsetGraphIterator::new(self) - } - fn next_index_entry(&mut self) -> Option> { if let Some(index_entry) = self.look_ahead.pop_last_value() { return Some(index_entry); @@ -314,7 +276,9 @@ pub struct ReverseRevsetGraphIterator<'index> { } impl<'index> ReverseRevsetGraphIterator<'index> { - fn new<'revset>(input: RevsetGraphIterator<'revset, 'index>) -> Self { + pub fn new<'revset>( + input: Box, Vec)> + 'revset>, + ) -> Self { let mut entries = vec![]; let mut reverse_edges: HashMap> = HashMap::new(); for (entry, edges) in input { diff --git a/lib/tests/test_revset_graph_iterator.rs b/lib/tests/test_revset_graph_iterator.rs index 7ecaaffae..49816bb60 100644 --- a/lib/tests/test_revset_graph_iterator.rs +++ b/lib/tests/test_revset_graph_iterator.rs @@ -15,7 +15,8 @@ use itertools::Itertools; use jujutsu_lib::default_revset_engine::revset_for_commits; use jujutsu_lib::repo::Repo; -use jujutsu_lib::revset_graph_iterator::{RevsetGraphEdge, RevsetGraphIterator}; +use jujutsu_lib::revset::RevsetGraphEdge; +use jujutsu_lib::revset_graph_iterator::{ReverseRevsetGraphIterator, RevsetGraphIterator}; use test_case::test_case; use testutils::{CommitGraphBuilder, TestRepo}; @@ -408,9 +409,7 @@ fn test_reverse_graph_iterator() { &repo, &[&commit_a, &commit_c, &commit_d, &commit_e, &commit_f], ); - let commits = RevsetGraphIterator::new(revset.as_ref()) - .reversed() - .collect_vec(); + let commits = ReverseRevsetGraphIterator::new(revset.iter_graph()).collect_vec(); assert_eq!(commits.len(), 5); assert_eq!(commits[0].0.commit_id(), *commit_a.id()); assert_eq!(commits[1].0.commit_id(), *commit_c.id()); diff --git a/src/commands/mod.rs b/src/commands/mod.rs index dd751ff89..00dfb4ba8 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -36,11 +36,10 @@ use jujutsu_lib::op_store::{RefTarget, WorkspaceId}; use jujutsu_lib::repo::{ReadonlyRepo, Repo}; use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::revset::{ - RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, -}; -use jujutsu_lib::revset_graph_iterator::{ - RevsetGraphEdge, RevsetGraphEdgeType, RevsetGraphIterator, + RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetGraphEdge, + RevsetGraphEdgeType, RevsetIteratorExt, }; +use jujutsu_lib::revset_graph_iterator::ReverseRevsetGraphIterator; use jujutsu_lib::rewrite::{back_out_commit, merge_commit_trees, rebase_commit, DescendantRebaser}; use jujutsu_lib::settings::UserSettings; use jujutsu_lib::tree::{merge_trees, Tree}; @@ -1487,9 +1486,9 @@ fn cmd_log(ui: &mut Ui, command: &CommandHelper, args: &LogArgs) -> Result<(), C let default_node_symbol = graph.default_node_symbol().to_owned(); let iter: Box)>> = if args.reversed { - Box::new(RevsetGraphIterator::new(revset.as_ref()).reversed()) + Box::new(ReverseRevsetGraphIterator::new(revset.iter_graph())) } else { - Box::new(RevsetGraphIterator::new(revset.as_ref())) + revset.iter_graph() }; for (index_entry, edges) in iter { let mut graphlog_edges = vec![];