From 2b36f40fb852623e82a8b8cb2707fb530df663a5 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Sun, 16 Jul 2023 19:47:46 +0900 Subject: [PATCH] revset_graph: group commits topologically The original idea was similar to Mercurial's "topo" sorting, but it was bad at handling merge-heavy history. In order to render merges of topic branches nicely, we need to prioritize branches at merge point, not at fork point. OTOH, we do also want to place unmerged branches as close to the fork point as possible. This commit implements the former requirement, and the latter will be addressed by the next commit. I think this is similar to Git's sorting logic described in the following blog post. In our case, the in-degree walk can be dumb since topological order is guaranteed by the index. We keep HashSet instead of an in-degree integer value, which will be used in the next commit to resolve new heads as late as possible. https://github.blog/2022-08-30-gits-database-internals-ii-commit-history-queries/#topological-sorting Compared to Sapling's beautify_graph(), this is lazy, and can roughly preserve the index (or chronological) order. I tried beautify_graph() with prioritizing the @ commit, but the result seemed too aggressively reordered. Perhaps, for more complex history, beautify_graph() would produce a better result. For my wip branches (~30 branches, a couple of commits per branch), this works pretty well. #242 --- CHANGELOG.md | 3 + lib/src/revset_graph.rs | 834 +++++++++++++++++++++++++++++++- src/commands/mod.rs | 12 +- tests/test_abandon_command.rs | 2 +- tests/test_chmod_command.rs | 10 +- tests/test_duplicate_command.rs | 50 +- tests/test_git_colocated.rs | 2 +- tests/test_new_command.rs | 80 +-- tests/test_rebase_command.rs | 28 +- tests/test_resolve_command.rs | 20 +- tests/test_restore_command.rs | 2 +- tests/test_squash_command.rs | 4 +- tests/test_templater.rs | 4 +- tests/test_unsquash_command.rs | 4 +- 14 files changed, 944 insertions(+), 111 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f16c56d4ce0..5029fad8356 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### New features +* `jj log` output is now topologically grouped. + [#242](https://github.com/martinvonz/jj/issues/242) + ### Fixed bugs ## [0.8.0] - 2023-07-09 diff --git a/lib/src/revset_graph.rs b/lib/src/revset_graph.rs index a7ff245dffd..9b4cc9e54a7 100644 --- a/lib/src/revset_graph.rs +++ b/lib/src/revset_graph.rs @@ -14,7 +14,7 @@ #![allow(missing_docs)] -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use crate::backend::CommitId; @@ -46,7 +46,6 @@ impl RevsetGraphEdge { } } - #[cfg(test)] // TODO: remove fn is_reachable(&self) -> bool { self.edge_type != RevsetGraphEdgeType::Missing } @@ -59,7 +58,6 @@ pub enum RevsetGraphEdgeType { Indirect, } -#[cfg(test)] // TODO: remove fn reachable_targets(edges: &[RevsetGraphEdge]) -> impl DoubleEndedIterator { edges .iter() @@ -106,6 +104,127 @@ impl Iterator for ReverseRevsetGraphIterator { } } +/// Graph iterator adapter to group topological branches. +/// +/// Basic idea is DFS from the heads. At fork point, the other descendant +/// branches will be visited. At merge point, the second (or the last) ancestor +/// branch will be visited first. This is practically [the same as Git][Git]. +/// +/// [Git]: https://github.blog/2022-08-30-gits-database-internals-ii-commit-history-queries/#topological-sorting +#[derive(Clone, Debug)] +pub struct TopoGroupedRevsetGraphIterator { + input_iter: I, + /// Graph nodes read from the input iterator but not yet emitted. + nodes: HashMap, + /// Stack of graph nodes to be emitted. + emittable_ids: Vec, + /// List of new head nodes found while processing unpopulated nodes. + new_head_ids: Vec, +} + +#[derive(Clone, Debug, Default)] +struct TopoGroupedGraphNode { + /// Graph nodes which must be emitted before. + child_ids: HashSet, + /// Graph edges to parent nodes. `None` until this node is populated. + edges: Option>, +} + +impl TopoGroupedRevsetGraphIterator +where + I: Iterator)>, +{ + /// Wraps the given iterator to group topological branches. The input + /// iterator must be topologically ordered. + pub fn new(input_iter: I) -> Self { + TopoGroupedRevsetGraphIterator { + input_iter, + nodes: HashMap::new(), + emittable_ids: Vec::new(), + new_head_ids: Vec::new(), + } + } + + #[must_use] + fn populate_one(&mut self) -> Option<()> { + let (current_id, edges) = self.input_iter.next()?; + + // Set up reverse reference + for parent_id in reachable_targets(&edges) { + let parent_node = self.nodes.entry(parent_id.clone()).or_default(); + parent_node.child_ids.insert(current_id.clone()); + } + + // Populate the current node + if let Some(current_node) = self.nodes.get_mut(¤t_id) { + assert!(current_node.edges.is_none()); + current_node.edges = Some(edges); + } else { + let current_node = TopoGroupedGraphNode { + edges: Some(edges), + ..Default::default() + }; + self.nodes.insert(current_id.clone(), current_node); + // Push to non-emitting list so the new head wouldn't be interleaved + self.new_head_ids.push(current_id); + } + + Some(()) + } + + #[must_use] + fn next_node(&mut self) -> Option<(CommitId, Vec)> { + // Based on Kahn's algorithm + loop { + if let Some(current_id) = self.emittable_ids.last() { + let current_node = self.nodes.get_mut(current_id).unwrap(); + if !current_node.child_ids.is_empty() { + // New children populated after emitting the other + self.emittable_ids.pop().unwrap(); + continue; + } + let Some(edges) = current_node.edges.take() else { + // Not yet populated + self.populate_one().expect("parent node should exist"); + continue; + }; + // The second (or the last) parent will be visited first + let current_id = self.emittable_ids.pop().unwrap(); + self.nodes.remove(¤t_id).unwrap(); + for parent_id in reachable_targets(&edges) { + let parent_node = self.nodes.get_mut(parent_id).unwrap(); + parent_node.child_ids.remove(¤t_id); + if parent_node.child_ids.is_empty() { + self.emittable_ids.push(parent_id.clone()); + } + } + return Some((current_id, edges)); + } else if !self.new_head_ids.is_empty() { + self.emittable_ids.extend(self.new_head_ids.drain(..).rev()); + } else { + // Populate the first or orphan head + self.populate_one()?; + } + } + } +} + +impl Iterator for TopoGroupedRevsetGraphIterator +where + I: Iterator)>, +{ + type Item = (CommitId, Vec); + + fn next(&mut self) -> Option { + if let Some(node) = self.next_node() { + Some(node) + } else { + assert!(self.nodes.is_empty(), "all nodes should have been emitted"); + None + } + } +} + #[cfg(test)] mod tests { use std::cmp::Ordering; @@ -281,4 +400,713 @@ mod tests { A "###); } + + fn topo_grouped(graph_iter: I) -> TopoGroupedRevsetGraphIterator + where + I: IntoIterator)>, + { + TopoGroupedRevsetGraphIterator::new(graph_iter.into_iter()) + } + + #[test] + fn test_topo_grouped_multiple_roots() { + let graph = vec![ + (id('C'), vec![missing('Y')]), + (id('B'), vec![missing('X')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + C # missing(Y) + + B # missing(X) + + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + C # missing(Y) + + B # missing(X) + + A + "###); + + // All nodes can be lazily emitted. + let mut iter = topo_grouped(graph.iter().cloned().peekable()); + assert_eq!(iter.next().unwrap().0, id('C')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('B')); + assert_eq!(iter.next().unwrap().0, id('B')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('A')); + } + + #[test] + fn test_topo_grouped_trivial_fork() { + let graph = vec![ + (id('E'), vec![direct('B')]), + (id('D'), vec![direct('A')]), + (id('C'), vec![direct('B')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + E # direct(B) + | + | D # direct(A) + | | + | | C # direct(B) + | | * + B | # direct(A) + |/ + A + "###); + // TODO + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + E # direct(B) + | + | D # direct(A) + | | + | | C # direct(B) + | | * + B | # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_fork_interleaved() { + let graph = vec![ + (id('F'), vec![direct('D')]), + (id('E'), vec![direct('C')]), + (id('D'), vec![direct('B')]), + (id('C'), vec![direct('B')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + F # direct(D) + | + | E # direct(C) + | | + D | # direct(B) + | | + | C # direct(B) + |/ + B # direct(A) + | + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + F # direct(D) + | + D # direct(B) + | + | E # direct(C) + | | + | C # direct(B) + |/ + B # direct(A) + | + A + "###); + + // F can be lazy, then E will be queued, then C. + let mut iter = topo_grouped(graph.iter().cloned().peekable()); + assert_eq!(iter.next().unwrap().0, id('F')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('E')); + assert_eq!(iter.next().unwrap().0, id('D')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('C')); + assert_eq!(iter.next().unwrap().0, id('E')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('B')); + } + + #[test] + fn test_topo_grouped_fork_multiple_heads() { + let graph = vec![ + (id('I'), vec![direct('E')]), + (id('H'), vec![direct('C')]), + (id('G'), vec![direct('A')]), + (id('F'), vec![direct('E')]), + (id('E'), vec![direct('C')]), + (id('D'), vec![direct('C')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + I # direct(E) + | + | H # direct(C) + | | + | | G # direct(A) + | | | + | | | F # direct(E) + | | | * + E | | # direct(C) + |/ / + | | D # direct(C) + | | * + C | # direct(A) + |/ + | B # direct(A) + |/ + A + "###); + // TODO + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + I # direct(E) + | + | H # direct(C) + | | + | | G # direct(A) + | | | + | | | F # direct(E) + | | | * + E | | # direct(C) + |/ / + | | D # direct(C) + | | * + C | # direct(A) + |/ + | B # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_fork_multiple_nested() { + let graph = vec![ + // Pull all sub graphs in reverse order: + (id('I'), vec![direct('A')]), + (id('H'), vec![direct('C')]), + (id('G'), vec![direct('E')]), + // Orphan sub graph G,F-E: + (id('F'), vec![direct('E')]), + (id('E'), vec![missing('Y')]), + // Orphan sub graph H,D-C: + (id('D'), vec![direct('C')]), + (id('C'), vec![missing('X')]), + // Orphan sub graph I,B-A: + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + I # direct(A) + | + | H # direct(C) + | | + | | G # direct(E) + | | | + | | | F # direct(E) + | | |/ + | | E # missing(Y) + | | + | | D # direct(C) + | |/ + | C # missing(X) + | + | B # direct(A) + |/ + A + "###); + // TODO + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + I # direct(A) + | + | H # direct(C) + | | + | | G # direct(E) + | | | + | | | F # direct(E) + | | |/ + | | E # missing(Y) + | | + | | D # direct(C) + | |/ + | C # missing(X) + | + | B # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_merge_interleaved() { + let graph = vec![ + (id('F'), vec![direct('E')]), + (id('E'), vec![direct('C'), direct('D')]), + (id('D'), vec![direct('B')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + F # direct(E) + | + E # direct(C), direct(D) + |\ + | D # direct(B) + | | + C | # direct(A) + | | + | B # direct(A) + |/ + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + F # direct(E) + | + E # direct(C), direct(D) + |\ + | D # direct(B) + | | + | B # direct(A) + | | + C | # direct(A) + |/ + A + "###); + + // F, E, and D can be lazy, then C will be queued, then B. + let mut iter = topo_grouped(graph.iter().cloned().peekable()); + assert_eq!(iter.next().unwrap().0, id('F')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('E')); + assert_eq!(iter.next().unwrap().0, id('E')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('D')); + assert_eq!(iter.next().unwrap().0, id('D')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('C')); + assert_eq!(iter.next().unwrap().0, id('B')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('A')); + } + + #[test] + fn test_topo_grouped_merge_but_missing() { + let graph = vec![ + (id('E'), vec![direct('D')]), + (id('D'), vec![missing('Y'), direct('C')]), + (id('C'), vec![direct('B'), missing('X')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + E # direct(D) + | + D # missing(Y), direct(C) + | + C # direct(B), missing(X) + | + B # direct(A) + | + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + E # direct(D) + | + D # missing(Y), direct(C) + | + C # direct(B), missing(X) + | + B # direct(A) + | + A + "###); + + // All nodes can be lazily emitted. + let mut iter = topo_grouped(graph.iter().cloned().peekable()); + assert_eq!(iter.next().unwrap().0, id('E')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('D')); + assert_eq!(iter.next().unwrap().0, id('D')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('C')); + assert_eq!(iter.next().unwrap().0, id('C')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('B')); + assert_eq!(iter.next().unwrap().0, id('B')); + assert_eq!(iter.input_iter.peek().unwrap().0, id('A')); + } + + #[test] + fn test_topo_grouped_merge_criss_cross() { + let graph = vec![ + (id('G'), vec![direct('E')]), + (id('F'), vec![direct('D')]), + (id('E'), vec![direct('B'), direct('C')]), + (id('D'), vec![direct('B'), direct('C')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + G # direct(E) + | + | F # direct(D) + | | + E | # direct(B), direct(C) + |\ \ + | | D # direct(B), direct(C) + | |/* + | C # direct(A) + | | + B | # direct(A) + |/ + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + G # direct(E) + | + E # direct(B), direct(C) + |\ + | | F # direct(D) + | | | + | | D # direct(B), direct(C) + | |/* + | C # direct(A) + | | + B | # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_merge_descendants_interleaved() { + let graph = vec![ + (id('H'), vec![direct('F')]), + (id('G'), vec![direct('E')]), + (id('F'), vec![direct('D')]), + (id('E'), vec![direct('C')]), + (id('D'), vec![direct('C'), direct('B')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + H # direct(F) + | + | G # direct(E) + | | + F | # direct(D) + | | + | E # direct(C) + | | + D | # direct(C), direct(B) + |\ + C | # direct(A) + | | + | B # direct(A) + |/ + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + H # direct(F) + | + F # direct(D) + | + D # direct(C), direct(B) + |\ + | B # direct(A) + | | + | | G # direct(E) + | | | + | | E # direct(C) + | | * + C | # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_merge_multiple_roots() { + let graph = vec![ + (id('D'), vec![direct('C')]), + (id('C'), vec![direct('B'), direct('A')]), + (id('B'), vec![missing('X')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + D # direct(C) + | + C # direct(B), direct(A) + |\ + B | # missing(X) + / + A + "###); + // A is emitted first because it's the second parent. + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + D # direct(C) + | + C # direct(B), direct(A) + |\ + | A + | + B # missing(X) + "###); + } + + #[test] + fn test_topo_grouped_merge_stairs() { + let graph = vec![ + // Merge topic branches one by one: + (id('J'), vec![direct('I'), direct('G')]), + (id('I'), vec![direct('H'), direct('E')]), + (id('H'), vec![direct('D'), direct('F')]), + // Topic branches: + (id('G'), vec![direct('D')]), + (id('F'), vec![direct('C')]), + (id('E'), vec![direct('B')]), + // Base nodes: + (id('D'), vec![direct('C')]), + (id('C'), vec![direct('B')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + J # direct(I), direct(G) + |\ + I | # direct(H), direct(E) + |\ \ + H | | # direct(D), direct(F) + |\ \ \ + | | | G # direct(D) + | | | * + | F | # direct(C) + | | | + | | E # direct(B) + | | | + D | | # direct(C) + |/ / + C | # direct(B) + |/ + B # direct(A) + | + A + "###); + // Second branches are visited first. + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + J # direct(I), direct(G) + |\ + | G # direct(D) + | | + I | # direct(H), direct(E) + |\ \ + | E | # direct(B) + | | | + H | | # direct(D), direct(F) + |\ \* + | F | # direct(C) + | | | + D | | # direct(C) + |/ / + C | # direct(B) + |/ + B # direct(A) + | + A + "###); + } + + #[test] + fn test_topo_grouped_merge_and_fork() { + let graph = vec![ + (id('J'), vec![direct('F')]), + (id('I'), vec![direct('E')]), + (id('H'), vec![direct('G')]), + (id('G'), vec![direct('D'), direct('E')]), + (id('F'), vec![direct('C')]), + (id('E'), vec![direct('B')]), + (id('D'), vec![direct('B')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + J # direct(F) + | + | I # direct(E) + | | + | | H # direct(G) + | | | + | | G # direct(D), direct(E) + | |/| + F | | # direct(C) + | | | + | E | # direct(B) + | | | + | | D # direct(B) + | |/ + C | # direct(A) + | | + | B # direct(A) + |/ + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + J # direct(F) + | + F # direct(C) + | + C # direct(A) + | + | I # direct(E) + | | + | | H # direct(G) + | | | + | | G # direct(D), direct(E) + | |/| + | E | # direct(B) + | | | + | | D # direct(B) + | |/ + | B # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_merge_and_fork_multiple_roots() { + let graph = vec![ + (id('J'), vec![direct('F')]), + (id('I'), vec![direct('G')]), + (id('H'), vec![direct('E')]), + (id('G'), vec![direct('E'), direct('B')]), + (id('F'), vec![direct('D')]), + (id('E'), vec![direct('C')]), + (id('D'), vec![direct('A')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![missing('X')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + J # direct(F) + | + | I # direct(G) + | | + | | H # direct(E) + | | | + | G | # direct(E), direct(B) + | |\ + F | | # direct(D) + | | | + | E | # direct(C) + | | | + D | | # direct(A) + | | | + | C | # direct(A) + |/ / + | B # missing(X) + | + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + J # direct(F) + | + F # direct(D) + | + D # direct(A) + | + | I # direct(G) + | | + | G # direct(E), direct(B) + | |\ + | | B # missing(X) + | | + | | H # direct(E) + | |/ + | E # direct(C) + | | + | C # direct(A) + |/ + A + "###); + } + + #[test] + fn test_topo_grouped_parallel_interleaved() { + let graph = vec![ + (id('E'), vec![direct('C')]), + (id('D'), vec![direct('B')]), + (id('C'), vec![direct('A')]), + (id('B'), vec![missing('X')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + E # direct(C) + | + | D # direct(B) + | | + C | # direct(A) + | | + | B # missing(X) + | + A + "###); + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + E # direct(C) + | + C # direct(A) + | + A + + D # direct(B) + | + B # missing(X) + "###); + } + + #[test] + fn test_topo_grouped_multiple_child_dependencies() { + let graph = vec![ + (id('I'), vec![direct('H'), direct('G')]), + (id('H'), vec![direct('D')]), + (id('G'), vec![direct('B')]), + (id('F'), vec![direct('E'), direct('C')]), + (id('E'), vec![direct('D')]), + (id('D'), vec![direct('B')]), + (id('C'), vec![direct('B')]), + (id('B'), vec![direct('A')]), + (id('A'), vec![]), + ]; + insta::assert_snapshot!(format_graph(graph.iter().cloned()), @r###" + I # direct(H), direct(G) + |\ + H | # direct(D) + | | + | G # direct(B) + | | + | | F # direct(E), direct(C) + | | |\ + | | E | # direct(D) + | | */ + D | | # direct(B) + |/ / + | C # direct(B) + |/ + B # direct(A) + | + A + "###); + // Topological order must be preserved. Depending on the implementation, + // E might be requested more than once by paths D->E and B->D->E. + insta::assert_snapshot!(format_graph(topo_grouped(graph.iter().cloned())), @r###" + I # direct(H), direct(G) + |\ + | G # direct(B) + | | + H | # direct(D) + | | + | | F # direct(E), direct(C) + | | |\ + | | | C # direct(B) + | | | * + | | E # direct(D) + | | * + D | # direct(B) + |/ + B # direct(A) + | + A + "###); + } } diff --git a/src/commands/mod.rs b/src/commands/mod.rs index b2d9b7770ad..c63c7f9bc79 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -43,7 +43,9 @@ use jj_lib::repo_path::RepoPath; use jj_lib::revset::{ RevsetAliasesMap, RevsetExpression, RevsetFilterPredicate, RevsetIteratorExt, }; -use jj_lib::revset_graph::{ReverseRevsetGraphIterator, RevsetGraphEdge, RevsetGraphEdgeType}; +use jj_lib::revset_graph::{ + ReverseRevsetGraphIterator, RevsetGraphEdgeType, TopoGroupedRevsetGraphIterator, +}; use jj_lib::rewrite::{back_out_commit, merge_commit_trees, rebase_commit, DescendantRebaser}; use jj_lib::settings::UserSettings; use jj_lib::tree::{merge_trees, Tree}; @@ -1587,11 +1589,11 @@ fn cmd_log(ui: &mut Ui, command: &CommandHelper, args: &LogArgs) -> Result<(), C if !args.no_graph { let mut graph = get_graphlog(command.settings(), formatter.raw()); let default_node_symbol = graph.default_node_symbol().to_owned(); - let iter: Box)>> = if args.reversed - { - Box::new(ReverseRevsetGraphIterator::new(revset.iter_graph())) + let forward_iter = TopoGroupedRevsetGraphIterator::new(revset.iter_graph()); + let iter: Box> = if args.reversed { + Box::new(ReverseRevsetGraphIterator::new(Box::new(forward_iter))) } else { - revset.iter_graph() + Box::new(forward_iter) }; for (commit_id, edges) in iter { let mut graphlog_edges = vec![]; diff --git a/tests/test_abandon_command.rs b/tests/test_abandon_command.rs index c67c95b4b9f..3e712066526 100644 --- a/tests/test_abandon_command.rs +++ b/tests/test_abandon_command.rs @@ -121,9 +121,9 @@ fn test_rebase_branch_with_merge() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ e ├─╮ + │ ◉ a b ◉ │ d ◉ │ c - │ ◉ a b ├─╯ ◉ "###); diff --git a/tests/test_chmod_command.rs b/tests/test_chmod_command.rs index 8fc5be042f6..72f4f158977 100644 --- a/tests/test_chmod_command.rs +++ b/tests/test_chmod_command.rs @@ -59,8 +59,8 @@ fn test_chmod_regular_conflict() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ x │ ◉ n + ◉ │ x ├─╯ ◉ base ◉ @@ -149,13 +149,13 @@ fn test_chmod_file_dir_deletion_conflicts() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ file_deletion ├─╮ + ◉ │ deletion │ │ ◉ file_dir │ ╭─┤ - │ │ ◉ dir - ◉ │ │ deletion + │ ◉ │ file + ├─╯ │ + │ ◉ dir ├───╯ - │ ◉ file - ├─╯ ◉ base ◉ "###); diff --git a/tests/test_duplicate_command.rs b/tests/test_duplicate_command.rs index 84941b370c2..2eced627700 100644 --- a/tests/test_duplicate_command.rs +++ b/tests/test_duplicate_command.rs @@ -43,8 +43,8 @@ fn test_duplicate() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 17a00fc21654 c ├─╮ - ◉ │ d370aee184ba b │ ◉ 2443ea76b0b1 a + ◉ │ d370aee184ba b ├─╯ ◉ 000000000000 "###); @@ -62,10 +62,10 @@ fn test_duplicate() { ◉ 2f6dc5a1ffc2 a │ @ 17a00fc21654 c │ ├─╮ - │ ◉ │ d370aee184ba b - ├─╯ │ - │ ◉ 2443ea76b0b1 a + │ │ ◉ 2443ea76b0b1 a ├───╯ + │ ◉ d370aee184ba b + ├─╯ ◉ 000000000000 "###); @@ -79,8 +79,8 @@ fn test_duplicate() { ├─╮ │ │ @ 17a00fc21654 c ╭─┬─╯ - ◉ │ d370aee184ba b │ ◉ 2443ea76b0b1 a + ◉ │ d370aee184ba b ├─╯ ◉ 000000000000 "###); @@ -101,9 +101,9 @@ fn test_duplicate_many() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 921dde6e55c0 e ├─╮ + │ ◉ 1394f625cbbd b ◉ │ ebd06dba20ec d ◉ │ c0cb3a0b73e7 c - │ ◉ 1394f625cbbd b ├─╯ ◉ 2443ea76b0b1 a ◉ 000000000000 @@ -120,11 +120,11 @@ fn test_duplicate_many() { ◉ │ 3b74d9691015 b │ │ @ 921dde6e55c0 e │ ╭─┤ - │ ◉ │ ebd06dba20ec d - │ ◉ │ c0cb3a0b73e7 c - ├─╯ │ - │ ◉ 1394f625cbbd b + │ │ ◉ 1394f625cbbd b ├───╯ + │ ◉ ebd06dba20ec d + │ ◉ c0cb3a0b73e7 c + ├─╯ ◉ 2443ea76b0b1 a ◉ 000000000000 "###); @@ -139,11 +139,11 @@ fn test_duplicate_many() { ◉ 0276d3d7c24d b │ @ 921dde6e55c0 e │ ├─╮ - │ ◉ │ ebd06dba20ec d - │ ◉ │ c0cb3a0b73e7 c - ├─╯ │ - │ ◉ 1394f625cbbd b + │ │ ◉ 1394f625cbbd b ├───╯ + │ ◉ ebd06dba20ec d + │ ◉ c0cb3a0b73e7 c + ├─╯ ◉ 2443ea76b0b1 a ◉ 000000000000 "###); @@ -159,16 +159,16 @@ fn test_duplicate_many() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ 0f7430f2727a e ├─╮ - ◉ │ 2181781b4f81 d │ ◉ fa167d18a83a b + ◉ │ 2181781b4f81 d │ │ @ 921dde6e55c0 e │ │ ├─╮ - │ │ ◉ │ ebd06dba20ec d - ├───╯ │ - ◉ │ │ c0cb3a0b73e7 c - ├─╯ │ - │ ◉ 1394f625cbbd b - ├─────╯ + │ │ │ ◉ 1394f625cbbd b + │ ├───╯ + │ │ ◉ ebd06dba20ec d + ├───╯ + ◉ │ c0cb3a0b73e7 c + ├─╯ ◉ 2443ea76b0b1 a ◉ 000000000000 "###); @@ -178,9 +178,9 @@ fn test_duplicate_many() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 921dde6e55c0 e ├─╮ + │ ◉ 1394f625cbbd b ◉ │ ebd06dba20ec d ◉ │ c0cb3a0b73e7 c - │ ◉ 1394f625cbbd b ├─╯ ◉ 2443ea76b0b1 a ◉ 000000000000 @@ -198,10 +198,10 @@ fn test_duplicate_many() { │ │ ◉ c6f7f8c4512e a │ │ │ @ 921dde6e55c0 e │ ╭───┤ + │ ◉ │ │ 1394f625cbbd b │ │ │ ◉ ebd06dba20ec d ├─────╯ ◉ │ │ c0cb3a0b73e7 c - │ ◉ │ 1394f625cbbd b ├─╯ │ ◉ │ 2443ea76b0b1 a ├───╯ @@ -221,16 +221,16 @@ fn test_duplicate_many() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ ee8fe64ed254 e ├─╮ + │ ◉ e13ac0adabdf b ◉ │ 2f2442db08eb d ◉ │ df53fa589286 c - │ ◉ e13ac0adabdf b ├─╯ ◉ 0fe67a05989e a │ @ 921dde6e55c0 e │ ├─╮ + │ │ ◉ 1394f625cbbd b │ ◉ │ ebd06dba20ec d │ ◉ │ c0cb3a0b73e7 c - │ │ ◉ 1394f625cbbd b │ ├─╯ │ ◉ 2443ea76b0b1 a ├─╯ diff --git a/tests/test_git_colocated.rs b/tests/test_git_colocated.rs index 7f68053ec16..20c47de800e 100644 --- a/tests/test_git_colocated.rs +++ b/tests/test_git_colocated.rs @@ -340,8 +340,8 @@ fn test_git_colocated_external_checkout() { // be abandoned. (#1042) insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 0521ce3b8c4e29aab79f3c750e2845dcbc4c3874 + ◉ a86754f975f953fa25da4265764adc0c62e9ce6b master HEAD@git A │ ◉ 66f4d1806ae41bd604f69155dece64062a0056cf B - ◉ │ a86754f975f953fa25da4265764adc0c62e9ce6b master HEAD@git A ├─╯ ◉ 0000000000000000000000000000000000000000 "###); diff --git a/tests/test_new_command.rs b/tests/test_new_command.rs index 8f6be198e68..f8857e7e15b 100644 --- a/tests/test_new_command.rs +++ b/tests/test_new_command.rs @@ -61,8 +61,8 @@ fn test_new_merge() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 0c4e5b9b68ae0cbe7ce3c61042619513d09005bf ├─╮ - ◉ │ f399209d9dda06e8a25a0c8e9a0cde9f421ff35d add file2 │ ◉ 38e8e2f6c92ffb954961fc391b515ff551b41636 add file1 + ◉ │ f399209d9dda06e8a25a0c8e9a0cde9f421ff35d add file2 ├─╯ ◉ 0000000000000000000000000000000000000000 "###); @@ -77,8 +77,8 @@ fn test_new_merge() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 200ed1a14c8acf09783dafefe5bebf2ff58f12fd ├─╮ - ◉ │ f399209d9dda06e8a25a0c8e9a0cde9f421ff35d add file2 │ ◉ 38e8e2f6c92ffb954961fc391b515ff551b41636 add file1 + ◉ │ f399209d9dda06e8a25a0c8e9a0cde9f421ff35d add file2 ├─╯ ◉ 0000000000000000000000000000000000000000 "###); @@ -115,8 +115,8 @@ fn test_new_insert_after() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ F ├─╮ - ◉ │ E │ ◉ D + ◉ │ E ├─╯ │ ◉ C │ ◉ B @@ -137,14 +137,14 @@ fn test_new_insert_after() { ◉ C │ ◉ F ╭─┤ + │ ◉ E @ │ G ├───╮ - │ ◉ │ E - ◉ │ │ D - ├─╯ │ - │ ◉ B - │ ◉ A - ├───╯ + │ │ ◉ B + │ │ ◉ A + │ ├─╯ + ◉ │ D + ├─╯ ◉ root "###); @@ -158,15 +158,15 @@ fn test_new_insert_after() { ◉ C │ ◉ F ╭─┤ + │ ◉ E ◉ │ G ├───╮ - @ │ │ H - │ ◉ │ E - ◉ │ │ D - ├─╯ │ - │ ◉ B - │ ◉ A - ├───╯ + │ │ ◉ B + │ │ ◉ A + │ ├─╯ + @ │ H + ◉ │ D + ├─╯ ◉ root "###); } @@ -180,8 +180,8 @@ fn test_new_insert_after_children() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ F ├─╮ - ◉ │ E │ ◉ D + ◉ │ E ├─╯ │ ◉ C │ ◉ B @@ -203,16 +203,16 @@ fn test_new_insert_after_children() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ G ├─╮ - │ │ ◉ F - │ │ ├─╮ - │ │ ◉ │ E - │ │ │ ◉ D - │ │ ├─╯ - ◉ │ │ C - ◉ │ │ B - ├─╯ │ - ◉ │ A + ◉ │ C + ◉ │ B + ├─╯ + ◉ A + │ ◉ F + │ ├─╮ + │ │ ◉ D ├───╯ + │ ◉ E + ├─╯ ◉ root "###); } @@ -226,8 +226,8 @@ fn test_new_insert_before() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ F ├─╮ - ◉ │ E │ ◉ D + ◉ │ E ├─╯ │ ◉ C │ ◉ B @@ -251,12 +251,12 @@ fn test_new_insert_before() { ├─╯ @ G ├─┬─╮ - ◉ │ │ E + │ │ ◉ B + │ │ ◉ A │ ◉ │ D - ├─╯ │ - │ ◉ B - │ ◉ A - ├───╯ + │ ├─╯ + ◉ │ E + ├─╯ ◉ root "###); } @@ -270,8 +270,8 @@ fn test_new_insert_before_root_successors() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ F ├─╮ - ◉ │ E │ ◉ D + ◉ │ E ├─╯ │ ◉ C │ ◉ B @@ -290,13 +290,13 @@ fn test_new_insert_before_root_successors() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" ◉ F ├─╮ + │ ◉ E + ◉ │ D │ │ ◉ C │ │ ◉ B - ◉ │ │ D │ │ ◉ A ├───╯ @ │ G - │ ◉ E ├─╯ ◉ root "###); @@ -313,8 +313,8 @@ fn test_new_insert_before_no_loop() { insta::assert_snapshot!(stdout, @r###" @ 7705d353bf5d F ├─╮ - ◉ │ 41a89ffcbba2 E │ ◉ c9257eff5bf9 D + ◉ │ 41a89ffcbba2 E ├─╯ │ ◉ ec18c57d72d8 C │ ◉ 6041917ceeb5 B @@ -339,8 +339,8 @@ fn test_new_insert_before_no_root_merge() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ F ├─╮ - ◉ │ E │ ◉ D + ◉ │ E ├─╯ │ ◉ C │ ◉ B @@ -359,12 +359,12 @@ fn test_new_insert_before_no_root_merge() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" ◉ F ├─╮ + │ ◉ E + ◉ │ D │ │ ◉ C - ◉ │ │ D │ │ ◉ B ├───╯ @ │ G - │ ◉ E ◉ │ A ├─╯ ◉ root @@ -380,8 +380,8 @@ fn test_new_insert_before_root() { insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" @ F ├─╮ - ◉ │ E │ ◉ D + ◉ │ E ├─╯ │ ◉ C │ ◉ B diff --git a/tests/test_rebase_command.rs b/tests/test_rebase_command.rs index 2eec5350f2a..18df91388c1 100644 --- a/tests/test_rebase_command.rs +++ b/tests/test_rebase_command.rs @@ -248,8 +248,8 @@ fn test_rebase_single_revision() { @ d ◉ c ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ "###); @@ -290,10 +290,10 @@ fn test_rebase_single_revision() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ d ├─╮ - │ │ ◉ c - ◉ │ │ b - ├───╯ │ ◉ a + ◉ │ b + ├─╯ + │ ◉ c ├─╯ ◉ "###); @@ -313,9 +313,9 @@ fn test_rebase_single_revision_merge_parent() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ d ├─╮ + │ ◉ a ◉ │ c ◉ │ b - │ ◉ a ├─╯ ◉ "###); @@ -333,9 +333,9 @@ fn test_rebase_single_revision_merge_parent() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ d ├─╮ + ◉ │ b │ │ ◉ c │ ├─╯ - ◉ │ b │ ◉ a ├─╯ ◉ @@ -366,8 +366,8 @@ fn test_rebase_multiple_destinations() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ a ├─╮ - @ │ c │ ◉ b + @ │ c ├─╯ ◉ "###); @@ -388,8 +388,8 @@ fn test_rebase_multiple_destinations() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ a ├─╮ - @ │ c │ ◉ b + @ │ c ├─╯ ◉ "###); @@ -417,8 +417,8 @@ fn test_rebase_multiple_destinations() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ a + ◉ b │ @ c - ◉ │ b ├─╯ ◉ "###); @@ -439,8 +439,8 @@ fn test_rebase_multiple_destinations() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ a ├─╮ - @ │ c │ ◉ b + @ │ c ├─╯ ◉ "###); @@ -467,8 +467,8 @@ fn test_rebase_with_descendants() { @ d ◉ c ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ "###); @@ -500,8 +500,8 @@ fn test_rebase_with_descendants() { @ d │ ◉ c ├─╯ + ◉ a │ ◉ b - ◉ │ a ├─╯ ◉ "###); @@ -512,8 +512,8 @@ fn test_rebase_with_descendants() { @ d ◉ c ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ "###); @@ -529,8 +529,8 @@ fn test_rebase_with_descendants() { "###); insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" ◉ c + ◉ b │ @ d - ◉ │ b ├─╯ ◉ a ◉ diff --git a/tests/test_resolve_command.rs b/tests/test_resolve_command.rs index d42ca2085da..bf2e9bef697 100644 --- a/tests/test_resolve_command.rs +++ b/tests/test_resolve_command.rs @@ -56,8 +56,8 @@ fn test_resolution() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ @@ -307,8 +307,8 @@ fn test_normal_conflict_input_files() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ @@ -348,8 +348,8 @@ fn test_baseless_conflict_input_files() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ @@ -416,8 +416,8 @@ fn test_edit_delete_conflict_input_files() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ @@ -459,8 +459,8 @@ fn test_file_vs_dir() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ @@ -506,11 +506,11 @@ fn test_description_with_dir_and_deletion() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─┬─╮ - ◉ │ │ del + │ │ ◉ edit │ ◉ │ dir - ├─╯ │ - │ ◉ edit - ├───╯ + │ ├─╯ + ◉ │ del + ├─╯ ◉ base ◉ "###); @@ -586,8 +586,8 @@ fn test_multiple_conflicts() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ diff --git a/tests/test_restore_command.rs b/tests/test_restore_command.rs index c5e296a14df..bb1d0d26431 100644 --- a/tests/test_restore_command.rs +++ b/tests/test_restore_command.rs @@ -123,8 +123,8 @@ fn test_restore_conflicted_merge() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ conflict ├─╮ - ◉ │ b │ ◉ a + ◉ │ b ├─╯ ◉ base ◉ diff --git a/tests/test_squash_command.rs b/tests/test_squash_command.rs index 4767bad150d..cefb8eee653 100644 --- a/tests/test_squash_command.rs +++ b/tests/test_squash_command.rs @@ -91,8 +91,8 @@ fn test_squash() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ c7a11b36d333 e ├─╮ - ◉ │ 5658521e0f8b d │ ◉ 90fe0a96fc90 c + ◉ │ 5658521e0f8b d ├─╯ ◉ fa5efbdf533c b ◉ 90aeefd03044 a @@ -115,8 +115,8 @@ fn test_squash() { @ 959145c11426 ◉ 80960125bb96 e ├─╮ - ◉ │ 5658521e0f8b d │ ◉ 90fe0a96fc90 c + ◉ │ 5658521e0f8b d ├─╯ ◉ fa5efbdf533c b ◉ 90aeefd03044 a diff --git a/tests/test_templater.rs b/tests/test_templater.rs index 25f04086daa..257d5de2386 100644 --- a/tests/test_templater.rs +++ b/tests/test_templater.rs @@ -68,10 +68,10 @@ fn test_templater_branches() { insta::assert_snapshot!(output, @r###" ◉ b1bb3766d584 branch3?? │ @ a5b4d15489cc branch2* new-branch - │ │ ◉ 21c33875443e branch1* - ├───╯ │ ◉ 8476341eb395 branch2@origin ├─╯ + │ ◉ 21c33875443e branch1* + ├─╯ ◉ 000000000000 "###); } diff --git a/tests/test_unsquash_command.rs b/tests/test_unsquash_command.rs index a6616ede7d2..979d7a62410 100644 --- a/tests/test_unsquash_command.rs +++ b/tests/test_unsquash_command.rs @@ -90,8 +90,8 @@ fn test_unsquash() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 1f8f152ff48e e ├─╮ - ◉ │ 5658521e0f8b d │ ◉ 90fe0a96fc90 c + ◉ │ 5658521e0f8b d ├─╯ ◉ fa5efbdf533c b ◉ 90aeefd03044 a @@ -114,8 +114,8 @@ fn test_unsquash() { insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###" @ 3217340cb761 ├─╮ - ◉ │ 5658521e0f8b d e?? │ ◉ 90fe0a96fc90 c e?? + ◉ │ 5658521e0f8b d e?? ├─╯ ◉ fa5efbdf533c b ◉ 90aeefd03044 a