fix!: prefixed ref iteration now properly deals with slashes.

Byron · Byron · commit f5385595987e · 2025-04-13T18:00:59.000+02:00
Previously, `refs/heads/foo/bar` would be listed when running
`repo.references()?.prefixed("refs/heads/b")`. The code identified that
the last component was not a directory and started to match it as a
filename prefix for all files in all recursive directories, effectively
matching `refs/heads/**/b*`.

This commit fixes that bug but also allows to use a trailing `/` in the
prefix, allowing to filter for `refs/heads/foo/` and not get
`refs/heads/foo-bar` as a result.
diff --git a/gix-ref/src/namespace.rs b/gix-ref/src/namespace.rs
@@ -1,4 +1,4 @@
-use std::{borrow::Cow, path::Path};
+use std::path::Path;
 
 use gix_object::bstr::{BStr, BString, ByteSlice, ByteVec};
 
@@ -18,9 +18,12 @@ impl Namespace {
         gix_path::from_byte_slice(&self.0)
     }
     /// Append the given `prefix` to this namespace so it becomes usable for prefixed iteration.
-    pub fn into_namespaced_prefix(mut self, prefix: &BStr) -> Cow<'_, BStr> {
-        self.0.push_str(prefix);
-        gix_path::to_unix_separators_on_windows(self.0)
+    ///
+    /// The prefix is a relative path with slash-separated path components.
+    // TODO: use `RelativePath` type instead (see #1921), or a trait that helps convert into it.
+    pub fn into_namespaced_prefix<'a>(mut self, prefix: impl Into<&'a BStr>) -> BString {
+        self.0.push_str(prefix.into());
+        gix_path::to_unix_separators_on_windows(self.0).into_owned()
     }
     pub(crate) fn into_namespaced_name(mut self, name: &FullNameRef) -> FullName {
         self.0.push_str(name.as_bstr());
diff --git a/gix-ref/src/store/file/loose/iter.rs b/gix-ref/src/store/file/loose/iter.rs
@@ -1,7 +1,4 @@
-use std::{
-    borrow::Cow,
-    path::{Path, PathBuf},
-};
+use std::path::{Path, PathBuf};
 
 use gix_features::fs::walkdir::DirEntryIter;
 use gix_object::bstr::ByteSlice;
@@ -11,7 +8,7 @@ use crate::{file::iter::LooseThenPacked, store_impl::file, BStr, BString, FullNa
 /// An iterator over all valid loose reference paths as seen from a particular base directory.
 pub(in crate::store_impl::file) struct SortedLoosePaths {
     pub(crate) base: PathBuf,
-    /// A optional prefix to match against if the prefix is not the same as the `file_walk` path.
+    /// An prefix like `refs/heads/foo/` or `refs/heads/prefix` that a returned reference must match against..
     prefix: Option<BString>,
     file_walk: Option<DirEntryIter>,
 }
@@ -48,12 +45,10 @@ impl Iterator for SortedLoosePaths {
                     let full_name = full_path
                         .strip_prefix(&self.base)
                         .expect("prefix-stripping cannot fail as base is within our root");
-                    let full_name = match gix_path::try_into_bstr(full_name) {
-                        Ok(name) => {
-                            let name = gix_path::to_unix_separators_on_windows(name);
-                            name.into_owned()
-                        }
-                        Err(_) => continue, // TODO: silently skipping ill-formed UTF-8 on windows here, maybe there are better ways?
+                    let Ok(full_name) = gix_path::try_into_bstr(full_name)
+                        .map(|name| gix_path::to_unix_separators_on_windows(name).into_owned())
+                    else {
+                        continue;
                     };
                     if let Some(prefix) = &self.prefix {
                         if !full_name.starts_with(prefix) {
@@ -86,11 +81,15 @@ impl file::Store {
 
     /// Return an iterator over all loose references that start with the given `prefix`.
     ///
-    /// Otherwise it's similar to [`loose_iter()`][file::Store::loose_iter()].
-    pub fn loose_iter_prefixed<'a>(
-        &self,
-        prefix: impl Into<Cow<'a, BStr>>,
-    ) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    /// Otherwise, it's similar to [`loose_iter()`](file::Store::loose_iter()).
+    ///
+    /// Note that if a prefix isn't using a trailing `/`, like in `refs/heads/foo`, it will effectively
+    /// start the traversal in the parent directory, e.g. `refs/heads/` and list everything inside that
+    /// starts with `foo`, like `refs/heads/foo` and `refs/heads/foobar`.
+    ///
+    /// Prefixes are relative paths with slash-separated components.
+    // TODO: use `RelativePath` type instead (see #1921), or a trait that helps convert into it.
+    pub fn loose_iter_prefixed<'a>(&self, prefix: impl Into<&'a BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {
         self.iter_prefixed_packed(prefix.into(), None)
     }
 }
diff --git a/gix-ref/src/store/file/overlay_iter.rs b/gix-ref/src/store/file/overlay_iter.rs
@@ -195,9 +195,16 @@ impl Platform<'_> {
         self.store.iter_packed(self.packed.as_ref().map(|b| &***b))
     }
 
-    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads/" or
+    /// As [`iter(…)`](file::Store::iter()), but filters by `prefix`, i.e. "refs/heads/" or
     /// "refs/heads/feature-".
-    pub fn prefixed<'a>(&self, prefix: impl Into<Cow<'a, BStr>>) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    ///
+    /// Note that if a prefix isn't using a trailing `/`, like in `refs/heads/foo`, it will effectively
+    /// start the traversal in the parent directory, e.g. `refs/heads/` and list everything inside that
+    /// starts with `foo`, like `refs/heads/foo` and `refs/heads/foobar`.
+    ///
+    /// Prefixes are relative paths with slash-separated components.
+    // TODO: use `RelativePath` type instead (see #1921), or a trait that helps convert into it.
+    pub fn prefixed<'a>(&self, prefix: impl Into<&'a BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {
         self.store
             .iter_prefixed_packed(prefix.into(), self.packed.as_ref().map(|b| &***b))
     }
@@ -227,7 +234,7 @@ pub(crate) enum IterInfo<'a> {
     BaseAndIterRoot {
         base: &'a Path,
         iter_root: PathBuf,
-        prefix: Cow<'a, Path>,
+        prefix: PathBuf,
         precompose_unicode: bool,
     },
     PrefixAndBase {
@@ -238,9 +245,9 @@ pub(crate) enum IterInfo<'a> {
     ComputedIterationRoot {
         /// The root to iterate over
         iter_root: PathBuf,
-        /// The top-level directory as boundary of all references, used to create their short-names after iteration
+        /// The top-level directory as boundary of all references, used to create their short-names after iteration.
         base: &'a Path,
-        /// The original prefix
+        /// The original prefix.
         prefix: Cow<'a, BStr>,
         /// If `true`, we will convert decomposed into precomposed unicode.
         precompose_unicode: bool,
@@ -290,7 +297,7 @@ impl<'a> IterInfo<'a> {
         precompose_unicode: bool,
     ) -> std::io::Result<Self> {
         let prefix = prefix.into();
-        let prefix_path = gix_path::from_bstring(prefix.clone().into_owned());
+        let prefix_path = gix_path::from_bstr(prefix.as_ref());
         if prefix_path.is_absolute() {
             return Err(std::io::Error::new(
                 std::io::ErrorKind::InvalidInput,
@@ -309,7 +316,7 @@ impl<'a> IterInfo<'a> {
             Ok(IterInfo::BaseAndIterRoot {
                 base,
                 iter_root,
-                prefix: prefix_path.into(),
+                prefix: prefix_path.into_owned(),
                 precompose_unicode,
             })
         } else {
@@ -363,24 +370,31 @@ impl file::Store {
         }
     }
 
-    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads/" or
-    /// "refs/heads/feature-".
-    pub fn iter_prefixed_packed<'s, 'p>(
+    /// As [`iter(…)`](file::Store::iter()), but filters by `prefix`, i.e. `refs/heads/` or
+    /// `refs/heads/feature-`.
+    /// Note that if a prefix isn't using a trailing `/`, like in `refs/heads/foo`, it will effectively
+    /// start the traversal in the parent directory, e.g. `refs/heads/` and list everything inside that
+    /// starts with `foo`, like `refs/heads/foo` and `refs/heads/foobar`.
+    ///
+    /// Prefixes are relative paths with slash-separated components.
+    // TODO: use `RelativePath` type instead (see #1921), or a trait that helps convert into it.
+    pub fn iter_prefixed_packed<'a, 's, 'p>(
         &'s self,
-        prefix: Cow<'_, BStr>,
+        prefix: impl Into<&'a BStr>,
         packed: Option<&'p packed::Buffer>,
     ) -> std::io::Result<LooseThenPacked<'p, 's>> {
+        let prefix = prefix.into();
         match self.namespace.as_ref() {
             None => {
-                let git_dir_info = IterInfo::from_prefix(self.git_dir(), prefix.clone(), self.precompose_unicode)?;
+                let git_dir_info = IterInfo::from_prefix(self.git_dir(), prefix, self.precompose_unicode)?;
                 let common_dir_info = self
                     .common_dir()
                     .map(|base| IterInfo::from_prefix(base, prefix, self.precompose_unicode))
                     .transpose()?;
                 self.iter_from_info(git_dir_info, common_dir_info, packed)
             }
             Some(namespace) => {
-                let prefix = namespace.to_owned().into_namespaced_prefix(&prefix);
+                let prefix = namespace.to_owned().into_namespaced_prefix(prefix);
                 let git_dir_info = IterInfo::from_prefix(self.git_dir(), prefix.clone(), self.precompose_unicode)?;
                 let common_dir_info = self
                     .common_dir()
diff --git a/gix-ref/tests/refs/file/store/iter.rs b/gix-ref/tests/refs/file/store/iter.rs
@@ -1,5 +1,3 @@
-use std::borrow::Cow;
-
 use gix_object::bstr::ByteSlice;
 
 use crate::{
@@ -326,7 +324,7 @@ fn loose_iter_with_prefix_wont_allow_absolute_paths() -> crate::Result {
     #[cfg(windows)]
     let abs_path = r"c:\hello";
 
-    match store.loose_iter_prefixed(Cow::Borrowed(abs_path.as_ref())) {
+    match store.loose_iter_prefixed(abs_path) {
         Ok(_) => unreachable!("absolute paths aren't allowed"),
         Err(err) => assert_eq!(err.to_string(), "prefix must be a relative path, like 'refs/heads/'"),
     }
@@ -335,11 +333,9 @@ fn loose_iter_with_prefix_wont_allow_absolute_paths() -> crate::Result {
 
 #[test]
 fn loose_iter_with_prefix() -> crate::Result {
-    // Test 'refs/heads/' with slash.
-    let store = store()?;
-
-    let actual = store
-        .loose_iter_prefixed(b"refs/heads/".as_bstr())?
+    let prefix_with_slash = b"refs/heads/";
+    let actual = store()?
+        .loose_iter_prefixed(prefix_with_slash)?
         .collect::<Result<Vec<_>, _>>()
         .expect("no broken ref in this subset")
         .into_iter()
@@ -365,11 +361,9 @@ fn loose_iter_with_prefix() -> crate::Result {
 
 #[test]
 fn loose_iter_with_partial_prefix_dir() -> crate::Result {
-    // Test 'refs/heads/' without slash.
-    let store = store()?;
-
-    let actual = store
-        .loose_iter_prefixed(b"refs/heads".as_bstr())?
+    let prefix_without_slash = b"refs/heads";
+    let actual = store()?
+        .loose_iter_prefixed(prefix_without_slash)?
         .collect::<Result<Vec<_>, _>>()
         .expect("no broken ref in this subset")
         .into_iter()
@@ -395,9 +389,7 @@ fn loose_iter_with_partial_prefix_dir() -> crate::Result {
 
 #[test]
 fn loose_iter_with_partial_prefix() -> crate::Result {
-    let store = store()?;
-
-    let actual = store
+    let actual = store()?
         .loose_iter_prefixed(b"refs/heads/d".as_bstr())?
         .collect::<Result<Vec<_>, _>>()
         .expect("no broken ref in this subset")
@@ -542,7 +534,7 @@ fn overlay_iter_with_prefix_wont_allow_absolute_paths() -> crate::Result {
     #[cfg(windows)]
     let abs_path = r"c:\hello";
 
-    match store.iter()?.prefixed(Cow::Borrowed(abs_path.as_ref())) {
+    match store.iter()?.prefixed(abs_path) {
         Ok(_) => unreachable!("absolute paths aren't allowed"),
         Err(err) => assert_eq!(err.to_string(), "prefix must be a relative path, like 'refs/heads/'"),
     }
@@ -556,7 +548,7 @@ fn overlay_prefixed_iter() -> crate::Result {
     let store = store_at("make_packed_ref_repository_for_overlay.sh")?;
     let ref_names = store
         .iter()?
-        .prefixed(b"refs/heads/".as_bstr())?
+        .prefixed(b"refs/heads/")?
         .map(|r| r.map(|r| (r.name.as_bstr().to_owned(), r.target)))
         .collect::<Result<Vec<_>, _>>()?;
     let c1 = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
@@ -579,7 +571,7 @@ fn overlay_partial_prefix_iter() -> crate::Result {
     let store = store_at("make_packed_ref_repository_for_overlay.sh")?;
     let ref_names = store
         .iter()?
-        .prefixed(b"refs/heads/m".as_bstr())? // 'm' is partial
+        .prefixed(b"refs/heads/m")? // 'm' is partial
         .map(|r| r.map(|r| (r.name.as_bstr().to_owned(), r.target)))
         .collect::<Result<Vec<_>, _>>()?;
     let c1 = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
@@ -597,11 +589,14 @@ fn overlay_partial_prefix_iter_reproduce_1934() -> crate::Result {
 
     let ref_names = store
         .iter()?
-        .prefixed(b"refs/d".as_bstr())?
+        .prefixed(b"refs/d")?
         .map(|r| r.map(|r| (r.name.as_bstr().to_owned(), r.target)))
         .collect::<Result<Vec<_>, _>>()?;
-    // Should not match `refs/heads/d1`.
-    assert_eq!(ref_names, vec![("refs/d1".into(), Object(c1)),]);
+    assert_eq!(
+        ref_names,
+        vec![("refs/d1".into(), Object(c1))],
+        "Should not match `refs/heads/d1`"
+    );
     Ok(())
 }
 
@@ -615,7 +610,7 @@ fn overlay_partial_prefix_iter_when_prefix_is_dir() -> crate::Result {
 
     let ref_names = store
         .iter()?
-        .prefixed(b"refs/prefix/feature".as_bstr())?
+        .prefixed(b"refs/prefix/feature")?
         .map(|r| r.map(|r| (r.name.as_bstr().to_owned(), r.target)))
         .collect::<Result<Vec<_>, _>>()?;
     assert_eq!(
@@ -628,7 +623,7 @@ fn overlay_partial_prefix_iter_when_prefix_is_dir() -> crate::Result {
 
     let ref_names = store
         .iter()?
-        .prefixed(b"refs/prefix/feature/".as_bstr())?
+        .prefixed(b"refs/prefix/feature/")?
         .map(|r| r.map(|r| (r.name.as_bstr().to_owned(), r.target)))
         .collect::<Result<Vec<_>, _>>()?;
     assert_eq!(
diff --git a/gix-ref/tests/refs/file/worktree.rs b/gix-ref/tests/refs/file/worktree.rs
@@ -191,7 +191,6 @@ mod read_only {
 mod writable {
     use gix_lock::acquire::Fail;
     use gix_ref::{
-        bstr::ByteSlice,
         file::{transaction::PackedRefs, Store},
         transaction::{Change, LogChange, PreviousValue, RefEdit},
         FullName, FullNameRef, Target,
@@ -291,7 +290,7 @@ mod writable {
             assert_eq!(
                 store
                     .iter()?
-                    .prefixed(b"refs/stacks/".as_bstr())?
+                    .prefixed(b"refs/stacks/")?
                     .map(Result::unwrap)
                     .map(|r| (r.name.to_string(), r.target.to_string()))
                     .collect::<Vec<_>>(),
@@ -572,7 +571,7 @@ mod writable {
             assert_eq!(
                 store
                     .iter()?
-                    .prefixed(b"refs/stacks/".as_bstr())?
+                    .prefixed(b"refs/stacks/")?
                     .map(Result::unwrap)
                     .map(|r| (r.name.to_string(), r.target.to_string()))
                     .collect::<Vec<_>>(),
diff --git a/gix-ref/tests/refs/namespace.rs b/gix-ref/tests/refs/namespace.rs
@@ -3,14 +3,14 @@ fn into_namespaced_prefix() {
     assert_eq!(
         gix_ref::namespace::expand("foo")
             .unwrap()
-            .into_namespaced_prefix("prefix".as_ref()),
-        "refs/namespaces/foo/prefix".as_ref(),
+            .into_namespaced_prefix("prefix"),
+        "refs/namespaces/foo/prefix",
     );
     assert_eq!(
         gix_ref::namespace::expand("foo")
             .unwrap()
-            .into_namespaced_prefix("prefix/".as_ref()),
-        "refs/namespaces/foo/prefix/".as_ref(),
+            .into_namespaced_prefix("prefix/"),
+        "refs/namespaces/foo/prefix/",
     );
 }
 
