From d17cb3a8d30c7da3a8583989feb2f547ee5ae764 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Wed, 16 Nov 2022 12:53:50 +0000 Subject: [PATCH] radicle-surf: use specialised iterators for names The `tag_names` and `branch_names` functions use the `Tags` and `Branches` functions which require to go through their `TryFrom` instances. This can be expensive, in the case of `Tags` especially. Add `TagNames` and `BranchNames` iterators that only return the names of the tags and branches, respectively. This requires to change the definition of Revisions, where the NonEmpty branch list is now foregone and there references are collected in a BTreeSet to ensure ordering. Signed-off-by: Fintan Halpenny X-Clacks-Overhead: GNU Terry Pratchett --- radicle-surf/src/git/repo.rs | 24 +++-------- radicle-surf/src/git/repo/iter.rs | 72 +++++++++++++++++++++++++++++-- radicle-surf/src/git/tag.rs | 63 ++++++++++++++------------- radicle-surf/src/revision.rs | 72 ++++++++++++++----------------- 4 files changed, 143 insertions(+), 88 deletions(-) diff --git a/radicle-surf/src/git/repo.rs b/radicle-surf/src/git/repo.rs index 5baa872..0d46c46 100644 --- a/radicle-surf/src/git/repo.rs +++ b/radicle-surf/src/git/repo.rs @@ -23,7 +23,7 @@ use std::{ }; use directory::{Directory, FileContent}; -use git_ref_format::{refspec::QualifiedPattern, Qualified, RefString}; +use git_ref_format::{refspec::QualifiedPattern, Qualified}; use radicle_git_ext::Oid; use thiserror::Error; @@ -51,6 +51,8 @@ use crate::{ pub mod iter; pub use iter::{Branches, Namespaces, Tags}; +use self::iter::{BranchNames, TagNames}; + /// Enumeration of errors that can occur in operations from [`crate::git`]. #[derive(Debug, Error)] #[non_exhaustive] @@ -336,25 +338,13 @@ impl<'a> RepositoryRef<'a> { } /// Lists branch names with `filter`. - pub fn branch_names(&self, filter: &Glob) -> Result, Error> { - let mut branches = self - .branches(filter)? - .map(|b| b.map(|b| b.refname().into())) - .collect::, _>>()?; - branches.sort(); - - Ok(branches) + pub fn branch_names(&self, filter: &Glob) -> Result { + Ok(self.branches(filter)?.names()) } /// Lists tag names in the local RefScope. - pub fn tag_names(&self) -> Result, Error> { - let mut tags = self - .tags(&Glob::tags("*")?)? - .map(|t| t.map_err(Error::from).map(|t| t.refname().into())) - .collect::, Error>>()?; - tags.sort(); - - Ok(tags) + pub fn tag_names(&self) -> Result { + Ok(self.tags(&Glob::tags("*")?)?.names()) } /// Returns the Oid of the current HEAD diff --git a/radicle-surf/src/git/repo/iter.rs b/radicle-surf/src/git/repo/iter.rs index 9bd17b9..36920ab 100644 --- a/radicle-surf/src/git/repo/iter.rs +++ b/radicle-surf/src/git/repo/iter.rs @@ -6,19 +6,30 @@ use std::{ convert::TryFrom as _, }; -use crate::git::{Branch, Namespace, Tag}; +use git_ref_format::{lit, Qualified}; -/// An iterator for tags. +use crate::git::{tag, Branch, Namespace, Tag}; + +/// Iterator over [`Tag`]s. #[derive(Default)] pub struct Tags<'a> { references: Vec>, current: usize, } +/// Iterator over the [`Qualified`] names of [`Tag`]s. +pub struct TagNames<'a> { + inner: Tags<'a>, +} + impl<'a> Tags<'a> { pub(super) fn push(&mut self, references: git2::References<'a>) { self.references.push(references) } + + pub fn names(self) -> TagNames<'a> { + TagNames { inner: self } + } } impl<'a> Iterator for Tags<'a> { @@ -43,17 +54,49 @@ impl<'a> Iterator for Tags<'a> { } } -/// An iterator for branches. +impl<'a> Iterator for TagNames<'a> { + type Item = Result, error::Tag>; + + fn next(&mut self) -> Option { + while self.inner.current < self.inner.references.len() { + match self.inner.references.get_mut(self.inner.current) { + Some(refs) => match refs.next() { + Some(res) => { + return Some(res.map_err(error::Tag::from).and_then(|r| { + tag::reference_name(&r) + .map(|name| lit::refs_tags(name).into()) + .map_err(error::Tag::from) + })) + }, + None => self.inner.current += 1, + }, + None => break, + } + } + None + } +} + +/// Iterator over [`Branch`]es. #[derive(Default)] pub struct Branches<'a> { references: Vec>, current: usize, } +/// Iterator over the [`Qualified`] names of [`Branch`]es. +pub struct BranchNames<'a> { + inner: Branches<'a>, +} + impl<'a> Branches<'a> { pub(super) fn push(&mut self, references: git2::References<'a>) { self.references.push(references) } + + pub fn names(self) -> BranchNames<'a> { + BranchNames { inner: self } + } } impl<'a> Iterator for Branches<'a> { @@ -78,6 +121,29 @@ impl<'a> Iterator for Branches<'a> { } } +impl<'a> Iterator for BranchNames<'a> { + type Item = Result, error::Branch>; + + fn next(&mut self) -> Option { + while self.inner.current < self.inner.references.len() { + match self.inner.references.get_mut(self.inner.current) { + Some(refs) => match refs.next() { + Some(res) => { + return Some(res.map_err(error::Branch::from).and_then(|r| { + Branch::try_from(&r) + .map(|branch| branch.refname().into_owned()) + .map_err(error::Branch::from) + })) + }, + None => self.inner.current += 1, + }, + None => break, + } + } + None + } +} + // TODO: not sure this buys us much /// An iterator for namespaces. pub struct Namespaces { diff --git a/radicle-surf/src/git/tag.rs b/radicle-surf/src/git/tag.rs index 587bd2d..274c750 100644 --- a/radicle-surf/src/git/tag.rs +++ b/radicle-surf/src/git/tag.rs @@ -117,35 +117,40 @@ impl TryFrom<&git2::Reference<'_>> for Tag { type Error = error::FromReference; fn try_from(reference: &git2::Reference) -> Result { - let name = { - let name = str::from_utf8(reference.name_bytes())?; - RefStr::try_from_str(name)? - .qualified() - .ok_or_else(|| error::FromReference::NotQualified(name.to_string()))? - }; - - let (_refs, tags, c, cs) = name.non_empty_components(); - - if tags == component::TAGS { - match reference.peel_to_tag() { - Ok(tag) => Tag::try_from(&tag).map_err(error::FromReference::from), - // If we get an error peeling to a tag _BUT_ we also have confirmed the - // reference is a tag, that means we have a lightweight tag, - // i.e. a commit SHA and name. - Err(err) - if err.class() == git2::ErrorClass::Object - && err.code() == git2::ErrorCode::InvalidSpec => - { - let commit = reference.peel_to_commit()?; - Ok(Tag::Light { - id: commit.id().into(), - name: refstr_join(c, cs), - }) - }, - Err(err) => Err(err.into()), - } - } else { - Err(error::FromReference::NotTag(name.into())) + let name = reference_name(reference)?; + match reference.peel_to_tag() { + Ok(tag) => Tag::try_from(&tag).map_err(error::FromReference::from), + // If we get an error peeling to a tag _BUT_ we also have confirmed the + // reference is a tag, that means we have a lightweight tag, + // i.e. a commit SHA and name. + Err(err) + if err.class() == git2::ErrorClass::Object + && err.code() == git2::ErrorCode::InvalidSpec => + { + let commit = reference.peel_to_commit()?; + Ok(Tag::Light { + id: commit.id().into(), + name, + }) + }, + Err(err) => Err(err.into()), } } } + +pub(crate) fn reference_name( + reference: &git2::Reference, +) -> Result { + let name = str::from_utf8(reference.name_bytes())?; + let name = RefStr::try_from_str(name)? + .qualified() + .ok_or_else(|| error::FromReference::NotQualified(name.to_string()))?; + + let (_refs, tags, c, cs) = name.non_empty_components(); + + if tags == component::TAGS { + Ok(refstr_join(c, cs)) + } else { + Err(error::FromReference::NotTag(name.into())) + } +} diff --git a/radicle-surf/src/revision.rs b/radicle-surf/src/revision.rs index e384c8b..938c9aa 100644 --- a/radicle-surf/src/revision.rs +++ b/radicle-surf/src/revision.rs @@ -17,8 +17,9 @@ //! Represents revisions +use std::collections::BTreeSet; + use git_ref_format::{lit, Qualified, RefString}; -use nonempty::NonEmpty; #[cfg(feature = "serialize")] use serde::{Deserialize, Serialize}; @@ -104,9 +105,9 @@ pub struct Revisions { /// The user who owns these revisions. pub user: U, /// List of branch reference names. - pub branches: NonEmpty, + pub branches: BTreeSet, /// List of tag reference names. - pub tags: Vec, + pub tags: BTreeSet, } /// Provide the [`Revisions`] for the given `peer_id`, looking for the @@ -117,26 +118,22 @@ pub struct Revisions { /// # Errors /// /// * If we cannot get the branches from the `Browser` -pub fn remote( - repo: &RepositoryRef, - peer_id: P, - user: U, -) -> Result>, Error> +pub fn remote(repo: &RepositoryRef, peer_id: P, user: U) -> Result, Error> where P: Clone + ToString, { - let remote_branches = - repo.branch_names(&Glob::remotes(&format!("{}/*", peer_id.to_string()))?)?; - Ok( - NonEmpty::from_vec(remote_branches).map(|branches| Revisions { - peer_id, - user, - branches, - // TODO(rudolfs): implement remote peer tags once we decide how - // https://radicle.community/t/git-tags/214 - tags: vec![], - }), - ) + let branches = repo + .branch_names(&Glob::remotes(&format!("{}/*", peer_id.to_string()))?)? + .map(|name| name.map(RefString::from)) + .collect::>()?; + Ok(Revisions { + peer_id, + user, + branches, + // TODO(rudolfs): implement remote peer tags once we decide how + // https://radicle.community/t/git-tags/214 + tags: BTreeSet::new(), + }) } /// Provide the [`Revisions`] for the given `peer_id`, looking for the @@ -147,24 +144,24 @@ where /// # Errors /// /// * If we cannot get the branches from the `Browser` -pub fn local( - repo: &RepositoryRef, - peer_id: P, - user: U, -) -> Result>, Error> +pub fn local(repo: &RepositoryRef, peer_id: P, user: U) -> Result, Error> where P: Clone + ToString, { - let local_branches = repo.branch_names(&Glob::heads("*")?)?; - let tags = repo.tag_names()?; - Ok( - NonEmpty::from_vec(local_branches).map(|branches| Revisions { - peer_id, - user, - branches, - tags, - }), - ) + let branches = repo + .branch_names(&Glob::heads("*")?)? + .map(|name| name.map(RefString::from)) + .collect::>()?; + let tags = repo + .tag_names()? + .map(|name| name.map(RefString::from)) + .collect::>()?; + Ok(Revisions { + peer_id, + user, + branches, + tags, + }) } /// Provide the [`Revisions`] of a peer. @@ -178,10 +175,7 @@ where /// # Errors /// /// * If we cannot get the branches from the `Browser` -pub fn revisions( - repo: &RepositoryRef, - peer: Category, -) -> Result>, Error> +pub fn revisions(repo: &RepositoryRef, peer: Category) -> Result, Error> where P: Clone + ToString, {