feat!: more type-safety for remote names by making clear they can be named after URLs.

`reference::remote::Name` was moved to `remote::Name` to represent all remote names without
necessarily having been validated.

Note that we also don't validate names anymore if read from configuration, but use it as is.
This means that remotes can be read and written back even with invalid names.

Author: Byron

git-repository/src/clone/fetch/mod.rs

@@ -1,3 +1,4 @@
+use crate::bstr::BString;
 use crate::{clone::PrepareFetch, Repository};
 
 /// The error returned by [`PrepareFetch::fetch_only()`].
@@ -58,13 +59,13 @@ impl PrepareFetch {
             .as_mut()
             .expect("user error: multiple calls are allowed only until it succeeds");
 
-        let remote_name = match self.remote_name.as_deref() {
+        let remote_name = match self.remote_name.as_ref() {
             Some(name) => name.to_owned(),
             None => repo
                 .config
                 .resolved
-                .string("clone", None, "defaultRemoteName")
-                .map(|n| crate::remote::name::validated(n.to_string()))
+                .string_by_key("clone.defaultRemoteName")
+                .map(|n| crate::remote::name::validated(n.into_owned()))
                 .unwrap_or_else(|| Ok("origin".into()))?,
         };
 
@@ -114,7 +115,7 @@ impl PrepareFetch {
             repo,
             &outcome.ref_map.remote_refs,
             reflog_message.as_ref(),
-            &remote_name,
+            remote_name.as_ref(),
         )?;
 
         Ok((self.repo.take().expect("still present"), outcome))
@@ -161,7 +162,7 @@ impl PrepareFetch {
     /// [`configure_remote()`][Self::configure_remote()].
     ///
     /// If not set here, it defaults to `origin` or the value of `clone.defaultRemoteName`.
-    pub fn with_remote_name(mut self, name: impl Into<String>) -> Result<Self, crate::remote::name::Error> {
+    pub fn with_remote_name(mut self, name: impl Into<BString>) -> Result<Self, crate::remote::name::Error> {
         self.remote_name = Some(crate::remote::name::validated(name)?);
         Ok(self)
     }

git-repository/src/clone/fetch/util.rs

@@ -7,14 +7,15 @@ use git_ref::{
 };
 
 use super::Error;
+use crate::bstr::BString;
 use crate::{
     bstr::{BStr, ByteSlice},
     Repository,
 };
 
 pub fn write_remote_to_local_config_file(
     remote: &mut crate::Remote<'_>,
-    remote_name: String,
+    remote_name: BString,
 ) -> Result<git_config::File<'static>, Error> {
     let mut metadata = git_config::file::Metadata::from(git_config::Source::Local);
     let config_path = remote.repo.git_dir().join("config");
@@ -50,7 +51,7 @@ pub fn update_head(
     repo: &mut Repository,
     remote_refs: &[git_protocol::handshake::Ref],
     reflog_message: &BStr,
-    remote_name: &str,
+    remote_name: &BStr,
 ) -> Result<(), Error> {
     use git_ref::{
         transaction::{PreviousValue, RefEdit},
@@ -171,7 +172,7 @@ fn setup_branch_config(
     repo: &mut Repository,
     branch: &FullNameRef,
     branch_id: Option<&git_hash::oid>,
-    remote_name: &str,
+    remote_name: &BStr,
 ) -> Result<(), Error> {
     let short_name = match branch.category_and_short_name() {
         Some((cat, shortened)) if cat == git_ref::Category::LocalBranch => match shortened.to_str() {

git-repository/src/clone/mod.rs

@@ -1,3 +1,4 @@
+use crate::bstr::BString;
 use std::convert::TryInto;
 
 type ConfigureRemoteFn = Box<dyn FnMut(crate::Remote<'_>) -> Result<crate::Remote<'_>, crate::remote::init::Error>>;
@@ -9,7 +10,7 @@ pub struct PrepareFetch {
     /// A freshly initialized repository which is owned by us, or `None` if it was handed to the user
     repo: Option<crate::Repository>,
     /// The name of the remote, which defaults to `origin` if not overridden.
-    remote_name: Option<String>,
+    remote_name: Option<BString>,
     /// A function to configure a remote prior to fetching a pack.
     configure_remote: Option<ConfigureRemoteFn>,
     /// Options for preparing a fetch operation.

git-repository/src/config/cache/access.rs

@@ -2,6 +2,7 @@ use std::{borrow::Cow, convert::TryInto, path::PathBuf, time::Duration};
 
 use git_lock::acquire::Fail;
 
+use crate::bstr::BStr;
 use crate::{
     config::{cache::util::ApplyLeniencyDefault, checkout_options, Cache},
     remote,
@@ -121,7 +122,7 @@ impl Cache {
     pub(crate) fn trusted_file_path(
         &self,
         section_name: impl AsRef<str>,
-        subsection_name: Option<&str>,
+        subsection_name: Option<&BStr>,
         key: impl AsRef<str>,
     ) -> Option<Result<Cow<'_, std::path::Path>, git_config::path::interpolate::Error>> {
         let path = self.resolved.path_filter(

git-repository/src/config/snapshot/access.rs

@@ -19,12 +19,12 @@ impl<'repo> Snapshot<'repo> {
     /// For a non-degenerating version, use [`try_boolean(…)`][Self::try_boolean()].
     ///
     /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust.
-    pub fn boolean(&self, key: &str) -> Option<bool> {
+    pub fn boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option<bool> {
         self.try_boolean(key).and_then(Result::ok)
     }
 
     /// Like [`boolean()`][Self::boolean()], but it will report an error if the value couldn't be interpreted as boolean.
-    pub fn try_boolean(&self, key: &str) -> Option<Result<bool, git_config::value::Error>> {
+    pub fn try_boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option<Result<bool, git_config::value::Error>> {
         let key = git_config::parse::key(key)?;
         self.repo
             .config
@@ -38,12 +38,12 @@ impl<'repo> Snapshot<'repo> {
     /// For a non-degenerating version, use [`try_integer(…)`][Self::try_integer()].
     ///
     /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust.
-    pub fn integer(&self, key: &str) -> Option<i64> {
+    pub fn integer<'a>(&self, key: impl Into<&'a BStr>) -> Option<i64> {
         self.try_integer(key).and_then(Result::ok)
     }
 
     /// Like [`integer()`][Self::integer()], but it will report an error if the value couldn't be interpreted as boolean.
-    pub fn try_integer(&self, key: &str) -> Option<Result<i64, git_config::value::Error>> {
+    pub fn try_integer<'a>(&self, key: impl Into<&'a BStr>) -> Option<Result<i64, git_config::value::Error>> {
         let key = git_config::parse::key(key)?;
         self.repo
             .config
@@ -54,7 +54,7 @@ impl<'repo> Snapshot<'repo> {
     /// Return the string at `key`, or `None` if there is no such value.
     ///
     /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust.
-    pub fn string(&self, key: &str) -> Option<Cow<'_, BStr>> {
+    pub fn string<'a>(&self, key: impl Into<&'a BStr>) -> Option<Cow<'_, BStr>> {
         let key = git_config::parse::key(key)?;
         self.repo
             .config
@@ -65,9 +65,9 @@ impl<'repo> Snapshot<'repo> {
     /// Return the trusted and fully interpolated path at `key`, or `None` if there is no such value
     /// or if no value was found in a trusted file.
     /// An error occurs if the path could not be interpolated to its final value.
-    pub fn trusted_path(
+    pub fn trusted_path<'a>(
         &self,
-        key: &str,
+        key: impl Into<&'a BStr>,
     ) -> Option<Result<Cow<'_, std::path::Path>, git_config::path::interpolate::Error>> {
         let key = git_config::parse::key(key)?;
         self.repo

git-repository/src/reference/remote/mod.rs renamed to git-repository/src/reference/remote.rs

@@ -1,20 +1,6 @@
-use std::{borrow::Cow, convert::TryInto};
+use std::convert::TryInto;
 
-use crate::{
-    bstr::{BStr, ByteSlice},
-    remote, Reference,
-};
-
-/// The name of a remote, either interpreted as symbol like `origin` or as url as returned by [`Reference::remote_name()`].
-#[derive(Debug, PartialEq, Eq, Clone)]
-pub enum Name<'repo> {
-    /// A symbolic name, like `origin`
-    Symbol(Cow<'repo, str>),
-    /// A url pointing to the remote host directly.
-    Url(Cow<'repo, BStr>),
-}
-
-mod name;
+use crate::{remote, Reference};
 
 /// Remotes
 impl<'repo> Reference<'repo> {
@@ -29,8 +15,8 @@ impl<'repo> Reference<'repo> {
     /// - it's recommended to use the [`remote(…)`][Self::remote()] method as it will configure the remote with additional
     ///   information.
     /// - `branch.<name>.pushRemote` falls back to `branch.<name>.remote`.
-    pub fn remote_name(&self, direction: remote::Direction) -> Option<Name<'repo>> {
-        let name = self.name().shorten().to_str().ok()?;
+    pub fn remote_name(&self, direction: remote::Direction) -> Option<remote::Name<'repo>> {
+        let name = self.name().shorten();
         let config = &self.repo.config.resolved;
         (direction == remote::Direction::Push)
             .then(|| {
@@ -54,8 +40,8 @@ impl<'repo> Reference<'repo> {
     ) -> Option<Result<crate::Remote<'repo>, remote::find::existing::Error>> {
         // TODO: use `branch.<name>.merge`
         self.remote_name(direction).map(|name| match name {
-            Name::Symbol(name) => self.repo.find_remote(name.as_ref()).map_err(Into::into),
-            Name::Url(url) => git_url::parse(url.as_ref()).map_err(Into::into).and_then(|url| {
+            remote::Name::Symbol(name) => self.repo.find_remote(name.as_ref()).map_err(Into::into),
+            remote::Name::Url(url) => git_url::parse(url.as_ref()).map_err(Into::into).and_then(|url| {
                 self.repo
                     .remote_at(url)
                     .map_err(|err| remote::find::existing::Error::Find(remote::find::Error::Init(err)))

git-repository/src/remote/access.rs

@@ -5,12 +5,8 @@ use crate::{bstr::BStr, remote, Remote};
 /// Access
 impl<'repo> Remote<'repo> {
     /// Return the name of this remote or `None` if it wasn't persisted to disk yet.
-    // TODO: name can also be a URL but we don't see it like this. There is a problem with accessing such names
-    //       too as they would require a BStr, but valid subsection names are strings, so some degeneration must happen
-    //       for access at least. Argh. Probably use `reference::remote::Name` and turn it into `remote::Name` as it's
-    //       actually correct.
-    pub fn name(&self) -> Option<&str> {
-        self.name.as_deref()
+    pub fn name(&self) -> Option<&remote::Name<'static>> {
+        self.name.as_ref()
     }
 
     /// Return our repository reference.

git-repository/src/remote/errors.rs

@@ -26,6 +26,8 @@ pub mod find {
 
     ///
     pub mod existing {
+        use crate::bstr::BString;
+
         /// The error returned by [`Repository::find_remote(…)`][crate::Repository::find_remote()].
         #[derive(Debug, thiserror::Error)]
         #[allow(missing_docs)]
@@ -35,7 +37,7 @@ pub mod find {
             #[error("remote name could not be parsed as URL")]
             UrlParse(#[from] git_url::parse::Error),
             #[error("The remote named {name:?} did not exist")]
-            NotFound { name: String },
+            NotFound { name: BString },
         }
     }
 }

git-repository/src/remote/init.rs

@@ -11,8 +11,6 @@ mod error {
     #[derive(Debug, thiserror::Error)]
     #[allow(missing_docs)]
     pub enum Error {
-        #[error(transparent)]
-        Name(#[from] crate::remote::name::Error),
         #[error(transparent)]
         Url(#[from] git_url::parse::Error),
         #[error("The rewritten {kind} url {rewritten_url:?} failed to parse")]
@@ -23,12 +21,13 @@ mod error {
         },
     }
 }
+use crate::bstr::BString;
 pub use error::Error;
 
 /// Initialization
 impl<'repo> Remote<'repo> {
     pub(crate) fn from_preparsed_config(
-        name: Option<String>,
+        name_or_url: Option<BString>,
         url: Option<git_url::Url>,
         push_url: Option<git_url::Url>,
         fetch_specs: Vec<RefSpec>,
@@ -44,7 +43,7 @@ impl<'repo> Remote<'repo> {
             .then(|| rewrite_urls(&repo.config, url.as_ref(), push_url.as_ref()))
             .unwrap_or(Ok((None, None)))?;
         Ok(Remote {
-            name: name.map(remote::name::validated).transpose()?,
+            name: name_or_url.map(Into::into),
             url,
             url_alias,
             push_url,

git-repository/src/remote/mod.rs

@@ -1,3 +1,6 @@
+use crate::bstr::BStr;
+use std::borrow::Cow;
+
 /// The direction of an operation carried out (or to be carried out) through a remote.
 #[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)]
 pub enum Direction {
@@ -17,35 +20,24 @@ 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)]
+pub enum Name<'repo> {
+    /// A symbolic name, like `origin`.
+    /// Note that it has not necessarily been validated yet.
+    Symbol(Cow<'repo, str>),
+    /// A url pointing to the remote host directly.
+    Url(Cow<'repo, BStr>),
+}
+
+///
+pub mod name;
+
 mod build;
 
 mod errors;
 pub use errors::find;
 
-///
-pub mod name {
-    /// The error returned by [validated()].
-    #[derive(Debug, thiserror::Error)]
-    #[error("remote names must be valid within refspecs for fetching: {name:?}")]
-    #[allow(missing_docs)]
-    pub struct Error {
-        source: git_refspec::parse::Error,
-        name: String,
-    }
-
-    /// Return `name` if it is valid or convert it into an `Error`.
-    pub fn validated(name: impl Into<String>) -> Result<String, Error> {
-        let name = name.into();
-        match git_refspec::parse(
-            format!("refs/heads/test:refs/remotes/{name}/test").as_str().into(),
-            git_refspec::parse::Operation::Fetch,
-        ) {
-            Ok(_) => Ok(name),
-            Err(err) => Err(Error { source: err, name }),
-        }
-    }
-}
-
 ///
 pub mod init;
 

git-repository/src/reference/remote/name.rs renamed to git-repository/src/remote/name.rs

@@ -1,7 +1,28 @@
 use std::{borrow::Cow, convert::TryFrom};
 
 use super::Name;
-use crate::bstr::{BStr, ByteSlice, ByteVec};
+use crate::bstr::{BStr, BString, ByteSlice, ByteVec};
+
+/// The error returned by [validated()].
+#[derive(Debug, thiserror::Error)]
+#[error("remote names must be valid within refspecs for fetching: {name:?}")]
+#[allow(missing_docs)]
+pub struct Error {
+    source: git_refspec::parse::Error,
+    name: BString,
+}
+
+/// Return `name` if it is valid or convert it into an `Error`.
+pub fn validated(name: impl Into<BString>) -> Result<BString, Error> {
+    let name = name.into();
+    match git_refspec::parse(
+        format!("refs/heads/test:refs/remotes/{name}/test").as_str().into(),
+        git_refspec::parse::Operation::Fetch,
+    ) {
+        Ok(_) => Ok(name),
+        Err(err) => Err(Error { source: err, name }),
+    }
+}
 
 impl Name<'_> {
     /// Obtain the name as string representation.
@@ -48,6 +69,12 @@ impl<'a> TryFrom<Cow<'a, BStr>> for Name<'a> {
     }
 }
 
+impl From<BString> for Name<'static> {
+    fn from(name: BString) -> Self {
+        Self::try_from(Cow::Owned(name)).expect("String is never illformed")
+    }
+}
+
 impl<'a> AsRef<BStr> for Name<'a> {
     fn as_ref(&self) -> &BStr {
         self.as_bstr()