Avoid writing config.toml and other files if no significant changes.

After this commit `audits.toml`, `config.toml`, and `imports.lock` will
not be written if their contents wouldn't have changed ("contents" in a
business logic sense - ignoring comments and/or formatting differences).

Fixes mozilla#589

src/errors.rs

@@ -448,9 +448,6 @@ pub enum StoreValidateError {
     InvalidCriteria(InvalidCriteriaError),
     #[diagnostic(transparent)]
     #[error(transparent)]
-    BadFormat(BadFormatError),
-    #[diagnostic(transparent)]
-    #[error(transparent)]
     BadWildcardEndDate(BadWildcardEndDateError),
     #[error("imports.lock is out-of-date with respect to configuration")]
     #[diagnostic(help("run `cargo vet` without --locked to update imports"))]
@@ -469,13 +466,6 @@ pub struct InvalidCriteriaError {
     pub valid_names: Arc<Vec<String>>,
 }
 
-#[derive(Debug, Error, Diagnostic)]
-#[error("A file in the store is not correctly formatted:\n\n{unified_diff}")]
-#[diagnostic(help("run `cargo vet` without --locked to reformat files in the store"))]
-pub struct BadFormatError {
-    pub unified_diff: String,
-}
-
 #[derive(Debug, Error, Diagnostic)]
 #[error("'{date}' is more than a year in the future")]
 #[diagnostic(help("wildcard audits must end at most a year in the future ({max})"))]

src/format.rs

@@ -551,7 +551,7 @@ pub struct TrustEntry {
 ////////////////////////////////////////////////////////////////////////////////////
 
 /// config.toml
-#[derive(Debug, serde::Serialize, serde::Deserialize, Clone)]
+#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, PartialEq, Eq)]
 pub struct ConfigFile {
     #[serde(rename = "cargo-vet")]
     #[serde(default = "CargoVetConfig::missing")]
@@ -595,7 +595,7 @@ fn is_default_criteria(val: &CriteriaName) -> bool {
 }
 
 /// The table of crate policies.
-#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, Default)]
+#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, Default, PartialEq, Eq)]
 #[serde(try_from = "serialization::policy::AllPolicies")]
 #[serde(into = "serialization::policy::AllPolicies")]
 pub struct Policy {
@@ -734,7 +734,7 @@ impl<'a> IntoIterator for &'a Policy {
 /// If the crate exists as a third-party crate anywhere in the dependency tree, crate versions for
 /// _all_ and _only_ the versions present in the dependency tree must be provided to set policies.
 /// Otherwise, versions may be omitted.
-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 // We have to use a slightly different serialization than than `serde(untagged)`, because toml only
 // parses `Spanned` elements (as contained in `PolicyEntry`) through their own Deseralizer, and
 // `serde(untagged)` deserializes everything into a buffer first to try different deserialization
@@ -759,7 +759,7 @@ pub enum PackagePolicyEntry {
 /// If this sounds overwhelming, don't worry, everything defaults to "nothing special"
 /// and an empty PolicyTable basically just means "everything should satisfy the
 /// default criteria in audits.toml".
-#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, Default)]
+#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, Default, PartialEq, Eq)]
 pub struct PolicyEntry {
     /// Whether this nominally-first-party crate should actually be subject to audits
     /// as-if it was third-party, based on matches to crates.io packages with the same
@@ -831,7 +831,7 @@ pub static DEFAULT_POLICY_CRITERIA: CriteriaStr = SAFE_TO_DEPLOY;
 pub static DEFAULT_POLICY_DEV_CRITERIA: CriteriaStr = SAFE_TO_RUN;
 
 /// A remote audits.toml that we trust the contents of (by virtue of trusting the maintainer).
-#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, Default)]
+#[derive(Debug, serde::Serialize, serde::Deserialize, Clone, Default, PartialEq, Eq)]
 pub struct RemoteImport {
     /// URL(s) of the foreign audits.toml
     #[serde(with = "serialization::string_or_vec")]
@@ -965,7 +965,7 @@ impl<'de> Deserialize<'de> for StoreVersion {
 }
 
 /// Cargo vet config metadata field for the store's config file.
-#[derive(Debug, Serialize, Deserialize, Clone)]
+#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
 pub struct CargoVetConfig {
     pub version: StoreVersion,
 }

src/storage.rs

@@ -21,13 +21,13 @@ use tracing::{error, info, log::warn, trace};
 use crate::{
     criteria::CriteriaMapper,
     errors::{
-        AggregateError, BadFormatError, BadWildcardEndDateError, CacheAcquireError,
-        CacheCommitError, CertifyError, CommandError, CrateInfoError, CriteriaChangeError,
-        CriteriaChangeErrors, DiffError, DownloadError, FetchAndDiffError,
-        FetchAuditAggregateError, FetchAuditError, FetchError, FetchRegistryError, FlockError,
-        InvalidCriteriaError, JsonParseError, LoadJsonError, LoadTomlError, SourceFile,
-        StoreAcquireError, StoreCommitError, StoreCreateError, StoreJsonError, StoreTomlError,
-        StoreValidateError, StoreValidateErrors, TomlParseError, UnpackCheckoutError, UnpackError,
+        AggregateError, BadWildcardEndDateError, CacheAcquireError, CacheCommitError, CertifyError,
+        CommandError, CrateInfoError, CriteriaChangeError, CriteriaChangeErrors, DiffError,
+        DownloadError, FetchAndDiffError, FetchAuditAggregateError, FetchAuditError, FetchError,
+        FetchRegistryError, FlockError, InvalidCriteriaError, JsonParseError, LoadJsonError,
+        LoadTomlError, SourceFile, StoreAcquireError, StoreCommitError, StoreCreateError,
+        StoreJsonError, StoreTomlError, StoreValidateError, StoreValidateErrors, TomlParseError,
+        UnpackCheckoutError, UnpackError,
     },
     flock::{FileLock, Filesystem},
     format::{
@@ -496,16 +496,43 @@ impl Store {
 
     /// Commit the store's contents back to disk
     pub fn commit(self) -> Result<(), StoreCommitError> {
+        fn has_diffs<T: for<'a> Deserialize<'a> + Eq>(
+            old_source: &SourceFile,
+            new_content: &T,
+        ) -> bool {
+            toml::from_str(old_source.source())
+                .ok()
+                .map(|old_content: T| old_content != *new_content)
+                .unwrap_or(true)
+        }
         // TODO: make this truly transactional?
         // (With a dir rename? Does that work with the lock? Fine because it's already closed?)
         if let Some(lock) = self.lock {
-            let mut audits = lock.write_audits()?;
-            let mut config = lock.write_config()?;
-            let mut imports = lock.write_imports()?;
+            let audits = if has_diffs(&self.audits_src, &self.audits) {
+                Some(lock.write_audits()?)
+            } else {
+                None
+            };
+            let config = if has_diffs(&self.config_src, &self.config) {
+                Some(lock.write_config()?)
+            } else {
+                None
+            };
+            let imports = if has_diffs(&self.imports_src, &self.imports) {
+                Some(lock.write_imports()?)
+            } else {
+                None
+            };
             let user_info = user_info_map(&self.imports);
-            audits.write_all(store_audits(self.audits, &user_info)?.as_bytes())?;
-            config.write_all(store_config(self.config)?.as_bytes())?;
-            imports.write_all(store_imports(self.imports, &user_info)?.as_bytes())?;
+            if let Some(mut audits) = audits {
+                audits.write_all(store_audits(self.audits, &user_info)?.as_bytes())?;
+            }
+            if let Some(mut config) = config {
+                config.write_all(store_config(self.config)?.as_bytes())?;
+            }
+            if let Some(mut imports) = imports {
+                imports.write_all(store_imports(self.imports, &user_info)?.as_bytes())?;
+            }
         }
         Ok(())
     }
@@ -661,46 +688,6 @@ impl Store {
             }
         }
 
-        // If requested, verify that files in the store are correctly formatted
-        // and have no unrecognized fields. We don't want to be reformatting
-        // them or dropping unused fields while in CI, as those changes will be
-        // ignored.
-        if check_file_formatting {
-            let user_info = user_info_map(&self.imports);
-            for (name, old, new) in [
-                (
-                    CONFIG_TOML,
-                    self.config_src.source(),
-                    store_config(self.config.clone())
-                        .unwrap_or_else(|_| self.config_src.source().to_owned()),
-                ),
-                (
-                    AUDITS_TOML,
-                    self.audits_src.source(),
-                    store_audits(self.audits.clone(), &user_info)
-                        .unwrap_or_else(|_| self.audits_src.source().to_owned()),
-                ),
-                (
-                    IMPORTS_LOCK,
-                    self.imports_src.source(),
-                    store_imports(self.imports.clone(), &user_info)
-                        .unwrap_or_else(|_| self.imports_src.source().to_owned()),
-                ),
-            ] {
-                if old.trim_end() != new.trim_end() {
-                    errors.push(StoreValidateError::BadFormat(BadFormatError {
-                        unified_diff: unified_diff(
-                            Algorithm::Myers,
-                            old,
-                            &new,
-                            3,
-                            Some((&format!("old/{name}"), &format!("new/{name}"))),
-                        ),
-                    }));
-                }
-            }
-        }
-
         // If we're locked, and therefore not fetching new live imports,
         // validate that our imports.lock is in sync with config.toml.
         if check_file_formatting && self.imports_lock_outdated() {