Repository: mozilla/cargo-vet
Pull request #373 (2023-01-16 18:12:17)

Diff each file separately, combining outputs with a pager
This greatly improves on the filtering capabilities which I built in https://github.com/mozilla/cargo-vet/pull/371, making them much more reliable. Without this change, a poor interaction between between orderfiles and --skip-to for diffs with --no-index in git diff would cause some files to occasionally not be ignored when they were supposed to be (such as Cargo.lock files) which could lead to issues when used with some git revisions.

The new approach means that we always generate diffs for only the files we intend to, in a more reliable manner. Like the GIT_EDITOR setup, this required some work to integrate with the git-provided pager, however most of the work could be re-used from GIT_EDITOR.

This will also make the `cargo vet diff` command interact better with the --out-file command line option, as previously the output would always be written to stdout.


Patch:
diff --git a/src/editor.rs b/src/git_tool.rs
similarity index 69%
rename from src/editor.rs
rename to src/git_tool.rs
index 6a5f062..fde4e92 100644
--- a/src/editor.rs
+++ b/src/git_tool.rs
@@ -1,15 +1,16 @@
-//! Helper utilities for opening files in the editor.
+//! Helper utilities for invoking git configured tools.
 
 use std::fs::File;
 use std::io::{self, BufRead, BufReader, Write};
 use std::path::{Path, PathBuf};
-use std::process::{Command, ExitStatus};
+use std::process::{Child, Command, ExitStatus, Stdio};
 use std::str;
 
 use tempfile::NamedTempFile;
 use tracing::warn;
 
 use crate::errors::EditError;
+use crate::out::Out;
 
 #[cfg(windows)]
 fn git_sh_path() -> Option<PathBuf> {
@@ -37,6 +38,27 @@ fn git_sh_path() -> Option<PathBuf> {
     Some("/bin/sh".into())
 }
 
+fn git_config(config: &str) -> Option<String> {
+    let output = Command::new("git")
+        .arg("config")
+        .arg(config)
+        .output()
+        .ok()?;
+    if !output.status.success() {
+        return None;
+    }
+    Some(str::from_utf8(&output.stdout).ok()?.trim().to_owned())
+}
+
+fn git_config_bool(config: &str) -> Option<bool> {
+    let value = git_config(config)?;
+    match &value[..] {
+        "true" | "yes" | "on" => Some(true),
+        "" | "false" | "no" | "off" => Some(false),
+        _ => Some(value.parse::<i64>().ok()? != 0),
+    }
+}
+
 /// Read the git configuration to determine the value for GIT_EDITOR.
 fn git_editor() -> Option<String> {
     // Testing environment variable to force using the fallback editor instead
@@ -237,3 +259,102 @@ impl<'a> Editor<'a> {
         Ok(lines.join("\n"))
     }
 }
+
+/// Read the git configuration to determine the value for GIT_EDITOR.
+fn git_pager() -> Option<String> {
+    let output = Command::new("git")
+        .arg("var")
+        .arg("GIT_PAGER")
+        .output()
+        .ok()?;
+    if !output.status.success() {
+        return None;
+    }
+    let pager = str::from_utf8(&output.stdout).ok()?.trim().to_owned();
+    if pager == "cat" {
+        return None;
+    }
+    Some(pager)
+}
+
+/// Get a Command which can be used to invoke the user's EDITOR to edit a
+/// document when passed an argument. This will try to use the user's configured
+/// GIT_EDITOR when possible.
+fn pager_command(out: &dyn Out) -> Option<Command> {
+    if !out.is_term() {
+        return None;
+    }
+
+    // Try to use the user's configured pager if we're able to locate their git
+    // install. If this fails, we'll not use a pager. Don't bother warning in
+    // that case.
+    let git_sh = git_sh_path()?;
+    let git_pager = git_pager()?;
+
+    let mut cmd = Command::new(git_sh);
+    cmd.arg("-c")
+        .arg(format!("{} \"$@\"", git_pager))
+        .arg(git_pager);
+    // These environment variables are hard-coded into `git` at build
+    // time, and are required to support colors in `less`.
+    cmd.env("LESS", "FRX").env("LV", "-c");
+    Some(cmd)
+}
+
+pub struct Pager<'a> {
+    out: &'a dyn Out,
+    child: Option<Child>,
+    use_color: bool,
+}
+
+impl<'a> Pager<'a> {
+    /// Create a new pager for the given output stream, or a dummy pager if no pager is available.
+    pub fn new(out: &'a dyn Out) -> Result<Self, io::Error> {
+        let child = if let Some(mut cmd) = pager_command(out) {
+            Some(cmd.stdin(Stdio::piped()).spawn()?)
+        } else {
+            None
+        };
+        let use_color = child.is_some()
+            && out.is_term()
+            && console::colors_enabled()
+            && git_config_bool("pager.color")
+                .or_else(|| git_config_bool("color.pager"))
+                .unwrap_or(true);
+        Ok(Pager {
+            out,
+            child,
+            use_color,
+        })
+    }
+
+    /// Should attempts to write to this pager include ANSI color codes?
+    pub fn use_color(&self) -> bool {
+        self.use_color
+    }
+
+    /// Wait for the pager to stop running, so it's OK to start writing to
+    /// output again.
+    pub fn wait(self) -> Result<(), io::Error> {
+        if let Some(mut child) = self.child {
+            child.wait()?;
+        }
+        Ok(())
+    }
+}
+
+impl<'a> io::Write for Pager<'a> {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        match &mut self.child {
+            Some(child) => child.stdin.as_mut().unwrap().write(buf),
+            None => self.out.write(buf),
+        }
+    }
+
+    fn flush(&mut self) -> io::Result<()> {
+        match &mut self.child {
+            Some(child) => child.stdin.as_mut().unwrap().flush(),
+            None => Ok(()),
+        }
+    }
+}
diff --git a/src/main.rs b/src/main.rs
index 3709006..757eccb 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -2,6 +2,7 @@ use std::collections::HashMap;
 use std::io::{BufRead, BufReader};
 use std::ops::Deref;
 use std::panic::panic_any;
+use std::process::Stdio;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
 use std::{fs::File, io, panic, path::PathBuf};
@@ -26,7 +27,6 @@ use out::{progress_bar, IncProgressOnDrop};
 use reqwest::Url;
 use serde::de::Deserialize;
 use serialization::spanned::Spanned;
-use tempfile::NamedTempFile;
 use thiserror::Error;
 use tracing::{error, info, trace, warn};
 
@@ -39,15 +39,16 @@ use crate::format::{
     ExemptedDependency, FetchCommand, ImportsFile, MetaConfig, MetaConfigInstance, PackageStr,
     SortedMap, StoreInfo,
 };
+use crate::git_tool::Pager;
 use crate::out::{indeterminate_spinner, Out, StderrLogWriter, MULTIPROGRESS};
 use crate::resolver::{CriteriaMapper, CriteriaNamespace, DepGraph, ResolveDepth};
 use crate::storage::{Cache, Store};
 
 mod cli;
-mod editor;
 pub mod errors;
 mod flock;
 pub mod format;
+mod git_tool;
 pub mod network;
 mod out;
 pub mod resolver;
@@ -1407,7 +1408,7 @@ fn cmd_diff(out: &Arc<dyn Out>, cfg: &Config, sub_args: &DiffArgs) -> Result<(),
     let version2 = &sub_args.version2;
     let package = &*sub_args.package;
 
-    let (fetched1, fetched2, filter_config) = {
+    let to_compare = {
         let network = Network::acquire(cfg);
         let store = Store::acquire(cfg, network.as_ref(), false)?;
         let cache = Cache::acquire(cfg)?;
@@ -1449,14 +1450,14 @@ fn cmd_diff(out: &Arc<dyn Out>, cfg: &Config, sub_args: &DiffArgs) -> Result<(),
             // NOTE: don't `try_join` everything as we don't want to abort the
             // prompt to the user if the download fails while it is being shown, as
             // that could be disorienting.
-            let (pkgs, eulas) = tokio::join!(
+            let (to_compare, eulas) = tokio::join!(
                 async {
                     let (pkg1, pkg2) = tokio::try_join!(
                         cache.fetch_package(network.as_ref(), package, version1),
                         cache.fetch_package(network.as_ref(), package, version2)
                     )?;
-                    let (_, filter_config) = cache.diffstat_package(&pkg1, &pkg2).await?;
-                    Ok::<_, FetchAndDiffError>((pkg1, pkg2, filter_config))
+                    let (_, to_compare) = cache.diffstat_package(&pkg1, &pkg2).await?;
+                    Ok::<_, FetchAndDiffError>(to_compare)
                 },
                 prompt_criteria_eulas(
                     out,
@@ -1470,34 +1471,39 @@ fn cmd_diff(out: &Arc<dyn Out>, cfg: &Config, sub_args: &DiffArgs) -> Result<(),
                 )
             );
             eulas.into_diagnostic()?;
-            pkgs.into_diagnostic()
+            to_compare.into_diagnostic()
         })?
     };
 
     writeln!(out);
 
-    // Create a temporary orderfile with the computed values in order to skip
-    // diffs which we want to ignore using --skip-to.
-    let mut orderfile = NamedTempFile::new().into_diagnostic()?;
-    filter_config
-        .write_order_file(&mut orderfile)
-        .into_diagnostic()?;
+    // Start a pager to show the output from our diff invocations. This will
+    // fall back to just printing to `stdout` if no pager is available or we're
+    // not piped to a terminal.
+    let mut pager = Pager::new(&**out).into_diagnostic()?;
+
+    for (from, to) in to_compare {
+        let output = std::process::Command::new("git")
+            .arg("-c")
+            .arg("core.safecrlf=false")
+            .arg("diff")
+            .arg(if pager.use_color() {
+                "--color=always"
+            } else {
+                "--color=never"
+            })
+            .arg("--no-index")
+            .arg("--ignore-cr-at-eol")
+            .arg(&from)
+            .arg(&to)
+            .stdout(Stdio::piped())
+            .output()
+            .map_err(CommandError::CommandFailed)
+            .into_diagnostic()?;
+        io::Write::write_all(&mut pager, &output.stdout).into_diagnostic()?;
+    }
 
-    std::process::Command::new("git")
-        .arg("-c")
-        .arg("core.safecrlf=false")
-        .arg("diff")
-        .arg("--no-index")
-        .arg("--ignore-cr-at-eol")
-        .arg("-O")
-        .arg(orderfile.path())
-        .arg("--skip-to")
-        .arg(filter_config.skip_to())
-        .arg(&fetched1)
-        .arg(&fetched2)
-        .status()
-        .map_err(CommandError::CommandFailed)
-        .into_diagnostic()?;
+    pager.wait().into_diagnostic()?;
 
     writeln!(out, "\nUse |cargo vet certify| to record your audit.");
 
diff --git a/src/out.rs b/src/out.rs
index 9032f03..d6e3f14 100644
--- a/src/out.rs
+++ b/src/out.rs
@@ -4,7 +4,7 @@
 //! In general, this type should be preferred over directly writing to stdout or
 //! stderr.
 
-use crate::editor::Editor;
+use crate::git_tool::Editor;
 use console::{Style, Term};
 use indicatif::{MultiProgress, ProgressBar, ProgressDrawTarget, ProgressStyle};
 use lazy_static::lazy_static;
diff --git a/src/storage.rs b/src/storage.rs
index ca078a4..6dff4d7 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -1046,39 +1046,6 @@ fn parse_imported_audit(valid_criteria: &[CriteriaName], value: toml::Value) ->
     Some(audit)
 }
 
-/// Helper type used to track information required to filter `git diff` output
-/// to match the filtering we do with diffstat.
-///
-/// This is done by using the `-O` (orderfile) and `--skip-to` arguments to `git
-/// diff` to place the paths we want to ignore first in the diff output, then
-/// specify `--skip-to` to skip past them.
-pub struct DiffFilterConfig {
-    paths: Vec<String>,
-    skip_to: Option<String>,
-}
-
-impl DiffFilterConfig {
-    /// Write out the contents of the orderfile which must be passed to the `-O`
-    /// flag for `git diff` to perform filtering.
-    pub fn write_order_file(&self, order_file: &mut impl io::Write) -> io::Result<()> {
-        for file in self.paths.iter().chain(&self.skip_to) {
-            order_file.write_all(file.as_bytes())?;
-        }
-        Ok(())
-    }
-
-    /// Get the path which should be passed to the `--skip-to` flag for `git
-    /// diff` to perform filtering.
-    pub fn skip_to(&self) -> &str {
-        // If we have no `skip_to`, it means that files were only removed, so
-        // skip to the first removed file.
-        match &self.skip_to {
-            Some(skip_to) => skip_to,
-            None => "/dev/null",
-        }
-    }
-}
-
 /// A Registry in CARGO_HOME (usually the crates.io one)
 pub struct CargoRegistry {
     /// The queryable index
@@ -1428,7 +1395,7 @@ impl Cache {
         &self,
         version1: &Path,
         version2: &Path,
-    ) -> Result<(DiffStat, DiffFilterConfig), DiffError> {
+    ) -> Result<(DiffStat, Vec<(PathBuf, PathBuf)>), DiffError> {
         let _permit = self
             .diff_semaphore
             .acquire()
@@ -1463,10 +1430,7 @@ impl Cache {
             insertions: 0,
             deletions: 0,
         };
-        let mut filter_config = DiffFilterConfig {
-            paths: Vec::new(),
-            skip_to: None,
-        };
+        let mut to_compare = Vec::new();
 
         // Thanks to the `-z` flag the output takes the rough format of:
         // "{INSERTED}\t{DELETED}\t\0{FROM_PATH}\0{TO_PATH}\0" for each file
@@ -1475,7 +1439,7 @@ impl Cache {
         // inserted & deleted counts.
         let output = String::from_utf8(out.stdout).map_err(CommandError::BadOutput)?;
         let mut chunks = output.split('\0');
-        while let (Some(changes_s), Some(_from_s), Some(to_s)) =
+        while let (Some(changes_s), Some(from_s), Some(to_s)) =
             (chunks.next(), chunks.next(), chunks.next())
         {
             // Check if the 'to' path is one of the files which is ignored. We
@@ -1485,14 +1449,12 @@ impl Cache {
                     .strip_prefix(version2)
                     .map_err(DiffError::UnexpectedPath)?;
                 if DIFF_SKIP_PATHS.iter().any(|p| Path::new(p) == to_path) {
-                    filter_config.paths.push(to_s.to_owned());
                     continue;
                 }
-                if filter_config.skip_to.is_none() {
-                    filter_config.skip_to = Some(to_s.to_owned());
-                }
             }
 
+            to_compare.push((from_s.into(), to_s.into()));
+
             diffstat.files_changed += 1;
 
             match changes_s.trim().split_once('\t') {
@@ -1508,7 +1470,7 @@ impl Cache {
                 None => Err(DiffError::InvalidOutput)?,
             };
         }
-        Ok((diffstat, filter_config))
+        Ok((diffstat, to_compare))
     }
 
     #[tracing::instrument(skip(self, network), err)]


Test patch:
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index e5e077e..59441d4 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -11,11 +11,11 @@ use clap::Parser;
 use serde_json::{json, Value};
 
 use crate::{
-    editor::Editor,
     format::{
         AuditKind, CriteriaName, CriteriaStr, DependencyCriteria, FastMap, MetaConfig, PackageName,
         PackageStr, PolicyEntry, SortedSet, VersionReq, SAFE_TO_DEPLOY, SAFE_TO_RUN,
     },
+    git_tool::Editor,
     init_files,
     out::Out,
     resolver::{ResolveDepth, ResolveReport},