fix!: skip the null-hash when validating the index.

This is needed for compatibility with `index.skipHash`, which may skip
producing the hash at the end of the index file, just filling in the
null-hash.

Author: Byron

gix-index/src/decode/mod.rs

@@ -54,7 +54,7 @@ pub struct Options {
 
 impl State {
     /// Decode an index state from `data` and store `timestamp` in the resulting instance for pass-through, assuming `object_hash`
-    /// to be used through the file.
+    /// to be used through the file. Also return the stored hash over all bytes in `data` or `None` if none was written due to `index.skipHash`.
     pub fn from_bytes(
         data: &[u8],
         timestamp: FileTime,
@@ -64,7 +64,7 @@ impl State {
             min_extension_block_in_bytes_for_threading,
             expected_checksum,
         }: Options,
-    ) -> Result<(Self, gix_hash::ObjectId), Error> {
+    ) -> Result<(Self, Option<gix_hash::ObjectId>), Error> {
         let _span = gix_features::trace::detail!("gix_index::State::from_bytes()");
         let (version, num_entries, post_header_data) = header::decode(data, object_hash)?;
         let start_of_extensions = extension::end_of_index_entry::decode(data, object_hash);
@@ -214,10 +214,11 @@ impl State {
         }
 
         let checksum = gix_hash::ObjectId::from(data);
-        if let Some(expected_checksum) = expected_checksum {
-            if checksum != expected_checksum {
+        let checksum = (!checksum.is_null()).then_some(checksum);
+        if let Some((expected_checksum, actual_checksum)) = expected_checksum.zip(checksum) {
+            if actual_checksum != expected_checksum {
                 return Err(Error::ChecksumMismatch {
-                    actual_checksum: checksum,
+                    actual_checksum,
                     expected_checksum,
                 });
             }

gix-index/src/file/init.rs

@@ -45,6 +45,9 @@ impl File {
     }
 
     /// Open an index file at `path` with `options`, assuming `object_hash` is used throughout the file.
+    ///
+    /// Note that the verification of the file hash depends on `options`, and even then it's performed after the file was read and not
+    /// before it is read. That way, invalid files would see a more descriptive error message as we try to parse them.
     pub fn at(path: impl Into<PathBuf>, object_hash: gix_hash::Kind, options: decode::Options) -> Result<Self, Error> {
         let _span = gix_features::trace::detail!("gix_index::File::at()");
         let path = path.into();
@@ -57,11 +60,7 @@ impl File {
         };
 
         let (state, checksum) = State::from_bytes(&data, mtime, object_hash, options)?;
-        let mut file = File {
-            state,
-            path,
-            checksum: Some(checksum),
-        };
+        let mut file = File { state, path, checksum };
         if let Some(mut link) = file.link.take() {
             link.dissolve_into(&mut file, object_hash, options)?;
         }
@@ -72,7 +71,7 @@ impl File {
     /// Consume `state` and pretend it was read from `path`, setting our checksum to `null`.
     ///
     /// `File` instances created like that should be written to disk to set the correct checksum via `[File::write()]`.
-    pub fn from_state(state: crate::State, path: impl Into<PathBuf>) -> Self {
+    pub fn from_state(state: State, path: impl Into<PathBuf>) -> Self {
         File {
             state,
             path: path.into(),

gix-index/src/file/verify.rs

@@ -14,8 +14,6 @@ mod error {
             actual: gix_hash::ObjectId,
             expected: gix_hash::ObjectId,
         },
-        #[error("Checksum of in-memory index wasn't computed yet")]
-        NoChecksum,
     }
 }
 pub use error::Error;
@@ -24,19 +22,22 @@ impl File {
     /// Verify the integrity of the index to assure its consistency.
     pub fn verify_integrity(&self) -> Result<(), Error> {
         let _span = gix_features::trace::coarse!("gix_index::File::verify_integrity()");
-        let checksum = self.checksum.ok_or(Error::NoChecksum)?;
-        let num_bytes_to_hash = self.path.metadata()?.len() - checksum.as_bytes().len() as u64;
-        let should_interrupt = AtomicBool::new(false);
-        let actual = gix_features::hash::bytes_of_file(
-            &self.path,
-            num_bytes_to_hash as usize,
-            checksum.kind(),
-            &mut gix_features::progress::Discard,
-            &should_interrupt,
-        )?;
-        (actual == checksum).then_some(()).ok_or(Error::ChecksumMismatch {
-            actual,
-            expected: checksum,
-        })
+        if let Some(checksum) = self.checksum {
+            let num_bytes_to_hash = self.path.metadata()?.len() - checksum.as_bytes().len() as u64;
+            let should_interrupt = AtomicBool::new(false);
+            let actual = gix_features::hash::bytes_of_file(
+                &self.path,
+                num_bytes_to_hash as usize,
+                checksum.kind(),
+                &mut gix_features::progress::Discard,
+                &should_interrupt,
+            )?;
+            (actual == checksum).then_some(()).ok_or(Error::ChecksumMismatch {
+                actual,
+                expected: checksum,
+            })
+        } else {
+            Ok(())
+        }
     }
 }

gix-index/tests/fixtures/loose_index/skip_hash.git-index

@@ -75,6 +75,28 @@ fn v2_empty() {
     assert!(tree.name.is_empty());
     assert!(tree.children.is_empty());
     assert_eq!(tree.id, hex_to_id("4b825dc642cb6eb9a060e54bf8d69288fbee4904"));
+    assert_eq!(
+        file.checksum(),
+        Some(hex_to_id("72d53f787d86a932a25a8537cee236d81846a8f1")),
+        "checksums are read but not validated by default"
+    );
+}
+
+#[test]
+fn v2_empty_skip_hash() {
+    let file = loose_file("skip_hash");
+    assert_eq!(file.version(), Version::V2);
+    assert_eq!(file.entries().len(), 0);
+    let tree = file.tree().unwrap();
+    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"));
+    assert_eq!(
+        file.checksum(),
+        None,
+        "unset checksums are represented in the type system"
+    );
 }
 
 #[test]