Prefix now validates all constraints and errors on violation (#298)

That way, min and max lengths is assumed to be correct.

Sebastian Thiel committed 4 years ago

Commit 75efa791 parent 837db626 files · +63 −18Browse files

Unified · Split

6 files

git-hash/src/lib.rs +1 −1
git-hash/src/owned.rs +29 −6
git-hash/tests/oid/mod.rs +16 −7
git-pack/src/index/mod.rs +4 −0
git-pack/src/multi_index/access.rs +2 −1
git-pack/tests/pack/multi_index/mod.rs +11 −3

@@ -11,7 +11,7 @@ use std::{convert::TryFrom, str::FromStr}; · 10 unmodified lines
11	pub use borrowed::oid;	11	pub use borrowed::oid;
12		12
13	mod owned;	13	mod owned;
14	pub use owned::{ObjectId, Prefix};	14	pub use owned::{prefix, ObjectId, Prefix};
15		15
16	#[allow(missing_docs)]	16	#[allow(missing_docs)]
17	pub mod decode {	17	pub mod decode {

git-hash/src/owned.rs+29−6

     hex_len: usize,
 }
+///
+pub mod prefix {
+    use quick_error::quick_error;
+    quick_error! {
+        /// TODO:
+        #[derive(Debug)]
+        #[allow(missing_docs)]
+        pub enum Error {
+            TooShort { hex_len: usize } {
+                display("The minimum hex length of a short object id is 4, got {}", hex_len)
+            }
+            TooLong { object_kind: crate::Kind, hex_len: usize } {
+                display("An object of kind {} cannot be larger than {} in hex, but {} was requested", object_kind, object_kind.len_in_hex(), hex_len)
+            }
+        }
+    }
+}
 impl Prefix {
     /// Create a new instance by taking a full `id` as input and truncating it to `hex_len`.
     ///
     /// For instance, with `hex_len` of 7 the resulting prefix is 3.5 bytes, or 3 bytes and 4 bits
     /// wide, with all other bytes and bits set to zero.
-    pub fn from_id(id: impl AsRef<oid>, hex_len: usize) -> Self {
+    pub fn try_from_id(id: impl AsRef<oid>, hex_len: usize) -> Result<Self, prefix::Error> {
         let id = id.as_ref();
-        assert!(
+        if hex_len > id.kind().len_in_hex() {
-            hex_len <= id.kind().len_in_hex(),
+            return Err(prefix::Error::TooLong {
-            "hex_len must not be larger than the maximum hex len of the input id"
+                object_kind: id.kind(),
-        );
+                hex_len,
+            });
+        } else if hex_len < 4 {
+            return Err(prefix::Error::TooShort { hex_len });
+        }
         let mut prefix = ObjectId::null(id.kind());
         let b = prefix.as_mut_slice();
             b[hex_len / 2] &= 0xf0;
         }
-        Prefix { bytes: prefix, hex_len }
+        Ok(Prefix { bytes: prefix, hex_len })
     }
     /// Returns the prefix as object id.

git-hash/tests/oid/mod.rs+16−7

             let oid_hex = "abcdefabcdefabcdefabcdefabcdefabcdefabcd";
             let oid = hex_to_id(oid_hex);
-            assert_eq!(git_hash::Prefix::from_id(oid, 0).as_oid(), ObjectId::null(oid.kind()));
+            for hex_len in 4..oid.kind().len_in_hex() {
-            for hex_len in 1..oid.kind().len_in_hex() {
                 let mut expected = String::from(&oid_hex[..hex_len]);
                 let num_of_zeros = oid.kind().len_in_hex() - hex_len;
                 expected.extend(std::iter::repeat('0').take(num_of_zeros));
-                let prefix = git_hash::Prefix::from_id(oid, hex_len);
+                let prefix = git_hash::Prefix::try_from_id(oid, hex_len).unwrap();
                 assert_eq!(prefix.as_oid().to_hex().to_string(), expected, "{}", hex_len);
                 assert_eq!(prefix.hex_len(), hex_len);
             }
         }
         #[test]
-        #[should_panic]
+        fn errors_if_hex_len_is_longer_than_oid_len_in_hex() {
-        fn panics_if_hex_len_is_longer_than_oid_len_in_hex() {
+            let kind = Kind::Sha1;
+            assert!(matches!(
+                git_hash::Prefix::try_from_id(ObjectId::null(kind), kind.len_in_hex() + 1),
+                Err(git_hash::prefix::Error::TooLong { .. })
+            ));
+        }
+        #[test]
+        fn errors_if_hex_len_is_too_short() {
             let kind = Kind::Sha1;
-            git_hash::Prefix::from_id(ObjectId::null(kind), kind.len_in_hex() + 1);
+            assert!(matches!(
+                git_hash::Prefix::try_from_id(ObjectId::null(kind), 3),
+                Err(git_hash::prefix::Error::TooShort { .. })
+            ));
         }
     }
 }

git-pack/src/index/mod.rs+4−0

@@ -99,6 +99,10 @@ impl Version { · 98 unmodified lines
99	}	99	}
100	}	100	}
101		101
		102	/// A way to indicate if a lookup, despite successful, was ambiguous or yielded exactly
		103	/// one result in the particular index.
		104	pub type PrefixLookupResult = Result<EntryIndex, ()>;
		105
102	/// The type for referring to indices of an entry within the index file.	106	/// The type for referring to indices of an entry within the index file.
103	pub type EntryIndex = u32;	107	pub type EntryIndex = u32;
104		108

git-pack/src/multi_index/access.rs+2−1

 use std::path::{Path, PathBuf};
+use crate::index::PrefixLookupResult;
 use crate::{
     data,
     multi_index::{EntryIndex, File, PackIndex, Version},
     }
     /// TODO
-    pub fn lookup_prefix(&self, _id: impl AsRef<git_hash::oid>, _hex_len: usize) -> Option<EntryIndex> {
+    pub fn lookup_prefix(&self, prefix: git_hash::Prefix) -> Option<PrefixLookupResult> {
         todo!()
     }

git-pack/tests/pack/multi_index/mod.rs+11−3

     use std::path::PathBuf;
     #[test]
-    fn lookup_abbrev() {
+    #[ignore]
+    fn lookup_with_ambiguity() {}
+    #[test]
+    fn lookup_prefix() {
         let (file, _path) = multi_index();
         for (idx, entry) in file.iter().enumerate() {
             let hex_len = (idx % file.object_hash().len_in_hex()).min(7);
             let hex_oid = entry.oid.to_hex_with_len(hex_len).to_string();
             assert_eq!(hex_oid.len(), hex_len);
-            let _oid_prefix = git_hash::Prefix::from_id(&entry.oid, hex_len);
+            let oid_prefix = git_hash::Prefix::try_from_id(&entry.oid, hex_len);
-            // file.lookup_prefix(oid_prefix, hex_len).expect("non-ambiguous")
+            let entry_index = file
+                .lookup_prefix(oid_prefix)
+                .expect("object found")
+                .expect("non-ambiguous");
+            assert_eq!(file.oid_at_index(entry_index), entry.oid);
         }
     }

Prefix now validates all constraints and errors on violation (#298)

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code