evolog: Implement --reversed flag when showing the graph

While this appears functional, I'm considering this a draft change because of
how clunky the reversal logic is. Ultimately, all I want is the logic in 
`ReverseGraphIterator::new` for reversing the DAG, but the `Iterator` impl
posed a problem to me.

I first convert the list of `Commits` into a `Vec` of `GraphNodes` so that I
can use the `ReverseGraphIterator` (RGE) constructor to reverse the DAG. RGE
requires each element to be wrapped in a Result, but there  didn't seem to be a
meaningful error type so I set the error type to `()`.

I then added a generic type to the `new` method, so that RGE can tolerate my
usage of `()` while still continuing to work elsewhere.

This is where I run into a problem. The `Iterator` implementation for RGE 
hardcodes the Item type to `Result<GraphNode<N>, RevsetEvaluationError>`, but
the normal forward iterator has its error type set to `()`. And even if it
wasn't set to `()` it would be something other than `RevsetEvaluationError`.

This makes the types of the forward and backward iterators mismatched since 
they return different kinds of Results for each Item.

Really, the `Iterator` implementation for RGE doesn't even care what the 
error type is, so it ought to be able to be "generified". But, I got very stuck
trying to reorganize the types without affecting existing code too much.

In the interest of getting something working, this change unwraps all the
results. This handles the mistmatched error types and unifies the type signature
of the iterator regardless of whether we're reversed or not.

I don't think this is a super great approach, but I figured I'd get some code
in front of maintainers to get some guidance / fix it during review.

cli/src/commands/evolog.rs

@@ -16,6 +16,10 @@ use clap_complete::ArgValueCandidates;
 use itertools::Itertools;
 use jj_lib::commit::Commit;
 use jj_lib::dag_walk::topo_order_reverse_ok;
+use jj_lib::graph::GraphEdge;
+use jj_lib::graph::GraphEdgeType;
+use jj_lib::graph::GraphNode;
+use jj_lib::graph::ReverseGraphIterator;
 use jj_lib::matchers::EverythingMatcher;
 use tracing::instrument;
 
@@ -154,14 +158,53 @@ pub(crate) fn cmd_evolog(
     if !args.no_graph {
         let mut raw_output = formatter.raw()?;
         let mut graph = get_graphlog(graph_style, raw_output.as_mut());
-        for commit in commits {
-            let edges = commit
-                .predecessor_ids()
-                .iter()
-                .map(|id| Edge::Direct(id.clone()))
-                .collect_vec();
+
+        let graph_nodes: Vec<Result<GraphNode<Commit>, ()>> = commits
+            .iter()
+            .map(|c| {
+                let edges = c
+                    .predecessors()
+                    // TODO: Figure out what to do with this unwrap, if this approach works.
+                    .map(|p| GraphEdge::direct(p.unwrap().clone()))
+                    .collect_vec();
+                Ok((c.clone(), edges))
+            })
+            .collect_vec();
+
+        // TODO: This exists to strip out the Result types because ReverseGraphIterator
+        // assumes that the error type of its input iterator is RevsetEvaluationError
+        // when in this case, the input iterator is infallible (E is `()`), or at the
+        // very least a different error type. This is clunky but monkeying around in
+        // lib/src/graph proved to be a bit much for me.
+        let mut iter_nodes = vec![];
+        if args.reversed {
+            let reverse = ReverseGraphIterator::new(graph_nodes.iter().cloned()).unwrap();
+            for r in reverse {
+                iter_nodes.push(r.unwrap().clone());
+            }
+        } else {
+            for n in graph_nodes.iter() {
+                iter_nodes.push(n.as_ref().unwrap().clone());
+            }
+        }
+
+        for node in iter_nodes.iter() {
+            let (commit, edges) = node.clone();
+            let mut graphlog_edges = vec![];
+            for edge in edges {
+                match edge.edge_type {
+                    GraphEdgeType::Direct => {
+                        graphlog_edges.push(Edge::Direct(edge.target.id().clone()));
+                    }
+                    // `graph_nodes` only produces GraphEdges of type Direct, so handling
+                    // the other cases is irrelevant.
+                    // TODO: Should this even be a match then?
+                    _ => unreachable!(),
+                }
+            }
             let mut buffer = vec![];
-            let within_graph = with_content_format.sub_width(graph.width(commit.id(), &edges));
+            let within_graph =
+                with_content_format.sub_width(graph.width(commit.id(), &graphlog_edges));
             within_graph.write(ui.new_formatter(&mut buffer).as_mut(), |formatter| {
                 template.format(&commit, formatter)
             })?;
@@ -183,7 +226,7 @@ pub(crate) fn cmd_evolog(
             let node_symbol = format_template(ui, &Some(commit.clone()), &node_template);
             graph.add_node(
                 commit.id(),
-                &edges,
+                &graphlog_edges,
                 &node_symbol,
                 &String::from_utf8_lossy(&buffer),
             )?;

cli/tests/test_evolog_command.rs

@@ -370,3 +370,48 @@ fn test_evolog_reversed_no_graph() {
     (empty) c
     ");
 }
+
+#[test]
+fn test_evolog_reverse_with_graph() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "a"]);
+    test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "b"]);
+    test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "c"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "-r", "description(c)", "-m", "d"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "-r", "description(c)", "-m", "e"]);
+    test_env.jj_cmd_ok(
+        &repo_path,
+        &[
+            "squash",
+            "--from",
+            "description(d)|description(e)",
+            "--to",
+            "description(c)",
+            "-m",
+            "c+d+e",
+        ],
+    );
+    let stdout = test_env.jj_cmd_success(
+        &repo_path,
+        &["evolog", "-r", "description(c+d+e)", "--reversed"],
+    );
+    insta::assert_snapshot!(stdout, @r"
+    ○  qpvuntsm hidden [email protected] 2001-02-03 08:05:07 230dd059
+    │  (empty) (no description set)
+    ○  qpvuntsm hidden [email protected] 2001-02-03 08:05:08 d8d5f980
+    │  (empty) a
+    ○  qpvuntsm hidden [email protected] 2001-02-03 08:05:09 b4584f54
+    │  (empty) b
+    ○  qpvuntsm hidden [email protected] 2001-02-03 08:05:10 5cb22a87
+    │  (empty) c
+    │ ○  mzvwutvl hidden [email protected] 2001-02-03 08:05:11 280cbb6e
+    ├─╯  (empty) d
+    │ ○  royxmykx hidden [email protected] 2001-02-03 08:05:12 031df638
+    ├─╯  (empty) e
+    ○  qpvuntsm [email protected] 2001-02-03 08:05:13 a177c2f2
+       (empty) c+d+e
+    ");
+}

lib/src/graph.rs

@@ -82,9 +82,7 @@ impl<N> ReverseGraphIterator<N>
 where
     N: Hash + Eq + Clone,
 {
-    pub fn new(
-        input: impl Iterator<Item = Result<GraphNode<N>, RevsetEvaluationError>>,
-    ) -> Result<Self, RevsetEvaluationError> {
+    pub fn new<E>(input: impl Iterator<Item = Result<GraphNode<N>, E>>) -> Result<Self, E> {
         let mut entries = vec![];
         let mut reverse_edges: HashMap<N, Vec<GraphEdge<N>>> = HashMap::new();
         for item in input {