Handle trailing slash in ref list prefix filtering

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.

Fixes #1934.

Author: moroten

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".as_bstr().into())?
                         .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::{borrow::Cow, path::Path};
 
 use gix_object::bstr::{BStr, BString, ByteSlice, ByteVec};
 
@@ -18,10 +18,9 @@ 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()
+    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)
     }
     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

@@ -1,22 +1,26 @@
-use std::path::{Path, PathBuf};
+use std::{
+    borrow::Cow,
+    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>,
+    /// A optional prefix to match against if the prefix is not the same as the `file_walk` path.
+    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 +45,21 @@ 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");
+                        .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?
                     };
-
+                    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)));
@@ -93,7 +87,7 @@ 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<'_, '_>> {
+    pub fn loose_iter_prefixed(&self, prefix: Cow<'_, BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {
         self.iter_prefixed_packed(prefix, 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,10 +195,9 @@ 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".
-    ///
-    /// Please note that "refs/heads" or "refs\\heads" is equivalent to "refs/heads/"
-    pub fn prefixed(&self, prefix: &Path) -> std::io::Result<LooseThenPacked<'_, '_>> {
+    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads/" or
+    /// "refs/heads/feature-".
+    pub fn prefixed(&self, prefix: Cow<'_, BStr>) -> std::io::Result<LooseThenPacked<'_, '_>> {
         self.store
             .iter_prefixed_packed(prefix, self.packed.as_ref().map(|b| &***b))
     }
@@ -242,22 +241,19 @@ pub(crate) enum IterInfo<'a> {
         /// 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>,
+        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 +277,37 @@ 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: Cow<'a, BStr>, precompose_unicode: bool) -> std::io::Result<Self> {
+        let prefix_path = gix_path::from_bstring(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(),
                 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 +316,6 @@ impl<'a> IterInfo<'a> {
                 base,
                 prefix,
                 iter_root,
-                remainder: filename_prefix,
                 precompose_unicode,
             })
         }
@@ -374,30 +358,28 @@ impl file::Store {
         }
     }
 
-    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads".
-    ///
-    /// Please note that "refs/heads" or "refs\\heads" is equivalent to "refs/heads/"
+    /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads/" or
+    /// "refs/heads/feature-".
     pub fn iter_prefixed_packed<'s, 'p>(
         &'s self,
-        prefix: &Path,
+        prefix: Cow<'_, BStr>,
         packed: Option<&'p packed::Buffer>,
     ) -> std::io::Result<LooseThenPacked<'p, 's>> {
         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.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)
             }
             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 prefix = namespace.to_owned().into_namespaced_prefix(prefix.as_ref());
+                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 +398,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_for_overlay.tar

@@ -10,10 +10,13 @@ git branch A
 git tag -m "tag object" tag-object
 
 mkdir -p .git/refs/remotes/origin
+mkdir -p .git/refs/heads/sub/dir
 
 cp .git/refs/heads/main .git/refs/remotes/origin/
 
 echo "ref: refs/remotes/origin/main" > .git/refs/remotes/origin/HEAD
+echo "ref: refs/heads/main" > .git/refs/heads-packed
+echo "ref: refs/heads/main" > .git/refs/heads/sub/dir/packed
 
 git pack-refs --all --prune
 

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

@@ -22,6 +22,11 @@ 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
 
+# Regression test for issue #1934 where prefix refs/m matched refs/heads/main.
+mkdir -p .git/refs/heads/sub/dir
+echo "ref: refs/remotes/origin/main" > .git/refs/heads-loose
+echo "ref: refs/remotes/origin/main" > .git/refs/heads/sub/dir/loose
+echo "ref: refs/remotes/origin/main" > .git/refs/remotes/origin/heads
 
 echo "ref: refs/loop-b" > .git/refs/loop-a
 echo "ref: refs/loop-a" > .git/refs/loop-b