radicle/storage/refs: Strengthen Encapsulation

`radicle::storage::Refs` carries a map from reference names to OIDs, and
there are some invariants.

crates/radicle-protocol/src/wire.rs

@@ -5,7 +5,6 @@ pub mod varint;
 pub use frame::StreamId;
 pub use message::{AddressType, MessageType};
 
-use std::collections::BTreeMap;
 use std::convert::TryFrom;
 use std::fmt::Debug;
 use std::mem;
@@ -318,7 +317,7 @@ impl Decode for PublicKey {
 impl Decode for Refs {
     fn decode(buf: &mut impl Buf) -> Result<Self, Error> {
         let len = Size::decode(buf)?;
-        let mut refs = BTreeMap::new();
+        let mut refs = Refs::new();
 
         for _ in 0..len {
             let name = String::decode(buf)?;
@@ -327,7 +326,7 @@ impl Decode for Refs {
 
             refs.insert(name, oid);
         }
-        Ok(refs.into())
+        Ok(refs)
     }
 }
 

crates/radicle/src/storage/git.rs

@@ -772,7 +772,8 @@ impl ReadRepository for Repository {
         let entries = self
             .backend
             .references_glob(format!("refs/namespaces/{remote}/*").as_str())?;
-        let mut refs = BTreeMap::new();
+
+        let mut refs = Refs::new();
 
         for e in entries {
             let e = e?;
@@ -788,7 +789,7 @@ impl ReadRepository for Repository {
                 }
             }
         }
-        Ok(refs.into())
+        Ok(refs)
     }
 
     fn references_glob(
@@ -992,10 +993,7 @@ impl SignRepository for Repository {
             self.set_remote_identity_root(remote)?;
         }
         let mut refs = self.references_of(remote)?;
-        // Don't sign the `rad/sigrefs` ref itself, and don't sign invalid OIDs.
+        refs.remove_sigrefs();
-        refs.retain(|name, oid| {
-            name.as_refstr() != refs::SIGREFS_BRANCH.as_ref() && !oid.is_zero()
-        });
         let signed = refs.signed(signer)?.verified(self)?;
         signed.save(self)?;
 
@@ -1062,7 +1060,7 @@ mod tests {
 
     use super::*;
     use crate::git;
-    use crate::storage::refs::SIGREFS_BRANCH;
+
     use crate::storage::{ReadRepository, ReadStorage};
     use crate::test::fixtures;
 
@@ -1129,8 +1127,7 @@ mod tests {
         let mut unsigned = stored.references_of(&alice).unwrap();
 
         // The signed refs doesn't contain the signature ref itself.
-        let sigref = (*SIGREFS_BRANCH).to_ref_string();
+        unsigned.remove_sigrefs().unwrap();
-        unsigned.remove(&sigref).unwrap();
 
         assert_eq!(remote.refs, signed);
         assert_eq!(*remote.refs, unsigned);

crates/radicle/src/storage/refs.rs

@@ -6,7 +6,7 @@ use std::fmt::Debug;
 use std::io;
 use std::io::{BufRead, BufReader};
 use std::marker::PhantomData;
-use std::ops::{Deref, DerefMut};
+use std::ops::Deref;
 use std::path::Path;
 use std::str::FromStr;
 
@@ -79,6 +79,10 @@ impl Error {
 pub struct Refs(BTreeMap<git::fmt::RefString, Oid>);
 
 impl Refs {
+    pub fn new() -> Self {
+        Self(BTreeMap::new())
+    }
+
     /// Verify the given signature on these refs, and return [`SignedRefs`] on success.
     pub fn verified<R: ReadRepository>(
         self,
@@ -126,9 +130,10 @@ impl Refs {
             let name = git::fmt::RefString::try_from(name)?;
             let oid = Oid::from_str(oid).map_err(|_| canonical::Error::InvalidFormat)?;
 
-            if oid.is_zero() {
+            if oid.is_zero() || name.as_refstr() == SIGREFS_BRANCH.as_ref() {
                 continue;
             }
+
             refs.insert(name, oid);
         }
         Ok(Self(refs))
@@ -137,14 +142,55 @@ impl Refs {
     fn canonical(&self) -> Vec<u8> {
         let mut buf = String::new();
 
-        for (name, oid) in self.iter() {
+        for (name, oid) in self.0.iter() {
+            debug_assert_ne!(oid, &Oid::sha1_zero());
+            debug_assert_ne!(name, &SIGREFS_BRANCH.to_ref_string());
+
             buf.push_str(&oid.to_string());
             buf.push(' ');
             buf.push_str(name);
             buf.push('\n');
         }
+
         buf.into_bytes()
     }
+
+    pub fn insert(&mut self, refname: git::fmt::RefString, target: Oid) -> Option<Oid> {
+        if target.is_zero() {
+            self.0.remove(&refname)
+        } else {
+            self.0.insert(refname, target)
+        }
+    }
+
+    pub(crate) fn keys<'a>(
+        &'a self,
+    ) -> std::collections::btree_map::Keys<'a, git::fmt::RefString, Oid> {
+        self.0.keys()
+    }
+
+    #[cfg(any(test, feature = "test"))]
+    pub(crate) fn values<'a>(
+        &'a self,
+    ) -> std::collections::btree_map::Values<'a, git::fmt::RefString, Oid> {
+        self.0.values()
+    }
+
+    pub fn iter<'a>(&'a self) -> std::collections::btree_map::Iter<'a, git::fmt::RefString, Oid> {
+        self.0.iter()
+    }
+
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+
+    pub(super) fn remove_sigrefs(&mut self) -> Option<Oid> {
+        self.0.remove(&SIGREFS_BRANCH.to_ref_string())
+    }
 }
 
 impl IntoIterator for Refs {
@@ -168,23 +214,16 @@ impl<V> From<SignedRefs<V>> for Refs {
     }
 }
 
-impl From<BTreeMap<git::fmt::RefString, Oid>> for Refs {
+impl<I> From<I> for Refs
-    fn from(refs: BTreeMap<git::fmt::RefString, Oid>) -> Self {
+where
-        Self(refs)
+    I: Iterator<Item = (git::fmt::RefString, Oid)>,
-    }
+{
-}
+    fn from(value: I) -> Self {
-
+        let mut refs = Self::new();
-impl Deref for Refs {
+        for (refname, target) in value {
-    type Target = BTreeMap<git::fmt::RefString, Oid>;
+            refs.insert(refname, target);
-
+        }
-    fn deref(&self) -> &Self::Target {
+        refs
-        &self.0
-    }
-}
-
-impl DerefMut for Refs {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        &mut self.0
     }
 }
 

crates/radicle/src/storage/refs/arbitrary.rs

@@ -27,7 +27,6 @@ where
 
 impl Arbitrary for Refs {
     fn arbitrary(g: &mut qcheck::Gen) -> Self {
-        let mut refs: BTreeMap<git::fmt::RefString, storage::Oid> = BTreeMap::new();
         let mut bytes: [u8; 20] = [0; 20];
         let names = &[
             "heads/master",
@@ -40,6 +39,7 @@ impl Arbitrary for Refs {
             "notes/1",
         ];
 
+        let mut refs = Self::new();
         for _ in 0..g.size().min(names.len()) {
             if let Some(name) = g.choose(names) {
                 for byte in &mut bytes {
@@ -51,7 +51,7 @@ impl Arbitrary for Refs {
                 refs.insert(name, oid);
             }
         }
-        Self::from(refs)
+        refs
     }
 }