fix tree ext reading and writing; round-trip with long path works now

Author: Byron

Cargo.lock

@@ -45,6 +45,7 @@ bstr = { version = "0.2.13", default-features = false }
 serde = { version = "1.0.114", optional = true, default-features = false, features = ["derive"] }
 smallvec = "1.7.0"
 atoi = "1.0.0"
+itoa = "1.0.3"
 bitflags = "1.3.2"
 
 document-features = { version = "0.2.0", optional = true }

git-index/Cargo.toml

@@ -26,7 +26,8 @@ pub struct Tree {
     pub id: git_hash::ObjectId,
     /// The amount of non-tree items in this directory tree, including sub-trees, recursively.
     /// The value of the top-level tree is thus equal to the value of the total amount of entries.
-    pub num_entries: u32,
+    /// If `None`, the tree is considered invalid and needs to be refreshed
+    pub num_entries: Option<u32>,
     /// The child-trees below the current tree.
     pub children: Vec<Tree>,
 }

git-index/src/extension/mod.rs

@@ -1,6 +1,7 @@
 use crate::extension::Tree;
 use crate::util::{split_at_byte_exclusive, split_at_pos};
 use git_hash::ObjectId;
+use std::convert::TryInto;
 
 /// A recursive data structure
 pub fn decode(data: &[u8], object_hash: git_hash::Kind) -> Option<Tree> {
@@ -17,13 +18,20 @@ fn one_recursive(data: &[u8], hash_len: usize) -> Option<(Tree, &[u8])> {
     let (path, data) = split_at_byte_exclusive(data, 0)?;
 
     let (entry_count, data) = split_at_byte_exclusive(data, b' ')?;
-    let num_entries: u32 = atoi::atoi(entry_count)?;
+    let num_entries: i32 = atoi::atoi(entry_count)?;
 
     let (subtree_count, data) = split_at_byte_exclusive(data, b'\n')?;
     let subtree_count: usize = atoi::atoi(subtree_count)?;
 
-    let (hash, mut data) = split_at_pos(data, hash_len)?;
-    let id = ObjectId::from(hash);
+    let (id, mut data) = if num_entries >= 0 {
+        let (hash, data) = split_at_pos(data, hash_len)?;
+        (ObjectId::from(hash), data)
+    } else {
+        (
+            ObjectId::null(git_hash::Kind::from_hex_len(hash_len * 2).expect("valid hex_len")),
+            data,
+        )
+    };
 
     let mut subtrees = Vec::with_capacity(subtree_count);
     for _ in 0..subtree_count {
@@ -42,7 +50,7 @@ fn one_recursive(data: &[u8], hash_len: usize) -> Option<(Tree, &[u8])> {
     Some((
         Tree {
             id,
-            num_entries,
+            num_entries: num_entries.try_into().ok(),
             name: path.into(),
             children: subtrees,
         },

git-index/src/extension/tree/decode.rs

@@ -16,6 +16,6 @@ mod tests {
 
     #[test]
     fn size_of_tree() {
-        assert_eq!(std::mem::size_of::<crate::extension::Tree>(), 80);
+        assert_eq!(std::mem::size_of::<crate::extension::Tree>(), 88);
     }
 }

git-index/src/extension/tree/mod.rs

@@ -57,7 +57,7 @@ impl Tree {
             let mut entries = 0;
             let mut prev = None::<&Tree>;
             for child in children {
-                entries += child.num_entries;
+                entries += child.num_entries.unwrap_or(0);
                 if let Some(prev) = prev {
                     if prev.name.cmp(&child.name) != Ordering::Less {
                         return Err(Error::OutOfOrder {
@@ -98,11 +98,11 @@ impl Tree {
                 // This is actually needed here as it's a mut ref, which isn't copy. We do a re-borrow here.
                 #[allow(clippy::needless_option_as_deref)]
                 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 {
+                if let Some((actual, num_entries)) = actual_num_entries.zip(child.num_entries) {
+                    if actual > num_entries {
                         return Err(Error::EntriesCount {
                             actual,
-                            expected: child.num_entries,
+                            expected: num_entries,
                         });
                     }
                 }
@@ -118,11 +118,11 @@ impl Tree {
 
         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 {
+        if let Some((actual, num_entries)) = declared_entries.zip(self.num_entries) {
+            if actual > num_entries {
                 return Err(Error::EntriesCount {
                     actual,
-                    expected: self.num_entries,
+                    expected: num_entries,
                 });
             }
         }

git-index/src/extension/tree/verify.rs

@@ -5,16 +5,22 @@ impl Tree {
     /// Serialize this instance to `out`.
     pub fn write_to(&self, mut out: impl std::io::Write) -> Result<(), std::io::Error> {
         fn tree_entry(out: &mut impl std::io::Write, tree: &Tree) -> Result<(), std::io::Error> {
-            let num_entries_ascii = tree.num_entries.to_string();
-            let num_children_ascii = tree.children.len().to_string();
+            let mut buf = itoa::Buffer::new();
+            let num_entries = match tree.num_entries {
+                Some(num_entries) => buf.format(num_entries),
+                None => buf.format(-1),
+            };
 
             out.write_all(tree.name.as_slice())?;
             out.write_all(b"\0")?;
-            out.write_all(num_entries_ascii.as_bytes())?;
+            out.write_all(num_entries.as_bytes())?;
             out.write_all(b" ")?;
-            out.write_all(num_children_ascii.as_bytes())?;
+            let num_children = buf.format(tree.children.len());
+            out.write_all(num_children.as_bytes())?;
             out.write_all(b"\n")?;
-            out.write_all(tree.id.as_bytes())?;
+            if tree.num_entries.is_some() {
+                out.write_all(tree.id.as_bytes())?;
+            }
 
             for child in &tree.children {
                 tree_entry(out, child)?;
@@ -25,7 +31,7 @@ impl Tree {
 
         let signature = tree::SIGNATURE;
 
-        let estimated_size = self.num_entries * (300 + 3 + 1 + 3 + 1 + 20);
+        let estimated_size = self.num_entries.unwrap_or(0) * (300 + 3 + 1 + 3 + 1 + 20);
         let mut entries: Vec<u8> = Vec::with_capacity(estimated_size as usize);
         tree_entry(&mut entries, self)?;
 

git-index/src/extension/tree/write.rs

@@ -52,7 +52,7 @@ fn v2_with_single_entry_tree_and_eoie_ext() {
         assert_eq!(entry.path(&file.state), "a");
 
         let tree = file.tree().unwrap();
-        assert_eq!(tree.num_entries, 1);
+        assert_eq!(tree.num_entries.unwrap_or_default(), 1);
         assert_eq!(tree.id, hex_to_id("496d6428b9cf92981dc9495211e6e1120fb6f2ba"));
         assert!(tree.name.is_empty());
         assert!(tree.children.is_empty());
@@ -64,7 +64,7 @@ fn v2_empty() {
     assert_eq!(file.version(), Version::V2);
     assert_eq!(file.entries().len(), 0);
     let tree = file.tree().unwrap();
-    assert_eq!(tree.num_entries, 0);
+    assert_eq!(tree.num_entries.unwrap_or_default(), 0);
     assert!(tree.name.is_empty());
     assert!(tree.children.is_empty());
     assert_eq!(tree.id, hex_to_id("4b825dc642cb6eb9a060e54bf8d69288fbee4904"));
@@ -86,13 +86,13 @@ fn v2_with_multiple_entries_without_eoie_ext() {
 
     let tree = file.tree().unwrap();
     assert_eq!(tree.id, hex_to_id("c9b29c3168d8e677450cc650238b23d9390801fb"));
-    assert_eq!(tree.num_entries, 6);
+    assert_eq!(tree.num_entries.unwrap_or_default(), 6);
     assert!(tree.name.is_empty());
     assert_eq!(tree.children.len(), 1);
 
     let tree = &tree.children[0];
     assert_eq!(tree.id, hex_to_id("765b32c65d38f04c4f287abda055818ec0f26912"));
-    assert_eq!(tree.num_entries, 3);
+    assert_eq!(tree.num_entries.unwrap_or_default(), 3);
     assert_eq!(tree.name.as_bstr(), "d");
 }
 
@@ -143,6 +143,15 @@ fn v2_very_long_path() {
             .chain(std::iter::once('q'))
             .collect::<String>()
     );
+    assert!(
+        file.tree().is_some(),
+        "Tree has invalid entries, but that shouldn't prevent us from loading it"
+    );
+    let tree = file.tree().expect("present");
+    assert_eq!(tree.num_entries, None, "root tree has invalid entries actually");
+    assert_eq!(tree.name.as_bstr(), "");
+    assert_eq!(tree.num_entries, None, "it's marked invalid actually");
+    assert!(tree.id.is_null(), "there is no id for the root")
 }
 
 #[test]

git-index/tests/index/file/read.rs

@@ -12,9 +12,18 @@ fn roundtrips() -> crate::Result {
     }
     use Kind::*;
     let input = [
-        (Loose("very-long-path"), write::Options::default(), false), // unclear why the file is smaller when written back
-        (Generated("v2"), write::Options::default(), true),
-        (Generated("V2_empty"), write::Options::default(), true),
+        (
+            Loose("very-long-path"),
+            write::Options {
+                extensions: write::Extensions::Given {
+                    end_of_index_entry: false,
+                    tree_cache: true,
+                },
+                ..write::Options::default()
+            },
+        ), // tree extension can't be read
+        (Generated("v2"), write::Options::default()),
+        (Generated("V2_empty"), write::Options::default()),
         (
             Generated("v2_more_files"),
             write::Options {
@@ -24,11 +33,10 @@ fn roundtrips() -> crate::Result {
                 },
                 ..write::Options::default()
             },
-            true,
         ),
     ];
 
-    for (fixture, options, compare_byte_by_byte) in input {
+    for (fixture, options) in input {
         let (path, fixture) = match fixture {
             Generated(name) => (crate::fixture_index_path(name), name),
             Loose(name) => (loose_file_path(name), name),
@@ -41,9 +49,7 @@ fn roundtrips() -> crate::Result {
         let (actual, _) = State::from_bytes(&out_bytes, FileTime::now(), decode::Options::default())?;
 
         compare_states(&actual, &expected, options, fixture);
-        if compare_byte_by_byte {
-            compare_raw_bytes(&out_bytes, &expected_bytes, fixture);
-        }
+        compare_raw_bytes(&out_bytes, &expected_bytes, fixture);
     }
     Ok(())
 }
@@ -179,7 +185,7 @@ fn compare_raw_bytes(generated: &[u8], expected: &[u8], fixture: &str) {
             let expected = &expected[range_left..range_right];
 
             panic! {"\n\nRoundtrip failed for index in fixture {:?} at position {:?}\n\
-            \t   Input: ... {:?} ...\n\
+            \t  Actual: ... {:?} ...\n\
             \tExpected: ... {:?} ...\n\n\
             ", &fixture, index, generated, expected}
         }

git-index/tests/index/file/write.rs

@@ -14,7 +14,7 @@ mod serde_only {
             /// The id of the directory tree of the associated tree object.
             id: String,
             /// The amount of non-tree entries contained within, and definitely not zero.
-            num_entries: u32,
+            num_entries: Option<u32>,
             children: Vec<Tree>,
         }