revset: pass revset, not iterator, into RevsetGraphIterator

I was thinking of replacing `RevsetIterator` by a regular
`Iterator<Item=IndexEntry>`. However, that would make it easier to
pass in an iterator that produces revisions in a non-topological order
into `RevsetGraphIterator`, which would produce unexpected results (it
would result in nodes that are not connected to their parents, if
their parents had already been emitted). I think it makes sense to
instead pass in a revset into `RevsetGraphIterator`.

Incidentally, it will also be useful to have the full revset available
in `RevsetGraphIterator` if we rewrite the algorithm to be more
similar to Mercurial's and Sapling's algorithm, which involves asking
the revset if it contains parent revisions.

lib/src/revset.rs

@@ -38,7 +38,6 @@ use crate::matchers::{EverythingMatcher, Matcher, PrefixMatcher};
 use crate::op_store::WorkspaceId;
 use crate::repo::Repo;
 use crate::repo_path::{FsPathParseError, RepoPath};
-use crate::revset_graph_iterator::RevsetGraphIterator;
 use crate::rewrite;
 use crate::store::Store;
 
@@ -1573,10 +1572,6 @@ impl<'revset, 'index> RevsetIterator<'revset, 'index> {
         }
     }
 
-    pub fn graph(self) -> RevsetGraphIterator<'revset, 'index> {
-        RevsetGraphIterator::new(self)
-    }
-
     fn into_predicate_fn(self) -> Box<dyn FnMut(&IndexEntry<'index>) -> bool + 'revset> {
         let mut iter = self.fuse().peekable();
         Box::new(move |entry| {

lib/src/revset_graph_iterator.rs

@@ -17,7 +17,7 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 
 use crate::index::{IndexEntry, IndexPosition};
 use crate::nightly_shims::BTreeMapExt;
-use crate::revset::RevsetIterator;
+use crate::revset::{Revset, RevsetIterator};
 
 #[derive(Debug, PartialEq, Eq, Clone, Hash)]
 pub struct RevsetGraphEdge {
@@ -134,9 +134,9 @@ pub struct RevsetGraphIterator<'revset, 'index> {
 }
 
 impl<'revset, 'index> RevsetGraphIterator<'revset, 'index> {
-    pub fn new(iter: RevsetIterator<'revset, 'index>) -> RevsetGraphIterator<'revset, 'index> {
+    pub fn new(revset: &'revset dyn Revset<'index>) -> RevsetGraphIterator<'revset, 'index> {
         RevsetGraphIterator {
-            input_set_iter: iter,
+            input_set_iter: revset.iter(),
             look_ahead: Default::default(),
             min_position: IndexPosition::MAX,
             edges: Default::default(),

lib/tests/test_revset_graph_iterator.rs

@@ -15,7 +15,7 @@
 use itertools::Itertools;
 use jujutsu_lib::repo::Repo;
 use jujutsu_lib::revset::revset_for_commits;
-use jujutsu_lib::revset_graph_iterator::RevsetGraphEdge;
+use jujutsu_lib::revset_graph_iterator::{RevsetGraphEdge, RevsetGraphIterator};
 use test_case::test_case;
 use testutils::{CommitGraphBuilder, TestRepo};
 
@@ -49,9 +49,7 @@ fn test_graph_iterator_linearized(skip_transitive_edges: bool) {
     let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
 
     let revset = revset_for_commits(&repo, &[&commit_a, &commit_d]);
-    let commits = revset
-        .iter()
-        .graph()
+    let commits = RevsetGraphIterator::new(revset.as_ref())
         .set_skip_transitive_edges(skip_transitive_edges)
         .collect_vec();
     assert_eq!(commits.len(), 2);
@@ -97,9 +95,7 @@ fn test_graph_iterator_virtual_octopus(skip_transitive_edges: bool) {
     let pos_c = repo.index().commit_id_to_pos(commit_c.id()).unwrap();
 
     let revset = revset_for_commits(&repo, &[&commit_a, &commit_b, &commit_c, &commit_f]);
-    let commits = revset
-        .iter()
-        .graph()
+    let commits = RevsetGraphIterator::new(revset.as_ref())
         .set_skip_transitive_edges(skip_transitive_edges)
         .collect_vec();
     assert_eq!(commits.len(), 4);
@@ -154,9 +150,7 @@ fn test_graph_iterator_simple_fork(skip_transitive_edges: bool) {
     let pos_a = repo.index().commit_id_to_pos(commit_a.id()).unwrap();
 
     let revset = revset_for_commits(&repo, &[&commit_a, &commit_c, &commit_e]);
-    let commits = revset
-        .iter()
-        .graph()
+    let commits = RevsetGraphIterator::new(revset.as_ref())
         .set_skip_transitive_edges(skip_transitive_edges)
         .collect_vec();
     assert_eq!(commits.len(), 3);
@@ -203,9 +197,7 @@ fn test_graph_iterator_multiple_missing(skip_transitive_edges: bool) {
     let pos_c = repo.index().commit_id_to_pos(commit_c.id()).unwrap();
 
     let revset = revset_for_commits(&repo, &[&commit_b, &commit_f]);
-    let commits = revset
-        .iter()
-        .graph()
+    let commits = RevsetGraphIterator::new(revset.as_ref())
         .set_skip_transitive_edges(skip_transitive_edges)
         .collect_vec();
     assert_eq!(commits.len(), 2);
@@ -257,9 +249,7 @@ fn test_graph_iterator_edge_to_ancestor(skip_transitive_edges: bool) {
     let pos_d = repo.index().commit_id_to_pos(commit_d.id()).unwrap();
 
     let revset = revset_for_commits(&repo, &[&commit_c, &commit_d, &commit_f]);
-    let commits = revset
-        .iter()
-        .graph()
+    let commits = RevsetGraphIterator::new(revset.as_ref())
         .set_skip_transitive_edges(skip_transitive_edges)
         .collect_vec();
     assert_eq!(commits.len(), 3);
@@ -337,9 +327,7 @@ fn test_graph_iterator_edge_escapes_from_(skip_transitive_edges: bool) {
         &repo,
         &[&commit_a, &commit_d, &commit_g, &commit_h, &commit_j],
     );
-    let commits = revset
-        .iter()
-        .graph()
+    let commits = RevsetGraphIterator::new(revset.as_ref())
         .set_skip_transitive_edges(skip_transitive_edges)
         .collect_vec();
     assert_eq!(commits.len(), 5);
@@ -420,7 +408,9 @@ fn test_reverse_graph_iterator() {
         &repo,
         &[&commit_a, &commit_c, &commit_d, &commit_e, &commit_f],
     );
-    let commits = revset.iter().graph().reversed().collect_vec();
+    let commits = RevsetGraphIterator::new(revset.as_ref())
+        .reversed()
+        .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());

src/commands/mod.rs

@@ -36,7 +36,9 @@ 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};
-use jujutsu_lib::revset_graph_iterator::{RevsetGraphEdge, RevsetGraphEdgeType};
+use jujutsu_lib::revset_graph_iterator::{
+    RevsetGraphEdge, RevsetGraphEdgeType, RevsetGraphIterator,
+};
 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};
@@ -1433,9 +1435,9 @@ fn cmd_log(ui: &mut Ui, command: &CommandHelper, args: &LogArgs) -> Result<(), C
             let mut graph = get_graphlog(command.settings(), formatter.raw());
             let iter: Box<dyn Iterator<Item = (IndexEntry, Vec<RevsetGraphEdge>)>> =
                 if args.reversed {
-                    Box::new(revset.iter().graph().reversed())
+                    Box::new(RevsetGraphIterator::new(revset.as_ref()).reversed())
                 } else {
-                    Box::new(revset.iter().graph())
+                    Box::new(RevsetGraphIterator::new(revset.as_ref()))
                 };
             for (index_entry, edges) in iter {
                 let mut graphlog_edges = vec![];