Merge pull request #1954 from GitoxideLabs/fix-recursive-list-refs-prefix

Fix recursive list refs prefix

Author: Byron

gix-negotiate/tests/baseline/mod.rs

@@ -71,7 +71,7 @@ fn run() -> crate::Result {
                 // }
                 for tip in lookup_names(&["HEAD"]).into_iter().chain(
                     refs.iter()?
-                        .prefixed("refs/heads".as_ref())?
+                        .prefixed(b"refs/heads")?
                         .filter_map(Result::ok)
                         .map(|r| r.target.into_id()),
                 ) {

gix-ref/src/namespace.rs

@@ -1,4 +1,4 @@
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
 use gix_object::bstr::{BStr, BString, ByteSlice, ByteVec};
 
@@ -18,10 +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: &Path) -> PathBuf {
-        let prefix = gix_path::into_bstr(prefix);
-        self.0.push_str(prefix.as_ref());
-        gix_path::to_native_path_on_windows(self.0).into_owned()
+    ///
+    /// 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());

gix-ref/src/store/file/loose/iter.rs

@@ -3,20 +3,21 @@ use std::path::{Path, PathBuf};
 use gix_features::fs::walkdir::DirEntryIter;
 use gix_object::bstr::ByteSlice;
 
-use crate::{file::iter::LooseThenPacked, store_impl::file, BString, FullName};
+use crate::{file::iter::LooseThenPacked, store_impl::file, BStr, BString, FullName};
 
 /// 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,
-    filename_prefix: Option<BString>,
+    /// An prefix like `refs/heads/foo/` or `refs/heads/prefix` that a returned reference must match against..
+    prefix: Option<BString>,
     file_walk: Option<DirEntryIter>,
 }
 
 impl SortedLoosePaths {
-    pub fn at(path: &Path, base: PathBuf, filename_prefix: Option<BString>, precompose_unicode: bool) -> Self {
+    pub fn at(path: &Path, base: PathBuf, prefix: Option<BString>, precompose_unicode: bool) -> Self {
         SortedLoosePaths {
             base,
-            filename_prefix,
+            prefix,
             file_walk: path.is_dir().then(|| {
                 // serial iteration as we expect most refs in packed-refs anyway.
                 gix_features::fs::walkdir_sorted_new(
@@ -41,31 +42,19 @@ impl Iterator for SortedLoosePaths {
                         continue;
                     }
                     let full_path = entry.path().into_owned();
-                    if let Some((prefix, name)) = self
-                        .filename_prefix
-                        .as_deref()
-                        .and_then(|prefix| full_path.file_name().map(|name| (prefix, name)))
-                    {
-                        match gix_path::os_str_into_bstr(name) {
-                            Ok(name) => {
-                                if !name.starts_with(prefix) {
-                                    continue;
-                                }
-                            }
-                            Err(_) => continue, // TODO: silently skipping ill-formed UTF-8 on windows - maybe this can be better?
-                        }
-                    }
                     let full_name = full_path
                         .strip_prefix(&self.base)
-                        .expect("prefix-stripping cannot fail as prefix is 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?
+                        .expect("prefix-stripping cannot fail as base is within our root");
+                    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) {
+                            continue;
+                        }
+                    }
                     if gix_validate::reference::name_partial(full_name.as_bstr()).is_ok() {
                         let name = FullName(full_name);
                         return Some(Ok((full_path, name)));
@@ -92,8 +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(&self, prefix: &Path) -> std::io::Result<LooseThenPacked<'_, '_>> {
-        self.iter_prefixed_packed(prefix, None)
+    /// 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)
     }
 }

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

@@ -1,9 +1,6 @@
-use std::{
-    borrow::Cow,
-    path::{Path, PathBuf},
-};
+use std::path::PathBuf;
 
-use crate::{bstr::BStr, store::WriteReflog, Namespace};
+use crate::{store::WriteReflog, Namespace};
 
 /// A store for reference which uses plain files.
 ///
@@ -92,11 +89,6 @@ pub struct Transaction<'s, 'p> {
     packed_refs: transaction::PackedRefs<'p>,
 }
 
-pub(in crate::store_impl::file) fn path_to_name<'a>(path: impl Into<Cow<'a, Path>>) -> Cow<'a, BStr> {
-    let path = gix_path::into_bstr(path.into());
-    gix_path::to_unix_separators_on_windows(path)
-}
-
 ///
 pub mod loose;
 mod overlay_iter;

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

@@ -7,9 +7,9 @@ use std::{
 };
 
 use crate::{
-    file::{loose, loose::iter::SortedLoosePaths, path_to_name},
+    file::{loose, loose::iter::SortedLoosePaths},
     store_impl::{file, packed},
-    BString, FullName, Namespace, Reference,
+    BStr, FullName, Namespace, Reference,
 };
 
 /// An iterator stepping through sorted input of loose references and packed references, preferring loose refs over otherwise
@@ -195,12 +195,18 @@ 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".
+    /// As [`iter(…)`](file::Store::iter()), but filters by `prefix`, i.e. "refs/heads/" or
+    /// "refs/heads/feature-".
     ///
-    /// Please note that "refs/heads" or "refs\\heads" is equivalent to "refs/heads/"
-    pub fn prefixed(&self, prefix: &Path) -> 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, self.packed.as_ref().map(|b| &***b))
+            .iter_prefixed_packed(prefix.into(), self.packed.as_ref().map(|b| &***b))
     }
 }
 
@@ -228,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 {
@@ -239,25 +245,22 @@ 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
-        prefix: Cow<'a, Path>,
-        /// The remainder of the prefix that wasn't a valid path
-        remainder: Option<BString>,
+        /// The original prefix.
+        prefix: Cow<'a, BStr>,
         /// If `true`, we will convert decomposed into precomposed unicode.
         precompose_unicode: bool,
     },
 }
 
 impl<'a> IterInfo<'a> {
-    fn prefix(&self) -> Option<&Path> {
+    fn prefix(&self) -> Option<Cow<'_, BStr>> {
         match self {
             IterInfo::Base { .. } => None,
-            IterInfo::PrefixAndBase { prefix, .. } => Some(*prefix),
-            IterInfo::ComputedIterationRoot { prefix, .. } | IterInfo::BaseAndIterRoot { prefix, .. } => {
-                prefix.as_ref().into()
-            }
+            IterInfo::PrefixAndBase { prefix, .. } => Some(gix_path::into_bstr(*prefix)),
+            IterInfo::BaseAndIterRoot { prefix, .. } => Some(gix_path::into_bstr(prefix.clone())),
+            IterInfo::ComputedIterationRoot { prefix, .. } => Some(prefix.clone()),
         }
     }
 
@@ -281,48 +284,42 @@ impl<'a> IterInfo<'a> {
             IterInfo::ComputedIterationRoot {
                 iter_root,
                 base,
-                prefix: _,
-                remainder,
+                prefix,
                 precompose_unicode,
-            } => SortedLoosePaths::at(&iter_root, base.into(), remainder, precompose_unicode),
+            } => SortedLoosePaths::at(&iter_root, base.into(), Some(prefix.into_owned()), precompose_unicode),
         }
         .peekable()
     }
 
-    fn from_prefix(base: &'a Path, prefix: Cow<'a, Path>, precompose_unicode: bool) -> std::io::Result<Self> {
-        if prefix.is_absolute() {
+    fn from_prefix(
+        base: &'a Path,
+        prefix: impl Into<Cow<'a, BStr>>,
+        precompose_unicode: bool,
+    ) -> std::io::Result<Self> {
+        let prefix = prefix.into();
+        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,
-                "prefix must be a relative path, like 'refs/heads'",
+                "prefix must be a relative path, like 'refs/heads/'",
             ));
         }
         use std::path::Component::*;
-        if prefix.components().any(|c| matches!(c, CurDir | ParentDir)) {
+        if prefix_path.components().any(|c| matches!(c, CurDir | ParentDir)) {
             return Err(std::io::Error::new(
                 std::io::ErrorKind::InvalidInput,
                 "Refusing to handle prefixes with relative path components",
             ));
         }
-        let iter_root = base.join(prefix.as_ref());
-        if iter_root.is_dir() {
+        let iter_root = base.join(&prefix_path);
+        if prefix.ends_with(b"/") {
             Ok(IterInfo::BaseAndIterRoot {
                 base,
                 iter_root,
-                prefix,
+                prefix: prefix_path.into_owned(),
                 precompose_unicode,
             })
         } else {
-            let filename_prefix = iter_root
-                .file_name()
-                .map(ToOwned::to_owned)
-                .map(|p| {
-                    gix_path::try_into_bstr(PathBuf::from(p))
-                        .map(std::borrow::Cow::into_owned)
-                        .map_err(|_| {
-                            std::io::Error::new(std::io::ErrorKind::InvalidInput, "prefix contains ill-formed UTF-8")
-                        })
-                })
-                .transpose()?;
             let iter_root = iter_root
                 .parent()
                 .expect("a parent is always there unless empty")
@@ -331,7 +328,6 @@ impl<'a> IterInfo<'a> {
                 base,
                 prefix,
                 iter_root,
-                remainder: filename_prefix,
                 precompose_unicode,
             })
         }
@@ -374,30 +370,35 @@ impl file::Store {
         }
     }
 
-    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads".
+    /// 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`.
     ///
-    /// Please note that "refs/heads" or "refs\\heads" is equivalent to "refs/heads/"
-    pub fn iter_prefixed_packed<'s, 'p>(
+    /// 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: &Path,
+        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.into(), 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.into(), self.precompose_unicode))
+                    .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 git_dir_info =
-                    IterInfo::from_prefix(self.git_dir(), prefix.clone().into(), self.precompose_unicode)?;
+                let git_dir_info = IterInfo::from_prefix(self.git_dir(), prefix.clone(), self.precompose_unicode)?;
                 let common_dir_info = self
                     .common_dir()
-                    .map(|base| IterInfo::from_prefix(base, prefix.into(), self.precompose_unicode))
+                    .map(|base| IterInfo::from_prefix(base, prefix, self.precompose_unicode))
                     .transpose()?;
                 self.iter_from_info(git_dir_info, common_dir_info, packed)
             }
@@ -416,7 +417,7 @@ impl file::Store {
             iter_packed: match packed {
                 Some(packed) => Some(
                     match git_dir_info.prefix() {
-                        Some(prefix) => packed.iter_prefixed(path_to_name(prefix).into_owned()),
+                        Some(prefix) => packed.iter_prefixed(prefix.into_owned()),
                         None => packed.iter(),
                     }
                     .map_err(|err| std::io::Error::new(std::io::ErrorKind::Other, err))?

gix-ref/tests/fixtures/generated-archives/make_packed_ref_repository.tar

@@ -10,9 +10,12 @@ git branch d1
 git branch A
 
 mkdir -p .git/refs/remotes/origin
+mkdir -p .git/refs/prefix/feature/sub/dir
 
 cp .git/refs/heads/main .git/refs/remotes/origin/
 cp .git/refs/heads/main .git/refs/d1
+cp .git/refs/heads/main .git/refs/prefix/feature-suffix
+cp .git/refs/heads/main .git/refs/prefix/feature/sub/dir/algo
 
 echo "ref: refs/remotes/origin/main" > .git/refs/remotes/origin/HEAD
 echo "notahexsha" > .git/refs/broken

gix-ref/tests/fixtures/generated-archives/make_packed_ref_repository_for_overlay.tar

@@ -10,8 +10,11 @@ git branch A
 git tag -m "tag object" tag-object
 
 mkdir -p .git/refs/remotes/origin
+mkdir -p .git/refs/prefix/feature/sub/dir
 
 cp .git/refs/heads/main .git/refs/remotes/origin/
+cp .git/refs/heads/main .git/refs/prefix/feature-suffix
+cp .git/refs/heads/main .git/refs/prefix/feature/sub/dir/algo
 
 echo "ref: refs/remotes/origin/main" > .git/refs/remotes/origin/HEAD
 

gix-ref/tests/fixtures/generated-archives/make_ref_repository.tar

@@ -10,9 +10,12 @@ git branch d1
 git branch A
 
 mkdir -p .git/refs/remotes/origin
+mkdir -p .git/refs/prefix/feature/sub/dir
 
 cp .git/refs/heads/main .git/refs/remotes/origin/
 cp .git/refs/heads/main .git/refs/d1
+cp .git/refs/heads/main .git/refs/prefix/feature-suffix
+cp .git/refs/heads/main .git/refs/prefix/feature/sub/dir/algo
 
 echo "ref: refs/remotes/origin/main" > .git/refs/remotes/origin/HEAD
 echo "notahexsha" > .git/refs/broken
@@ -22,7 +25,6 @@ echo "ref: refs/tags/multi-link-target2" > .git/refs/heads/multi-link-target1
 echo "ref: refs/remotes/origin/multi-link-target3" > .git/refs/tags/multi-link-target2
 git rev-parse HEAD > .git/refs/remotes/origin/multi-link-target3
 
-
 echo "ref: refs/loop-b" > .git/refs/loop-a
 echo "ref: refs/loop-a" > .git/refs/loop-b