refactor logic to handle crate-deletes

Author: syphar

src/index.rs

@@ -1,9 +1,8 @@
-use super::CrateVersion;
-use serde_json;
+use super::{Change, CrateVersion};
 use std::path::Path;
 
 use git2::{
-    build::RepoBuilder, Delta, DiffFormat, Error as GitError, ErrorClass, Object, ObjectType, Oid,
+    build::RepoBuilder, Delta, Error as GitError, ErrorClass, Object, ObjectType, Oid,
     Reference, Repository, Tree,
 };
 use std::str;
@@ -147,11 +146,11 @@ impl Index {
     }
 
     /// As `peek_changes_with_options`, but without the options.
-    pub fn peek_changes(&self) -> Result<(Vec<CrateVersion>, git2::Oid), GitError> {
+    pub fn peek_changes(&self) -> Result<(Vec<Change>, git2::Oid), GitError> {
         self.peek_changes_with_options(None)
     }
 
-    /// Return all `CrateVersion`s that are observed between the last time `fetch_changes(…)` was called
+    /// Return all `Change`s that are observed between the last time `fetch_changes(…)` was called
     /// and the latest state of the `crates.io` index repository, which is obtained by fetching
     /// the remote called `origin`.
     /// The `last_seen_reference()` will not be created or updated.
@@ -169,7 +168,7 @@ impl Index {
     pub fn peek_changes_with_options(
         &self,
         options: Option<&mut git2::FetchOptions<'_>>,
-    ) -> Result<(Vec<CrateVersion>, git2::Oid), GitError> {
+    ) -> Result<(Vec<Change>, git2::Oid), GitError> {
         let from = self
             .last_seen_reference()
             .and_then(|r| {
@@ -201,11 +200,11 @@ impl Index {
     }
 
     /// As `fetch_changes_with_options`, but without the options.
-    pub fn fetch_changes(&self) -> Result<Vec<CrateVersion>, GitError> {
+    pub fn fetch_changes(&self) -> Result<Vec<Change>, GitError> {
         self.fetch_changes_with_options(None)
     }
 
-    /// Return all `CrateVersion`s that are observed between the last time this method was called
+    /// Return all `Change`s that are observed between the last time this method was called
     /// and the latest state of the `crates.io` index repository, which is obtained by fetching
     /// the remote called `origin`.
     /// The `last_seen_reference()` will be created or adjusted to point to the latest fetched
@@ -221,7 +220,7 @@ impl Index {
     pub fn fetch_changes_with_options(
         &self,
         options: Option<&mut git2::FetchOptions<'_>>,
-    ) -> Result<Vec<CrateVersion>, GitError> {
+    ) -> Result<Vec<Change>, GitError> {
         let (changes, to) = self.peek_changes_with_options(options)?;
         self.set_last_seen_reference(to)?;
         Ok(changes)
@@ -253,7 +252,7 @@ impl Index {
         &self,
         from: impl AsRef<str>,
         to: impl AsRef<str>,
-    ) -> Result<Vec<CrateVersion>, GitError> {
+    ) -> Result<Vec<Change>, GitError> {
         self.changes_from_objects(
             &self.repo.revparse_single(from.as_ref())?,
             &self.repo.revparse_single(to.as_ref())?,
@@ -266,7 +265,7 @@ impl Index {
         &self,
         from: &Object,
         to: &Object,
-    ) -> Result<Vec<CrateVersion>, GitError> {
+    ) -> Result<Vec<Change>, GitError> {
         fn into_tree<'a>(repo: &'a Repository, obj: &Object) -> Result<Tree<'a>, GitError> {
             repo.find_tree(match obj.kind() {
                 Some(ObjectType::Commit) => obj
@@ -285,24 +284,41 @@ impl Index {
             Some(&into_tree(&self.repo, to)?),
             None,
         )?;
-        let mut res: Vec<CrateVersion> = Vec::new();
-        diff.print(DiffFormat::Patch, |delta, _, diffline| {
-            if diffline.origin() != LINE_ADDED_INDICATOR {
-                return true;
-            }
+        let mut changes: Vec<Change> = Vec::new();
+        let mut deletes: Vec<String> = Vec::new();
+        diff.foreach(
+            &mut |delta, _| {
+                if delta.status() == Delta::Deleted {
+                    if let Some(path) = delta.new_file().path() {
+                        if let Some(file_name) = path.file_name() {
+                            deletes.push(file_name.to_string_lossy().to_string());
+                        }
+                    }
+                }
+                true
+            },
+            None,
+            None,
+            Some(&mut |delta, _hunk, diffline| {
+                if diffline.origin() != LINE_ADDED_INDICATOR {
+                    return true;
+                }
+                if !matches!(delta.status(), Delta::Added | Delta::Modified) {
+                    return true;
+                }
 
-            if !match delta.status() {
-                Delta::Added | Delta::Modified => true,
-                _ => false,
-            } {
-                return true;
-            }
+                if let Ok(crate_version) = serde_json::from_slice::<CrateVersion>(diffline.content()) {
+                    if crate_version.yanked {
+                        changes.push(Change::Yanked(crate_version));
+                    } else {
+                        changes.push(Change::Added(crate_version));
+                    }
+                }
+                true
+            }),
+        )?;
 
-            if let Ok(c) = serde_json::from_slice(diffline.content()) {
-                res.push(c)
-            }
-            true
-        })
-        .map(|_| res)
+        changes.extend(deletes.iter().map(|krate| Change::Deleted(krate.clone())));
+        Ok(changes)
     }
 }

src/version.rs

@@ -1,67 +1,27 @@
-use serde::de::{Deserialize, Deserializer};
-use serde::ser::{Serialize, Serializer};
 use std::collections::HashMap;
 
 use std::fmt;
 
 /// Identify a kind of change that occurred to a crate
-#[derive(Clone, Copy, Ord, PartialOrd, Eq, PartialEq, Debug)]
-pub enum ChangeKind {
-    /// A crate version was added
-    Added,
+#[derive(Clone, Eq, PartialEq, Debug)]
+pub enum Change {
     /// A crate version was added or it was unyanked.
-    Yanked,
+    Added(CrateVersion),
+    /// A crate version was yanked.
+    Yanked(CrateVersion),
+    /// A crate was deleted
+    Deleted(String),
 }
 
-impl Default for ChangeKind {
-    fn default() -> Self {
-        ChangeKind::Added
-    }
-}
-
-impl<'de> Deserialize<'de> for ChangeKind {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: Deserializer<'de>,
-    {
-        struct Visitor;
-        impl<'de> ::serde::de::Visitor<'de> for Visitor {
-            type Value = ChangeKind;
-            fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
-                formatter.write_str("boolean")
-            }
-            fn visit_bool<E>(self, value: bool) -> Result<ChangeKind, E>
-            where
-                E: ::serde::de::Error,
-            {
-                if value {
-                    Ok(ChangeKind::Yanked)
-                } else {
-                    Ok(ChangeKind::Added)
-                }
-            }
-        }
-        deserializer.deserialize_bool(Visitor)
-    }
-}
-
-impl Serialize for ChangeKind {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        serializer.serialize_bool(self == &ChangeKind::Yanked)
-    }
-}
-
-impl fmt::Display for ChangeKind {
+impl fmt::Display for Change {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         write!(
             f,
             "{}",
             match *self {
-                ChangeKind::Added => "added",
-                ChangeKind::Yanked => "yanked",
+                Change::Added(_) => "added",
+                Change::Yanked(_) => "yanked",
+                Change::Deleted(_) => "deleted",
             }
         )
     }
@@ -72,9 +32,8 @@ impl fmt::Display for ChangeKind {
 pub struct CrateVersion {
     /// The crate name, i.e. `clap`.
     pub name: String,
-    /// The kind of change.
-    #[serde(rename = "yanked")]
-    pub kind: ChangeKind,
+    /// is the release yanked?
+    pub yanked: bool,
     /// The semantic version of the crate.
     #[serde(rename = "vers")]
     pub version: String,

tests/index.rs

@@ -8,6 +8,7 @@ const NUM_VERSIONS_AT_RECENT_COMMIT: usize = 39752;
 const REV_ONE_ADDED: &'static str = "615c9c41942a3ba13e088fbcb1470c61b169a187";
 const REV_ONE_YANKED: &'static str = "8cf8fbad7876586ced34c4b778f6a80fadd2a59b";
 const REV_ONE_UNYANKED: &'static str = "f8cb00181";
+const REV_CRATE_DELETE: &str = "de5be3e8bb6cd7a3179857bdbdf28ca4fa23f84c";
 
 #[test]
 #[ignore] // This test takes too long for my taste, this library is stable by now
@@ -106,12 +107,20 @@ fn peek_changes_since_last_fetch() {
     );
 }
 
-fn changes_of(index: &Index, commit: &str) -> Vec<CrateVersion> {
+fn changes_of(index: &Index, commit: &str) -> Vec<Change> {
     index
-        .changes(format!("{}~1^{{tree}}", commit), format!("{}", commit))
+        .changes(format!("{}~1^{{tree}}", commit), commit)
         .expect("id to be valid and diff OK")
 }
 
+#[test]
+fn crate_delete() {
+    let (index, _tmp) = make_index();
+
+    let changes = changes_of(&index, REV_CRATE_DELETE);
+    assert_eq!(changes, vec![Change::Deleted("rustdecimal".to_string())],);
+}
+
 #[test]
 #[ignore]
 fn quick_traverse_unyanked_crates() {
@@ -121,9 +130,9 @@ fn quick_traverse_unyanked_crates() {
     let crates = changes_of(&index, REV_ONE_UNYANKED);
     assert_eq!(
         crates,
-        vec![CrateVersion {
+        vec![Change::Added(CrateVersion {
             name: "gfx_text".to_owned(),
-            kind: ChangeKind::Added,
+            yanked: false,
             version: "0.13.2".to_owned(),
             dependencies: vec![
                 Dependency {
@@ -174,7 +183,7 @@ fn quick_traverse_unyanked_crates() {
                 h
             },
             checksum: "d0b1240e3627e646f69685ddd3e7d83dd3ff3d586afe83bf3679082028183f2d".into(),
-        }]
+        })]
     );
 }
 
@@ -186,14 +195,14 @@ fn quick_traverse_yanked_crates() {
     let crates = changes_of(&index, REV_ONE_YANKED);
     assert_eq!(
         crates,
-        vec![CrateVersion {
+        vec![Change::Yanked(CrateVersion {
             name: "sha3".to_owned(),
-            kind: ChangeKind::Yanked,
+            yanked: true,
             version: "0.0.0".to_owned(),
             dependencies: Vec::new(),
             features: HashMap::new(),
             checksum: "dbba9d72d3d04e2167fb9c76ce22aed118eb003727bbe59774b9bf3603fa1f43".into(),
-        }]
+        })]
     );
 }
 
@@ -207,9 +216,9 @@ fn quick_traverse_added_crates() {
     let crates = changes_of(&index, REV_ONE_ADDED);
     assert_eq!(
         crates,
-        vec![CrateVersion {
+        vec![Change::Added(CrateVersion {
             name: "rpwg".to_owned(),
-            kind: ChangeKind::Added,
+            yanked: false,
             version: "0.1.0".to_owned(),
             dependencies: vec![
                 Dependency {
@@ -235,6 +244,6 @@ fn quick_traverse_added_crates() {
             ],
             features: HashMap::new(),
             checksum: "14437a3702699dba0c49ddc401a0529898e83f8b769348549985a0f4d818d3ca".into(),
-        }]
+        })]
     );
 }

tests/version.rs

@@ -18,7 +18,7 @@ fn test_parse_crate_version() {
         c,
         CrateVersion {
             name: "test".to_string(),
-            kind: ChangeKind::Yanked,
+            yanked: true,
             version: "1.0.0".to_string(),
             dependencies: Vec::new(),
             features: HashMap::new(),