From 24fac819c3a56fc2e7ac99456844d2a4560691b0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 11 Feb 2024 10:41:41 -0800 Subject: [PATCH 1/3] cli: fix a few references to "legacy" as default graph style The "curved" style has been the default for a long time now. --- cli/src/config-schema.json | 2 +- cli/tests/test_log_command.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 08b84189f8..716c7eb534 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -111,7 +111,7 @@ "ascii", "ascii-large" ], - "default": "legacy" + "default": "curved" } } }, diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 36ab070977..0e43b0c12c 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -1147,7 +1147,7 @@ fn test_graph_styles() { &["new", "-m", "merge", r#"description("main branch 1")"#, "@"], ); - // Default (legacy) style + // Default (curved) style let stdout = test_env.jj_cmd_success(&repo_path, &["log", "-T=description"]); insta::assert_snapshot!(stdout, @r###" @ merge From ea9b91fbefcb2b7f2580d911eea9c792e10b9257 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 11 Feb 2024 10:43:06 -0800 Subject: [PATCH 2/3] cli: drop support for legacy graph style I don't think anyone uses the legacy graph style. It's very similar to the "ascii" style from Sapling's `renderdag` crate. --- CHANGELOG.md | 3 + cli/src/config-schema.json | 1 - cli/src/graphlog.rs | 886 +--------------------------------- cli/tests/test_log_command.rs | 22 - docs/config.md | 3 +- 5 files changed, 7 insertions(+), 908 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9edf62e44c..3814724150 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 reindexing failed, you'll need to clean up corrupted operation history by `jj op abandon ..`. +* Dropped support for the "legacy" graph-drawing style. Use "ascii" for a very + similar result. + ### New features * Templates now support logical operators: `||`, `&&`, `!` diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 716c7eb534..966d61e50c 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -105,7 +105,6 @@ "style": { "description": "Style of connectors/markings used to render the graph. See https://github.com/martinvonz/jj/blob/main/docs/config.md#graph-style", "enum": [ - "legacy", "curved", "square", "ascii", diff --git a/cli/src/graphlog.rs b/cli/src/graphlog.rs index d57498bb74..a7736d71f1 100644 --- a/cli/src/graphlog.rs +++ b/cli/src/graphlog.rs @@ -13,8 +13,8 @@ // limitations under the License. use std::hash::Hash; +use std::io; use std::io::Write; -use std::{cmp, io}; use itertools::Itertools; use jj_lib::settings::UserSettings; @@ -138,7 +138,6 @@ pub fn get_graphlog<'a, K: Clone + Eq + Hash + 'a>( let builder = GraphRowRenderer::new().output().with_min_row_height(0); match settings.graph_style().as_str() { - "curved" => SaplingGraphLog::create(builder.build_box_drawing(), formatter, "◉"), "square" => SaplingGraphLog::create( builder.build_box_drawing().with_square_glyphs(), formatter, @@ -146,886 +145,7 @@ pub fn get_graphlog<'a, K: Clone + Eq + Hash + 'a>( ), "ascii" => SaplingGraphLog::create(builder.build_ascii(), formatter, "o"), "ascii-large" => SaplingGraphLog::create(builder.build_ascii_large(), formatter, "o"), - _ => Box::new(AsciiGraphDrawer::new(formatter)), - } -} - -pub struct AsciiGraphDrawer<'writer, K> { - writer: &'writer mut dyn Write, - edges: Vec>, - pending_text: Vec, -} - -impl<'writer, K> AsciiGraphDrawer<'writer, K> -where - K: Clone + Eq + Hash, -{ - pub fn new(writer: &'writer mut dyn Write) -> Self { - Self { - writer, - edges: Default::default(), - pending_text: Default::default(), - } - } -} - -impl<'writer, K: Clone + Eq + Hash> GraphLog for AsciiGraphDrawer<'writer, K> { - fn add_node( - &mut self, - id: &K, - edges: &[Edge], - node_symbol: &str, - text: &str, - ) -> io::Result<()> { - assert!(self.pending_text.is_empty()); - for line in text.split(|x| x == '\n') { - self.pending_text.push(line.into()); - } - if self.pending_text.last() == Some(&"".into()) { - self.pending_text.pop().unwrap(); - } - self.pending_text.reverse(); - - // Check if an existing edge should be terminated by the new node. If there - // is, draw the new node in the same column. Otherwise, insert it at the right. - let edge_index = if let Some(edge_index) = self.index_by_target(id) { - // This edge terminates in the node we're adding - - // If we're inserting a merge somewhere that's not the very right, the edges - // right of it will move further right, so we need to prepare by inserting rows - // of '\'. - if edges.len() > 2 && edge_index < self.edges.len() - 1 { - for i in 2..edges.len() { - for edge in self.edges.iter().take(edge_index + 1) { - AsciiGraphDrawer::straight_edge(&mut self.writer, edge)?; - } - for _ in 0..i - 2 { - write!(self.writer, " ")?; - } - for _ in edge_index + 1..self.edges.len() { - write!(self.writer, " \\")?; - } - writeln!(self.writer)?; - } - } - - self.edges.remove(edge_index); - edge_index - } else { - self.edges.len() - }; - - // Draw the edges to the left of the new node - for edge in self.edges.iter().take(edge_index) { - AsciiGraphDrawer::straight_edge(&mut self.writer, edge)?; - } - // Draw the new node - write!(self.writer, "{node_symbol}")?; - // If it's a merge of many nodes, draw a vertical line to the right - for _ in 3..edges.len() { - write!(self.writer, "--")?; - } - if edges.len() > 2 { - write!(self.writer, "-.")?; - } - write!(self.writer, " ")?; - // Draw the edges to the right of the new node - for edge in self.edges.iter().skip(edge_index) { - AsciiGraphDrawer::straight_edge(&mut self.writer, edge)?; - } - if edges.len() > 1 { - write!(self.writer, " ")?; - } - - self.maybe_write_pending_text()?; - - // Update the data model. - for (i, edge) in edges.iter().enumerate() { - self.edges.insert(edge_index + i, edge.clone()); - } - - // If it's a merge commit, insert a row of '\'. - if edges.len() >= 2 { - for edge in self.edges.iter().take(edge_index) { - AsciiGraphDrawer::straight_edge(&mut self.writer, edge)?; - } - AsciiGraphDrawer::straight_edge_no_space(&mut self.writer, &self.edges[edge_index])?; - for _ in edge_index + 1..self.edges.len() { - write!(self.writer, "\\ ")?; - } - write!(self.writer, " ")?; - self.maybe_write_pending_text()?; - } - - let pad_to_index = self.edges.len() + usize::from(edges.is_empty()); - // Close any edges to missing nodes. - for (i, edge) in edges.iter().enumerate().rev() { - if *edge == Edge::Missing { - self.close_missing_edge(edge_index + i, pad_to_index)?; - } - } - // If this was the last node in a chain, add a "/" for any edges to the right of - // it. - if edges.is_empty() && edge_index < self.edges.len() { - self.close_edge(edge_index, pad_to_index)?; - } - - // Merge new edges that share the same target. - let mut source_index = 1; - while source_index < self.edges.len() { - if let Edge::Present { target, .. } = &self.edges[source_index] { - if let Some(target_index) = self.index_by_target(target) { - // There already is an edge leading to the same target node. Mark that we - // want to merge the higher index into the lower index. - if source_index > target_index { - self.merge_edges(source_index, target_index, pad_to_index)?; - // Don't increment source_index. - continue; - } - } - } - source_index += 1; - } - - // Emit any remaining lines of text. - while !self.pending_text.is_empty() { - for edge in &self.edges { - AsciiGraphDrawer::straight_edge(&mut self.writer, edge)?; - } - for _ in self.edges.len()..pad_to_index { - write!(self.writer, " ")?; - } - self.maybe_write_pending_text()?; - } - - Ok(()) - } - - fn default_node_symbol(&self) -> &str { - "o" - } - - fn width(&self, id: &K, edges: &[Edge]) -> usize { - let orig = self.edges.len() - usize::from(self.index_by_target(id).is_some()); - let added = cmp::max(edges.len(), 1); - 2 * (orig + added) - } -} - -impl<'writer, K: Clone + Eq + Hash> AsciiGraphDrawer<'writer, K> { - fn index_by_target(&self, id: &K) -> Option { - for (i, edge) in self.edges.iter().enumerate() { - match edge { - Edge::Present { target, .. } if target == id => return Some(i), - _ => {} - } - } - None - } - - /// Not an instance method so the caller doesn't need mutable access to the - /// whole struct. - fn straight_edge(writer: &mut dyn Write, edge: &Edge) -> io::Result<()> { - AsciiGraphDrawer::straight_edge_no_space(writer, edge)?; - write!(writer, " ") - } - - /// Not an instance method so the caller doesn't need mutable access to the - /// whole struct. - fn straight_edge_no_space(writer: &mut dyn Write, edge: &Edge) -> io::Result<()> { - match edge { - Edge::Present { direct: true, .. } => { - write!(writer, "|")?; - } - Edge::Present { direct: false, .. } => { - write!(writer, ":")?; - } - Edge::Missing => { - write!(writer, "|")?; - } - } - Ok(()) - } - - fn merge_edges(&mut self, source: usize, target: usize, pad_to_index: usize) -> io::Result<()> { - assert!(target < source); - self.edges.remove(source); - for i in 0..target { - AsciiGraphDrawer::straight_edge(&mut self.writer, &self.edges[i])?; - } - if source == target + 1 { - // If we're merging exactly one step to the left, draw a '/' to join the lines. - AsciiGraphDrawer::straight_edge_no_space(&mut self.writer, &self.edges[target])?; - for _ in source..self.edges.len() + 1 { - write!(self.writer, "/ ")?; - } - write!(self.writer, " ")?; - for _ in self.edges.len() + 1..pad_to_index { - write!(self.writer, " ")?; - } - } else { - // If we're merging more than one step to the left, we need two rows: - // | |_|_|/ - // |/| | | - AsciiGraphDrawer::straight_edge(&mut self.writer, &self.edges[target])?; - for i in target + 1..source - 1 { - AsciiGraphDrawer::straight_edge_no_space(&mut self.writer, &self.edges[i])?; - write!(self.writer, "_")?; - } - AsciiGraphDrawer::straight_edge_no_space(&mut self.writer, &self.edges[source - 1])?; - for _ in source..self.edges.len() + 1 { - write!(self.writer, "/ ")?; - } - write!(self.writer, " ")?; - for _ in self.edges.len() + 1..pad_to_index { - write!(self.writer, " ")?; - } - self.maybe_write_pending_text()?; - - for i in 0..target { - AsciiGraphDrawer::straight_edge(&mut self.writer, &self.edges[i])?; - } - AsciiGraphDrawer::straight_edge_no_space(&mut self.writer, &self.edges[target])?; - write!(self.writer, "/")?; - for i in target + 1..self.edges.len() { - AsciiGraphDrawer::straight_edge(&mut self.writer, &self.edges[i])?; - } - for _ in self.edges.len()..pad_to_index { - write!(self.writer, " ")?; - } - } - self.maybe_write_pending_text()?; - - Ok(()) - } - - fn close_missing_edge(&mut self, source: usize, pad_to_index: usize) -> io::Result<()> { - self.edges.remove(source); - for i in 0..source { - AsciiGraphDrawer::straight_edge(&mut self.writer, &self.edges[i])?; - } - write!(self.writer, "~")?; - for _ in source..self.edges.len() { - write!(self.writer, "/ ")?; - } - write!(self.writer, " ")?; - for _ in self.edges.len() + 1..pad_to_index { - write!(self.writer, " ")?; - } - self.maybe_write_pending_text() - } - - fn close_edge(&mut self, source: usize, pad_to_index: usize) -> io::Result<()> { - for i in 0..source { - AsciiGraphDrawer::straight_edge(&mut self.writer, &self.edges[i])?; - } - write!(self.writer, " ")?; - for _ in source..self.edges.len() { - write!(self.writer, "/ ")?; - } - write!(self.writer, " ")?; - for _ in self.edges.len() + 1..pad_to_index { - write!(self.writer, " ")?; - } - self.maybe_write_pending_text() - } - - fn maybe_write_pending_text(&mut self) -> io::Result<()> { - if let Some(text) = self.pending_text.pop() { - write!(self.writer, "{text}")?; - } - writeln!(self.writer) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - /// Adapter to insert width calculation for each `.add_node()` - struct AsciiGraphDrawerWithWidths<'writer, K> { - inner: AsciiGraphDrawer<'writer, K>, - } - - impl<'writer, K: Clone + Eq + Hash> AsciiGraphDrawerWithWidths<'writer, K> { - fn new(writer: &'writer mut dyn Write) -> Self { - AsciiGraphDrawerWithWidths { - inner: AsciiGraphDrawer::new(writer), - } - } - - fn add_node( - &mut self, - id: &K, - edges: &[Edge], - node_symbol: &str, - text: &str, - ) -> io::Result<()> { - let width = self.inner.width(id, edges); - let text = format!("{text} [width={width}]"); - self.inner.add_node(id, edges, node_symbol, &text) - } - } - - #[test] - fn single_node() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&1, &[], "@", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @ node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn long_description() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawer::new(&mut buffer); - graph.add_node(&2, &[Edge::direct(1)], "@", "many\nlines\nof\ntext\n")?; - graph.add_node(&1, &[], "o", "single line")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @ many - | lines - | of - | text - o single line - "###); - - Ok(()) - } - - #[test] - fn long_description_blank_lines() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawer::new(&mut buffer); - graph.add_node( - &2, - &[Edge::direct(1)], - "@", - "\n\nmany\n\nlines\n\nof\n\ntext\n\n\n", - )?; - graph.add_node(&1, &[], "o", "single line")?; - - // A final newline is ignored but all other newlines are respected. - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @ - | - | many - | - | lines - | - | of - | - | text - | - | - o single line - "###); - - Ok(()) - } - - #[test] - fn chain() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&3, &[Edge::direct(2)], "@", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @ node 3 [width=2] - o node 2 [width=2] - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn interleaved_chains() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&7, &[Edge::direct(5)], "o", "node 7")?; - graph.add_node(&6, &[Edge::direct(4)], "o", "node 6")?; - graph.add_node(&5, &[Edge::direct(3)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(2)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "@", "node 3")?; - graph.add_node(&2, &[], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 7 [width=2] - | o node 6 [width=4] - o | node 5 [width=4] - | o node 4 [width=4] - @ | node 3 [width=4] - | o node 2 [width=4] - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn independent_nodes() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&3, &[Edge::missing()], "o", "node 3")?; - graph.add_node(&2, &[Edge::missing()], "o", "node 2")?; - graph.add_node(&1, &[Edge::missing()], "@", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 3 [width=2] - ~ - o node 2 [width=2] - ~ - @ node 1 [width=2] - ~ - "###); - - Ok(()) - } - - #[test] - fn left_chain_ends() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&4, &[Edge::direct(2)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::missing()], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 4 [width=2] - | o node 3 [width=4] - o | node 2 [width=4] - ~/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn left_chain_ends_with_no_missing_edge() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&4, &[Edge::direct(2)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[], "o", "node 2\nmore\ntext")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 4 [width=2] - | o node 3 [width=4] - o | node 2 - / more - | text [width=4] - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn right_chain_ends() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(2)], "o", "node 3")?; - graph.add_node(&2, &[Edge::missing()], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1\nmore\ntext")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 4 [width=2] - | o node 3 [width=4] - | o node 2 [width=4] - | ~ - o node 1 - more - text [width=2] - "###); - - Ok(()) - } - - #[test] - fn right_chain_ends_long_description() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawer::new(&mut buffer); - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node( - &2, - &[Edge::missing()], - "o", - "node 2\nwith\nlong\ndescription", - )?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 3 - | o node 2 - | ~ with - | long - | description - o node 1 - "###); - - Ok(()) - } - - #[test] - fn fork_multiple() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&4, &[Edge::direct(1)], "@", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @ node 4 [width=2] - | o node 3 [width=4] - |/ - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn fork_multiple_chains() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&10, &[Edge::direct(7)], "o", "node 10")?; - graph.add_node(&9, &[Edge::direct(6)], "o", "node 9")?; - graph.add_node(&8, &[Edge::direct(5)], "o", "node 8")?; - graph.add_node(&7, &[Edge::direct(4)], "o", "node 7")?; - graph.add_node(&6, &[Edge::direct(3)], "o", "node 6")?; - graph.add_node(&5, &[Edge::direct(2)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 10 [width=2] - | o node 9 [width=4] - | | o node 8 [width=6] - o | | node 7 [width=6] - | o | node 6 [width=6] - | | o node 5 [width=6] - o | | node 4 [width=6] - | o | node 3 [width=6] - |/ / - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn cross_over() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&5, &[Edge::direct(1)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(2)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 5 [width=2] - | o node 4 [width=4] - | | o node 3 [width=6] - | |/ - |/| - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn cross_over_multiple() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&7, &[Edge::direct(1)], "o", "node 7")?; - graph.add_node(&6, &[Edge::direct(3)], "o", "node 6")?; - graph.add_node(&5, &[Edge::direct(2)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 7 [width=2] - | o node 6 [width=4] - | | o node 5 [width=6] - | | | o node 4 [width=8] - | |_|/ - |/| | - | o | node 3 [width=6] - |/ / - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn cross_over_new_on_left() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&6, &[Edge::direct(3)], "o", "node 6")?; - graph.add_node(&5, &[Edge::direct(2)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 6 [width=2] - | o node 5 [width=4] - | | o node 4 [width=6] - o | | node 3 [width=6] - | |/ - |/| - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn merge_multiple() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node( - &5, - &[ - Edge::direct(1), - Edge::direct(2), - Edge::direct(3), - Edge::direct(4), - ], - "@", - "node 5\nmore\ntext", - )?; - graph.add_node(&4, &[Edge::missing()], "o", "node 4")?; - graph.add_node(&3, &[Edge::missing()], "o", "node 3")?; - graph.add_node(&2, &[Edge::missing()], "o", "node 2")?; - graph.add_node(&1, &[Edge::missing()], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @---. node 5 - |\ \ \ more - | | | | text [width=8] - | | | o node 4 [width=8] - | | | ~ - | | o node 3 [width=6] - | | ~ - | o node 2 [width=4] - | ~ - o node 1 [width=2] - ~ - "###); - - Ok(()) - } - - #[test] - fn fork_merge_in_central_edge() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&8, &[Edge::direct(1)], "o", "node 8")?; - graph.add_node(&7, &[Edge::direct(5)], "o", "node 7")?; - graph.add_node( - &6, - &[Edge::direct(2)], - "o", - "node 6\nwith\nsome\nmore\nlines", - )?; - graph.add_node(&5, &[Edge::direct(4), Edge::direct(3)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 8 [width=2] - | o node 7 [width=4] - | | o node 6 - | | | with - | | | some - | | | more - | | | lines [width=6] - | o | node 5 [width=8] - | |\ \ - | o | | node 4 [width=8] - |/ / / - | o | node 3 [width=6] - |/ / - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn fork_merge_multiple() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&6, &[Edge::direct(5)], "o", "node 6")?; - graph.add_node( - &5, - &[Edge::direct(2), Edge::direct(3), Edge::direct(4)], - "o", - "node 5", - )?; - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 6 [width=2] - o-. node 5 [width=6] - |\ \ - | | o node 4 [width=6] - | o | node 3 [width=6] - | |/ - o | node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn fork_merge_multiple_in_central_edge() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&10, &[Edge::direct(1)], "o", "node 10")?; - graph.add_node(&9, &[Edge::direct(7)], "o", "node 9")?; - graph.add_node(&8, &[Edge::direct(2)], "o", "node 8")?; - graph.add_node( - &7, - &[ - Edge::direct(6), - Edge::direct(5), - Edge::direct(4), - Edge::direct(3), - ], - "o", - "node 7", - )?; - graph.add_node(&6, &[Edge::direct(1)], "o", "node 6")?; - graph.add_node(&5, &[Edge::direct(1)], "o", "node 5")?; - graph.add_node(&4, &[Edge::direct(1)], "o", "node 4")?; - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node(&2, &[Edge::direct(1)], "o", "node 2")?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 10 [width=2] - | o node 9 [width=4] - | | o node 8 [width=6] - | | \ - | | \ - | o---. | node 7 [width=12] - | |\ \ \ \ - | o | | | | node 6 [width=12] - |/ / / / / - | o | | | node 5 [width=10] - |/ / / / - | o | | node 4 [width=8] - |/ / / - | o | node 3 [width=6] - |/ / - | o node 2 [width=4] - |/ - o node 1 [width=2] - "###); - - Ok(()) - } - - #[test] - fn merge_multiple_missing_edges() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node( - &1, - &[ - Edge::missing(), - Edge::missing(), - Edge::missing(), - Edge::missing(), - ], - "@", - "node 1\nwith\nmany\nlines\nof\ntext", - )?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - @---. node 1 - |\ \ \ with - | | | ~ many - | | ~ lines - | ~ of - ~ text [width=8] - "###); - - Ok(()) - } - - #[test] - fn merge_missing_edges_and_fork() -> io::Result<()> { - let mut buffer = vec![]; - let mut graph = AsciiGraphDrawerWithWidths::new(&mut buffer); - graph.add_node(&3, &[Edge::direct(1)], "o", "node 3")?; - graph.add_node( - &2, - &[ - Edge::missing(), - Edge::indirect(1), - Edge::missing(), - Edge::indirect(1), - ], - "o", - "node 2\nwith\nmany\nlines\nof\ntext", - )?; - graph.add_node(&1, &[], "o", "node 1")?; - - insta::assert_snapshot!(String::from_utf8_lossy(&buffer), @r###" - o node 3 [width=2] - | o---. node 2 - | |\ \ \ with - | | : ~/ many - | ~/ / lines - |/ / of - |/ text [width=10] - o node 1 [width=2] - "###); - - Ok(()) + // "curved" + _ => SaplingGraphLog::create(builder.build_box_drawing(), formatter, "◉"), } } diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 0e43b0c12c..32d1220a2b 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -1310,28 +1310,6 @@ fn test_log_word_wrap() { 4 5 6 7 8 9 "###); - insta::assert_snapshot!( - render(&["log", "-T", template, "--config-toml=ui.graph.style='legacy'"], 9, true), - @r###" - @ 0 1 2 - |\ 3 4 5 - | | 6 7 8 - | | 9 - | o 0 1 2 - | | 3 4 5 - | | 6 7 8 - | | 9 - | o 0 1 2 - |/ 3 4 5 - | 6 7 8 - | 9 - o 0 1 2 3 - | 4 5 6 7 - | 8 9 - o 0 1 2 3 - 4 5 6 7 - 8 9 - "###); // Shouldn't panic with $COLUMNS < graph_width insta::assert_snapshot!(render(&["log", "-r@"], 0, true), @r###" diff --git a/docs/config.md b/docs/config.md index 39b580bb8b..29f983b6cf 100644 --- a/docs/config.md +++ b/docs/config.md @@ -218,8 +218,7 @@ revsets.log = "main@origin.." ### Graph style ```toml -# Possible values: "curved" (default), "square", "ascii", "ascii-large", -# "legacy" +# Possible values: "curved" (default), "square", "ascii", "ascii-large" ui.graph.style = "square" ``` From d83bca7c9af33a40604b13b01942b1dd0bed636c Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 11 Feb 2024 10:09:31 -0800 Subject: [PATCH 3/3] log: optionally render elided parts of the graph as a synthetic node This adds a config to render a synthetic node with a "(elided revisions)" description for elided segments of the graph. I didn't add any templating support for the elided nodes because I'm not sure how we would want that to work. In particular, I don't know what `commit_id` and most other keywords should return for elided revisions. --- CHANGELOG.md | 6 +++ cli/src/commands/log.rs | 64 ++++++++++++++++++----- cli/src/config-schema.json | 7 ++- cli/src/config/colors.toml | 13 ++--- cli/src/config/misc.toml | 1 + cli/src/graphlog.rs | 16 ++++-- cli/tests/test_log_command.rs | 98 +++++++++++++++++++++++++++++++++++ 7 files changed, 183 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3814724150..593fb01494 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Built-in pager based on [minus](https://github.com/arijit79/minus/) +* Set config `ui.log-synthetic-elided-nodes = true` to make `jj log` include + synthetic nodes in the graph where some revisions were elided + ([#1252](https://github.com/martinvonz/jj/issues/1252), + [#2971](https://github.com/martinvonz/jj/issues/2971)). This may become the + default depending on feedback. + ### Fixed bugs * On Windows, symlinks in the repo are now materialized as regular files in the diff --git a/cli/src/commands/log.rs b/cli/src/commands/log.rs index 58bad83a55..9324cfd667 100644 --- a/cli/src/commands/log.rs +++ b/cli/src/commands/log.rs @@ -104,6 +104,10 @@ pub(crate) fn cmd_log( Some(value) => value.to_string(), None => command.settings().config().get_string("templates.log")?, }; + let use_elided_nodes = command + .settings() + .config() + .get_bool("ui.log-synthetic-elided-nodes")?; let template = workspace_command.parse_commit_template(&template_string)?; let with_content_format = LogContentFormat::new(ui, command.settings())?; @@ -115,6 +119,7 @@ pub(crate) fn cmd_log( if !args.no_graph { let mut graph = get_graphlog(command.settings(), formatter.raw()); let default_node_symbol = graph.default_node_symbol().to_owned(); + let elided_node_symbol = graph.elided_node_symbol().to_owned(); let forward_iter = TopoGroupedRevsetGraphIterator::new(revset.iter_graph()); let iter: Box> = if args.reversed { Box::new(ReverseRevsetGraphIterator::new(forward_iter)) @@ -122,35 +127,50 @@ pub(crate) fn cmd_log( Box::new(forward_iter) }; for (commit_id, edges) in iter.take(args.limit.unwrap_or(usize::MAX)) { + // The graph is keyed by (CommitId, is_synthetic) let mut graphlog_edges = vec![]; // TODO: Should we update RevsetGraphIterator to yield this flag instead of all // the missing edges since we don't care about where they point here // anyway? let mut has_missing = false; + let mut elided_targets = vec![]; for edge in edges { match edge.edge_type { RevsetGraphEdgeType::Missing => { has_missing = true; } - RevsetGraphEdgeType::Direct => graphlog_edges.push(Edge::Present { - direct: true, - target: edge.target, - }), - RevsetGraphEdgeType::Indirect => graphlog_edges.push(Edge::Present { - direct: false, - target: edge.target, - }), + RevsetGraphEdgeType::Direct => { + graphlog_edges.push(Edge::Present { + direct: true, + target: (edge.target, false), + }); + } + RevsetGraphEdgeType::Indirect => { + if use_elided_nodes { + elided_targets.push(edge.target.clone()); + graphlog_edges.push(Edge::Present { + direct: true, + target: (edge.target, true), + }); + } else { + graphlog_edges.push(Edge::Present { + direct: false, + target: (edge.target, false), + }); + } + } } } if has_missing { graphlog_edges.push(Edge::Missing); } let mut buffer = vec![]; - let commit = store.get_commit(&commit_id)?; + let key = (commit_id, false); + let commit = store.get_commit(&key.0)?; with_content_format.write_graph_text( ui.new_formatter(&mut buffer).as_mut(), |formatter| template.format(&commit, formatter), - || graph.width(&commit_id, &graphlog_edges), + || graph.width(&key, &graphlog_edges), )?; if !buffer.ends_with(b"\n") { buffer.push(b'\n'); @@ -166,18 +186,38 @@ pub(crate) fn cmd_log( &diff_formats, )?; } - let node_symbol = if Some(&commit_id) == wc_commit_id { + let node_symbol = if Some(&key.0) == wc_commit_id { "@" } else { &default_node_symbol }; graph.add_node( - &commit_id, + &key, &graphlog_edges, node_symbol, &String::from_utf8_lossy(&buffer), )?; + for elided_target in elided_targets { + let elided_key = (elided_target, true); + let real_key = (elided_key.0.clone(), false); + let edges = [Edge::Present { + direct: true, + target: real_key, + }]; + let mut buffer = vec![]; + with_content_format.write_graph_text( + ui.new_formatter(&mut buffer).as_mut(), + |formatter| writeln!(formatter.labeled("elided"), "(elided revisions)"), + || graph.width(&elided_key, &edges), + )?; + graph.add_node( + &elided_key, + &edges, + &elided_node_symbol, + &String::from_utf8_lossy(&buffer), + )?; + } } } else { let iter: Box> = if args.reversed { diff --git a/cli/src/config-schema.json b/cli/src/config-schema.json index 966d61e50c..c3f0285fa6 100644 --- a/cli/src/config-schema.json +++ b/cli/src/config-schema.json @@ -119,6 +119,11 @@ "description": "Whether to wrap log template output", "default": false }, + "log-synthetic-elided-nodes": { + "type": "boolean", + "description": "Whether to render elided parts of the graph as synthetic nodes.", + "default": false + }, "editor": { "type": "string", "description": "Editor to use for commands that involve editing text" @@ -381,4 +386,4 @@ } } } -} +} \ No newline at end of file diff --git a/cli/src/config/colors.toml b/cli/src/config/colors.toml index 6f9f2d6b0a..a9fde23526 100644 --- a/cli/src/config/colors.toml +++ b/cli/src/config/colors.toml @@ -10,10 +10,10 @@ "change_id" = "magenta" # Unique prefixes and the rest for change & commit ids -"prefix" = { bold = true} +"prefix" = { bold = true } "rest" = "bright black" "divergent rest" = "red" -"divergent prefix" = {fg = "red", underline=true} +"divergent prefix" = { fg = "red", underline = true } "hidden prefix" = "default" "email" = "yellow" @@ -28,13 +28,14 @@ "git_refs" = "green" "git_head" = "green" "divergent" = "red" -"divergent change_id"="red" +"divergent change_id" = "red" "conflict" = "red" "empty" = "green" "placeholder" = "red" "description placeholder" = "yellow" "empty description placeholder" = "green" "separator" = "bright black" +"elided" = "bright black" "root" = "green" "working_copy" = { bold = true } @@ -45,13 +46,13 @@ "working_copy email" = "yellow" "working_copy timestamp" = "bright cyan" "working_copy working_copies" = "bright magenta" -"working_copy branch" = "bright magenta" +"working_copy branch" = "bright magenta" "working_copy branches" = "bright magenta" "working_copy local_branches" = "bright magenta" "working_copy remote_branches" = "bright magenta" "working_copy tags" = "bright magenta" "working_copy git_refs" = "bright green" -"working_copy divergent" = "bright red" +"working_copy divergent" = "bright red" "working_copy divergent change_id" = "bright red" "working_copy conflict" = "bright red" "working_copy empty" = "bright green" @@ -71,5 +72,5 @@ "op_log time" = "cyan" "op_log current_operation" = { bold = true } "op_log current_operation id" = "bright blue" -"op_log current_operation user" = "yellow" # No bright yellow, see comment above +"op_log current_operation user" = "yellow" # No bright yellow, see comment above "op_log current_operation time" = "bright cyan" diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml index d4684ee202..e247778fdc 100644 --- a/cli/src/config/misc.toml +++ b/cli/src/config/misc.toml @@ -8,6 +8,7 @@ tree-level-conflicts = true paginate = "auto" pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } } log-word-wrap = false +log-synthetic-elided-nodes = false [snapshot] max-new-file-size = "1MiB" diff --git a/cli/src/graphlog.rs b/cli/src/graphlog.rs index a7736d71f1..74daa1c298 100644 --- a/cli/src/graphlog.rs +++ b/cli/src/graphlog.rs @@ -58,6 +58,8 @@ pub trait GraphLog { fn default_node_symbol(&self) -> &str; + fn elided_node_symbol(&self) -> &str; + fn width(&self, id: &K, edges: &[Edge]) -> usize; } @@ -65,6 +67,7 @@ pub struct SaplingGraphLog<'writer, R> { renderer: R, writer: &'writer mut dyn Write, default_node_symbol: String, + elided_node_symbol: String, } impl From<&Edge> for Ancestor { @@ -106,6 +109,10 @@ where &self.default_node_symbol } + fn elided_node_symbol(&self) -> &str { + &self.elided_node_symbol + } + fn width(&self, id: &K, edges: &[Edge]) -> usize { let parents = edges.iter().map_into().collect(); let w: u64 = self.renderer.width(Some(id), Some(&parents)); @@ -118,6 +125,7 @@ impl<'writer, R> SaplingGraphLog<'writer, R> { renderer: R, formatter: &'writer mut dyn Write, default_node_symbol: &str, + elided_node_symbol: &str, ) -> Box + 'writer> where K: Clone + Eq + Hash + 'writer, @@ -127,6 +135,7 @@ impl<'writer, R> SaplingGraphLog<'writer, R> { renderer, writer: formatter, default_node_symbol: default_node_symbol.to_owned(), + elided_node_symbol: elided_node_symbol.to_owned(), }) } } @@ -142,10 +151,11 @@ pub fn get_graphlog<'a, K: Clone + Eq + Hash + 'a>( builder.build_box_drawing().with_square_glyphs(), formatter, "◉", + "◌", ), - "ascii" => SaplingGraphLog::create(builder.build_ascii(), formatter, "o"), - "ascii-large" => SaplingGraphLog::create(builder.build_ascii_large(), formatter, "o"), + "ascii" => SaplingGraphLog::create(builder.build_ascii(), formatter, "o", "."), + "ascii-large" => SaplingGraphLog::create(builder.build_ascii_large(), formatter, "o", "."), // "curved" - _ => SaplingGraphLog::create(builder.build_box_drawing(), formatter, "◉"), + _ => SaplingGraphLog::create(builder.build_box_drawing(), formatter, "◉", "◌"), } } diff --git a/cli/tests/test_log_command.rs b/cli/tests/test_log_command.rs index 32d1220a2b..c2f5a14310 100644 --- a/cli/tests/test_log_command.rs +++ b/cli/tests/test_log_command.rs @@ -1333,3 +1333,101 @@ fn test_log_word_wrap() { merge "###); } + +#[test] +fn test_elided() { + // Test that elided commits are shown as synthetic nodes. + let test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let repo_path = test_env.env_root().join("repo"); + + test_env.jj_cmd_ok(&repo_path, &["describe", "-m", "initial"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "main branch 1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "main branch 2"]); + test_env.jj_cmd_ok(&repo_path, &["new", "@--", "-m", "side branch 1"]); + test_env.jj_cmd_ok(&repo_path, &["new", "-m", "side branch 2"]); + test_env.jj_cmd_ok( + &repo_path, + &["new", "-m", "merge", r#"description("main branch 2")"#, "@"], + ); + + let get_log = |revs: &str| -> String { + test_env.jj_cmd_success( + &repo_path, + &["log", "-T", r#"description ++ "\n""#, "-r", revs], + ) + }; + + // Test the setup + insta::assert_snapshot!(get_log("::"), @r###" + @ merge + ├─╮ + │ ◉ side branch 2 + │ │ + │ ◉ side branch 1 + │ │ + ◉ │ main branch 2 + │ │ + ◉ │ main branch 1 + ├─╯ + ◉ initial + │ + ◉ + "###); + + // Elide some commits from each side of the merge. It's unclear that a revision + // was skipped on the left side. + insta::assert_snapshot!(get_log("@ | @- | description(initial)"), @r###" + @ merge + ├─╮ + │ ◉ side branch 2 + │ ╷ + ◉ ╷ main branch 2 + ├─╯ + ◉ initial + │ + ~ + "###); + + // Elide shared commits. It's unclear that a revision was skipped on the right + // side (#1252). + insta::assert_snapshot!(get_log("@-- | root()"), @r###" + ◉ side branch 1 + ╷ + ╷ ◉ main branch 1 + ╭─╯ + ◉ + "###); + + // Now test the same thing with synthetic nodes for elided commits + + // Elide some commits from each side of the merge + test_env.add_config("ui.log-synthetic-elided-nodes = true"); + insta::assert_snapshot!(get_log("@ | @- | description(initial)"), @r###" + @ merge + ├─╮ + │ ◉ side branch 2 + │ │ + │ ◌ (elided revisions) + ◉ │ main branch 2 + │ │ + ◌ │ (elided revisions) + ├─╯ + ◉ initial + │ + ~ + "###); + + // Elide shared commits. To keep the implementation simple, it still gets + // rendered as two synthetic nodes. + insta::assert_snapshot!(get_log("@-- | root()"), @r###" + ◉ side branch 1 + │ + ◌ (elided revisions) + │ ◉ main branch 1 + │ │ + │ ◌ (elided revisions) + ├─╯ + ◉ + "###); +}