radicle-surf: Namespace using RefString

A `Namespace` is a '/' separated list of components, where each
component represents another level of the namespaces hierarchy. Notice
that a `RefString` is also a '/' separated list of components, thus it
could be reused to define a `Namespace`.

This change uses this observation and reworks `Namespace` to be backed
by a `RefString`.

This naturally leads to defining its helper functions to return
`Namespaced` and `NamespacedPattern` reference names. They are
reworked to take a `Qualified` for regular reference names and
`QualifiedPattern` for refspec names, respectively.

This leads to changing the backing vector of `Glob` to use
`QualifiedPattern` instead of `PatternString`.

While changing `Glob`, it was also noticed that the PhantomData was
not used correctly -- all `impl`s used a generic parameter. This was
changed to use the `Namespace`, `Tag`, and `Branch` types directly,
providing the type-safety that would be expected.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett

radicle-surf/src/commit.rs

@@ -194,7 +194,7 @@ pub fn commit<R: git::Revision>(repo: &RepositoryRef, rev: R) -> Result<Commit,
     }
 
     let branches = repo
-        .revision_branches(&sha1, &Glob::heads("*")?.and_remotes("*")?)?
+        .revision_branches(&sha1, Glob::heads("*")?.and_remotes("*")?)?
         .into_iter()
         .map(|b| b.refname().into())
         .collect();

radicle-surf/src/git/glob.rs

@@ -17,9 +17,15 @@
 
 use std::{convert::TryFrom, marker::PhantomData, str};
 
-use git_ref_format::refspec::PatternString;
+use git_ref_format::{
+    refname,
+    refspec::{PatternString, QualifiedPattern},
+    RefString,
+};
 use thiserror::Error;
 
+use crate::git::{Branch, Namespace, Tag};
+
 #[derive(Debug, Error)]
 pub enum Error {
     #[error(transparent)]
@@ -28,22 +34,20 @@ pub enum Error {
 
 /// A collection of globs for T (a git reference type).
 pub struct Glob<T> {
-    globs: Vec<PatternString>,
+    globs: Vec<QualifiedPattern<'static>>,
     glob_type: PhantomData<T>, // To support different methods for different T.
 }
 
 impl<T> Glob<T> {
-    /// Returns the globs.
-    pub fn globs(&self) -> Vec<&str> {
-        self.globs.iter().map(|g| g.as_str()).collect()
+    pub fn globs(&self) -> impl Iterator<Item = &QualifiedPattern<'static>> {
+        self.globs.iter()
     }
 }
 
-impl<Namespace> Glob<Namespace> {
+impl Glob<Namespace> {
     /// Creates a `Glob` for namespaces.
     pub fn namespaces(glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/namespaces/{}", glob))?;
-        let globs = vec![pattern];
+        let globs = vec![Self::qualify(glob)?];
         Ok(Self {
             globs,
             glob_type: PhantomData,
@@ -52,16 +56,38 @@ impl<Namespace> Glob<Namespace> {
 
     /// Adds namespaces patterns to existing `Glob`.
     pub fn and(mut self, glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/namespaces/{}", glob))?;
-        self.globs.push(pattern);
+        self.globs.push(Self::qualify(glob)?);
         Ok(self)
     }
+
+    fn qualify(glob: &str) -> Result<QualifiedPattern<'static>, Error> {
+        Ok(
+            qualify(refname!("refs/namespaces"), PatternString::try_from(glob)?)
+                .expect("BUG: pattern is qualified"),
+        )
+    }
 }
 
-impl<Tag> Glob<Tag> {
+impl FromIterator<PatternString> for Glob<Namespace> {
+    fn from_iter<T: IntoIterator<Item = PatternString>>(iter: T) -> Self {
+        let globs = iter
+            .into_iter()
+            .map(|pat| {
+                qualify(refname!("refs/namespaces"), pat).expect("BUG: pattern is qualified")
+            })
+            .collect();
+
+        Self {
+            globs,
+            glob_type: PhantomData,
+        }
+    }
+}
+
+impl Glob<Tag> {
     /// Creates a `Glob` for local tags.
     pub fn tags(glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/tags/{}", glob))?;
+        let pattern = Self::qualify(glob)?;
         let globs = vec![pattern];
         Ok(Self {
             globs,
@@ -71,17 +97,36 @@ impl<Tag> Glob<Tag> {
 
     /// Updates a `Glob` to include other tags.
     pub fn and_tags(mut self, glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/tags/{}", glob))?;
-        self.globs.push(pattern);
+        self.globs.push(Self::qualify(glob)?);
         Ok(self)
     }
+
+    fn qualify(glob: &str) -> Result<QualifiedPattern<'static>, Error> {
+        Ok(
+            qualify(refname!("refs/tags"), PatternString::try_from(glob)?)
+                .expect("BUG: pattern is qualified"),
+        )
+    }
 }
 
-impl<Branch> Glob<Branch> {
+impl FromIterator<PatternString> for Glob<Tag> {
+    fn from_iter<T: IntoIterator<Item = PatternString>>(iter: T) -> Self {
+        let globs = iter
+            .into_iter()
+            .map(|pat| qualify(refname!("refs/tags"), pat).expect("BUG: pattern is qualified"))
+            .collect();
+
+        Self {
+            globs,
+            glob_type: PhantomData,
+        }
+    }
+}
+
+impl Glob<Branch> {
     /// Creates a `Glob` for local branches.
     pub fn heads(glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/heads/{}", glob))?;
-        let globs = vec![pattern];
+        let globs = vec![Self::qualify_heads(glob)?];
         Ok(Self {
             globs,
             glob_type: PhantomData,
@@ -90,8 +135,7 @@ impl<Branch> Glob<Branch> {
 
     /// Creates a `Glob` for remote branches.
     pub fn remotes(glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/remotes/{}", glob))?;
-        let globs = vec![pattern];
+        let globs = vec![Self::qualify_remotes(glob)?];
         Ok(Self {
             globs,
             glob_type: PhantomData,
@@ -100,15 +144,31 @@ impl<Branch> Glob<Branch> {
 
     /// Updates a `Glob` to include local branches.
     pub fn and_heads(mut self, glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/heads/{}", glob))?;
-        self.globs.push(pattern);
+        self.globs.push(Self::qualify_heads(glob)?);
         Ok(self)
     }
 
     /// Updates a `Glob` to include remote branches.
     pub fn and_remotes(mut self, glob: &str) -> Result<Self, Error> {
-        let pattern = PatternString::try_from(format!("refs/remotes/{}", glob))?;
-        self.globs.push(pattern);
+        self.globs.push(Self::qualify_remotes(glob)?);
         Ok(self)
     }
+
+    fn qualify_heads(glob: &str) -> Result<QualifiedPattern<'static>, Error> {
+        Ok(
+            qualify(refname!("refs/heads"), PatternString::try_from(glob)?)
+                .expect("BUG: pattern is qualified"),
+        )
+    }
+
+    fn qualify_remotes(glob: &str) -> Result<QualifiedPattern<'static>, Error> {
+        Ok(
+            qualify(refname!("refs/remotes"), PatternString::try_from(glob)?)
+                .expect("BUG: pattern is qualified"),
+        )
+    }
+}
+
+fn qualify(prefix: RefString, glob: PatternString) -> Option<QualifiedPattern<'static>> {
+    prefix.to_pattern(glob).qualified().map(|q| q.into_owned())
 }

radicle-surf/src/git/namespace.rs

@@ -15,8 +15,20 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use std::{convert::TryFrom, fmt, str};
+use std::{
+    convert::TryFrom,
+    fmt,
+    str::{self, FromStr},
+};
 
+use git_ref_format::{
+    refspec::{NamespacedPattern, PatternString, QualifiedPattern},
+    Component,
+    Namespaced,
+    Qualified,
+    RefStr,
+    RefString,
+};
 use nonempty::NonEmpty;
 pub use radicle_git_ext::Oid;
 use thiserror::Error;
@@ -25,49 +37,113 @@ use thiserror::Error;
 pub enum Error {
     /// When parsing a namespace we may come across one that was an empty
     /// string.
-    #[error("tried parsing the namespace but it was empty")]
+    #[error("namespaces must not be empty")]
     EmptyNamespace,
     #[error(transparent)]
+    RefFormat(#[from] git_ref_format::Error),
+    #[error(transparent)]
     Utf8(#[from] str::Utf8Error),
 }
 
 /// A `Namespace` value allows us to switch the git namespace of
 /// a repo.
+///
+/// A `Namespace` is one or more name components separated by `/`, e.g. `surf`,
+/// `surf/git`.
+///
+/// For each `Namespace`, the reference name will add a single `refs/namespaces`
+/// prefix, e.g. `refs/namespaces/surf`,
+/// `refs/namespaces/surf/refs/namespaces/git`.
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct Namespace {
-    /// Since namespaces can be nested we have a vector of strings.
-    /// This means that the namespaces `"foo/bar"` is represented as
-    /// `vec!["foo", "bar"]`.
-    pub(super) values: NonEmpty<String>,
+    // XXX: we rely on RefString being non-empty here, which
+    // git-ref-format ensures that there's no way to construct one.
+    pub(super) namespaces: RefString,
 }
 
 impl Namespace {
-    /// Appends a `refname` to this namespace, and returns
-    /// the full reference path.
-    pub fn append_refname(&self, refname: &str) -> String {
-        let mut prefix = String::new();
-        for value in self.values.iter() {
-            prefix = format!("{}refs/namespaces/{}/", &prefix, value);
+    /// Take a `Qualified` reference name and convert it to a `Namespaced` using
+    /// this `Namespace`.
+    ///
+    /// # Example
+    ///
+    /// ```no_run
+    /// let ns = "surf/git".parse::<Namespace>();
+    /// let name = ns.to_namespaced(qualified!("refs/heads/main"));
+    /// assert_eq!(
+    ///     name.as_str(),
+    ///     "refs/namespaces/surf/refs/namespaces/git/refs/heads/main"
+    /// );
+    /// ```
+    pub(crate) fn to_namespaced<'a>(&self, name: &Qualified<'a>) -> Namespaced<'a> {
+        let mut components = self.namespaces.components().rev();
+        let mut namespaced = name.with_namespace(
+            components
+                .next()
+                .expect("BUG: 'namespaces' cannot be empty"),
+        );
+        for ns in components {
+            let qualified = namespaced.into_qualified();
+            namespaced = qualified.with_namespace(ns);
         }
-        format!("{}{}", &prefix, refname)
+        namespaced
+    }
+
+    /// Take a `QualifiedPattern` reference name and convert it to a
+    /// `NamespacedPattern` using this `Namespace`.
+    ///
+    /// # Example
+    ///
+    /// ```no_run
+    /// let ns = "surf/git".parse::<Namespace>();
+    /// let name = ns.to_namespaced(pattern!("refs/heads/*").to_qualified().unwrap());
+    /// assert_eq!(
+    ///     name.as_str(),
+    ///     "refs/namespaces/surf/refs/namespaces/git/refs/heads/*"
+    /// );
+    /// ```
+    pub(crate) fn to_namespaced_pattern<'a>(
+        &self,
+        pat: &QualifiedPattern<'a>,
+    ) -> NamespacedPattern<'a> {
+        let pattern = PatternString::from(self.namespaces.clone());
+        let mut components = pattern.components().rev();
+        let mut namespaced = pat
+            .with_namespace(
+                components
+                    .next()
+                    .expect("BUG: 'namespaces' cannot be empty"),
+            )
+            .expect("BUG: 'namespace' cannot have globs");
+        for ns in components {
+            let qualified = namespaced.into_qualified();
+            namespaced = qualified
+                .with_namespace(ns)
+                .expect("BUG: 'namespaces' cannot have globs");
+        }
+        namespaced
     }
 }
 
 impl fmt::Display for Namespace {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        let values: Vec<_> = self.values.clone().into();
-        write!(f, "{}", values.join("/"))
+        write!(f, "{}", self.namespaces)
+    }
+}
+
+impl<'a> From<NonEmpty<Component<'a>>> for Namespace {
+    fn from(cs: NonEmpty<Component<'a>>) -> Self {
+        Self {
+            namespaces: cs.into_iter().collect::<RefString>(),
+        }
     }
 }
 
 impl TryFrom<&str> for Namespace {
     type Error = Error;
 
-    fn try_from(namespace: &str) -> Result<Self, Self::Error> {
-        let values = namespace.split('/').map(|n| n.to_string()).collect();
-        NonEmpty::from_vec(values)
-            .map(|values| Self { values })
-            .ok_or(Error::EmptyNamespace)
+    fn try_from(name: &str) -> Result<Self, Self::Error> {
+        Self::from_str(name)
     }
 }
 
@@ -77,29 +153,38 @@ impl TryFrom<&[u8]> for Namespace {
     fn try_from(namespace: &[u8]) -> Result<Self, Self::Error> {
         str::from_utf8(namespace)
             .map_err(Error::from)
-            .and_then(Namespace::try_from)
+            .and_then(Self::from_str)
     }
 }
 
-impl TryFrom<git2::Reference<'_>> for Namespace {
+impl FromStr for Namespace {
+    type Err = Error;
+
+    fn from_str(name: &str) -> Result<Self, Self::Err> {
+        let namespaces = RefStr::try_from_str(name)?.to_ref_string();
+        Ok(Self { namespaces })
+    }
+}
+
+impl From<Namespaced<'_>> for Namespace {
+    fn from(namespaced: Namespaced<'_>) -> Self {
+        let mut namespaces = namespaced.namespace().to_ref_string();
+        let mut qualified = namespaced.strip_namespace();
+        while let Some(namespaced) = qualified.to_namespaced() {
+            namespaces.push(namespaced.namespace());
+            qualified = namespaced.strip_namespace();
+        }
+        Self { namespaces }
+    }
+}
+
+impl TryFrom<&git2::Reference<'_>> for Namespace {
     type Error = Error;
 
-    fn try_from(reference: git2::Reference) -> Result<Self, Self::Error> {
-        let re = regex::Regex::new(r"refs/namespaces/([^/]+)/").unwrap();
-        let ref_name = str::from_utf8(reference.name_bytes())?;
-        let values = re
-            .find_iter(ref_name)
-            .map(|m| {
-                String::from(
-                    m.as_str()
-                        .trim_start_matches("refs/namespaces/")
-                        .trim_end_matches('/'),
-                )
-            })
-            .collect::<Vec<_>>();
-
-        NonEmpty::from_vec(values)
-            .map(|values| Self { values })
+    fn try_from(reference: &git2::Reference) -> Result<Self, Self::Error> {
+        let name = RefStr::try_from_str(str::from_utf8(reference.name_bytes())?)?;
+        name.to_namespaced()
             .ok_or(Error::EmptyNamespace)
+            .map(Self::from)
     }
 }

radicle-surf/src/git/repo.rs

@@ -23,7 +23,7 @@ use std::{
 };
 
 use directory::{Directory, FileContent};
-use git_ref_format::RefString;
+use git_ref_format::{refspec::QualifiedPattern, Qualified, RefString};
 use radicle_git_ext::Oid;
 use thiserror::Error;
 
@@ -72,10 +72,14 @@ pub enum Error {
     Glob(#[from] glob::Error),
     #[error(transparent)]
     Namespace(#[from] namespace::Error),
+    #[error("the reference '{0}' should be of the form 'refs/<category>/<path>'")]
+    NotQualified(String),
     /// The requested file was not found.
     #[error("path not found for: {0}")]
     PathNotFound(file_system::Path),
     #[error(transparent)]
+    RefFormat(#[from] git_ref_format::Error),
+    #[error(transparent)]
     Revision(Box<dyn std::error::Error + Send + Sync + 'static>),
     /// A `revspec` was provided that could not be parsed into a branch, tag, or
     /// commit object.
@@ -131,8 +135,8 @@ impl<'a> RepositoryRef<'a> {
     /// Returns an iterator of branches that match `pattern`.
     pub fn branches(&self, pattern: &Glob<Branch>) -> Result<Branches, Error> {
         let mut branches = Branches::default();
-        for glob in pattern.globs().iter() {
-            let namespaced = self.namespaced_refname(glob)?;
+        for glob in pattern.globs() {
+            let namespaced = self.namespaced_pattern(glob)?;
             let references = self.repo_ref.references_glob(&namespaced)?;
             branches.push(references);
         }
@@ -142,8 +146,8 @@ impl<'a> RepositoryRef<'a> {
     /// Returns an iterator of tags that match `pattern`.
     pub fn tags(&self, pattern: &Glob<Tag>) -> Result<Tags, Error> {
         let mut tags = Tags::default();
-        for glob in pattern.globs().iter() {
-            let namespaced = self.namespaced_refname(glob)?;
+        for glob in pattern.globs() {
+            let namespaced = self.namespaced_pattern(glob)?;
             let references = self.repo_ref.references_glob(&namespaced)?;
             tags.push(references);
         }
@@ -153,14 +157,14 @@ impl<'a> RepositoryRef<'a> {
     /// Returns an iterator of namespaces that match `pattern`.
     pub fn namespaces(&self, pattern: &Glob<Namespace>) -> Result<Namespaces, Error> {
         let mut set = BTreeSet::new();
-        for glob in pattern.globs().iter() {
+        for glob in pattern.globs() {
             let new_set = self
                 .repo_ref
                 .references_glob(glob)?
                 .map(|reference| {
                     reference
                         .map_err(Error::Git)
-                        .and_then(|r| Namespace::try_from(r).map_err(Error::from))
+                        .and_then(|r| Namespace::try_from(&r).map_err(Error::from))
                 })
                 .collect::<Result<BTreeSet<Namespace>, Error>>()?;
             set.extend(new_set);
@@ -366,10 +370,25 @@ impl<'a> RepositoryRef<'a> {
     }
 
     /// Returns a full reference name with namespace(s) included.
-    pub(crate) fn namespaced_refname(&self, refname: &str) -> Result<String, Error> {
+    pub(crate) fn namespaced_refname(
+        &'a self,
+        refname: &Qualified<'a>,
+    ) -> Result<Qualified<'a>, Error> {
         let fullname = match self.which_namespace()? {
-            Some(namespace) => namespace.append_refname(refname),
-            None => refname.to_string(),
+            Some(namespace) => namespace.to_namespaced(refname).into_qualified(),
+            None => refname.clone(),
+        };
+        Ok(fullname)
+    }
+
+    /// Returns a full reference name with namespace(s) included.
+    pub(crate) fn namespaced_pattern(
+        &'a self,
+        refname: &QualifiedPattern<'a>,
+    ) -> Result<QualifiedPattern<'a>, Error> {
+        let fullname = match self.which_namespace()? {
+            Some(namespace) => namespace.to_namespaced_pattern(refname).into_qualified(),
+            None => refname.clone(),
         };
         Ok(fullname)
     }
@@ -408,12 +427,12 @@ impl<'a> RepositoryRef<'a> {
     }
 
     /// Lists branches that are reachable from `oid`.
-    pub fn revision_branches(&self, oid: &Oid, glob: &Glob<Branch>) -> Result<Vec<Branch>, Error> {
+    pub fn revision_branches(&self, oid: &Oid, glob: Glob<Branch>) -> Result<Vec<Branch>, Error> {
         let mut contained_branches = vec![];
-        for branch in self.branches(glob)? {
+        for branch in self.branches(&glob)? {
             let branch = branch?;
             let namespaced = self.namespaced_refname(&branch.refname())?;
-            let reference = self.repo_ref.find_reference(&namespaced)?;
+            let reference = self.repo_ref.find_reference(namespaced.as_str())?;
             if self.reachable_from(&reference, oid)? {
                 contained_branches.push(branch);
             }

radicle-surf/t/src/git.rs

@@ -8,7 +8,7 @@ use radicle_surf::git::{Author, Commit};
 use radicle_surf::{
     diff::*,
     file_system::{unsound, DirectoryEntry, Path},
-    git::{Branch, Error, Glob, Namespace, Oid, Repository},
+    git::{Branch, Error, Glob, Oid, Repository},
 };
 
 const GIT_PLATINUM: &str = "../data/git-platinum";
@@ -53,10 +53,7 @@ mod namespace {
         assert_eq!(repo.which_namespace().unwrap(), None);
 
         repo.switch_namespace("me")?;
-        assert_eq!(
-            repo.which_namespace().unwrap(),
-            Some(Namespace::try_from("me")?)
-        );
+        assert_eq!(repo.which_namespace().unwrap(), Some("me".parse()?));
 
         let history_feature = repo.history(&Branch::local(refname!("feature/#1194")))?;
         assert_eq!(history.head(), history_feature.head());
@@ -93,10 +90,7 @@ mod namespace {
 
         repo.switch_namespace("golden")?;
 
-        assert_eq!(
-            repo.which_namespace().unwrap(),
-            Some(Namespace::try_from("golden")?)
-        );
+        assert_eq!(repo.which_namespace().unwrap(), Some("golden".parse()?));
 
         let golden_history = repo.history(&Branch::local(refname!("master")))?;
         assert_eq!(history.head(), golden_history.head());
@@ -141,7 +135,7 @@ mod namespace {
         repo.switch_namespace("golden/silver")?;
         assert_eq!(
             repo.which_namespace().unwrap(),
-            Some(Namespace::try_from("golden/silver")?)
+            Some("golden/silver".parse()?)
         );
         let silver_history = repo.history(&Branch::local(refname!("master")))?;
         assert_ne!(history.head(), silver_history.head());