feat!: Repository::remote_names|remote_default_name() now returns Cow<'_, BStr> instead of Cow<'_, str>.

Byron · Byron · commit 5c07c760e392 · 2023-12-18T18:25:12.000+01:00
That way information won't degenerate due to enforcement of UTF-8.
diff --git a/gix/src/remote/mod.rs b/gix/src/remote/mod.rs
@@ -22,7 +22,7 @@ impl Direction {
 }
 
 /// The name of a remote, either interpreted as symbol like `origin` or as url as returned by [`Remote::name()`][crate::Remote::name()].
-#[derive(Debug, PartialEq, Eq, Clone)]
+#[derive(Debug, PartialEq, Eq, Clone, Ord, PartialOrd, Hash)]
 pub enum Name<'repo> {
     /// A symbolic name, like `origin`.
     /// Note that it has not necessarily been validated yet.
diff --git a/gix/src/repository/config/mod.rs b/gix/src/repository/config/mod.rs
@@ -135,16 +135,25 @@ impl crate::Repository {
 mod transport;
 
 mod remote {
+    use crate::bstr::BStr;
     use std::{borrow::Cow, collections::BTreeSet};
 
-    use crate::{bstr::ByteSlice, remote};
+    use crate::remote;
 
     impl crate::Repository {
         /// Returns a sorted list unique of symbolic names of remotes that
         /// we deem [trustworthy][crate::open::Options::filter_config_section()].
-        // TODO: Use `remote::Name` here
-        pub fn remote_names(&self) -> BTreeSet<&str> {
-            self.subsection_names_of("remote")
+        pub fn remote_names(&self) -> BTreeSet<Cow<'_, BStr>> {
+            self.config
+                .resolved
+                .sections_by_name("remote")
+                .map(|it| {
+                    let filter = self.filter_config_section();
+                    it.filter(move |s| filter(s.meta()))
+                        .filter_map(|section| section.header().subsection_name().map(Cow::Borrowed))
+                        .collect()
+                })
+                .unwrap_or_default()
         }
 
         /// Obtain the branch-independent name for a remote for use in the given `direction`, or `None` if it could not be determined.
@@ -155,25 +164,23 @@ mod remote {
         /// # Notes
         ///
         /// It's up to the caller to determine what to do if the current `head` is unborn or detached.
-        // TODO: use remote::Name here
-        pub fn remote_default_name(&self, direction: remote::Direction) -> Option<Cow<'_, str>> {
+        pub fn remote_default_name(&self, direction: remote::Direction) -> Option<Cow<'_, BStr>> {
             let name = (direction == remote::Direction::Push)
                 .then(|| {
                     self.config
                         .resolved
                         .string_filter("remote", None, "pushDefault", &mut self.filter_config_section())
-                        .and_then(|s| match s {
-                            Cow::Borrowed(s) => s.to_str().ok().map(Cow::Borrowed),
-                            Cow::Owned(s) => s.to_str().ok().map(|s| Cow::Owned(s.into())),
-                        })
                 })
                 .flatten();
             name.or_else(|| {
                 let names = self.remote_names();
                 match names.len() {
                     0 => None,
-                    1 => names.iter().next().copied().map(Cow::Borrowed),
-                    _more_than_one => names.get("origin").copied().map(Cow::Borrowed),
+                    1 => names.into_iter().next(),
+                    _more_than_one => {
+                        let origin = Cow::Borrowed("origin".into());
+                        names.contains(&origin).then_some(origin)
+                    }
                 }
             })
         }
@@ -191,8 +198,12 @@ mod branch {
     impl crate::Repository {
         /// Return a set of unique short branch names for which custom configuration exists in the configuration,
         /// if we deem them [trustworthy][crate::open::Options::filter_config_section()].
+        ///
+        /// ### Note
+        ///
+        /// Branch names that have illformed UTF-8 will silently be skipped.
         pub fn branch_names(&self) -> BTreeSet<&str> {
-            self.subsection_names_of("branch")
+            self.subsection_str_names_of("branch")
         }
 
         /// Returns the validated reference on the remote associated with the given `short_branch_name`,
@@ -237,7 +248,7 @@ impl crate::Repository {
             .unwrap_or(config::section::is_trusted)
     }
 
-    fn subsection_names_of<'a>(&'a self, header_name: &'a str) -> BTreeSet<&'a str> {
+    fn subsection_str_names_of<'a>(&'a self, header_name: &'a str) -> BTreeSet<&'a str> {
         self.config
             .resolved
             .sections_by_name(header_name)
diff --git a/gix/tests/remote/mod.rs b/gix/tests/remote/mod.rs
@@ -1,4 +1,4 @@
-use std::{borrow::Cow, path::PathBuf};
+use std::path::PathBuf;
 
 use gix_testtools::scripted_fixture_read_only;
 
@@ -64,10 +64,6 @@ pub(crate) fn into_daemon_remote_if_async<'repo, 'a>(
     }
 }
 
-pub(crate) fn cow_str(s: &str) -> Cow<str> {
-    Cow::Borrowed(s)
-}
-
 mod connect;
 pub(crate) mod fetch;
 mod ref_map;
diff --git a/gix/tests/repository/config/remote.rs b/gix/tests/repository/config/remote.rs
@@ -1,6 +1,16 @@
+use gix::bstr::BStr;
+use std::borrow::Cow;
 use std::iter::FromIterator;
 
-use crate::{named_repo, remote, remote::cow_str, Result};
+use crate::{named_repo, remote, Result};
+
+fn remote_names<'a>(it: impl IntoIterator<Item = &'a str>) -> Vec<Cow<'a, BStr>> {
+    it.into_iter().map(|n| Cow::Borrowed(n.into())).collect()
+}
+
+fn remote_name(name: &str) -> Cow<'_, BStr> {
+    Cow::Borrowed(name.into())
+}
 
 #[test]
 fn remote_and_branch_names() {
@@ -13,15 +23,15 @@ fn remote_and_branch_names() {
     let repo = remote::repo("clone");
     assert_eq!(
         Vec::from_iter(repo.remote_names().into_iter()),
-        vec!["myself", "origin"]
+        remote_names(["myself", "origin"])
     );
     assert_eq!(
         repo.remote_default_name(gix::remote::Direction::Fetch),
-        Some(cow_str("origin"))
+        Some(remote_name("origin"))
     );
     assert_eq!(
         repo.remote_default_name(gix::remote::Direction::Push),
-        Some(cow_str("origin"))
+        Some(remote_name("origin"))
     );
     assert_eq!(Vec::from_iter(repo.branch_names()), vec!["main"]);
 }
@@ -32,7 +42,7 @@ fn remote_default_name() {
 
     assert_eq!(
         repo.remote_default_name(gix::remote::Direction::Push),
-        Some(cow_str("myself")),
+        Some(remote_name("myself")),
         "overridden via remote.pushDefault"
     );
 
diff --git a/gix/tests/repository/remote.rs b/gix/tests/repository/remote.rs
@@ -133,7 +133,7 @@ mod find_remote {
         ];
         for (name, (url, refspec)) in repo.remote_names().into_iter().zip(expected) {
             count += 1;
-            let remote = repo.find_remote(name)?;
+            let remote = repo.find_remote(name.as_ref())?;
             assert_eq!(remote.name().expect("set").as_bstr(), name);
 
             assert_eq!(

Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@ impl Direction {`
`22`	`22`	`}`
`23`	`23`
`24`	`24`	/// The name of a remote, either interpreted as symbol like `origin` or as url as returned by [`Remote::name()`][crate::Remote::name()].
`25`		`-#[derive(Debug, PartialEq, Eq, Clone)]`
	`25`	`+#[derive(Debug, PartialEq, Eq, Clone, Ord, PartialOrd, Hash)]`
`26`	`26`	`pub enum Name<'repo> {`
`27`	`27`	/// A symbolic name, like `origin`.
`28`	`28`	`/// Note that it has not necessarily been validated yet.`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-use std::{borrow::Cow, path::PathBuf};`
	`1`	`+use std::path::PathBuf;`
`2`	`2`
`3`	`3`	`use gix_testtools::scripted_fixture_read_only;`
`4`	`4`
`@@ -64,10 +64,6 @@ pub(crate) fn into_daemon_remote_if_async<'repo, 'a>(`
`64`	`64`	`}`
`65`	`65`	`}`
`66`	`66`
`67`		`-pub(crate) fn cow_str(s: &str) -> Cow<str> {`
`68`		`- Cow::Borrowed(s)`
`69`		`-}`
`70`		`-`
`71`	`67`	`mod connect;`
`72`	`68`	`pub(crate) mod fetch;`
`73`	`69`	`mod ref_map;`