From 84d594caf3f04f1ce337e455343278a6f4674957 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 09:28:42 +0100 Subject: [PATCH] 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. --- git-repository/src/clone/fetch/mod.rs | 11 ++--- git-repository/src/clone/fetch/util.rs | 7 ++-- git-repository/src/clone/mod.rs | 3 +- git-repository/src/config/cache/access.rs | 3 +- git-repository/src/config/snapshot/access.rs | 14 +++---- .../reference/{remote/mod.rs => remote.rs} | 26 +++--------- git-repository/src/remote/access.rs | 8 +--- git-repository/src/remote/errors.rs | 4 +- git-repository/src/remote/init.rs | 7 ++-- git-repository/src/remote/mod.rs | 40 ++++++++----------- .../src/{reference => }/remote/name.rs | 29 +++++++++++++- git-repository/src/remote/save.rs | 9 +++-- git-repository/src/repository/config/mod.rs | 16 +++++--- git-repository/src/repository/remote.rs | 40 ++++++++++++------- git-repository/src/types.rs | 4 +- git-repository/tests/clone/mod.rs | 8 ++-- git-repository/tests/reference/remote.rs | 6 ++- git-repository/tests/remote/save.rs | 8 ++-- .../repository/config/config_snapshot/mod.rs | 4 +- git-repository/tests/repository/remote.rs | 5 ++- 20 files changed, 140 insertions(+), 112 deletions(-) rename git-repository/src/reference/{remote/mod.rs => remote.rs} (73%) rename git-repository/src/{reference => }/remote/name.rs (62%) diff --git a/git-repository/src/clone/fetch/mod.rs b/git-repository/src/clone/fetch/mod.rs index f5ac611572..f02b425fda 100644 --- a/git-repository/src/clone/fetch/mod.rs +++ b/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) -> Result { + pub fn with_remote_name(mut self, name: impl Into) -> Result { self.remote_name = Some(crate::remote::name::validated(name)?); Ok(self) } diff --git a/git-repository/src/clone/fetch/util.rs b/git-repository/src/clone/fetch/util.rs index 9685c277cf..6f763041b3 100644 --- a/git-repository/src/clone/fetch/util.rs +++ b/git-repository/src/clone/fetch/util.rs @@ -7,6 +7,7 @@ use git_ref::{ }; use super::Error; +use crate::bstr::BString; use crate::{ bstr::{BStr, ByteSlice}, Repository, @@ -14,7 +15,7 @@ use crate::{ pub fn write_remote_to_local_config_file( remote: &mut crate::Remote<'_>, - remote_name: String, + remote_name: BString, ) -> Result, 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() { diff --git a/git-repository/src/clone/mod.rs b/git-repository/src/clone/mod.rs index b3f7b8701e..1591ea7b6a 100644 --- a/git-repository/src/clone/mod.rs +++ b/git-repository/src/clone/mod.rs @@ -1,3 +1,4 @@ +use crate::bstr::BString; use std::convert::TryInto; type ConfigureRemoteFn = Box) -> Result, 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, /// The name of the remote, which defaults to `origin` if not overridden. - remote_name: Option, + remote_name: Option, /// A function to configure a remote prior to fetching a pack. configure_remote: Option, /// Options for preparing a fetch operation. diff --git a/git-repository/src/config/cache/access.rs b/git-repository/src/config/cache/access.rs index 81fa50165c..dce0168d95 100644 --- a/git-repository/src/config/cache/access.rs +++ b/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, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option, git_config::path::interpolate::Error>> { let path = self.resolved.path_filter( diff --git a/git-repository/src/config/snapshot/access.rs b/git-repository/src/config/snapshot/access.rs index 3f4e268b6d..fa66ba7725 100644 --- a/git-repository/src/config/snapshot/access.rs +++ b/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 { + pub fn boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option { 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> { + pub fn try_boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option> { 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 { + pub fn integer<'a>(&self, key: impl Into<&'a BStr>) -> Option { 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> { + pub fn try_integer<'a>(&self, key: impl Into<&'a BStr>) -> Option> { 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> { + pub fn string<'a>(&self, key: impl Into<&'a BStr>) -> Option> { 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, git_config::path::interpolate::Error>> { let key = git_config::parse::key(key)?; self.repo diff --git a/git-repository/src/reference/remote/mod.rs b/git-repository/src/reference/remote.rs similarity index 73% rename from git-repository/src/reference/remote/mod.rs rename to git-repository/src/reference/remote.rs index b0c72c86ae..83da9f7fc1 100644 --- a/git-repository/src/reference/remote/mod.rs +++ b/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..pushRemote` falls back to `branch..remote`. - pub fn remote_name(&self, direction: remote::Direction) -> Option> { - let name = self.name().shorten().to_str().ok()?; + pub fn remote_name(&self, direction: remote::Direction) -> Option> { + 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, remote::find::existing::Error>> { // TODO: use `branch..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))) diff --git a/git-repository/src/remote/access.rs b/git-repository/src/remote/access.rs index 534cd505a3..c18f7ea4a8 100644 --- a/git-repository/src/remote/access.rs +++ b/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. diff --git a/git-repository/src/remote/errors.rs b/git-repository/src/remote/errors.rs index d6a29963fe..b73e12d030 100644 --- a/git-repository/src/remote/errors.rs +++ b/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 }, } } } diff --git a/git-repository/src/remote/init.rs b/git-repository/src/remote/init.rs index bb29280c7a..64d5fd97ff 100644 --- a/git-repository/src/remote/init.rs +++ b/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, + name_or_url: Option, url: Option, push_url: Option, fetch_specs: Vec, @@ -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, diff --git a/git-repository/src/remote/mod.rs b/git-repository/src/remote/mod.rs index b83874858c..9aae7dce4d 100644 --- a/git-repository/src/remote/mod.rs +++ b/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) -> Result { - 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; diff --git a/git-repository/src/reference/remote/name.rs b/git-repository/src/remote/name.rs similarity index 62% rename from git-repository/src/reference/remote/name.rs rename to git-repository/src/remote/name.rs index 1137006bba..7a178e0c31 100644 --- a/git-repository/src/reference/remote/name.rs +++ b/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) -> Result { + 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> for Name<'a> { } } +impl From for Name<'static> { + fn from(name: BString) -> Self { + Self::try_from(Cow::Owned(name)).expect("String is never illformed") + } +} + impl<'a> AsRef for Name<'a> { fn as_ref(&self) -> &BStr { self.as_bstr() diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index d6c0e885d0..ebc01cb86f 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -1,3 +1,4 @@ +use crate::bstr::BString; use std::convert::TryInto; use crate::Remote; @@ -39,7 +40,7 @@ impl Remote<'_> { .to_owned(), })?; if let Some(section_ids) = config.sections_and_ids_by_name("remote").map(|it| { - it.filter_map(|(s, id)| (s.header().subsection_name() == Some(name.into())).then(|| id)) + it.filter_map(|(s, id)| (s.header().subsection_name() == Some(name.as_bstr())).then(|| id)) .collect::>() }) { let mut sections_to_remove = Vec::new(); @@ -62,7 +63,7 @@ impl Remote<'_> { } } let mut section = config - .section_mut_or_create_new("remote", Some(name)) + .section_mut_or_create_new("remote", Some(name.as_ref())) .expect("section name is validated and 'remote' is acceptable"); if let Some(url) = self.url.as_ref() { section.push("url".try_into().expect("valid"), Some(url.to_bstring().as_ref())) @@ -91,12 +92,12 @@ impl Remote<'_> { /// and the caller should account for that. pub fn save_as_to( &mut self, - name: impl Into, + name: impl Into, config: &mut git_config::File<'static>, ) -> Result<(), AsError> { let name = crate::remote::name::validated(name)?; let prev_name = self.name.take(); - self.name = name.into(); + self.name = Some(name.into()); self.save_to(config).map_err(|err| { self.name = prev_name; err.into() diff --git a/git-repository/src/repository/config/mod.rs b/git-repository/src/repository/config/mod.rs index 8ce0df414b..bb887254c4 100644 --- a/git-repository/src/repository/config/mod.rs +++ b/git-repository/src/repository/config/mod.rs @@ -44,6 +44,7 @@ mod 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") } @@ -56,6 +57,7 @@ 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> { let name = (direction == remote::Direction::Push) .then(|| { @@ -83,6 +85,7 @@ mod remote { mod branch { use std::{borrow::Cow, collections::BTreeSet, convert::TryInto}; + use crate::bstr::BStr; use git_ref::FullNameRef; use git_validate::reference::name::Error as ValidateNameError; @@ -99,13 +102,13 @@ mod branch { /// The returned reference is the one we track on the remote side for merging and pushing. /// Returns `None` if the remote reference was not found. /// May return an error if the reference is invalid. - pub fn branch_remote_ref( + pub fn branch_remote_ref<'a>( &self, - short_branch_name: &str, + short_branch_name: impl Into<&'a BStr>, ) -> Option, ValidateNameError>> { self.config .resolved - .string("branch", Some(short_branch_name), "merge") + .string("branch", Some(short_branch_name.into()), "merge") .map(|v| match v { Cow::Borrowed(v) => v.try_into().map(Cow::Borrowed), Cow::Owned(v) => v.try_into().map(Cow::Owned), @@ -119,10 +122,13 @@ mod branch { /// /// See also [Reference::remote_name()][crate::Reference::remote_name()] for a more typesafe version /// to be used when a `Reference` is available. - pub fn branch_remote_name(&self, short_branch_name: &str) -> Option> { + pub fn branch_remote_name<'a>( + &self, + short_branch_name: impl Into<&'a BStr>, + ) -> Option> { self.config .resolved - .string("branch", Some(short_branch_name), "remote") + .string("branch", Some(short_branch_name.into()), "remote") .and_then(|name| name.try_into().ok()) } } diff --git a/git-repository/src/repository/remote.rs b/git-repository/src/repository/remote.rs index c23fbb10b6..0a838ceab3 100644 --- a/git-repository/src/repository/remote.rs +++ b/git-repository/src/repository/remote.rs @@ -1,5 +1,6 @@ use std::convert::TryInto; +use crate::bstr::BStr; use crate::{remote, remote::find, Remote}; impl crate::Repository { @@ -23,13 +24,16 @@ impl crate::Repository { Remote::from_fetch_url(url, false, self) } - /// Find the remote with the given `name` or report an error, similar to [`try_find_remote(…)`][Self::try_find_remote()]. + /// Find the remote with the given `name_or_url` or report an error, similar to [`try_find_remote(…)`][Self::try_find_remote()]. /// - /// Note that we will include remotes only if we deem them [trustworthy][crate::open::Options::filter_config_section()]. - pub fn find_remote(&self, name: &str) -> Result, find::existing::Error> { + /// Note that we will obtain remotes only if we deem them [trustworthy][crate::open::Options::filter_config_section()]. + pub fn find_remote<'a>(&self, name_or_url: impl Into<&'a BStr>) -> Result, find::existing::Error> { + let name_or_url = name_or_url.into(); Ok(self - .try_find_remote(name) - .ok_or_else(|| find::existing::Error::NotFound { name: name.into() })??) + .try_find_remote(name_or_url) + .ok_or_else(|| find::existing::Error::NotFound { + name: name_or_url.into(), + })??) } /// Find the default remote as configured, or `None` if no such configuration could be found. @@ -43,7 +47,7 @@ impl crate::Repository { .map(|name| self.find_remote(name.as_ref())) } - /// Find the remote with the given `name` or return `None` if it doesn't exist, for the purpose of fetching or pushing + /// Find the remote with the given `name_or_url` or return `None` if it doesn't exist, for the purpose of fetching or pushing /// data to a remote. /// /// There are various error kinds related to partial information or incorrectly formatted URLs or ref-specs. @@ -53,23 +57,31 @@ impl crate::Repository { /// as negations/excludes are applied after includes. /// /// We will only include information if we deem it [trustworthy][crate::open::Options::filter_config_section()]. - pub fn try_find_remote(&self, name: &str) -> Option, find::Error>> { - self.try_find_remote_inner(name, true) + pub fn try_find_remote<'a>(&self, name_or_url: impl Into<&'a BStr>) -> Option, find::Error>> { + self.try_find_remote_inner(name_or_url, true) } /// Similar to [try_find_remote()][Self::try_find_remote()], but removes a failure mode if rewritten URLs turn out to be invalid /// as it skips rewriting them. /// Use this in conjunction with [`Remote::rewrite_urls()`] to non-destructively apply the rules and keep the failed urls unchanged. - pub fn try_find_remote_without_url_rewrite(&self, name: &str) -> Option, find::Error>> { - self.try_find_remote_inner(name, false) + pub fn try_find_remote_without_url_rewrite<'a>( + &self, + name_or_url: impl Into<&'a BStr>, + ) -> Option, find::Error>> { + self.try_find_remote_inner(name_or_url, false) } - fn try_find_remote_inner(&self, name: &str, rewrite_urls: bool) -> Option, find::Error>> { + fn try_find_remote_inner<'a>( + &self, + name_or_url: impl Into<&'a BStr>, + rewrite_urls: bool, + ) -> Option, find::Error>> { let mut filter = self.filter_config_section(); + let name_or_url = name_or_url.into(); let mut config_url = |field: &str, kind: &'static str| { self.config .resolved - .string_filter("remote", name.into(), field, &mut filter) + .string_filter("remote", Some(name_or_url), field, &mut filter) .map(|url| { git_url::parse::parse(url.as_ref()).map_err(|err| find::Error::Url { kind, @@ -88,7 +100,7 @@ impl crate::Repository { }; self.config .resolved - .strings_filter("remote", name.into(), kind, &mut filter) + .strings_filter("remote", Some(name_or_url), kind, &mut filter) .map(|specs| { specs .into_iter() @@ -139,7 +151,7 @@ impl crate::Repository { Some( Remote::from_preparsed_config( - name.to_owned().into(), + Some(name_or_url.to_owned()), url, push_url, fetch_specs, diff --git a/git-repository/src/types.rs b/git-repository/src/types.rs index a7c3ae701f..16b658659c 100644 --- a/git-repository/src/types.rs +++ b/git-repository/src/types.rs @@ -2,7 +2,7 @@ use std::{cell::RefCell, path::PathBuf}; use git_hash::ObjectId; -use crate::head; +use crate::{head, remote}; /// The kind of repository. #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] @@ -181,7 +181,7 @@ pub struct ThreadSafeRepository { #[derive(Debug, Clone, PartialEq)] pub struct Remote<'repo> { /// The remotes symbolic name, only present if persisted in git configuration files. - pub(crate) name: Option, + pub(crate) name: Option>, /// The url of the host to talk to, after application of replacements. If it is unset, the `push_url` must be set. /// and fetches aren't possible. pub(crate) url: Option, diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index cb67921940..51ce0253f3 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -141,16 +141,14 @@ mod blocking_io { "local clone always adopts the name of the remote" ); - let short_name = referent.name().shorten().to_str_lossy(); + let short_name = referent.name().shorten(); assert_eq!( - repo.branch_remote_name(short_name.as_ref()) - .expect("remote is set") - .as_ref(), + repo.branch_remote_name(short_name).expect("remote is set").as_ref(), remote_name, "the remote branch information is fully configured" ); assert_eq!( - repo.branch_remote_ref(short_name.as_ref()).expect("present")?.as_bstr(), + repo.branch_remote_ref(short_name).expect("present")?.as_bstr(), "refs/heads/main" ); diff --git a/git-repository/tests/reference/remote.rs b/git-repository/tests/reference/remote.rs index fc2fea7a9a..7fe0e15137 100644 --- a/git-repository/tests/reference/remote.rs +++ b/git-repository/tests/reference/remote.rs @@ -19,14 +19,16 @@ fn push_defaults_to_fetch() -> crate::Result { .remote(git::remote::Direction::Push) .expect("configured")? .name() - .expect("set"), + .expect("set") + .as_bstr(), "origin" ); assert_eq!( head.into_remote(git::remote::Direction::Push) .expect("same with branch")? .name() - .expect("set"), + .expect("set") + .as_bstr(), "origin" ); Ok(()) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 78c885ce1d..e5d224785c 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -23,7 +23,7 @@ mod save_to { let previous_remote_state = repo .config_snapshot() .plumbing() - .section("remote", Some("origin")) + .section_by_key("remote.origin") .expect("present") .to_bstring(); let mut config = repo.config_snapshot().plumbing().clone(); @@ -34,7 +34,7 @@ mod save_to { "amount of remotes are unaltered" ); assert_eq!( - config.section("remote", Some("origin")).expect("present").to_bstring(), + config.section_by_key("remote.origin").expect("present").to_bstring(), previous_remote_state, "the serialization doesn't modify anything" ); @@ -96,11 +96,11 @@ mod save_as_to { new_section.push("a".try_into().unwrap(), Some("value".into())); config - .section_mut_or_create_new("initially-empty-not-removed", Some("name")) + .section_mut_or_create_new("initially-empty-not-removed", Some("name".into())) .expect("works"); let mut existing_section = config - .section_mut_or_create_new("remote", Some("origin")) + .section_mut_or_create_new("remote", Some("origin".into())) .expect("works"); existing_section.push("free".try_into().unwrap(), Some("should not be removed".into())) } diff --git a/git-repository/tests/repository/config/config_snapshot/mod.rs b/git-repository/tests/repository/config/config_snapshot/mod.rs index 354f3a6ad9..20884cd481 100644 --- a/git-repository/tests/repository/config/config_snapshot/mod.rs +++ b/git-repository/tests/repository/config/config_snapshot/mod.rs @@ -58,7 +58,9 @@ fn values_are_set_in_memory_only() { { let mut config = repo.config_snapshot_mut(); config.set_raw_value("hallo", None, "welt", "true").unwrap(); - config.set_raw_value("hallo", Some("unter"), "welt", "value").unwrap(); + config + .set_raw_value("hallo", Some("unter".into()), "welt", "value") + .unwrap(); } assert_eq!( diff --git a/git-repository/tests/repository/remote.rs b/git-repository/tests/repository/remote.rs index 4028a0b8e9..6e060f0d92 100644 --- a/git-repository/tests/repository/remote.rs +++ b/git-repository/tests/repository/remote.rs @@ -121,7 +121,7 @@ mod find_remote { for (name, (url, refspec)) in repo.remote_names().into_iter().zip(expected) { count += 1; let remote = repo.find_remote(name).expect("no error"); - assert_eq!(remote.name(), Some(name)); + assert_eq!(remote.name().expect("set").as_bstr(), name); let url = git::url::parse(url.into()).expect("valid"); assert_eq!(remote.url(Direction::Fetch).unwrap(), &url); @@ -285,7 +285,8 @@ mod find_default_remote { .transpose()? .expect("present") .name() - .expect("always named"), + .expect("always named") + .as_bstr(), "origin" ); Ok(())