Assert store tree cache matches actual source objects (#293)

Author: Byron

git-index/src/extension/tree.rs

@@ -17,6 +17,15 @@ pub mod verify {
     quick_error! {
         #[derive(Debug)]
         pub enum Error {
+            MissingTreeDirectory { parent_id: git_hash::ObjectId, entry_id: git_hash::ObjectId, name: BString } {
+                display("The entry {} at path '{}' in parent tree {} wasn't found in the nodes children, making it incomplete", entry_id, name, parent_id)
+            }
+            TreeNodeNotFound { oid: git_hash::ObjectId } {
+                display("The tree with id {} wasn't found in the object database", oid)
+            }
+            TreeNodeChildcountMismatch { oid: git_hash::ObjectId, expected_childcount: usize, actual_childcount: usize } {
+                display("The tree with id {} should have {} children, but its cached representation had {} of them", oid, expected_childcount, actual_childcount)
+            }
             RootWithName { name: BString } {
                 display("The root tree was named '{}', even though it should be empty", name)
             }
@@ -31,23 +40,25 @@ pub mod verify {
 }
 
 impl Tree {
-    pub fn verify(&self) -> Result<(), verify::Error> {
-        fn verify_recursive(parent_id: git_hash::ObjectId, children: &[Tree]) -> Result<Option<u32>, verify::Error> {
+    pub fn verify<F>(&self, use_find: bool, mut find: F) -> Result<(), verify::Error>
+    where
+        F: for<'a> FnMut(&git_hash::oid, &'a mut Vec<u8>) -> Option<git_object::TreeRefIter<'a>>,
+    {
+        fn verify_recursive<F>(
+            parent_id: git_hash::ObjectId,
+            children: &[Tree],
+            mut find_buf: Option<&mut Vec<u8>>,
+            find: &mut F,
+        ) -> Result<Option<u32>, verify::Error>
+        where
+            F: for<'a> FnMut(&git_hash::oid, &'a mut Vec<u8>) -> Option<git_object::TreeRefIter<'a>>,
+        {
             if children.is_empty() {
                 return Ok(None);
             }
             let mut entries = 0;
             let mut prev = None::<&Tree>;
             for child in children {
-                let actual_num_entries = verify_recursive(child.id, &child.children)?;
-                if let Some(actual) = actual_num_entries {
-                    if actual > child.num_entries {
-                        return Err(verify::Error::EntriesCount {
-                            actual,
-                            expected: child.num_entries,
-                        });
-                    }
-                }
                 entries += child.num_entries;
                 if let Some(prev) = prev {
                     if prev.name.cmp(&child.name) != Ordering::Less {
@@ -60,6 +71,43 @@ impl Tree {
                 }
                 prev = Some(child);
             }
+            if let Some(buf) = find_buf.as_mut() {
+                let tree_entries =
+                    find(&parent_id, *buf).ok_or_else(|| verify::Error::TreeNodeNotFound { oid: parent_id })?;
+                let mut num_entries = 0;
+                for entry in tree_entries
+                    .filter_map(Result::ok)
+                    .filter(|e| e.mode == git_object::tree::EntryMode::Tree)
+                {
+                    children
+                        .binary_search_by(|e| e.name.as_bstr().cmp(&entry.filename))
+                        .map_err(|_| verify::Error::MissingTreeDirectory {
+                            parent_id,
+                            entry_id: entry.oid.to_owned(),
+                            name: entry.filename.to_owned(),
+                        })?;
+                    num_entries += 1;
+                }
+
+                if num_entries != children.len() {
+                    return Err(verify::Error::TreeNodeChildcountMismatch {
+                        oid: parent_id,
+                        expected_childcount: num_entries,
+                        actual_childcount: children.len(),
+                    });
+                }
+            }
+            for child in children {
+                let actual_num_entries = verify_recursive(child.id, &child.children, find_buf.as_deref_mut(), find)?;
+                if let Some(actual) = actual_num_entries {
+                    if actual > child.num_entries {
+                        return Err(verify::Error::EntriesCount {
+                            actual,
+                            expected: child.num_entries,
+                        });
+                    }
+                }
+            }
             Ok(entries.into())
         }
 
@@ -69,7 +117,8 @@ impl Tree {
             });
         }
 
-        let declared_entries = verify_recursive(self.id, &self.children)?;
+        let mut buf = Vec::new();
+        let declared_entries = verify_recursive(self.id, &self.children, use_find.then(|| &mut buf), &mut find)?;
         if let Some(actual) = declared_entries {
             if actual > self.num_entries {
                 return Err(verify::Error::EntriesCount {

git-index/src/verify.rs

@@ -19,7 +19,7 @@ pub mod extensions {
     use crate::extension;
     use quick_error::quick_error;
 
-    pub fn no_find<'a>(_: &git_hash::oid, _: &'a mut Vec<u8>) -> Option<git_object::TreeRef<'a>> {
+    pub fn no_find<'a>(_: &git_hash::oid, _: &'a mut Vec<u8>) -> Option<git_object::TreeRefIter<'a>> {
         None
     }
 
@@ -56,11 +56,11 @@ impl State {
     }
 
     /// Note: `find` cannot be `Option<F>` as we can't call it with a closure then due to the indirection through `Some`.
-    pub fn verify_extensions<F>(&self, _use_find: bool, _find: F) -> Result<(), extensions::Error>
+    pub fn verify_extensions<F>(&self, use_find: bool, find: F) -> Result<(), extensions::Error>
     where
-        F: for<'a> FnMut(&git_hash::oid, &'a mut Vec<u8>) -> Option<git_object::TreeRef<'a>>,
+        F: for<'a> FnMut(&git_hash::oid, &'a mut Vec<u8>) -> Option<git_object::TreeRefIter<'a>>,
     {
-        self.tree().map(|t| t.verify()).transpose()?;
+        self.tree().map(|t| t.verify(use_find, find)).transpose()?;
         // TODO: verify links by running the whole set of tests on the index
         //       - do that once we load it as well, or maybe that's lazy loaded? Too many questions for now.
         Ok(())

gitoxide-core/src/repository.rs

@@ -43,7 +43,7 @@ pub mod verify {
     ) -> anyhow::Result<()> {
         let repo = git_repository::open(repo)?;
         #[cfg_attr(not(feature = "serde1"), allow(unused))]
-        let outcome = repo.objects.verify_integrity(
+        let mut outcome = repo.objects.verify_integrity(
             progress,
             should_interrupt,
             git_repository::odb::pack::index::verify::integrity::Options {
@@ -61,8 +61,9 @@ pub mod verify {
             index.verify_extensions(true, {
                 use git::odb::FindExt;
                 let handle = repo.objects.to_handle();
-                move |oid, buf: &mut Vec<u8>| handle.find_tree(oid, buf).ok()
+                move |oid, buf: &mut Vec<u8>| handle.find_tree_iter(oid, buf).ok()
             })?;
+            outcome.progress.info(format!("Index at '{}' OK", index.path.display()));
         }
         match output_statistics {
             Some(OutputFormat::Human) => writeln!(out, "Human output is currently unsupported, use JSON instead")?,