Update cargo tree from review comments.

Author: ehuss

src/cargo/ops/tree/format/mod.rs

@@ -1,6 +1,6 @@
 use self::parse::{Parser, RawChunk};
 use super::{Graph, Node};
-use anyhow::{anyhow, Error};
+use anyhow::{bail, Error};
 use std::fmt;
 
 mod parse;
@@ -27,9 +27,9 @@ impl Pattern {
                 RawChunk::Argument("r") => Chunk::Repository,
                 RawChunk::Argument("f") => Chunk::Features,
                 RawChunk::Argument(a) => {
-                    return Err(anyhow!("unsupported pattern `{}`", a));
+                    bail!("unsupported pattern `{}`", a);
                 }
-                RawChunk::Error(err) => return Err(anyhow!("{}", err)),
+                RawChunk::Error(err) => bail!("{}", err),
             };
             chunks.push(chunk);
         }
@@ -63,8 +63,8 @@ impl<'a> fmt::Display for Display<'a> {
             } => {
                 let package = self.graph.package_for_id(*package_id);
                 for chunk in &self.pattern.0 {
-                    match *chunk {
-                        Chunk::Raw(ref s) => fmt.write_str(s)?,
+                    match chunk {
+                        Chunk::Raw(s) => fmt.write_str(s)?,
                         Chunk::Package => {
                             write!(fmt, "{} v{}", package.name(), package.version())?;
 
@@ -74,12 +74,12 @@ impl<'a> fmt::Display for Display<'a> {
                             }
                         }
                         Chunk::License => {
-                            if let Some(ref license) = package.manifest().metadata().license {
+                            if let Some(license) = &package.manifest().metadata().license {
                                 write!(fmt, "{}", license)?;
                             }
                         }
                         Chunk::Repository => {
-                            if let Some(ref repository) = package.manifest().metadata().repository {
+                            if let Some(repository) = &package.manifest().metadata().repository {
                                 write!(fmt, "{}", repository)?;
                             }
                         }
@@ -98,6 +98,8 @@ impl<'a> fmt::Display for Display<'a> {
                             write!(fmt, " (command-line)")?;
                         }
                     }
+                    // The node_index in Node::Feature must point to a package
+                    // node, see `add_feature`.
                     _ => panic!("unexpected feature node {:?}", for_node),
                 }
             }

src/cargo/ops/tree/format/parse.rs

@@ -1,12 +1,34 @@
+//! Parser for the `--format` string for `cargo tree`.
+
 use std::iter;
 use std::str;
 
 pub enum RawChunk<'a> {
+    /// Raw text to include in the output.
     Text(&'a str),
+    /// A substitution to place in the output. For example, the argument "p"
+    /// emits the package name.
     Argument(&'a str),
+    /// Indicates an error in the format string. The given string is a
+    /// human-readable message explaining the error.
     Error(&'static str),
 }
 
+/// `cargo tree` format parser.
+///
+/// The format string indicates how each package should be displayed. It
+/// includes simple markers surrounded in curly braces that will be
+/// substituted with their corresponding values. For example, the text
+/// "{p} license:{l}" will substitute the `{p}` with the package name/version
+/// (and optionally source), and the `{l}` will be the license from
+/// `Cargo.toml`.
+///
+/// Substitutions are alphabetic characters between curly braces, like `{p}`
+/// or `{foo}`. The actual interpretation of these are done in the `Pattern`
+/// struct.
+///
+/// Bare curly braces can be included in the output with double braces like
+/// `{{` will include a single `{`, similar to Rust's format strings.
 pub struct Parser<'a> {
     s: &'a str,
     it: iter::Peekable<str::CharIndices<'a>>,

src/cargo/ops/tree/graph.rs

@@ -92,17 +92,15 @@ impl<'a> Graph<'a> {
     }
 
     /// Returns a list of nodes the given node index points to for the given kind.
-    ///
-    /// Returns None if there are none.
-    pub fn connected_nodes(&self, from: usize, kind: &Edge) -> Option<Vec<usize>> {
+    pub fn connected_nodes(&self, from: usize, kind: &Edge) -> Vec<usize> {
         match self.edges[from].0.get(kind) {
             Some(indexes) => {
                 // Created a sorted list for consistent output.
                 let mut indexes = indexes.clone();
                 indexes.sort_unstable_by(|a, b| self.nodes[*a].cmp(&self.nodes[*b]));
-                Some(indexes)
+                indexes
             }
-            None => None,
+            None => Vec::new(),
         }
     }
 
@@ -123,6 +121,8 @@ impl<'a> Graph<'a> {
             })
             .map(|(i, node)| (node, i))
             .collect();
+        // Sort for consistent output (the same command should always return
+        // the same output). "unstable" since nodes should always be unique.
         result.sort_unstable();
         result.into_iter().map(|(_node, i)| i).collect()
     }
@@ -403,13 +403,15 @@ fn add_feature(
     to: usize,
     kind: Edge,
 ) -> usize {
+    // `to` *must* point to a package node.
+    assert!(matches! {graph.nodes[to], Node::Package{..}});
     let node = Node::Feature {
         node_index: to,
         name,
     };
-    let (node_index, _new) = match graph.index.get(&node) {
-        Some(idx) => (*idx, false),
-        None => (graph.add_node(node), true),
+    let node_index = match graph.index.get(&node) {
+        Some(idx) => *idx,
+        None => graph.add_node(node),
     };
     if let Some(from) = from {
         graph.edges[from].add_edge(kind, node_index);

src/cargo/ops/tree/mod.rs

@@ -291,10 +291,10 @@ fn print_dependencies<'a>(
     print_stack: &mut Vec<usize>,
     kind: &Edge,
 ) {
-    let deps = match graph.connected_nodes(node_index, kind) {
-        Some(deps) => deps,
-        None => return,
-    };
+    let deps = graph.connected_nodes(node_index, kind);
+    if deps.is_empty() {
+        return;
+    }
 
     let name = match kind {
         Edge::Dep(DepKind::Normal) => None,