Merge pull request #2105 from jalil-salame/fix-2103

fix(gix): extend lifetime of iterators

Author: Byron

gix-ref/src/store/file/overlay_iter.rs

@@ -189,11 +189,11 @@ impl Iterator for LooseThenPacked<'_, '_> {
     }
 }
 
-impl Platform<'_> {
+impl<'repo> Platform<'repo> {
     /// Return an iterator over all references, loose or packed, sorted by their name.
     ///
     /// Errors are returned similarly to what would happen when loose and packed refs were iterated by themselves.
-    pub fn all(&self) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    pub fn all<'p>(&'p self) -> std::io::Result<LooseThenPacked<'p, 'repo>> {
         self.store.iter_packed(self.packed.as_ref().map(|b| &***b))
     }
 
@@ -205,14 +205,14 @@ impl Platform<'_> {
     /// starts with `foo`, like `refs/heads/foo` and `refs/heads/foobar`.
     ///
     /// Prefixes are relative paths with slash-separated components.
-    pub fn prefixed(&self, prefix: &RelativePath) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    pub fn prefixed<'p>(&'p self, prefix: &RelativePath) -> std::io::Result<LooseThenPacked<'p, 'repo>> {
         self.store
             .iter_prefixed_packed(prefix, self.packed.as_ref().map(|b| &***b))
     }
 
     /// Return an iterator over the pseudo references, like `HEAD` or `FETCH_HEAD`, or anything else suffixed with `HEAD`
     /// in the root of the `.git` directory, sorted by name.
-    pub fn pseudo(&self) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    pub fn pseudo<'p>(&'p self) -> std::io::Result<LooseThenPacked<'p, 'repo>> {
         self.store.iter_pseudo()
     }
 }

gix/src/reference/iter.rs

@@ -13,15 +13,15 @@ pub struct Platform<'r> {
 }
 
 /// An iterator over references, with or without filter.
-pub struct Iter<'r> {
-    inner: gix_ref::file::iter::LooseThenPacked<'r, 'r>,
+pub struct Iter<'packed, 'repo> {
+    inner: gix_ref::file::iter::LooseThenPacked<'packed, 'repo>,
     peel_with_packed: Option<gix_ref::file::packed::SharedBufferSnapshot>,
     peel: bool,
-    repo: &'r crate::Repository,
+    repo: &'repo crate::Repository,
 }
 
-impl<'r> Iter<'r> {
-    fn new(repo: &'r crate::Repository, platform: gix_ref::file::iter::LooseThenPacked<'r, 'r>) -> Self {
+impl<'packed, 'repo> Iter<'packed, 'repo> {
+    fn new(repo: &'repo crate::Repository, platform: gix_ref::file::iter::LooseThenPacked<'packed, 'repo>) -> Self {
         Iter {
             inner: platform,
             peel_with_packed: None,
@@ -31,13 +31,13 @@ impl<'r> Iter<'r> {
     }
 }
 
-impl Platform<'_> {
+impl<'repo> Platform<'repo> {
     /// Return an iterator over all references in the repository, excluding
     /// pseudo references.
     ///
     /// Even broken or otherwise unparsable or inaccessible references are returned and have to be handled by the caller on a
     /// case by case basis.
-    pub fn all(&self) -> Result<Iter<'_>, init::Error> {
+    pub fn all(&self) -> Result<Iter<'_, 'repo>, init::Error> {
         Ok(Iter::new(self.repo, self.platform.all()?))
     }
 
@@ -47,23 +47,22 @@ impl Platform<'_> {
     pub fn prefixed<'a>(
         &self,
         prefix: impl TryInto<&'a RelativePath, Error = gix_path::relative_path::Error>,
-    ) -> Result<Iter<'_>, init::Error> {
+    ) -> Result<Iter<'_, 'repo>, init::Error> {
         Ok(Iter::new(self.repo, self.platform.prefixed(prefix.try_into()?)?))
     }
 
-    // TODO: tests
     /// Return an iterator over all references that are tags.
     ///
     /// They are all prefixed with `refs/tags`.
-    pub fn tags(&self) -> Result<Iter<'_>, init::Error> {
+    pub fn tags(&self) -> Result<Iter<'_, 'repo>, init::Error> {
         Ok(Iter::new(self.repo, self.platform.prefixed(b"refs/tags/".try_into()?)?))
     }
 
     // TODO: tests
     /// Return an iterator over all local branches.
     ///
     /// They are all prefixed with `refs/heads`.
-    pub fn local_branches(&self) -> Result<Iter<'_>, init::Error> {
+    pub fn local_branches(&self) -> Result<Iter<'_, 'repo>, init::Error> {
         Ok(Iter::new(
             self.repo,
             self.platform.prefixed(b"refs/heads/".try_into()?)?,
@@ -72,23 +71,23 @@ impl Platform<'_> {
 
     // TODO: tests
     /// Return an iterator over all local pseudo references.
-    pub fn pseudo(&self) -> Result<Iter<'_>, init::Error> {
+    pub fn pseudo(&self) -> Result<Iter<'_, 'repo>, init::Error> {
         Ok(Iter::new(self.repo, self.platform.pseudo()?))
     }
 
     // TODO: tests
     /// Return an iterator over all remote branches.
     ///
     /// They are all prefixed with `refs/remotes`.
-    pub fn remote_branches(&self) -> Result<Iter<'_>, init::Error> {
+    pub fn remote_branches(&self) -> Result<Iter<'_, 'repo>, init::Error> {
         Ok(Iter::new(
             self.repo,
             self.platform.prefixed(b"refs/remotes/".try_into()?)?,
         ))
     }
 }
 
-impl Iter<'_> {
+impl Iter<'_, '_> {
     /// Automatically peel references before yielding them during iteration.
     ///
     /// This has the same effect as using `iter.map(|r| {r.peel_to_id_in_place(); r})`.
@@ -104,7 +103,7 @@ impl Iter<'_> {
     }
 }
 
-impl<'r> Iterator for Iter<'r> {
+impl<'r> Iterator for Iter<'_, 'r> {
     type Item = Result<crate::Reference<'r>, Box<dyn std::error::Error + Send + Sync + 'static>>;
 
     fn next(&mut self) -> Option<Self::Item> {

gix/tests/gix/repository/reference.rs

@@ -183,6 +183,21 @@ mod iter_references {
         );
         Ok(())
     }
+
+    /// Regression test for https://github.com/GitoxideLabs/gitoxide/issues/2103
+    /// This only ensures we can return a reference, not that the code below is correct
+    #[test]
+    fn tags() -> crate::Result {
+        let repo = repo()?;
+        let actual = repo
+            .references()?
+            .tags()?
+            .filter_map(Result::ok)
+            .max_by_key(|tag| tag.name().shorten().to_owned())
+            .ok_or(std::io::Error::other("latest tag not found"))?;
+        assert_eq!(actual.name().as_bstr(), "refs/tags/t1");
+        Ok(())
+    }
 }
 
 mod head {