From 0c47d06f2d63a7bbd9c4c401b8b4794a1528284f Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Tue, 24 Feb 2026 15:16:36 +0000 Subject: [PATCH] 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 +- crates/radicle/src/storage/git.rs | 15 ++-- crates/radicle/src/storage/refs.rs | 79 +++++++++++++++----- crates/radicle/src/storage/refs/arbitrary.rs | 4 +- 4 files changed, 69 insertions(+), 34 deletions(-) diff --git a/crates/radicle-protocol/src/wire.rs b/crates/radicle-protocol/src/wire.rs index ab7d08ee..df5cd639 100644 --- a/crates/radicle-protocol/src/wire.rs +++ b/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 { 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) } } diff --git a/crates/radicle/src/storage/git.rs b/crates/radicle/src/storage/git.rs index 0a9af664..61fbd8e9 100644 --- a/crates/radicle/src/storage/git.rs +++ b/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.retain(|name, oid| { - name.as_refstr() != refs::SIGREFS_BRANCH.as_ref() && !oid.is_zero() - }); + refs.remove_sigrefs(); 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(&sigref).unwrap(); + unsigned.remove_sigrefs().unwrap(); assert_eq!(remote.refs, signed); assert_eq!(*remote.refs, unsigned); diff --git a/crates/radicle/src/storage/refs.rs b/crates/radicle/src/storage/refs.rs index e60c9e45..434efdf6 100644 --- a/crates/radicle/src/storage/refs.rs +++ b/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); impl Refs { + pub fn new() -> Self { + Self(BTreeMap::new()) + } + /// Verify the given signature on these refs, and return [`SignedRefs`] on success. pub fn verified( 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 { 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 { + 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 { + self.0.remove(&SIGREFS_BRANCH.to_ref_string()) + } } impl IntoIterator for Refs { @@ -168,23 +214,16 @@ impl From> for Refs { } } -impl From> for Refs { - fn from(refs: BTreeMap) -> Self { - Self(refs) - } -} - -impl Deref for Refs { - type Target = BTreeMap; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl DerefMut for Refs { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.0 +impl From for Refs +where + I: Iterator, +{ + fn from(value: I) -> Self { + let mut refs = Self::new(); + for (refname, target) in value { + refs.insert(refname, target); + } + refs } } diff --git a/crates/radicle/src/storage/refs/arbitrary.rs b/crates/radicle/src/storage/refs/arbitrary.rs index 7da31b25..5fc690b3 100644 --- a/crates/radicle/src/storage/refs/arbitrary.rs +++ b/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 = 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 } }